On Wed, May 22, 2024 at 09:07:55AM -0700, Junio C Hamano wrote:
> Ian Wienand <iwienand@xxxxxxxxxx> writes:
>
> > For a "split" alias, e.g. test = "!echo $*" you will see
> >
> > $ GIT_TRACE=1 git test hello
> > 11:00:45.959420 git.c:755 trace: exec: git-test hello
> > 11:00:45.959737 run-command.c:657 trace: run_command: git-test hello
> > 11:00:45.961424 run-command.c:657 trace: run_command: 'echo $*' hello
> > 11:00:45.961528 run-command.c:437 trace: prepare_cmd: /bin/sh -c 'echo $* "$@"' 'echo $*' hello
> > hello hello
> >
> > which clearly shows you the appended "$@".
>
> A question and a comment on this part.
>
> Who are "you" in this context?
Heh, "you" is the person who has been given a large git alias that
probably should have been written some other way (but likely due to
the fact that stuffing all the workflow logic into a git config-file
means that nothing extra like shell/python scripts needed to be
distributed) that "you" have to now debug :) I feel the extra info in
GIT_TRACE output showing what is _actually_ happening after
prepare_cmd would really help this person (who was me :).
> It is somewhat surprising if we do not consistently add "$@" (or do
> an equivalent), as the point of adding "$@" is to allow an alias to
> specify "the initial part of a command line", e.g.
>
> [alias] lg = log --oneline
>
> to let you say "git lg" to mean "git log --oneline" and also you can
> say "git lg --graph" to mean "git log --oneline --graph". In other
> words, what you type after "git lg" makes "the rest of the command
> line", while alias gives "the initial part".
>
> Are you sure that with
>
> [alias] lg = log
>
> you get the rest of the command line ignored, in other words, if you
> say "git lg --oneline", it does not do "git log --oneline"?
I'm not quite sure I'm following here, sorry. The behaviour is
different for "shell" aliases prefixed with !. There the stragtegy is
to append "$@", but only if there are arguments. Whatever the merits
or not of that, I feel like it's impossible to change because it's out
there now.
Now I've been looking, I've seen a number of interesting ways people
have been dealing with this. Some people have ignored it, for example
I've seen credential helpers use ":" to ignore it, like
"!echo password=\$${TOKEN_NAME}; :"
The git documentation for credential helpers gives another way,
defining an inline function, so the "$@" appended becomes function
arguments to "f()" like
"!f() { echo $1 $2 ; }; f"
Then I've seen other people nest in a "sh -c" call, and "fake" $0 so
that the appended "$@" becomes the $1 arguments to the interior shell
call (cue quoting nightmares!) like
"!sh -c 'echo $1 $2' foo"
Once you realise what is going on, these are somewhat understandable,
but the documentation as written doesn't guide you in the right
direction.
> The above does a bit too many things in a single patch to be
> reviewable. Perhaps make the above change in two patches?
Sure, I am happy to split this up
> You have a lengthy new text in the documentation to help confused
> users, but it looks to me that it places too much stress on the
> mechanics (i.e. '$@' is added to the command line) without saying
> much about the intent (i.e. you need to use '$@' to allow the
> command line invocation to supply "the rest of the command line"
> when you are running the alias via the shell). I've learned over
> the course of this project that readers learn better when the intent
> behind the mechanics is given in an understandable form.
Let me take your feedback here and work it into the split up change,
and we can discuss further. Thanks.
> > diff --git a/run-command.c b/run-command.c
> > index 1b821042b4..36b2b2f194 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -435,6 +435,7 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
> > }
> > }
> >
> > + trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
> > return 0;
> > }
>
> Hmph, we do not have much tests that look for 'trace: foo' in our
> test suite, but t0014 seems to use it. Don't we need to cover this
> new trace output in the test in the same patch (probably becomes
> patch 3 after the two documentation changes)?
Ok, thanks for the pointer, I'll look at expanding this one.
-i
