[RFC,v2,0/2] suppress trailing whitespace on empty "notes" lines

Message

Ævar Arnfjörð Bjarmason Aug. 30, 2021, 10:47 a.m. UTC

This is a review of Eric Sunshin's
<[e-mail]> series.

Side note:

    I'm generally trying to see if just sending a "proposed vX" is
    more productive for everyone than patch feedback effectively
    describing it in prose. I don't mean for this thing to be picked
    up as-is by Junio without the consent of the submitter, and don't
    have any desire to "pick up" the series myself.

    My review workflow is just applying the patches locally, fiddling
    with them, so it seems like the most straightforward and helpful
    thing to send the result of that local end-state, rather than
    describing the changes I made in prose, and expect the original
    submitter to reverse engineer that state if they're interested in
    trying it out locally themselves.

I really like the end goal of
<[e-mail]> series, but this
seems like a more straightforward way to get to that goal.

I.e. the original 1/3 and 2/3 starts out by making the tests
whitespace-independent. If we just skip that 1/3, and then in 3/3
tweak the relevant failing tests for the code change we won't even
need a new test, all the existing tests previously made
whitespace-independent in 1/3 will assert this new behavior.

Eric Sunshine (2):
  t3303/t9301: make `notes` tests less brittle
  notes: don't indent empty lines

 notes.c                      |  2 +-
 t/t3301-notes.sh             | 28 ++++++++++++++--------------
 t/t3303-notes-subtrees.sh    | 13 ++++++++-----
 t/t9301-fast-import-notes.sh | 36 +++++++++++++++++++-----------------
 4 files changed, 42 insertions(+), 37 deletions(-)

Range-diff against v1:
1:  d2915b20aee < -:  ----------- t3301: tolerate minor notes-related presentation changes
2:  478d8b8d104 ! 1:  5a1ddd30859 t3303/t9301: make `notes` tests less brittle
    @@ Commit message
         desired information in a stable machine-consumable format.
     
         Signed-off-by: Eric Sunshine <[e-mail]>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## t/t3303-notes-subtrees.sh ##
     @@ t/t3303-notes-subtrees.sh: INPUT_END
3:  56d05862a67 < -:  ----------- notes: don't indent empty lines
-:  ----------- > 2:  4b546b83fd7 notes: don't indent empty lines

Comments

Eric Sunshine Aug. 30, 2021, 4:45 p.m. UTC | #1

On Mon, Aug 30, 2021 at 6:47 AM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
> Side note:
>
>     I'm generally trying to see if just sending a "proposed vX" is
>     more productive for everyone than patch feedback effectively
>     describing it in prose. I don't mean for this thing to be picked
>     up as-is by Junio without the consent of the submitter, and don't
>     have any desire to "pick up" the series myself.
>
> I really like the end goal of
> <[e-mail]> series, but this
> seems like a more straightforward way to get to that goal.
>
> I.e. the original 1/3 and 2/3 starts out by making the tests
> whitespace-independent. If we just skip that 1/3, and then in 3/3
> tweak the relevant failing tests for the code change we won't even
> need a new test, all the existing tests previously made
> whitespace-independent in 1/3 will assert this new behavior.

It probably won't surprise you that this fix to `notes` started out as
a single patch which made the change to `notes.c` and adjusted the
existing tests to account for it. In particular, my original changes
to t3301 were exactly the same changes you made (i.e. merely dropping
the empty-line `${indent}` from the few necessary places). I wasn't
happy about the additional complexity I had to add to t3303 and t9301
to continue plucking the notes out of the default git-log output, thus
simplified by making those tests less brittle. That, of course,
deserved its own patch. I wavered quite a bit about whether to make
t3301 less brittle too, or to simply apply the minimal changes which I
had already made (and which you made independently). Eventually, I
decided to split that out as a brittle-fixing patch, as well, to
better future-proof it, but perhaps that's terribly important.

I don't have strong feelings between my v1 and your v2 of this series,
and would be happy to see Junio pick up either version.

Eric Sunshine Aug. 30, 2021, 4:50 p.m. UTC | #2

On Mon, Aug 30, 2021 at 12:45 PM Eric Sunshine <[e-mail]> wrote:
> It probably won't surprise you that this fix to `notes` started out as
> a single patch which made the change to `notes.c` and adjusted the
> existing tests to account for it. In particular, my original changes
> to t3301 were exactly the same changes you made (i.e. merely dropping
> the empty-line `${indent}` from the few necessary places). I wasn't
> happy about the additional complexity I had to add to t3303 and t9301
> to continue plucking the notes out of the default git-log output, thus
> simplified by making those tests less brittle. That, of course,
> deserved its own patch. I wavered quite a bit about whether to make
> t3301 less brittle too, or to simply apply the minimal changes which I
> had already made (and which you made independently). Eventually, I
> decided to split that out as a brittle-fixing patch, as well, to
> better future-proof it, but perhaps that's terribly important.

...that's NOT terribly important.

(last-second editing mistake strikes again)

> I don't have strong feelings between my v1 and your v2 of this series,
> and would be happy to see Junio pick up either version.

Junio C Hamano Aug. 30, 2021, 5:04 p.m. UTC | #3

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> This is a review of Eric Sunshin's
> <[e-mail]> series.
>
> Side note:
>
>     I'm generally trying to see if just sending a "proposed vX" is
>     more productive for everyone than patch feedback effectively
>     describing it in prose. I don't mean for this thing to be picked
>     up as-is by Junio without the consent of the submitter, and don't
>     have any desire to "pick up" the series myself.

It is impossible to read the rationale behind the change between v1
and "proposed v2" in such arrangement, simply because there is no
place in the "proposed v2" patch to write it down.  Proposed commit
log for it should not refer to what "v1" said, proposed postimage of
the patch should not refer to what "v1" did.  Worse, it is harder to
pick good bits from "proposed v2" and reject the others.

It is not a good way to give a feedback on "v1".  It does not help
the original author.

It certainly does not help the maintainer, who now has to read two
competing series, sort out the numbering mess.

I do not know if it helps other reviewers, but offhand I do not know
how it would, compared to the in-line comments on "v1", which is how
we usually do reviews.

>     My review workflow is just applying the patches locally, fiddling
>     with them, so it seems like the most straightforward and helpful
>     thing to send the result of that local end-state, rather than
>     describing the changes I made in prose, and expect the original
>     submitter to reverse engineer that state if they're interested in
>     trying it out locally themselves.

The end-state as an additional reference material attached as the
end note of the review may be helpful, but the in-line comments on
"v1" is a much better way to convey the reason why the change is
suggested, I would think.