Adding Reviewed-by/Tested-by tags to other peoples commits

Adding Reviewed-by/Tested-by tags to other peoples commits

[Alex Bennee]

HI,

I've just tested/reviewed a patch of someone elses and I want to
forward it on the appropriate mailing list. I gather for Linux you
just add the appropriate tags to the commit. Does git offer a shortcut
for doing this or do you have to do a reset HEAD^ and re-commit with a
copy&pasted and modified commit message?

Or does everyone have their own magic scripts for tagging commits
during reviews?

-- 
Alex, homepage: http://www.bennee.com/~alex/

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Jeff King]

On Sat, Oct 11, 2008 at 07:37:04AM +0100, Alex Bennee wrote:

> I've just tested/reviewed a patch of someone elses and I want to
> forward it on the appropriate mailing list. I gather for Linux you
> just add the appropriate tags to the commit. Does git offer a shortcut
> for doing this or do you have to do a reset HEAD^ and re-commit with a
> copy&pasted and modified commit message?

Try "git commit --amend" to edit the commit message. There's no
automatic way of adding acked-by or tested-by tags with git; most people
who do those things often would probably configure their editor to make
it easier.

-Peff

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Johannes Schindelin]

Hi,

On Sat, 11 Oct 2008, Jeff King wrote:

> On Sat, Oct 11, 2008 at 07:37:04AM +0100, Alex Bennee wrote:
> 
> > I've just tested/reviewed a patch of someone elses and I want to 
> > forward it on the appropriate mailing list. I gather for Linux you 
> > just add the appropriate tags to the commit. Does git offer a shortcut 
> > for doing this or do you have to do a reset HEAD^ and re-commit with a 
> > copy&pasted and modified commit message?
> 
> Try "git commit --amend" to edit the commit message. There's no 
> automatic way of adding acked-by or tested-by tags with git; most people 
> who do those things often would probably configure their editor to make 
> it easier.

Of course, you could also automate it with a script:

-- snip --
GIT_EDITOR='sh -c "perl -pi.bak -e s/pick/edit/ \"$1\""' \
git rebase -i HEAD~2
while test -d .git/rebase-merge
do
 GIT_EDITOR='sh -c "echo \"Reviewed-by: Mini Me <mi@ni.me>\" >> \"$1\""' \
 git commit --amend &&
 git rebase --continue ||
 break
done
-- snap --

(Of course you would make HEAD~2 a parameter...)

BTW what happened with the "amend" patches to rebase -i?  AFAIR Peff or 
MadCoder (or was it j6t?) had patches to teach rebase -i the "amend" 
command, which would not fall back to the command line just to fix a typo 
in a commit message... I would like to have that.

Hth,
Dscho

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Oct 11, 2008 at 07:37:04AM +0100, Alex Bennee wrote:
>
>> I've just tested/reviewed a patch of someone elses and I want to
>> forward it on the appropriate mailing list. I gather for Linux you
>> just add the appropriate tags to the commit. Does git offer a shortcut
>> for doing this or do you have to do a reset HEAD^ and re-commit with a
>> copy&pasted and modified commit message?
>
> Try "git commit --amend" to edit the commit message. There's no
> automatic way of adding acked-by or tested-by tags with git; most people
> who do those things often would probably configure their editor to make
> it easier.

These Distimmed-by: fields are cumulative in nature, so once recorded in a
commit log message you cannot add new ones without amending, which means
you cannot exchange such commits by pushing or pulling but by forwarding
patches via e-mails.  I thought people added these in their MUAs.  IOW, I
share your "would probably" with s/editor/MUA/.

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Johannes Schindelin]

Hi,

On Sat, 11 Oct 2008, Alex Bennee wrote:

> I've just tested/reviewed a patch of someone elses and I want to forward 
> it on the appropriate mailing list. I gather for Linux you just add the 
> appropriate tags to the commit. Does git offer a shortcut for doing this 
> or do you have to do a reset HEAD^ and re-commit with a copy&pasted and 
> modified commit message?

http://thread.gmane.org/gmane.comp.version-control.git/75250/focus=76304

In the end, nothing happened, but I could see that you might want to push 
for this patch.  However, you'll have to replace the strbuf_initf() by 
the ugly strbuf_init() && strbuf_addf() sequence, because strbuf_initf() 
never made it into git.git.

Ciao,
Dscho

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sat, 11 Oct 2008, Alex Bennee wrote:
>
>> I've just tested/reviewed a patch of someone elses and I want to forward 
>> it on the appropriate mailing list. I gather for Linux you just add the 
>> appropriate tags to the commit. Does git offer a shortcut for doing this 
>> or do you have to do a reset HEAD^ and re-commit with a copy&pasted and 
>> modified commit message?
>
> http://thread.gmane.org/gmane.comp.version-control.git/75250/focus=76304
>
> In the end, nothing happened, but I could see that you might want to push 
> for this patch.

