Re: [PATCH v7 0/4] worktree: Support `--orphan` when creating new worktrees
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 9, 2023, 12:26 UTC
- Message-ID
- <230109.86r0w328nu.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20230107045757.30037-1-jacobabel@nullpo.dev>
On Sat, Jan 07 2023, Jacob Abel wrote:
Show 28 quoted lines
> While working with the worktree based git workflow, I realised that setting > up a new git repository required switching between the traditional and > worktree based workflows. Searching online I found a SO answer [1] which > seemed to support this and which indicated that adding support for this should > not be technically difficult. > > This patchset has four parts: > * adding `-B` to the usage docs (noticed during dev and it seemed too small > to justify a separate submission) > * adding a helper fn to simplify testing for mutual exclusion of options > in `t/t2400-worktree-add.sh` > * adding orphan branch functionality (as is present in `git-switch`) > to `git-worktree-add` > * adding an advise for using --orphan when `git worktree add` fails due to > a bad ref. > > Changes from v6: > * Removed helper save_param_arr() introduced in v6 from t2400 (2/4) [2]. > * Reverted changes introduced in v6 to test_wt_add_excl() from t2400 (2/4) [3]. > * Changed test_wt_add_excl() to use `local opts="$*"` (2/4) [3]. > * Added check to test_wt_add_excl() to better validate test results (2/4). > * Re-add &&-chains to test_wt_add_excl() (2/4) [4]. > * Reverted changes introduced in v6 to test_wt_add_empty_repo_orphan_hint() > from t2400 (4/4) [3]. > * Changed test_wt_add_empty_repo_orphan_hint() to use `local opts="$*"` (4/4) [3]. > * Added check to test_wt_add_empty_repo_orphan_hint() to better validate test > results (4/4). > * Re-add &&-chains to test_wt_add_empty_repo_orphan_hint() (2/4) [4].
This round looks good to me, except for a small tiny (and new) portability issue that needs fixing:
Show 32 quoted lines
> Range-diff against v6:
> 1: a9ef3eca7b = 1: a9ef3eca7b worktree add: include -B in usage docs
> 2: c03c112f79 ! 2: d124cc481c worktree add: refactor opt exclusion tests
> @@ t/t2400-worktree-add.sh: test_expect_success '"add" no auto-vivify with --detach
> -test_expect_success '"add" -b/-B mutually exclusive' '
> - test_must_fail git worktree add -b poodle -B poodle bamboo main
> -'
> -+# Saves parameter sequence/array as a string so they can be safely stored in a
> -+# variable and restored with `eval "set -- $arr"`. Sourced from
> -+# https://stackoverflow.com/a/27503158/15064705
> -+save_param_arr () {
> -+ local i
> -+ for i;
> -+ do
> -+ # For each argument:
> -+ # 1. Append "\n" after each entry
> -+ # 2. Convert "'" into "'\''"
> -+ # 3. Prepend "'" before each entry
> -+ # 4. Append " \" after each entry
> -+ printf "%s\\n" "$i" | sed "
> -+ s/'/'\\\\''/g
> -+ 1s/^/'/
> -+ \$s/\$/' \\\\/
> -+ "
> -+ done
> -+ echo " "
> -+}
> -
> +-
> -test_expect_success '"add" -b/--detach mutually exclusive' '
> - test_must_fail git worktree add -b poodle --detach bamboo main
> -'Good to get rid of this.
Show 12 quoted lines
> +# Helper function to test mutually exclusive options.
> ++#
> ++# Note: Quoted arguments containing spaces are not supported.
> +test_wt_add_excl () {
> -+ local arr=$(save_param_arr "$@")
> -+ test_expect_success "'worktree add' with $* has mutually exclusive options" '
> -+ eval "set -- $arr" &&
> -+ test_must_fail git worktree add "$@"
> ++ local opts="$*" &&
> ++ test_expect_success "'worktree add' with '$opts' has mutually exclusive options" '
> ++ test_must_fail git worktree add $opts 2>actual &&
> ++ grep -P "fatal:( options)? .* cannot be used together" actualThis is the new unportable code, the "-P" option is specific to GNU grep, and here you're relying on the "?" syntax along with other ERE-like syntax.
It looks like the minimal fix here is to just change -P to -E, which we can use, you're not using any PCRE-syntax, but just syntax that's part of ERE.