Re: [PATCH 2/5] t/lib-commit-graph.sh: avoid directory change in `graph_git_behavior()`

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jul 21, 2023 at 02:01:58PM -0400, Eric Sunshine wrote:
> > diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh
> > @@ -20,12 +20,14 @@ graph_git_behavior() {
> >         test_expect_success "check normal git operations: $MSG" '
> > -               cd "$TRASH_DIRECTORY/$DIR" &&
> > -               graph_git_two_modes "log --oneline $BRANCH" &&
> > -               graph_git_two_modes "log --topo-order $BRANCH" &&
> > -               graph_git_two_modes "log --graph $COMPARE..$BRANCH" &&
> > -               graph_git_two_modes "branch -vv" &&
> > -               graph_git_two_modes "merge-base -a $BRANCH $COMPARE"
> > +               (
> > +                       cd "$TRASH_DIRECTORY" &&
> > +                       graph_git_two_modes "${DIR:+-C $DIR} log --oneline $BRANCH" &&
> > +                       graph_git_two_modes "${DIR:+-C $DIR} log --topo-order $BRANCH" &&
> > +                       graph_git_two_modes "${DIR:+-C $DIR} log --graph $COMPARE..$BRANCH" &&
> > +                       graph_git_two_modes "${DIR:+-C $DIR} branch -vv" &&
> > +                       graph_git_two_modes "${DIR:+-C $DIR} merge-base -a $BRANCH $COMPARE"
> > +               )
> >         '
> >  }
>
> As mentioned in my review of patch [1/5], for safety, you'd probably
> want to quote the expansion of DIR in case it ever contains whitespace
> (or other weird characters). The obvious POSIX-correct way to do this
> would be:
>
>     graph_git_two_modes "${DIR:+-C \"$DIR\"} log ..." &&
>
> Unfortunately, however, some older broken shells incorrectly expand
> this to a single argument ("-C <dir>") rather than the expected two
> arguments (-C and "<dir>")[1,2,3,4]. The workaround is unsightly but
> doable:
>
>     graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} log ..." &&

Hmm. I get what you're saying, but I think in this case we're OK, since
this all goes to `graph_git_two_modes`, whose implementation looks like:

    graph_git_two_modes() {
        git -c core.commitGraph=true $1 >output
    }

So I think we really do want everything smashed together into a single
argument.

(Just to sure, here's the diff of what I applied on top of [2/5] before
replying to your message):

--- 8< ---
diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh
index c93969ae74..d60e64cb0b 100755
--- a/t/lib-commit-graph.sh
+++ b/t/lib-commit-graph.sh
@@ -22,11 +22,11 @@ graph_git_behavior() {
 	test_expect_success "check normal git operations: $MSG" '
 		(
 			cd "$TRASH_DIRECTORY" &&
-			graph_git_two_modes "${DIR:+-C $DIR} log --oneline $BRANCH" &&
-			graph_git_two_modes "${DIR:+-C $DIR} log --topo-order $BRANCH" &&
-			graph_git_two_modes "${DIR:+-C $DIR} log --graph $COMPARE..$BRANCH" &&
-			graph_git_two_modes "${DIR:+-C $DIR} branch -vv" &&
-			graph_git_two_modes "${DIR:+-C $DIR} merge-base -a $BRANCH $COMPARE"
+			graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} log --oneline $BRANCH" &&
+			graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} log --topo-order $BRANCH" &&
+			graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} log --graph $COMPARE..$BRANCH" &&
+			graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} branch -vv" &&
+			graph_git_two_modes "${DIR:+-C} ${DIR:+\"$DIR\"} merge-base -a $BRANCH $COMPARE"
 		)
 	'
 }
--- >8 ---

> [1]: https://lore.kernel.org/git/20160517215214.GA16905@sigill.intra.peff.net/
> [2]: https://lore.kernel.org/git/e3bfc53363b14826d828e1adffbbeea@74d39fa044aa309eaea14b9f57fe79c/
> [3]: https://lore.kernel.org/git/20160518010609.Horde.sM8QUFek6WMAAwho56DDob8@webmail.informatik.kit.edu/
> [4]: https://lore.kernel.org/git/1240044459-57227-1-git-send-email-ben@ben.com/

Thanks,
Taylor