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

compare: always prefix git status output with manifest-rev #672

Merged

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jun 23, 2023

It's not unusual to make "quick and dirty", temporary changes in a git
repo that is on the manifest: either add some untracked files or create
a branch. As expected, this makes the repository show in west compare.
If such repos and the manifest repo are the only repos
appearing in the west compare output, then no manifest-rev + HEAD
banner ever appeared. When no banner ever appears it's really not
obvious which repos are on the manifest-rev vs not. In other words: the
main west compare feature is defeated.

Clear this doubt by always showing the banner, even when HEAD and
manifest-rev are the same. See sample output below.

I think the original design idea was to follow diff's "spirit" not to
show anything that's identical and to save as many lines as
possible. However I don't think it works in this particular case because
invoking git status for a repo without showing where its HEAD is at
is way too subtle, especially when there is no other repo with a
banner to provide a comparison point and some contrast. Explicitly and
consistently prefixing every git status output with a manifest-rev
banner is much more clear and obvious.

Moreover, git status output is relatively verbose already so always
prefixing it with a 2 lines long banner makes negligible difference to
the total.

Before:

$ west compare

=== hal_xtensa (modules/hal/xtensa):
--- status:
    On branch somebranch
    nothing to commit, working tree clean

After:

$ west compare

=== hal_xtensa (modules/hal/xtensa):
--- manifest-rev: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
            HEAD: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
--- status:
    On branch somebranch
    nothing to commit, working tree clean

Signed-off-by: Marc Herbert marc.herbert@intel.com

@marc-hb marc-hb marked this pull request as ready for review June 23, 2023 18:54
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jul 12, 2023

@aescolar , @carlescufi, any opinion?

It's a very small change.

@aescolar
Copy link
Member

any opinion?

@marc-hb . I'm not incredibly keen on this change, but neither would I oppose it strongly if others want it.
I tend to recall not having a header when the project is on the manifets-rev commit is what we had discussed originally. I just lean towards less verbose output.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jul 19, 2023

Thanks @aescolar for the feedback! Would you mind trying it locally for some time and evaluate the verbosity trade-off "in real-life"? It's practically a one-line change.

The more I've been using this the more I like it.

@marc-hb marc-hb requested review from mbolivar-ampere and removed request for mbolivar-nordic August 28, 2023 12:24
Copy link
Collaborator

@mbolivar-ampere mbolivar-ampere left a comment

Choose a reason for hiding this comment

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

I have to say I don't like the unconditional print of head_info on line 733's partial redundancy with the git status output.

What about adding command line and configuration options to enable always printing this? Barring that, what about printing the manifest-rev info unconditionally on the banner line?

Just trying to explore alternatives that don't result in redundancy here.

@marc-hb

This comment was marked as resolved.

@marc-hb

This comment was marked as resolved.

@mbolivar-ampere
Copy link
Collaborator

Oh, I see! You meant printing the HEAD SHA1 twice. I think I get you know, let me think about it.

Yes, exactly

@marc-hb
Copy link
Collaborator Author

marc-hb commented Aug 29, 2023

You meant printing the HEAD SHA1 twice.

Actually, that duplication was only in a particular case. I changed the example and now the is duplication gone, see the new commit message.

Now I could try to test whether the repo has a detached HEAD in order to eliminate the duplication in that removed, detached HEAD example. But:

  1. It would make the code much more complicated (whereas this PR makes it a bit simpler)
  2. It would make the output even more variable when this PR makes it more predictable.

Let me insist on point 2, which is the main point of this PR: simple and predictable output. Unsurprisingly, it goes with simpler code.

Throughout the discussions about this new compare feature I sensed a little bit of an obsession to minimize the length of the output (e.g., #648). Of course the output should be as short as possible. But not shorter: if the elimination of absolutely every possible bit of redundant information eventually requires the reader to think in order to answer the main (!) question (= is this repo on the manifest?), then clearly the axe went too far.

It's not unusual to make "quick and dirty", temporary changes in a git
repo that is on the manifest: either add some untracked files or create
a branch. As expected, this makes the repository show in `west
compare`. If such repos and the manifest repo are the only repos
appearing in the `west compare` output, then no manifest-rev + HEAD
banner ever appeared. When no banner ever appears it's really not
obvious which repos are on the manifest-rev vs not. In other words: the
main `west compare` feature is defeated.

Clear this doubt by always showing the banner, even when `HEAD` and
`manifest-rev` are the same. See sample output below.

I think the original design idea was to follow diff's "spirit" not to
show anything that's identical and to save as many lines as
possible. However I don't think it works in this particular case because
invoking `git status` for a repo _without_ showing where its HEAD is at
is way too subtle, _especially_ when there is no other repo with a
banner to provide a comparison point and some contrast. Explicitly and
consistently prefixing every `git status` output with a manifest-rev
banner is much more clear and obvious.

Moreover, `git status` output is relatively verbose already so always
prefixing it with a 2 lines long banner makes negligible difference to
the total.

Before:
```
$ west compare

=== hal_xtensa (modules/hal/xtensa):
--- status:
    On branch somebranch
    nothing to commit, working tree clean
```

After:
```
$ west compare

=== hal_xtensa (modules/hal/xtensa):
--- manifest-rev: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
            HEAD: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
--- status:
    On branch somebranch
    nothing to commit, working tree clean
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@mbolivar-ampere mbolivar-ampere merged commit 98800b3 into zephyrproject-rtos:main Sep 1, 2023
9 checks passed
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.

3 participants