git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
	Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Subject: Re: [RFC] Hook management via 'git hooks' command
Date: Tue, 26 Nov 2019 00:28:06 +0000
Message-ID: <20191126002806.GD2404748@camp.crustytoothpaste.net> (raw)
In-Reply-To: <20191125222113.GA83137@google.com>

[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]

On 2019-11-25 at 22:21:13, Emily Shaffer wrote:
> I think that you are saying we should do the nice equivalent of:
> 
>   system("git lfs pre-push");
> 
> and tack the args onto the end. (I suppose that's run-command.h, but I'm
> trying to use a shorthand that is easy to understand.)

Yeah, I'm proposing we run the command using run-command.c with the
use_shell flag, which does something very much like that, except a
little bit saner.

> It seems like this would solve the PATH issue, yes. However, I feel
> tentative because of pushback on that approach in the bugreport review.
> Hmmm. I think this is different, because the user understands that the
> hook they configure themselves will be invoked on the shell of their
> choosing. That is, I think run-command.h with "C:\myhook.exe" would
> still work, no?

This will always use /bin/sh, as we do for editors.

bash on Windows does understand the Windows paths and works correctly in
this case, AIUI, so that should be fine.

> Will this approach "just work" for Windows, et al.?

Yes.  Windows ships with bash (or in Portable Git, busybox sh), which is
not only required to run the testsuite, but required to invoke editors.

> > I do, however, think we should require folks to have a suitable hook
> > that accepts the right arguments.  So "git grep blahblah" isn't a valid
> > hook in most cases, because it doesn't take the right arguments and read
> > the right data from stdin if necessary.
> 
> I'm not sure how we would guarantee that. Are you suggesting we should
> try dry running a hook somehow when it's added with 'git hook add'? Even
> doing that won't stop someone from popping open ~/.gitconfig with nano
> and adding their hook that doesn't take the right args that way.

We don't need to guarantee that.  We just need to document it, so that
(a) people write hooks in the expected way and (b) if people don't, we
can point them to the documentation and explain why their hooks don't
work.  I can see people thinking of this as a set of commands that just
checks exit statuses, and there's likely to be confusion.

> > I'm just thinking about existing hook wrappers that invoke multiple
> > scripts at the moment, something like how
> > https://gist.github.com/mjackson/7e602a7aa357cfe37dadcc016710931b works
> > at the moment and how we'd replace that with a config-based model.
> > 
> > I think using the shell avoids the entire proposal, because it then
> > becomes trivial to script that in the command and port it over, since we
> > can use my ugly hack above.
> 
> Still not sure I understand what the issue was before. Is it that the
> trampoline script needs to know $PATH to be able to invoke the child
> hooks in hookname.d? Or it's because the current working directory
> isn't clear, so a relative path in the trampoline script may not be
> resolved well?

I think we need to have either (a) a way to explicitly invoke hooks
underneath the hook directory or (b) a shell invocation to allow looking
up the hook directory.  People want to have per-repository hooks that
are not a part of the project, and they need a place to store them,
which is logically the hook directory.

If we want to allow people to have multiple hooks under something like
.git/hooks/pre-push.d, then we need to have a way to wire them up in the
configuration using the correct location of the hook directory instead
of using a helper script like the one I linked above.

We don't know that the hook directory will necessarily be under
.git/hooks, so if the user has moved it elsewhere, we'd want to follow
that.  A relative path would be wrong if the user changed the hook
directory to a different location.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-16  1:11 Emily Shaffer
2019-11-16  5:45 ` brian m. carlson
2019-11-18 22:38   ` Emily Shaffer
2019-11-19  0:51     ` brian m. carlson
2019-11-23  1:19       ` Emily Shaffer
2019-11-25  3:04         ` brian m. carlson
2019-11-25 22:21           ` Emily Shaffer
2019-11-25 22:45             ` Emily Shaffer
2019-11-26  0:28             ` brian m. carlson [this message]
2019-11-26  0:56               ` Emily Shaffer
2019-11-26  2:41                 ` brian m. carlson
2019-12-02 23:46                   ` Emily Shaffer

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191126002806.GD2404748@camp.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=avarab@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git