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

[Junio C Hamano]

Paolo Bonzini  writes:

> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (, ) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different ".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?  It's not hard to implement, but I'm wondering if it's
> too ad hoc.

Yeah, it makes me wonder exactly that, too.

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

[Paolo Bonzini]

On 06/10/2017 15:19, Christian Couder wrote:
> Now it would be strange to have "moveIfClosest" without having "move"
> first and I don't see how "move" would be different from the existing
> "replace".
> Or maybe "move" means "replaceIfIdentical", in this case I think it
> would help users to just call it "replaceIfIdentical".

Well, the effect of "replacing if identical" *is* to move the existing
identical trailer to the new position. :)

Paolo

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

[Christian Couder]

On Fri, Oct 6, 2017 at 2:39 PM, Paolo Bonzini  wrote:
> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (, ) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different ".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?

First yeah these would not make sense anyway if where=after or where=before.

Now it would be strange to have "moveIfClosest" without having "move"
first and I don't see how "move" would be different from the existing
"replace".
Or maybe "move" means "replaceIfIdentical", in this case I think it
would help users to just call it "replaceIfIdentical".

Also there is "addIfDifferentNeighbor" so we already have "Neighbor"
which means "just above or below". Then if we use "Closest" I think it
will be harder to distinguish it from "Neighbor" than if we use
"Close".

That's why I think "replaceIfIdenticalClose" is better. It could
enable us to eventually use a regexp like
"(add|replace)(If(Different|Identical)(Close|Neighbor)+)+"  to parse
the add* and replace* options.

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

[Paolo Bonzini]

On 06/10/2017 14:33, Christian Couder wrote:
> Ok. I think you might want something called for example
> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
> trailer with the same (, ) pair above or below the line
> where the replaced trailer will be put when ignoring trailers with a
> different ".

So basically "moveIfClosest" (move if last for where=end, move if first
for where=begin; for where=after and where=before it would just end up
doing nothing)?  It's not hard to implement, but I'm wondering if it's
too ad hoc.

Paolo

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

[Christian Couder]

On Fri, Oct 6, 2017 at 12:40 PM, Paolo Bonzini  wrote:
> On 06/10/2017 12:30, Christian Couder wrote:

>> Did you try using `--where end --if-exists replace --trailer "$sob"`?
>
> Yes, it's a different behavior; "--if-exists replace" matches on others'
> SoB as well, so it would eat the original author's SoB if I didn't have one.
>
> So "move" does get it wrong for
>
> Signed-off-by: Me
> Signed-off-by: Friend
>
> (Me gets moved last, which may not be what you want) but "replace" gets
> it wrong in the arguably more common case of
>
> Signed-off-by: Friend
>
> which is damaged to just "Signed-off-by: Me".

Ok. I think you might want something called for example
"replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
trailer with the same (, ) pair above or below the line
where the replaced trailer will be put when ignoring trailers with a
different ".

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

[Paolo Bonzini]

On 06/10/2017 12:30, Christian Couder wrote:
> On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini  wrote:
>> 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 
>>Reviewed-by: Junio C Hamano 
>>
>> 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...
> 
> Did you try using `--where end --if-exists replace --trailer "$sob"`?

Yes, it's a different behavior; "--if-exists replace" matches on others'
SoB as well, so it would eat the original author's SoB if I didn't have one.

So "move" does get it wrong for

Signed-off-by: Me
Signed-off-by: Friend

(Me gets moved last, which may not be what you want) but "replace" gets
it wrong in the arguably more common case of

Signed-off-by: Friend

which is damaged to just "Signed-off-by: Me".

Paolo

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

[Christian Couder]

On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini  wrote:
> 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 
>Reviewed-by: Junio C Hamano 
>
> 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...

Did you try using `--where end --if-exists replace --trailer "$sob"`?

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

[Paolo Bonzini]

On 06/10/2017 08:44, Junio C Hamano wrote:
> Paolo Bonzini  writes:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.
>>
>> #! /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...
>
> 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.

Yes, I agree.  Though I also tried implementing "--where beforeLastSOB", 
and in the end decided against it because there were more corner cases.
I include the patch for reference at the end of this message, to be
applied on top of these four.

> 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?

So:

   Signed-off-by: Me
   Signed-off-by: Friend
   Signed-off-by: Me   <<<
   Reviewed-by: Linus
   Signed-off-by: Me

I do think the SoB line marked with "<<<" is a bit "special".  SoB lines 
before it represent the path followed by the contribution, according to 
clause (c) of the Developer Certificate of Origin.  Multiple 
*consecutive* SoB lines from the same person do not add much, while 
multiple separate SoB lines from the same person must be kept.

Of course, using "--where start" for Reviewed/Tested/Acked-by lines *is* 
an option.  On the other hand, for "Cc: sta...@vger.kernel.org" the 
placement hints at *who* decided the patch to be worth of inclusion in a 
stable version.  That person might be the right one to bug if the patch 
doesn't apply and needs a manual backport.  It's not science of course, 
but in practice I found the "always apply with -s, and use 'move' to 
keep my SoB at the end" workflow to be the least error-prone.

> 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?

Possibly, but the DCO doesn't care and SoB lines are first and firemost 
about the DCO.

On the other hand, "move" does not provide exactly what we want in the 
case where the user's SoB is there, but is not the last.  So, the above 
script pretty much assumes that you apply the patch with "-s"; if you 
didn't, you'd need something more like "moveLast".  It is trivial to 
implement "moveLast" on top of the first three patches in this series, 
but things start getting a bit out of hand perhaps...

Paolo

> 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.

--- 8< 
>From a238b973821586eaaf26d608172cdc72f19d6071 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Wed, 12 Jul 2017 18:11:44 +0200
Subject: [PATCH] trailer: add "beforeLastSOB" configuration for trailer.where

In some cases, people apply patches to a queue branch immediately
with "git am -3 -s", and later collect Reviewed-by or Acked-by
trailers as they come in from the mailing list.

In this case, "where=after" does not have the desired behavior,
because it will add the trailer in an unorthodox position, after the
committer's Signed-off-by line.  Introduce a "beforeLastSOB" value
for trailer.where; this of course makes the most sense if the
last Signed-off-by is from the committer 

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

[Junio C Hamano]

Paolo Bonzini  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 
>Reviewed-by: Junio C Hamano 
>
> 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.

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

[Paolo Bonzini]

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 
   Reviewed-by: Junio C Hamano 

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...

Paolo Bonzini (4):
  trailer: push free_arg_item up
  trailer: simplify check_if_different
  trailer: create a new function to handle adding trailers
  trailer: add "move" configuration for trailer.ifExists

 Documentation/git-interpret-trailers.txt |  13 ++-
 t/t7513-interpret-trailers.sh|  37 +++
 trailer.c| 169 ++-
 trailer.h|   1 +
 4 files changed, 149 insertions(+), 71 deletions(-)

-- 
2.14.2