Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Junio C Hamano <gitster@pobox.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> The purpose of this action is for scripts to be able to keep the
> user's Signed-off-by at the end.  For example say I have a script
> that adds a Reviewed-by tag:
>
>     #! /bin/sh
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" | \
>       git commit --amend -F-
>
> Now, this script will leave my Signed-off-by line in a non-canonical
> place, like
>
>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>    Reviewed-by: Junio C Hamano <gitster@pobox.com>
>
> This new option enables the following improvement:
>
>     #! /bin/sh
>     me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     sob="Signed-off-by: $me"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
>                              --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
>       git commit --amend -F-
>
> which lets me keep the SoB line at the end, as it should be.
> Posting as RFC because it's possible that I'm missing a simpler
> way to achieve this...

While I think "move" may turn out to be handy in some use case, an
example to move S-o-b does not sound convincing to me at all.  

If anything, the above (assuming that you wrote a patch, sent out
for a review with or without signing it off, and then after getting
a review, you are adding reviewed-by to the commit) does not
demonstrate the need for "move".  The use of "move" in the example
looks like a mere workaround that reviewed-by was added at the wrong
place (i.e. --where end) in the first place.

But that is not the primary reason why I find the example using
S-o-b convincing.  If the patch in your example originally did not
have just one S-o-b by you, but yours was at the end of the chain of
patch passing, use of "move" may become even more problematic.  Your
friend may write an original, sign it off and pass it to you, who
then signs it off and sends to the mailng list.  It gets picked up
by somebody else, who tweaks and adds her sign off, then you pick it
up and relay it to the final destination (i.e. the first sign-off is
by your friend, then you have two sign-offs of yours, one sign off
from somebody else in between, and the chain records how the patch
"flowed").  And then Linus says "yeah, this is good, I throughly
reviewed it."  Where would you place that reviewed-by?  Before your
second (and last) sign-off?  What makes that last one special?
Would it more faithfully reflect the order of events if you added
Linus's reviewed-by and then your own sign-off to conclude the
chain?

So I am not opposed to the idea of "move", but I am not sure in what
situation it is useful and what use case it makes it easier to use.
The example makes me suspect that what we want is not a new
operation, but a way to specify "where" in a richer way.