[PATCH RFC] git-am: support any number of signatures

[PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

I'm using different signature tags for git am depending on the patch,
project and other factors.

Sometimes I add multiple tags as well, e.g. QEMU
wants both Reviewed-by and Signed-off-by tags.

This patch makes it easy to do so:
1.  new parameter am.signoff can be used any number
	of times:

[am]
	signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
	signoff = "Signed-off-by: Michael S. Tsirkin <mst@redhat.com>"

	if set all signatures are picked up when git am -s is used.

2.  Any number of alternative signatures

[am "a"]
	signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"

	if set the signature type can be specified by passing
	a parameter to the -s flag:

	git am -sa

No docs or tests, sorry, so not yet ready for master, but I'm using this
all the time without any issues so maybe ok for pu.
Early flames/feedback/help welcome.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 git-am.sh | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index ee61a77..e992c34 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -13,7 +13,7 @@ i,interactive   run interactively
 b,binary*       (historical option -- no-op)
 3,3way          allow fall back on 3way merging if needed
 q,quiet         be quiet
-s,signoff       add a Signed-off-by line to the commit message
+s,signoff?      add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
 keep-non-patch  pass -b flag to git-mailinfo
@@ -136,7 +136,7 @@ fall_back_3way () {
     eval "$cmd" &&
     GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \
     git write-tree >"$dotest/patch-merge-base+" ||
-    cannot_fallback "$(gettext "Repository lacks necessary blobs to fall back on 3-way merge.")"
+    cannot_fallback "$(gettext "Repository lsignoffs necessary blobs to fall back on 3-way merge.")"
 
     say "$(gettext "Using index info to reconstruct a base tree...")"
 
@@ -383,6 +383,7 @@ then
     keepcr=t
 fi
 
+signoffs=
 while test $# != 0
 do
 	case "$1" in
@@ -394,8 +395,15 @@ it will be removed. Please do not use it anymore."
 		;;
 	-3|--3way)
 		threeway=t ;;
-	-s|--signoff)
-		sign=t ;;
+	--signoff)
+		sign=t
+		s=$(git config --get-all am.signoff)
+		signoffs=("${signoffs[@]}" "${s[@]}") ;;
+	--signoff=*)
+		sign=t
+		a="${1#--signoff=}"
+		s=$(git config --get-all am."${a}".signoff)
+		signoffs=("${signoffs[@]}" "${s[@]}") ;;
 	-u|--utf8)
 		utf8=t ;; # this is now default
 	--no-utf8)
@@ -644,14 +652,25 @@ fi
 git_apply_opt=$(cat "$dotest/apply-opt")
 if test "$(cat "$dotest/sign")" = t
 then
-	SIGNOFF=$(git var GIT_COMMITTER_IDENT | sed -e '
-			s/>.*/>/
-			s/^/Signed-off-by: /'
-		)
+	for ack in "${signoffs[@]}"; do
+		if test "$SIGNOFF"
+		then
+			SIGNOFF="$SIGNOFF\n$ack"
+		else
+			SIGNOFF="$ack"
+		fi
+	done
+	if test -z "$SIGNOFF"
+	then
+		SIGNOFF=$(git var GIT_COMMITTER_IDENT | sed -e '
+				s/>.*/>/
+				s/^/Signed-off-by: /'
+			)
+	fi
 else
 	SIGNOFF=
 fi
 
 last=$(cat "$dotest/last")
 this=$(cat "$dotest/next")
 if test "$skip" = t
-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> I'm using different signature tags for git am depending on the patch,
> project and other factors.
>
> Sometimes I add multiple tags as well, e.g. QEMU
> wants both Reviewed-by and Signed-off-by tags.
>
> This patch makes it easy to do so:
> 1.  new parameter am.signoff can be used any number
> 	of times:
>
> [am]
> 	signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
> 	signoff = "Signed-off-by: Michael S. Tsirkin <mst@redhat.com>"
>
> 	if set all signatures are picked up when git am -s is used.

How does this interact with the logic to avoid appending the same
Signed-off-by: line as the last one the incoming message already
has?

> 2.  Any number of alternative signatures
>
> [am "a"]
> 	signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
>
> 	if set the signature type can be specified by passing
> 	a parameter to the -s flag:
>
> 	git am -sa
>
> No docs or tests, sorry, so not yet ready for master, but I'm using this
> all the time without any issues so maybe ok for pu.
> Early flames/feedback/help welcome.

How does that "a" in [am "a"] work?  If it defines some kind of
scope (i.e. use am.a.* instead of am.* when I specify I am using "a"
set somehow), that might be something interesting, but if it applies
only to sign-off and other things, then I am not sure if I like it,
as that would invite confusions from end users.

> +		signoffs=("${signoffs[@]}" "${s[@]}") ;;

Is this a shell array?  It won't fly in our codebase if that is the
case.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 12:07:03PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > I'm using different signature tags for git am depending on the patch,
> > project and other factors.
> >
> > Sometimes I add multiple tags as well, e.g. QEMU
> > wants both Reviewed-by and Signed-off-by tags.
> >
> > This patch makes it easy to do so:
> > 1.  new parameter am.signoff can be used any number
> > 	of times:
> >
> > [am]
> > 	signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
> > 	signoff = "Signed-off-by: Michael S. Tsirkin <mst@redhat.com>"
> >
> > 	if set all signatures are picked up when git am -s is used.
> 
> How does this interact with the logic to avoid appending the same
> Signed-off-by: line as the last one the incoming message already
> has?

Not handled if you have multiple signatures.
That will have to be fixed.
Do we only care about the last line?

Signed-off-by: A
Signed-off-by: B

do we want to add

Signed-off-by: A

or would it be better to replace with
Signed-off-by: B
Signed-off-by: A

?

Current git am will add A twice, I wonder if this is
a feature or a bug.