The fact a particular change has been reviewed is an attribute of a
commit, and by recording the fact once (perhaps when you commit for the
first time, or if your workflow is "commit blindly, then review, and then
amend" then when you amend that commit), the commit object will remember
that fact.

The patch you quoted adds Reviewed-by: at format-patch time, but I suspect
that is a wrong approach.  You have to remember and recall which ones you
reviewed (and which ones you didn't) when you run format-patch.  People
who commit and immediately format-patch to send, or people who do not
review until immediately before format-patch to send, would not realize
the downside of the approach, but when your work style is to perform
commit/review and e-mail communication in separate phases, it matters.

This is a bit tangent, but perhaps rebase needs a hook so that users can
strip certain tags automatically from the commit log messages (e.g.
things like Reviewd-by: and Tested-by: become less trustworthy when you
rebase; S-o-b: becomes somewhat less trustworthy when you "edit" in
rebase-i; etc).

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Johannes Schindelin]

Hi,

On Sat, 11 Oct 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sat, 11 Oct 2008, Alex Bennee wrote:
> >
> >> I've just tested/reviewed a patch of someone elses and I want to 
> >> forward it on the appropriate mailing list. I gather for Linux you 
> >> just add the appropriate tags to the commit. Does git offer a 
> >> shortcut for doing this or do you have to do a reset HEAD^ and 
> >> re-commit with a copy&pasted and modified commit message?
> >
> > http://thread.gmane.org/gmane.comp.version-control.git/75250/focus=76304
> >
> > In the end, nothing happened, but I could see that you might want to 
> > push for this patch.
> 
> The fact a particular change has been reviewed is an attribute of a 
> commit, and by recording the fact once (perhaps when you commit for the 
> first time, or if your workflow is "commit blindly, then review, and 
> then amend" then when you amend that commit), the commit object will 
> remember that fact.

If that was the goal, you would _have_ to add commit notes.  Because 
otherwise you would have to pretend to have changed the commit, which you 
did not at all.

> The patch you quoted adds Reviewed-by: at format-patch time, but I 
> suspect that is a wrong approach.

Color me puzzled.  You said in another mail that you think this is the 
task for the MUA.  When I send patches (even forwarded ones), I use 
format-patch just before pasting into the MUA (I do not trust send-email).  
And that's where Git can kick in: format-patch.  Not the MUA.

So technically, I understood that the format-patch approach is exactly the 
same as you were proposing, only that you do not ask the MUA to do Git's 
job.

Unless, of course, you are talking about the reviewing style where the 
patch does not leave the MUA until it is to be forwarded.

> You have to remember and recall which ones you reviewed (and which ones 
> you didn't) when you run format-patch.

Don't you have to do that anyway?  I do not see how giving format-patch a 
new option --reviewed-by would change the equation in any way.

> This is a bit tangent, but perhaps rebase needs a hook so that users can 
> strip certain tags automatically from the commit log messages (e.g. 
> things like Reviewd-by: and Tested-by: become less trustworthy when you 
> rebase; S-o-b: becomes somewhat less trustworthy when you "edit" in 
> rebase-i; etc).

Maybe.  I am not really convinced of the S-o-b.  You kept stressing that 
the SOB is not about validity, but a statement that the patch is 
intellectually proper or some such (IOW it means something like "Darl, 
forget it").  And the point of origin does not change, even if you rebase 
the commit.

Ciao,
Dscho

Re: Adding Reviewed-by/Tested-by tags to other peoples commits

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Color me puzzled.  You said in another mail that you think this is the 
> task for the MUA.

Not really.  I said that I think people usually do this in MUA with the
current system.  I did not mean to say that I think such a partition of
jobs between commit and MUA is ideal.

>> This is a bit tangent, but perhaps rebase needs a hook so that users can 
>> strip certain tags automatically from the commit log messages (e.g. 
>> things like Reviewd-by: and Tested-by: become less trustworthy when you 
>> rebase; S-o-b: becomes somewhat less trustworthy when you "edit" in 
>> rebase-i; etc).
>
> Maybe.  I am not really convinced of the S-o-b.  You kept stressing that 
> the SOB is not about validity, but a statement that the patch is 
> intellectually proper or some such (IOW it means something like "Darl, 
> forget it").  And the point of origin does not change, even if you rebase 
> the commit.

The "somewhat less trustworthy" kicks in when you "edit" in rebase-i if
you change the tree that gets recorded.  You are right that it is
irrelevant if you ran rebase-i to only edit the commit log message.