Re: [PATCH] alias: document caveats and add trace of prepared command

[Ian Wienand <iwienand@redhat.com>]

On Wed, May 22, 2024 at 09:07:55AM -0700, Junio C Hamano wrote:
> Ian Wienand <iwienand@redhat.com> 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