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

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

[Ian Wienand]

There are a number of hidden caveats when using command/shell
expansion ("!") in an alias.  The command is exec'd, unless it can be
split, when it is then run as an argument to "sh -c".  Split commands
have "$@" appended.

Firstly this updates the documentation to explain this a little
clearer.

Secondly, this adds a trace point in prepare_cmd where this
substitution is done, so we can see the command being run without
having to resort to strace/code inspection.  e.g. "test = !echo" you
will show:

 $ GIT_TRACE=1 git test hello
 10:58:56.877234 git.c:755               trace: exec: git-test hello
 10:58:56.877382 run-command.c:657       trace: run_command: git-test hello
 10:58:56.878655 run-command.c:657       trace: run_command: echo hello
 10:58:56.878747 run-command.c:437       trace: prepare_cmd: /usr/bin/echo hello
 hello

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 "$@".  This can be very helpful
when an alias is giving you an unexpected synatx error that is very
difficult figure out from only the run_command trace point,
e.g. test = "!for i in 1 2 3; do echo $i; done"

 $ GIT_TRACE=1 test hello
 11:02:39.813030 git.c:755               trace: exec: git-test hello
 11:02:39.813233 run-command.c:657       trace: run_command: git-test hello
 11:02:39.814384 run-command.c:657       trace: run_command: 'for i in 1 2 3; do echo $i; done' hello
 11:02:39.814468 run-command.c:437       trace: prepare_cmd: /bin/sh -c 'for i in 1 2 3; do echo $i; done "$@"' 'for i in 1 2 3; do echo $i; done' hello
 for i in 1 2 3; do echo $i; done: -c: line 1: syntax error near unexpected token `"$@"'
 for i in 1 2 3; do echo $i; done: -c: line 1: `for i in 1 2 3; do echo $i; done "$@"'

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 26 +++++++++++++++++++++-----
 run-command.c                  |  1 +
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..a3f090d79d 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,24 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
+* Single commands will be executed directly.  Commands that can be "split"
+  (contain whitespace or shell-reserved characters) will be run as shell
+  commands via an argument to `sh -c`.
+* When arguments are present to a "split" command running in a shell,
+  the shell command will have `"$@"` appended.  The first non-command
+  argument to `sh -c` (i.e. `$0` to the command) is always the alias
+  string, and other user specified arguments will follow.
+** This may initially be confusing if your command references argument
+   variables or is not expecting the presence of `"$@"`.  For example:
+   `alias.echo = "!echo $1"` when run as `git echo arg` will execute
+   `sh -c "echo $1 $@" "echo $1" "1"` resulting in output `1 1`.
+   An alias `alias.for = "!for i in 1 2 3; do echo $i; done"` will fail
+   if any arguments are specified to `git for` as the appended `"$@"` will
+   create invalid shell syntax.  Setting `GIT_TRACE=1` can help debug
+   the command being run.
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;
 }
 
-- 
2.45.1

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

[Eric Sunshine]

On Tue, May 21, 2024 at 10:42 PM Ian Wienand <iwienand@redhat.com> wrote:
> [...]
> 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 "$@".  This can be very helpful
> when an alias is giving you an unexpected synatx error that is very

s/synatx/syntax/

> difficult figure out from only the run_command trace point,
> e.g. test = "!for i in 1 2 3; do echo $i; done"
> [...]
> Signed-off-by: Ian Wienand <iwienand@redhat.com>

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

[Junio C Hamano]

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?

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

>  Documentation/config/alias.txt | 26 +++++++++++++++++++++-----
>  run-command.c                  |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
> index 01df96fab3..a3f090d79d 100644
> --- a/Documentation/config/alias.txt
> +++ b/Documentation/config/alias.txt
> @@ -21,8 +21,24 @@ If the alias expansion is prefixed with an exclamation point,
>  it will be treated as a shell command.  For example, defining
>  `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
>  `git new` is equivalent to running the shell command
> -`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
> -executed from the top-level directory of a repository, which may
> -not necessarily be the current directory.
> -`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
> -from the original current directory. See linkgit:git-rev-parse[1].
> +`gitk --all --not ORIG_HEAD`.  Note:
> ++
> +* Shell commands will be executed from the top-level directory of a
> +  repository, which may not necessarily be the current directory.
> +* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
> +  from the original current directory. See linkgit:git-rev-parse[1].
> +* Single commands will be executed directly.  Commands that can be "split"
> +  (contain whitespace or shell-reserved characters) will be run as shell
> +  commands via an argument to `sh -c`.
> +* When arguments are present to a "split" command running in a shell,
> +  the shell command will have `"$@"` appended.  The first non-command
> +  argument to `sh -c` (i.e. `$0` to the command) is always the alias
> +  string, and other user specified arguments will follow.
> +** This may initially be confusing if your command references argument
> +   variables or is not expecting the presence of `"$@"`.  For example:
> +   `alias.echo = "!echo $1"` when run as `git echo arg` will execute
> +   `sh -c "echo $1 $@" "echo $1" "1"` resulting in output `1 1`.
> +   An alias `alias.for = "!for i in 1 2 3; do echo $i; done"` will fail
> +   if any arguments are specified to `git for` as the appended `"$@"` will
> +   create invalid shell syntax.  Setting `GIT_TRACE=1` can help debug
> +   the command being run.

The above does a bit too many things in a single patch to be
reviewable.  Perhaps make the above change in two patches?

 (1) Reformulate the existing test into "Note:" followed by a
     bulletted list.

 (2) Add new items to the updated and easier-to-read form prepared
     by the first patch.

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.

I think the idea is "we want the 'concatenation of what the alias
supplies and what the user gives when invoking the alias from the
command line' to work sensibly".  The most generic way to do so when
processing "git lg --graph -3" with "[alias] lg = !git log --oneline" is

    sh -c 'git log --oneline "$@"' - '--graph' '-3'

(readers can try this at home by adding 'echo ' before 'log').  We
may try to "optimize" the most generic pattern when there is no need
to use "sh -c" go a more direct route.  For example, if you have

    [alias] !foo = bar

then there is no need to use "sh -c" in the first place.  "git foo
baz quux" should behave just like when you said "sh bar baz quux" on
the command line, i.e. execute your shell script "bar" and give
"baz" and "quux" as two arguments to that script.  We can prepare
argv[] = ("sh", "bar", "baz", "quux") and fork-exec that, so the
command line the underlying exec(2) call sees may not have "$@" (and
it will not have "-c", either).

You can undo the "optimization" and rewrite the command line to

    sh -c 'bar "$@"' - 'baz' 'quux'

and it should mean the same thing.

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

Thanks.

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

[Ian Wienand]

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

[PATCH v2 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v2 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.
---
 Documentation/config/alias.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..51fa876c91 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,24 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* If the shell alias is the full path to a binary, it will be executed
+  directly with any arguments.
+* If the alias contains any whitespace or reserved characters, it will
+  be considered an inline script and run as an argument to `sh -c`.
+* When running as a script, Git appends "$@" to the alias shell
+  command when arguments are present.  If there are no arguments,
+  `"$@"` will not be appended.
+** This may initially be confusing if your alias script references
+   argument variables, or is otherwise not expecting the presence of
+   `"$@"`.  For example: `alias.echo = "!echo $1"` when run as `git
+   echo arg` will actually execute `sh -c "echo $1 $@" "echo $1"
+   "arg"` resulting in output `arg arg`.  An alias `alias.for = "!for
+   i in 1 2 3; do echo $i; done"` will fail if any arguments are
+   specified to `git for` as the appended `"$@"` will create invalid
+   shell syntax.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.echo = "!e() {
+   echo $* ; }; e" will work as expected, with the function `e`
+   receiving any arugments from the command-line.
+** Setting `GIT_TRACE=1` can help debug the command being run.
-- 
2.45.1

