Re: [PATCH 2/5] t/lib-commit-graph.sh: avoid directory change in `graph_git_behavior()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 21, 2023, 18:39 UTC
- Message-ID
- <ZLrQyZAoRIaga1mT@nand.local>
- In-Reply-To
- <CAPig+cTse-bgpApQR3jVGir0Tur22NEgAoTKpmLek+pTRovxwg@mail.gmail.com>
On Fri, Jul 21, 2023 at 02:01:58PM -0400, Eric Sunshine wrote:
Show 33 quoted lines
> > 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