> > 2.  Any number of alternative signatures
> >
> > [am "a"]
> > 	signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
> >
> > 	if set the signature type can be specified by passing
> > 	a parameter to the -s flag:
> >
> > 	git am -sa
> >
> > No docs or tests, sorry, so not yet ready for master, but I'm using this
> > all the time without any issues so maybe ok for pu.
> > Early flames/feedback/help welcome.
> 
> How does that "a" in [am "a"] work?  If it defines some kind of
> scope (i.e. use am.a.* instead of am.* when I specify I am using "a"
> set somehow), that might be something interesting, but if it applies
> only to sign-off and other things, then I am not sure if I like it,
> as that would invite confusions from end users.
> 
> > +		signoffs=("${signoffs[@]}" "${s[@]}") ;;
> 
> Is this a shell array?  It won't fly in our codebase if that is the
> case.

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Jun 12, 2014 at 12:07:03PM -0700, Junio C Hamano wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> ...
>> > 1.  new parameter am.signoff can be used any number
>> > 	of times:
>> >
>> > [am]
>> > 	signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
>> > 	signoff = "Signed-off-by: Michael S. Tsirkin <mst@redhat.com>"
>> >
>> > 	if set all signatures are picked up when git am -s is used.
>> 
>> How does this interact with the logic to avoid appending the same
>> Signed-off-by: line as the last one the incoming message already
>> has?
>
> Not handled if you have multiple signatures.
> That will have to be fixed.
> Do we only care about the last line?
>
> Signed-off-by: A
> Signed-off-by: B
>
> do we want to add
>
> Signed-off-by: A
>
> or would it be better to replace with
> Signed-off-by: B
> Signed-off-by: A
>
> ?
>
> Current git am will add A twice, I wonder if this is
> a feature or a bug.

This is very much deliberate.

Appending A after existing A and B is meant to record that the patch
originated from A, passed thru B possibly with changes by B, came
back to A who wants to assert that the result is still under DCO.

The only case we can safely omit appending A's sign-off is when the
last one in the chain is by A.  Imagine that you had a patch signed
off by B, which A may have tweaked and forwarded under DCO with A's
sign-off.  Such a patch would have sign-off chain B-A.

Now A makes further changes to the patch and says "the further
change is also something I am authorized to release as open source"
with the "-s" option or some other way.  It would not change that A
can contribute under DCO if we did not add an extra A after existing
B-A sign-off chain in that case.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Fri, Jun 13, 2014 at 10:32:09AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jun 12, 2014 at 12:07:03PM -0700, Junio C Hamano wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> ...
> >> > 1.  new parameter am.signoff can be used any number
> >> > 	of times:
> >> >
> >> > [am]
> >> > 	signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
> >> > 	signoff = "Signed-off-by: Michael S. Tsirkin <mst@redhat.com>"
> >> >
> >> > 	if set all signatures are picked up when git am -s is used.
> >> 
> >> How does this interact with the logic to avoid appending the same
> >> Signed-off-by: line as the last one the incoming message already
> >> has?
> >
> > Not handled if you have multiple signatures.
> > That will have to be fixed.
> > Do we only care about the last line?
> >
> > Signed-off-by: A
> > Signed-off-by: B
> >
> > do we want to add
> >
> > Signed-off-by: A
> >
> > or would it be better to replace with
> > Signed-off-by: B
> > Signed-off-by: A
> >
> > ?
> >
> > Current git am will add A twice, I wonder if this is
> > a feature or a bug.
> 
> This is very much deliberate.
> 
> Appending A after existing A and B is meant to record that the patch
> originated from A, passed thru B possibly with changes by B, came
> back to A who wants to assert that the result is still under DCO.
> 
> The only case we can safely omit appending A's sign-off is when the
> last one in the chain is by A.  Imagine that you had a patch signed
> off by B, which A may have tweaked and forwarded under DCO with A's
> sign-off.  Such a patch would have sign-off chain B-A.
> 
> Now A makes further changes to the patch and says "the further
> change is also something I am authorized to release as open source"
> with the "-s" option or some other way.  It would not change that A
> can contribute under DCO if we did not add an extra A after existing
> B-A sign-off chain in that case.

OK imagine we have signatures:
A
B

Now A wants to sign this patch.

I think there are two reasonable ways to behave:
1. What you describe above:
A
B
A

2. For things like Tested-by: tags, removing tag from
where it was and adding it at the bottom:

B
A


This probably calls for a separate feature:
maybe adding "acks" along with "signoffs"?
acks would be unique, re-adding ack removes it from
the message and adds at the bottom.

-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Now A wants to sign this patch.
>
> I think there are two reasonable ways to behave:
> 1. What you describe above:
> A
> B
> A

That is the only sensible thing to do for Signed-off-by footers.

> 2. For things like Tested-by: tags, removing tag from
> where it was and adding it at the bottom:
>
> B
> A

You can make it arbitrarily more complex.  A sends with sign-off, B
tweaks and tests *that* version and forwards with sign-off, A
further tweaks, which may invalidate the earlier tests, so A asks B
to retest and then forward it to Linus when everything checks out.

What should the end result look like?

	SoB: A
	SoB: B
        Tested-by: B
        SoB: A
        Tested-by: B
	SoB: B

perhaps?

> This probably calls for a separate feature:
> maybe adding "acks" along with "signoffs"?
> acks would be unique, re-adding ack removes it from
> the message and adds at the bottom.

This kind of complication is mostly up to the policies of the
projects that are users of Git, and not something we as the Git
project want to set in stone inside "git am" implementation.

Perhaps consult what has been discussed on Christian's "trailer"
series in the list archive and then think about how to integrate /
make use of it inside "am"?

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Mon, Jun 16, 2014 at 11:06:20AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Now A wants to sign this patch.
> >
> > I think there are two reasonable ways to behave:
> > 1. What you describe above:
> > A
> > B
> > A
> 
> That is the only sensible thing to do for Signed-off-by footers.

OK, after looking into this for a while, I realize
this is a special property of the Signed-off-by footer.
For now I think it's reasonable to just avoid de-duplicating
other footers if any. Agree?

To have it apply to other footers, will have to implement
^[A-Z]*-by: logic that I have implemented for sendemail, in am as well.
I'll get back to that, but I think it's separate from this feature.

-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> OK, after looking into this for a while, I realize
> this is a special property of the Signed-off-by footer.
> For now I think it's reasonable to just avoid de-duplicating
> other footers if any. Agree?

Not really. I'd rather see "git am" hardcode as little such policy as possible.
We do need to support S-o-b footer and the policy we defined for it long time
ago, if only for backward compatiblity, but for any other footers,
policy decision
such as "dedup by default" isn't something "am" should know about.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Tue, Jun 17, 2014 at 11:49:11PM -0700, Junio C Hamano wrote:
> On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > OK, after looking into this for a while, I realize
> > this is a special property of the Signed-off-by footer.
> > For now I think it's reasonable to just avoid de-duplicating
> > other footers if any. Agree?
> 
> Not really. I'd rather see "git am" hardcode as little such policy as possible.
> We do need to support S-o-b footer and the policy we defined for it long time
> ago, if only for backward compatiblity, but for any other footers,
> policy decision
> such as "dedup by default" isn't something "am" should know about.

OK happily that's exactly what v2 that I just posted does.
Default S-o-b footer gets the existing policy.
Any other footers are added on top without any tricky
deduplication.


