[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 <***@redhat.com>"
signoff = "Signed-off-by: Michael S. Tsirkin <***@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 <***@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 <***@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

[Junio C Hamano]

Post by 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.
1. new parameter am.signoff can be used any number
[am]
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?

Post by Michael S. Tsirkin
2. Any number of alternative signatures
[am "a"]
if set the signature type can be specified by passing
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.
Is this a shell array? It won't fly in our codebase if that is the
case.

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by 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.
1. new parameter am.signoff can be used any number
[am]
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.

Post by Junio C Hamano

Post by Michael S. Tsirkin
2. Any number of alternative signatures
[am "a"]
if set the signature type can be specified by passing
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.
Is this a shell array? It won't fly in our codebase if that is the
case.

[Junio C Hamano]

Post by Michael S. Tsirkin

Post by Junio C Hamano
...

Post by Michael S. Tsirkin
1. new parameter am.signoff can be used any number
[am]
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.

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Michael S. Tsirkin

Post by Junio C Hamano
...

Post by Michael S. Tsirkin
1. new parameter am.signoff can be used any number
[am]
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.

[Junio C Hamano]

Post by Michael S. Tsirkin
Now A wants to sign this patch.
A
B
A

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

Post by Michael S. Tsirkin
2. For things like Tested-by: tags, removing tag from
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?

Post by Michael S. Tsirkin
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"?

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Michael S. Tsirkin
Now A wants to sign this patch.
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.

[Junio C Hamano]

Post by Michael S. Tsirkin
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.

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Michael S. Tsirkin
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.

[Junio C Hamano]

Post by Junio C Hamano

Post by Michael S. Tsirkin
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.

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Junio C Hamano

Post by Michael S. Tsirkin
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 :)

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Junio C Hamano

Post by Michael S. Tsirkin
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 <***@redhat.com>"

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

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

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


Any help will be appreciated.

[Junio C Hamano]

Post by Michael S. Tsirkin
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 ;-)

[Christian Couder]

Hi Michael,

Post by Michael S. Tsirkin
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"]
[am "r"]
[am "t"]
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
<***@redhat.com>" --trailer "Reviewed-by: Michael S. Tsirkin
<***@redhat.com>" --trailer "Tested-by: Michael S. Tsirkin
<***@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

Post by Michael S. Tsirkin
to_apply/0001-foo.patch

And if you use an env variable:

$ ME="Michael S. Tsirkin <***@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 <***@redhat.com>"
$ git config trailer.r.defaultvalue "Michael S. Tsirkin <***@redhat.com>"
$ git config trailer.t.defaultvalue "Michael S. Tsirkin <***@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.

[Michael S. Tsirkin]

Post by Christian Couder
Hi Michael,

