Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
On Mon, Jun 30, 2014 at 1:57 PM, Jakub Narębski jna...@gmail.com wrote: Christian Couder wrote: + + +* Configure a 'sign' trailer with a command to automatically add a + 'Signed-off-by: ' with the author information only if there is no + 'Signed-off-by: ' already, and show how it works: ++ + +$ git config trailer.sign.key Signed-off-by: +$ git config trailer.sign.ifmissing add +$ git config trailer.sign.ifexists doNothing +$ git config trailer.sign.command 'echo $(git config user.name) $(git config user.email)' +$ git interpret-trailers EOF + EOF How to configure git-interpret-trailers command so that it follow current rules for DCO: * Signed-off-by: is always at bottom; we can have signoff+signoff+ack+signoff * Signed-off-by: can repeat itself with the same author; this denotes steps in coming up with current version of the patch. * but we shouldn't repeat the same signoff one after another Right now something like: signoff+signoff+ack+signoff is not supported. It could be, if someone implements more options for the trailer.token.where config variable. Right now the only options are after and before, but it could be possible to have end and start too. And maybe end should be the default instead of after. When I first worked on this series I was under the impression that people wanted to group all the trailers with the same token together. So we want to allow this: Signed-off-by: A U Thor aut...@example.com Signed-off-by: Joe R. Hacker j...@hacker.com Acked-by: D E Veloper develo...@example.com Signed-off-by: C O Mitter commit...@example.com but prevent this Signed-off-by: C O Mitter commit...@example.com Signed-off-by: C O Mitter commit...@example.com This can be prevented by using addIfDifferentNeighbor, for example like this: $ git config trailer.sign.ifexists addIfDifferentNeighbor So I think the full config for what you want would be something like: $ git config trailer.sign.key Signed-off-by: $ git config trailer.sign.ifmissing add $ git config trailer.sign.ifexists addIfDifferentNeighbor $ git config trailer.sign.where end $ git config trailer.sign.command 'echo $(git config user.name) $(git config user.email)' $ git config trailer.ack.key Acked-by: $ git config trailer.ack.ifmissing add $ git config trailer.ack.ifexists addIfDifferentNeighbor $ git config trailer.ack.where end Best, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: While I agree with Michael on the other thread that we should limit the syntax and start with ':' only, if you really want to allow random syntax like Bug #12345 and Acked-by= Peff, for which you have demonstrations in the tests in the other patch, the above rule should be updated to also allow prefix matches to possible set of keys defined by the user, so that an existing line that is produced by your tool, e.g. Acked-by= Peff, can be picked up as matching with some token having a key Acked-by= . Otherwise, the input side of your tool is inconsistent with the output side of your own tool, and that will make the flexiblity of the output side useless. I don't think that the flexibility of the output side would be useless. Flexibility is useful, only if you can control it. We already emit stuff like: (cherry picked from commit f72baf07969242882128aff4c95ec8059e7fd054) and we don't care about any input side when we do that. That is something you may want to _fix_, not take as an excuse to make things worse, no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder wrote: + + +* Configure a 'sign' trailer with a command to automatically add a + 'Signed-off-by: ' with the author information only if there is no + 'Signed-off-by: ' already, and show how it works: ++ + +$ git config trailer.sign.key Signed-off-by: +$ git config trailer.sign.ifmissing add +$ git config trailer.sign.ifexists doNothing +$ git config trailer.sign.command 'echo $(git config user.name) $(git config user.email)' +$ git interpret-trailers EOF + EOF How to configure git-interpret-trailers command so that it follow current rules for DCO: * Signed-off-by: is always at bottom; we can have signoff+signoff+ack+signoff * Signed-off-by: can repeat itself with the same author; this denotes steps in coming up with current version of the patch. * but we shouldn't repeat the same signoff one after another So we want to allow this: Signed-off-by: A U Thor aut...@example.com Signed-off-by: Joe R. Hacker j...@hacker.com Acked-by: D E Veloper develo...@example.com Signed-off-by: C O Mitter commit...@example.com but prevent this Signed-off-by: C O Mitter commit...@example.com Signed-off-by: C O Mitter commit...@example.com IIRC -- Jakub Narębski -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
From: Junio C Hamano gits...@pobox.com Christian Couder chrisc...@tuxfamily.org writes: +The trailers are recognized in the input message using the following +rules: + +* only lines that contains a ':' (colon) are considered trailers, + +* the trailer lines must all be next to each other, + +* after them it's only possible to have some lines that contain only + spaces, and then a patch; the patch part is recognized using the + fact that its first line starts with '---' (three minus signs), + +* before them there must be at least one line with only spaces. While I agree with Michael on the other thread that we should limit the syntax and start with ':' only, if you really want to allow random syntax like Bug #12345 and Acked-by= Peff, for which you have demonstrations in the tests in the other patch, the above rule should be updated to also allow prefix matches to possible set of keys defined by the user, so that an existing line that is produced by your tool, e.g. Acked-by= Peff, can be picked up as matching with some token having a key Acked-by= . Otherwise, the input side of your tool is inconsistent with the output side of your own tool, and that will make the flexiblity of the output side useless. I don't think that the flexibility of the output side would be useless. We already emit stuff like: (cherry picked from commit f72baf07969242882128aff4c95ec8059e7fd054) and we don't care about any input side when we do that. So being able to emit lot of different stuff is valuable even if we are not yet able to parse it. For example what if people wanted cherry-pick messages written like: Cherry picked from f72baf0796 (do this and that, 2014-01-01) we just cannot let them have the above if we decide that ':' has to always be used as the separator. We also emit stuff like Merge commit '71260bf' or Merge tag 'mystuff' or Merge branch 'dev' and we don't let people customize this and we don't care about any input side. And when there is a merge conflict we emit: Conflicts: file1 file2 ... instead of: Conflicts: file1 Conflicts: file2 ... Of course at least for cherry-pick it would have been nice if since the beginning we would have written something in a canonical trailer way like: Cherry-picked-from: f72baf0796 This way we could now use git interpret-trailers to both emit the above and to read it. But it is still be better to just be able to emit it than to not be able to do anything about it. Because if we are able to emit it with git interpret-trailers, then we can let people customize how it is emitted, and this might be enough for many people. Also now those who are ok to output it using the canonical way, could now configure that. And this is not just for us, something like: fixes #42 (do this and that, 2014-01-01) for example could be nice for both Github and human beings. Thanks, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: +The trailers are recognized in the input message using the following +rules: + +* only lines that contains a ':' (colon) are considered trailers, + +* the trailer lines must all be next to each other, + +* after them it's only possible to have some lines that contain only + spaces, and then a patch; the patch part is recognized using the + fact that its first line starts with '---' (three minus signs), + +* before them there must be at least one line with only spaces. While I agree with Michael on the other thread that we should limit the syntax and start with ':' only, if you really want to allow random syntax like Bug #12345 and Acked-by= Peff, for which you have demonstrations in the tests in the other patch, the above rule should be updated to also allow prefix matches to possible set of keys defined by the user, so that an existing line that is produced by your tool, e.g. Acked-by= Peff, can be picked up as matching with some token having a key Acked-by= . Otherwise, the input side of your tool is inconsistent with the output side of your own tool, and that will make the flexiblity of the output side useless. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 11/11] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: +trailer.token.key:: + This `key` will be used instead of token in the + trailer. After the last alphanumeric character, this variable + can contain some non alphanumeric characters, like for example + `'%'` (one percent sign), `' = '` (one space followed by one + equal sign and then one space), `' #'` (one space followed by + one number sign) or even just `' '` (one space), that will be + used to separate the token from the value in the + trailer. The default separator, `': '` (one colon followed by + one space), which is the usual standard, is added only if the + last character in `key` is alphanumeric, so watch out for + unwanted trailing spaces in this variable. Perhaps corollary to the other review comment to this patch, I think this is overly complex without giving more value to the users than it causes confusion. If the goal is to allow random syntax on the output side, why do you even need to list percent, pound, etc., when you can just say The key is emitted and then your value and the user will get exactly what they specified to be emitted? Any magic applied on top (namely, we would add : only in certain circumstances) only makes things unnecessarily complex; I think your having to say so watch out for... is a clear indication of that. If we use the separator, so that we can recognise a line with an unseen key as a trailer we do not know about, we should stick to just a single standard one ':' and I think it is OK to add a missing : by magic if that is the direction we are going to take, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
