| Message ID | cover-00.34-00000000000-20220902T092734Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | doc/UX: make txt & -h output more consistent | expand |
Ævar Arnfjörð Bjarmason wrote: > We are currently wildly inconsistent in whether the SYNOPSIS in the > manual page matches the first line of the -h output, and as we add new > options we routinely forget to add them to one or the other (or both). > > Without a more complex approach it's hard to do something about the > "or both" case. But we can rather easily test whether the -h output > matches the *.txt version, and report differences. > > As this series shows that allows us to fix a lot of issues we've > effectively already "fixed", we just fixed them in one version, but > not the other. > > We now have a test that checks that these are the same for all > built-ins. Out of 141 built-in commands we still have 58 cases that > differ at the end of this series, but that's a lot better than before. Thanks! Codifying these expectations into an automated check will be extremely helpful for avoiding easy-to-miss mistakes that could potentially confuse end users. > > This series is 34 patches, but it's been structured in a way that > reviewers should be able to mostly trust the "doc txt & -h > consistency" commits already. In all of those cases we already have > the post-image in-tree, it's just in either the *.txt or -h version, > not both. All of the "doc txt & -h consistency" commits merely make > those consistent together. In general, I don't think it's wise to implicitly trust *any* patches, no matter how simple they appear. I know there are benefits to doing everything at once, but the volume and variety of these changes makes it difficult to review the series and *confidently* say we haven't introduced any regressions, missed any cases, or made the "wrong" decisions w.r.t changing either the .txt or -h documentation to make them consistent. After reading through this series a few times, it looks like it contains a mix of: * updated syntax guidance in CodingGuidelines * clear syntax or naming fixes (e.g., adding a missing "]" to a '-h' output, "keyid" -> "key-id" for translation purposes) * internal refactoring * changing an error message's content * changing the listed options and/or usage in '-h' and/or the .txt SYNOPSIS * adding an option description in a .txt doc * adding the new test If I could offer a suggestion, my preference would be that you split the series into three parts: one with the straightforward, easier-to-review changes, another with the more substantial updates to user-facing docs/information (which might warrant more discussion, i.e. which options we should be showing for a command in the SYNOPSIS/-h), and the last catching any new inconsistencies & adding the test. That way, more involved discussion on some patches doesn't prevent *all* of them from being merged. I think the following patches would fit a "straightforward, easier-to-review" series: * Patch 1 (CodingGuidelines: update and clarify command-line conventions) * Patch 2 (builtin/bundle.c: use \t, not fix indentation 2-SP indentation) * Patch 3 (bundle: define subcommand -h in terms of command -h) * Patch 5 (doc SYNOPSIS: don't use ' for subcommands) * Patch 6 (doc SYNOPSIS: consistently use ' for commands) * Patch 7 (doc SYNOPSIS & -h: fix incorrect alternates syntax) * Patch 8 (built-ins: consistently add "\n" between "usage" and options) * Patch 9 (doc txt & -h consistency: word-wrap) * Patch 10 (doc txt & -h consistency: fix incorrect alternates syntax) * Patch 12 (doc txt & -h consistency: add missing "]" to bugreport "-h") * Patch 13 (doc txt & -h consistency: correct padding around "[]()") * Patch 14 (stash doc SYNOPSIS & -h: correct padding around "[]()") * Patch 15 (doc txt & -h consistency: use "<options>", not "<options>...") * Patch 16 (t/helper/test-proc-receive.c: use "<options>", not "<options>...") * Patch 17 (doc txt & -h consistency: fix mismatching labels) * Patch 18 (doc txt & -h consistency: add or fix optional "--" syntax) (basically, 1-18, skipping patch 4 because it changes the content of an error message & patch 11 because it adds an option to the -h of 'cat-file') In terms of review, my only comment on those patches is that 7 & 10 could probably benefit from being squashed together [1]. Otherwise, with the changes you mentioned in response to Junio's feedback [2], I think that subset of the series would be ready to merge. I hope that helps! -Victoria [1] https://lore.kernel.org/git/b1c44436-92d1-067c-fb0a-be4049f3031b@github.com/ [2] https://lore.kernel.org/git/220908.865yhyl31c.gmgdl@evledraar.gmail.com/ > > Ævar Arnfjörð Bjarmason (34): > CodingGuidelines: update and clarify command-line conventions > builtin/bundle.c: use \t, not fix indentation 2-SP indentation > bundle: define subcommand -h in terms of command -h > blame: use a more detailed usage_msg_optf() error on bad -L > doc SYNOPSIS: don't use ' for subcommands > doc SYNOPSIS: consistently use ' for commands > doc SYNOPSIS & -h: fix incorrect alternates syntax > built-ins: consistently add "\n" between "usage" and options > doc txt & -h consistency: word-wrap > doc txt & -h consistency: fix incorrect alternates syntax > doc txt & -h consistency: add "-z" to cat-file "-h" > doc txt & -h consistency: add missing "]" to bugreport "-h" > doc txt & -h consistency: correct padding around "[]()" > stash doc SYNOPSIS & -h: correct padding around "[]()" > doc txt & -h consistency: use "<options>", not "<options>..." > t/helper/test-proc-receive.c: use "<options>", not "<options>..." > doc txt & -h consistency: fix mismatching labels > doc txt & -h consistency: add or fix optional "--" syntax > doc txt & -h consistency: make output order consistent > doc txt & -h consistency: add missing options and labels > doc txt & -h consistency: make "rerere" consistent > doc txt & -h consistency: make "read-tree" consistent > doc txt & -h consistency: make "bundle" consistent > doc txt & -h consistency: use "git foo" form, not "git-foo" > doc txt & -h consistency: add missing options > doc txt & -h consistency: make "stash" consistent > doc txt & -h consistency: make "annotate" consistent > doc txt & -h consistency: use "[<label>...]" for "zero or more" > doc txt & -h consistency: make "diff-tree" consistent > doc txt & -h consistency: make "commit" consistent > reflog doc: list real subcommands up-front > worktree: define subcommand -h in terms of command -h > doc txt & -h consistency: make "worktree" consistent > tests: start asserting that *.txt SYNOPSIS matches -h output >
Victoria Dye wrote: > If I could offer a suggestion, my preference would be that you split the > series into three parts: one with the straightforward, easier-to-review > changes, another with the more substantial updates to user-facing > docs/information (which might warrant more discussion, i.e. which options we > should be showing for a command in the SYNOPSIS/-h), and the last catching > any new inconsistencies & adding the test. That way, more involved > discussion on some patches doesn't prevent *all* of them from being merged. > > I think the following patches would fit a "straightforward, > easier-to-review" series: > > * Patch 1 (CodingGuidelines: update and clarify command-line conventions) > * Patch 2 (builtin/bundle.c: use \t, not fix indentation 2-SP indentation) > * Patch 3 (bundle: define subcommand -h in terms of command -h) > * Patch 5 (doc SYNOPSIS: don't use ' for subcommands) > * Patch 6 (doc SYNOPSIS: consistently use ' for commands) > * Patch 7 (doc SYNOPSIS & -h: fix incorrect alternates syntax) > * Patch 8 (built-ins: consistently add "\n" between "usage" and options) > * Patch 9 (doc txt & -h consistency: word-wrap) > * Patch 10 (doc txt & -h consistency: fix incorrect alternates syntax) > * Patch 12 (doc txt & -h consistency: add missing "]" to bugreport "-h") > * Patch 13 (doc txt & -h consistency: correct padding around "[]()") > * Patch 14 (stash doc SYNOPSIS & -h: correct padding around "[]()") > * Patch 15 (doc txt & -h consistency: use "<options>", not "<options>...") > * Patch 16 (t/helper/test-proc-receive.c: use "<options>", not > "<options>...") > * Patch 17 (doc txt & -h consistency: fix mismatching labels) > * Patch 18 (doc txt & -h consistency: add or fix optional "--" syntax) > > (basically, 1-18, skipping patch 4 because it changes the content of an > error message & patch 11 because it adds an option to the -h of 'cat-file') > > In terms of review, my only comment on those patches is that 7 & 10 could > probably benefit from being squashed together [1]. Otherwise, with the > changes you mentioned in response to Junio's feedback [2], I think that > subset of the series would be ready to merge. > I forgot to mention this in the previous message, but it's probably worth noting - regardless of whether or not you split the series, I am still planning to review the remaining patches you've submitted here (that is, everything *other than* what's in the list above, which I've already looked over/commented on). I likely won't get the chance until later this week at the earliest, though. Thanks! -Victoria