[PATCH v2 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in prepare_cmd, so we can see the actual
command being run without having to resort to strace/code inspection.

e.g. "test = !echo" when run under GIT_TRACE will show:

 $ GIT_TRACE=1 git test hello
 10:58:56.877234 git.c:755               trace: exec: git-test hello
 10:58:56.877382 run-command.c:657       trace: run_command: git-test hello
 10:58:56.878655 run-command.c:657       trace: run_command: echo hello
 10:58:56.878747 run-command.c:437       trace: prepare_cmd: /usr/bin/echo hello
 hello

A "split" alias, e.g. test = "!echo $*" will show the shell wrapping
and appending of "$@".

 $ 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

For example, this can be very helpful when an alias is giving you an
unexpected syntax error that is very difficult figure out from only
the run_command trace point, e.g.

test = "!for i in 1 2 3; do echo $i; done"

will fail if there is an argument given, we can see why from the
output.

 $ GIT_TRACE=1 test hello
 11:02:39.813030 git.c:755               trace: exec: git-test hello
 11:02:39.813233 run-command.c:657       trace: run_command: git-test hello
 11:02:39.814384 run-command.c:657       trace: run_command: 'for i in 1 2 3; do echo $i; done' hello
 11:02:39.814468 run-command.c:437       trace: prepare_cmd: /bin/sh -c 'for i in 1 2 3; do echo $i; done "$@"' 'for i in 1 2 3; do echo $i; done' hello
 for i in 1 2 3; do echo $i; done: -c: line 1: syntax error near unexpected token `"$@"'
 for i in 1 2 3; do echo $i; done: -c: line 1: `for i in 1 2 3; do echo $i; done "$@"'

A test case is added.
---
 run-command.c    | 1 +
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 8 insertions(+)

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;
 }
 
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..75c8763a6c 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,11 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows full prepared command' '
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo argument 2>output &&
+	cp output /tmp/output &&
+	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v2 1/3] Documentation: alias: rework notes into points

[Eric Sunshine]

On Thu, May 23, 2024 at 12:22 AM Ian Wienand <iwienand@redhat.com> wrote:
> There are a number of caveats when using aliases.  Rather than
> stuffing them all together in a paragraph, let's separate them out
> into individual points to make it clearer what's going on.
> ---

Missing sign-off.

[PATCH v3 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v3 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..51fa876c91 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,24 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* If the shell alias is the full path to a binary, it will be executed
+  directly with any arguments.
+* If the alias contains any whitespace or reserved characters, it will
+  be considered an inline script and run as an argument to `sh -c`.
+* When running as a script, Git appends "$@" to the alias shell
+  command when arguments are present.  If there are no arguments,
+  `"$@"` will not be appended.
+** This may initially be confusing if your alias script references
+   argument variables, or is otherwise not expecting the presence of
+   `"$@"`.  For example: `alias.echo = "!echo $1"` when run as `git
+   echo arg` will actually execute `sh -c "echo $1 $@" "echo $1"
+   "arg"` resulting in output `arg arg`.  An alias `alias.for = "!for
+   i in 1 2 3; do echo $i; done"` will fail if any arguments are
+   specified to `git for` as the appended `"$@"` will create invalid
+   shell syntax.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.echo = "!e() {
+   echo $* ; }; e" will work as expected, with the function `e`
+   receiving any arugments from the command-line.
+** Setting `GIT_TRACE=1` can help debug the command being run.
-- 
2.45.1

[PATCH v3 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in prepare_cmd, so we can see the actual
command being run without having to resort to strace/code inspection.

e.g. "test = !echo" when run under GIT_TRACE will show:

 $ GIT_TRACE=1 git test hello
 10:58:56.877234 git.c:755               trace: exec: git-test hello
 10:58:56.877382 run-command.c:657       trace: run_command: git-test hello
 10:58:56.878655 run-command.c:657       trace: run_command: echo hello
 10:58:56.878747 run-command.c:437       trace: prepare_cmd: /usr/bin/echo hello
 hello

A "split" alias, e.g. test = "!echo $*" will show the shell wrapping
and appending of "$@".

 $ 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

For example, this can be very helpful when an alias is giving you an
unexpected syntax error that is very difficult figure out from only
the run_command trace point, e.g.

test = "!for i in 1 2 3; do echo $i; done"

will fail if there is an argument given, we can see why from the
output.

 $ GIT_TRACE=1 test hello
 11:02:39.813030 git.c:755               trace: exec: git-test hello
 11:02:39.813233 run-command.c:657       trace: run_command: git-test hello
 11:02:39.814384 run-command.c:657       trace: run_command: 'for i in 1 2 3; do echo $i; done' hello
 11:02:39.814468 run-command.c:437       trace: prepare_cmd: /bin/sh -c 'for i in 1 2 3; do echo $i; done "$@"' 'for i in 1 2 3; do echo $i; done' hello
 for i in 1 2 3; do echo $i; done: -c: line 1: syntax error near unexpected token `"$@"'
 for i in 1 2 3; do echo $i; done: -c: line 1: `for i in 1 2 3; do echo $i; done "$@"'

A test case is added.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 run-command.c    | 1 +
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 8 insertions(+)

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;
 }
 
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..75c8763a6c 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,11 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows full prepared command' '
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo argument 2>output &&
+	cp output /tmp/output &&
+	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v2 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

On Thu, May 23, 2024 at 12:27:17AM -0400, Eric Sunshine wrote:
> On Thu, May 23, 2024 at 12:22 AM Ian Wienand <iwienand@redhat.com> wrote:
> > There are a number of caveats when using aliases.  Rather than
> > stuffing them all together in a paragraph, let's separate them out
> > into individual points to make it clearer what's going on.
> > ---
> 
> Missing sign-off.

Sigh .. thanks sent v3

-i

Re: [PATCH v3 1/3] Documentation: alias: rework notes into points

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> There are a number of caveats when using aliases.  Rather than
> stuffing them all together in a paragraph, let's separate them out
> into individual points to make it clearer what's going on.

Nicely explained.

> diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
> index 01df96fab3..40851ef429 100644
> --- a/Documentation/config/alias.txt
> +++ b/Documentation/config/alias.txt
> @@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
>  it will be treated as a shell command.  For example, defining
>  `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
>  `git new` is equivalent to running the shell command
> +`gitk --all --not ORIG_HEAD`.  Note:
> ++
> +* Shell commands will be executed from the top-level directory of a
> +  repository, which may not necessarily be the current directory.
> +* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
> +  from the original current directory. See linkgit:git-rev-parse[1].

Looking for '[NOTE]' in the Documentation/ files finds sections
marked up like this (this one is from Documentation/git-blame.txt):

    ...
    parser (which should be quite natural for most scripting languages).
    +
    [NOTE]
    For people who do parsing: to make it more robust, just ignore any
    lines between the first and last one ("<sha1>" and "filename" lines)
    ...

and its rendition looks like this:

https://git.github.io/htmldocs/git-blame.html#:~:text=most%20scripting%20languages).-,Note,-For%20people%20who

I am undecided if it gives a better presentation, especially when we
are giving bulletted list, so let's take the patch as-is and leave
it to interested folks to explore the use of [NOTE] _after_ this set
of patches lands as #leftoverbit follow-up topic.

Thanks.

Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> This adds a trace point in prepare_cmd, so we can see the actual
> command being run without having to resort to strace/code inspection.
>
> e.g. "test = !echo" when run under GIT_TRACE will show:
>
>  $ GIT_TRACE=1 git test hello
>  10:58:56.877234 git.c:755               trace: exec: git-test hello
>  10:58:56.877382 run-command.c:657       trace: run_command: git-test hello
>  10:58:56.878655 run-command.c:657       trace: run_command: echo hello
>  10:58:56.878747 run-command.c:437       trace: prepare_cmd: /usr/bin/echo hello
>  hello

Nice.

> A "split" alias, e.g. test = "!echo $*" will show the shell wrapping
> and appending of "$@".
>
>  $ 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

Nice again.

But ...

> For example, this can be very helpful when an alias is giving you an
> unexpected syntax error that is very difficult figure out from only
> the run_command trace point, e.g.
>
> test = "!for i in 1 2 3; do echo $i; done"
>
> will fail if there is an argument given, we can see why from the
> output.

... if the reader truly understands "the alias gives the command and
its leading arguments, to which the invocation can supply even more
arguments", the reader wouldn't be writing such a command line to
begin with, no?

So I find the example a bit suboptimal.  Hopefully additional
explanation in patch 2/3 stressed on that point well enough with
much more stress than it talks about the implementation detail of
using "sh -c" and "$@", so that readers who read it would not even
dream of writing such an alias in the first place.

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

Nice addition that is obviously correct.

> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 95568342be..75c8763a6c 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -44,4 +44,11 @@ test_expect_success 'run-command formats empty args properly' '
>      test_cmp expect actual
>  '
>  
> +test_expect_success 'tracing a shell alias with arguments shows full prepared command' '
> +	git config alias.echo "!echo \$*" &&
> +	env GIT_TRACE=1 git echo argument 2>output &&
> +	cp output /tmp/output &&
> +	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
> +'

This is probably too specific search string, I suspect, given that
runcommand.c:prepare_shell_cmd() uses SHELL_PATH or "sh" so if your
SHELL_PATH is anything but /bin/sh (or if you are unlucky enough to
be running this test on Windows), the pattern would not match.
You'd want to loosen it a bit, perhaps with "/bin/sh" -> ".*", as
the rest of the output are expected to stay constant.

By the way, common to this step and also to the previous step,
you'd want to use

	echo "$@"

instead of

	echo $*

to encourage better variable reference hygiene.  It makes difference
when any arguments given to "e" has whitespace, i.e.

    $ sh -c 'echo $*'   - a 'b    c'
    a b c
    $ sh -c 'echo "$@"' - a 'b    c'
    a b    c

Thanks.

Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	git config alias.echo "!echo \$*" &&
>> +	env GIT_TRACE=1 git echo argument 2>output &&
>> +	cp output /tmp/output &&
>> +	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
>> +'
>
> This is probably too specific search string, ...

And of course, 'seen' no longer passes the test with this topic
merged when run with SHELL_PATH=/bin/dash for obvious reasons.

I've added the following band-aid to help 'seen' pass CI, but this
has to be fixed in the main patch by squashing it in.

Thanks.

--- >8 ---
Subject: [PATCH] fixup! run-command: show prepared command

Do not assume everybody sets SHELL_PATH to /bin/sh
---
 t/t0014-alias.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 75c8763a6c..0d42d2b454 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -48,7 +48,8 @@ test_expect_success 'tracing a shell alias with arguments shows full prepared co
 	git config alias.echo "!echo \$*" &&
 	env GIT_TRACE=1 git echo argument 2>output &&
 	cp output /tmp/output &&
-	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
+	# ".*" can be "sh", or SHELL_PATH (not always "/bin/sh")
+	test_grep "^trace: prepare_cmd: .* -c '\''echo \$\* \"\$@\"" output
 '
 
 test_done
-- 
2.45.1-246-gb9cfe4845c

Re: [PATCH v3 3/3] run-command: show prepared command

[Ian Wienand]

On Thu, May 23, 2024 at 08:29:21AM -0700, Junio C Hamano wrote:
> > For example, this can be very helpful when an alias is giving you an
> > unexpected syntax error that is very difficult figure out from only
> > the run_command trace point, e.g.
> >
> > test = "!for i in 1 2 3; do echo $i; done"
> >
> > will fail if there is an argument given, we can see why from the
> > output.
> 
> ... if the reader truly understands "the alias gives the command and
> its leading arguments, to which the invocation can supply even more
> arguments", the reader wouldn't be writing such a command line to
> begin with, no?
> 
> So I find the example a bit suboptimal.  Hopefully additional
> explanation in patch 2/3 stressed on that point well enough with
> much more stress than it talks about the implementation detail of
> using "sh -c" and "$@", so that readers who read it would not even
> dream of writing such an alias in the first place.

Right; I was seeing this in a more convoluted way via our tool but
essentially the same issue.  I was just looking for the simplest thing
that also gave the syntax error output, which I thought was something
people might search for (the "unexpected "$@" stuff).

Should I just leave as is?

> > +test_expect_success 'tracing a shell alias with arguments shows full prepared command' '
> > +	git config alias.echo "!echo \$*" &&
> > +	env GIT_TRACE=1 git echo argument 2>output &&
> > +	cp output /tmp/output &&
> > +	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
> > +'
> 
> This is probably too specific search string, I suspect, given that
> runcommand.c:prepare_shell_cmd() uses SHELL_PATH or "sh" so if your
> SHELL_PATH is anything but /bin/sh (or if you are unlucky enough to
> be running this test on Windows), the pattern would not match.
> You'd want to loosen it a bit, perhaps with "/bin/sh" -> ".*", as
> the rest of the output are expected to stay constant.

OK, should this perhaps just look for '^trace: prepare_cmd.*'?

My initial thinking was to enforce seeing the "$@" appended, but
perhaps that is implementation details that don't really need to be
covered; the interesting thing is we show the person tracing the full
command as constructed, so this is useful just to ensure the
tracepoint remains in place?

-i

Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Subject: [PATCH] fixup! run-command: show prepared command
>
> Do not assume everybody sets SHELL_PATH to /bin/sh
> ---
>  t/t0014-alias.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 75c8763a6c..0d42d2b454 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -48,7 +48,8 @@ test_expect_success 'tracing a shell alias with arguments shows full prepared co
>  	git config alias.echo "!echo \$*" &&
>  	env GIT_TRACE=1 git echo argument 2>output &&
>  	cp output /tmp/output &&
> -	test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
> +	# ".*" can be "sh", or SHELL_PATH (not always "/bin/sh")
> +	test_grep "^trace: prepare_cmd: .* -c '\''echo \$\* \"\$@\"" output
>  '

So with this applied on top of the topic, 'seen' does pass with
SHELL_PATH set to say /bin/dash, but this still fails CI jobs on
Windows.  

A sample run that failed (you'd probably need to be logged in as a
github user there):

  https://github.com/git/git/actions/runs/9216303673/job/25360383492#step:5:237

With the lack of "trace: prepare_cmd:" in the "output" file, on the
platform, perhaps the code is failing to find an executable for
"echo" on the $PATH and taking the early return codepath that sets
errno to ENOENT and returns -1?  Or it is that even prepare_cmd()
is not called?  I do not do Windows, so I'll stop here.

Oh by the way, I just noticed that there is an apparent leftover
debugging statement that accesses outside the test playpen.  When
you reroll the patch, please make sure that "cp output /tmp/output"
is removed.

Thanks.

Re: [PATCH v3 3/3] run-command: show prepared command

[Ian Wienand]

On Thu, May 23, 2024 at 11:09:01PM -0700, Junio C Hamano wrote:
> So with this applied on top of the topic, 'seen' does pass with
> SHELL_PATH set to say /bin/dash, but this still fails CI jobs on
> Windows.  

Sigh, it looks like windows uses prepare_shell_cmd() instead.  I think
it's still probably valid to dump the full command there for the same
reasons, which I'll add in v4.

I've reduced to the check to just look for the trace prefix.  This is
enough to ensure we don't regress by removing it; the exact details of
what comes out really isn't the main point -- it's that the log is
there at all to help you debug what is being run.

> Oh by the way, I just noticed that there is an apparent leftover
> debugging statement that accesses outside the test playpen.  When
> you reroll the patch, please make sure that "cp output /tmp/output"
> is removed.

Done

-i

[PATCH v4 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v4 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..51fa876c91 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,24 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* If the shell alias is the full path to a binary, it will be executed
+  directly with any arguments.
+* If the alias contains any whitespace or reserved characters, it will
+  be considered an inline script and run as an argument to `sh -c`.
+* When running as a script, Git appends "$@" to the alias shell
+  command when arguments are present.  If there are no arguments,
+  `"$@"` will not be appended.
+** This may initially be confusing if your alias script references
+   argument variables, or is otherwise not expecting the presence of
+   `"$@"`.  For example: `alias.echo = "!echo $1"` when run as `git
+   echo arg` will actually execute `sh -c "echo $1 $@" "echo $1"
+   "arg"` resulting in output `arg arg`.  An alias `alias.for = "!for
+   i in 1 2 3; do echo $i; done"` will fail if any arguments are
+   specified to `git for` as the appended `"$@"` will create invalid
+   shell syntax.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.echo = "!e() {
+   echo $* ; }; e" will work as expected, with the function `e`
+   receiving any arugments from the command-line.
+** Setting `GIT_TRACE=1` can help debug the command being run.
-- 
2.45.1

