| Message ID | 20191026005159.98405-2-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some clarifications to MyFirstContribution | expand |
Hi, Emily Shaffer wrote: > Users can discover commands and their brief usage by running 'git help > git' or 'git help -a'; both of these pages list all available commands > based on the contents of 'command-list.txt'. That means adding a new > command there is an important part of the new command process, and > therefore belongs in the new command tutorial. Makes sense. Not about this patch: is there a way to detect this automatically? E.g. if a command in git.c::commands doesn't appear in command-list.txt, could we make Git fail "make test"? [...] > --- a/Documentation/MyFirstContribution.txt > +++ b/Documentation/MyFirstContribution.txt > @@ -534,6 +534,28 @@ you want to pass as a parameter something which would usually be interpreted as > a flag.) `parse_options()` will terminate parsing when it reaches `--` and give > you the rest of the options afterwards, untouched. > > +Now that you have a usage hint, you can teach Git how to show it in the general > +command list shown by `git help git` or `git help -a`, which is generated from > +`command-list.txt`. Find the line for 'git-pull' so you can add your 'git-psuh' > +line above it in alphabetical order. Now, we can add some attributes about the > +command which impacts where it shows up in the aforementioned help commands. The nit: s/impacts/impact/, to agree with "attributes" > +top of `command-list.txt` shares some information about what each attribute > +means; in those help pages, the commands are sorted according to these > +attributes. `git psuh` is user-facing, or porcelain - so we will mark it as ^^^^^^^^^ optional: This might be an unfamiliar term to people not thinking of the plumbing fixture / chrome analogy. Is there anything we should do to help them understand what's going on? E.g. "git help glossary" says Porcelains expose more of a SCM interface than the plumbing. > +"mainporcelain". For "mainporcelain" commands, the comments at the top of > +`command-list.txt` indicate we can also optionally add an attribute from another > +list; since `git psuh` shows some information about the user's workspace but > +doesn't modify anything, let's mark it as "info". Make sure to keep your > +attributes in the same style as the rest of `command-list.txt` using spaces to > +align and delineate them: > + > +---- > +git-prune-packed plumbingmanipulators > +git-psuh mainporcelain info tabs snuck in. > +git-pull mainporcelain remote > +git-push mainporcelain remote > +---- > + The rest looks good. Thanks, Jonathan
On Fri, Oct 25, 2019 at 06:08:57PM -0700, Jonathan Nieder wrote: > > Users can discover commands and their brief usage by running 'git help > > git' or 'git help -a'; both of these pages list all available commands > > based on the contents of 'command-list.txt'. That means adding a new > > command there is an important part of the new command process, and > > therefore belongs in the new command tutorial. > > Makes sense. > > Not about this patch: is there a way to detect this automatically? > E.g. if a command in git.c::commands doesn't appear in > command-list.txt, could we make Git fail "make test"? We almost detect this already: $ sed -i -e '/^git-bisect/d' command-list.txt $ make check-docs make -C Documentation lint-docs make[1]: Entering directory '/home/szeder/src/git/Documentation' GEN cmd-list.made GEN doc.dep make[2]: Entering directory '/home/szeder/src/git' make[2]: 'GIT-VERSION-FILE' is up to date. make[2]: Leaving directory '/home/szeder/src/git' make[2]: Entering directory '/home/szeder/src/git' make[2]: 'GIT-VERSION-FILE' is up to date. make[2]: Leaving directory '/home/szeder/src/git' LINT lint-docs make[1]: Leaving directory '/home/szeder/src/git/Documentation' no link: git-bisect $ echo $? 0 See that last "no link: git-bisect" line? That's what happened to catch my eyes when Derrick forgot to add his new 'sparse-checkout' builtin to 'command-list.txt'. I still haven't looked up what that 'no link' is supposed to mean, but if it were an error, then we would have surely detected the missing entry in 'command-list.txt' in out CI builds. Another possibility would be to auto-generate that long list of 'cmd_foo()' function declaration in 'builtin.h' from 'command-list.txt', by adding a 'builtin' attribute to mark builtin commands.
SZEDER Gábor <szeder.dev@gmail.com> writes: > See that last "no link: git-bisect" line? That's what happened to > catch my eyes when Derrick forgot to add his new 'sparse-checkout' > builtin to 'command-list.txt'. I still haven't looked up what that > 'no link' is supposed to mean, but if it were an error, then we would Build procedure for the Documentation uses ../command-list.txt as its input to produce cmds-<class>.txt files, that are included in git.txt (hence resulting git.1 and git.html). While it works, it tries to make sure that a command that has its own documentation page at Documentation/git-<command>.txt is listed; otherwise we have a page for <command> to which there is no link from git.{1,html}. I think "make check-builtins" might also want to sanity check command-list.txt.
On Mon, Oct 28, 2019 at 10:24:31AM +0900, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > See that last "no link: git-bisect" line? That's what happened to > > catch my eyes when Derrick forgot to add his new 'sparse-checkout' > > builtin to 'command-list.txt'. I still haven't looked up what that > > 'no link' is supposed to mean, but if it were an error, then we would > > Build procedure for the Documentation uses ../command-list.txt as > its input to produce cmds-<class>.txt files, that are included in > git.txt (hence resulting git.1 and git.html). While it works, it > tries to make sure that a command that has its own documentation > page at Documentation/git-<command>.txt is listed; otherwise we have > a page for <command> to which there is no link from git.{1,html}. Oh, then it doesn't quite do what I thought it does. > I think "make check-builtins" might also want to sanity check > command-list.txt. I haven't noticed that we have something like that. FWIW, our documentation CI job runs it already. And indeed it can be easily extended to check 'command-list.txt' as well, but then it will find some other builtins not included in 'command-list.txt', namely: bisect--helper env--helper fsck-objects init-db merge-ours merge-recursive merge-subtree remote-ext remote-fd submodule--helper I think it makes sense not to include the '*--helper' commands, or 'fsck-objects' and 'init-db', but I'm not sure about the others. OTOH, it won't help if we add a new git-foo script and forgot to include it in 'command-list.txt'.
Hi Gábor, On Sat, 26 Oct 2019, SZEDER Gábor wrote: > On Fri, Oct 25, 2019 at 06:08:57PM -0700, Jonathan Nieder wrote: > > > Users can discover commands and their brief usage by running 'git help > > > git' or 'git help -a'; both of these pages list all available commands > > > based on the contents of 'command-list.txt'. That means adding a new > > > command there is an important part of the new command process, and > > > therefore belongs in the new command tutorial. > > > > Makes sense. > > > > Not about this patch: is there a way to detect this automatically? > > E.g. if a command in git.c::commands doesn't appear in > > command-list.txt, could we make Git fail "make test"? > > We almost detect this already: > > $ sed -i -e '/^git-bisect/d' command-list.txt > $ make check-docs > make -C Documentation lint-docs > make[1]: Entering directory '/home/szeder/src/git/Documentation' > GEN cmd-list.made > GEN doc.dep > make[2]: Entering directory '/home/szeder/src/git' > make[2]: 'GIT-VERSION-FILE' is up to date. > make[2]: Leaving directory '/home/szeder/src/git' > make[2]: Entering directory '/home/szeder/src/git' > make[2]: 'GIT-VERSION-FILE' is up to date. > make[2]: Leaving directory '/home/szeder/src/git' > LINT lint-docs > make[1]: Leaving directory '/home/szeder/src/git/Documentation' > no link: git-bisect > $ echo $? > 0 > > See that last "no link: git-bisect" line? That's what happened to > catch my eyes when Derrick forgot to add his new 'sparse-checkout' > builtin to 'command-list.txt'. I still haven't looked up what that > 'no link' is supposed to mean, but if it were an error, then we would > have surely detected the missing entry in 'command-list.txt' in out CI > builds. FWIW I think the only reason that this is not an error in `check-docs` (which _is_ run by our CI/PR builds, in the `Documentation` job) is that it used to be buggy. However, I think I managed to address the remaining issues in `js/misc-doc-fixes` (and maybe there were a couple spill-overs into `js/check-docs-exe`). In short: I think we can make this type of issue trigger an error in `check-docs` now (as in: non-zero exit code). Ciao, Dscho > > Another possibility would be to auto-generate that long list of > 'cmd_foo()' function declaration in 'builtin.h' from > 'command-list.txt', by adding a 'builtin' attribute to mark builtin > commands. > >
diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index 5e9b808f5f..12b7256454 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -534,6 +534,28 @@ you want to pass as a parameter something which would usually be interpreted as a flag.) `parse_options()` will terminate parsing when it reaches `--` and give you the rest of the options afterwards, untouched. +Now that you have a usage hint, you can teach Git how to show it in the general +command list shown by `git help git` or `git help -a`, which is generated from +`command-list.txt`. Find the line for 'git-pull' so you can add your 'git-psuh' +line above it in alphabetical order. Now, we can add some attributes about the +command which impacts where it shows up in the aforementioned help commands. The +top of `command-list.txt` shares some information about what each attribute +means; in those help pages, the commands are sorted according to these +attributes. `git psuh` is user-facing, or porcelain - so we will mark it as +"mainporcelain". For "mainporcelain" commands, the comments at the top of +`command-list.txt` indicate we can also optionally add an attribute from another +list; since `git psuh` shows some information about the user's workspace but +doesn't modify anything, let's mark it as "info". Make sure to keep your +attributes in the same style as the rest of `command-list.txt` using spaces to +align and delineate them: + +---- +git-prune-packed plumbingmanipulators +git-psuh mainporcelain info +git-pull mainporcelain remote +git-push mainporcelain remote +---- + Build again. Now, when you run with `-h`, you should see your usage printed and your command terminated before anything else interesting happens. Great!
Users can discover commands and their brief usage by running 'git help git' or 'git help -a'; both of these pages list all available commands based on the contents of 'command-list.txt'. That means adding a new command there is an important part of the new command process, and therefore belongs in the new command tutorial. Teach new users how to add their command, and include a brief overview of how to discover which attributes to place on the command in the list. Since 'git psuh' prints some workspace info, doesn't modify anything, and is targeted as a user-facing porcelain command, list it as a 'mainporcelain' and 'info' command. As the usage string is required to generate this documentation, don't add the command to the list until after the usage string is added to the tutorial. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/MyFirstContribution.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)