-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> OK, after looking into this for a while, I realize
>> this is a special property of the Signed-off-by footer.
>> For now I think it's reasonable to just avoid de-duplicating
>> other footers if any. Agree?
>
> Not really. I'd rather see "git am" hardcode as little such policy as possible.
> We do need to support S-o-b footer and the policy we defined for it long time
> ago, if only for backward compatiblity, but for any other footers,
> policy decision
> such as "dedup by default" isn't something "am" should know about.

By the way, "append without looking for dups" is a policy decision
that is as bad to have as "append with dedup".

I'd rather not to see "am.signoff", or any name that implies what
the "-s" option to the command is about for that matter, to be used
in futzing with the trailers other than S-o-b in any way.  As I
understand it, our longer term goal is to defer that task, including
the user-programmable policy decisions, to something like the
'trailer' Christian started.

I suspect that it may add unnecessary work later if we overloaded
"signoff" with a similar feature with the change under discussion.
I would feel safer to see it outlined how we envision to transition
to a more generic 'trailer' solution later if we were to enhance
"am" with "am.signoff" now.

Thanks.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> OK, after looking into this for a while, I realize
> >> this is a special property of the Signed-off-by footer.
> >> For now I think it's reasonable to just avoid de-duplicating
> >> other footers if any. Agree?
> >
> > Not really. I'd rather see "git am" hardcode as little such policy as possible.
> > We do need to support S-o-b footer and the policy we defined for it long time
> > ago, if only for backward compatiblity, but for any other footers,
> > policy decision
> > such as "dedup by default" isn't something "am" should know about.
> 
> By the way, "append without looking for dups" is a policy decision
> that is as bad to have as "append with dedup".
> 
> I'd rather not to see "am.signoff", or any name that implies what
> the "-s" option to the command is about for that matter, to be used
> in futzing with the trailers other than S-o-b in any way.  As I
> understand it, our longer term goal is to defer that task, including
> the user-programmable policy decisions, to something like the
> 'trailer' Christian started.
> 
> I suspect that it may add unnecessary work later if we overloaded
> "signoff" with a similar feature with the change under discussion.
> I would feel safer to see it outlined how we envision to transition
> to a more generic 'trailer' solution later if we were to enhance
> "am" with "am.signoff" now.
> 
> Thanks.

I'll need to look at trailers, if indeed they are a superset of this
functionality, I will transition to trailers when they land on master.
In this case seems easier for me to keep this out tree patch for now,
Good thing I didn't spend time writing docs and tests :)

-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> OK, after looking into this for a while, I realize
> >> this is a special property of the Signed-off-by footer.
> >> For now I think it's reasonable to just avoid de-duplicating
> >> other footers if any. Agree?
> >
> > Not really. I'd rather see "git am" hardcode as little such policy as possible.
> > We do need to support S-o-b footer and the policy we defined for it long time
> > ago, if only for backward compatiblity, but for any other footers,
> > policy decision
> > such as "dedup by default" isn't something "am" should know about.
> 
> By the way, "append without looking for dups" is a policy decision
> that is as bad to have as "append with dedup".
> 
> I'd rather not to see "am.signoff", or any name that implies what
> the "-s" option to the command is about for that matter, to be used
> in futzing with the trailers other than S-o-b in any way.  As I
> understand it, our longer term goal is to defer that task, including
> the user-programmable policy decisions, to something like the
> 'trailer' Christian started.
> 
> I suspect that it may add unnecessary work later if we overloaded
> "signoff" with a similar feature with the change under discussion.
> I would feel safer to see it outlined how we envision to transition
> to a more generic 'trailer' solution later if we were to enhance
> "am" with "am.signoff" now.
> 
> Thanks.

Hi Junio, Christian,
it's been a while.
I see that the work on trailers is going on.
I tried going over the documentation but I could not figure
out how would one implement multiple signatures using the
trailers mechanism.

As a reminder, this old patchset (that I replied to) enhanced git am -s
with an option to add different signatures depending on
the option passed to the -s flag.
E.g. I have
[am "a"]
        signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"

[am "r"]
        signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"

[am "t"]
        signoff = "Tested-by: Michael S. Tsirkin <mst@redhat.com>"

and now:
	git am -s art
adds all 3 signatures when applying the patch.


Any help will be appreciated.

-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Hi Junio, Christian,
> it's been a while.
> I see that the work on trailers is going on.
> I tried going over the documentation but I could not figure
> out how would one implement multiple signatures using the
> trailers mechanism.

Good.  Christian has been rerolling the series numerous times but he
has been working without getting much input from people who would
envision using the mechanism themselves (and my comments on the
series do not count because I am not as one of them).

Please bombard us with questions and usability improvement
suggestions ;-)

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

Hi Michael,

On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Hi Junio, Christian,
> it's been a while.
> I see that the work on trailers is going on.
> I tried going over the documentation but I could not figure
> out how would one implement multiple signatures using the
> trailers mechanism.
>
> As a reminder, this old patchset (that I replied to) enhanced git am -s
> with an option to add different signatures depending on
> the option passed to the -s flag.
> E.g. I have
> [am "a"]
>         signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
>
> [am "r"]
>         signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
>
> [am "t"]
>         signoff = "Tested-by: Michael S. Tsirkin <mst@redhat.com>"
>
> and now:
>         git am -s art
> adds all 3 signatures when applying the patch.

This is probably not as simple as you would like but it works with
something like:

$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
<mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
<mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
<mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch

and then:

