| Message ID | 20221212014003.20290-3-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: > +static int make_worktree_orphan(const char * ref, const struct add_opts *opts, > + struct strvec *child_env) > +{ > + int ret; You can avoid this variable entirely.... > + struct strbuf symref = STRBUF_INIT; > + struct child_process cp = CHILD_PROCESS_INIT; > + cp.git_cmd = 1; (aside: We usually split up variables & decls, I think this is better right before the run_command() line). > + > + validate_new_branchname(ref, &symref, 0); > + strvec_pushl(&cp.args, "symbolic-ref", "HEAD", symref.buf, NULL); ...by just calling strbuf_release(&symref); right after this line, we'll never need it again, and the strvec will have its own copy. > + if (opts->quiet) > + strvec_push(&cp.args, "--quiet"); > + strvec_pushv(&cp.env, child_env->v); So: > + ret = run_command(&cp); > + strbuf_release(&symref); > + return ret; We don't have to carry the "ret" here, and can just do: return run_command(&cp); > +} > + > static int add_worktree(const char *path, const char *refname, > const struct add_opts *opts) > { > @@ -393,8 +415,9 @@ static int add_worktree(const char *path, const char *refname, > die_if_checked_out(symref.buf, 0); > } > commit = lookup_commit_reference_by_name(refname); > - if (!commit) > + if (!commit && !opts->orphan) { > die(_("invalid reference: %s"), refname); > + } We don't add {}'s for one-statement if's like this, see CodingGuidelines. So skip the {}'s. > > name = worktree_basename(path, &len); > strbuf_add(&sb, name, path + len - name); > @@ -482,10 +505,10 @@ static int add_worktree(const char *path, const char *refname, > strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path); > cp.git_cmd = 1; > > - if (!is_branch) > + if (!is_branch && commit) { > strvec_pushl(&cp.args, "update-ref", "HEAD", > oid_to_hex(&commit->object.oid), NULL); > - else { > + } else { Here that style change is good, even if it inflates the diff size a litte bit with the while-at-it fixu-up. > + /* > + * When creating a new branch, new_branch now contains the branch to > + * create. > + * > + * Past this point, new_branch_force can be treated solely as a > + * boolean flag to indicate whether `-B` was selected. > + */ > if (new_branch_force) { > struct strbuf symref = STRBUF_INIT; > I think I commented on this commentary in an earlier round. IMO it could just be omitted, as the code is rather self-explanatory. To the extent that it isn't this commentary just makes things more confusing, at least to my reading. It's not explaining what the code is doing now, because the very next line after this context (omitted here) is: new_branch = new_branch_force So we're saying it "can be treated solely as a boolean flag", but it isn't being treated as such by the code now. And the "new_branch now contains the branch to create" is also inaccurate, we're about to make it true with that assignment, but (and again, I don't think a comment is needed at all) *if* we think that's worth commenting on then surely the first paragraph of the comment should be split off, and come just before that assignment. > - if (new_branch) { > + if (opts.orphan) { > + branch = new_branch; > + } else if (!lookup_commit_reference_by_name(branch)) { > + /* > + * If `branch` does not reference a valid commit, a new > + * worktree (and/or branch) cannot be created based off of it. > + */ I think with the advice added in 3/3 this comment can also just be omitted here, as the end result is that the comment will be re-explaining something which should be obvious from the inline advice string (and if it isn't, that inline string needs improving). > -test_expect_success '"add" -b/-B mutually exclusive' ' > - test_must_fail git worktree add -b poodle -B poodle bamboo main > -' > - > -test_expect_success '"add" -b/--detach mutually exclusive' ' > - test_must_fail git worktree add -b poodle --detach bamboo main > -' > +# Helper function to test mutually exclusive options. > +test_wt_add_excl() { > + local opts="$@" && > + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' > + test_must_fail git worktree add $opts > + ' > +} > > -test_expect_success '"add" -B/--detach mutually exclusive' ' > - test_must_fail git worktree add -B poodle --detach bamboo main > -' > +test_wt_add_excl -b poodle -B poodle bamboo main > +test_wt_add_excl -b poodle --orphan poodle bamboo > +test_wt_add_excl -b poodle --detach bamboo main > +test_wt_add_excl -B poodle --detach bamboo main > +test_wt_add_excl -B poodle --detach bamboo main > +test_wt_add_excl -B poodle --orphan poodle bamboo > +test_wt_add_excl --orphan poodle --detach bamboo > +test_wt_add_excl --orphan poodle --no-checkout bamboo > +test_wt_add_excl --orphan poodle bamboo main It's good to see this as a helper function, but I think it would be nice to have this split up into its own pre-refactoring commit. As here we're changing some existing tests that are per-se unrelated, just so that they can use this new helper. This commit could then add tests that use the helper, and which are new for --orphan.
On 22/12/12 09:11AM, Ævar Arnfjörð Bjarmason wrote: > > > +static int make_worktree_orphan(const char * ref, const struct add_opts *opts, > > + struct strvec *child_env) > > +{ > > + int ret; > > You can avoid this variable entirely.... > > > + > > + validate_new_branchname(ref, &symref, 0); > > + strvec_pushl(&cp.args, "symbolic-ref", "HEAD", symref.buf, NULL); > > ...by just calling strbuf_release(&symref); right after this line, we'll > never need it again, and the strvec will have its own copy. > > > + if (opts->quiet) > > + strvec_push(&cp.args, "--quiet"); > > + strvec_pushv(&cp.env, child_env->v); > > So: > > > + ret = run_command(&cp); > > + strbuf_release(&symref); > > + return ret; > > We don't have to carry the "ret" here, and can just do: > > return run_command(&cp); > Done. > > > + struct strbuf symref = STRBUF_INIT; > > + struct child_process cp = CHILD_PROCESS_INIT; > > + cp.git_cmd = 1; > > (aside: We usually split up variables & decls, I think this is better > right before the run_command() line). Sorry, I'm not quite clear what you mean. > > +} > > + > > static int add_worktree(const char *path, const char *refname, > > const struct add_opts *opts) > > { > > @@ -393,8 +415,9 @@ static int add_worktree(const char *path, const char *refname, > > die_if_checked_out(symref.buf, 0); > > } > > commit = lookup_commit_reference_by_name(refname); > > - if (!commit) > > + if (!commit && !opts->orphan) { > > die(_("invalid reference: %s"), refname); > > + } > > We don't add {}'s for one-statement if's like this, see > CodingGuidelines. So skip the {}'s. > Ah. I think that slipped in when I temporarily added in logging for debug purposes. Removed. > > > > name = worktree_basename(path, &len); > > strbuf_add(&sb, name, path + len - name); > > @@ -482,10 +505,10 @@ static int add_worktree(const char *path, const char *refname, > > strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path); > > cp.git_cmd = 1; > > > > - if (!is_branch) > > + if (!is_branch && commit) { > > strvec_pushl(&cp.args, "update-ref", "HEAD", > > oid_to_hex(&commit->object.oid), NULL); > > - else { > > + } else { > > Here that style change is good, even if it inflates the diff size a > litte bit with the while-at-it fixu-up. > > > + /* > > + * When creating a new branch, new_branch now contains the branch to > > + * create. > > + * > > + * Past this point, new_branch_force can be treated solely as a > > + * boolean flag to indicate whether `-B` was selected. > > + */ > > if (new_branch_force) { > > struct strbuf symref = STRBUF_INIT; > > > > I think I commented on this commentary in an earlier round. IMO it could > just be omitted, as the code is rather self-explanatory. > > To the extent that it isn't this commentary just makes things more > confusing, at least to my reading. It's not explaining what the code is > doing now, because the very next line after this context (omitted here) is: > > new_branch = new_branch_force > > So we're saying it "can be treated solely as a boolean flag", but it > isn't being treated as such by the code now. > > And the "new_branch now contains the branch to create" is also > inaccurate, we're about to make it true with that assignment, but (and > again, I don't think a comment is needed at all) *if* we think that's > worth commenting on then surely the first paragraph of the comment > should be split off, and come just before that assignment. Ah yep. In a previous round I removed the other comment but forgot this one. Removed. > > > - if (new_branch) { > > + if (opts.orphan) { > > + branch = new_branch; > > + } else if (!lookup_commit_reference_by_name(branch)) { > > + /* > > + * If `branch` does not reference a valid commit, a new > > + * worktree (and/or branch) cannot be created based off of it. > > + */ > > I think with the advice added in 3/3 this comment can also just be > omitted here, as the end result is that the comment will be > re-explaining something which should be obvious from the inline advice > string (and if it isn't, that inline string needs improving). Done. > > > -test_expect_success '"add" -b/-B mutually exclusive' ' > > - test_must_fail git worktree add -b poodle -B poodle bamboo main > > -' > > - > > -test_expect_success '"add" -b/--detach mutually exclusive' ' > > - test_must_fail git worktree add -b poodle --detach bamboo main > > -' > > +# Helper function to test mutually exclusive options. > > +test_wt_add_excl() { > > + local opts="$@" && > > + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' > > + test_must_fail git worktree add $opts > > + ' > > +} > > > > -test_expect_success '"add" -B/--detach mutually exclusive' ' > > - test_must_fail git worktree add -B poodle --detach bamboo main > > -' > > +test_wt_add_excl -b poodle -B poodle bamboo main > > +test_wt_add_excl -b poodle --orphan poodle bamboo > > +test_wt_add_excl -b poodle --detach bamboo main > > +test_wt_add_excl -B poodle --detach bamboo main > > +test_wt_add_excl -B poodle --detach bamboo main > > +test_wt_add_excl -B poodle --orphan poodle bamboo > > +test_wt_add_excl --orphan poodle --detach bamboo > > +test_wt_add_excl --orphan poodle --no-checkout bamboo > > +test_wt_add_excl --orphan poodle bamboo main > > It's good to see this as a helper function, but I think it would be nice > to have this split up into its own pre-refactoring commit. > > As here we're changing some existing tests that are per-se unrelated, > just so that they can use this new helper. > > This commit could then add tests that use the helper, and which are new > for --orphan. Done. Also at some point I think I accidentally rolled back the change I made to remove the duplicate `test_wt_add_excl -B poodle --detach bamboo main` so I've made sure to remove that this time.
On Mon, Dec 12 2022, Jacob Abel wrote: > On 22/12/12 09:11AM, Ævar Arnfjörð Bjarmason wrote: >> >> > + struct strbuf symref = STRBUF_INIT; >> > + struct child_process cp = CHILD_PROCESS_INIT; >> > + cp.git_cmd = 1; >> >> (aside: We usually split up variables & decls, I think this is better >> right before the run_command() line). > > Sorry, I'm not quite clear what you mean. I mean that we usually put two newlines between the last deceleration and the start of any code. And additionally, that the usual pattern for CHILD_PROCESS_INIT is to do these "flag" assignments right before the run_command(). See e.g. the code in "check_clean_worktree()" So following its example would resolve the decl style nit
On 22/12/12 07:14PM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 12 2022, Jacob Abel wrote: > > > On 22/12/12 09:11AM, Ævar Arnfjörð Bjarmason wrote: > >> > >> > + struct strbuf symref = STRBUF_INIT; > >> > + struct child_process cp = CHILD_PROCESS_INIT; > >> > + cp.git_cmd = 1; > >> > >> (aside: We usually split up variables & decls, I think this is better > >> right before the run_command() line). > > > > Sorry, I'm not quite clear what you mean. > > I mean that we usually put two newlines between the last deceleration > and the start of any code. > > And additionally, that the usual pattern for CHILD_PROCESS_INIT is to do > these "flag" assignments right before the run_command(). > > See e.g. the code in "check_clean_worktree()" > > So following its example would resolve the decl style nit Ah OK. I understand what you mean now. I've made the change.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 4dd658012b..c6e6899d8b 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -11,6 +11,8 @@ SYNOPSIS [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [[-b | -B] <new-branch>] <path> [<commit-ish>] +'git worktree add' [-f] [--lock [--reason <string>]] + --orphan <new-branch> <path> 'git worktree list' [-v | --porcelain [-z]] 'git worktree lock' [--reason <string>] <worktree> 'git worktree move' <worktree> <new-path> @@ -95,6 +97,15 @@ exist, a new branch based on `HEAD` is automatically created as if `-b <branch>` was given. If `<branch>` does exist, it will be checked out in the new worktree, if it's not checked out anywhere else, otherwise the command will refuse to create the worktree (unless `--force` is used). ++ +------------ +$ git worktree add --orphan <branch> <path> +------------ ++ +Create a worktree containing no files, with an empty index, and associated +with a new orphan branch named `<branch>`. The first commit made on this new +branch will have no parents and will be the root of a new history disconnected +from any other branches. list:: @@ -222,6 +233,10 @@ This can also be set up as the default behaviour by using the With `prune`, do not remove anything; just report what it would remove. +--orphan <new-branch>:: + With `add`, make the new worktree and index empty, associating + the worktree with a new orphan branch named `<new-branch>`. + --porcelain:: With `list`, output in an easy-to-parse format for scripts. This format will remain stable across Git versions and regardless of user diff --git a/builtin/worktree.c b/builtin/worktree.c index fccb17f070..51b247b2a7 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -17,7 +17,10 @@ #define BUILTIN_WORKTREE_ADD_USAGE \ N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \ - " [[-b | -B] <new-branch>] <path> [<commit-ish>]") + " [[-b | -B] <new-branch>] <path> [<commit-ish>]"), \ + N_("git worktree add [-f] [--lock [--reason <string>]]\n" \ + " --orphan <new-branch> <path>") + #define BUILTIN_WORKTREE_LIST_USAGE \ N_("git worktree list [-v | --porcelain [-z]]") #define BUILTIN_WORKTREE_LOCK_USAGE \ @@ -90,6 +93,7 @@ struct add_opts { int detach; int quiet; int checkout; + int orphan; const char *keep_locked; }; @@ -364,6 +368,24 @@ static int checkout_worktree(const struct add_opts *opts, return run_command(&cp); } +static int make_worktree_orphan(const char * ref, const struct add_opts *opts, + struct strvec *child_env) +{ + int ret; + struct strbuf symref = STRBUF_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; + + validate_new_branchname(ref, &symref, 0); + strvec_pushl(&cp.args, "symbolic-ref", "HEAD", symref.buf, NULL); + if (opts->quiet) + strvec_push(&cp.args, "--quiet"); + strvec_pushv(&cp.env, child_env->v); + ret = run_command(&cp); + strbuf_release(&symref); + return ret; +} + static int add_worktree(const char *path, const char *refname, const struct add_opts *opts) { @@ -393,8 +415,9 @@ static int add_worktree(const char *path, const char *refname, die_if_checked_out(symref.buf, 0); } commit = lookup_commit_reference_by_name(refname); - if (!commit) + if (!commit && !opts->orphan) { die(_("invalid reference: %s"), refname); + } name = worktree_basename(path, &len); strbuf_add(&sb, name, path + len - name); @@ -482,10 +505,10 @@ static int add_worktree(const char *path, const char *refname, strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path); cp.git_cmd = 1; - if (!is_branch) + if (!is_branch && commit) { strvec_pushl(&cp.args, "update-ref", "HEAD", oid_to_hex(&commit->object.oid), NULL); - else { + } else { strvec_pushl(&cp.args, "symbolic-ref", "HEAD", symref.buf, NULL); if (opts->quiet) @@ -497,6 +520,10 @@ static int add_worktree(const char *path, const char *refname, if (ret) goto done; + if (opts->orphan && + (ret = make_worktree_orphan(refname, opts, &child_env))) + goto done; + if (opts->checkout && (ret = checkout_worktree(opts, &child_env))) goto done; @@ -516,7 +543,7 @@ static int add_worktree(const char *path, const char *refname, * Hook failure does not warrant worktree deletion, so run hook after * is_junk is cleared, but do return appropriate code when hook fails. */ - if (!ret && opts->checkout) { + if (!ret && opts->checkout && !opts->orphan) { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL); @@ -605,6 +632,7 @@ static int add(int ac, const char **av, const char *prefix) char *path; const char *branch; const char *new_branch = NULL; + const char *orphan_branch = NULL; const char *opt_track = NULL; const char *lock_reason = NULL; int keep_locked = 0; @@ -616,6 +644,8 @@ static int add(int ac, const char **av, const char *prefix) N_("create a new branch")), OPT_STRING('B', NULL, &new_branch_force, N_("branch"), N_("create or reset a branch")), + OPT_STRING(0, "orphan", &orphan_branch, N_("branch"), + N_("new unparented branch")), OPT_BOOL('d', "detach", &opts.detach, N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")), OPT_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")), @@ -633,8 +663,20 @@ static int add(int ac, const char **av, const char *prefix) memset(&opts, 0, sizeof(opts)); opts.checkout = 1; ac = parse_options(ac, av, prefix, options, git_worktree_add_usage, 0); + opts.orphan = !!orphan_branch; if (!!opts.detach + !!new_branch + !!new_branch_force > 1) die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach"); + if (!!opts.detach + !!opts.orphan + !!new_branch + !!new_branch_force > 1) + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), + "-b", "-B", "--orphan", "--detach"); + if (opts.orphan && opt_track) + die(_("'%s' and '%s' cannot be used together"), "--orphan", "--track"); + if (opts.orphan && !opts.checkout) + die(_("'%s' and '%s' cannot be used together"), "--orphan", + "--no-checkout"); + if (opts.orphan && ac == 2) + die(_("'%s' and '%s' cannot be used together"), "--orphan", + _("<commit-ish>")); if (lock_reason && !keep_locked) die(_("the option '%s' requires '%s'"), "--reason", "--lock"); if (lock_reason) @@ -651,6 +693,13 @@ static int add(int ac, const char **av, const char *prefix) if (!strcmp(branch, "-")) branch = "@{-1}"; + /* + * When creating a new branch, new_branch now contains the branch to + * create. + * + * Past this point, new_branch_force can be treated solely as a + * boolean flag to indicate whether `-B` was selected. + */ if (new_branch_force) { struct strbuf symref = STRBUF_INIT; @@ -663,7 +712,9 @@ static int add(int ac, const char **av, const char *prefix) strbuf_release(&symref); } - if (ac < 2 && !new_branch && !opts.detach) { + if (opts.orphan) { + new_branch = orphan_branch; + } else if (ac < 2 && !new_branch && !opts.detach) { const char *s = dwim_branch(path, &new_branch); if (s) branch = s; @@ -686,7 +737,15 @@ static int add(int ac, const char **av, const char *prefix) if (!opts.quiet) print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force); - if (new_branch) { + if (opts.orphan) { + branch = new_branch; + } else if (!lookup_commit_reference_by_name(branch)) { + /* + * If `branch` does not reference a valid commit, a new + * worktree (and/or branch) cannot be created based off of it. + */ + die(_("invalid reference: %s"), branch); + } else if (new_branch) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; strvec_push(&cp.args, "branch"); diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index d587e0b20d..6118ace92d 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -298,17 +298,23 @@ test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' ' test_must_fail git -C mish/mash symbolic-ref HEAD ' -test_expect_success '"add" -b/-B mutually exclusive' ' - test_must_fail git worktree add -b poodle -B poodle bamboo main -' - -test_expect_success '"add" -b/--detach mutually exclusive' ' - test_must_fail git worktree add -b poodle --detach bamboo main -' +# Helper function to test mutually exclusive options. +test_wt_add_excl() { + local opts="$@" && + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' + test_must_fail git worktree add $opts + ' +} -test_expect_success '"add" -B/--detach mutually exclusive' ' - test_must_fail git worktree add -B poodle --detach bamboo main -' +test_wt_add_excl -b poodle -B poodle bamboo main +test_wt_add_excl -b poodle --orphan poodle bamboo +test_wt_add_excl -b poodle --detach bamboo main +test_wt_add_excl -B poodle --detach bamboo main +test_wt_add_excl -B poodle --detach bamboo main +test_wt_add_excl -B poodle --orphan poodle bamboo +test_wt_add_excl --orphan poodle --detach bamboo +test_wt_add_excl --orphan poodle --no-checkout bamboo +test_wt_add_excl --orphan poodle bamboo main test_expect_success '"add -B" fails if the branch is checked out' ' git rev-parse newmain >before && @@ -330,6 +336,46 @@ test_expect_success 'add --quiet' ' test_must_be_empty actual ' +test_expect_success '"add --orphan"' ' + test_when_finished "git worktree remove -f -f orphandir" && + git worktree add --orphan neworphan orphandir && + echo refs/heads/neworphan >expected && + git -C orphandir symbolic-ref HEAD >actual && + test_cmp expected actual +' + +test_expect_success '"add --orphan" fails if the branch already exists' ' + test_when_finished "git branch -D existingbranch" && + test_when_finished "git worktree remove -f -f orphandir" && + git worktree add -b existingbranch orphandir main && + test_must_fail git worktree add --orphan existingbranch orphandir2 && + test_path_is_missing orphandir2 +' + +test_expect_success '"add --orphan" with empty repository' ' + test_when_finished "rm -rf empty_repo" && + echo refs/heads/newbranch >expected && + GIT_DIR="empty_repo" git init --bare && + git -C empty_repo worktree add --orphan newbranch worktreedir && + git -C empty_repo/worktreedir symbolic-ref HEAD >actual && + test_cmp expected actual +' + +test_expect_success '"add" worktree with orphan branch and lock' ' + git worktree add --lock --orphan orphanbr orphan-with-lock && + test_when_finished "git worktree unlock orphan-with-lock || :" && + test -f .git/worktrees/orphan-with-lock/locked +' + +test_expect_success '"add" worktree with orphan branch, lock, and reason' ' + lock_reason="why not" && + git worktree add --detach --lock --reason "$lock_reason" orphan-with-lock-reason main && + test_when_finished "git worktree unlock orphan-with-lock-reason || :" && + test -f .git/worktrees/orphan-with-lock-reason/locked && + echo "$lock_reason" >expect && + test_cmp expect .git/worktrees/orphan-with-lock-reason/locked +' + test_expect_success 'local clone from linked checkout' ' git clone --local here here-clone && ( cd here-clone && git fsck ) @@ -446,6 +492,14 @@ setup_remote_repo () { ) } +test_expect_success '"add" <path> <remote/branch> w/ no HEAD' ' + test_when_finished rm -rf repo_upstream repo_local foo && + setup_remote_repo repo_upstream repo_local && + git -C repo_local config --bool core.bare true && + git -C repo_local branch -D main && + git -C repo_local worktree add ./foo repo_upstream/foo +' + test_expect_success '--no-track avoids setting up tracking' ' test_when_finished rm -rf repo_upstream repo_local foo && setup_remote_repo repo_upstream repo_local &&