Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app: project: Allow to diff against manifest revision #719

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

pdgendt
Copy link
Collaborator

@pdgendt pdgendt commented Jul 19, 2024

Add an argument to the west diff command that allows to set the manifest revision as the merge-base.

pdgendt added 2 commits July 19, 2024 15:55
Add an argument to the west diff command that allows to set the
manifest revision as the merge-base.

Signed-off-by: Pieter De Gendt <[email protected]>
Run the diff command with --manifest argument.

Signed-off-by: Pieter De Gendt <[email protected]>
@carlescufi carlescufi merged commit 6aaf248 into zephyrproject-rtos:main Aug 27, 2024
9 checks passed
@pdgendt pdgendt deleted the git-diff-manifest branch August 27, 2024 11:31
@marc-hb
Copy link
Collaborator

marc-hb commented Aug 27, 2024

While I'm truly in awe of @pdgendt 's west productivity, I have concerns this one was a bit rushed... why --merge-base? git diff is useful both with and without --merge-base but:

  • The --help text never mentions --merge-base.
  • Why is the choice not given?
  • While merge-base is simple in... simple cases, it can be very complicated in general: you can have 0 merge based, or more than one. What happens then? As west is a general purpose git tool, this should be at the very least documented.

See for instance codecov/codecov-bash#83

@pdgendt
Copy link
Collaborator Author

pdgendt commented Aug 28, 2024

Right, I've created a follow-up PR that simplifies it: #728

@pdgendt pdgendt added this to the v1.3.0 milestone Sep 13, 2024
@@ -792,9 +794,10 @@ def do_run(self, args, ignored):
for project in self._cloned_projects(args, only_active=not args.all):
# Use paths that are relative to the base directory to make it
# easier to see where the changes are
merge_base = ['--merge-base', project.revision] if args.manifest else []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

project.revision is not OK: it can be a remote branch name and then git diff will fail. Or something else and who knows what might happen.

I just submitted a replacement of project.revision by manifest-rev in #748

Longer story in #747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants