Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
tl;dr: This patch series wants to introduce a permanent new Git data format. The current version can write trailers in formats that it is incapable of reading, which I consider broken. I advocate a stricter specification of the format of trailers, at least until we get feedback from users that they need more flexibility. On 05/25/2014 10:37 AM, Christian Couder wrote: From: Michael Haggerty mhag...@alum.mit.edu [...] * A way for users to add information to commits for whatever purpose they want, without having to convince upstream to built support in. Yeah, I agree this is the main purpose of trailers. * A standard format for that information, so that all tools can agree how to read/write trailers without being confused by or breaking trailers that they didn't know about in advance. Yeah, but don't you think this goal can sometimes go against the previous goal? I mean, if some users for their project think that it's better, for example, if they use trailers like Fix #42 instead of Fix: 42, because their bug tracking system supports Fix #42 better, we should let them do what suits them better, even if Git supports them not as well as if they used Fix: 42. The flexibility that comes from offering our users a more-or-less general key/value store already accomplishes the first goal. With that the users *could* store their data as Fix: 42 or Fixes: #42 and satisfy their functional requirements. Giving them the option to use Fix #42 doesn't make *any* new functionality possible. It is pure eye-candy. And it would come at the IMO high cost of making it harder for *everybody* to work with the metadata. It makes the specification more complicated. It makes the code more complicated. It makes the configuration more complicated. It makes it more likely that there will be false positives (text in a commit message that our code recognizes as key/value data even though it was not meant to be). And in my opinion it makes the k/v data itself harder for a human to read because it is not in a uniform format. The only justification I can think of for allowing more flexible formats would be to retroactively support metadata that people already have in their history. Are there famous or important existing metadata formats that are incompatible with Key: Value? More to the point: do you have a concrete reason for wanting to support alternative formats like Fix #42, or is it based more on the feeling that users will want it? Remember, it would be really easy to release v1 of this feature with a strict format, then wait and see if users clamor for more flexibility. We can always add more flexibility later. Whereas if v1 already supports more flexible formats, we pretty much have to support them forever. * A format that is straightforward enough that it can be machine- readable with minimum ambiguity. Yeah, but again this could go against the main purpose of trailers above. No, the users have all the flexibility they need if they can choose their own key/value schema. Allowing alternative formats adds very little. I feel strongly that it would be a bad mistake to leave the specification of trailers so loose that they cannot be machine-readable with a good degree of confidence. Tools that add trailers should be composable. The current scheme is not. For example, suppose one tool wants to add a Fix #42 line and another one wants to add Signed-off-by: git config trailer.fix.key Fix # git config trailer.sign.key Signed-off-by: git config trailer.sign.ifexists doNothing echo subject Signed-off-by: Alice al...@example.com | git interpret-trailers --trailer fix=42 | git interpret-trailers --trailer sign=Bob b...@example.com - output -- subject Signed-off-by: Alice al...@example.com Fix #42 Signed-off-by: Bob b...@example.com --- The result is that the trailers end up not in one block but in two (meaning that the first block is no longer recognized as a trailer block), and the second Signed-off-by line, which should have been omitted because of ifexists=doNothing, was incorrectly added. Or let's do something like the commit template example from the documentation, but using Fix # instead of Fixes: : echo ***subject*** ***message*** Fix # Cc: Reviewed-by: Signed-off-by: | sed -Ee 's/(Reviewed-by.*)/\1me/' | git interpret-trailers --trim-empty --trailer git-version: foo -- output -- ***subject*** ***message*** Fix # Cc: Reviewed-by: me Signed-off-by: git-version: foo Not only haven't the empty lines been stripped off, but a new trailer block has been created for git-version. I consider this broken. [...] For
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On Tue, May 27, 2014 at 10:21 AM, Michael Haggerty mhag...@alum.mit.edu wrote: tl;dr: This patch series wants to introduce a permanent new Git data format. The current version can write trailers in formats that it is incapable of reading, which I consider broken. I advocate a stricter specification of the format of trailers, at least until we get feedback from users that they need more flexibility. On 05/25/2014 10:37 AM, Christian Couder wrote: [...] My opinion is that many Git developers have been engaged and you can see that in the Cc. I cannot tell if they are all very happy or not but I suppose that if they were very unhappy they would tell it. [...] It was unfair of me to try to characterize the opinions of other developers. On the other hand, even though many people have commented on this proposal over its long lifetime, I didn't get the feeling that it has won a consensus of +1s in its current form. I'd love to hear the opinion of others because maybe I'm totally out in left field. FWIW, after a quick read, I find myself agreeing very much with Michael's arguments for a stricter format (at least in its initial version). We are formalizing and applying tools/automation to a part of the commit message that has so far been ad hoc and very informal. There is no expectation that _every_ _single_ existing use of (informal) trailers (except the somewhat-formalized support for --signoff) must be supported by git-interpret-trailers. However, there _is_ an expectation that git-interpret-trailers is self-consistent and does not stumble over its own trailers. Therefore, it makes perfect sense to make v1 very strict in what formats it produce (i.e. a strict key: value format is enough for now). And I want to reiterate that the reason I'm so emphatic about this proposal is because I think it will be such a great new feature. I just think that some tweaks would make it a more solid foundation for building even more functionality onto. Fully agreed. git-interpret-trailers have come up in several other discussion, both on the git mailing list and elsewhere, and I have no doubt that this will be a very useful feature that will be put to good use in many projects. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: From: Michael Haggerty mhag...@alum.mit.edu ... An option like --input-separator might be enough to support this. For me this means: * Enumerating a list of allowed separators (e.g., [:=#]) Junio suggested in a message that users might use different separators like '%'. I actually think we shouldn't go any fancier than : and nothing else, not even #. I was hoping that you would eventually realize that there are only two viable extremes when I suggested the users may want to use other random characters like '%' and also the users can specify the 'key' with colon and trailing SP (in $gmane/245960). - If you want to give the projects greater control of the format, then you cannot rely on separators anyway. Your users can list all possible footer keys the particular project would use, so that they are recognized by Git, be that Fixes: 4a28f16, Bug #12354, without hard-coding what separator Git must pay attention to. You can easily find a run of lines that begin with any of the key (e.g. Fixes: , Signed-off-by: , Bug #, ...) starting from the tail-end of the log message and that is your footer block. No need for separators at all. - If you want to give the projects freedom to come up with random new kinds of footers without pre-arrangement, then you need to have a reliable way to say if any line you have never seen could be a footer material. A colon has been used everywhere, and used even in the Fixes: 4a28f16 example you took from the kernel circle. I think you presented it with '#' but I do not think they even want that, looking at: http://lists.linuxfoundation.org/pipermail/ksummit-discuss/2014-May/000618.html I also think that bug tracking system using Bug #12345 is an unrelated issue, as log viewers would want to highlight and make links out of them anywhere in the log message text, not limited to the log footer part. As to which one of these two we should take, I tend to think that we should start small and limited; loosening the syntax later is much easier than going the other way, i.e. : and nothing else. -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
From: Michael Haggerty mhag...@alum.mit.edu On 04/25/2014 11:07 PM, Christian Couder wrote: I don't think there is a lot of complexity. But maybe I need to explain how it works better. Feel free to suggest me sentences I could add. I am really excited about having better support for trailers in Git, and I want to thank you for your work. For me the promise of trailers is * A way for users to add information to commits for whatever purpose they want, without having to convince upstream to built support in. Yeah, I agree this is the main purpose of trailers. * A standard format for that information, so that all tools can agree how to read/write trailers without being confused by or breaking trailers that they didn't know about in advance. Yeah, but don't you think this goal can sometimes go against the previous goal? I mean, if some users for their project think that it's better, for example, if they use trailers like Fix #42 instead of Fix: 42, because their bug tracking system supports Fix #42 better, we should let them do what suits them better, even if Git supports them not as well as if they used Fix: 42. * A format that is straightforward enough that it can be machine- readable with minimum ambiguity. Yeah, but again this could go against the main purpose of trailers above. * Some command-line tools to make it easy for scripts to work with trailers, and that serve as a reference implementation that other Git implementations can imitate. Yeah, ok, as long as we keep in mind the main purpose. For example, I totally expect that we will soon want a command-line tool for inquiring about the presence and contents of trailers, for use in scripting. Eventually we will want to be able to do stuff like git trailers --get-all s-o-b origin/master..origin/next git rev-list --trailer=s-o-b:gits...@pobox.com master git trailers --pipe --draft \ --add-first fixes \ --append '# You can delete the following line:' \ --append s-o-b:$GIT_AUTHOR_NAME $GIT_AUTHOR_EMAIL \ --unset private git trailers --pipe --verify --tidy-up Yeah, feel free to help make this kind of things possible :-) I think it is really important to nail down the format of trailers tightly enough that everybody who reads/writes a commit message agrees about exactly what trailers are there. I think we should have a default format for trailers that is clear, but we should not force users to use this format. Because forcing it would go against the main goal of trailers that you listed first above. For example the specification might look something like: A commit message can optionally end with a block of trailers. The trailers, if present, must be separated from the rest of the commit message by one or more blank lines (lines that contain only whitespace). There must be no blank lines within the list of trailers. It is allowed to have blank lines after the trailers. Each trailer line must match the following Perl regular expression: ^([A-Za-z0-9_-]+)\s*:\s*(.*[^\s])\s*$ The string matching the first group is called the key and the string matching the second is called the value. Keys are considered to be case-insensitive [or should they be case-sensitive?]. The interpretation of values is left entirely up to the application. Values must not be empty. I tried to be clearer in the v12 I just posted, and I think it should be enough to be very clear. We might want to tweak a little the specifications later, so being too strict might be counter productive. And as other tools might already use trailers in a case-sensitive way and yet other tools in a case-insensitive way, I am not sure we would gain anything by specifying if keys or values should be interpreted in a case-sensitive or case-insensitive way. On the contrary we might upset people already using some of these tools for no good reason. However, in --draft and --cleanup modes, empty values *are* allowed, as are comments (lines starting with `core.commentchar`) within the trailer block. In --draft mode such lines are passed through unchanged, and in --cleanup mode such lines are removed. I am not sure we should use modes. I think options like --trim-empty, --allow-comments, --allow-empty might be clearer. I'm not saying this is the exact definition that we want; I'm just providing an example of the level of precision that I think is needed. Yeah, but I think too much precision can be counter productive. With regard to the separator character, my concern is not about how to document the rules for this one tool. It's more about having really well-defined rules that are consistent between reading and writing. For me it seems silly to let git interpret-trailers output trailers that it doesn't know how to read back in, and pretty much be a show-stopper if
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
From: Jeremy Morton ad...@game-point.net On 29/04/2014 12:47, Christian Couder wrote: Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) You may need to write a small script for that. Then you just need the trailer.m-o-b.command config value to point to your script. ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? You can script that too. But it would be nicer if the logic were built-in, then you wouldn't have to share some script with your work colleagues. :-) The above logic is very specific to your workflow. For example some people might a Made-on-branch: trailer only when they are on real branches except dev and master. 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On 28/04/2014 17:37, Junio C Hamano wrote: Christian Couderchrisc...@tuxfamily.org writes: From: Junio C Hamanogits...@pobox.com Christian Couderchrisc...@tuxfamily.org writes: ... + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. I assume that this is for things like bug #538 and the configuration would say something like: [trailer bug] key = bug # For completeness (of this example), the bog-standard s-o-b would look like Signed-off-by: Christian Couderchrisc...@tuxfamily.org and the configuration for it that spell the redundant key would be: [trailer Signed-off-by] key = Signed-off-by: Yeah, but you can use the following instead: [trailer s-o-b] key = Signed-off-by: One thing I'm not quite understanding is where the Christian Couderchrisc...@tuxfamily.org bit comes from. So you've defined the trailer token and key, but interpret-trailers then needs to get the value it will give for the key from somewhere. Does it have to just be hardcoded in? We probably want some way to get various variables like current branch name, current git version, etc. So in the case of always adding a trailer for the branch that the commit was checked in to at the time (Developed-on, Made-on-branch, Author-branch, etc. [I think my favourite is Made-on-branch]), you'd want something like: [trailer m-o-b] key = Made-on-branch: value = $currentBranch ... resulting in the trailer (for example): Made-on-branch: pacman-minigame Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? -- Best regards, Jeremy Morton (Jez) -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On Tue, Apr 29, 2014 at 1:05 PM, Jeremy Morton ad...@game-point.net wrote: On 28/04/2014 17:37, Junio C Hamano wrote: Christian Couderchrisc...@tuxfamily.org writes: From: Junio C Hamanogits...@pobox.com Christian Couderchrisc...@tuxfamily.org writes: ... + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. I assume that this is for things like bug #538 and the configuration would say something like: [trailer bug] key = bug # For completeness (of this example), the bog-standard s-o-b would look like Signed-off-by: Christian Couderchrisc...@tuxfamily.org and the configuration for it that spell the redundant key would be: [trailer Signed-off-by] key = Signed-off-by: Yeah, but you can use the following instead: [trailer s-o-b] key = Signed-off-by: One thing I'm not quite understanding is where the Christian Couderchrisc...@tuxfamily.org bit comes from. So you've defined the trailer token and key, but interpret-trailers then needs to get the value it will give for the key from somewhere. Does it have to just be hardcoded in? We probably want some way to get various variables like current branch name, current git version, etc. So in the case of always adding a trailer for the branch that the commit was checked in to at the time (Developed-on, Made-on-branch, Author-branch, etc. [I think my favourite is Made-on-branch]), you'd want something like: [trailer m-o-b] key = Made-on-branch: value = $currentBranch ... resulting in the trailer (for example): Made-on-branch: pacman-minigame In the documentation patch, there is: trailer.token.command:: This option can be used to specify a shell command that will be used to automatically add or modify a trailer with the specified 'token'. When this option is specified, it is like if a special 'token=value' argument is added at the end of the command line, where 'value' will be given by the standard output of the specified command. If the command contains the `$ARG` string, this string will be replaced with the 'value' part of an existing trailer with the same token, if any, before the command is launched. That's why Something like the following should work if git commit automitically runs git interpret-trailers: [trailer m-o-b] key = Made-on-branch: command = git name-rev --name-only HEAD Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) You may need to write a small script for that. Then you just need the trailer.m-o-b.command config value to point to your script. ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? You can script that too. 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On 29/04/2014 12:47, Christian Couder wrote: Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) You may need to write a small script for that. Then you just need the trailer.m-o-b.command config value to point to your script. ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? You can script that too. But it would be nicer if the logic were built-in, then you wouldn't have to share some script with your work colleagues. :-) -- Best regards, Jeremy Morton (Jez) -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On 29/04/2014 12:47, Christian Couder wrote: Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) You may need to write a small script for that. Then you just need the trailer.m-o-b.command config value to point to your script. ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? You can script that too. But it would be nicer if the logic were built-in, then you wouldn't have to share some script with your work colleagues. :-) -- Best regards, Jeremy Morton (Jez) -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On 04/25/2014 11:07 PM, Christian Couder wrote: From: Michael Haggerty mhag...@alum.mit.edu +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. Does this apply to existing trailers, new trailers, or both? Both. If it applies to existing trailers, then it seems a bit dangerous, in the sense that the command might end up changing trailers that are unrelated to the one that the command is trying to add. The command is not just for adding trailers. But there could be an option to just trim trailers that are added. Maybe that should be the *only* behavior of this option. Maybe there should be a trailer.token.trimEmpty config option. One possible usage of the git interpret-trailers command that was discussed in the early threads was the following: 1) You have a commit message template like the following: - ***SUBJECT*** ***MESSAGE*** Fixes: Cc: Cc: Reviewed-by: Reviewed-by: Signed-off-by: - [...etc...] Thanks for the explanation. Now the --trim-empty option makes a lot more sense. If a commit message containing trailer lines with separators other than ':' is input to the program, will it recognize them as trailer lines? No, '=' and '#' are not supported in the input message, only in the output. Do such trailer lines have to have the same separator as the one listed in this configuration setting to be recognized? No they need to have ':' as a separator. The reason why only ':' is supported is because it is the cannonical trailer separator and it could create problems with many input messages if other separators where supported. Maybe we could detect a special line like the following: # TRAILERS START in the input message and consider everyhting after that line as trailers. In this case it would be ok to accept other separators. It would be ugly to have to use such a line. I think it would be preferable to be more restrictive about trailer separators than to require something like this. The code is already very restrictive about trailer separators. From what you've said above, it sounds like your code might get confused with the following input commit message: This is the human-readable comment Foo: bar Fixes #123 Plugh: xyzzy It seems to me that none of these lines would be accepted as trailers, because they include a non-trailer Fixes line (non-trailer in the sense that it doesn't use a colon separator). Yeah, they would not be accepted because the code is very restrictive. The following would be accepted: Foo: bar Fixes: 123 Plugh: xyzzy I suppose that there is some compelling reason to allow non-colon separators here. If not, it seems like it adds a lot of complexity and should maybe be omitted, or limited to only a few specific separators. Yeah, but in the early threads concerning this subject, someone said that GitHub for example uses bug #XXX. I will have a look again. Yes, that's true: GitHub recognizes strings like fixes #33 but not if there is an intervening colon like in fixes: #33. OTOH GitHub recognizes such strings wherever they appear in the commit message (they don't have to be in trailer lines). So I'm not sure that the added complication is worth it if GitHub is the only use case. (And maybe we could convince GitHub to recognize Fixes: #33 if such syntax becomes the de-facto Git standard for trailers.) I don't think there is a lot of complexity. But maybe I need to explain how it works better. Feel free to suggest me sentences I could add. I am really excited about having better support for trailers in Git, and I want to thank you for your work. For me the promise of trailers is * A way for users to add information to commits for whatever purpose they want, without having to convince upstream to built support in. * A standard format for that information, so that all tools can agree how to read/write trailers without being confused by or breaking trailers that they didn't know about in advance. * A format that is straightforward enough that it can be machine- readable with minimum ambiguity. * Some command-line tools to make it easy for scripts to work with trailers, and that serve as a reference implementation that other Git implementations can imitate. For example, I totally expect that we will soon want a command-line tool for inquiring about the presence and contents of trailers, for use in scripting. Eventually we will want to be able to do stuff like git trailers --get-all s-o-b origin/master..origin/next git rev-list --trailer=s-o-b:gits...@pobox.com master git trailers --pipe --draft \ --add-first fixes \ --append '# You can delete the following line:' \ --append s-o-b:$GIT_AUTHOR_NAME $GIT_AUTHOR_EMAIL \
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: From: Junio C Hamano gits...@pobox.com Christian Couder chrisc...@tuxfamily.org writes: ... + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. I assume that this is for things like bug #538 and the configuration would say something like: [trailer bug] key = bug # For completeness (of this example), the bog-standard s-o-b would look like Signed-off-by: Christian Couder chrisc...@tuxfamily.org and the configuration for it that spell the redundant key would be: [trailer Signed-off-by] key = Signed-off-by: Yeah, but you can use the following instead: [trailer s-o-b] key = Signed-off-by: Sure, but note that both of these have a SP at the end in the value part (which I think is a sensible thing to do). The token and the key can be different. Am I reading the intention correctly? Yeah, I think so. That is, when trailer.token.key is not defined, the value defaults to token: (with one SP after the label and colon), Yes. and when it is defined, the value can come directly after it. The value can come directly after the key, only if the key ends with '#'. If it ends with something else, except spaces, one SP will be added between the key and the value. And I do not think we want (or even need) this only when it ends with # special casing in the code at all. When the project's convention is to say frotz# value-of-frotz, the users will specify that with 'key = frotz# ' (with a trailing SP in the value part), and in a project that wants 'nitfol %value-of-nitfol', your parser will find 'key = nitfol %'. The users will obtain the result they want for either case, and a hard-coded special casing in the code that only has incomplete knowledge on the project convention will actively harm them. I'd suggest dropping that special case. -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
From: Junio C Hamano gits...@pobox.com Christian Couder chrisc...@tuxfamily.org writes: +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. I think it is somewhat misleading to use the word headers like that. 'trailers' look similar to RFC-822-headers but they come at the end. The sentence however reads as if they are headers that look like RFC 822. Perhaps shuffling words like so: Help adding 'trailers' lines, that look similar to RFC 822 e-mail headers, at the end of the ... would make it less confusing. Ok, I made this change in v11. +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + Mental note: this does assume that the final output for the 'token' is to have a line label that is followed by a colon :, SP and the value. And the natural way to express that on the command line would be to say token: value, I would think, but let's just read on. +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. s/that are in RFC 822/for RFC 822 headers/. s/line breaking/line folding/. (see RFC 822, 3.1.1) Ok, it's in v11 too. +OPTIONS +--- +--trim-empty:: +If the 'value' part of any trailer contains only whitespace, +the whole trailer will be removed from the resulting message. + +CONFIGURATION VARIABLES +--- + +trailer.token.key:: +This 'key' will be used instead of 'token' in the As `key` is something that is typed literally, it should be typeset as `key` in the descriptive text. Ok, I used `key` in v11. I think other manpages spell the placeholder as `token` (or 'token', I am not sure which...). I found mostly token, so I used that in v11. +trailer. After some alphanumeric characters, it can contain +some non alphanumeric characters like ':', '=' or '#' that will +be used instead of ':' to separate the token from the value in +the trailer, though the default ':' is more standard. I assume that this is for things like bug #538 and the configuration would say something like: [trailer bug] key = bug # For completeness (of this example), the bog-standard s-o-b would look like Signed-off-by: Christian Couder chrisc...@tuxfamily.org and the configuration for it that spell the redundant key would be: [trailer Signed-off-by] key = Signed-off-by: Yeah, but you can use the following instead: [trailer s-o-b] key = Signed-off-by: The token and the key can be different. Am I reading the intention correctly? Yeah, I think so. That is, when trailer.token.key is not defined, the value defaults to token: (with one SP after the label and colon), Yes. and when it is defined, the value can come directly after it. The value can come directly after the key, only if the key ends with '#'. If it ends with something else, except spaces, one SP will be added between the key and the value. Yeah, I made '#' special in the hope that it would be more compatible with GitHub and other services that might also use '#'. 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
From: Michael Haggerty mhag...@alum.mit.edu On 04/08/2014 01:35 PM, Christian Couder wrote: The trailers are recognized in the input commit message using the following rules: - only lines that contains a ':' 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, - before them there must be at least one line with only spaces Thanks for all the explanation. I think that most/all of this information should be included in the documentation. Ok, I included the above rules in v11, but maybe not other pieces of information that you might have wanted. +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. Does this apply to existing trailers, new trailers, or both? Both. If it applies to existing trailers, then it seems a bit dangerous, in the sense that the command might end up changing trailers that are unrelated to the one that the command is trying to add. The command is not just for adding trailers. But there could be an option to just trim trailers that are added. Maybe that should be the *only* behavior of this option. Maybe there should be a trailer.token.trimEmpty config option. One possible usage of the git interpret-trailers command that was discussed in the early threads was the following: 1) You have a commit message template like the following: - ***SUBJECT*** ***MESSAGE*** Fixes: Cc: Cc: Reviewed-by: Reviewed-by: Signed-off-by: - 2) The user add some information when committing: $ git commit --trailer Fixes:534 --trailer Signed-off-by: Michael mhag...@alum.mit.edu 3) git interpret-trailers is used automatically by git commit without --trim-empty, and it is passed the --trailer arguments and the commit message template, so the user is shown the result which is for example the following: - ***SUBJECT*** ***MESSAGE*** Fixes: 534 Cc: Cc: Reviewed-by: Reviewed-by: Signed-off-by: Michael mhag...@alum.mit.edu - 4) The user adds some information and the resulting message is for example: - Doing foo and bar And also baz. Fixes: 534 Cc: Cc: Peff p...@peff.net Reviewed-by: Junio gits...@pobox.com Reviewed-by: Signed-off-by: Michael mhag...@alum.mit.edu - 5) Then a post commit hook automatically uses git interpret-trailers --trim-empty on the result, so the commit message is eventually the following: - Doing foo and bar And also baz. Fixes: 534 Cc: Peff p...@peff.net Reviewed-by: Junio gits...@pobox.com Signed-off-by: Michael mhag...@alum.mit.edu - So I think it could be very useful to have --trim-empty work on all the trailers, not just those passed as arguments. If a commit message containing trailer lines with separators other than ':' is input to the program, will it recognize them as trailer lines? No, '=' and '#' are not supported in the input message, only in the output. Do such trailer lines have to have the same separator as the one listed in this configuration setting to be recognized? No they need to have ':' as a separator. The reason why only ':' is supported is because it is the cannonical trailer separator and it could create problems with many input messages if other separators where supported. Maybe we could detect a special line like the following: # TRAILERS START in the input message and consider everyhting after that line as trailers. In this case it would be ok to accept other separators. It would be ugly to have to use such a line. I think it would be preferable to be more restrictive about trailer separators than to require something like this. The code is already very restrictive about trailer separators. From what you've said above, it sounds like your code might get confused with the following input commit message: This is the human-readable comment Foo: bar Fixes #123 Plugh: xyzzy It seems to me that none of these lines would be accepted as trailers, because they include a non-trailer Fixes line (non-trailer in the sense that it doesn't use a colon separator). Yeah, they would not be accepted because the code is very restrictive. The following would be accepted: Foo: bar Fixes: 123 Plugh: xyzzy I suppose that there is some compelling reason to allow non-colon separators here. If not, it seems like it adds a lot of complexity and should maybe be omitted, or limited to only a few specific separators. Yeah, but in the early threads concerning this subject, someone said that GitHub for example uses bug #XXX. I will have a look again. Yes, that's true: GitHub recognizes strings like fixes #33 but not if there is an intervening colon like in fixes: #33. OTOH GitHub recognizes
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Sorry for reappearing in this thread after such a long absence. I wanted to see what is coming up (I think this interpret-trailers command will be handy!) so I read this documentation patch carefully, and added some questions and suggestions below. On 04/06/2014 07:02 PM, Christian Couder wrote: Signed-off-by: Christian Couder chrisc...@tuxfamily.org Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/git-interpret-trailers.txt | 123 +++ 1 file changed, 123 insertions(+) create mode 100644 Documentation/git-interpret-trailers.txt diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt new file mode 100644 index 000..75ae386 --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,123 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [(token[(=|:)value])...] + +DESCRIPTION +--- +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. + +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. s/emited/emitted/ + +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + + +By default, if there are already trailers with the same 'token', the +new trailer will appear just after the last trailer with the same +'token'. Otherwise it will appear at the end of the message. How are existing trailers recognized in the input commit message? Do trailers have to be configured to be recognized? Or are all lines matching a specific pattern considered trailers? If so, it might be helpful to include a regexp here that describes the trailer syntax. What about blank lines? I see that you try to add a blank line before new trailers. But what about on input? Do the trailer lines have to be separated from the free-form comment by a blank line to be recognized? What if there are blank lines between trailer lines, or after them? Is it allowed to have non-trailer lines between or after trailer lines? + +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. + +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. Does this apply to existing trailers, new trailers, or both? If it applies to existing trailers, then it seems a bit dangerous, in the sense that the command might end up changing trailers that are unrelated to the one that the command is trying to add. + +CONFIGURATION VARIABLES +--- + +trailer.token.key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain Trailer keys can also contain '-', right? + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. Above it looks like the default separator is not ':' but rather ': ' (with a space). Is the space always added regardless of the value of this configuration variable, or should the configuration value include the trailing space if it is desired? Is there any way to get a trailer that doesn't include a space, like foo=bar ? (Changing this to foo= bar would look pretty ugly.) If a commit message containing trailer lines with separators other than ':' is input to the program, will it recognize them as trailer lines? Do such trailer lines have to have the same separator as the one listed in this configuration setting to be recognized? I suppose that there is some compelling reason to allow non-colon separators here. If not, it seems like it adds a lot of complexity and should maybe be omitted, or limited to only a few specific separators. + +trailer.token.where:: + This can be either `after`, which is the
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On Tue, Apr 8, 2014 at 9:30 AM, Michael Haggerty mhag...@alum.mit.edu wrote: +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. s/emited/emitted/ Ok. +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + + +By default, if there are already trailers with the same 'token', the +new trailer will appear just after the last trailer with the same +'token'. Otherwise it will appear at the end of the message. How are existing trailers recognized in the input commit message? Do trailers have to be configured to be recognized? Or are all lines matching a specific pattern considered trailers? If so, it might be helpful to include a regexp here that describes the trailer syntax. The trailers are recognized in the input commit message using the following rules: - only lines that contains a ':' 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, - before them there must be at least one line with only spaces What about blank lines? I see that you try to add a blank line before new trailers. But what about on input? One line with only spaces has to be before the trailers. Some can be after the trailers. Do the trailer lines have to be separated from the free-form comment by a blank line to be recognized? Yes. What if there are blank lines between trailer lines, or after them? After them is ok. Between is not ok (only the trailers after the blank lines will be recognized). Is it allowed to have non-trailer lines between or after trailer lines? No except lines with spaces after the trailers lines. +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. + +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. Does this apply to existing trailers, new trailers, or both? Both. If it applies to existing trailers, then it seems a bit dangerous, in the sense that the command might end up changing trailers that are unrelated to the one that the command is trying to add. The command is not just for adding trailers. But there could be an option to just trim trailers that are added. +CONFIGURATION VARIABLES +--- + +trailer.token.key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain Trailer keys can also contain '-', right? Yes. I should have written after the last alphanumeric character. I will fix that. + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. Above it looks like the default separator is not ':' but rather ': ' (with a space). Is the space always added regardless of the value of this configuration variable, or should the configuration value include the trailing space if it is desired? Is there any way to get a trailer that doesn't include a space, like foo=bar ? (Changing this to foo= bar would look pretty ugly.) I will have a look, but I think that: - a space is always added after ':' or '=', - a space is never added after '#', - it doesn't matter if there is a space or not in the configured key. If a commit message containing trailer lines with separators other than ':' is input to the program, will it recognize them as trailer lines? No, '=' and '#' are not supported in the input message, only in the output. Do such trailer lines have to have the same separator as the one listed in this configuration setting to be recognized? No they need to have ':' as a separator. The reason why only ':' is supported is because it is the cannonical trailer separator and it could create problems with many input messages if other separators where supported. Maybe we could detect a special line like the following: # TRAILERS START in the input message and consider everyhting after
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On 04/08/2014 01:35 PM, Christian Couder wrote: On Tue, Apr 8, 2014 at 9:30 AM, Michael Haggerty mhag...@alum.mit.edu wrote: How are existing trailers recognized in the input commit message? Do trailers have to be configured to be recognized? Or are all lines matching a specific pattern considered trailers? If so, it might be helpful to include a regexp here that describes the trailer syntax. The trailers are recognized in the input commit message using the following rules: - only lines that contains a ':' 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, - before them there must be at least one line with only spaces Thanks for all the explanation. I think that most/all of this information should be included in the documentation. +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. Does this apply to existing trailers, new trailers, or both? Both. If it applies to existing trailers, then it seems a bit dangerous, in the sense that the command might end up changing trailers that are unrelated to the one that the command is trying to add. The command is not just for adding trailers. But there could be an option to just trim trailers that are added. Maybe that should be the *only* behavior of this option. Maybe there should be a trailer.token.trimEmpty config option. +CONFIGURATION VARIABLES +--- + +trailer.token.key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain Trailer keys can also contain '-', right? Yes. I should have written after the last alphanumeric character. I will fix that. + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. Above it looks like the default separator is not ':' but rather ': ' (with a space). Is the space always added regardless of the value of this configuration variable, or should the configuration value include the trailing space if it is desired? Is there any way to get a trailer that doesn't include a space, like foo=bar ? (Changing this to foo= bar would look pretty ugly.) I will have a look, but I think that: - a space is always added after ':' or '=', - a space is never added after '#', - it doesn't matter if there is a space or not in the configured key. If a commit message containing trailer lines with separators other than ':' is input to the program, will it recognize them as trailer lines? No, '=' and '#' are not supported in the input message, only in the output. Do such trailer lines have to have the same separator as the one listed in this configuration setting to be recognized? No they need to have ':' as a separator. The reason why only ':' is supported is because it is the cannonical trailer separator and it could create problems with many input messages if other separators where supported. Maybe we could detect a special line like the following: # TRAILERS START in the input message and consider everyhting after that line as trailers. In this case it would be ok to accept other separators. It would be ugly to have to use such a line. I think it would be preferable to be more restrictive about trailer separators than to require something like this. From what you've said above, it sounds like your code might get confused with the following input commit message: This is the human-readable comment Foo: bar Fixes #123 Plugh: xyzzy It seems to me that none of these lines would be accepted as trailers, because they include a non-trailer Fixes line (non-trailer in the sense that it doesn't use a colon separator). I suppose that there is some compelling reason to allow non-colon separators here. If not, it seems like it adds a lot of complexity and should maybe be omitted, or limited to only a few specific separators. Yeah, but in the early threads concerning this subject, someone said that GitHub for example uses bug #XXX. I will have a look again. Yes, that's true: GitHub recognizes strings like fixes #33 but not if there is an intervening colon like in fixes: #33. OTOH GitHub recognizes such strings wherever they appear in the commit message (they don't have to be in trailer lines). So I'm not sure that the added complication is worth it if GitHub is the only use case. (And maybe we could convince GitHub to recognize Fixes: #33 if such syntax becomes the de-facto Git standard for trailers.) +trailer.token.where:: + This can be either `after`, which is the default, or + `before`. If it is `before`, then a trailer with the specified + token, will
Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Michael Haggerty mhag...@alum.mit.edu writes: Sorry for reappearing in this thread after such a long absence. I wanted to see what is coming up (I think this interpret-trailers command will be handy!) so I read this documentation patch carefully, and added some questions and suggestions below. Thanks for reading the patch carefully. It helps to have fresh set of eyes that are not contaminated by the preconception formed by previous discussions, especially when reviewing the documentation whose primary target audiences are those who do not care about these previous back-and-forth. +trailer.token.where:: +This can be either `after`, which is the default, or +`before`. If it is `before`, then a trailer with the specified +token, will appear before, instead of after, other trailers +with the same token, or otherwise at the beginning, instead of +at the end, of all the trailers. Brainstorming: some other options that might make sense here someday: ... +trailer.token.ifexist:: +This option makes it possible to choose what action will be +performed when there is already at least one trailer with the +same token in the message. ++ +The valid values for this option are: `addIfDifferent` (this is the +default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`. Are these option values case sensitive? It is interesting and somewhat sad that it all has to come back together inter-twined. From the very beginning, I was opposed to having logical complexity that requires multi-words in both variable names (e.g. if-exist) and values (e.g. add-if-different), and after $gmane/241929 where I let the devil's advocate how about making the variable simpler without logical operation and put all the conditional on the value side? suggestion shot down, I somehow was hoping that the value part got a lot simpler not to require multi-words, which would have meant that we would not have to worry about Is it addIfDifferent? add-if-different? or Add_If_Different? at all. Sadly that is not what we have ended up with. So, with that realization... If so, it might be a little bit confusing because the same camel-case is often used in documentation for configuration *keys*, which are not case sensitive [1], and users might have gotten used to thinking of strings that look like this to be non-case-sensitive. ... very true. Having to have these enum values as so complex to require multi-words is probably the root cause of the confusion, and we might probably be better off if we did not have to, but it would be helpful to allow various different spellings (i.e. make them case insensitive to allow random camel spellings, and also accept things like add-if-different as well) if we absolutely have to have these complex values. But you had a lot of good questions and suggestions for possible future enhancements that we would need to take into account while designing the overall scheme to later allow them to fit into. Maybe a value that is a single-token that consists of just a few words (e.g. addIfDifferent) may not be the best way to go after all. I dunno. What if there are multiple existing trailers with the same token? Are they all overwritten? ... What if the key appears multiple times in existing trailers? All good questions, I would think. -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. I think it is somewhat misleading to use the word headers like that. 'trailers' look similar to RFC-822-headers but they come at the end. The sentence however reads as if they are headers that look like RFC 822. Perhaps shuffling words like so: Help adding 'trailers' lines, that look similar to RFC 822 e-mail headers, at the end of the ... would make it less confusing. +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + Mental note: this does assume that the final output for the 'token' is to have a line label that is followed by a colon :, SP and the value. And the natural way to express that on the command line would be to say token: value, I would think, but let's just read on. +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. s/that are in RFC 822/for RFC 822 headers/. s/line breaking/line folding/. (see RFC 822, 3.1.1) +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. + +CONFIGURATION VARIABLES +--- + +trailer.token.key:: + This 'key' will be used instead of 'token' in the As `key` is something that is typed literally, it should be typeset as `key` in the descriptive text. I think other manpages spell the placeholder as `token` (or 'token', I am not sure which...). + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. I assume that this is for things like bug #538 and the configuration would say something like: [trailer bug] key = bug # For completeness (of this example), the bog-standard s-o-b would look like Signed-off-by: Christian Couder chrisc...@tuxfamily.org and the configuration for it that spell the redundant key would be: [trailer Signed-off-by] key = Signed-off-by: Am I reading the intention correctly? That is, when trailer.token.key is not defined, the value defaults to token: (with one SP after the label and colon), and when it is defined, the value can come directly after it. -- 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
