zephyrproject-rtos / west

West, Zephyr's meta-tool
https://docs.zephyrproject.org/latest/guides/west/index.html
Apache License 2.0
215 stars 117 forks source link

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

Closed marc-hb closed 10 months ago

marc-hb commented 1 year ago

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 commented 12 months ago

@aescolar , @carlescufi, any opinion?

It's a very small change.

aescolar commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

... partial redundancy with the git status output.

I don't understand how git status can tell me whether HEAD == manifest-rev or not, how could it? Where's the redundancy?

If you look at the "Before" example in the commit message, NOT knowing whether HEAD == manifest-rev is precisely why I submitted this PR.

marc-hb commented 10 months ago

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

mbolivar-ampere commented 10 months ago

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 commented 10 months ago

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.