Signed-off-by vs Reviewed-by

Signed-off-by vs Reviewed-by

[Miklos Vajna]

Hi,

Some projects like LibreOffice don't use Signed-off-by, instead usually
use Gerrit for code review, and reviewers add a Reviewed-by line when
they are OK with a patch.  In this workflow it's a bit unfortunate that
adding a Signed-off-by line is just a command-line switch, but adding a
Reviewed-by line is more complex.

Is there anything in git that could help this situation? I didn't see
any related config option; I wonder if a patch would be accepted to make
the "Signed-off-by" line configurable, or there is a better way.

Like, would a patch that adds e.g. a core.signedOffString configuration
option to make the string customizable welcome?

Thanks,

Miklos

Re: Signed-off-by vs Reviewed-by

[Pranit Bauva]

On Thu, Mar 31, 2016 at 6:05 PM, Miklos Vajna <vmiklos@collabora.co.uk> wrote:
> Hi,
>
> Some projects like LibreOffice don't use Signed-off-by, instead usually
> use Gerrit for code review, and reviewers add a Reviewed-by line when
> they are OK with a patch.  In this workflow it's a bit unfortunate that
> adding a Signed-off-by line is just a command-line switch, but adding a
> Reviewed-by line is more complex.
>
> Is there anything in git that could help this situation? I didn't see
> any related config option; I wonder if a patch would be accepted to make
> the "Signed-off-by" line configurable, or there is a better way.

Actually there is a "related" config option format.signOff (more about
this in Documentation/config.txt) which is a boolean.
But that will only enable the "-s" by default.

> Like, would a patch that adds e.g. a core.signedOffString configuration
> option to make the string customizable welcome?

Are you suggesting to use a different email address for commiting,
signing off and reviewing?

> Thanks,
>
> Miklos

Re: Signed-off-by vs Reviewed-by

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi,

On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Are you suggesting to use a different email address for commiting,
> signing off and reviewing?

Let's say project A has a workflow where patch authors and maintainers
add a "Signed-off-by: A B <a@example.com>" line. This is well-supported
by git, various commands have a -s option to add that line.

However, if project B has a workflow where patch authors add no such
line, and reviewers add a "Reviewed-by: A B <a@example.com>" line, then
you have to add that line manually when you do a review.

I suggest to give a bit more support to this workflow in git. One way of
doing that would be to make the Signed-off-by string configurable. I can
look into implementing that, but first I wanted to discuss the idea here
on the list -- perhaps there is a better way to support that. :-)

Typing that line (including copy&pasting your name + email all the time)
is a bit boring.

Regards,

Miklos

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Signed-off-by vs Reviewed-by

[Sidhant Sharma]

Hi,

On Thursday 31 March 2016 08:05 PM, Miklos Vajna wrote:
> Hi,
>
> On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Are you suggesting to use a different email address for commiting,
>> signing off and reviewing?
> Let's say project A has a workflow where patch authors and maintainers
> add a "Signed-off-by: A B <a@example.com>" line. This is well-supported
> by git, various commands have a -s option to add that line.
>
> However, if project B has a workflow where patch authors add no such
> line, and reviewers add a "Reviewed-by: A B <a@example.com>" line, then
> you have to add that line manually when you do a review.
When making the string configurable, would it be a good idea to
support more than one sign-off strings? For instance, often patches
here in Git have both a Signed-Off and a Reviewed-by line. What would
you suggest for such a case?


Regards,
Sidhant

Re: Signed-off-by vs Reviewed-by

[Christian Couder]

On Thu, Mar 31, 2016 at 4:57 PM, Sidhant Sharma <tigerkid001@gmail.com> wrote:
> Hi,
>
> On Thursday 31 March 2016 08:05 PM, Miklos Vajna wrote:
>> Hi,
>>
>> On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Are you suggesting to use a different email address for commiting,
>>> signing off and reviewing?
>> Let's say project A has a workflow where patch authors and maintainers
>> add a "Signed-off-by: A B <a@example.com>" line. This is well-supported
>> by git, various commands have a -s option to add that line.
>>
>> However, if project B has a workflow where patch authors add no such
>> line, and reviewers add a "Reviewed-by: A B <a@example.com>" line, then
>> you have to add that line manually when you do a review.
> When making the string configurable, would it be a good idea to
> support more than one sign-off strings? For instance, often patches
> here in Git have both a Signed-Off and a Reviewed-by line. What would
> you suggest for such a case?

"git interpret-trailers" supports many kinds of trailers. There were a
lot of related discussions/bikeshedding when it was designed and
worked on.

Re: Signed-off-by vs Reviewed-by

[Junio C Hamano]

Miklos Vajna <vmiklos@collabora.co.uk> writes:

> Typing that line (including copy&pasting your name + email all the time)
> is a bit boring.

I think the last message from Christian in the thread points at the
right direction in the future.

The internal "parse the existing trailer block and manipulate it by
adding, conditionally adding, replacing and deleting it" logic was
done as an experimental "interpret-trailers" program, but polishing
it (both its design and implementation) and integrating it to the
front-line programs (e.g. "git commit") hasn't been done.

As to the last step of "integration", we cannot use short-and-sweet
single letter options like '-s' (for sign-off) for each and every
custom trailer different projects use for their own purpose (as
there are only 26 of the lowercase ASCII alphabet letters), so the
most general syntax for the option has to become "--trailer <arg>"
or some variation of it, and at that point "-s" would look like a
short-hand for "--trailer signed-off-by".