[PATCH v4 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in prepare_cmd (for windows,
prepare_shell_cmd), so we can see the actual command being run without
having to resort to strace/code inspection.

e.g. "test = !echo" when run under GIT_TRACE will show:

 $ GIT_TRACE=1 git test hello
 10:58:56.877234 git.c:755               trace: exec: git-test hello
 10:58:56.877382 run-command.c:657       trace: run_command: git-test hello
 10:58:56.878655 run-command.c:657       trace: run_command: echo hello
 10:58:56.878747 run-command.c:437       trace: prepare_cmd: /usr/bin/echo hello
 hello

A "split" alias, e.g. test = "!echo $*" will show the shell wrapping
and appending of "$@".

 $ 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

Prior changes have made the documentation around the internals of the
alias command execution clearer, but I have still found this detailed
view of the aliased command being run helpful for debugging purposes.

A test case is added.  This simply looks for the trace point output;
the details change depending on platform -- the important thing is
that we have a verbose log point, not so much the details of what
happens on each platform.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 run-command.c    | 2 ++
 t/t0014-alias.sh | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/run-command.c b/run-command.c
index 1b821042b4..13e35fb76e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -296,6 +296,7 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 	}
 
 	strvec_pushv(out, argv);
+	trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
 	return out->v;
 }
 
@@ -435,6 +436,7 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
 		}
 	}
 
+	trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
 	return 0;
 }
 
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..b7affbe93a 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,10 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo argument 2>output &&
+	test_grep "^trace: prepare_cmd:.*" output
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> On Thu, May 23, 2024 at 11:09:01PM -0700, Junio C Hamano wrote:
>> So with this applied on top of the topic, 'seen' does pass with
>> SHELL_PATH set to say /bin/dash, but this still fails CI jobs on
>> Windows.  
>
> Sigh, it looks like windows uses prepare_shell_cmd() instead.  I think
> it's still probably valid to dump the full command there for the same
> reasons, which I'll add in v4.

Ahh, it's the large conditional compilation in start_command().

On non-Windows side, we just do

	if (prepare_cmd(&argv, cmd) < 0) {
		failed_errno = errno;
		...
		goto end_of_spawn;
	}
	... pipe(), fork(), and exec() ...

but on Windows side, we have

	if (cmd->git_cmd)
		cmd->args.v = prepare_git_cmd(&nargv, sargv);
	else if (cmd->use_shell)
		cmd->args.v = prepare_shell_cmd(&nargv, sargv);

	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
				  (char**) cmd->env.v,
				  cmd->dir, fhin, fhout, fherr);

And the thing is, prepare_cmd() already has

	if (cmd->git_cmd) {
		prepare_git_cmd(out, cmd->args.v);
	} else if (cmd->use_shell) {
		prepare_shell_cmd(out, cmd->args.v);
	} else {
		strvec_pushv(out, cmd->args.v);
	}

I am wondering (and not suggesting to do this as a part of this
series, but to decide if we should leave a left-over-bits note here)
if the Windows side can be updated to also use prepare_cmd().  Do
folks a lot more familiar with Windows than I (cc'ed j6t and dscho)
have comments on this?

Anyway.

If you unconditionally add the printf's to both prepare_cmd() and
prepare_shell_cmd(), you'll see two printf output on a non-Windows
platform if we are spawning a shell cmd.

It appears that the best place to "show the prepared command" is
*NOT* in any of these prepare_* helper functions, but after these
two callers in start_command() receives the prepared command from
these helpers, namely:

        diff --git c/run-command.c w/run-command.c
        index 1b821042b4..9d0fa6afe4 100644
        --- c/run-command.c
        +++ w/run-command.c
        @@ -745,6 +745,7 @@ int start_command(struct child_process *cmd)
                                error_errno("cannot run %s", cmd->args.v[0]);
                        goto end_of_spawn;
                }
        +	/* Here show argv in the trace */

                if (pipe(notify_pipe))
                        notify_pipe[0] = notify_pipe[1] = -1;
        @@ -912,6 +913,7 @@ int start_command(struct child_process *cmd)
                        cmd->args.v = prepare_git_cmd(&nargv, sargv);
                else if (cmd->use_shell)
                        cmd->args.v = prepare_shell_cmd(&nargv, sargv);
        +	/* Here show cmd->args.v in the trace */

                cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
                                          (char**) cmd->env.v,

We would not have to do so and instead have trace-printf(s) only in
prepare_cmd() if/when the Windows side is updated to stop switching
between the prepare_git/prepare_shell and instead let prepare_cmd()
do the switching, (which may require an update to prepare_cmd() if
needed).  But until that happens, logging at the caller seems to be
the best approach among equally bad ones.

Thanks.

Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> On Thu, May 23, 2024 at 08:29:21AM -0700, Junio C Hamano wrote:
>> ... if the reader truly understands "the alias gives the command and
>> its leading arguments, to which the invocation can supply even more
>> arguments", the reader wouldn't be writing such a command line to
>> begin with, no?
>> 
>> So I find the example a bit suboptimal.  Hopefully additional
>> explanation in patch 2/3 stressed on that point well enough with
>> much more stress than it talks about the implementation detail of
>> using "sh -c" and "$@", so that readers who read it would not even
>> dream of writing such an alias in the first place.
>
> Right; I was seeing this in a more convoluted way via our tool but
> essentially the same issue.  I was just looking for the simplest thing
> that also gave the syntax error output, which I thought was something
> people might search for (the "unexpected "$@" stuff).
>
> Should I just leave as is?

If I found as-is would be good enough, I wouldn't have been
commenting on this.

Even in this third iteration, I still didn't see the added
documentation talk about the principle behind the design, i.e. what
you write after the "git your-alias" are appended to the command
line to be used as additional arguments.

Re: [PATCH v4 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> +test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
> +	git config alias.echo "!echo \$*" &&
> +	env GIT_TRACE=1 git echo argument 2>output &&
> +	test_grep "^trace: prepare_cmd:.*" output
> +'

If you run

    $ cd t && sh ./t0014-alias.sh -d && cat trash*.t0014-alias/output

you'll see two "prepare_cmd" logged, because you added one to
prepare_shell_cmd() and another to prepare_cmd().  If you were
tracing something that uses prepare_git_cmd() on Windows, you would
not see any trace output, on the other hand, I would suspect (I do
not do Windows so this is from code inspection only).

Perhaps squashing something like this is sufficient?

 run-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/run-command.c w/run-command.c
index 13e35fb76e..7eb68a541d 100644
--- c/run-command.c
+++ w/run-command.c
@@ -296,7 +296,6 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 	}
 
 	strvec_pushv(out, argv);
-	trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
 	return out->v;
 }
 
@@ -436,7 +435,6 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
 		}
 	}
 
-	trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
 	return 0;
 }
 
@@ -747,6 +745,7 @@ int start_command(struct child_process *cmd)
 			error_errno("cannot run %s", cmd->args.v[0]);
 		goto end_of_spawn;
 	}
+	trace_argv_printf(&argv.v[1], "trace: prepare_cmd:");
 
 	if (pipe(notify_pipe))
 		notify_pipe[0] = notify_pipe[1] = -1;
@@ -914,6 +913,7 @@ int start_command(struct child_process *cmd)
 		cmd->args.v = prepare_git_cmd(&nargv, sargv);
 	else if (cmd->use_shell)
 		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
+	trace_argv_printf(&cmd->args.v[1], "trace: prepare_cmd:");
 
 	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
 				  (char**) cmd->env.v,

Re: [PATCH v4 3/3] run-command: show prepared command

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	test_grep "^trace: prepare_cmd:.*" output
>> +'
>
> If you run
>
>     $ cd t && sh ./t0014-alias.sh -d && cat trash*.t0014-alias/output
>
> you'll see two "prepare_cmd" logged, because you added one to
> prepare_shell_cmd() and another to prepare_cmd().

To avoid similar issues, perhaps the test can try to see a bit more
into the output?  We already know that we cannot expect the output
to be identical everywhere, but we can do something along the lines
of the attached patch, or is the expected pattern below a bit too
specific?

 t/t0014-alias.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git c/t/t0014-alias.sh w/t/t0014-alias.sh
index b7affbe93a..a3c133be96 100755
--- c/t/t0014-alias.sh
+++ w/t/t0014-alias.sh
@@ -45,9 +45,17 @@ test_expect_success 'run-command formats empty args properly' '
 '
 
 test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
+	cat >expect <<-EOF &&
+	trace: exec: git-echo argument
+	trace: run_command: git-echo argument
+	trace: run_command: ${SQ}echo \$*${SQ} argument
+	trace: prepare_cmd: SHELL -c ${SQ}echo \$* "\$@"${SQ} ${SQ}echo \$*${SQ} argument
+	EOF
 	git config alias.echo "!echo \$*" &&
 	env GIT_TRACE=1 git echo argument 2>output &&
-	test_grep "^trace: prepare_cmd:.*" output
+	# redact platform differences
+	sed -e "s/^\(trace: prepare_cmd:\) .* -c /\1 SHELL -c /" output >actual &&
+	test_cmp expect actual
 '
 
 test_done

Re: [PATCH v3 3/3] run-command: show prepared command

[Ian Wienand]

On Fri, May 24, 2024 at 10:50:33AM -0700, Junio C Hamano wrote:
> > Should I just leave as is?
> 
> If I found as-is would be good enough, I wouldn't have been
> commenting on this.

I think this "for ... done" example from me is perhaps too specific
and weird, and not being helpful.  I'll trim all this out as I think
the point can be made clearer.

> Even in this third iteration, I still didn't see the added
> documentation talk about the principle behind the design, i.e. what
> you write after the "git your-alias" are appended to the command
> line to be used as additional arguments.

I'll send an update that makes it clearer in a separate point that for
simple arguments, the added "$@" just passes through arguments as
additional positional arguments.

-i

Re: [PATCH v4 3/3] run-command: show prepared command

[Ian Wienand]

On Fri, May 24, 2024 at 12:16:34PM -0700, Junio C Hamano wrote:
> Ian Wienand <iwienand@redhat.com> writes:
> 
> > +test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
> > +	git config alias.echo "!echo \$*" &&
> > +	env GIT_TRACE=1 git echo argument 2>output &&
> > +	test_grep "^trace: prepare_cmd:.*" output
> > +'
> 
> If you run
> 
>     $ cd t && sh ./t0014-alias.sh -d && cat trash*.t0014-alias/output
> 
> you'll see two "prepare_cmd" logged, because you added one to
> prepare_shell_cmd() and another to prepare_cmd().

Sorry that's a bit embarrasing that I missed that.  Reading this more
carefully, I agree that the points you've identified in start_command
are probably the best place.  I've updated to this.

-i

[PATCH v5 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v5 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..f32b86cde3 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,31 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* If the shell alias is the full path to a binary, it will be executed
+  directly with any arguments as positional arguments.
+* If the alias contains any white-space or reserved characters, it
+  will be considered an inline script and run as an argument to `sh
+  -c`.
+* When running as a script, if arguments are provided to the alias
+  call, Git makes them available to the process by appending "$@" to
+  the alias shell command.  This is not appended if arguments are not
+  provided.
+** For "simple" commands, such as calling a single binary
+  (e.g. `alias.myapp = !myapp --myflag1`) this will result in any
+  arguments becoming additional regular positional arguments to the
+  called binary, appended after any arguments specified in the aliased
+  command.
+** Care should be taken if your alias script has multiple commands
+   (e.g. in a pipeline), references argument variables, or is
+   otherwise not expecting the presence of the appended `"$@"`.  For
+   example: `alias.echo = "!echo $1"` when run as `git echo arg` will
+   actually execute `sh -c "echo $1 $@" "echo $1" "arg"` resulting in
+   output `arg arg`.  When writing such aliases, you should ensure
+   that the appended "$@" when arguments are present does not cause
+   syntax errors or unintended side-effects.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.cmd = "!c() {
+   cmd $1 | cmd $2 ; }; c" will allow you to work with separate
+   arguments.
+** Setting `GIT_TRACE=1` can help debug the command being run.
-- 
2.45.1

[PATCH v5 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in start_command so we can see the full
command invocation without having to resort to strace/code inspection.
For example:

 $ GIT_TRACE=1 git test foo
 git.c:755               trace: exec: git-test foo
 run-command.c:657       trace: run_command: git-test foo
 run-command.c:657       trace: run_command: 'echo $*' foo
 run-command.c:749       trace: start_command: /bin/sh -c 'echo $* "$@"' 'echo $*' foo

Prior changes have made the documentation around the internals of the
alias command execution clearer, but I have still found this detailed
view of the aliased command being run helpful for debugging purposes.

A test case is added.  This simply looks for the trace point output;
the details change depending on platform -- the important thing is
that we have a verbose log point, not so much the details of what
happens on each platform.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 run-command.c    | 3 +++
 t/t0014-alias.sh | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/run-command.c b/run-command.c
index 1b821042b4..9892c4421c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -746,6 +746,8 @@ int start_command(struct child_process *cmd)
 		goto end_of_spawn;
 	}
 