Post by Michael S. Tsirkin
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"]
[am "r"]
[am "t"]
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
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ git am to_apply/*.patch
$ 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".

Post by Christian Couder
$ git interpret-trailers --trailer "a: Michael S. Tsirkin

Post by Michael S. Tsirkin
to_apply/0001-foo.patch

$ 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 --trailer "a: $ME" --trailer "r: $ME" --trailer "t: $ME" 0001-foo.patch
Maybe we wil also assign a one letter shortcut to --trailer, for
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.

Post by Christian Couder
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.

Post by Christian Couder
git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
And then we could also allow people to define default values for

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

Post by Christian Couder
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.

[Christian Couder]

Post by Michael S. Tsirkin

Post by Christian Couder
This is probably not as simple as you would like but it works with
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ git am to_apply/*.patch
$ 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.

Post by Michael S. Tsirkin

Post by Christian Couder
$ git interpret-trailers --trailer "a: Michael S. Tsirkin

Post by Christian Couder
to_apply/0001-foo.patch

$ 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 --trailer "a: $ME" --trailer "r: $ME" --trailer "t: $ME" 0001-foo.patch
Maybe we wil also assign a one letter shortcut to --trailer, for
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.

Post by Michael S. Tsirkin
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.

Post by Michael S. Tsirkin

Post by Christian Couder
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.

Post by Michael S. Tsirkin

Post by Christian Couder
git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
And then we could also allow people to define default values for

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

Post by Michael S. Tsirkin
string as the key?

The whole 'Acked-by: Michael S. Tsirkin <***@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.

Post by Michael S. Tsirkin

Post by Christian Couder
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.

[Michael S. Tsirkin]

Post by Christian Couder

Post by Michael S. Tsirkin

Post by Christian Couder
This is probably not as simple as you would like but it works with
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ git am to_apply/*.patch
$ 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
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?

Post by Christian Couder

Post by Michael S. Tsirkin

Post by Christian Couder
$ git interpret-trailers --trailer "a: Michael S. Tsirkin

Post by Christian Couder
to_apply/0001-foo.patch

$ 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 --trailer "a: $ME" --trailer "r: $ME" --trailer "t: $ME" 0001-foo.patch
Maybe we wil also assign a one letter shortcut to --trailer, for
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.

Post by Michael S. Tsirkin
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.

Post by Michael S. Tsirkin

Post by Christian Couder
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.

Post by Michael S. Tsirkin

Post by Christian Couder
git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch
And then we could also allow people to define default values for

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

Post by Michael S. Tsirkin
string as the key?

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.

Post by Michael S. Tsirkin

Post by Christian Couder
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.

[Junio C Hamano]

Post by Christian Couder

Post by Michael S. Tsirkin
...
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"]
[am "r"]
[am "t"]
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
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ 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.

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

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: ***@my.domain", for example.

[Junio C Hamano]

Post by Junio C Hamano
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.

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

[Christian Couder]

Post by Junio C Hamano

Post by Christian Couder
This is probably not as simple as you would like but it works with
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ 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.

Post by Junio C Hamano

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

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.

Post by Junio C Hamano
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.

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

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.

[Junio C Hamano]

On Thu, Sep 25, 2014 at 3:04 AM, Christian Couder

Post by Christian Couder

Post by Junio C Hamano
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.

[Christian Couder]

Post by Junio C Hamano
On Thu, Sep 25, 2014 at 3:04 AM, Christian Couder

Post by Christian Couder

Post by Junio C Hamano
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.

Post by Junio C Hamano
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.

Post by Junio C Hamano
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.

[Christian Couder]

Post by Christian Couder

Post by Junio C Hamano
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.)

Post by Christian Couder

Post by Junio C Hamano
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.

[Christian Couder]

On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder

Post by Christian Couder
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?

[Junio C Hamano]

Post by Christian Couder
On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder

Post by Christian Couder
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'?

[Christian Couder]

Post by Junio C Hamano

Post by Christian Couder
On Sun, Oct 12, 2014 at 11:36 AM, Christian Couder

Post by Christian Couder
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!

Post by Junio C Hamano
Are these now ready for 'next'?

Yeah, I think so.

Thanks,
Christian.

[Michael S. Tsirkin]

Post by Junio C Hamano

Post by Christian Couder

Post by Michael S. Tsirkin
...
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"]
[am "r"]
[am "t"]
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
$ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
$ 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?

[Jeff King]

Post by Michael S. Tsirkin

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

[Junio C Hamano]

Post by Jeff King
Optional parameters for arguments make backwards-compatibility tricky.
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 ;-)

[René Scharfe]

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

[Michael S. Tsirkin]

Post by René Scharfe

Post by Michael S. Tsirkin
@@ -136,7 +136,7 @@ fall_back_3way () {
eval "$cmd" &&
GIT_INDEX_FILE=3D"$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.")"

Post by René Scharfe

Post by Michael S. Tsirkin
+ cannot_fallback "$(gettext "Repository lsignoffs necessary blob=

s to fall back on 3-way merge.")"

Post by René Scharfe
=20
lsignoffs?

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