Re: [PATCH/RFC] range-diff: make output format more useful for "rebase --onto"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Sep 18 2021, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Sep 17 2021, Junio C Hamano wrote:
>
>> In a range-diff output, we often see an early part of an updated
>> series having no changes since the previous iteration.  After
>> applying an updated patch submission to the same base as the
>> previous round,
>>
>>     $ git range-diff master..topic@{1} master..topic
>>
>> to view the differences since the previous edition, we might see
>> something like this:
>>
>>     1:  9a05f02b1d = 1:  a05f02b1d9 t/helper/test-bitmap.c: add  ...
>>     2:  78de300e1f = 2:  8de300e1f7 pack-bitmap.c: propagate nam ...
>>     3:  7caca3c9f0 = 3:  caca3c9f07 midx.c: respect 'pack.writeB ...
>>     4:  72082224f1 = 4:  2082224f17 p5326: create missing 'perf- ...
>>     5:  097b89c815 = 5:  97b89c8150 p5326: don't set core.multiP ...
>>     6:  a1dd4c97b9 < -:  ---------- p5326: generate pack bitmaps ...
>>     -:  ---------- > 6:  bf4a60874a p5326: generate pack bitmaps ...
>>     7:  2b909ebad3 ! 7:  54156af0d6 t5326: test propagating hash ...
>> 	@@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'pack.preferBitmapTips' '
>> 	 +	(
>> 	 +		cd repo &&
>>     ...
>>
>> Now, after noticing that up to step #5 there is no change since the
>> previous round, cleaning up the application result with
>>
>>     $ git rebase --onto 097b89c815 97b89c8150
>>
>> will help making it easier to see that earliser part did not change
>> before committing this in the longer term history.
>>
>> The output format of the range-diff unfortunately makes it a bit
>> cumbersome than necessary to come up with the rebase command line.
>> Because "= 5:" gets in the way, copying the two object names from
>> there and pasting them as the command line arguments to "git rebase
>> --onto" becomes a chore.
>>
>> Tweak the output so that the change numbers and comparison sign come
>> first on the line, followed by two object names and then the title
>> of commit, to make it easier to copy the two object names together.
>>
>> The updated output format looks like this instead:
>>
>>     1 = 1 a05f02b1d9 a05f02b1d9 : t/helper/test-bitmap.c: add  ...
>>     2 = 2 8de300e1f7 8de300e1f7 : pack-bitmap.c: propagate nam ...
>>     3 = 3 caca3c9f07 caca3c9f07 : midx.c: respect 'pack.writeB ...
>>     4 = 4 2082224f17 2082224f17 : p5326: create missing 'perf- ...
>>     5 = 5 97b89c8150 97b89c8150 : p5326: don't set core.multiP ...
>>     6 < - a1dd4c97b9 ---------- : p5326: generate pack bitmaps ...
>>     - > 6 ---------- bf4a60874a : p5326: generate pack bitmaps ...
>>     7 ! 7 2b909ebad3 54156af0d6 : t5326: test propagating hash ...
>> 	@@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'pack.preferBitmapTips' '
>> 	 +	(
>> 	 +		cd repo &&
>>
>> Incidentally, it becomes easier to see the correspondence and spot
>> the reordering of the commits with this change, for the same reason
>> why it becomes easier to see the two commit object names---they sit
>> close to each other with their peers.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>
>>  * Obviously an RFC.  The output format would be fairly subjective,
>>    but I do not care deeply enough to make it configurable.  If
>>    enough people appreciate the convenience of seeing the two object
>>    names and two change numbers next to each other like I do, and
>>    nobody processes the current output with tools, then it may be OK
>>    to take this patch as the final version, but otherwise, this is
>>    only meant as an inspiration piece for somebody else to have a
>>    mechanism to allow the output configurable in some way (which I
>>    am not intereseted in doing myself).
>
> I think we carved out a sufficient exception in df569c3f31f (range-diff
> doc: add a section about output stability, 2018-11-09) to just change
> the output.
>
> I do happen to have a one-liner as part of my build process that relies
> on the current output, but I've only got myself to blame. Aside from the
> change being proposed here I think we can just change it in general if
> we come up with better output.
>
> As for the proposed output, I'm a bit negative on it, so first, if we're
> trying to note that two commits are the same wouldn't it be much better
> to just omit the second SHA-1? I.e. consider this variation of your
> proposed output;
>
>      1 = 1 a05f02b1d9 a05102bfd9 : t/helper/test-bitmap.c: add  ...
>
> You might eyeball that for a while before discovering that I switched
> the "f" and "1" around, i.e. the SHA-1s look /almost/ the same. Isn't
> this better? (or we could use "++++++++++" to not overlead any meaning
> "----------" has):
>
>      1 = 1 a05f02b1d9 ---------- : t/helper/test-bitmap.c: add  ...
>      2 = 2 8de300e1f7 ---------- : pack-bitmap.c: propagate nam ...
>      3 = 3 caca3c9f07 ---------- : midx.c: respect 'pack.writeB ...
>      4 = 4 2082224f17 ---------- : p5326: create missing 'perf- ...
>      5 = 5 97b89c8150 ---------- : p5326: don't set core.multiP ...
>      6 < - a1dd4c97b9 ---------- : p5326: generate pack bitmaps ...
>      - > 6 ---------- bf4a60874a : p5326: generate pack bitmaps ...
>      7 ! 7 2b909ebad3 54156af0d6 : t5326: test propagating hash ...
>
> (Or whatever other syntax would follow from the shorthand of the "1 =
> 1". Having it repeated in the human-readable output just for the one
> use-case of passing it to rebase --onto doesn't seem worth it.
>
> But then if we get support for say --ignore-matching-lines (which I've
> wanted to for a while to omit the Signed-off-by lines you add) we'll
> note the same SHA1 if you used that option, but a different one in the
> same slot if you omit it (i.e. we'd start showing that part of the
> diff).
>
> Then combine that with --left-only or --right-only and we'd ignore the
> diff depending on what "side" it was, and then either duplicate the
> SHA-1 or not.


As a follow-up, this proposal really mixes two things, the appearance of
the text format & its semantics. I.e. it's changing (1)

    1:  9a05f02b1d = 1:  a05f02b1d9 t/helper/test-bitmap.c: add  ...

To (2)

    1 = 1 a05f02b1d9 a05f02b1d9 : t/helper/test-bitmap.c: add  ...

Not just (3)

    1 = 1 9a05f02b1d a05f02b1d9 : t/helper/test-bitmap.c: add  ...

I happen to like (3) a lot better to format the existing information, it
emits the "1 = 1" so you can see at a glance what maps to what, but I'm
not sure about (2) as a semantic change..