{"thread":{"id":"41883","subject":"Signed-off-by vs Reviewed-by","startedAt":"2016-03-31T12:35:07Z","lastAt":"2016-04-01T14:10:36Z","messageCount":11,"participants":["Miklos Vajna","Pranit Bauva","Jeff King","Sidhant Sharma","Christian Couder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282301","messageId":"20160331123507.GC19857@collabora.co.uk","threadId":"41883","inReplyTo":null,"subject":"Signed-off-by vs Reviewed-by","fromName":"Miklos Vajna","fromEmail":"vmiklos@collabora.co.uk","sentAt":"2016-03-31T12:35:07Z","receivedAt":"2016-03-31T12:35:07Z","isPatch":false,"sender":{"key":"vmiklos@collabora.co.uk","avatar":null},"body":"Hi,\n\nSome projects like LibreOffice don't use Signed-off-by, instead usually\nuse Gerrit for code review, and reviewers add a Reviewed-by line when\nthey are OK with a patch. In this workflow it's a bit unfortunate that\nadding a Signed-off-by line is just a command-line switch, but adding a\nReviewed-by line is more complex.\n\nIs there anything in git that could help this situation? I didn't see\nany related config option; I wonder if a patch would be accepted to make\nthe \"Signed-off-by\" line configurable, or there is a better way.\n\nLike, would a patch that adds e.g. a core.signedOffString configuration\noption to make the string customizable welcome?\n\nThanks,\n\nMiklos\n"},{"id":"282310","messageId":"CAFZEwPMzcqrd8NEP6MH5saXL2KdUKAyN51uuoS5=aeU0aPWjJQ@mail.gmail.com","threadId":"41883","inReplyTo":"20160331123507.GC19857@collabora.co.uk","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T14:24:47Z","receivedAt":"2016-03-31T14:24:47Z","isPatch":false,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 31, 2016 at 6:05 PM, Miklos Vajna wrote:\n> Hi,\n>\n> Some projects like LibreOffice don't use Signed-off-by, instead usually\n> use Gerrit for code review, and reviewers add a Reviewed-by line when\n> they are OK with a patch. In this workflow it's a bit unfortunate that\n> adding a Signed-off-by line is just a command-line switch, but adding a\n> Reviewed-by line is more complex.\n>\n> Is there anything in git that could help this situation? I didn't see\n> any related config option; I wonder if a patch would be accepted to make\n> the \"Signed-off-by\" line configurable, or there is a better way.\n\nActually there is a \"related\" config option format.signOff (more about\nthis in Documentation/config.txt) which is a boolean.\nBut that will only enable the \"-s\" by default.\n\n> Like, would a patch that adds e.g. a core.signedOffString configuration\n> option to make the string customizable welcome?\n\nAre you suggesting to use a different email address for commiting,\nsigning off and reviewing?\n\n> Thanks,\n>\n> Miklos\n"},{"id":"282313","messageId":"20160331143244.GD31116@sigill.intra.peff.net","threadId":"41883","inReplyTo":"20160331123507.GC19857@collabora.co.uk","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T14:32:45Z","receivedAt":"2016-03-31T14:32:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 02:35:07PM +0200, Miklos Vajna wrote:\n\n> Hi,\n> \n> Some projects like LibreOffice don't use Signed-off-by, instead usually\n> use Gerrit for code review, and reviewers add a Reviewed-by line when\n> they are OK with a patch. In this workflow it's a bit unfortunate that\n> adding a Signed-off-by line is just a command-line switch, but adding a\n> Reviewed-by line is more complex.\n> \n> Is there anything in git that could help this situation? I didn't see\n> any related config option; I wonder if a patch would be accepted to make\n> the \"Signed-off-by\" line configurable, or there is a better way.\n\nThere's git-interpret-trailers, which can do the heavy lifting of adding\nit in the right place. But I don't know how you'd want to trigger it; it\nwould depend on the workflow that people use to add their signoff in the\nfirst place. I don't think there is anything as easy as \"git commit\n--amend -s\", but I'm not all that familiar with the interpret-trailers\ncode.\n\n-Peff\n"},{"id":"282314","messageId":"20160331143501.GE19857@collabora.co.uk","threadId":"41883","inReplyTo":"CAFZEwPMzcqrd8NEP6MH5saXL2KdUKAyN51uuoS5=aeU0aPWjJQ@mail.gmail.com","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Miklos Vajna","fromEmail":"vmiklos@collabora.co.uk","sentAt":"2016-03-31T14:35:02Z","receivedAt":"2016-03-31T14:35:02Z","isPatch":false,"sender":{"key":"vmiklos@collabora.co.uk","avatar":null},"body":"Hi,\n\nOn Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva wrote:\n> Are you suggesting to use a different email address for commiting,\n> signing off and reviewing?\n\nLet's say project A has a workflow where patch authors and maintainers\nadd a \"Signed-off-by: A B \" line. This is well-supported\nby git, various commands have a -s option to add that line.\n\nHowever, if project B has a workflow where patch authors add no such\nline, and reviewers add a \"Reviewed-by: A B \" line, then\nyou have to add that line manually when you do a review.\n\nI suggest to give a bit more support to this workflow in git. One way of\ndoing that would be to make the Signed-off-by string configurable. I can\nlook into implementing that, but first I wanted to discuss the idea here\non the list -- perhaps there is a better way to support that. :-)\n\nTyping that line (including copy&pasting your name + email all the time)\nis a bit boring.\n\nRegards,\n\nMiklos\n"},{"id":"282319","messageId":"56FD3ABC.2000500@gmail.com","threadId":"41883","inReplyTo":"20160331143501.GE19857@collabora.co.uk","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Sidhant Sharma","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-31T14:57:00Z","receivedAt":"2016-03-31T14:57:00Z","isPatch":false,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"Hi,\n\nOn Thursday 31 March 2016 08:05 PM, Miklos Vajna wrote:\n> Hi,\n>\n> On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva wrote:\n>> Are you suggesting to use a different email address for commiting,\n>> signing off and reviewing?\n> Let's say project A has a workflow where patch authors and maintainers\n> add a \"Signed-off-by: A B \" line. This is well-supported\n> by git, various commands have a -s option to add that line.\n>\n> However, if project B has a workflow where patch authors add no such\n> line, and reviewers add a \"Reviewed-by: A B \" line, then\n> you have to add that line manually when you do a review.\nWhen making the string configurable, would it be a good idea to\nsupport more than one sign-off strings? For instance, often patches\nhere in Git have both a Signed-Off and a Reviewed-by line. What would\nyou suggest for such a case?\n\n\nRegards,\nSidhant\n"},{"id":"282322","messageId":"CAP8UFD3UEPGA80C8OZ9uOg+Hsd3x7uJ-nJOQMGGDtB8a+3QCrA@mail.gmail.com","threadId":"41883","inReplyTo":"20160331143244.GD31116@sigill.intra.peff.net","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-31T15:02:12Z","receivedAt":"2016-03-31T15:02:12Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Mar 31, 2016 at 4:32 PM, Jeff King wrote:\n> On Thu, Mar 31, 2016 at 02:35:07PM +0200, Miklos Vajna wrote:\n>\n>> Hi,\n>>\n>> Some projects like LibreOffice don't use Signed-off-by, instead usually\n>> use Gerrit for code review, and reviewers add a Reviewed-by line when\n>> they are OK with a patch. In this workflow it's a bit unfortunate that\n>> adding a Signed-off-by line is just a command-line switch, but adding a\n>> Reviewed-by line is more complex.\n>>\n>> Is there anything in git that could help this situation? I didn't see\n>> any related config option; I wonder if a patch would be accepted to make\n>> the \"Signed-off-by\" line configurable, or there is a better way.\n>\n> There's git-interpret-trailers, which can do the heavy lifting of adding\n> it in the right place. But I don't know how you'd want to trigger it; it\n> would depend on the workflow that people use to add their signoff in the\n> first place. I don't think there is anything as easy as \"git commit\n> --amend -s\", but I'm not all that familiar with the interpret-trailers\n> code.\n\nThe plan was to make it possible for many commands, like commit,\ncherry-pick, am, and so on, to accept \"--trailer ...\" options and to\npass them to interpret-trailers that would process them. I remember\nstarting working on that and sending some patches at one point...\n"},{"id":"282324","messageId":"CAP8UFD0ot86bmuzxkfe8hzY1LmTiupre6h7QufkDpezT_fOsrA@mail.gmail.com","threadId":"41883","inReplyTo":"56FD3ABC.2000500@gmail.com","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-31T15:09:17Z","receivedAt":"2016-03-31T15:09:17Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Mar 31, 2016 at 4:57 PM, Sidhant Sharma wrote:\n> Hi,\n>\n> On Thursday 31 March 2016 08:05 PM, Miklos Vajna wrote:\n>> Hi,\n>>\n>> On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva wrote:\n>>> Are you suggesting to use a different email address for commiting,\n>>> signing off and reviewing?\n>> Let's say project A has a workflow where patch authors and maintainers\n>> add a \"Signed-off-by: A B \" line. This is well-supported\n>> by git, various commands have a -s option to add that line.\n>>\n>> However, if project B has a workflow where patch authors add no such\n>> line, and reviewers add a \"Reviewed-by: A B \" line, then\n>> you have to add that line manually when you do a review.\n> When making the string configurable, would it be a good idea to\n> support more than one sign-off strings? For instance, often patches\n> here in Git have both a Signed-Off and a Reviewed-by line. What would\n> you suggest for such a case?\n\n\"git interpret-trailers\" supports many kinds of trailers. There were a\nlot of related discussions/bikeshedding when it was designed and\nworked on.\n"},{"id":"282334","messageId":"xmqqtwjmpq6b.fsf@gitster.mtv.corp.google.com","threadId":"41883","inReplyTo":"20160331143501.GE19857@collabora.co.uk","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:28:44Z","receivedAt":"2016-03-31T16:28:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> Typing that line (including copy&pasting your name + email all the time)\n> is a bit boring.\n\nI think the last message from Christian in the thread points at the\nright direction in the future.\n\nThe internal \"parse the existing trailer block and manipulate it by\nadding, conditionally adding, replacing and deleting it\" logic was\ndone as an experimental \"interpret-trailers\" program, but polishing\nit (both its design and implementation) and integrating it to the\nfront-line programs (e.g. \"git commit\") hasn't been done.\n\nAs to the last step of \"integration\", we cannot use short-and-sweet\nsingle letter options like '-s' (for sign-off) for each and every\ncustom trailer different projects use for their own purpose (as\nthere are only 26 of the lowercase ASCII alphabet letters), so the\nmost general syntax for the option has to become \"--trailer \"\nor some variation of it, and at that point \"-s\" would look like a\nshort-hand for \"--trailer signed-off-by\".\n"},{"id":"282349","messageId":"20160331172104.GA1623@sigill.intra.peff.net","threadId":"41883","inReplyTo":"xmqqtwjmpq6b.fsf@gitster.mtv.corp.google.com","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T17:21:05Z","receivedAt":"2016-03-31T17:21:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:\n\n> As to the last step of \"integration\", we cannot use short-and-sweet\n> single letter options like '-s' (for sign-off) for each and every\n> custom trailer different projects use for their own purpose (as\n> there are only 26 of the lowercase ASCII alphabet letters), so the\n> most general syntax for the option has to become \"--trailer \"\n> or some variation of it, and at that point \"-s\" would look like a\n> short-hand for \"--trailer signed-off-by\".\n\nI can imagine it would be useful to give one short-and-sweet to \"add my\nstandard trailers\", where that standard set is defined in the config\nfile. But that is just a guess; I do not personally have a workflow\nwhere such standard trailers exist, beyond the normal s-o-b.\n\n-Peff\n"},{"id":"282350","messageId":"xmqqziteo92u.fsf@gitster.mtv.corp.google.com","threadId":"41883","inReplyTo":"20160331172104.GA1623@sigill.intra.peff.net","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T17:23:21Z","receivedAt":"2016-03-31T17:23:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:\n>\n>> As to the last step of \"integration\", we cannot use short-and-sweet\n>> single letter options like '-s' (for sign-off) for each and every\n>> custom trailer different projects use for their own purpose (as\n>> there are only 26 of the lowercase ASCII alphabet letters), so the\n>> most general syntax for the option has to become \"--trailer \"\n>> or some variation of it, and at that point \"-s\" would look like a\n>> short-hand for \"--trailer signed-off-by\".\n>\n> I can imagine it would be useful to give one short-and-sweet to \"add my\n> standard trailers\", where that standard set is defined in the config\n> file. But that is just a guess; I do not personally have a workflow\n> where such standard trailers exist, beyond the normal s-o-b.\n\nYup, I agree; I meant by \"some variation of it\" to cover such an\narrangement ;-)\n"},{"id":"282461","messageId":"20160401141036.GG800@collabora.co.uk","threadId":"41883","inReplyTo":"xmqqtwjmpq6b.fsf@gitster.mtv.corp.google.com","subject":"Re: Signed-off-by vs Reviewed-by","fromName":"Miklos Vajna","fromEmail":"vmiklos@collabora.co.uk","sentAt":"2016-04-01T14:10:36Z","receivedAt":"2016-04-01T14:10:36Z","isPatch":false,"sender":{"key":"vmiklos@collabora.co.uk","avatar":null},"body":"Hi,\n\nOn Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:\n> The internal \"parse the existing trailer block and manipulate it by\n> adding, conditionally adding, replacing and deleting it\" logic was\n> done as an experimental \"interpret-trailers\" program, but polishing\n> it (both its design and implementation) and integrating it to the\n> front-line programs (e.g. \"git commit\") hasn't been done.\n\nI had a look at interpret-trailers, and one use-case I miss is: being\nable to define a trailer type, but only add it when asked explicitly.\n\nExample:\n\n----\n$ git config trailer.review.key \"Reviewed-by: \"\n$ git config trailer.review.command 'echo \"$(git config user.name) <$(git config user.email)>\"'\n$ echo foo|git interpret-trailers\nfoo\n\nReviewed-by: A U Thor \n$ echo foo|git interpret-trailers --trailer review\nfoo\n\nReviewed-by: A U Thor \n----\n\nI can imagine e.g. a new configuration vaulue named\ntrailer..ifMissing explicit, and when that's set, the trailer\nwould be only added if it's spelled out explicitly using '--trailer\n'.\n\nDoes this sound like a good idea, or did I miss some way how this is\nalready possible? :-)\n\n> As to the last step of \"integration\", we cannot use short-and-sweet\n> single letter options like '-s' (for sign-off) for each and every\n> custom trailer different projects use for their own purpose (as\n> there are only 26 of the lowercase ASCII alphabet letters), so the\n> most general syntax for the option has to become \"--trailer \"\n> or some variation of it, and at that point \"-s\" would look like a\n> short-hand for \"--trailer signed-off-by\".\n\nHmm, I think the above has to be implemented first, otherwise it'll be\nhard to make \"-s\" an alias of \"--trailer signed-off-by\". (I mean having\ngit understand what \"signed-off-by\" is, still adding it conditionally.)\n\nRegards,\n\nMiklos\n"}]}