| Message ID | 20221110233137.10414-3-jacobabel@nullpo.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: Support `--orphan` when creating new worktrees | expand |
On Thu, Nov 10 2022, Jacob Abel wrote: > Adds support for creating an orphan branch when adding a new worktree. > This functionality is equivalent to git switch's --orphan flag. > > The original reason this feature was implemented was to allow a user > to initialise a new repository using solely the worktree oriented > workflow. Example usage included below. > > $ GIT_DIR=".git" git init --bare > $ git worktree add --orphan master master/ > > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> > --- > Documentation/git-worktree.txt | 14 +++++++- > builtin/worktree.c | 64 ++++++++++++++++++++++++++++++---- > t/t2400-worktree-add.sh | 45 ++++++++++++++++++++++++ > 3 files changed, 115 insertions(+), 8 deletions(-) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 4dd658012b..1310bfb564 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -10,7 +10,7 @@ SYNOPSIS > -------- > [verse] > 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] > - [[-b | -B] <new-branch>] <path> [<commit-ish>] > + [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] > 'git worktree list' [-v | --porcelain [-z]] > 'git worktree lock' [--reason <string>] <worktree> > 'git worktree move' <worktree> <new-path> > @@ -95,6 +95,14 @@ 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 an orphan branch named `<branch>` with a > +clean working directory. See `--orphan` in linkgit:git-switch[1] for > +more details. Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent in general to avoid linking to it in lieu of "git checkout". In this case in particular though the "more details" are almost completely absent from the "git-switch" docs, and they don't (which is their won flaw) link to the more detailed "git-checkout" docs. But for this patch, it seems much better to link to the "checkout" docs, no? > +--orphan <new-branch>:: > + With `add`, create a new orphan branch named `<new-branch>` in the new > + worktree. See `--orphan` in linkgit:git-switch[1] for details. Ditto. > +test_expect_success '"add" --orphan/-b mutually exclusive' ' > + test_must_fail git worktree add --orphan poodle -b poodle bamboo > +' > + > +test_expect_success '"add" --orphan/-B mutually exclusive' ' > + test_must_fail git worktree add --orphan poodle -B poodle bamboo > +' > + > +test_expect_success '"add" --orphan/--detach mutually exclusive' ' > + test_must_fail git worktree add --orphan poodle --detach bamboo > +' > + > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' > + test_must_fail git worktree add --orphan poodle --no-checkout bamboo > +' > + > +test_expect_success '"add" -B/--detach mutually exclusive' ' > + test_must_fail git worktree add -B poodle --detach bamboo main > +' > + This would be much better as a for-loop: for opt in -b -B ... do test_expect_success "...$opt" '<test here, uses $opt>' done Note the ""-quotes for the description, and '' for the test, that's not a mistake, we eval() the latter. > +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 ! -d orphandir2 I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants to be a "test_path_is_missing"? In general we try to test what a thing is, not what it isn't, in this case don't we fail to create the dir entirely? So "not exists" would cover it?
On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Thu, Nov 10 2022, Jacob Abel wrote: > > Adds support for creating an orphan branch when adding a new worktree. > > This functionality is equivalent to git switch's --orphan flag. > > > > The original reason this feature was implemented was to allow a user > > to initialise a new repository using solely the worktree oriented > > workflow. Example usage included below. > > > > $ GIT_DIR=".git" git init --bare > > $ git worktree add --orphan master master/ > > > > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> > > --- > > +Create a worktree containing an orphan branch named `<branch>` with a > > +clean working directory. See `--orphan` in linkgit:git-switch[1] for > > +more details. > > Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent > in general to avoid linking to it in lieu of "git checkout". > > In this case in particular though the "more details" are almost > completely absent from the "git-switch" docs, and they don't (which is > their won flaw) link to the more detailed "git-checkout" docs. > > But for this patch, it seems much better to link to the "checkout" docs, > no? Sorry, no. The important point here is that the --orphan option being added to `git worktree add` closely follows the behavior of `git switch --orphan`, which is quite different from the behavior of `git checkout --orphan`. The `git switch --orphan` documentation doesn't seem particularly lacking; it correctly describes the (very) simplified behavior of that command over `git checkout --orphan`. I might agree that there isn't much reason to link to git-switch for "more details", though, since there isn't really anything else that needs to be said. If we did want to say something else here, we might copy one sentence from the `git checkout --orphan` documentation: The first commit made on this new branch will have no parents and it will be the root of a new history totally disconnected from all the other branches and commits. The same sentence could be added to `git switch --orphan` documentation, but that's outside the scope of this patch series (thus can be done later by someone). > > +test_expect_success '"add" --orphan/-b mutually exclusive' ' > > + test_must_fail git worktree add --orphan poodle -b poodle bamboo > > +' > > + > > +test_expect_success '"add" --orphan/-B mutually exclusive' ' > > + test_must_fail git worktree add --orphan poodle -B poodle bamboo > > +' > > + > > +test_expect_success '"add" --orphan/--detach mutually exclusive' ' > > + test_must_fail git worktree add --orphan poodle --detach bamboo > > +' > > + > > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' > > + test_must_fail git worktree add --orphan poodle --no-checkout bamboo > > +' > > + > > +test_expect_success '"add" -B/--detach mutually exclusive' ' > > + test_must_fail git worktree add -B poodle --detach bamboo main > > +' > > + > > This would be much better as a for-loop: > > for opt in -b -B ... > do > test_expect_success "...$opt" '<test here, uses $opt>' > done > > Note the ""-quotes for the description, and '' for the test, that's not > a mistake, we eval() the latter. Such a loop would need to be more complex than this, wouldn't it, to account for all the combinations? I'd normally agree about the loop, but given that it requires extra complexity, I don't really mind seeing the individual tests spelled out manually in this case; they're dead simple to understand as written. I don't feel strongly either way, but I also don't want to ask for extra work from the patch author for a subjective change.
On Thu, Nov 10 2022, Jacob Abel wrote: So, on a second read-through... > 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] > - [[-b | -B] <new-branch>] <path> [<commit-ish>] > + [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] This synopsis is now at least partially wrong, and .... > +--orphan <new-branch>:: > + With `add`, create a new orphan branch named `<new-branch>` in the new > + worktree. See `--orphan` in linkgit:git-switch[1] for details. > + > --porcelain:: > .... > #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 | --orphan] <new-branch>] <path> [<commit-ish>]") ...here we say the same, but surely it's only: git worktree add --orphan new-branch /tmp/orphan And not e.g.: git worktree add --orphan new-branch /tmp/orphan origin/next Or whatever, but it's incompatible with <commit-ish>. I think this on top should fix it up: diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 1310bfb564f..3afef985154 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -10,7 +10,9 @@ SYNOPSIS -------- [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] - [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] + [[-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> diff --git a/builtin/worktree.c b/builtin/worktree.c index 71786b72f6b..2b811630b3a 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 | --orphan] <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 \ @@ -668,6 +671,9 @@ static int add(int ac, const char **av, const char *prefix) if (opts.orphan_branch && !opts.checkout) die(_("'%s' and '%s' cannot be used together"), "--orphan", "--no-checkout"); + if (opts.orphan_branch && 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) diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 93c340f4aff..47461d02115 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -326,6 +326,10 @@ test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' test_must_fail git worktree add --orphan poodle --no-checkout bamboo ' +test_expect_success '"add" --orphan and <commit-ish> mutually exclusive' ' + test_must_fail git worktree add --orphan poodle bamboo main +' + test_expect_success '"add" -B/--detach mutually exclusive' ' test_must_fail git worktree add -B poodle --detach bamboo main ' > - if (ac < 2 && !new_branch && !opts.detach) { > + /* > + * As the orphan cannot be created until the contents of branch > + * are staged, opts.orphan_branch should be treated as both a boolean > + * indicating that `--orphan` was selected and as the name of the new > + * orphan branch from this point on. > + */ I've re-read this a couple of times, and I honestly still don't see what point is trying to drive home. So, "--orphan" is an OPT_STRING(), so it always has a value: $ ./git worktree add --orphan error: option `orphan' requires a value But we init it to NULL, and above we just used it as a boolean *and* below. I.e. this comment would seem to me to fit with code where the reader might be surprised that we're using "opts.orphan_branch" as a string from then on, but we're just copying that to "new_branch", then we always use "opts.orphan_branch" as a boolean for the rest of the function. I may be missing something, but I think this would probably be better just without this comment. E.g. we use "--track", "--lock-reason" etc. in similar ways, and those don't have a comment like that. > + if (opts.orphan_branch) { > + new_branch = opts.orphan_branch; > + } > + > + if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) { In general we shouldn't combine random "if"'s just because a a sufficiently smart compiler could discover a way to reduce work. But in this case these seem to be inherently connected, we always want the not-DWIM behavior with "orphan", no? So shouldn't this just be: if (opts.orphan_branch) { ... } else if (ac < 2 && !new_branch && !opts.detach) { .... } ?
On Tue, Nov 15 2022, Eric Sunshine wrote: > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> On Thu, Nov 10 2022, Jacob Abel wrote: >> > Adds support for creating an orphan branch when adding a new worktree. >> > This functionality is equivalent to git switch's --orphan flag. >> > >> > The original reason this feature was implemented was to allow a user >> > to initialise a new repository using solely the worktree oriented >> > workflow. Example usage included below. >> > >> > $ GIT_DIR=".git" git init --bare >> > $ git worktree add --orphan master master/ >> > >> > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> >> > --- >> > +Create a worktree containing an orphan branch named `<branch>` with a >> > +clean working directory. See `--orphan` in linkgit:git-switch[1] for >> > +more details. >> >> Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent >> in general to avoid linking to it in lieu of "git checkout". >> >> In this case in particular though the "more details" are almost >> completely absent from the "git-switch" docs, and they don't (which is >> their won flaw) link to the more detailed "git-checkout" docs. >> >> But for this patch, it seems much better to link to the "checkout" docs, >> no? > > Sorry, no. The important point here is that the --orphan option being > added to `git worktree add` closely follows the behavior of `git > switch --orphan`, which is quite different from the behavior of `git > checkout --orphan`. > > The `git switch --orphan` documentation doesn't seem particularly > lacking; it correctly describes the (very) simplified behavior of that > command over `git checkout --orphan`. I might agree that there isn't > much reason to link to git-switch for "more details", though, since > there isn't really anything else that needs to be said. Aside from what it says now: 1/2 of what I'm saying is that linking to it while it says it's "EXPERIMENTAL" might be either jumping the gun. Or maybe we should just declare it non-"EXPERIMENTAL", but in any case this unrelated topic might want to avoid that altogether and just link to the "checkout" version. A quick grep of our docs (for linkgit:git-switch) that this would be the first mention outside of user-manual.txt where we link to it when it's not in the context of "checkout or switch", or where we're explaining something switch-specific (i.e. the "suggestDetachingHead" advice). Having said that I don't really care, just a suggestion... > If we did want to say something else here, we might copy one sentence > from the `git checkout --orphan` documentation: > > The first commit made on this new branch will have no parents and > it will be the root of a new history totally disconnected from all > the other branches and commits. > > The same sentence could be added to `git switch --orphan` > documentation, but that's outside the scope of this patch series (thus > can be done later by someone). I think I was partially confused by skimming the SYNOPSIS and thinking this supported <start-point> like checkout, which as I found in https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ just seems to be a missing assertion where we want to die() if that's provided in this mode. What I also found a bit confusing (but maybe it's just me) is that the "with a clean working directory" seemed at first to be drawing a distinction between this behavior and that of "git switch", but from poking at it some more it seems to be expressing "this is like git switch's --orphan" with that. I think instead of "clean working tree" it would be better to talk about "tracked files", as "git switch --orphan" does, which AFAICT is what it means. But then again the reason "switch" does that is because you have *existing* tracked files, which inherently doesn't apply for "worktree". Hrm. So, I guess it depends on your mental model of this operation, but at least I think it's more intuitive to explain it in terms of "git checkout --orphan", not "git switch --orphan". I.e.: Create a worktree containing an orphan branch named `<branch>`. This works like linkgit:git-checkout[1]'s `--orphan' option, except '<start-point>` isn't supported, and the "clear the index" doesn't apply (as "worktree add" will always have a new index)". Whereas defining this in terms of git-switch's "All tracked files are removed" might just be more confusing. What files? Since it's "worktree add" there weren't any in the first place. Anyway, I don't mind it as it is, but maybe the above write-up helps for #leftoverbits if we ever want to unify these docs. I.e. AFAICT we could: * Link from git-worktree to git-checkout, saying the above * Link from git-switch to git-checkout, ditto, but that we also "remove tracked files [of the current HEAD]". >> > +test_expect_success '"add" --orphan/-b mutually exclusive' ' >> > + test_must_fail git worktree add --orphan poodle -b poodle bamboo >> > +' >> > + >> > +test_expect_success '"add" --orphan/-B mutually exclusive' ' >> > + test_must_fail git worktree add --orphan poodle -B poodle bamboo >> > +' >> > + >> > +test_expect_success '"add" --orphan/--detach mutually exclusive' ' >> > + test_must_fail git worktree add --orphan poodle --detach bamboo >> > +' >> > + >> > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' >> > + test_must_fail git worktree add --orphan poodle --no-checkout bamboo >> > +' >> > + >> > +test_expect_success '"add" -B/--detach mutually exclusive' ' >> > + test_must_fail git worktree add -B poodle --detach bamboo main >> > +' >> > + >> >> This would be much better as a for-loop: >> >> for opt in -b -B ... >> do >> test_expect_success "...$opt" '<test here, uses $opt>' >> done >> >> Note the ""-quotes for the description, and '' for the test, that's not >> a mistake, we eval() the latter. > > Such a loop would need to be more complex than this, wouldn't it, to > account for all the combinations? I'd normally agree about the loop, > but given that it requires extra complexity, I don't really mind > seeing the individual tests spelled out manually in this case; they're > dead simple to understand as written. I don't feel strongly either > way, but I also don't want to ask for extra work from the patch author > for a subjective change. Yeah, it's probably not worth it. This is partially cleaning up existing tests, but maybe: diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 93c340f4aff..5acfd48f418 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -298,37 +298,21 @@ 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 -' - -test_expect_success '"add" -B/--detach mutually exclusive' ' - test_must_fail git worktree add -B poodle --detach bamboo main -' - -test_expect_success '"add" --orphan/-b mutually exclusive' ' - test_must_fail git worktree add --orphan poodle -b poodle bamboo -' - -test_expect_success '"add" --orphan/-B mutually exclusive' ' - test_must_fail git worktree add --orphan poodle -B poodle bamboo -' - -test_expect_success '"add" --orphan/--detach mutually exclusive' ' - test_must_fail git worktree add --orphan poodle --detach bamboo -' - -test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' - test_must_fail git worktree add --orphan poodle --no-checkout bamboo -' - -test_expect_success '"add" -B/--detach mutually exclusive' ' - test_must_fail git worktree add -B poodle --detach bamboo main -' +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_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 && I re-arranged that a bit, but probably not worth a loop. I *did* spot in doing that that if I sort the options I end up with a duplicate test, i.e. we test "-B poodle --detach bamboo main" twice. That seems to be added by mistake in 2/2, i.e. it's the existing test you can see in the diff context, just added at the end.
On Tue, Nov 15, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Nov 15 2022, Eric Sunshine wrote: > > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> But for this patch, it seems much better to link to the "checkout" docs, > >> no? > > > > Sorry, no. The important point here is that the --orphan option being > > added to `git worktree add` closely follows the behavior of `git > > switch --orphan`, which is quite different from the behavior of `git > > checkout --orphan`. > > > > The `git switch --orphan` documentation doesn't seem particularly > > lacking; it correctly describes the (very) simplified behavior of that > > command over `git checkout --orphan`. I might agree that there isn't > > much reason to link to git-switch for "more details", though, since > > there isn't really anything else that needs to be said. > > Aside from what it says now: 1/2 of what I'm saying is that linking to > it while it says it's "EXPERIMENTAL" might be either jumping the gun. > > Or maybe we should just declare it non-"EXPERIMENTAL", but in any case > this unrelated topic might want to avoid that altogether and just link > to the "checkout" version. Even better would be for the documentation added by this patch to be self-contained and not bother linking anywhere to further explain --orphan. That would satisfy your concern, I think, as well as my concern that `git checkout --orphan` documentation is inappropriate for `git worktree add --orphan`. > > If we did want to say something else here, we might copy one sentence > > from the `git checkout --orphan` documentation: > > > > The first commit made on this new branch will have no parents and > > it will be the root of a new history totally disconnected from all > > the other branches and commits. > > > > The same sentence could be added to `git switch --orphan` > > documentation, but that's outside the scope of this patch series (thus > > can be done later by someone). > > I think I was partially confused by skimming the SYNOPSIS and thinking > this supported <start-point> like checkout, which as I found in > https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ > just seems to be a missing assertion where we want to die() if that's > provided in this mode. I haven't read v3 yet, so I wasn't aware that the SYNOPSIS hadn't been updated to match the reworked --orphan behavior implemented by v3, but I can certainly understand how that would have led you astray. You're quite correct that the SYNOPSIS should not be saying that <commit-ish> is allowed with --orphan. > What I also found a bit confusing (but maybe it's just me) is that the > "with a clean working directory" seemed at first to be drawing a > distinction between this behavior and that of "git switch", but from > poking at it some more it seems to be expressing "this is like git > switch's --orphan" with that. "clean working directory" may indeed be ambiguous and confusing. It's not necessarily clear if it means "no changes to tracked files" or "no files in directory". We should use more precise terminology. > I think instead of "clean working tree" it would be better to talk about > "tracked files", as "git switch --orphan" does, which AFAICT is what it > means. But then again the reason "switch" does that is because you have > *existing* tracked files, which inherently doesn't apply for "worktree". > > Hrm. > > So, I guess it depends on your mental model of this operation, but at > least I think it's more intuitive to explain it in terms of "git > checkout --orphan", not "git switch --orphan". I.e.: > > Create a worktree containing an orphan branch named > `<branch>`. This works like linkgit:git-checkout[1]'s `--orphan' > option, except '<start-point>` isn't supported, and the "clear > the index" doesn't apply (as "worktree add" will always have a > new index)". > > Whereas defining this in terms of git-switch's "All tracked files are > removed" might just be more confusing. What files? Since it's "worktree > add" there weren't any in the first place. I would find it clearer not to talk about or reference `git checkout --orphan` at all. And, as mentioned above, it shouldn't need to reference `git switch --orphan` either. How about something like this for the description of the `add` subcommand? Create a worktree containing no files and 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. And then to document the --orphan command: With `add`, make the new worktree and index empty, and associate the worktree with a new orphan branch named `<new-branch>`. > >> This would be much better as a for-loop: > > > > Such a loop would need to be more complex than this, wouldn't it, to > > account for all the combinations? I'd normally agree about the loop, > > but given that it requires extra complexity, I don't really mind > > seeing the individual tests spelled out manually in this case; they're > > dead simple to understand as written. I don't feel strongly either > > way, but I also don't want to ask for extra work from the patch author > > for a subjective change. > > Yeah, it's probably not worth it. This is partially cleaning up existing > tests, but maybe: > > -test_expect_success '"add" -b/-B mutually exclusive' ' > - test_must_fail git worktree add -b poodle -B poodle bamboo main > -' > - > +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_wt_add_excl -b poodle -B poodle bamboo main > +test_wt_add_excl -b poodle --orphan poodle bamboo I'm rather "meh" here. Yes it's one line per test rather than rather than two or three, but it isn't saving much typing, and it isn't really making it easier for a reader to see what's going on. So, considering that it's so subjective and I'd like to avoid asking the patch author for subjective changes, I'm fine with the way it's done already in the patch. > I re-arranged that a bit, but probably not worth a loop. I *did* spot in > doing that that if I sort the options I end up with a duplicate test, > i.e. we test "-B poodle --detach bamboo main" twice. > > That seems to be added by mistake in 2/2, i.e. it's the existing test > you can see in the diff context, just added at the end. Dropping the duplicate sounds like a good idea.
On 22/11/15 10:08PM, Ævar Arnfjörð Bjarmason wrote: > > > +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 ! -d orphandir2 > > I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants > to be a "test_path_is_missing"? > > In general we try to test what a thing is, not what it isn't, in this > case don't we fail to create the dir entirely? So "not exists" would > cover it? Ah yes that would be preferable. I've updated it for v4. This shows up in the file in a few other places in this file as well (from before this patch). Should I make the changes there as well and put those changes into an additional patch in this patchset?
On 22/11/15 11:09PM, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Nov 10 2022, Jacob Abel wrote: > > So, on a second read-through... > > > 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] > > - [[-b | -B] <new-branch>] <path> [<commit-ish>] > > + [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] > > This synopsis is now at least partially wrong, and .... > > > +--orphan <new-branch>:: > > + With `add`, create a new orphan branch named `<new-branch>` in the new > > + worktree. See `--orphan` in linkgit:git-switch[1] for details. > > + > > --porcelain:: > > .... > > #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 | --orphan] <new-branch>] <path> [<commit-ish>]") > > > ...here we say the same, but surely it's only: > > git worktree add --orphan new-branch /tmp/orphan > > And not e.g.: > > git worktree add --orphan new-branch /tmp/orphan origin/next > > Or whatever, but it's incompatible with <commit-ish>. I think this on > top should fix it up: > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 1310bfb564f..3afef985154 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -10,7 +10,9 @@ SYNOPSIS > -------- > [verse] > 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] > - [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] > + [[-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> > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 71786b72f6b..2b811630b3a 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 | --orphan] <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 \ > @@ -668,6 +671,9 @@ static int add(int ac, const char **av, const char *prefix) > if (opts.orphan_branch && !opts.checkout) > die(_("'%s' and '%s' cannot be used together"), "--orphan", > "--no-checkout"); > + if (opts.orphan_branch && 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) > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh > index 93c340f4aff..47461d02115 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -326,6 +326,10 @@ test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' > test_must_fail git worktree add --orphan poodle --no-checkout bamboo > ' > > +test_expect_success '"add" --orphan and <commit-ish> mutually exclusive' ' > + test_must_fail git worktree add --orphan poodle bamboo main > +' > + > test_expect_success '"add" -B/--detach mutually exclusive' ' > test_must_fail git worktree add -B poodle --detach bamboo main > ' Yep, you are right. I applied the patch as part of this 2/2 patch and will include it in v4. When it comes to attribution, is there a preferred way to handle this? > > > - if (ac < 2 && !new_branch && !opts.detach) { > > + /* > > + * As the orphan cannot be created until the contents of branch > > + * are staged, opts.orphan_branch should be treated as both a boolean > > + * indicating that `--orphan` was selected and as the name of the new > > + * orphan branch from this point on. > > + */ > > I've re-read this a couple of times, and I honestly still don't see what > point is trying to drive home. > > So, "--orphan" is an OPT_STRING(), so it always has a value: > > $ ./git worktree add --orphan > error: option `orphan' requires a value > > But we init it to NULL, and above we just used it as a boolean *and* > below. > > I.e. this comment would seem to me to fit with code where the reader > might be surprised that we're using "opts.orphan_branch" as a string > from then on, but we're just copying that to "new_branch", then we > always use "opts.orphan_branch" as a boolean for the rest of the > function. > > I may be missing something, but I think this would probably be better > just without this comment. E.g. we use "--track", "--lock-reason" > etc. in similar ways, and those don't have a comment like that. > Originally the new orphan branch's name was passed into `add_worktree(path, refname, opts)` via the `orphan_branch` field in `opts` and the branch which was to be checked out first(to mimic `git checkout --orphan`) was passed in via `refname`. Now that the behavior was changed to use `git switch`, that "checkout then make orphan" behavior was unneeded and `refname` also contains the name of the orphan branch. For `make_worktree_orphan(opts, child_env)` however since I used the same function signature as `checkout_worktree(opts, child_env)`, `refname` wasn't passed in and I used `opts->orphan_branch` to access the branch name from that scope. I can change `make_worktree_orphan(opts, child_env)` to `make_worktree_orphan(ref, opts, child_env)` instead and then `orphan_branch` would be able to be treated as a boolean like those other flags. > > > + if (opts.orphan_branch) { > > + new_branch = opts.orphan_branch; > > + } > > + > > + if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) { > > In general we shouldn't combine random "if"'s just because a a > sufficiently smart compiler could discover a way to reduce work. > > But in this case these seem to be inherently connected, we always want > the not-DWIM behavior with "orphan", no? > > So shouldn't this just be: > > if (opts.orphan_branch) { > ... > } else if (ac < 2 && !new_branch && !opts.detach) { > .... > } > > ? Yes. I've updated that for v4.
On 22/11/15 11:35PM, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Nov 15 2022, Eric Sunshine wrote: > > > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> On Thu, Nov 10 2022, Jacob Abel wrote: > >> > Adds support for creating an orphan branch when adding a new worktree. > >> > This functionality is equivalent to git switch's --orphan flag. > >> > > >> > The original reason this feature was implemented was to allow a user > >> > to initialise a new repository using solely the worktree oriented > >> > workflow. Example usage included below. > >> > > >> > $ GIT_DIR=".git" git init --bare > >> > $ git worktree add --orphan master master/ > >> > > >> > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> > >> > --- > >> > +Create a worktree containing an orphan branch named `<branch>` with a > >> > +clean working directory. See `--orphan` in linkgit:git-switch[1] for > >> > +more details. > >> > >> Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent > >> in general to avoid linking to it in lieu of "git checkout". > >> > >> In this case in particular though the "more details" are almost > >> completely absent from the "git-switch" docs, and they don't (which is > >> their won flaw) link to the more detailed "git-checkout" docs. > >> > >> But for this patch, it seems much better to link to the "checkout" docs, > >> no? > > > > Sorry, no. The important point here is that the --orphan option being > > added to `git worktree add` closely follows the behavior of `git > > switch --orphan`, which is quite different from the behavior of `git > > checkout --orphan`. > > > > The `git switch --orphan` documentation doesn't seem particularly > > lacking; it correctly describes the (very) simplified behavior of that > > command over `git checkout --orphan`. I might agree that there isn't > > much reason to link to git-switch for "more details", though, since > > there isn't really anything else that needs to be said. > > Aside from what it says now: 1/2 of what I'm saying is that linking to > it while it says it's "EXPERIMENTAL" might be either jumping the gun. > > Or maybe we should just declare it non-"EXPERIMENTAL", but in any case > this unrelated topic might want to avoid that altogether and just link > to the "checkout" version. > > A quick grep of our docs (for linkgit:git-switch) that this would be the > first mention outside of user-manual.txt where we link to it when it's > not in the context of "checkout or switch", or where we're explaining > something switch-specific (i.e. the "suggestDetachingHead" advice). > > Having said that I don't really care, just a suggestion... > > > If we did want to say something else here, we might copy one sentence > > from the `git checkout --orphan` documentation: > > > > The first commit made on this new branch will have no parents and > > it will be the root of a new history totally disconnected from all > > the other branches and commits. > > > > The same sentence could be added to `git switch --orphan` > > documentation, but that's outside the scope of this patch series (thus > > can be done later by someone). > > I think I was partially confused by skimming the SYNOPSIS and thinking > this supported <start-point> like checkout, which as I found in > https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ > just seems to be a missing assertion where we want to die() if that's > provided in this mode. > > What I also found a bit confusing (but maybe it's just me) is that the > "with a clean working directory" seemed at first to be drawing a > distinction between this behavior and that of "git switch", but from > poking at it some more it seems to be expressing "this is like git > switch's --orphan" with that. > > I think instead of "clean working tree" it would be better to talk about > "tracked files", as "git switch --orphan" does, which AFAICT is what it > means. But then again the reason "switch" does that is because you have > *existing* tracked files, which inherently doesn't apply for "worktree". > > Hrm. > > So, I guess it depends on your mental model of this operation, but at > least I think it's more intuitive to explain it in terms of "git > checkout --orphan", not "git switch --orphan". I.e.: > > Create a worktree containing an orphan branch named > `<branch>`. This works like linkgit:git-checkout[1]'s `--orphan' > option, except '<start-point>` isn't supported, and the "clear > the index" doesn't apply (as "worktree add" will always have a > new index)". > > Whereas defining this in terms of git-switch's "All tracked files are > removed" might just be more confusing. What files? Since it's "worktree > add" there weren't any in the first place. > > Anyway, I don't mind it as it is, but maybe the above write-up helps for > #leftoverbits if we ever want to unify these docs. I.e. AFAICT we could: > > * Link from git-worktree to git-checkout, saying the above > * Link from git-switch to git-checkout, ditto, but that we also "remove > tracked files [of the current HEAD]". Apologies for the mistake in the SYNOPSIS. As mentioned in the other replies I've updated it as you indicated to correct that. As for a path forwards on the referencing of either git-checkout or git-switch from git-worktree, I think I'm leaning towards Eric's approach (in his reply to this message) where we don't reference either and fully outline the behavior itself. > > >> > +test_expect_success '"add" --orphan/-b mutually exclusive' ' > >> > + test_must_fail git worktree add --orphan poodle -b poodle bamboo > >> > +' > >> > + > >> > +test_expect_success '"add" --orphan/-B mutually exclusive' ' > >> > + test_must_fail git worktree add --orphan poodle -B poodle bamboo > >> > +' > >> > + > >> > +test_expect_success '"add" --orphan/--detach mutually exclusive' ' > >> > + test_must_fail git worktree add --orphan poodle --detach bamboo > >> > +' > >> > + > >> > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' > >> > + test_must_fail git worktree add --orphan poodle --no-checkout bamboo > >> > +' > >> > + > >> > +test_expect_success '"add" -B/--detach mutually exclusive' ' > >> > + test_must_fail git worktree add -B poodle --detach bamboo main > >> > +' > >> > + > >> > >> This would be much better as a for-loop: > >> > >> for opt in -b -B ... > >> do > >> test_expect_success "...$opt" '<test here, uses $opt>' > >> done > >> > >> Note the ""-quotes for the description, and '' for the test, that's not > >> a mistake, we eval() the latter. > > > > Such a loop would need to be more complex than this, wouldn't it, to > > account for all the combinations? I'd normally agree about the loop, > > but given that it requires extra complexity, I don't really mind > > seeing the individual tests spelled out manually in this case; they're > > dead simple to understand as written. I don't feel strongly either > > way, but I also don't want to ask for extra work from the patch author > > for a subjective change. > > Yeah, it's probably not worth it. This is partially cleaning up existing > tests, but maybe: > > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh > index 93c340f4aff..5acfd48f418 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -298,37 +298,21 @@ 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 > -' > - > -test_expect_success '"add" -B/--detach mutually exclusive' ' > - test_must_fail git worktree add -B poodle --detach bamboo main > -' > - > -test_expect_success '"add" --orphan/-b mutually exclusive' ' > - test_must_fail git worktree add --orphan poodle -b poodle bamboo > -' > - > -test_expect_success '"add" --orphan/-B mutually exclusive' ' > - test_must_fail git worktree add --orphan poodle -B poodle bamboo > -' > - > -test_expect_success '"add" --orphan/--detach mutually exclusive' ' > - test_must_fail git worktree add --orphan poodle --detach bamboo > -' > - > -test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' > - test_must_fail git worktree add --orphan poodle --no-checkout bamboo > -' > - > -test_expect_success '"add" -B/--detach mutually exclusive' ' > - test_must_fail git worktree add -B poodle --detach bamboo main > -' > +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_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 && > > I re-arranged that a bit, but probably not worth a loop. I *did* spot in > doing that that if I sort the options I end up with a duplicate test, > i.e. we test "-B poodle --detach bamboo main" twice. > > That seems to be added by mistake in 2/2, i.e. it's the existing test > you can see in the diff context, just added at the end. This is much clearer and more succinct. I've applied this to 2/2 for v4.
On 22/11/15 07:19PM, Eric Sunshine wrote: > On Tue, Nov 15, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > On Tue, Nov 15 2022, Eric Sunshine wrote: > > > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason > > > <avarab@gmail.com> wrote: > > >> But for this patch, it seems much better to link to the "checkout" docs, > > >> no? > > > > > > Sorry, no. The important point here is that the --orphan option being > > > added to `git worktree add` closely follows the behavior of `git > > > switch --orphan`, which is quite different from the behavior of `git > > > checkout --orphan`. > > > > > > The `git switch --orphan` documentation doesn't seem particularly > > > lacking; it correctly describes the (very) simplified behavior of that > > > command over `git checkout --orphan`. I might agree that there isn't > > > much reason to link to git-switch for "more details", though, since > > > there isn't really anything else that needs to be said. > > > > Aside from what it says now: 1/2 of what I'm saying is that linking to > > it while it says it's "EXPERIMENTAL" might be either jumping the gun. > > > > Or maybe we should just declare it non-"EXPERIMENTAL", but in any case > > this unrelated topic might want to avoid that altogether and just link > > to the "checkout" version. > > Even better would be for the documentation added by this patch to be > self-contained and not bother linking anywhere to further explain > --orphan. That would satisfy your concern, I think, as well as my > concern that `git checkout --orphan` documentation is inappropriate > for `git worktree add --orphan`. > > > > If we did want to say something else here, we might copy one sentence > > > from the `git checkout --orphan` documentation: > > > > > > The first commit made on this new branch will have no parents and > > > it will be the root of a new history totally disconnected from all > > > the other branches and commits. > > > > > > The same sentence could be added to `git switch --orphan` > > > documentation, but that's outside the scope of this patch series (thus > > > can be done later by someone). > > > > I think I was partially confused by skimming the SYNOPSIS and thinking > > this supported <start-point> like checkout, which as I found in > > https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ > > just seems to be a missing assertion where we want to die() if that's > > provided in this mode. > > I haven't read v3 yet, so I wasn't aware that the SYNOPSIS hadn't been > updated to match the reworked --orphan behavior implemented by v3, but > I can certainly understand how that would have led you astray. You're > quite correct that the SYNOPSIS should not be saying that <commit-ish> > is allowed with --orphan. > > > What I also found a bit confusing (but maybe it's just me) is that the > > "with a clean working directory" seemed at first to be drawing a > > distinction between this behavior and that of "git switch", but from > > poking at it some more it seems to be expressing "this is like git > > switch's --orphan" with that. > > "clean working directory" may indeed be ambiguous and confusing. It's > not necessarily clear if it means "no changes to tracked files" or "no > files in directory". We should use more precise terminology. > > > I think instead of "clean working tree" it would be better to talk about > > "tracked files", as "git switch --orphan" does, which AFAICT is what it > > means. But then again the reason "switch" does that is because you have > > *existing* tracked files, which inherently doesn't apply for "worktree". > > > > Hrm. > > > > So, I guess it depends on your mental model of this operation, but at > > least I think it's more intuitive to explain it in terms of "git > > checkout --orphan", not "git switch --orphan". I.e.: > > > > Create a worktree containing an orphan branch named > > `<branch>`. This works like linkgit:git-checkout[1]'s `--orphan' > > option, except '<start-point>` isn't supported, and the "clear > > the index" doesn't apply (as "worktree add" will always have a > > new index)". > > > > Whereas defining this in terms of git-switch's "All tracked files are > > removed" might just be more confusing. What files? Since it's "worktree > > add" there weren't any in the first place. > > I would find it clearer not to talk about or reference `git checkout > --orphan` at all. And, as mentioned above, it shouldn't need to > reference `git switch --orphan` either. How about something like this > for the description of the `add` subcommand? > > Create a worktree containing no files and 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. > > And then to document the --orphan command: > > With `add`, make the new worktree and index empty, and associate > the worktree with a new orphan branch named `<new-branch>`. I really like this approach. My original intent was that by referencing git-checkout, users could check the source documentation for the underlying command. Since we now call neither `git checkout` or `git switch`, just documenting the behavior outright seems like the best course of action.
On Sat, Nov 19 2022, Jacob Abel wrote: > On 22/11/15 11:35PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Tue, Nov 15 2022, Eric Sunshine wrote: >> >> > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason >> > <avarab@gmail.com> wrote: >> >> On Thu, Nov 10 2022, Jacob Abel wrote: >> >> > Adds support for creating an orphan branch when adding a new worktree. >> >> > This functionality is equivalent to git switch's --orphan flag. >> >> > >> >> > The original reason this feature was implemented was to allow a user >> >> > to initialise a new repository using solely the worktree oriented >> >> > workflow. Example usage included below. >> >> > >> >> > $ GIT_DIR=".git" git init --bare >> >> > $ git worktree add --orphan master master/ >> >> > >> >> > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> >> >> > --- >> >> > +Create a worktree containing an orphan branch named `<branch>` with a >> >> > +clean working directory. See `--orphan` in linkgit:git-switch[1] for >> >> > +more details. >> >> >> >> Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent >> >> in general to avoid linking to it in lieu of "git checkout". >> >> >> >> In this case in particular though the "more details" are almost >> >> completely absent from the "git-switch" docs, and they don't (which is >> >> their won flaw) link to the more detailed "git-checkout" docs. >> >> >> >> But for this patch, it seems much better to link to the "checkout" docs, >> >> no? >> > >> > Sorry, no. The important point here is that the --orphan option being >> > added to `git worktree add` closely follows the behavior of `git >> > switch --orphan`, which is quite different from the behavior of `git >> > checkout --orphan`. >> > >> > The `git switch --orphan` documentation doesn't seem particularly >> > lacking; it correctly describes the (very) simplified behavior of that >> > command over `git checkout --orphan`. I might agree that there isn't >> > much reason to link to git-switch for "more details", though, since >> > there isn't really anything else that needs to be said. >> >> Aside from what it says now: 1/2 of what I'm saying is that linking to >> it while it says it's "EXPERIMENTAL" might be either jumping the gun. >> >> Or maybe we should just declare it non-"EXPERIMENTAL", but in any case >> this unrelated topic might want to avoid that altogether and just link >> to the "checkout" version. >> >> A quick grep of our docs (for linkgit:git-switch) that this would be the >> first mention outside of user-manual.txt where we link to it when it's >> not in the context of "checkout or switch", or where we're explaining >> something switch-specific (i.e. the "suggestDetachingHead" advice). >> >> Having said that I don't really care, just a suggestion... >> >> > If we did want to say something else here, we might copy one sentence >> > from the `git checkout --orphan` documentation: >> > >> > The first commit made on this new branch will have no parents and >> > it will be the root of a new history totally disconnected from all >> > the other branches and commits. >> > >> > The same sentence could be added to `git switch --orphan` >> > documentation, but that's outside the scope of this patch series (thus >> > can be done later by someone). >> >> I think I was partially confused by skimming the SYNOPSIS and thinking >> this supported <start-point> like checkout, which as I found in >> https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ >> just seems to be a missing assertion where we want to die() if that's >> provided in this mode. >> >> What I also found a bit confusing (but maybe it's just me) is that the >> "with a clean working directory" seemed at first to be drawing a >> distinction between this behavior and that of "git switch", but from >> poking at it some more it seems to be expressing "this is like git >> switch's --orphan" with that. >> >> I think instead of "clean working tree" it would be better to talk about >> "tracked files", as "git switch --orphan" does, which AFAICT is what it >> means. But then again the reason "switch" does that is because you have >> *existing* tracked files, which inherently doesn't apply for "worktree". >> >> Hrm. >> >> So, I guess it depends on your mental model of this operation, but at >> least I think it's more intuitive to explain it in terms of "git >> checkout --orphan", not "git switch --orphan". I.e.: >> >> Create a worktree containing an orphan branch named >> `<branch>`. This works like linkgit:git-checkout[1]'s `--orphan' >> option, except '<start-point>` isn't supported, and the "clear >> the index" doesn't apply (as "worktree add" will always have a >> new index)". >> >> Whereas defining this in terms of git-switch's "All tracked files are >> removed" might just be more confusing. What files? Since it's "worktree >> add" there weren't any in the first place. >> >> Anyway, I don't mind it as it is, but maybe the above write-up helps for >> #leftoverbits if we ever want to unify these docs. I.e. AFAICT we could: >> >> * Link from git-worktree to git-checkout, saying the above >> * Link from git-switch to git-checkout, ditto, but that we also "remove >> tracked files [of the current HEAD]". > > Apologies for the mistake in the SYNOPSIS. As mentioned in the other replies > I've updated it as you indicated to correct that. > > As for a path forwards on the referencing of either git-checkout or git-switch > from git-worktree, I think I'm leaning towards Eric's approach (in his reply > to this message) where we don't reference either and fully outline the > behavior itself. Yeah, that makes sense. >> >> >> > +test_expect_success '"add" --orphan/-b mutually exclusive' ' >> >> > + test_must_fail git worktree add --orphan poodle -b poodle bamboo >> >> > +' >> >> > + >> >> > +test_expect_success '"add" --orphan/-B mutually exclusive' ' >> >> > + test_must_fail git worktree add --orphan poodle -B poodle bamboo >> >> > +' >> >> > + >> >> > +test_expect_success '"add" --orphan/--detach mutually exclusive' ' >> >> > + test_must_fail git worktree add --orphan poodle --detach bamboo >> >> > +' >> >> > + >> >> > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' >> >> > + test_must_fail git worktree add --orphan poodle --no-checkout bamboo >> >> > +' >> >> > + >> >> > +test_expect_success '"add" -B/--detach mutually exclusive' ' >> >> > + test_must_fail git worktree add -B poodle --detach bamboo main >> >> > +' >> >> > + >> >> >> >> This would be much better as a for-loop: >> >> >> >> for opt in -b -B ... >> >> do >> >> test_expect_success "...$opt" '<test here, uses $opt>' >> >> done >> >> >> >> Note the ""-quotes for the description, and '' for the test, that's not >> >> a mistake, we eval() the latter. >> > >> > Such a loop would need to be more complex than this, wouldn't it, to >> > account for all the combinations? I'd normally agree about the loop, >> > but given that it requires extra complexity, I don't really mind >> > seeing the individual tests spelled out manually in this case; they're >> > dead simple to understand as written. I don't feel strongly either >> > way, but I also don't want to ask for extra work from the patch author >> > for a subjective change. >> >> Yeah, it's probably not worth it. This is partially cleaning up existing >> tests, but maybe: >> >> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh >> index 93c340f4aff..5acfd48f418 100755 >> --- a/t/t2400-worktree-add.sh >> +++ b/t/t2400-worktree-add.sh >> @@ -298,37 +298,21 @@ 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 >> -' >> - >> -test_expect_success '"add" -B/--detach mutually exclusive' ' >> - test_must_fail git worktree add -B poodle --detach bamboo main >> -' >> - >> -test_expect_success '"add" --orphan/-b mutually exclusive' ' >> - test_must_fail git worktree add --orphan poodle -b poodle bamboo >> -' >> - >> -test_expect_success '"add" --orphan/-B mutually exclusive' ' >> - test_must_fail git worktree add --orphan poodle -B poodle bamboo >> -' >> - >> -test_expect_success '"add" --orphan/--detach mutually exclusive' ' >> - test_must_fail git worktree add --orphan poodle --detach bamboo >> -' >> - >> -test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' >> - test_must_fail git worktree add --orphan poodle --no-checkout bamboo >> -' >> - >> -test_expect_success '"add" -B/--detach mutually exclusive' ' >> - test_must_fail git worktree add -B poodle --detach bamboo main >> -' >> +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_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 && >> >> I re-arranged that a bit, but probably not worth a loop. I *did* spot in >> doing that that if I sort the options I end up with a duplicate test, >> i.e. we test "-B poodle --detach bamboo main" twice. >> >> That seems to be added by mistake in 2/2, i.e. it's the existing test >> you can see in the diff context, just added at the end. > > This is much clearer and more succinct. I've applied this to 2/2 for v4. Great, nice that it helped!
On Sat, Nov 19 2022, Jacob Abel wrote: > On 22/11/15 11:09PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Thu, Nov 10 2022, Jacob Abel wrote: >> >> So, on a second read-through... >> >> > 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] >> > - [[-b | -B] <new-branch>] <path> [<commit-ish>] >> > + [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] >> >> This synopsis is now at least partially wrong, and .... >> >> > +--orphan <new-branch>:: >> > + With `add`, create a new orphan branch named `<new-branch>` in the new >> > + worktree. See `--orphan` in linkgit:git-switch[1] for details. >> > + >> > --porcelain:: >> > .... >> > #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 | --orphan] <new-branch>] <path> [<commit-ish>]") >> >> >> ...here we say the same, but surely it's only: >> >> git worktree add --orphan new-branch /tmp/orphan >> >> And not e.g.: >> >> git worktree add --orphan new-branch /tmp/orphan origin/next >> >> Or whatever, but it's incompatible with <commit-ish>. I think this on >> top should fix it up: >> >> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt >> index 1310bfb564f..3afef985154 100644 >> --- a/Documentation/git-worktree.txt >> +++ b/Documentation/git-worktree.txt >> @@ -10,7 +10,9 @@ SYNOPSIS >> -------- >> [verse] >> 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] >> - [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] >> + [[-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> >> diff --git a/builtin/worktree.c b/builtin/worktree.c >> index 71786b72f6b..2b811630b3a 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 | --orphan] <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 \ >> @@ -668,6 +671,9 @@ static int add(int ac, const char **av, const char *prefix) >> if (opts.orphan_branch && !opts.checkout) >> die(_("'%s' and '%s' cannot be used together"), "--orphan", >> "--no-checkout"); >> + if (opts.orphan_branch && 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) >> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh >> index 93c340f4aff..47461d02115 100755 >> --- a/t/t2400-worktree-add.sh >> +++ b/t/t2400-worktree-add.sh >> @@ -326,6 +326,10 @@ test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' >> test_must_fail git worktree add --orphan poodle --no-checkout bamboo >> ' >> >> +test_expect_success '"add" --orphan and <commit-ish> mutually exclusive' ' >> + test_must_fail git worktree add --orphan poodle bamboo main >> +' >> + >> test_expect_success '"add" -B/--detach mutually exclusive' ' >> test_must_fail git worktree add -B poodle --detach bamboo main >> ' > > Yep, you are right. I applied the patch as part of this 2/2 patch and will > include it in v4. When it comes to attribution, is there a preferred way to > handle this? Feel free to add my: Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> I'm also fine with no attribution, but we add that for copyright reasons, and this is *probably* significant enough to qualify, but I'm no lawyer etc. Anyway, probably better to add it when in doubt... >> >> > - if (ac < 2 && !new_branch && !opts.detach) { >> > + /* >> > + * As the orphan cannot be created until the contents of branch >> > + * are staged, opts.orphan_branch should be treated as both a boolean >> > + * indicating that `--orphan` was selected and as the name of the new >> > + * orphan branch from this point on. >> > + */ >> >> I've re-read this a couple of times, and I honestly still don't see what >> point is trying to drive home. >> >> So, "--orphan" is an OPT_STRING(), so it always has a value: >> >> $ ./git worktree add --orphan >> error: option `orphan' requires a value >> >> But we init it to NULL, and above we just used it as a boolean *and* >> below. >> >> I.e. this comment would seem to me to fit with code where the reader >> might be surprised that we're using "opts.orphan_branch" as a string >> from then on, but we're just copying that to "new_branch", then we >> always use "opts.orphan_branch" as a boolean for the rest of the >> function. >> >> I may be missing something, but I think this would probably be better >> just without this comment. E.g. we use "--track", "--lock-reason" >> etc. in similar ways, and those don't have a comment like that. >> > > Originally the new orphan branch's name was passed into > `add_worktree(path, refname, opts)` via the `orphan_branch` field in `opts` and > the branch which was to be checked out first(to mimic `git checkout --orphan`) > was passed in via `refname`. > > Now that the behavior was changed to use `git switch`, that > "checkout then make orphan" behavior was unneeded and `refname` also contains > the name of the orphan branch. > > For `make_worktree_orphan(opts, child_env)` however since I used the same > function signature as `checkout_worktree(opts, child_env)`, `refname` wasn't > passed in and I used `opts->orphan_branch` to access the branch name from > that scope. > > I can change `make_worktree_orphan(opts, child_env)` to > `make_worktree_orphan(ref, opts, child_env)` instead and then `orphan_branch` > would be able to be treated as a boolean like those other flags. I think nothing needs to be changed here on my account, just pointing out that I found the comment a bit confusing. Do with that what you will :) >> >> > + if (opts.orphan_branch) { >> > + new_branch = opts.orphan_branch; >> > + } >> > + >> > + if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) { >> >> In general we shouldn't combine random "if"'s just because a a >> sufficiently smart compiler could discover a way to reduce work. >> >> But in this case these seem to be inherently connected, we always want >> the not-DWIM behavior with "orphan", no? >> >> So shouldn't this just be: >> >> if (opts.orphan_branch) { >> ... >> } else if (ac < 2 && !new_branch && !opts.detach) { >> .... >> } >> >> ? > > Yes. I've updated that for v4. Nice!
On Fri, Nov 18, 2022 at 8:44 PM Jacob Abel <jacobabel@nullpo.dev> wrote: > On 22/11/15 10:08PM, Ævar Arnfjörð Bjarmason wrote: > > > + test_must_fail git worktree add --orphan existingbranch orphandir2 && > > > + test ! -d orphandir2 > > > > I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants > > to be a "test_path_is_missing"? > > > > In general we try to test what a thing is, not what it isn't, in this > > case don't we fail to create the dir entirely? So "not exists" would > > cover it? > > Ah yes that would be preferable. I've updated it for v4. > > This shows up in the file in a few other places in this file as well > (from before this patch). Should I make the changes there as well and put > those changes into an additional patch in this patchset? With my reviewer's hat on, my goal is to help the series land in Junio's tree, which means I'd like to see fewer changes with each iteration. Adding a new patch which is only tangentially related to what the series wants to achieve isn't a priority, and could end up delaying acceptance of the series if problems in the new patch end up requiring additional rerolls. So, yes, you could do that cleanup as a preparatory patch in the series if you want to tackle it. It would be an appropriate cleanup since you're working on code nearby. Or it could be done as a follow up to this series. Given how small the cleanup patch would likely be, it may not make a difference one way or the other, especially if the commit message explains the change well (for instance, by paraphrasing what Ævar said about "testing what a thing is, not what it isn't").
On 22/11/22 01:00AM, Eric Sunshine wrote: > On Fri, Nov 18, 2022 at 8:44 PM Jacob Abel <jacobabel@nullpo.dev> wrote: > > On 22/11/15 10:08PM, Ævar Arnfjörð Bjarmason wrote: > > > > + test_must_fail git worktree add --orphan existingbranch orphandir2 && > > > > + test ! -d orphandir2 > > > > > > I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants > > > to be a "test_path_is_missing"? > > > > > > In general we try to test what a thing is, not what it isn't, in this > > > case don't we fail to create the dir entirely? So "not exists" would > > > cover it? > > > > Ah yes that would be preferable. I've updated it for v4. > > > > This shows up in the file in a few other places in this file as well > > (from before this patch). Should I make the changes there as well and put > > those changes into an additional patch in this patchset? > > With my reviewer's hat on, my goal is to help the series land in > Junio's tree, which means I'd like to see fewer changes with each > iteration. Adding a new patch which is only tangentially related to > what the series wants to achieve isn't a priority, and could end up > delaying acceptance of the series if problems in the new patch end up > requiring additional rerolls. > > So, yes, you could do that cleanup as a preparatory patch in the > series if you want to tackle it. It would be an appropriate cleanup > since you're working on code nearby. Or it could be done as a follow > up to this series. Given how small the cleanup patch would likely be, > it may not make a difference one way or the other, especially if the > commit message explains the change well (for instance, by paraphrasing > what Ævar said about "testing what a thing is, not what it isn't"). In that case I'll make a note and send in a cleanup patch with that change (referencing this thread) some time down the road after this series.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 4dd658012b..1310bfb564 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] - [[-b | -B] <new-branch>] <path> [<commit-ish>] + [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>] 'git worktree list' [-v | --porcelain [-z]] 'git worktree lock' [--reason <string>] <worktree> 'git worktree move' <worktree> <new-path> @@ -95,6 +95,14 @@ 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 an orphan branch named `<branch>` with a +clean working directory. See `--orphan` in linkgit:git-switch[1] for +more details. list:: @@ -222,6 +230,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`, create a new orphan branch named `<new-branch>` in the new + worktree. See `--orphan` in linkgit:git-switch[1] for details. + --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..71786b72f6 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -17,7 +17,7 @@ #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 | --orphan] <new-branch>] <path> [<commit-ish>]") #define BUILTIN_WORKTREE_LIST_USAGE \ N_("git worktree list [-v | --porcelain [-z]]") #define BUILTIN_WORKTREE_LOCK_USAGE \ @@ -90,6 +90,7 @@ struct add_opts { int detach; int quiet; int checkout; + const char *orphan_branch; const char *keep_locked; }; @@ -364,6 +365,24 @@ static int checkout_worktree(const struct add_opts *opts, return run_command(&cp); } +static int make_worktree_orphan(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(opts->orphan_branch, &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,7 +412,7 @@ 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_branch) die(_("invalid reference: %s"), refname); name = worktree_basename(path, &len); @@ -482,10 +501,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 +516,10 @@ static int add_worktree(const char *path, const char *refname, if (ret) goto done; + if (opts->orphan_branch && + (ret = make_worktree_orphan(opts, &child_env))) + goto done; + if (opts->checkout && (ret = checkout_worktree(opts, &child_env))) goto done; @@ -516,7 +539,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_branch) { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL); @@ -616,6 +639,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", &opts.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")), @@ -635,6 +660,14 @@ static int add(int ac, const char **av, const char *prefix) ac = parse_options(ac, av, prefix, options, git_worktree_add_usage, 0); 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 + !!new_branch + !!new_branch_force + !!opts.orphan_branch > 1) + die(_("options '%s', '%s', '%s', and '%s' cannot be used together"), + "-b", "-B", "--orphan", "--detach"); + if (opts.orphan_branch && opt_track) + die(_("'%s' and '%s' cannot be used together"), "--orphan", "--track"); + if (opts.orphan_branch && !opts.checkout) + die(_("'%s' and '%s' cannot be used together"), "--orphan", + "--no-checkout"); if (lock_reason && !keep_locked) die(_("the option '%s' requires '%s'"), "--reason", "--lock"); if (lock_reason) @@ -651,6 +684,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 +703,17 @@ static int add(int ac, const char **av, const char *prefix) strbuf_release(&symref); } - if (ac < 2 && !new_branch && !opts.detach) { + /* + * As the orphan cannot be created until the contents of branch + * are staged, opts.orphan_branch should be treated as both a boolean + * indicating that `--orphan` was selected and as the name of the new + * orphan branch from this point on. + */ + if (opts.orphan_branch) { + new_branch = opts.orphan_branch; + } + + if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) { const char *s = dwim_branch(path, &new_branch); if (s) branch = s; @@ -686,7 +736,7 @@ 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 (new_branch && !opts.orphan_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..93c340f4af 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -310,6 +310,26 @@ test_expect_success '"add" -B/--detach mutually exclusive' ' test_must_fail git worktree add -B poodle --detach bamboo main ' +test_expect_success '"add" --orphan/-b mutually exclusive' ' + test_must_fail git worktree add --orphan poodle -b poodle bamboo +' + +test_expect_success '"add" --orphan/-B mutually exclusive' ' + test_must_fail git worktree add --orphan poodle -B poodle bamboo +' + +test_expect_success '"add" --orphan/--detach mutually exclusive' ' + test_must_fail git worktree add --orphan poodle --detach bamboo +' + +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' ' + test_must_fail git worktree add --orphan poodle --no-checkout bamboo +' + +test_expect_success '"add" -B/--detach mutually exclusive' ' + test_must_fail git worktree add -B poodle --detach bamboo main +' + test_expect_success '"add -B" fails if the branch is checked out' ' git rev-parse newmain >before && test_must_fail git worktree add -B newmain bamboo main && @@ -330,6 +350,31 @@ 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 ! -d 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 'local clone from linked checkout' ' git clone --local here here-clone && ( cd here-clone && git fsck )
Adds support for creating an orphan branch when adding a new worktree. This functionality is equivalent to git switch's --orphan flag. The original reason this feature was implemented was to allow a user to initialise a new repository using solely the worktree oriented workflow. Example usage included below. $ GIT_DIR=".git" git init --bare $ git worktree add --orphan master master/ Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> --- Documentation/git-worktree.txt | 14 +++++++- builtin/worktree.c | 64 ++++++++++++++++++++++++++++++---- t/t2400-worktree-add.sh | 45 ++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 8 deletions(-) -- 2.37.4