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:
signature.asc
Description: PGP signature