+	trace_argv_printf(&argv.v[1], "trace: start_command:");
+
 	if (pipe(notify_pipe))
 		notify_pipe[0] = notify_pipe[1] = -1;
 
@@ -913,6 +915,7 @@ int start_command(struct child_process *cmd)
 	else if (cmd->use_shell)
 		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
 
+	trace_argv_printf(&cmd->args.v[1], "trace: start_command:");
 	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
 				  (char**) cmd->env.v,
 				  cmd->dir, fhin, fhout, fherr);
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..db747950c7 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,10 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo argument 2>output &&
+	test_grep "^trace: start_command:.*" output
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v5 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> diff --git a/run-command.c b/run-command.c
> index 1b821042b4..9892c4421c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -746,6 +746,8 @@ int start_command(struct child_process *cmd)
>  		goto end_of_spawn;
>  	}
>  
> +	trace_argv_printf(&argv.v[1], "trace: start_command:");
> +
>  	if (pipe(notify_pipe))
>  		notify_pipe[0] = notify_pipe[1] = -1;

This side is OK ...

> @@ -913,6 +915,7 @@ int start_command(struct child_process *cmd)
>  	else if (cmd->use_shell)
>  		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
>  
> +	trace_argv_printf(&cmd->args.v[1], "trace: start_command:");
>  	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
>  				  (char**) cmd->env.v,
>  				  cmd->dir, fhin, fhout, fherr);

... but this side should pass "cmd->args.v" (i.e., the entire array,
without omitting the zeroth element) to be consistent with the other
side.  I made the same mistake in my "how about doing it this way"
draft, by the way.

It is because prepare_cmd() does this:

        static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
        {
                ...
                /*
                 * Add SHELL_PATH so in the event exec fails with ENOEXEC we can
                 * attempt to interpret the command with 'sh'.
                 */
                strvec_push(out, SHELL_PATH);

                if (cmd->git_cmd) {
                        prepare_git_cmd(out, cmd->args.v);
                } else if (cmd->use_shell) {
                        prepare_shell_cmd(out, cmd->args.v);
                ...

So, the other side (i.e. non Windows, that used the result from
prepare_cmd()) skips the argv.v[0] element (which is the SHELL_PATH
pushed by prepare_cmd()), but because a bare use of
prepare_shell_cmd() done on the Windows side does not have that
excess element at the beginning.

Re: [PATCH v5 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> +test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
> +	git config alias.echo "!echo \$*" &&
> +	env GIT_TRACE=1 git echo argument 2>output &&
> +	test_grep "^trace: start_command:.*" output
> +'

This will succeed even if you have two or more instances of this
log, which will not help you avoid the mistake we saw in an earlier
iteration.

How about making sure that the prepared command looks reasonable
enough and we have only one, by doing something like this?

test_expect_success 'tracing a shell alias with arg shows trace of prepared command' '
	cat >expect <<-EOF &&
	trace: prepare_cmd: SHELL -c ${SQ}echo \$* "\$@"${SQ} ${SQ}echo \$*${SQ} arg
	EOF
	git config alias.echo "!echo \$*" &&
	env GIT_TRACE=1 git echo arg 2>output &&
	# redact platform differences
	sed -n -e "s/^\(trace: prepare_cmd:\) .* -c /\1 SHELL -c /p" output >actual &&
	test_cmp expect actual
'

A sample run of CI job with the above change (and others I sent on
the list) can be seen at:

    https://github.com/git/git/actions/runs/9233329671

You can fetch 'seen' and can find commit 05ebf54569, which is the
tip of your topic including a few "fixup" commits I made on top.

  $ git log --decorate --oneline master..05ebf54569
  05ebf54569 (iw/trace-argv-on-alias) fixup! run-command: show prepared command
  11056975bb fixup! run-command: show prepared command
  770dda8f2b run-command: show prepared command
  242b9d4d63 Documentation: alias: add notes on shell expansion
  e4331ad0d4 Documentation: alias: rework notes into points

It hasn't finished running yet, but all the windows tests have
passed, so I'll happily go to bed now ;-)

Thanks.

[PATCH v6 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v6 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..f32b86cde3 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,31 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* If the shell alias is the full path to a binary, it will be executed
+  directly with any arguments as positional arguments.
+* If the alias contains any white-space or reserved characters, it
+  will be considered an inline script and run as an argument to `sh
+  -c`.
+* When running as a script, if arguments are provided to the alias
+  call, Git makes them available to the process by appending "$@" to
+  the alias shell command.  This is not appended if arguments are not
+  provided.
+** For "simple" commands, such as calling a single binary
+  (e.g. `alias.myapp = !myapp --myflag1`) this will result in any
+  arguments becoming additional regular positional arguments to the
+  called binary, appended after any arguments specified in the aliased
+  command.
+** Care should be taken if your alias script has multiple commands
+   (e.g. in a pipeline), references argument variables, or is
+   otherwise not expecting the presence of the appended `"$@"`.  For
+   example: `alias.echo = "!echo $1"` when run as `git echo arg` will
+   actually execute `sh -c "echo $1 $@" "echo $1" "arg"` resulting in
+   output `arg arg`.  When writing such aliases, you should ensure
+   that the appended "$@" when arguments are present does not cause
+   syntax errors or unintended side-effects.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.cmd = "!c() {
+   cmd $1 | cmd $2 ; }; c" will allow you to work with separate
+   arguments.
+** Setting `GIT_TRACE=1` can help debug the command being run.
-- 
2.45.1

[PATCH v6 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in start_command so we can see the full
command invocation without having to resort to strace/code inspection.
For example:

 $ GIT_TRACE=1 git test foo
 git.c:755               trace: exec: git-test foo
 run-command.c:657       trace: run_command: git-test foo
 run-command.c:657       trace: run_command: 'echo $*' foo
 run-command.c:749       trace: start_command: /bin/sh -c 'echo $* "$@"' 'echo $*' foo

Prior changes have made the documentation around the internals of the
alias command execution clearer, but I have still found this detailed
view of the aliased command being run helpful for debugging purposes.

A test case is added.  This simply looks for the trace point output;
the details change depending on platform -- the important thing is
that we have a verbose log point, not so much the details of what
happens on each platform.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 run-command.c    |  3 +++
 t/t0014-alias.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/run-command.c b/run-command.c
index 1b821042b4..31b20123d8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -746,6 +746,8 @@ int start_command(struct child_process *cmd)
 		goto end_of_spawn;
 	}
 
+	trace_argv_printf(&argv.v[1], "trace: start_command:");
+
 	if (pipe(notify_pipe))
 		notify_pipe[0] = notify_pipe[1] = -1;
 
@@ -913,6 +915,7 @@ int start_command(struct child_process *cmd)
 	else if (cmd->use_shell)
 		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
 
+	trace_argv_printf(cmd->args.v, "trace: start_command:");
 	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
 				  (char**) cmd->env.v,
 				  cmd->dir, fhin, fhout, fherr);
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..634f6d78ef 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,16 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
+	cat >expect <<-EOF &&
+	trace: start_command: SHELL -c ${SQ}echo \$* "\$@"${SQ} ${SQ}echo \$*${SQ} arg
+	EOF
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo arg 2>output &&
+	# redact platform differences
+	cat output &&
+	sed -n -e "s/^\(trace: start_command:\) .* -c /\1 SHELL -c /p" output >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v5 3/3] run-command: show prepared command

[Ian Wienand]

On Fri, May 24, 2024 at 11:06:14PM -0700, Junio C Hamano wrote:
> A sample run of CI job with the above change (and others I sent on
> the list) can be seen at:
> 
>     https://github.com/git/git/actions/runs/9233329671

Thanks for this; I had in my head that the pipeline wouldn't run in a
personal fork, but after I enabled it, it does ... I don't know who's
ultimately paying for it but I'll take it :)

 https://github.com/ianw/git/actions/runs/9238552513

is for the v6 version; I'll make sure to run it through that from now
on!

-i

Re: [PATCH v6 3/3] run-command: show prepared command

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> This adds a trace point in start_command so we can see the full
> command invocation without having to resort to strace/code inspection.
> For example:
>
>  $ GIT_TRACE=1 git test foo
>  git.c:755               trace: exec: git-test foo
>  run-command.c:657       trace: run_command: git-test foo
>  run-command.c:657       trace: run_command: 'echo $*' foo
>  run-command.c:749       trace: start_command: /bin/sh -c 'echo $* "$@"' 'echo $*' foo
>
> Prior changes have made the documentation around the internals of the
> alias command execution clearer, but I have still found this detailed
> view of the aliased command being run helpful for debugging purposes.
>
> A test case is added.  This simply looks for the trace point output;
> the details change depending on platform -- the important thing is
> that we have a verbose log point, not so much the details of what
> happens on each platform.
>
> Signed-off-by: Ian Wienand <iwienand@redhat.com>
> ---
>  run-command.c    |  3 +++
>  t/t0014-alias.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 1b821042b4..31b20123d8 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -746,6 +746,8 @@ int start_command(struct child_process *cmd)
>  		goto end_of_spawn;
>  	}
>  
> +	trace_argv_printf(&argv.v[1], "trace: start_command:");
> +
>  	if (pipe(notify_pipe))
>  		notify_pipe[0] = notify_pipe[1] = -1;
>  
> @@ -913,6 +915,7 @@ int start_command(struct child_process *cmd)
>  	else if (cmd->use_shell)
>  		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
>  
> +	trace_argv_printf(cmd->args.v, "trace: start_command:");
>  	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
>  				  (char**) cmd->env.v,
>  				  cmd->dir, fhin, fhout, fherr);
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 95568342be..634f6d78ef 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -44,4 +44,16 @@ test_expect_success 'run-command formats empty args properly' '
>      test_cmp expect actual
>  '
>  
> +test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
> +	cat >expect <<-EOF &&
> +	trace: start_command: SHELL -c ${SQ}echo \$* "\$@"${SQ} ${SQ}echo \$*${SQ} arg
> +	EOF
> +	git config alias.echo "!echo \$*" &&
> +	env GIT_TRACE=1 git echo arg 2>output &&
> +	# redact platform differences
> +	cat output &&