Re: Signed-off-by vs Reviewed-by

[Jeff King]

On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:

> As to the last step of "integration", we cannot use short-and-sweet
> single letter options like '-s' (for sign-off) for each and every
> custom trailer different projects use for their own purpose (as
> there are only 26 of the lowercase ASCII alphabet letters), so the
> most general syntax for the option has to become "--trailer <arg>"
> or some variation of it, and at that point "-s" would look like a
> short-hand for "--trailer signed-off-by".

I can imagine it would be useful to give one short-and-sweet to "add my
standard trailers", where that standard set is defined in the config
file. But that is just a guess; I do not personally have a workflow
where such standard trailers exist, beyond the normal s-o-b.

-Peff

Re: Signed-off-by vs Reviewed-by

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:
>
>> As to the last step of "integration", we cannot use short-and-sweet
>> single letter options like '-s' (for sign-off) for each and every
>> custom trailer different projects use for their own purpose (as
>> there are only 26 of the lowercase ASCII alphabet letters), so the
>> most general syntax for the option has to become "--trailer <arg>"
>> or some variation of it, and at that point "-s" would look like a
>> short-hand for "--trailer signed-off-by".
>
> I can imagine it would be useful to give one short-and-sweet to "add my
> standard trailers", where that standard set is defined in the config
> file. But that is just a guess; I do not personally have a workflow
> where such standard trailers exist, beyond the normal s-o-b.

Yup, I agree; I meant by "some variation of it" to cover such an
arrangement ;-)

Re: Signed-off-by vs Reviewed-by

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

Hi,

On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> The internal "parse the existing trailer block and manipulate it by
> adding, conditionally adding, replacing and deleting it" logic was
> done as an experimental "interpret-trailers" program, but polishing
> it (both its design and implementation) and integrating it to the
> front-line programs (e.g. "git commit") hasn't been done.

I had a look at interpret-trailers, and one use-case I miss is: being
able to define a trailer type, but only add it when asked explicitly.

Example:

----
$ git config trailer.review.key "Reviewed-by: "
$ git config trailer.review.command 'echo "$(git config user.name) <$(git config user.email)>"'
$ echo foo|git interpret-trailers
foo

Reviewed-by: A U Thor <author@example.com>
$ echo foo|git interpret-trailers --trailer review
foo

Reviewed-by: A U Thor <author@example.com>
----

I can imagine e.g. a new configuration vaulue named
trailer.<token>.ifMissing explicit, and when that's set, the trailer
would be only added if it's spelled out explicitly using '--trailer
<token>'.

Does this sound like a good idea, or did I miss some way how this is
already possible? :-)

> As to the last step of "integration", we cannot use short-and-sweet
> single letter options like '-s' (for sign-off) for each and every
> custom trailer different projects use for their own purpose (as
> there are only 26 of the lowercase ASCII alphabet letters), so the
> most general syntax for the option has to become "--trailer <arg>"
> or some variation of it, and at that point "-s" would look like a
> short-hand for "--trailer signed-off-by".

Hmm, I think the above has to be implemented first, otherwise it'll be
hard to make "-s" an alias of "--trailer signed-off-by". (I mean having
git understand what "signed-off-by" is, still adding it conditionally.)

Regards,

Miklos

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Signed-off-by vs Reviewed-by

[Jeff King]

On Thu, Mar 31, 2016 at 02:35:07PM +0200, Miklos Vajna wrote:

> Hi,
> 
> Some projects like LibreOffice don't use Signed-off-by, instead usually
> use Gerrit for code review, and reviewers add a Reviewed-by line when
> they are OK with a patch.  In this workflow it's a bit unfortunate that
> adding a Signed-off-by line is just a command-line switch, but adding a
> Reviewed-by line is more complex.
> 
> Is there anything in git that could help this situation? I didn't see
> any related config option; I wonder if a patch would be accepted to make
> the "Signed-off-by" line configurable, or there is a better way.

There's git-interpret-trailers, which can do the heavy lifting of adding
it in the right place. But I don't know how you'd want to trigger it; it
would depend on the workflow that people use to add their signoff in the
first place.  I don't think there is anything as easy as "git commit
--amend -s", but I'm not all that familiar with the interpret-trailers
code.

-Peff

Re: Signed-off-by vs Reviewed-by

[Christian Couder]

On Thu, Mar 31, 2016 at 4:32 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 31, 2016 at 02:35:07PM +0200, Miklos Vajna wrote:
>
>> Hi,
>>
>> Some projects like LibreOffice don't use Signed-off-by, instead usually
>> use Gerrit for code review, and reviewers add a Reviewed-by line when
>> they are OK with a patch.  In this workflow it's a bit unfortunate that
>> adding a Signed-off-by line is just a command-line switch, but adding a
>> Reviewed-by line is more complex.
>>
>> Is there anything in git that could help this situation? I didn't see
>> any related config option; I wonder if a patch would be accepted to make
>> the "Signed-off-by" line configurable, or there is a better way.
>
> There's git-interpret-trailers, which can do the heavy lifting of adding
> it in the right place. But I don't know how you'd want to trigger it; it
> would depend on the workflow that people use to add their signoff in the
> first place.  I don't think there is anything as easy as "git commit
> --amend -s", but I'm not all that familiar with the interpret-trailers
> code.

The plan was to make it possible for many commands, like commit,
cherry-pick, am, and so on, to accept "--trailer ..." options and to
pass them to interpret-trailers that would process them. I remember
starting working on that and sending some patches at one point...