Re: [PATCH v4 3/9] hook: add list command

From: Emily Shaffer <emilyshaffer@google.com>
Date: Oct 6, 2020, 20:20 UTC
Message-ID: <20201006201957.GK331156@google.com>
In-Reply-To: <CAN0heSq1iVtJycjgh-iv2O6vNr6CLn69N3286hzkfu5sXLjf5w@mail.gmail.com>

On Sun, Sep 27, 2020 at 09:23:35PM +0200, Martin Ågren wrote:

> 
> Hi Emily,

Firstly, thanks for the doc review - this is great stuff.

Show 10 quoted lines

> 
> On Wed, 9 Sep 2020 at 02:54, Emily Shaffer <emilyshaffer@google.com> wrote:
> 
> >  DESCRIPTION
> >  -----------
> >  You can list, add, and modify hooks with this command.
> 
> (BTW, I think this patch could teach this to say "You can list hooks
> with this command." If/when we add the other commands, we can expand
> on this.)

Done. I sort of glued this together with Jonathan Nieder's suggestion in the setup patch, and ended up saying "later you will be able to blah".

Show 8 quoted lines

> 
> > +This command parses the default configuration files for sections "hook" and
> > +"hookcmd". "hook" is used to describe the commands which will be run during a
> 
> I propose s/"hook"/`hook`/ and similar to set this as monospace since we
> are discussing configuration sections. If we want to avoid starting
> sentences with "hook" (or `hookcmd`; do we?), maybe something like "The
> section `hook` ..." would work fine.

Nice - done. I don't see much problem with starting a sentence with monospaced lower-cased section name... someone can disagree with me :)

Show 9 quoted lines

> 
> > +particular hook event; commands are run in config order. "hookcmd" is used to
> 
> "config order" feels a bit too colloquial/vague. You use the same phrase
> in the commit message and I think it works well there for the indented
> audience. But for this document, I'm not so sure. How about
> 
>   Commands are run in the order they are encountered as the Git
>   configuration files are processed (see linkgit:git-config[1]).

I don't mind colloquial - I think that improves the readability of user documentation - but you're right that it's vague. "...commands are run in the order Git encounters them during the configuration parse (see linkgitblah)" seemed like an okay balance to me.

> 
> ? It's also quite possible that "config order" hits the exact right tone
> -- please trust your judgment.

Nah, I think you're right that "config order" is easily understood by Git devs, but probably not by Git users. I like that linking out to the config doc invites users to also learn a little more about how config files work :)

Show 18 quoted lines

> 
> > +describe attributes of a specific command. If additional attributes don't need
> > +to be specified, a command to run can be specified directly in the "hook"
> > +section; if a "hookcmd" by that name isn't found, Git will attempt to run the
> > +provided value directly. For example:
> 
> > +  [hook "post-commit"]
> > +    command = "linter"
> > +    command = "~/typocheck.sh"
> > +
> > +  [hookcmd "linter"]
> > +    command = "/bin/linter --c"
> 
> Hmm. "hook", "command" and "hookcmd". Should that be "cmd", or
> "hookcommand"? I'd favour the latter, but the current proposal somehow
> feels asymmetric. (If code uses, and is consistent about using,
> "hookcmd" that's another thing entirely, I think. It's just that for the
> configuration, it looks a bit odd.)

I'm not entirely in love with the name "hookcmd" but somehow I like "hookcommand" even less - especially since you end up with "hook.command" referencing a "hookcommand" which also has a "hookcommand.command" - blech.

Some possible alternatives to "hookcmd":
- hookmodule/hook-module
- reusable-hook
- hook-with-options/hook-options (nah, this sounds like it means
  "options for hook execution")
- hook-details/detailed-hook
- named-hook

I'll think on this more. I like "named-hook" quite a lot. Very interested in hearing other ideas - "the two hardest problems in computer science are naming, cache invalidation, and off-by-one errors" ;)

Show 14 quoted lines

> 
> > +List the hooks which have been configured for <hook-name>. Hooks appear
> 
> `<hook-name>` with backticks.
> 
> > +in the order they should be run, and note the config scope where the relevant
> > +`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
> 
> I had to read and re-read this a few times. The "and note the" does not
> mean "and please observe that", but rather "and they make note of". Not
> sure how that can be done clearer. The second thing that tripped me up
> was that last part. Maybe end the sentence after "specified", then add
> something like "The scope is not affected by if and where
> `hookcmd.<hook-name>.command` appears.".

Occam's Razor suggests "Hooks appear in the order they should be run, and print the config scope blah". Thanks for pointing out "and note that" collision - I never use that phrase so it didn't occur to me!

Show 17 quoted lines

> 
> I think you could add
> 
>   CONFIGURATION
>   -------------
>   include::config/hook.txt[]
> 
> here and add such a file
> 
>   hook.<hook-name>.command::
>          ...
> 
>   hookcmd.<hook-name>.command::
>          ...
> 
> where you define/describe those items. And you can include it from
> config.txt as well.

Yes, totally. Thanks.

 - Emily