That "cat" is not about "redact"ing platform differences.  Leftover
debugging?  I'll remove it while queuing.

> +	sed -n -e "s/^\(trace: start_command:\) .* -c /\1 SHELL -c /p" output >actual &&
> +	test_cmp expect actual
> +'

Other than that, this step looks good to me.

Thanks.

Re: [PATCH v6 2/3] Documentation: alias: add notes on shell expansion

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> +* If the shell alias is the full path to a binary, it will be executed
> +  directly with any arguments as positional arguments.
> +* If the alias contains any white-space or reserved characters, it
> +  will be considered an inline script and run as an argument to `sh
> +  -c`.
> +* When running as a script, if arguments are provided to the alias
> +  call, Git makes them available to the process by appending "$@" to
> +  the alias shell command.  This is not appended if arguments are not
> +  provided.

These are not technically wrong per-se.

> +** For "simple" commands, such as calling a single binary
> +  (e.g. `alias.myapp = !myapp --myflag1`) this will result in any
> +  arguments becoming additional regular positional arguments to the
> +  called binary, appended after any arguments specified in the aliased
> +  command.

But the single-command script still receives the arguments in
argv[], so what the alias command has to do is the same.  The
earlier ones that are "not technically wrong" are merely
implemenation detail.

In a single command case, e.g., "[alias] single = !one", you may
write

    #!/bin/sh
    echo "$1" | grep "$2"

in 'one' script, and "git single 1 2" will be turned into

    start_command: one 1 2

i.e. one receives two arguments in argv[].  It is an implementation
detail that we can bypass "sh" or "-c" or "$@"

If you write exactly the same thing like

    $ git -c 'alias=single=!one ' single 1 2

you'll instead see

    start_command: /bin/sh -c 'one' "$@" "one " 1 2

because the trailing SP in the alias disables the optimization to
bypass a more generic 'sh -c ... "$@"' construction.  What gets run
is an equivalent of the reader saying

    $ /bin/sh -c 'one "$@"' "one " 1 2

bypassing git from the command line.

What the script (one) has to write does not change at all either
case.

As I keep saying over multiple iterations, the above three bullet
points stress too much on the minute implementation detail while
failing to tell readers that the end-user alias receives the rest of
the command line as arguments.

> +** Care should be taken if your alias script has multiple commands
> +   (e.g. in a pipeline), references argument variables, or is

"argument variables" -> "arguments".

> +   otherwise not expecting the presence of the appended `"$@"`.  

"otherwise not expecting" is SIMPLY BUGGY but the readers may not
understand it unless you tell them that the arguments are fed to
their aliased command by appending them.

When you look at the implementation detail of "sh -c '... $@' -
$ARGS" as something to fight against, readers would lose sight to
see the crux of the problem they are trying to solve.  I think it is
a wrong way to frame the issue.  The problem readers are solving when
coming up with their own alias is this:

    How would one write a single-liner that can take arguments
    appended at the end?

I think giving that to the readers upfront, i.e. "when you write an
alias, you are forming a single-liner that takes arguments appended
at the end", would go a long way without having them lose sight in
the implementation details of "sometimes args directly come in
argv[], sometimes your alias is wrapped in "sh -c" and "$@" is used.
They do the same thing to feed the arguments to your script.

Going back that 'one' example, if 'echo "$1" | grep "$2"' was what
you wanted to run,  how would you write a single-liner that does

    echo "$1" | grep "$2"

and can take its arguments at the end?  You do *not* want to see
your invocation of the alias

    $ git single 1 2

turn into 

    $ echo "$1" | grep "$2" 1 2

of course, and remember, "$@" is merely an implementation detail
that the end-users do not need to see.

Of course the simplest one-liner, if you had the "one" script
already stored in the file, is to say

    $ one                                  1 2

i.e. "[alias] single = !one".  But calling that a "single-liner" is
cheating.

You can do one of two easy things.

    $ sh -c 'echo "$1" | grep "$2"' -      1 2
    $ e(){ echo "$1" | grep "$2"; };e      1 2

The earlier string (before "1 2" is appended) of either of these
gives you "a single-liner that takes arguments at the end" that does
the "echo the first one, pipe it to grep that looks for the second
one", which you would make the body of the alias.  If the reader
understands the earlier example that stores it in a file, the former
is more mechanical and straight-forward rewrite of it.  The latter
may be a bit more convoluted, but says the same thing in the same
number of letters.

HTH.

