Re: [RFC/PATCH] Add interpret-trailers builtin
On Wed, Nov 6, 2013 at 9:42 PM, Junio C Hamano gits...@pobox.com wrote: Christian Couder chrisc...@tuxfamily.org writes: From: Junio C Hamano gits...@pobox.com But that is insufficient to emulate what we do, no? I.e. append unless the last one is from the same person we are about to add. Yeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using: [trailer signoff] key = Signed-off-by: if_exist = dont_repeat_previous if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' Anything is possible, but possible does not justify it is better way than other possible ways. What are the plausible values for if_missing? If if_missing needs prepend, for example, in addition to append, does it mean if_exist also needs corresponding prepend variant for the value dont_repeat_previous you would give to if_exist? Yeah, we could add prepend to the possible values for if_missing and also other values that prepend instead of append to if_exist. But I am not sure they would be as useful. If we think they are useful, then maybe we could instead add another configuration to say if we want to append or prepend or put in the middle, and change the others to make them clearer. For example we could have: where = (after | middle | before) if_exist = (add_if_different | add_if_neighbor_different | add | overwrite | do_nothing) if_missing = (do_nothing | add) (The default being the first choice.) Having two that are seemingly independent configuration does not seem to be helping in reducing complexity (by keeping settings that can be independently set orthogonal, by saying if the other rule decides to add, do we append, prepend, insert at the middle?, for example). I am not sure I understand what you mean. In my opinion, if we want to use just one configuration, instead of 2 or 3, it will not reduce complexity. Maybe we could parse something like: style = if_exist:add_if_different:after, if_missing:add:before or like: style = (append | prepend | insert | append_unless regexp | prepend_unless regexp | insert_unless regexp) with regexp being a regexp where $KEY is the key and $VALUE is the value, so you can say: append_unless ^$KEY[:=]$VALUE.* or like: style = (append_if cmd | prepend_if cmd | insert_if cmd) with cmd being a shell command that should exit with code 0. (The command would be passed the existing trailers in stdin. So you could say things like: append_if true or append_if tail -1 | grep -v '$KEY: $VALUE') But if we want to keep things simple, I think what I suggest first above is probably best. And by the way, later we might add add_if_match regexp or add_if_true cmd to if_exist and if_missing, so we could still be as powerful as the other styles. How would one differentiate between there is a field with that key and there is a field with that key, value combination with a single if_exist? Add another variable if_exist_exact? if_exist = do_nothing would mean: do nothing if there is a field with that key if_exist = overwrite would mean: overwrite the existing value of a field with that key if_exist = add would mean: add if there is one or more fields with that key (whatever the value(s), so the value(s) could be the same) if_exist = add_if_different would mean: add only if there is no field with the same key, value pair if_exist = add_if_different_neighbor would mean: add only if there is no field with the same key, value pair where we are going to add if_missing = do_nothing would mean: do nothing if there is no field with that key if_missing= add would mean: add if there is no field with that key where = after would mean: if we decide to add, we will put the trailer after the other trailers where = middle would mean: if we decide to add, we will put the trailer in the middle of the other trailers where = before would mean: if we decide to add, we will put the trailer before the other trailers In my opinion, that should be enough for most use cases. It is true that some people might want something more complex (like adding only if there is no value for the same key that match a given regexp) or something stranger like adding only if there is already a field with the same key, value pair. But we can take care of these special cases later if they actually happen. Then we will hopefully have more experience. And we could add things like add_if_no_value_match regexp or add_if cmd to if_exist and if_missing . 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: [RFC/PATCH] Add interpret-trailers builtin
On Wed, Nov 6, 2013 at 7:43 AM, Christian Couder chrisc...@tuxfamily.org wrote: Of course in the latter case, a command should probably be specified to tell which value should be used with the key. For example: [trailer signoff] key = Signed-off-by: if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' would append a s-o-b line only if there is no s-o-b already. Sorry, I realize that I was wrong above. As the default for if_exist is dont_repeat, the above would append a s-o-b if there is no s-o-b or if there is one but with a different value. To append a s-o-b only if there is no s-o-b already, one would need to use: [trailer signoff] key = Signed-off-by: if_exist = dont_append if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' 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: [RFC/PATCH] Add interpret-trailers builtin
Christian Couder christian.cou...@gmail.com writes: To append a s-o-b only if there is no s-o-b already, one would need to use: [trailer signoff] key = Signed-off-by: if_exist = dont_append if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' But that is insufficient to emulate what we do, no? I.e. append unless the last one is from the same person we are about to add. -- 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: [RFC/PATCH] Add interpret-trailers builtin
From: Junio C Hamano gits...@pobox.com Christian Couder christian.cou...@gmail.com writes: To append a s-o-b only if there is no s-o-b already, one would need to use: [trailer signoff] key = Signed-off-by: if_exist = dont_append if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' But that is insufficient to emulate what we do, no? I.e. append unless the last one is from the same person we are about to add. Yeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using: [trailer signoff] key = Signed-off-by: if_exist = dont_repeat_previous if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' Cheers, 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: [RFC/PATCH] Add interpret-trailers builtin
Christian Couder chrisc...@tuxfamily.org writes: From: Junio C Hamano gits...@pobox.com Christian Couder christian.cou...@gmail.com writes: To append a s-o-b only if there is no s-o-b already, one would need to use: [trailer signoff] key = Signed-off-by: if_exist = dont_append if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' But that is insufficient to emulate what we do, no? I.e. append unless the last one is from the same person we are about to add. Yeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using: [trailer signoff] key = Signed-off-by: if_exist = dont_repeat_previous if_missing = append command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL' Anything is possible, but possible does not justify it is better way than other possible ways. What are the plausible values for if_missing? If if_missing needs prepend, for example, in addition to append, does it mean if_exist also needs corresponding prepend variant for the value dont_repeat_previous you would give to if_exist? Having two that are seemingly independent configuration does not seem to be helping in reducing complexity (by keeping settings that can be independently set orthogonal, by saying if the other rule decides to add, do we append, prepend, insert at the middle?, for example). How would one differentiate between there is a field with that key and there is a field with that key, value combination with a single if_exist? Add another variable if_exist_exact? -- 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: [RFC/PATCH] Add interpret-trailers builtin
Johan Herland jo...@herland.net writes:
On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano gits...@pobox.com wrote:
Johan Herland jo...@herland.net writes:
+{
+ char *end = strchr(arg, '=');
+ if (!end)
+ end = strchr(arg, ':');
So both '=' (preferred) and ':' are accepted as field/value
separators. That's ok for the command-line, I believe.
Why?
Sometimes you have to be loose from the beginning _if_ some existing
uses and established conventions make it easier for the users, but
if you do not have to start from being loose, it is almost always a
mistake to do so. The above code just closed the door to use :
for some other useful purposes we may later discover, and will make
us regret for doing so.
Although I agree with the principle, I think there are (at least) two
established conventions that will be commonly used from the start, and
that we should support:
- Using short forms with '=', e.g. ack=Peff. There is already a
convention on how we specify name + value pairs on the command
line, e.g. git -c foo=bar ...
I do not have much problem with this.
- Copy-pasting footers from existing commit messages. These will have
the same format as the expected output of this command, and not
accepting the same format in its input seems silly, IMHO.
I am not sure about this, but syntactically, it is very similar to
--message CC: Johan Herland j...@h.net
so probably it is OK, but then we do not even have to limit it to
colon, no? E.g. appending an arbitrary footer, with its literal
value, may be done with something like:
--footer CC: Johan Herland j...@h.net
--footer Closes #12345
Also, there is a distinction between fields with the same field name
appearing twice and fields with the same field name and the same
field value appearing twice. Some headers do want to have them, some
do not.
True. This complexity is partly why I initially wanted to leave this
whole thing up to hooks, but I guess now we have to deal with it...
If we are adding anything, it has to be able to express what we
already do for Signed-off-by: line, so we cannot start from
somewhere any simpler than this, I am afraid.
--
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: [RFC/PATCH] Add interpret-trailers builtin
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
* trailer seems better than commitTrailer as the config key because
it looks like all the config keys are lower case and committrailer is not
very readable.
And closes the door for other things from later acquiring trailers?
I don't think it really closes the door, as they can have a name like
stufftrailer then. Or better they could call it something else than
trailer everywhere from the beginning to avoid mistakes in the first
place.
Or if they are really trailers, for example maybe tag trailers, then
in many cases they might want to share the same configuration as
commit trailers. In this case, it would be a mistake to use
commitTrailer when most people would like them to also apply to
tags.
If/when tag trailers appear, then we can decide that trailer is
common for all trailers, commitTrailer is for commits only and
tagTrailer for tags only.
And anyway commit trailers have existed since the beginning or nearly
the beginning of Git, and we haven't seen yet any other kind of
trailer, so it's reasonnable to think that we can safely take the name
and not worry, be happy.
* trailer.token.value looks better than trailer.token.trailer, so
I chose the former.
If that is a literal value, then I think .value is indeed good.
That was what I thought first too.
But, after thinking about what Johan said, I think that it might be
confusing for some people, so now I wonder if I should call it key.
* Rather than only one trailer.token.style config option, it seems
better to me to have both trailer.token.if_exist and
trailer.token.if_missing.
As there is no .if_exist, .if_missing, etc. here, I cannot guess
what these seemingly independent and orthogonal, but some
combinations are impossible configuration variables are meant to be
used, let alone agreeing to the above claim that they are better
than a single .style.
Yeah, I should have explained more. So I will do it now.
In the code I used the following enums:
enum style_if_exist { DONT_REPEAT, OVERWRITE, REPEAT };
enum style_if_missing { DONT_APPEND, APPEND };
The style_if_exist enum is meant to decide what is done when we have
to deal with 2 or more trailers, from the infile or the command line,
that have the same key but different not empty values.
For example, me might have the 3 following trailers:
Acked-by: joe joe@example
Acked-by: bob bob@example
Acked-by: joe joe@example
- The DONT_REPEAT style, which should be the default in my opinion,
would mean that we don't repeat the same values. So we would get:
Acked-by: joe joe@example
Acked-by: bob bob@example
- The OVERWRITE style would mean that we keep only one, (for example
the last one). So we would get:
Acked-by: joe joe@example
- The REPEAT style would mean that we keep everything. So we would
get:
Acked-by: joe joe@example
Acked-by: bob bob@example
Acked-by: joe joe@example
The style_if_missing enum is meant to decide what is done when we have
no trailer with the specified key. Then DONT_APPEND means that we do
nothing, which should be the default, and APPEND means that we append
a trailer with the specified key.
Of course in the latter case, a command should probably be specified
to tell which value should be used with the key.
For example:
[trailer signoff]
key = Signed-off-by:
if_missing = append
command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL'
would append a s-o-b line only if there is no s-o-b already.
Note that to always append a specific trailer one could use:
[trailer signoff]
key = Signed-off-by:
if_missing = append
if_exist = repeat
command = echo $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL'
I think you took the .style from my
thinking-aloud message the other day, but that aloud-thinking lead
to .style by taking various use cases people wanted to have
footers into account, including:
Ok, I will try to guess below, how the use cases could be configured.
- just appending, allowing duplication of the field name (e.g. more
than one cc: can name different recipients);
That would be the default (if_exist = dont_repeat, if_missing = dont_append).
- appending, allowing duplication of the field name,val pair
(e.g. a patch that flowed from person A to B and possibly
somewhere else then finally back to A may have Signed-off-by:
A, chain of other's Sob, and another Signed-off-by: A);
That would be: if_exist = repeat, if_missing = dont_append
- appending, but without consecutive repeats (e.g. the same
Signed-off-by: example; person A does not pass a patch to
himself, adding two same name,val pair next to each other);
I could add a DONT_REPEAT_PREVIOUS into the style_if_exist enum, for
this case.
Then that would be: if_exist = dont_repeat_previous, if_missing = dont_append
- adding only if there is no same name (e.g. Change-Id:);
I could add a DONT_APPEND into the
Re: [RFC/PATCH] Add interpret-trailers builtin
Christian Couder chrisc...@tuxfamily.org writes: This RFC patch shows the work in progress to implement a new command: git interpret-trailers 1) Rational: This command should help with RFC 822 style headers, called trailers, that are found at the end of commit messages. For a long time, these trailers have become a de facto standard way to add helpful information into commit messages. Until now git commit has only supported the well known Signed-off-by: trailer, that is used by many projects like the Linux kernel and Git. It is better to implement features for these trailers in a new command rather than in builtin/commit.c, because this way the prepare-commit-msg and commit-msg hooks can reuse this command. 2) Current state: Currently the usage string of this command is: git interpret-trailers [--trim-empty] [--infile=file] [token[=value]...] The following features are implemented: - the result is printed on stdout - the [token[=value]...] arguments are interpreted - a commit message passed using the --infile=file option is interpreted - the trailer.token.value options in the config are interpreted The following features are planned but not yet implemented: - some documentation - more tests - the trailer.token.if_exist config option - the trailer.token.if_missing config option - the trailer.token.command config option 3) Notes: * trailer seems better than commitTrailer as the config key because it looks like all the config keys are lower case and committrailer is not very readable. And closes the door for other things from later acquiring trailers? * trailer.token.value looks better than trailer.token.trailer, so I chose the former. If that is a literal value, then I think .value is indeed good. * Rather than only one trailer.token.style config option, it seems better to me to have both trailer.token.if_exist and trailer.token.if_missing. As there is no .if_exist, .if_missing, etc. here, I cannot guess what these seemingly independent and orthogonal, but some combinations are impossible configuration variables are meant to be used, let alone agreeing to the above claim that they are better than a single .style. I think you took the .style from my thinking-aloud message the other day, but that aloud-thinking lead to .style by taking various use cases people wanted to have footers into account, including: - just appending, allowing duplication of the field name (e.g. more than one cc: can name different recipients); - appending, allowing duplication of the field name,val pair (e.g. a patch that flowed from person A to B and possibly somewhere else then finally back to A may have Signed-off-by: A, chain of other's Sob, and another Signed-off-by: A); - appending, but without consecutive repeats (e.g. the same Signed-off-by: example; person A does not pass a patch to himself, adding two same name,val pair next to each other); - adding only if there is no same name (e.g. Change-Id:); - adding only if there is no name,val pair (e.g. Closes: #bugId); - adding one if there is none, replacing one if there already is. I do not think it is easier for users to express these (and other) semantics as combinations of seemingly independent and orthogonal, but some combinations are impossible configuration variables. * I might send a patch series instead of just one big patch when there will be fewer big changes in the code. Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- .gitignore| 1 + Makefile | 1 + builtin.h | 1 + builtin/interpret-trailers.c | 284 ++ git.c | 1 + strbuf.c | 7 ++ strbuf.h | 1 + I think you're better off having trailers.c at the top level that is called from builtin/interpret-trailers.c (aside from names), so that we can later hook the former up with builtin/commit.c codepath. -- 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: [RFC/PATCH] Add interpret-trailers builtin
Johan Herland jo...@herland.net writes:
Thanks for looking this over. I am mostly in agreement and will
elide the parts I do not have much to add.
This command should help with RFC 822 style headers, called
trailers, that are found at the end of commit messages.
As has been asked earlier in this discussion, we should probably
specify explicitly what we _mean_ with RFC822-style
headers/footers/trailers, and exactly how closely we follow the actual
RFC... E.g. do we make use of the linebreaking rules? encoding
handling? etc... We may want to take a more relaxed approach (after
all, we're not including a complete RFC822/RFC2822 implementation),
but we should at least state so, and possibly how/why we do so.
Indeed; especially I do not think we would want to do the 2822
contination lines, or 2047 MIME quoting, ever.
For a long time, these trailers have become a de facto standard
way to add helpful information into commit messages.
helpful is not the key aspect of these footers. They are added at
the end to introduce some structure into a free-form part of the
commit objects.
The following features are implemented:
- the result is printed on stdout
- the [token[=value]...] arguments are interpreted
- a commit message passed using the --infile=file option is
interpreted
If the output is written to stdout, then why is not the input taken
from stdin? Or vice versa: why not --outfile?
I'd say just make it a filter without --in/outfile ;-)
+{
+ char *end = strchr(arg, '=');
+ if (!end)
+ end = strchr(arg, ':');
So both '=' (preferred) and ':' are accepted as field/value
separators. That's ok for the command-line, I believe.
Why?
Sometimes you have to be loose from the beginning _if_ some existing
uses and established conventions make it easier for the users, but
if you do not have to start from being loose, it is almost always a
mistake to do so. The above code just closed the door to use :
for some other useful purposes we may later discover, and will make
us regret for doing so.
From the enum values, I assume that these declare the desired behavior
when there are two (or more?) or the same footer (i.e. same field
name). However, it's not (yet) obvious to me in which order they are
processed. There are several opportunities for multiple instances of
the same field name:
- Two (or more) occurences in the infile
- Two (or more) occurences on the command line
- One occurence in the infile, and one of the command line
Also, there is a distinction between fields with the same field name
appearing twice and fields with the same field name and the same
field value appearing twice. Some headers do want to have them, some
do not.
--
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: [RFC/PATCH] Add interpret-trailers builtin
On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano gits...@pobox.com wrote:
Johan Herland jo...@herland.net writes:
+{
+ char *end = strchr(arg, '=');
+ if (!end)
+ end = strchr(arg, ':');
So both '=' (preferred) and ':' are accepted as field/value
separators. That's ok for the command-line, I believe.
Why?
Sometimes you have to be loose from the beginning _if_ some existing
uses and established conventions make it easier for the users, but
if you do not have to start from being loose, it is almost always a
mistake to do so. The above code just closed the door to use :
for some other useful purposes we may later discover, and will make
us regret for doing so.
Although I agree with the principle, I think there are (at least) two
established conventions that will be commonly used from the start, and
that we should support:
- Using short forms with '=', e.g. ack=Peff. There is already a
convention on how we specify name + value pairs on the command
line, e.g. git -c foo=bar ...
- Copy-pasting footers from existing commit messages. These will have
the same format as the expected output of this command, and not
accepting the same format in its input seems silly, IMHO.
That said, I think this applies only to the formatting on the _command
line_. When it comes to how the resulting footers are formatted in the
commit message, I would argue it only makes sense to use ':', and I
think the testcase named 'with config setup and = sign' in the above
patch is ugly and unnecessary.
From the enum values, I assume that these declare the desired behavior
when there are two (or more?) or the same footer (i.e. same field
name). However, it's not (yet) obvious to me in which order they are
processed. There are several opportunities for multiple instances of
the same field name:
- Two (or more) occurences in the infile
- Two (or more) occurences on the command line
- One occurence in the infile, and one of the command line
Also, there is a distinction between fields with the same field name
appearing twice and fields with the same field name and the same
field value appearing twice. Some headers do want to have them, some
do not.
True. This complexity is partly why I initially wanted to leave this
whole thing up to hooks, but I guess now we have to deal with it...
That said, I believe we don't need to cater to every possibility under
the sun, just the most common ones. If a minority of users are not
satisfied, they can simply configure this to leave all duplicates in
place, and then write their own commit-msg hook to do whatever weird
consolidation/cleanup they prefer.
...Johan
PS: Since this program will be run by git commit, and might also be
invoked by hooks, we need to keep the following in mind:
- Design for idempotence. A given set of headers might be filtered
through this program multiple times, and we should make sure that
multiple runs over the same data does not itself cause problems.
- Optional/flexible configuration. When a hook runs this program, it
may want to impose its own set of rules that does not entirely (or at
all) coincide with what is set in the config. We should therefore
consider providing a way for the caller to specify a separate source
of trailer/footer config to apply on a given run.
--
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: [RFC/PATCH] Add interpret-trailers builtin
From: Johan Herland jo...@herland.net
On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano gits...@pobox.com wrote:
Johan Herland jo...@herland.net writes:
+{
+ char *end = strchr(arg, '=');
+ if (!end)
+ end = strchr(arg, ':');
So both '=' (preferred) and ':' are accepted as field/value
separators. That's ok for the command-line, I believe.
Why?
Sometimes you have to be loose from the beginning _if_ some existing
uses and established conventions make it easier for the users,
The users are already used to appending Acked-by: Joe j...@example.com.
So I think it makes it easier for the user to accept what they are already
used to provide.
but
if you do not have to start from being loose, it is almost always a
mistake to do so. The above code just closed the door to use :
for some other useful purposes we may later discover, and will make
us regret for doing so.
Although I agree with the principle, I think there are (at least) two
established conventions that will be commonly used from the start, and
that we should support:
- Using short forms with '=', e.g. ack=Peff. There is already a
convention on how we specify name + value pairs on the command
line, e.g. git -c foo=bar ...
- Copy-pasting footers from existing commit messages. These will have
the same format as the expected output of this command, and not
accepting the same format in its input seems silly, IMHO.
I agree. Also I think it will avoid some mistakes by the users.
Because it would require an effort for them to remember that if they
want to see Acked-by: Joe j...@example.com they have to put
Acked-by= Joe j...@example.com on the command line.
That said, I think this applies only to the formatting on the _command
line_.
But wouldn't it be nice if the same parsing function could be used for
both the command line and the commit message template?
When it comes to how the resulting footers are formatted in the
commit message, I would argue it only makes sense to use ':', and I
think the testcase named 'with config setup and = sign' in the above
patch is ugly and unnecessary.
I wanted to support configurations like this:
[trailer ack]
value = Acked-by=
[trailer bug]
value = Bug #
because Peff said that GitHub uses '#' and while at it I suppose some
people might prefer '=' over ':'.
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
[RFC/PATCH] Add interpret-trailers builtin
This RFC patch shows the work in progress to implement a new
command:
git interpret-trailers
1) Rational:
This command should help with RFC 822 style headers, called
trailers, that are found at the end of commit messages.
For a long time, these trailers have become a de facto standard
way to add helpful information into commit messages.
Until now git commit has only supported the well known
Signed-off-by: trailer, that is used by many projects like
the Linux kernel and Git.
It is better to implement features for these trailers in a new
command rather than in builtin/commit.c, because this way the
prepare-commit-msg and commit-msg hooks can reuse this command.
2) Current state:
Currently the usage string of this command is:
git interpret-trailers [--trim-empty] [--infile=file] [token[=value]...]
The following features are implemented:
- the result is printed on stdout
- the [token[=value]...] arguments are interpreted
- a commit message passed using the --infile=file option is
interpreted
- the trailer.token.value options in the config are interpreted
The following features are planned but not yet implemented:
- some documentation
- more tests
- the trailer.token.if_exist config option
- the trailer.token.if_missing config option
- the trailer.token.command config option
3) Notes:
* trailer seems better than commitTrailer as the config key because
it looks like all the config keys are lower case and committrailer is not
very readable.
* trailer.token.value looks better than trailer.token.trailer, so
I chose the former.
* Rather than only one trailer.token.style config option, it seems
better to me to have both trailer.token.if_exist and
trailer.token.if_missing.
* I might send a patch series instead of just one big patch when there will
be fewer big changes in the code.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
.gitignore| 1 +
Makefile | 1 +
builtin.h | 1 +
builtin/interpret-trailers.c | 284 ++
git.c | 1 +
strbuf.c | 7 ++
strbuf.h | 1 +
t/t7513-interpret-trailers.sh | 101 +++
8 files changed, 397 insertions(+)
create mode 100644 builtin/interpret-trailers.c
create mode 100755 t/t7513-interpret-trailers.sh
diff --git a/.gitignore b/.gitignore
index 66199ed..e6cf15b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,6 +73,7 @@
/git-index-pack
/git-init
/git-init-db
+/git-interpret-trailers
/git-instaweb
/git-log
/git-lost-found
diff --git a/Makefile b/Makefile
index af847f8..96441f1 100644
--- a/Makefile
+++ b/Makefile
@@ -937,6 +937,7 @@ BUILTIN_OBJS += builtin/hash-object.o
BUILTIN_OBJS += builtin/help.o
BUILTIN_OBJS += builtin/index-pack.o
BUILTIN_OBJS += builtin/init-db.o
+BUILTIN_OBJS += builtin/interpret-trailers.o
BUILTIN_OBJS += builtin/log.o
BUILTIN_OBJS += builtin/ls-files.o
BUILTIN_OBJS += builtin/ls-remote.o
diff --git a/builtin.h b/builtin.h
index b56cf07..88c2999 100644
--- a/builtin.h
+++ b/builtin.h
@@ -71,6 +71,7 @@ extern int cmd_hash_object(int argc, const char **argv, const
char *prefix);
extern int cmd_help(int argc, const char **argv, const char *prefix);
extern int cmd_index_pack(int argc, const char **argv, const char *prefix);
extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_interpret_trailers(int argc, const char **argv, const char
*prefix);
extern int cmd_log(int argc, const char **argv, const char *prefix);
extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
new file mode 100644
index 000..2bcd480
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,284 @@
+/*
+ * Builtin git interpret-trailers
+ *
+ * Copyright (c) 2013 Christian Couder chrisc...@tuxfamily.org
+ *
+ */
+
+#include cache.h
+#include builtin.h
+#include parse-options.h
+#include strbuf.h
+
+static const char * const git_interpret_trailers_usage[] = {
+ N_(git interpret-trailers [--trim-empty] [--infile=file]
[token[=value]...]),
+ NULL
+};
+
+static void parse_arg(struct strbuf *tok, struct strbuf *val, const char *arg)
+{
+ char *end = strchr(arg, '=');
+ if (!end)
+ end = strchr(arg, ':');
+ if (end) {
+ strbuf_add(tok, arg, end - arg);
+ strbuf_trim(tok);
+ strbuf_addstr(val, end + 1);
+ strbuf_trim(val);
+ } else {
+ strbuf_addstr(tok, arg);
+ strbuf_trim(tok);
+ }
+}
+
+static struct string_list trailer_list;
+
+enum style_if_exist { DONT_REPEAT, OVERWRITE, REPEAT };
+enum style_if_missing { DONT_APPEND, APPEND };
+
Re: [RFC/PATCH] Add interpret-trailers builtin
On Sun, Nov 3, 2013 at 10:17 PM, Christian Couder chrisc...@tuxfamily.org wrote: This RFC patch shows the work in progress to implement a new command: First of all: Thanks for working on this! This looks like a really good start. Plenty of comments below (mostly either to learn myself, or to check what alternatives you've considered), but overall, I'm happy about where this is going. git interpret-trailers Pesonally, I'd rather name it git process-footers, since I think process better captures the two-way functionality of this program (both parsing/interpreting _and_ generating/writiing), and I believe I'm not alone in preferring footer over trailer. That said, this is certainly bikeshedding territory, and I am not the one writing the code, so feel free to ignore. 1) Rational: s/Rational/Rationale/ This command should help with RFC 822 style headers, called trailers, that are found at the end of commit messages. As has been asked earlier in this discussion, we should probably specify explicitly what we _mean_ with RFC822-style headers/footers/trailers, and exactly how closely we follow the actual RFC... E.g. do we make use of the linebreaking rules? encoding handling? etc... We may want to take a more relaxed approach (after all, we're not including a complete RFC822/RFC2822 implementation), but we should at least state so, and possibly how/why we do so. For a long time, these trailers have become a de facto standard way to add helpful information into commit messages. Until now git commit has only supported the well known Signed-off-by: trailer, that is used by many projects like the Linux kernel and Git. It is better to implement features for these trailers in a new command rather than in builtin/commit.c, because this way the prepare-commit-msg and commit-msg hooks can reuse this command. 2) Current state: Currently the usage string of this command is: git interpret-trailers [--trim-empty] [--infile=file] [token[=value]...] --trim-empty will remove empty footers given on the command-line? Or in the infile? or both? I think I'm fine with both, but I wonder if there is a use case for treating the two types of empty footers separately... The following features are implemented: - the result is printed on stdout - the [token[=value]...] arguments are interpreted - a commit message passed using the --infile=file option is interpreted If the output is written to stdout, then why is not the input taken from stdin? Or vice versa: why not --outfile? - the trailer.token.value options in the config are interpreted This is the default value of the footer if not further specified on the command-line? The following features are planned but not yet implemented: - some documentation - more tests - the trailer.token.if_exist config option - the trailer.token.if_missing config option Not sure what these two options will do, or what kind of values they take... - the trailer.token.command config option I guess the value of .command is executed to generate the default content of the footer? 3) Notes: * trailer seems better than commitTrailer as the config key because it looks like all the config keys are lower case and committrailer is not very readable. As stated above, I prefer footer over trailer... * trailer.token.value looks better than trailer.token.trailer, so I chose the former. * Rather than only one trailer.token.style config option, it seems better to me to have both trailer.token.if_exist and trailer.token.if_missing. * I might send a patch series instead of just one big patch when there will be fewer big changes in the code. Maybe at least split in two: - One patch to deal with the administrivia of adding a new command. - One (or more) patch(es) to introduce the logic/functionality of the new command. Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- .gitignore| 1 + Makefile | 1 + builtin.h | 1 + builtin/interpret-trailers.c | 284 ++ git.c | 1 + strbuf.c | 7 ++ strbuf.h | 1 + t/t7513-interpret-trailers.sh | 101 +++ 8 files changed, 397 insertions(+) create mode 100644 builtin/interpret-trailers.c create mode 100755 t/t7513-interpret-trailers.sh diff --git a/.gitignore b/.gitignore index 66199ed..e6cf15b 100644 --- a/.gitignore +++ b/.gitignore @@ -73,6 +73,7 @@ /git-index-pack /git-init /git-init-db +/git-interpret-trailers /git-instaweb /git-log /git-lost-found diff --git a/Makefile b/Makefile index af847f8..96441f1 100644 --- a/Makefile +++ b/Makefile @@ -937,6 +937,7 @@ BUILTIN_OBJS += builtin/hash-object.o BUILTIN_OBJS += builtin/help.o BUILTIN_OBJS += builtin/index-pack.o BUILTIN_OBJS +=
