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

From: Ian Wienand <iwienand@redhat.com>
Date: May 23, 2024, 00:38 UTC
Message-ID: <Zk6P8AMIBUt5X1A1@fedora19.localdomain>
In-Reply-To: <xmqq8r017rtw.fsf@gitster.g>

On Wed, May 22, 2024 at 09:07:55AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> 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 :).

Show 17 quoted lines

> 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

Show 8 quoted lines

> 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.

Show 16 quoted lines

> > 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