[18/30] subtree: use $* instead of $@ as appropriate

Commit Message

Luke Shumaker April 23, 2021, 7:42 p.m. UTC

From: Luke Shumaker <[e-mail]>

$* is for when you want to smash things together, whitespace-separated;
$@ is for when you want them to be separate strings.  There are a couple
of places in subtree that erroneously use $@ when smashing args together
in to an error message.

Signed-off-by: Luke Shumaker <[e-mail]>
---
 contrib/subtree/git-subtree.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eric Sunshine April 23, 2021, 8:40 p.m. UTC | #1

On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <[e-mail]> wrote:
> $* is for when you want to smash things together, whitespace-separated;
> $@ is for when you want them to be separate strings.  There are a couple
> of places in subtree that erroneously use $@ when smashing args together
> in to an error message.

Can we be explicit and say "$@" in the commit message rather than bare
$@ since the unquoted form is not magical and acts exactly like $*.

Also: s/in to/into/

Nit: I have some trouble following what the commit message is actually
trying to say with "smash things" and "separate strings". It might be
simpler to say merely that use of "$@" in these particular instances
is overkill and possibly misleading to readers not familiar with the
finer details of $* vs. "$@".

The patch itself makes sense.

Luke Shumaker April 23, 2021, 11:50 p.m. UTC | #2

On Fri, 23 Apr 2021 14:40:31 -0600,
Eric Sunshine wrote:
> 
> On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <[e-mail]> wrote:
> > $* is for when you want to smash things together, whitespace-separated;
> > $@ is for when you want them to be separate strings.  There are a couple
> > of places in subtree that erroneously use $@ when smashing args together
> > in to an error message.
> 
> Can we be explicit and say "$@" in the commit message rather than bare
> $@ since the unquoted form is not magical and acts exactly like $*.
> 
> Also: s/in to/into/
> 
> Nit: I have some trouble following what the commit message is actually
> trying to say with "smash things" and "separate strings". It might be
> simpler to say merely that use of "$@" in these particular instances
> is overkill and possibly misleading to readers not familiar with the
> finer details of $* vs. "$@".
> 
> The patch itself makes sense.

How's this:

---
subtree: use "$*" instead of "$@" as appropriate

"$*" is for when you want to concatenate the args together,
whitespace-separated; and "$@" is for when you want them to be separate
strings.

There are several places in subtree that erroneously use $@ when
concatenating args together into an error message.

For instance, if the args are argv[1]="dead" and argv[2]="beef", then
the line

    die "You must provide exactly one revision.  Got: '$@'"

surely intends to call 'die' with the argument

    argv[1]="You must provide exactly one revision.  Got: 'dead beef'"

however, because the line used $@ instead of $*, it will actually call
'die' with the arguments

    argv[1]="You must provide exactly one revision.  Got: 'dead"
    argv[2]="beef'"

This isn't a big deal, because 'die' concatenates its arguments together
anyway (using "$*").  But that doesn't change the fact that it was a
mistake to use $@ instead of $*, even though in the end $@ still ended
up doing the right thing.
---

Eric Sunshine April 24, 2021, 5:18 a.m. UTC | #3

On Fri, Apr 23, 2021 at 7:50 PM Luke Shumaker <[e-mail]> wrote:
> On Fri, 23 Apr 2021 14:40:31 -0600, Eric Sunshine wrote:
> > Nit: I have some trouble following what the commit message is actually
> > trying to say with "smash things" and "separate strings". It might be
> > simpler to say merely that use of "$@" in these particular instances
> > is overkill and possibly misleading to readers not familiar with the
> > finer details of $* vs. "$@".

Oof, I somehow misread the code this patch is touching and ended up
confusing myself, and the confusion bled into my review comments.
Sorry.

> How's this:
> ---
> subtree: use "$*" instead of "$@" as appropriate
>
> "$*" is for when you want to concatenate the args together,
> whitespace-separated; and "$@" is for when you want them to be separate
> strings.
>
> There are several places in subtree that erroneously use $@ when
> concatenating args together into an error message.
>
> For instance, if the args are argv[1]="dead" and argv[2]="beef", then
> the line
>
>     die "You must provide exactly one revision.  Got: '$@'"
>
> surely intends to call 'die' with the argument
>
>     argv[1]="You must provide exactly one revision.  Got: 'dead beef'"
>
> however, because the line used $@ instead of $*, it will actually call
> 'die' with the arguments
>
>     argv[1]="You must provide exactly one revision.  Got: 'dead"
>     argv[2]="beef'"
>
> This isn't a big deal, because 'die' concatenates its arguments together
> anyway (using "$*").  But that doesn't change the fact that it was a
> mistake to use $@ instead of $*, even though in the end $@ still ended
> up doing the right thing.

This explanation spells out the problem nicely.

Patch

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index d7de4b0653..3105eb8033 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -58,14 +58,14 @@  progress () {
 assert () {
 	if ! "$@"
 	then
-		die "assertion failed: " "$@"
+		die "assertion failed: $*"
 	fi
 }
 
 ensure_single_rev () {
 	if test $# -ne 1
 	then
-		die "You must provide exactly one revision.  Got: '$@'"
+		die "You must provide exactly one revision.  Got: '$*'"
 	fi
 }
 
@@ -690,7 +690,7 @@  cmd_add () {
 
 		cmd_add_repository "$@"
 	else
-		say >&2 "error: parameters were '$@'"
+		say >&2 "error: parameters were '$*'"
 		die "Provide either a commit or a repository and commit."
 	fi
 }