Re: [PATCH v6 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

On Sun, May 26, 2024 at 04:26:51PM -0700, Junio C Hamano wrote:
> As I keep saying over multiple iterations, the above three bullet
> points stress too much on the minute implementation detail while
> failing to tell readers that the end-user alias receives the rest of
> the command line as arguments.

OK, I can agree that perhaps I've been a bit to fixated on the
addition of "$@" and the mechanics of this.  I will propose again with
this trimmed.  What I didn't have to help me at the time was the full
command in GIT_TRACE, which I think is probably a more appropriate way
to communicate these details of what's actually hitting the exec
calls.

> Of course the simplest one-liner, if you had the "one" script
> already stored in the file, is to say

So the reason I fell into this, and I wonder how much this plays out
for others too, is that shipping these workflow bits as stand-alone
scripts would mean no !shell tricks required, it's all very logical
and I would never have looked at any of this :) However, when all you
have is a hammer ... since a git config .inc file was needed for other
things, it has been overloaded into essentially being mini
package-manger that avoids having to install additional dependencies;
one-liner shell script at a time :)

> You can do one of two easy things.
> 
>     $ sh -c 'echo "$1" | grep "$2"' -      1 2

Ok, I think "sh -c" in ! aliases is a bit confusing, personally.  You
end up two shells nested deep, and you really have to explain what's
going on with $0; it's very easy to miss.

>     $ e(){ echo "$1" | grep "$2"; };e      1 2

This method, which is used elsewhere in the docs as well, I think
makes the most sense.  So I've left that in as the example.

-i

[PATCH v7 1/3] Documentation: alias: rework notes into points

[Ian Wienand]

There are a number of caveats when using aliases.  Rather than
stuffing them all together in a paragraph, let's separate them out
into individual points to make it clearer what's going on.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 01df96fab3..40851ef429 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -21,8 +21,9 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
 `git new` is equivalent to running the shell command
-`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
-executed from the top-level directory of a repository, which may
-not necessarily be the current directory.
-`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
-from the original current directory. See linkgit:git-rev-parse[1].
+`gitk --all --not ORIG_HEAD`.  Note:
++
+* Shell commands will be executed from the top-level directory of a
+  repository, which may not necessarily be the current directory.
+* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
+  from the original current directory. See linkgit:git-rev-parse[1].
-- 
2.45.1

[PATCH v7 2/3] Documentation: alias: add notes on shell expansion

[Ian Wienand]

When writing inline shell for shell-expansion aliases (i.e. prefixed
with "!"), there are some caveats around argument parsing to be aware
of.  This series of notes attempts to explain what is happening more
clearly.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 Documentation/config/alias.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
index 40851ef429..75f9f5e26f 100644
--- a/Documentation/config/alias.txt
+++ b/Documentation/config/alias.txt
@@ -27,3 +27,17 @@ it will be treated as a shell command.  For example, defining
   repository, which may not necessarily be the current directory.
 * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
   from the original current directory. See linkgit:git-rev-parse[1].
+* Shell command aliases always receive any extra arguments provided to
+  the Git command-line as positional arguments.
+** Care should be taken if your shell alias is a "one-liner" script
+   with multiple commands (e.g. in a pipeline), references multiple
+   arguments, or is otherwise not able to handle positional arguments
+   appended via `"$@"`.  For example: `alias.cmd = "!echo $1 | grep
+   $2"` called as `git cmd 1 2` will be executed as 'echo $1 | grep $2
+   1 2', which is not what you want.
+** A convenient way to deal with this is to write your script
+   operations in an inline function that is then called with any
+   arguments from the command-line.  For example `alias.cmd = "!c() {
+   echo $1 | grep $2 ; }; c" will correctly execute the prior example.
+** Setting `GIT_TRACE=1` can help you debug the command being run for
+   your alias.
-- 
2.45.1

[PATCH v7 3/3] run-command: show prepared command

[Ian Wienand]

This adds a trace point in start_command so we can see the full
command invocation without having to resort to strace/code inspection.
For example:

 $ GIT_TRACE=1 git test foo
 git.c:755               trace: exec: git-test foo
 run-command.c:657       trace: run_command: git-test foo
 run-command.c:657       trace: run_command: 'echo $*' foo
 run-command.c:749       trace: start_command: /bin/sh -c 'echo $* "$@"' 'echo $*' foo

Prior changes have made the documentation around the internals of the
alias command execution clearer, but I have still found this detailed
view of the aliased command being run helpful for debugging purposes.

A test case is added to ensure the full command output is present in
the execution flow.

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 run-command.c    |  3 +++
 t/t0014-alias.sh | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/run-command.c b/run-command.c
index 1b821042b4..31b20123d8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -746,6 +746,8 @@ int start_command(struct child_process *cmd)
 		goto end_of_spawn;
 	}
 
+	trace_argv_printf(&argv.v[1], "trace: start_command:");
+
 	if (pipe(notify_pipe))
 		notify_pipe[0] = notify_pipe[1] = -1;
 
@@ -913,6 +915,7 @@ int start_command(struct child_process *cmd)
 	else if (cmd->use_shell)
 		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
 
+	trace_argv_printf(cmd->args.v, "trace: start_command:");
 	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
 				  (char**) cmd->env.v,
 				  cmd->dir, fhin, fhout, fherr);
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 95568342be..854d59ec58 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -44,4 +44,15 @@ test_expect_success 'run-command formats empty args properly' '
     test_cmp expect actual
 '
 
+test_expect_success 'tracing a shell alias with arguments shows trace of prepared command' '
+	cat >expect <<-EOF &&
+	trace: start_command: SHELL -c ${SQ}echo \$* "\$@"${SQ} ${SQ}echo \$*${SQ} arg
+	EOF
+	git config alias.echo "!echo \$*" &&
+	env GIT_TRACE=1 git echo arg 2>output &&
+	# redact platform differences
+	sed -n -e "s/^\(trace: start_command:\) .* -c /\1 SHELL -c /p" output >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.45.1

Re: [PATCH v7 2/3] Documentation: alias: add notes on shell expansion

[Junio C Hamano]

Ian Wienand <iwienand@redhat.com> writes:

> When writing inline shell for shell-expansion aliases (i.e. prefixed
> with "!"), there are some caveats around argument parsing to be aware
> of.  This series of notes attempts to explain what is happening more
> clearly.
>
> Signed-off-by: Ian Wienand <iwienand@redhat.com>
> ---
>  Documentation/config/alias.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

This one has become much shorter but very much to the point.  

Let's further do "appended via `"$@"`" -> "added at the end".  The
example given in that bullet point would be easier to understand
that way.

Will queue.

> diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
> index 40851ef429..75f9f5e26f 100644
> --- a/Documentation/config/alias.txt
> +++ b/Documentation/config/alias.txt
> @@ -27,3 +27,17 @@ it will be treated as a shell command.  For example, defining
>    repository, which may not necessarily be the current directory.
>  * `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
>    from the original current directory. See linkgit:git-rev-parse[1].
> +* Shell command aliases always receive any extra arguments provided to
> +  the Git command-line as positional arguments.
> +** Care should be taken if your shell alias is a "one-liner" script
> +   with multiple commands (e.g. in a pipeline), references multiple
> +   arguments, or is otherwise not able to handle positional arguments
> +   appended via `"$@"`.  For example: `alias.cmd = "!echo $1 | grep
> +   $2"` called as `git cmd 1 2` will be executed as 'echo $1 | grep $2
> +   1 2', which is not what you want.
> +** A convenient way to deal with this is to write your script
> +   operations in an inline function that is then called with any
> +   arguments from the command-line.  For example `alias.cmd = "!c() {
> +   echo $1 | grep $2 ; }; c" will correctly execute the prior example.
> +** Setting `GIT_TRACE=1` can help you debug the command being run for
> +   your alias.