Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'
Patch 17/17 of v4 failed to arrive in my inbox for some reason, so I'll just reply to v3 since there's another error I noticed which is still present in v4, plus a comment specific to v4 (see below). On Mon, Jan 27, 2014 at 3:33 PM, Christian Couder wrote: > From: Eric Sunshine >> On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder >> wrote: >>> +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 white spaces, and the resulting trimmed 'token' >> >> Other git documentation uniformly spells it as "whitespace" rather >> than "white spaces". > > You are right I will change that. The rest of git documentation consistently spells it "whitespace", but v4 uses "whitespaces". >>> +infile=file:: >>> + Read the commit message from `file` instead of the standard >>> + input. I didn't notice this when reviewing v3, and the same text appears in v4. There are a couple extra hyphens before 'infile', and you want < and > around "file", so: s/infile=file/--infile=/ -- 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 v3 17/17] Documentation: add documentation for 'git interpret-trailers'
Christian Couder writes: >> I find it a bad taste to allow unbound set of on the LHS of >> '=' on the command line, but that is a separate issue in the design, >> not in the documentation of the design. > > I don't understand this sentence, sorry. It is a bad design taste to structure the command line argument in such a way that it takes git cmd xyzzy=frotz nitfol=rezrov some other args where these 'xyzzy', 'nitfol', etc. form an unbound set. It prevents future enhancements to the command from allowing anything that contain '=' in "some other args" part. Allowing not just '=' but also ':' makes it even worse. But these are issues in the design itself. Not the issue in the patch 17/17 that is trying to document the design. This patch under discussion documents the bad design correctly ;-) -- 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 v3 17/17] Documentation: add documentation for 'git interpret-trailers'
From: Junio C Hamano Subject: Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers' Date: Mon, 27 Jan 2014 13:20:18 -0800 > Christian Couder writes: > >>>> +'git interpret-trailers' [--trim-empty] [--infile=file] >>>> [...] >>> >>> Would it be more consistent with existing documentation to format this as >>> so? >>> >>> [--infile=] [[=]]... >> >> No, it would be very inconsistent: >> >> $ grep '\.\.\.\]' *.txt | wc -l >> 103 >> $ grep '\]\.\.\.' *.txt | wc -l >> 0 > > I have a feeling that you are missing the point Eric is making. Yeah I realize I missed some of his points. Sorry about that. > The > value given to the --infile option can be anything, i.e. 'file' > there is a placeholder, hence "--infile=" not "--infile=file" > as you wrote. Ok, I agree with that. > Also I think "[[=]]..." is the correct way to spell > that there can be 0 or more "[=]". Perhaps but it is not very consistent with what there is in other synopsis strings. For example the commands that accept a path have a synopsis that ends with "[--] [...]" and not "[--] []...". Perhaps [([=])...] is more correct, but not worth the added complexity. > "" > in the original does not make any sense, as <> is meant to say "this > thing is a placeholder", and we do not try to say, with the string > inside <>, what shape that placeholder takes. In fact '=' part is > _not_ a placeholder but is required syntactically when you want to > supply a value to the token, so the original doubly is incorrect. Yeah perhaps, except that in the code ':' is accepted instead of '=', and the documention says that. So perhaps [([(=|:)])...] is even more correct. > I find it a bad taste to allow unbound set of on the LHS of > '=' on the command line, but that is a separate issue in the design, > not in the documentation of the design. I don't understand this sentence, sorry. 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 v3 17/17] Documentation: add documentation for 'git interpret-trailers'
Christian Couder writes: >>> +'git interpret-trailers' [--trim-empty] [--infile=file] >>> [...] >> >> Would it be more consistent with existing documentation to format this as so? >> >> [--infile=] [[=]]... > > No, it would be very inconsistent: > > $ grep '\.\.\.\]' *.txt | wc -l > 103 > $ grep '\]\.\.\.' *.txt | wc -l > 0 I have a feeling that you are missing the point Eric is making. The value given to the --infile option can be anything, i.e. 'file' there is a placeholder, hence "--infile=" not "--infile=file" as you wrote. Also I think "[[=]]..." is the correct way to spell that there can be 0 or more "[=]". "" in the original does not make any sense, as <> is meant to say "this thing is a placeholder", and we do not try to say, with the string inside <>, what shape that placeholder takes. In fact '=' part is _not_ a placeholder but is required syntactically when you want to supply a value to the token, so the original doubly is incorrect. I find it a bad taste to allow unbound set of on the LHS of '=' on the command line, but that is a separate issue in the design, not in the documentation of the design. Thanks. -- 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 v3 17/17] Documentation: add documentation for 'git interpret-trailers'
From: Eric Sunshine > > On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder > wrote: >> Signed-off-by: Christian Couder >> --- >> diff --git a/Documentation/git-interpret-trailers.txt >> b/Documentation/git-interpret-trailers.txt >> new file mode 100644 >> index 000..f74843e >> --- /dev/null >> +++ b/Documentation/git-interpret-trailers.txt >> @@ -0,0 +1,137 @@ >> +git-interpret-trailers(1) >> += >> + >> +NAME >> + >> +git-interpret-trailers - help add stuctured information into commit messages >> + >> +SYNOPSIS >> + >> +[verse] >> +'git interpret-trailers' [--trim-empty] [--infile=file] [...] > > Would it be more consistent with existing documentation to format this as so? > > [--infile=] [[=]]... No, it would be very inconsistent: $ grep '\.\.\.\]' *.txt | wc -l 103 $ grep '\]\.\.\.' *.txt | wc -l 0 >> +DESCRIPTION >> +--- >> +Help add RFC 822 like headers, called 'trailers', at the end of the > > s/822 like/822-like/ Ok. > Was the suggestion, early in the discussion, to use "footer" rather > than "trailer" dismissed? During the early discussions people initially talked about "footer" while Junio and others talked about "trailer". See: http://thread.gmane.org/gmane.comp.version-control.git/236429/ http://thread.gmane.org/gmane.comp.version-control.git/236770/ I have no preference for one or the other, but by default I use what Junio uses. >> +otherwise free-form part of a commit message. >> + >> +Unless `--infile=file` is used, this command is a filter. It reads the >> +standard input for a commit message and apply the `token` arguments, > > s/apply/applies/ Ok. >> +if any, to this message. The resulting message is emited on the >> +standard output. >> + >> +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 white spaces, and the resulting trimmed 'token' > > Other git documentation uniformly spells it as "whitespace" rather > than "white spaces". You are right I will change that. >> +and 'value' will appear in the message like this: >> + >> + >> +token: value >> + >> + >> +By default, if there are already trailers with the same 'token' the >> +trailer will appear just after the last trailer with the same > > It might be a bit clearer to say "the _new_ trailer will appear...". Ok. >> +'token'. Otherwise it will appear at the end of the message. >> + >> +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. >> + >> +Trailers have become a de facto standard way to add helpful structured >> +information into commit messages. For example the well known >> +"Signed-off-by: " trailer is used by many projects like the Linux >> +kernel and Git. > > This "justification" paragraph might make more sense near or at the > very the top of the Description section. Yeah, or maybe I can remove it entirely. >> +OPTIONS >> +--- >> +--trim-empty:: >> + If the 'value' part of any trailer contains onlywhite spaces, > > s/onlywhite spaces/only whitespace/ Ok. >> + the whole trailer will be removed from the resulting message. >> + >> +infile=file:: >> + Read the commit message from `file` instead of the standard >> + input. >> + >> +CONFIGURATION VARIABLES >> +--- >> + >> +trailer..key:: >> + This 'key' will be used instead of 'token' in the >> + 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. >> + >> +trailer..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. >> + >> +trailer..ifexist:: >> + This option makes it possible to chose what action will be > > s/chose/choose/ Ok. >> + 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`, `ove
Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'
On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder wrote: > Signed-off-by: Christian Couder > --- > diff --git a/Documentation/git-interpret-trailers.txt > b/Documentation/git-interpret-trailers.txt > new file mode 100644 > index 000..f74843e > --- /dev/null > +++ b/Documentation/git-interpret-trailers.txt > @@ -0,0 +1,137 @@ > +git-interpret-trailers(1) > += > + > +NAME > + > +git-interpret-trailers - help add stuctured information into commit messages > + > +SYNOPSIS > + > +[verse] > +'git interpret-trailers' [--trim-empty] [--infile=file] [...] Would it be more consistent with existing documentation to format this as so? [--infile=] [[=]]... > +DESCRIPTION > +--- > +Help add RFC 822 like headers, called 'trailers', at the end of the s/822 like/822-like/ Was the suggestion, early in the discussion, to use "footer" rather than "trailer" dismissed? > +otherwise free-form part of a commit message. > + > +Unless `--infile=file` is used, this command is a filter. It reads the > +standard input for a commit message and apply the `token` arguments, s/apply/applies/ > +if any, to this message. The resulting message is emited on the > +standard output. > + > +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 white spaces, and the resulting trimmed 'token' Other git documentation uniformly spells it as "whitespace" rather than "white spaces". > +and 'value' will appear in the message like this: > + > + > +token: value > + > + > +By default, if there are already trailers with the same 'token' the > +trailer will appear just after the last trailer with the same It might be a bit clearer to say "the _new_ trailer will appear...". > +'token'. Otherwise it will appear at the end of the message. > + > +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. > + > +Trailers have become a de facto standard way to add helpful structured > +information into commit messages. For example the well known > +"Signed-off-by: " trailer is used by many projects like the Linux > +kernel and Git. This "justification" paragraph might make more sense near or at the very the top of the Description section. > +OPTIONS > +--- > +--trim-empty:: > + If the 'value' part of any trailer contains onlywhite spaces, s/onlywhite spaces/only whitespace/ > + the whole trailer will be removed from the resulting message. > + > +infile=file:: > + Read the commit message from `file` instead of the standard > + input. > + > +CONFIGURATION VARIABLES > +--- > + > +trailer..key:: > + This 'key' will be used instead of 'token' in the > + 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. > + > +trailer..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. > + > +trailer..ifexist:: > + This option makes it possible to chose what action will be s/chose/choose/ > + 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`. > ++ > +With `addIfDifferent`, a new trailer will be added only if no trailer > +with the same (token, value) pair is already in the message. > ++ > +With `addIfDifferentNeighbor`, a new trailer will be added only if no > +trailer with the same (token, value) pair is above or below the line > +where the new trailer will be added. > ++ > +With `add`, a new trailer will be added, even if some trailers with > +the same (token, value) pair are already in the message. > ++ > +With `overwrite`, the new trailer will overwrite an existing trailer > +with the same token. > ++ > +With `doNothing`, nothing will be done, that is no new trailer will be > +added if there is already o