$ git am to_apply/*.patch

Also by using something like:

$ git config trailer.a.key Acked-by
$ git config trailer.r.key Reviewed-by
$ git config trailer.t.key Tested-by

the first command could be simplified to:

$ git interpret-trailers --trailer "a: Michael S. Tsirkin
<mst@redhat.com>" --trailer "r: Michael S. Tsirkin <mst@redhat.com>"
--trailer "t: Michael S. Tsirkin <mst@redhat.com>" 0001-foo.patch
>to_apply/0001-foo.patch

And if you use an env variable:

$ ME="Michael S. Tsirkin <mst@redhat.com>"
$ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME"
--trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch

Maybe later we will integrate git interpret-trailers with git commit,
git am and other commands, so that you can do directly:

git am --trailer "a: $ME" --trailer "r: $ME"  --trailer "t: $ME" 0001-foo.patch

Maybe we wil also assign a one letter shortcut to --trailer, for
example "z", so that could be:

git am -z "a: $ME" -z "r: $ME"  -z "t: $ME" 0001-foo.patch

We could also allow many separators in the same -z argument as long as
they are separated by say "~", so you could have:

git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch

And then we could also allow people to define default values for
trailers with something like:

$ git config trailer.a.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
$ git config trailer.r.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
$ git config trailer.t.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"

So that in the end you could have:

git am -z a~r~t 0001-foo.patch

which is very close to "git am -s art".

Best,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Tue, Sep 23, 2014 at 09:45:50AM +0200, Christian Couder wrote:
> Hi Michael,
> 
> On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > Hi Junio, Christian,
> > it's been a while.
> > I see that the work on trailers is going on.
> > I tried going over the documentation but I could not figure
> > out how would one implement multiple signatures using the
> > trailers mechanism.
> >
> > As a reminder, this old patchset (that I replied to) enhanced git am -s
> > with an option to add different signatures depending on
> > the option passed to the -s flag.
> > E.g. I have
> > [am "a"]
> >         signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
> >
> > [am "r"]
> >         signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
> >
> > [am "t"]
> >         signoff = "Tested-by: Michael S. Tsirkin <mst@redhat.com>"
> >
> > and now:
> >         git am -s art
> > adds all 3 signatures when applying the patch.
> 
> This is probably not as simple as you would like but it works with
> something like:
> 
> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
> <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
> <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
> <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
> 
> and then:
> 
> $ git am to_apply/*.patch
> 
> Also by using something like:
> 
> $ git config trailer.a.key Acked-by
> $ git config trailer.r.key Reviewed-by
> $ git config trailer.t.key Tested-by

I would like multiple keys to match a specific
letter, e.g. as a maintainer I need
both reviewed by and signed off by when I
apply a patch, I like applying them with
a single "-s m".

> the first command could be simplified to:
> 
> $ git interpret-trailers --trailer "a: Michael S. Tsirkin
> <mst@redhat.com>" --trailer "r: Michael S. Tsirkin <mst@redhat.com>"
> --trailer "t: Michael S. Tsirkin <mst@redhat.com>" 0001-foo.patch
> >to_apply/0001-foo.patch
> 
> And if you use an env variable:
> 
> $ ME="Michael S. Tsirkin <mst@redhat.com>"
> $ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME"
> --trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch
> 
> Maybe later we will integrate git interpret-trailers with git commit,
> git am and other commands, so that you can do directly:
> 
> git am --trailer "a: $ME" --trailer "r: $ME"  --trailer "t: $ME" 0001-foo.patch
> 
> Maybe we wil also assign a one letter shortcut to --trailer, for
> example "z", so that could be:
> 
> git am -z "a: $ME" -z "r: $ME"  -z "t: $ME" 0001-foo.patch

-s could apply here, right?
It doesn't have a parameter at the moment.

> We could also allow many separators in the same -z argument as long as
> they are separated by say "~",

I think -z a -z r -z t is enough.

> so you could have:
> 
> git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
> 
> And then we could also allow people to define default values for
> trailers with something like:
> 
> $ git config trailer.a.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
> $ git config trailer.r.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
> $ git config trailer.t.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"

I'm kind of confused by the key/value concept.
Can't I define the whole 'Acked-by: Michael S. Tsirkin <mst@redhat.com>'
string as the key?


> So that in the end you could have:
> 
> git am -z a~r~t 0001-foo.patch
> 
> which is very close to "git am -s art".
> 
> Best,
> Christian.

If I figure out the defaultvalue thing, I might
find the time to work on git am integration.

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

On Tue, Sep 23, 2014 at 10:07 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Sep 23, 2014 at 09:45:50AM +0200, Christian Couder wrote:
>> This is probably not as simple as you would like but it works with
>> something like:
>>
>> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
>> <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
>> <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
>> <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
>>
>> and then:
>>
>> $ git am to_apply/*.patch
>>
>> Also by using something like:
>>
>> $ git config trailer.a.key Acked-by
>> $ git config trailer.r.key Reviewed-by
>> $ git config trailer.t.key Tested-by
>
> I would like multiple keys to match a specific
> letter, e.g. as a maintainer I need
> both reviewed by and signed off by when I
> apply a patch, I like applying them with
> a single "-s m".

That's different from what you implemented in your patch.
And franckly I think that for this kind of specific use cases, you
could create your own aliases, either Git aliases or just shell
aliases.

For example if we implement default values and make git am call git
interpret-trailers, a shell alias could simply be:

alias gamsm='git am --trailer r --trailer s'

I use "git log --oneline --decorate --graph" very often, so I made my
own alias for it, and I suppose a lot of other people have done so.

The number of people who will use trailers will probably be much
smaller than the number of people using git log, so if we don't make
shortcuts for "git log --oneline --decorate --graph", I see no ground
to ask for a specific shortcut that adds both a reviewed by and a
signed off by.

>> the first command could be simplified to:
>>
>> $ git interpret-trailers --trailer "a: Michael S. Tsirkin
>> <mst@redhat.com>" --trailer "r: Michael S. Tsirkin <mst@redhat.com>"
>> --trailer "t: Michael S. Tsirkin <mst@redhat.com>" 0001-foo.patch
>> >to_apply/0001-foo.patch
>>
>> And if you use an env variable:
>>
>> $ ME="Michael S. Tsirkin <mst@redhat.com>"
>> $ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME"
>> --trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch
>>
>> Maybe later we will integrate git interpret-trailers with git commit,
>> git am and other commands, so that you can do directly:
>>
>> git am --trailer "a: $ME" --trailer "r: $ME"  --trailer "t: $ME" 0001-foo.patch
>>
>> Maybe we wil also assign a one letter shortcut to --trailer, for
>> example "z", so that could be:
>>
>> git am -z "a: $ME" -z "r: $ME"  -z "t: $ME" 0001-foo.patch
>
> -s could apply here, right?

I don't know what we will do with -s. Maybe if we use -z, we don't need -s.

> It doesn't have a parameter at the moment.

We will have to discuss that kind of thing when we make it possible
for git commit, git am and maybe other commands to accept trailers
arguments and pass them to git interpret-trailers.

In his email Junio seems to say that we don't need a shortcut like -z,
we could only have --trailer.
And I think that it is indeed sound to at least wait a little before
using up one shortcut like -z in many commands.

>> We could also allow many separators in the same -z argument as long as
>> they are separated by say "~",
>
> I think -z a -z r -z t is enough.

Great! I think you will likely have at least "--trailer a --trailer r
--trailer t", but I don't think it is too bad as you can use aliases
to make it shorter to type.

>> so you could have:
>>
>> git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
>>
>> And then we could also allow people to define default values for
>> trailers with something like:
>>
>> $ git config trailer.a.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
>> $ git config trailer.r.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
>> $ git config trailer.t.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
>
> I'm kind of confused by the key/value concept.

A "defaultvalue" would be the value used when no value is passed.
The "key" is just what we will use in the first part of the trailer
(the part before the separator).

For example with the above "defaultvalue" and "key", "--trailer a:
Junio <gitster@pobox.com>" would add:

Acked-by: Junio <gitster@pobox.com>

while "--trailer a" would add:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> Can't I define the whole 'Acked-by: Michael S. Tsirkin <mst@redhat.com>'
> string as the key?

The whole 'Acked-by: Michael S. Tsirkin <mst@redhat.com>' is a full
trailer, not a "key".

And it is not possible right now to define a full trailer. Maybe we
could find a way to make it possible, but a default value and a way to
have a small nickname for the token (like "a" for "Acked-by") should
get people quite far. And then for very specific use cases, it may be
better to use aliases anyway.

>> So that in the end you could have:
>>
>> git am -z a~r~t 0001-foo.patch
>>
>> which is very close to "git am -s art".
>
> If I figure out the defaultvalue thing, I might
> find the time to work on git am integration.

That would be great!

Thanks,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Wed, Sep 24, 2014 at 12:00:40PM +0200, Christian Couder wrote:
> On Tue, Sep 23, 2014 at 10:07 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Sep 23, 2014 at 09:45:50AM +0200, Christian Couder wrote:
> >> This is probably not as simple as you would like but it works with
> >> something like:
> >>
> >> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
> >> <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
> >> <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
> >> <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
> >>
> >> and then:
> >>
> >> $ git am to_apply/*.patch
> >>
> >> Also by using something like:
> >>
> >> $ git config trailer.a.key Acked-by
> >> $ git config trailer.r.key Reviewed-by
> >> $ git config trailer.t.key Tested-by
> >
> > I would like multiple keys to match a specific
> > letter, e.g. as a maintainer I need
> > both reviewed by and signed off by when I
> > apply a patch, I like applying them with
> > a single "-s m".
> 
> That's different from what you implemented in your patch.
> And franckly I think that for this kind of specific use cases, you
> could create your own aliases, either Git aliases or just shell
> aliases.
> 
> For example if we implement default values and make git am call git
> interpret-trailers, a shell alias could simply be:
> 
> alias gamsm='git am --trailer r --trailer s'
> 
> I use "git log --oneline --decorate --graph" very often, so I made my
> own alias for it, and I suppose a lot of other people have done so.
> 
> The number of people who will use trailers will probably be much
> smaller than the number of people using git log, so if we don't make
> shortcuts for "git log --oneline --decorate --graph", I see no ground
> to ask for a specific shortcut that adds both a reviewed by and a
> signed off by.

I've been thinking: how about a generic ability to add option shortcuts
for commands in .config?
For example:

[am "-z"]
	command = "--trailer foobar"

would replace any -z in git am command line with --trailer foobar.


Does this sound useful?


> >> the first command could be simplified to:
> >>
> >> $ git interpret-trailers --trailer "a: Michael S. Tsirkin
> >> <mst@redhat.com>" --trailer "r: Michael S. Tsirkin <mst@redhat.com>"
> >> --trailer "t: Michael S. Tsirkin <mst@redhat.com>" 0001-foo.patch
> >> >to_apply/0001-foo.patch
> >>
> >> And if you use an env variable:
> >>
> >> $ ME="Michael S. Tsirkin <mst@redhat.com>"
> >> $ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME"
> >> --trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch
> >>
> >> Maybe later we will integrate git interpret-trailers with git commit,
> >> git am and other commands, so that you can do directly:
> >>
> >> git am --trailer "a: $ME" --trailer "r: $ME"  --trailer "t: $ME" 0001-foo.patch
> >>
> >> Maybe we wil also assign a one letter shortcut to --trailer, for
> >> example "z", so that could be:
> >>
> >> git am -z "a: $ME" -z "r: $ME"  -z "t: $ME" 0001-foo.patch
> >
> > -s could apply here, right?
> 
> I don't know what we will do with -s. Maybe if we use -z, we don't need -s.
> 
> > It doesn't have a parameter at the moment.
> 
> We will have to discuss that kind of thing when we make it possible
> for git commit, git am and maybe other commands to accept trailers
> arguments and pass them to git interpret-trailers.
> 
> In his email Junio seems to say that we don't need a shortcut like -z,
> we could only have --trailer.
> And I think that it is indeed sound to at least wait a little before
> using up one shortcut like -z in many commands.
> 
> >> We could also allow many separators in the same -z argument as long as
> >> they are separated by say "~",
> >
> > I think -z a -z r -z t is enough.
> 
> Great! I think you will likely have at least "--trailer a --trailer r
> --trailer t", but I don't think it is too bad as you can use aliases
> to make it shorter to type.
> 
> >> so you could have:
> >>
> >> git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
> >>
> >> And then we could also allow people to define default values for
> >> trailers with something like:
> >>
> >> $ git config trailer.a.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
> >> $ git config trailer.r.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
> >> $ git config trailer.t.defaultvalue "Michael S. Tsirkin <mst@redhat.com>"
> >
> > I'm kind of confused by the key/value concept.
> 
> A "defaultvalue" would be the value used when no value is passed.
> The "key" is just what we will use in the first part of the trailer
> (the part before the separator).
> 
> For example with the above "defaultvalue" and "key", "--trailer a:
> Junio <gitster@pobox.com>" would add:
> 
> Acked-by: Junio <gitster@pobox.com>
> 
> while "--trailer a" would add:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> > Can't I define the whole 'Acked-by: Michael S. Tsirkin <mst@redhat.com>'
> > string as the key?
> 
> The whole 'Acked-by: Michael S. Tsirkin <mst@redhat.com>' is a full
> trailer, not a "key".
> 
> And it is not possible right now to define a full trailer. Maybe we
> could find a way to make it possible, but a default value and a way to
> have a small nickname for the token (like "a" for "Acked-by") should
> get people quite far. And then for very specific use cases, it may be
> better to use aliases anyway.
> 
> >> So that in the end you could have:
> >>
> >> git am -z a~r~t 0001-foo.patch
> >>
> >> which is very close to "git am -s art".
> >
> > If I figure out the defaultvalue thing, I might
> > find the time to work on git am integration.
> 
> That would be great!
> 
> Thanks,
> Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> ...
>> As a reminder, this old patchset (that I replied to) enhanced git am -s
>> with an option to add different signatures depending on
>> the option passed to the -s flag.
>> E.g. I have
>> [am "a"]
>>         signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
>>
>> [am "r"]
>>         signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
>>
>> [am "t"]
>>         signoff = "Tested-by: Michael S. Tsirkin <mst@redhat.com>"
>>
>> and now:
>>         git am -s art
>> adds all 3 signatures when applying the patch.
>
> This is probably not as simple as you would like but it works with
> something like:
>
> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
> <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
> <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
> <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
>
> and then:
>
> $ git am to_apply/*.patch

If I understand it correctly, Michael is envisioning to implement
his "git am -s art" (I would recommend against reusing -s for this,
though.  "git am --trailer art" is fine) and doing so by using
interpret-trailers as an internal implementation detail, so I would
say the above is a perfectly fine way to do so.  An equivalent of
that command line is synthesized and run internally in his version
of "git am" when his "git am" sees "--trailer art" option using
those am.{"a","r","t"}.trailer configuration variables.

> Also by using something like:
>
> $ git config trailer.a.key Acked-by
> $ git config trailer.r.key Reviewed-by
> $ git config trailer.t.key Tested-by
>
> the first command could be simplified to:

So I think this mechanism buys Michael's use case very little, if
any.  It might be useful in other contexts, though.

What would be more interesting is if the primitives you have,
e.g. "replace", "append", etc. are sufficient to express his use
case and similar ones.  For example, when working on multiple
trailers (e.g. "am --trailer art" would muck with three kinds), how
should "do this if exists at the end and do that otherwise" work?
To an existing message ends with Michael's Signed-off-by:, if his
"git am --trailer arts" is called to add these three and then a
Signed-off-by: from him, should it add an extra S-o-b (because his
existing S-o-b will no longer be the last one after adding Acked and
others), or should it refrain from doing so?  Can you express both
preferences?

Another thing that got me wondered this morning while I was thinking
about this topic was if "replace" is flexible enough.  We may want
to have "if an entry exists (not necessarily at the end), remove it
and then append a new one with this value at the end" to implement
"Last-tested-by: me@my.domain", for example.

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> What would be more interesting is if the primitives you have,
> e.g. "replace", "append", etc. are sufficient to express his use
> case and similar ones.  For example, when working on multiple
> trailers (e.g. "am --trailer art" would muck with three kinds), how
> should "do this if exists at the end and do that otherwise" work?
> To an existing message ends with Michael's Signed-off-by:, if his
> "git am --trailer arts" is called to add these three and then a
> Signed-off-by: from him, should it add an extra S-o-b (because his
> existing S-o-b will no longer be the last one after adding Acked and
> others), or should it refrain from doing so?  Can you express both
> preferences?

By the way, the answer to this can be "no, but it does not matter.",
of course.  If you can only express the latter (i.e. the addition of
multiple trailers is done as an atomic event, what was the last
before addition of them will be treated during the whole time of
addition of all of them), that may be perfectly fine because the
former (i.e. the addition is done one by one) can easily be emulated
by calling the program multiple times, feeding the trailers one by
one.

> Another thing that got me wondered this morning while I was thinking
> about this topic was if "replace" is flexible enough.  We may want
> to have "if an entry exists (not necessarily at the end), remove it
> and then append a new one with this value at the end" to implement
> "Last-tested-by: me@my.domain", for example.

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

On Tue, Sep 23, 2014 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>>
>> This is probably not as simple as you would like but it works with
>> something like:
>>
>> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
>> <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
>> <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
>> <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
>>
>> and then:
>>
>> $ git am to_apply/*.patch
>
> If I understand it correctly, Michael is envisioning to implement
> his "git am -s art" (I would recommend against reusing -s for this,
> though.  "git am --trailer art" is fine) and doing so by using
> interpret-trailers as an internal implementation detail, so I would
> say the above is a perfectly fine way to do so.  An equivalent of
> that command line is synthesized and run internally in his version
> of "git am" when his "git am" sees "--trailer art" option using
> those am.{"a","r","t"}.trailer configuration variables.

Yeah, that's the idea, except that I think "--trailer art" should mean
a trailer like:

art: <default value>

(if there is no trailer.art.key config variable defined).

Having am.{"a","r","t"}.trailer configuration variables to define full
trailers seems too specific and quite confusing regarding how git
interpret-trailers work without those variables.

>> Also by using something like:
>>
>> $ git config trailer.a.key Acked-by
>> $ git config trailer.r.key Reviewed-by
>> $ git config trailer.t.key Tested-by
>>
>> the first command could be simplified to:
>
> So I think this mechanism buys Michael's use case very little, if
> any.  It might be useful in other contexts, though.
>
> What would be more interesting is if the primitives you have,
> e.g. "replace", "append", etc. are sufficient to express his use
> case and similar ones.  For example, when working on multiple
> trailers (e.g. "am --trailer art" would muck with three kinds), how
> should "do this if exists at the end and do that otherwise" work?

The way the "trailer.<foo>.ifexists" and "trailer.<foo>.where" work is
quite orthogonal to the way we decide what the content of the trailer
is.
If we make "--trailer art" mean "--trailer Acked-by: Michael --trailer
Reviewed-by: Michael --trailer Tested-by: Michael", then it should
work as if we had passed the latter to the command line.

> To an existing message ends with Michael's Signed-off-by:, if his
> "git am --trailer arts" is called to add these three and then a
> Signed-off-by: from him, should it add an extra S-o-b (because his
> existing S-o-b will no longer be the last one after adding Acked and
> others), or should it refrain from doing so?  Can you express both
> preferences?

The default for "trailer.where" is "end", and for "trailer.ifexists"
it is "addIfDifferentNeighbor".
That means that by default it will add the four new trailers at the end.

If either "trailer.ifexists" or "trailer.S-o-b.ifexists" is set to
"addIfDifferent", then only the first 3 new trailers will be added at
the end. So yes you can express both preferences.

> Another thing that got me wondered this morning while I was thinking
> about this topic was if "replace" is flexible enough.  We may want
> to have "if an entry exists (not necessarily at the end), remove it
> and then append a new one with this value at the end" to implement
> "Last-tested-by: me@my.domain", for example.

That's what "replace" does already. That's why I changed the previous
name for this option from "overwrite" to "replace". You have an
"overwrite behavior" with where = after and ifexist = replace, and you
have a "remove old one and append new one behavior" with where = end
and ifexist = replace.

Thanks,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

On Thu, Sep 25, 2014 at 3:04 AM, Christian Couder
<christian.couder@gmail.com> wrote:
>> To an existing message ends with Michael's Signed-off-by:, if his
>> "git am --trailer arts" is called to add these three and then a
>> Signed-off-by: from him, should it add an extra S-o-b (because his
>> existing S-o-b will no longer be the last one after adding Acked and
>> others), or should it refrain from doing so?  Can you express both
>> preferences?
>
> The default for "trailer.where" is "end", and for "trailer.ifexists"
> it is "addIfDifferentNeighbor".
> That means that by default it will add the four new trailers at the end.
>
> If either "trailer.ifexists" or "trailer.S-o-b.ifexists" is set to
> "addIfDifferent", then only the first 3 new trailers will be added at
> the end. So yes you can express both preferences.

Suppose that the original ends with Sob, and Michael's "am" invokes
interpret-trailers with Acked/Reviewed/Tested/Sob in this order. The first
three are set to addifDifferent and Sob is set to addIfDifferentNeighbor,
which would be the traditional and sensible default.

Now, how is addIfDifferentNeighbor interpreted? Adding the new Sob
with this single request to add these four is done on a message that
has the same Sob at the end (i.e. Neighbor). Does _you_ adding of
Acked/Reviewed/Tested invalidate the Neighbor-ness and you add the
Sob anew?

If that is what happens, it is not a workable workaround to set Sob to
addIfDifferent only for this invocation.

Alternatively, if Neighbor-ness is evaluated first before you add A/R/T
in response to this request, then you'd refrain from adding a duplicate
Sob. It wasn't quite clear from your description what your design was,
and your explanation above is not still clear, at least to me.

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
> On Thu, Sep 25, 2014 at 3:04 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>>> To an existing message ends with Michael's Signed-off-by:, if his
>>> "git am --trailer arts" is called to add these three and then a
>>> Signed-off-by: from him, should it add an extra S-o-b (because his
>>> existing S-o-b will no longer be the last one after adding Acked and
>>> others), or should it refrain from doing so?  Can you express both
>>> preferences?
>>
>> The default for "trailer.where" is "end", and for "trailer.ifexists"
>> it is "addIfDifferentNeighbor".
>> That means that by default it will add the four new trailers at the end.
>>
>> If either "trailer.ifexists" or "trailer.S-o-b.ifexists" is set to
>> "addIfDifferent", then only the first 3 new trailers will be added at
>> the end. So yes you can express both preferences.
> 
> Suppose that the original ends with Sob, and Michael's "am" invokes
> interpret-trailers with Acked/Reviewed/Tested/Sob in this order. The first
> three are set to addifDifferent and Sob is set to addIfDifferentNeighbor,
> which would be the traditional and sensible default.
> 
> Now, how is addIfDifferentNeighbor interpreted? Adding the new Sob
> with this single request to add these four is done on a message that
> has the same Sob at the end (i.e. Neighbor). Does _you_ adding of
> Acked/Reviewed/Tested invalidate the Neighbor-ness and you add the
> Sob anew?

The trailers are processed in the order they appear on the command
line. And when a trailer is processed, the input message it "sees" is
the result from the processing of all the previous trailers on the
command line. It is not any more the original input message.

So after Acked/Reviewed/Tested have been added, when the S-o-b at the
end of the command line is processed, it "sees" an input message that
has the Tested trailer at the end. That's why with
addIfDifferentNeighbor the S-o-b will still be added at the end.

> If that is what happens, it is not a workable workaround to set Sob to
> addIfDifferent only for this invocation.

Setting S-o-b to addIfDifferent for this invocation would not add the
S-o-b at the end, because another S-o-b still exists in the input
message "seen" when the last S-o-b is processed.

> Alternatively, if Neighbor-ness is evaluated first before you add A/R/T
> in response to this request, then you'd refrain from adding a duplicate
> Sob. It wasn't quite clear from your description what your design was,
> and your explanation above is not still clear, at least to me.

I hope it is clearer now. Maybe I should add something in the doc to
better explain how it works.

Thanks,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

On Sun, Sep 28, 2014 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> From: Junio C Hamano <gitster@pobox.com>
>>
>>> If that is what happens, it is not a workable workaround to set Sob to
>>> addIfDifferent only for this invocation.
>>
>> Setting S-o-b to addIfDifferent for this invocation would not add the
>> S-o-b at the end, because another S-o-b still exists in the input
>> message "seen" when the last S-o-b is processed.
>
> So there is no workaround whatsoever, which is worse X-<.

I think there might be a misunderstanding.

With v16 you can easily choose if you want to have the S-o-b in the
output or not, when there is already one, see:

$ cat test.txt

Signed-off-by: <Michael>

$ cat test.txt | git interpret-trailers --trailer 'Acked-by:
<Michael>' --trailer 'Reviewed-by: <Michael>' --trailer 'Tested-by:
<Michael>' --trailer 'Signed-off-by: <Michael>'

Signed-off-by: <Michael>
Acked-by: <Michael>
Reviewed-by: <Michael>
Tested-by: <Michael>
Signed-off-by: <Michael>

$ cat test.txt | git -c 'trailer.ifexists=addIfDifferent'
interpret-trailers --trailer 'Acked-by: <Michael>' --trailer
'Reviewed-by: <Michael>' --trailer 'Tested-by: <Michael>' --trailer
'Signed-off-by: <Michael>'

Signed-off-by: <Michael>
Acked-by: <Michael>
Reviewed-by: <Michael>
Tested-by: <Michael>

Or:

$ cat test.txt | git -c
'trailer.Signed-off-by.ifexists=addIfDifferent' interpret-trailers
--trailer 'Acked-by: <Michael>' --trailer 'Reviewed-by: <Michael>'
--trailer 'Tested-by: <Michael>' --trailer 'Signed-off-by: <Michael>'

Signed-off-by: <Michael>
Acked-by: <Michael>
Reviewed-by: <Michael>
Tested-by: <Michael>

(There was a small bug in v15 with the last command above.)

>>> Alternatively, if Neighbor-ness is evaluated first before you add A/R/T
>>> in response to this request, then you'd refrain from adding a duplicate
>>> Sob. It wasn't quite clear from your description what your design was,
>>> and your explanation above is not still clear, at least to me.
>>
>> I hope it is clearer now. Maybe I should add something in the doc to
>> better explain how it works.
>
> I doubt that it would help the users materially to document that we
> chose to implement a less useful way when there are multiple ways in
> which a feature can work, though.
>
> Unless I am mis-reading you and you are actually saying that the
> users can emulate the "atomic" variant without much hassle by doing
> X and Y, that is.  If so, it would help readers to document them.

If you would like me to document the 3 above commands in an example, I
am ok to do that.

Thanks,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder
<christian.couder@gmail.com> wrote:
>
> With v16 you can easily choose if you want to have the S-o-b in the
> output or not, when there is already one, ...

By the way, I sent v16 just before the above email, but the series
still hasn't hit the mailing list.
Did some of you guys in cc receive something?

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> With v16 you can easily choose if you want to have the S-o-b in the
>> output or not, when there is already one, ...
>
> By the way, I sent v16 just before the above email, but the series
> still hasn't hit the mailing list.
> Did some of you guys in cc receive something?

I see them and picked them up to replace.

Are these now ready for 'next'?

Re: [PATCH RFC] git-am: support any number of signatures

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
>
> Christian Couder <christian.couder@gmail.com> writes:
> 
>> On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>>
>>> With v16 you can easily choose if you want to have the S-o-b in the
>>> output or not, when there is already one, ...
>>
>> By the way, I sent v16 just before the above email, but the series
>> still hasn't hit the mailing list.
>> Did some of you guys in cc receive something?
> 
> I see them and picked them up to replace.

Thanks!
 
> Are these now ready for 'next'?

Yeah, I think so.

Thanks,
Christian.

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Tue, Sep 23, 2014 at 10:15:47AM -0700, Junio C Hamano wrote:
> Christian Couder <christian.couder@gmail.com> writes:
> 
> > On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> ...
> >> As a reminder, this old patchset (that I replied to) enhanced git am -s
> >> with an option to add different signatures depending on
> >> the option passed to the -s flag.
> >> E.g. I have
> >> [am "a"]
> >>         signoff = "Acked-by: Michael S. Tsirkin <mst@redhat.com>"
> >>
> >> [am "r"]
> >>         signoff = "Reviewed-by: Michael S. Tsirkin <mst@redhat.com>"
> >>
> >> [am "t"]
> >>         signoff = "Tested-by: Michael S. Tsirkin <mst@redhat.com>"
> >>
> >> and now:
> >>         git am -s art
> >> adds all 3 signatures when applying the patch.
> >
> > This is probably not as simple as you would like but it works with
> > something like:
> >
> > $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
> > <mst@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
> > <mst@redhat.com>"  --trailer "Tested-by: Michael S. Tsirkin
> > <mst@redhat.com>" 0001-foo.patch >to_apply/0001-foo.patch
> >
> > and then:
> >
> > $ git am to_apply/*.patch
> 
> If I understand it correctly, Michael is envisioning to implement
> his "git am -s art" (I would recommend against reusing -s for this,
> though.  "git am --trailer art" is fine) and doing so by using
> interpret-trailers as an internal implementation detail, so I would
> say the above is a perfectly fine way to do so.  An equivalent of
> that command line is synthesized and run internally in his version
> of "git am" when his "git am" sees "--trailer art" option using
> those am.{"a","r","t"}.trailer configuration variables.

Hmm I wonder why do you dislike reusing -s with a parameter for this.
To me, this looks like a superset of the default -s functionality: -s
adds the default signature, -s "x" adds signature "x" ...  Users don't
really care that one is implemented as a trailer and another isn't.  In
fact, default -s can be implemented as a trailer too, right?

Could you clarify please?

-- 
MST

Re: [PATCH RFC] git-am: support any number of signatures

[Jeff King]

On Wed, Oct 08, 2014 at 12:29:37AM +0300, Michael S. Tsirkin wrote:

> > If I understand it correctly, Michael is envisioning to implement
> > his "git am -s art" (I would recommend against reusing -s for this,
> > though.  "git am --trailer art" is fine) and doing so by using
> > interpret-trailers as an internal implementation detail, so I would
> > say the above is a perfectly fine way to do so.  An equivalent of
> > that command line is synthesized and run internally in his version
> > of "git am" when his "git am" sees "--trailer art" option using
> > those am.{"a","r","t"}.trailer configuration variables.
> 
> Hmm I wonder why do you dislike reusing -s with a parameter for this.
> To me, this looks like a superset of the default -s functionality: -s
> adds the default signature, -s "x" adds signature "x" ...  Users don't
> really care that one is implemented as a trailer and another isn't.  In
> fact, default -s can be implemented as a trailer too, right?
> 
> Could you clarify please?

Optional parameters for arguments make backwards-compatibility tricky.
In this case, the command:

  git am -s mbox1 mbox2

means "apply the patches from mbox1 and mbox2, and signoff the patches".
Under your scheme, it now means "apply from mbox2, and use the trailer
mbox1".

I think it would make more sense for "-s" to use a trailer called
"signoff" if it is configured (and if not, have a baked-in "signoff"
trailer config that behaves like "-s" does now). So "-s" (and
"--signoff") become "sign off in the way I usually do for my project",
not just "add a signed-off-by line". If you want to something more
fancy, you have to use "--trailer=...".

Just my two cents, as one who has not been closely following this
discussion. Apologies if this idea was already presented and shot down.
:)

-Peff

Re: [PATCH RFC] git-am: support any number of signatures

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Optional parameters for arguments make backwards-compatibility tricky.
> In this case, the command:
>
>   git am -s mbox1 mbox2
>
> means "apply the patches from mbox1 and mbox2, and signoff the patches".
> Under your scheme, it now means "apply from mbox2, and use the trailer
> mbox1".

Thanks for saving me from typing ;-)

Re: [PATCH RFC] git-am: support any number of signatures

[René Scharfe]

Am 12.06.2014 18:12, schrieb Michael S. Tsirkin:
> @@ -136,7 +136,7 @@ fall_back_3way () {
>       eval "$cmd" &&
>       GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \
>       git write-tree >"$dotest/patch-merge-base+" ||
> -    cannot_fallback "$(gettext "Repository lacks necessary blobs to fall back on 3-way merge.")"
> +    cannot_fallback "$(gettext "Repository lsignoffs necessary blobs to fall back on 3-way merge.")"

lsignoffs?

Re: [PATCH RFC] git-am: support any number of signatures

[Michael S. Tsirkin]

On Thu, Jun 12, 2014 at 09:25:54PM +0200, René Scharfe wrote:
> Am 12.06.2014 18:12, schrieb Michael S. Tsirkin:
> >@@ -136,7 +136,7 @@ fall_back_3way () {
> >      eval "$cmd" &&
> >      GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \
> >      git write-tree >"$dotest/patch-merge-base+" ||
> >-    cannot_fallback "$(gettext "Repository lacks necessary blobs to fall back on 3-way merge.")"
> >+    cannot_fallback "$(gettext "Repository lsignoffs necessary blobs to fall back on 3-way merge.")"
> 
> lsignoffs?

Heh good catch, I'll fix this up.
Thanks!