| Message ID | 20221212014003.20290-4-jacobabel@nullpo.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | worktree: Support `--orphan` when creating new worktrees | expand |
On Mon, Dec 12 2022, Jacob Abel wrote: > int git_default_advice_config(const char *var, const char *value); > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 51b247b2a7..ea306e18de 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -744,6 +744,14 @@ static int add(int ac, const char **av, const char *prefix) > * If `branch` does not reference a valid commit, a new > * worktree (and/or branch) cannot be created based off of it. > */ > + advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, > + "If you meant to create a worktree containing a new orphan branch\n" > + "(branch with no commits) for this repository, e.g. '%s',\n" I think this '%s' is just confusing, how is repeating the name of the branch at the user (which we're about to mention in the example command) helpful? Shouldn't we just omit it, or reword this to e.g.: If you'd like the '%s' branch to be a worktree containing a new.... > + "you can do so using the --orphan option:\n" > + "\n" > + " git worktree add --orphan %s %s\n" > + "\n", Missing the usual: "Turn this message off by running\n" "\"git config advice.worktreeAddOrphan false\"" blurb. Also, should we really add twe newlines at the end? I see some other API users that don't add a \n at all. > +# Helper function to test hints for using --orphan in an empty repo. FWIW I think we can do without the comment... > +test_wt_add_empty_repo_orphan_hint() { > + local context="$1" && > + local opts="${@:2}" && This appears to be some shell pseudo-syntax, and my shell hard-errors on this. But as we don't "shift" after the "$1" I don't see how what you *probably* meant could work, i.e. we always have arguments, so surely we'd always use "$@"? > + test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" ' > + test_when_finished "rm -rf empty_repo" && > + GIT_DIR="empty_repo" git init --bare && > + test_must_fail git -C empty_repo worktree add $opts 2> actual && > + grep "hint: If you meant to create a worktree containing a new orphan branch" actual > + ' > +} > + > +test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/ > +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/ > +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/ You're just testing how these options interact, so let's have the "foobar" part provided by the test function itself.
On 22/12/12 09:35AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 12 2022, Jacob Abel wrote: > > > int git_default_advice_config(const char *var, const char *value); > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 51b247b2a7..ea306e18de 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -744,6 +744,14 @@ static int add(int ac, const char **av, const char *prefix) > > * If `branch` does not reference a valid commit, a new > > * worktree (and/or branch) cannot be created based off of it. > > */ > > + advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, > > + "If you meant to create a worktree containing a new orphan branch\n" > > + "(branch with no commits) for this repository, e.g. '%s',\n" > > I think this '%s' is just confusing, how is repeating the name of the > branch at the user (which we're about to mention in the example command) > helpful? > > Shouldn't we just omit it, or reword this to e.g.: > > If you'd like the '%s' branch to be a worktree containing a > new.... > > > > + "you can do so using the --orphan option:\n" > > + "\n" > > + " git worktree add --orphan %s %s\n" > > + "\n", Done. > > Missing the usual: > > "Turn this message off by running\n" > "\"git config advice.worktreeAddOrphan false\"" > > blurb. That blurb is added at runtime by `advise_if_enabled()`. I originally added it but it was giving me the line twice so I took it out. > > Also, should we really add twe newlines at the end? I see some other API > users that don't add a \n at all. Removed. > > > +# Helper function to test hints for using --orphan in an empty repo. > > FWIW I think we can do without the comment... Removed. > > > +test_wt_add_empty_repo_orphan_hint() { > > + local context="$1" && > > + local opts="${@:2}" && > > This appears to be some shell pseudo-syntax, and my shell hard-errors on > this. > > But as we don't "shift" after the "$1" I don't see how what you > *probably* meant could work, i.e. we always have arguments, so surely > we'd always use "$@"? Ah. The "${@:2}" is a bashism I think. I got it from [1] to try and grab all the remaining arguments. Changed to just shifting. The &&ing together was a mistake on my part (not paying attention and mimicking the &&ing present in the test cases). I've removed that. > > > > + test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" ' > > + test_when_finished "rm -rf empty_repo" && > > + GIT_DIR="empty_repo" git init --bare && > > + test_must_fail git -C empty_repo worktree add $opts 2> actual && > > + grep "hint: If you meant to create a worktree containing a new orphan branch" actual > > + ' > > +} > > + > > +test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/ > > +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/ > > +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/ > > You're just testing how these options interact, so let's have the > "foobar" part provided by the test function itself. Done. The following are what I have made for this set of changes (against v4). diff --git a/builtin/worktree.c b/builtin/worktree.c index 8bb1453e0f..38f7a27927 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -732,12 +732,11 @@ static int add(int ac, const char **av, const char *prefix) } else if (!lookup_commit_reference_by_name(branch)) { advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, "If you meant to create a worktree containing a new orphan branch\n" - "(branch with no commits) for this repository, e.g. '%s',\n" - "you can do so using the --orphan option:\n" + "(branch with no commits) for this repository, you can do so\n" + "using the --orphan option:\n" "\n" - " git worktree add --orphan %s %s\n" - "\n", - new_branch, new_branch, path); + " git worktree add --orphan %s %s\n", + new_branch, path); die(_("invalid reference: %s"), branch); } else if (new_branch) { struct child_process cp = CHILD_PROCESS_INIT; diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 0970989ee5..05539aa03c 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -375,21 +375,21 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' ' test_cmp expect .git/worktrees/orphan-with-lock-reason/locked ' -# Helper function to test hints for using --orphan in an empty repo. test_wt_add_empty_repo_orphan_hint() { - local context="$1" && - local opts="${@:2}" && + local context="$1" + shift + local opts="$@" test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" ' test_when_finished "rm -rf empty_repo" && GIT_DIR="empty_repo" git init --bare && - test_must_fail git -C empty_repo worktree add $opts 2> actual && + test_must_fail git -C empty_repo worktree add $opts foobar/ 2> actual && grep "hint: If you meant to create a worktree containing a new orphan branch" actual ' } -test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/ -test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/ -test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/ +test_wt_add_empty_repo_orphan_hint 'DWIM' +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch test_expect_success 'local clone from linked checkout' ' git clone --local here here-clone && Also, Is there an easier way to debug the `test_expect_success` statements? I tried enabling tracing mode but it doesn't seem to trace into those statements. 1. https://stackoverflow.com/a/9057392/15064705
On Mon, Dec 12 2022, Jacob Abel wrote: > On 22/12/12 09:35AM, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Dec 12 2022, Jacob Abel wrote: >> >> > int git_default_advice_config(const char *var, const char *value); >> > diff --git a/builtin/worktree.c b/builtin/worktree.c >> > index 51b247b2a7..ea306e18de 100644 >> > --- a/builtin/worktree.c >> > +++ b/builtin/worktree.c >> > @@ -744,6 +744,14 @@ static int add(int ac, const char **av, const char *prefix) >> > * If `branch` does not reference a valid commit, a new >> > * worktree (and/or branch) cannot be created based off of it. >> > */ >> > + advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, >> > + "If you meant to create a worktree containing a new orphan branch\n" >> > + "(branch with no commits) for this repository, e.g. '%s',\n" >> >> I think this '%s' is just confusing, how is repeating the name of the >> branch at the user (which we're about to mention in the example command) >> helpful? >> >> Shouldn't we just omit it, or reword this to e.g.: >> >> If you'd like the '%s' branch to be a worktree containing a >> new.... >> >> >> > + "you can do so using the --orphan option:\n" >> > + "\n" >> > + " git worktree add --orphan %s %s\n" >> > + "\n", > > Done. > >> >> Missing the usual: >> >> "Turn this message off by running\n" >> "\"git config advice.worktreeAddOrphan false\"" >> >> blurb. > > That blurb is added at runtime by `advise_if_enabled()`. I originally added it > but it was giving me the line twice so I took it out. Ah, sorry. I just forgot it did that. Looks good then! > The following are what I have made for this set of changes (against v4). > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 8bb1453e0f..38f7a27927 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -732,12 +732,11 @@ static int add(int ac, const char **av, const char *prefix) > } else if (!lookup_commit_reference_by_name(branch)) { > advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, > "If you meant to create a worktree containing a new orphan branch\n" > - "(branch with no commits) for this repository, e.g. '%s',\n" > - "you can do so using the --orphan option:\n" > + "(branch with no commits) for this repository, you can do so\n" > + "using the --orphan option:\n" > "\n" > - " git worktree add --orphan %s %s\n" > - "\n", > - new_branch, new_branch, path); > + " git worktree add --orphan %s %s\n", > + new_branch, path); Looks good, we'd usually put the "new_branch, path" on the previous line (to the extent that it fits within 79 chars. Also: This string should use _() to mark this for translation. > -# Helper function to test hints for using --orphan in an empty repo. > test_wt_add_empty_repo_orphan_hint() { > - local context="$1" && > - local opts="${@:2}" && > + local context="$1" > + shift > + local opts="$@" > test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" ' > test_when_finished "rm -rf empty_repo" && > GIT_DIR="empty_repo" git init --bare && > - test_must_fail git -C empty_repo worktree add $opts 2> actual && > + test_must_fail git -C empty_repo worktree add $opts foobar/ 2> actual && Looks good. > grep "hint: If you meant to create a worktree containing a new orphan branch" actual > ' > } > > -test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/ > -test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/ > -test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/ > +test_wt_add_empty_repo_orphan_hint 'DWIM' > +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch > +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch > > test_expect_success 'local clone from linked checkout' ' > git clone --local here here-clone && > > > Also, Is there an easier way to debug the `test_expect_success` statements? I > tried enabling tracing mode but it doesn't seem to trace into those statements. Not really, other than just enabling "-x" for the test-lib.sh itself, i.e.: sh -x ./t0001-init.sh I *think* that should work, but I didn't test it...
On Mon, Dec 12, 2022 at 1:19 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Mon, Dec 12 2022, Jacob Abel wrote: > > Also, Is there an easier way to debug the `test_expect_success` statements? I > > tried enabling tracing mode but it doesn't seem to trace into those statements. > > Not really, other than just enabling "-x" for the test-lib.sh itself, i.e.: > > sh -x ./t0001-init.sh > > I *think* that should work, but I didn't test it... Isn't the question here how to debug the body of a test_expect_success? If that's the case, then running the test with -x and -i should help: ./t001-init.sh -x -i The -x makes it print all the output as it's executing the body of the test_expect_success rather than suppressing it, and -i makes it stop as soon as it encounters a failing test, which makes it easier to examine the state of that test. After the script aborts (via -i), look inside the "trash*" directory. Also, you can insert calls to test_pause in the body of a test, which will make it drop into an interactive shell session in the trash directory at the point of the test_pause, so you can examine the state of the test directly as it exists at that point (as opposed to examining it after the test failed, as with -i).
On 22/12/12 01:35PM, Eric Sunshine wrote: > On Mon, Dec 12, 2022 at 1:19 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > On Mon, Dec 12 2022, Jacob Abel wrote: > > > Also, Is there an easier way to debug the `test_expect_success` statements? I > > > tried enabling tracing mode but it doesn't seem to trace into those statements. > > > > Not really, other than just enabling "-x" for the test-lib.sh itself, i.e.: > > > > sh -x ./t0001-init.sh > > > > I *think* that should work, but I didn't test it... > > Isn't the question here how to debug the body of a > test_expect_success? If that's the case, then running the test with -x > and -i should help: > > ./t001-init.sh -x -i > > The -x makes it print all the output as it's executing the body of the > test_expect_success rather than suppressing it, and -i makes it stop > as soon as it encounters a failing test, which makes it easier to > examine the state of that test. After the script aborts (via -i), look > inside the "trash*" directory. Also, you can insert calls to > test_pause in the body of a test, which will make it drop into an > interactive shell session in the trash directory at the point of the > test_pause, so you can examine the state of the test directly as it > exists at that point (as opposed to examining it after the test > failed, as with -i). This works fantastically. I'm not sure why `sh -x ./tXXXX-script.sh` doesn't work but `./tXXXX-script.sh -x` does but this makes debugging a lot simpler. The `-i` flag is also super useful here. Appreciated.
On 22/12/12 07:16PM, Ævar Arnfjörð Bjarmason wrote: > > The following are what I have made for this set of changes (against v4). > > > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 8bb1453e0f..38f7a27927 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -732,12 +732,11 @@ static int add(int ac, const char **av, const char *prefix) > > } else if (!lookup_commit_reference_by_name(branch)) { > > advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, > > "If you meant to create a worktree containing a new orphan branch\n" > > - "(branch with no commits) for this repository, e.g. '%s',\n" > > - "you can do so using the --orphan option:\n" > > + "(branch with no commits) for this repository, you can do so\n" > > + "using the --orphan option:\n" > > "\n" > > - " git worktree add --orphan %s %s\n" > > - "\n", > > - new_branch, new_branch, path); > > + " git worktree add --orphan %s %s\n", > > + new_branch, path); > > Looks good, we'd usually put the "new_branch, path" on the previous line > (to the extent that it fits within 79 chars. > > Also: This string should use _() to mark this for translation. Done.
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index a00d0100a8..3e58521613 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -136,4 +136,8 @@ advice.*:: Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] is asked to update index entries outside the current sparse checkout. + worktreeAddOrphan:: + Advice shown when a user tries to create a worktree from an + invalid reference, to instruct how to create a new orphan + branch instead. -- diff --git a/advice.c b/advice.c index fd18968943..53e91fdb85 100644 --- a/advice.c +++ b/advice.c @@ -75,6 +75,7 @@ static struct { [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 }, [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 }, [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 }, }; static const char turn_off_instructions[] = diff --git a/advice.h b/advice.h index 07e0f76833..919d8c0064 100644 --- a/advice.h +++ b/advice.h @@ -50,6 +50,7 @@ struct string_list; ADVICE_UPDATE_SPARSE_PATH, ADVICE_WAITING_FOR_EDITOR, ADVICE_SKIPPED_CHERRY_PICKS, + ADVICE_WORKTREE_ADD_ORPHAN, }; int git_default_advice_config(const char *var, const char *value); diff --git a/builtin/worktree.c b/builtin/worktree.c index 51b247b2a7..ea306e18de 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -744,6 +744,14 @@ static int add(int ac, const char **av, const char *prefix) * If `branch` does not reference a valid commit, a new * worktree (and/or branch) cannot be created based off of it. */ + advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN, + "If you meant to create a worktree containing a new orphan branch\n" + "(branch with no commits) for this repository, e.g. '%s',\n" + "you can do so using the --orphan option:\n" + "\n" + " git worktree add --orphan %s %s\n" + "\n", + new_branch, new_branch, path); die(_("invalid reference: %s"), branch); } else if (new_branch) { struct child_process cp = CHILD_PROCESS_INIT; diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 6118ace92d..455cddcdd2 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -376,6 +376,22 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' ' test_cmp expect .git/worktrees/orphan-with-lock-reason/locked ' +# Helper function to test hints for using --orphan in an empty repo. +test_wt_add_empty_repo_orphan_hint() { + local context="$1" && + local opts="${@:2}" && + test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" ' + test_when_finished "rm -rf empty_repo" && + GIT_DIR="empty_repo" git init --bare && + test_must_fail git -C empty_repo worktree add $opts 2> actual && + grep "hint: If you meant to create a worktree containing a new orphan branch" actual + ' +} + +test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/ +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/ +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/ + test_expect_success 'local clone from linked checkout' ' git clone --local here here-clone && ( cd here-clone && git fsck )
Adds a new advice/hint in `git worktree add` for when the user tries to create a new worktree from a reference that doesn't exist. Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> --- Documentation/config/advice.txt | 4 ++++ advice.c | 1 + advice.h | 1 + builtin/worktree.c | 8 ++++++++ t/t2400-worktree-add.sh | 16 ++++++++++++++++ 5 files changed, 30 insertions(+) -- 2.37.4