From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 21 Jul 2023 18:01:58 GMT
Subject: Re: [PATCH 2/5] t/lib-commit-graph.sh: avoid directory change in `graph_git_behavior()`
Message-ID: <CAPig+cTse-bgpApQR3jVGir0Tur22NEgAoTKpmLek+pTRovxwg@mail.gmail.com>
In-Reply-To: <715a160903be6dc8873a7215c55f90894e62f589.1689960606.git.me@ttaylorr.com>

On Fri, Jul 21, 2023 at 1:32 PM Taylor Blau <me@ttaylorr.com> wrote:
> The `graph_git_behavior()` helper asserts that a number of common Git
> operations (such as `git log --oneline`, `git log --topo-order`, etc.)
> produce identical output regardless of whether or not a commit-graph is
> in use.
>
> This helper takes as its second argument the location (relative to the
> `$TRASH_DIRECTORY`) of the Git repostiory under test. In order to run
> each of its commands within that repository, it first changes into that
> directory, without the use of a sub-shell.
>
> This pollutes future tests which expect to be run in the top-level
> `$TRASH_DIRECTORY` as usual. We could wrap `graph_git_behavior()` in a
> sub-shell, like:
>
>     graph_git_behavior() {
>       # ...
>       (
>         cd "$TRASH_DIRECTORY/$DIR" &&
>         graph_git_two_modesl
>       )
>     }
>
> , but since we're invoking git directly, we can pass along a "-C $DIR"
> when "$DIR" is non-empty.
>
> Note, however, that until the remaining callers are cleaned up to avoid
> changing working directories outside of a sub-shell, that we need to
> ensure that we are operating in the top-level $TRASH_DIRECTORY. The
> inner-subshell will go away in a future commit once it is no longer
> necessary.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> 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 ..." &&

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