On 10/28/21 12:21 PM, Jean-Noël Avila via GitGitGadget wrote: > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@xxxxxxx> > > According to CodingGuidelines, spaces and underscores are not > allowed in placeholders. I have a patch under review that touches the same files you are modifying here. As I've been pointed to these changes, I'd like to make a quick observation. > @@ -101,9 +101,9 @@ commits are displayed, but not the way the diff is shown e.g. with > `git log --raw`. To get full object names in a raw diff format, > use `--no-abbrev`. > > -* 'format:<string>' > +* 'format:<format-string>' > + > -The 'format:<string>' format allows you to specify which information > +The 'format:<format-string>' format allows you to specify which information > you want to show. It works a little bit like printf format, > with the notable exception that you get a newline with '%n' > instead of '\n'. > @@ -273,12 +273,12 @@ endif::git-rev-list[] > If any option is provided multiple times the > last occurrence wins. > + > -The boolean options accept an optional value `[=<BOOL>]`. The values > +The boolean options accept an optional value `[=<value>]`. The values Here you change "BOOL" to "value", below you change it to "bool-value". > `true`, `false`, `on`, `off` etc. are all accepted. See the "boolean" > sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean > option is given with no value, it's enabled. > + > -** 'key=<K>': only show trailers with specified key. Matching is done > +** 'key=<key>': only show trailers with specified <key>. Matching is done > case-insensitively and trailing colon is optional. If option is > given multiple times trailer lines matching any of the keys are > shown. This option automatically enables the `only` option so that > @@ -286,9 +286,9 @@ option is given with no value, it's enabled. > desired it can be disabled with `only=false`. E.g., > `%(trailers:key=Reviewed-by)` shows trailer lines with key > `Reviewed-by`. > -** 'only[=<BOOL>]': select whether non-trailer lines from the trailer > +** 'only[=<bool-value>]': select whether non-trailer lines from the trailer > block should be included. > -** 'separator=<SEP>': specify a separator inserted between trailer > +** 'separator=<sep>': specify a separator inserted between trailer > lines. When this option is not given each trailer line is > terminated with a line feed character. The string SEP may contain > the literal formatting codes described above. To use comma as > @@ -296,15 +296,15 @@ option is given with no value, it's enabled. > next option. E.g., `%(trailers:key=Ticket,separator=%x2C )` > shows all trailer lines whose key is "Ticket" separated by a comma > and a space. > -** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold` > +** 'unfold[=<bool-value>]': make it behave as if interpret-trailer's `--unfold` > option was given. E.g., > `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. > -** 'keyonly[=<BOOL>]': only show the key part of the trailer. > -** 'valueonly[=<BOOL>]': only show the value part of the trailer. > -** 'key_value_separator=<SEP>': specify a separator inserted between > +** 'keyonly[=<bool-value>]': only show the key part of the trailer. > +** 'valueonly[=<bool-value>]': only show the value part of the trailer. > +** 'key_value_separator=<sep>': specify a separator inserted between > trailer lines. When this option is not given each trailer key-value > pair is separated by ": ". Otherwise it shares the same semantics > - as 'separator=<SEP>' above. > + as 'separator=<sep>' above. > > NOTE: Some placeholders may depend on other options given to the > revision traversal engine. For example, the `%g*` reflog options will These changes over here are contrary to the statement in the commit message. In addition to switching to hyphens, they: - switch casing (okay, makes sense, you point this out in the cover letter but maybe it's worth mentioning it in the commit message too? idk) - change the terms used -- and this I don't understand. I'm not really bothered by switching <n> to <number> or <k> to <key>, as these changes seem reasonable (though again, they are not mentioned in the commit message). However, "bool-value" seems odd. Why that and not "number-value"? IMHO the "value" is redundant here, let it be "bool" and "number". Similarly "the 'format:<format-string>' format" feels highly redundant, I expect the reader knows that <string> contains a format inside it as it's mentioned immediately before *and* after. -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User
