git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees
@ 2022-11-04  1:02 Jacob Abel
  2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
                   ` (5 more replies)
  0 siblings, 6 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  1:02 UTC (permalink / raw)
  To: git; +Cc: Jacob Abel

While working with the worktree based git workflow, I realised that setting
up a new git repository required switching between the traditional and
worktree based workflows. Searching online I found a SO answer [1] which
seemed to support this and which indicated that adding support for this should
not be technically difficult.

Note: This is my first patchset for git so please let me know if there's anything
I should do differently to improve contributions in the future.

This patchset has three parts:

  * adding `-B` to the usage docs (noticed during dev and it seemed too small
    to justify a separate submission)
  * switching from `git reset --hard` to `git checkout` for worktree checkout
  * adding orphan branch functionality (as is present in `git-checkout`)
    to `git-worktree-add`

1. https://stackoverflow.com/a/68717229/15064705

Jacob Abel (4):
  worktree add: Include -B in usage docs
  builtin/worktree.c: Update checkout_worktree() to use git-worktree
  worktree add: add --orphan flag
  worktree add: Add unit tests for --orphan

 Documentation/git-worktree.txt | 18 ++++++++-
 builtin/worktree.c             | 72 +++++++++++++++++++++++++---------
 t/t2400-worktree-add.sh        | 54 +++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 20 deletions(-)


base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193
--
2.37.4



^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH 1/4] worktree add: Include -B in usage docs
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
@ 2022-11-04  1:03 ` Jacob Abel
  2022-11-04  3:05   ` Eric Sunshine
  2022-11-04  1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  1:03 UTC (permalink / raw)
  To: git; +Cc: Jacob Abel

While -B behavior is already documented, it was not included in the
usage docs for either the man page or the help text. This change fixes
that and brings the usage docs in line with how the flags are documented
in other commands such as git checkout.

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 Documentation/git-worktree.txt | 2 +-
 builtin/worktree.c             | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 063d6eeb99..4dd658012b 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 <new-branch>] <path> [<commit-ish>]
+		   [[-b | -B] <new-branch>] <path> [<commit-ish>]
 '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 4a24d53be1..b3373fbbd6 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -15,9 +15,9 @@
 #include "worktree.h"
 #include "quote.h"

-#define BUILTIN_WORKTREE_ADD_USAGE \
+#define BUILTIN_WORKTREE_ADD_USAGE                                                        \
 	N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \
-	   "                 [-b <new-branch>] <path> [<commit-ish>]")
+	   "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")
 #define BUILTIN_WORKTREE_LIST_USAGE \
 	N_("git worktree list [-v | --porcelain [-z]]")
 #define BUILTIN_WORKTREE_LOCK_USAGE \
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
@ 2022-11-04  1:03 ` Jacob Abel
  2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
  2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  1:03 UTC (permalink / raw)
  To: git; +Cc: Jacob Abel

Updates the function to call `git checkout` instead of
`git reset --hard` to simplify adding orphan support.

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b3373fbbd6..d40f771848 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -357,7 +357,7 @@ static int checkout_worktree(const struct add_opts *opts,
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
+	strvec_pushl(&cp.args, "checkout", "--no-recurse-submodules", NULL);
 	if (opts->quiet)
 		strvec_push(&cp.args, "--quiet");
 	strvec_pushv(&cp.env, child_env->v);
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
  2022-11-04  1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
@ 2022-11-04  1:03 ` Jacob Abel
  2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
  2022-11-04  5:03   ` Eric Sunshine
  2022-11-04  1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  1:03 UTC (permalink / raw)
  To: git; +Cc: Jacob Abel

Adds support for creating an orphan branch when adding a new worktree.
This functionality is equivalent to git checkout'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 | 18 ++++++++-
 builtin/worktree.c             | 68 +++++++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 4dd658012b..92bd75564f 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,17 @@ 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> [<commit-ish>]
+------------
++
+Create a worktree containing an orphan branch named `<branch>` based
+on `<commit-ish>`. If `<commit-ish>` is not specified, the new orphan branch
+will be created based on `HEAD`.
++
+Note that unlike with `-b` or `-B`, this operation will succeed even if
+`<commit-ish>` is a branch that is currently checked out somewhere else.

 list::

@@ -222,6 +233,11 @@ 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 based on `<commit-ish>`. If `<commit-ish>` is omitted, it
+	defaults to `HEAD`.
+
 --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 d40f771848..70f319a6b5 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,8 @@ struct add_opts {
 	int detach;
 	int quiet;
 	int checkout;
+	int implicit;
+	const char *orphan_branch;
 	const char *keep_locked;
 };

@@ -360,6 +362,8 @@ static int checkout_worktree(const struct add_opts *opts,
 	strvec_pushl(&cp.args, "checkout", "--no-recurse-submodules", NULL);
 	if (opts->quiet)
 		strvec_push(&cp.args, "--quiet");
+	if (opts->orphan_branch)
+		strvec_pushl(&cp.args, "--orphan", opts->orphan_branch, NULL);
 	strvec_pushv(&cp.env, child_env->v);
 	return run_command(&cp);
 }
@@ -393,7 +397,8 @@ 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->implicit)
 		die(_("invalid reference: %s"), refname);

 	name = worktree_basename(path, &len);
@@ -482,10 +487,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)
@@ -516,7 +521,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);
@@ -608,33 +613,52 @@ static int add(int ac, const char **av, const char *prefix)
 	const char *opt_track = NULL;
 	const char *lock_reason = NULL;
 	int keep_locked = 0;
+
 	struct option options[] = {
-		OPT__FORCE(&opts.force,
-			   N_("checkout <branch> even if already checked out in other worktree"),
-			   PARSE_OPT_NOCOMPLETE),
+		OPT__FORCE(
+			&opts.force,
+			N_("checkout <branch> even if already checked out in other worktree"),
+			PARSE_OPT_NOCOMPLETE),
 		OPT_STRING('b', NULL, &new_branch, N_("branch"),
 			   N_("create a new branch")),
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a 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")),
+		OPT_STRING(0, "orphan", &opts.orphan_branch, N_("branch"),
+			   N_("create a 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")),
 		OPT_STRING(0, "reason", &lock_reason, N_("string"),
 			   N_("reason for locking")),
 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		OPT_PASSTHRU(0, "track", &opt_track, NULL,
 			     N_("set up tracking mode (see git-branch(1))"),
 			     PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
-		OPT_BOOL(0, "guess-remote", &guess_remote,
-			 N_("try to match the new branch name with a remote-tracking branch")),
+		OPT_BOOL(
+			0, "guess-remote", &guess_remote,
+			N_("try to match the new branch name with a remote-tracking branch")),
 		OPT_END()
 	};

 	memset(&opts, 0, sizeof(opts));
 	opts.checkout = 1;
 	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");
+
+	opts.implicit = ac < 2;
+
+	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' cannot be used with '%s'"), "--orphan", "--track");
+	if (opts.orphan_branch && !opts.checkout)
+		die(_("'%s' cannot be used with '%s'"), "--orphan",
+		    "--no-checkout");
 	if (lock_reason && !keep_locked)
 		die(_("the option '%s' requires '%s'"), "--reason", "--lock");
 	if (lock_reason)
@@ -646,11 +670,16 @@ static int add(int ac, const char **av, const char *prefix)
 		usage_with_options(git_worktree_add_usage, options);

 	path = prefix_filename(prefix, av[0]);
-	branch = ac < 2 ? "HEAD" : av[1];
+	branch = opts.implicit ? "HEAD" : av[1];

 	if (!strcmp(branch, "-"))
 		branch = "@{-1}";

+	/*
+	 * From here on, new_branch will contain the branch to be checked out,
+	 * and new_branch_force and opts.orphan_branch will tell us which one of
+	 * -b/-B/--orphan is being used.
+	 */
 	if (new_branch_force) {
 		struct strbuf symref = STRBUF_INIT;

@@ -663,6 +692,11 @@ static int add(int ac, const char **av, const char *prefix)
 		strbuf_release(&symref);
 	}

+	if (opts.orphan_branch) {
+		new_branch = opts.orphan_branch;
+		opts.force = 1;
+	}
+
 	if (ac < 2 && !new_branch && !opts.detach) {
 		const char *s = dwim_branch(path, &new_branch);
 		if (s)
@@ -686,7 +720,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");
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 4/4] worktree add: Add unit tests for --orphan
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
                   ` (2 preceding siblings ...)
  2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
@ 2022-11-04  1:03 ` Jacob Abel
  2022-11-04  1:37   ` Ævar Arnfjörð Bjarmason
  2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
  5 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  1:03 UTC (permalink / raw)
  To: git; +Cc: Jacob Abel

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 t/t2400-worktree-add.sh | 54 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..064e1336e2 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 main
+'
+
+test_expect_success '"add" --orphan/-B mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle -B poodle bamboo main
+'
+
+test_expect_success '"add" --orphan/--detach mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle --detach bamboo main
+'
+
+test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle --no-checkout 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" 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,40 @@ test_expect_success 'add --quiet' '
 	test_must_be_empty actual
 '

+test_expect_success '"add --orphan"' '
+	git worktree add --orphan neworphan orphandir main &&
+	git -C orphandir symbolic-ref HEAD >actual &&
+	echo refs/heads/neworphan >expected &&
+	test_cmp expected actual &&
+	(
+		cd orphandir &&
+		git diff main
+	)
+'
+
+test_expect_success '"add --orphan" fails if the branch already exists' '
+	git worktree add -b existingbranch orphandir2 main &&
+	test_must_fail git worktree add --orphan existingbranch orphandir3 main &&
+	[ ! -d orphandir3 ]
+'
+
+test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
+	test_must_fail git worktree add --orphan badcommitish orphandir4 eee2222 &&
+	[ ! -d orphandir4 ]
+'
+
+test_expect_success '"add --orphan" with empty repository' '
+	(
+		mkdir emptyorphanrepo &&
+		cd emptyorphanrepo &&
+		GIT_DIR=".git" git init --bare &&
+		git worktree add --orphan newbranch worktreedir &&
+		git -C worktreedir symbolic-ref HEAD >actual &&
+		echo refs/heads/newbranch >expected &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'local clone from linked checkout' '
 	git clone --local here here-clone &&
 	( cd here-clone && git fsck )
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree
  2022-11-04  1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
@ 2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
  2022-11-04  3:58     ` Jacob Abel
  2022-11-04 20:45     ` Taylor Blau
  0 siblings, 2 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-04  1:32 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git


On Fri, Nov 04 2022, Jacob Abel wrote:

> Updates the function to call `git checkout` instead of
> `git reset --hard` to simplify adding orphan support.
>
> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
>  builtin/worktree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index b3373fbbd6..d40f771848 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -357,7 +357,7 @@ static int checkout_worktree(const struct add_opts *opts,
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	cp.git_cmd = 1;
> -	strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
> +	strvec_pushl(&cp.args, "checkout", "--no-recurse-submodules", NULL);
>  	if (opts->quiet)
>  		strvec_push(&cp.args, "--quiet");
>  	strvec_pushv(&cp.env, child_env->v);

Won't we now start to run the post-checkout when doing this, and is that
intended?

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
@ 2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
  2022-11-04  4:11     ` Jacob Abel
  2022-11-04  5:03   ` Eric Sunshine
  1 sibling, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-04  1:33 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git


On Fri, Nov 04 2022, Jacob Abel wrote:

>  	commit = lookup_commit_reference_by_name(refname);
> -	if (!commit)
> +

Here.

> +	if (!commit && !opts->implicit)
>  		die(_("invalid reference: %s"), refname);
>
>  	name = worktree_basename(path, &len);
> @@ -482,10 +487,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);

And here we have a stray style change, in this case conforming to our
CodingGuidelines (it's agnostic on the former), but IMO better to keep
this out, or split it into a "various style stuff" commit, makes this
harder to review...

> -	else {
> +	} else {
>  		strvec_pushl(&cp.args, "symbolic-ref", "HEAD",
>  			     symref.buf, NULL);
>  		if (opts->quiet)
> @@ -516,7 +521,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);
> @@ -608,33 +613,52 @@ static int add(int ac, const char **av, const char *prefix)
>  	const char *opt_track = NULL;
>  	const char *lock_reason = NULL;
>  	int keep_locked = 0;
> +

ditto, we don't usually \n\n split up varibale decls.

>  	struct option options[] = {
> -		OPT__FORCE(&opts.force,
> -			   N_("checkout <branch> even if already checked out in other worktree"),
> -			   PARSE_OPT_NOCOMPLETE),
> +		OPT__FORCE(
> +			&opts.force,
> +			N_("checkout <branch> even if already checked out in other worktree"),
> +			PARSE_OPT_NOCOMPLETE),

This is just a stray refactoring of existing code to not-our-usual-style
(first arg is on the same line as the "(", rest aligned with "(").

>  		OPT_STRING('b', NULL, &new_branch, N_("branch"),
>  			   N_("create a new branch")),
>  		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
>  			   N_("create or reset a 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")),
> +		OPT_STRING(0, "orphan", &opts.orphan_branch, N_("branch"),
> +			   N_("create a 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")),

Ditto, these look like they're too-long in the pre-image, but please
resist re-flowing existing code while at it.

>  		OPT_STRING(0, "reason", &lock_reason, N_("string"),
>  			   N_("reason for locking")),
>  		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
>  		OPT_PASSTHRU(0, "track", &opt_track, NULL,
>  			     N_("set up tracking mode (see git-branch(1))"),
>  			     PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
> -		OPT_BOOL(0, "guess-remote", &guess_remote,
> -			 N_("try to match the new branch name with a remote-tracking branch")),
> +		OPT_BOOL(
> +			0, "guess-remote", &guess_remote,
> +			N_("try to match the new branch name with a remote-tracking branch")),

ditto.

>  		OPT_END()
>  	};
>
>  	memset(&opts, 0, sizeof(opts));
>  	opts.checkout = 1;
>  	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");
> +
> +	opts.implicit = ac < 2;
> +
> +	if (!!opts.detach + !!new_branch + !!new_branch_force +
> +		    !!opts.orphan_branch >
> +	    1)

The continued "if" is mis-indented, and that "1" is on a line of its
own...

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 4/4] worktree add: Add unit tests for --orphan
  2022-11-04  1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
@ 2022-11-04  1:37   ` Ævar Arnfjörð Bjarmason
  2022-11-04  4:17     ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-04  1:37 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git


On Fri, Nov 04 2022, Jacob Abel wrote:

We usually add tests along with the feature, so this should be squashed
into your 3/4.

> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
>  t/t2400-worktree-add.sh | 54 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index d587e0b20d..064e1336e2 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 main
> +'
> +
> +test_expect_success '"add" --orphan/-B mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle -B poodle bamboo main
> +'
> +
> +test_expect_success '"add" --orphan/--detach mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle --detach bamboo main
> +'
> +
> +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle --no-checkout 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"' '
> +	git worktree add --orphan neworphan orphandir main &&
> +	git -C orphandir symbolic-ref HEAD >actual &&
> +	echo refs/heads/neworphan >expected &&
> +	test_cmp expected actual &&

nit: we usually do:

	echo ...
	git ...
	test_cmp

So there's never doubt that the "expected" is before the command
(i.e. not impacted by it).

> +	(
> +		cd orphandir &&
> +		git diff main
> +	)

Here you can avoid the sub-shell, with:

	git -C orphandir diff main

> +'
> +
> +test_expect_success '"add --orphan" fails if the branch already exists' '
> +	git worktree add -b existingbranch orphandir2 main &&
> +	test_must_fail git worktree add --orphan existingbranch orphandir3 main &&
> +	[ ! -d orphandir3 ]

Don't use "[", use "test", but in this case use one of the "test -d",
"test -f" etc. wrappers in test-lib-functions.sh.

> +'
> +
> +test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
> +	test_must_fail git worktree add --orphan badcommitish orphandir4 eee2222 &&
> +	[ ! -d orphandir4 ]

ditto.

> +'
> +
> +test_expect_success '"add --orphan" with empty repository' '
> +	(
> +		mkdir emptyorphanrepo &&
> +		cd emptyorphanrepo &&
> +		GIT_DIR=".git" git init --bare &&
> +		git worktree add --orphan newbranch worktreedir &&
> +		git -C worktreedir symbolic-ref HEAD >actual &&
> +		echo refs/heads/newbranch >expected &&
> +		test_cmp expected actual
> +	)

Ditto we can avoid sub-shelling here, also when using sub-shells, make
the "cd" the first command in it, if you keep it the "mkdir" should go
outside of it.

But isn't this (untested, maybe I'm missing a subtlety):

	test_when_finished "rm -rf repo" &&
	GIT_DIR=.git git init --bare repo &&
	git -C worktree ...
	git -C worktree/subdir ..
	echo refs ...
	test_cmp ...

I.e. do we need to create the dir manually before "git init --bare"?

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 1/4] worktree add: Include -B in usage docs
  2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
@ 2022-11-04  3:05   ` Eric Sunshine
  2022-11-04  4:24     ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-04  3:05 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git

On Thu, Nov 3, 2022 at 9:05 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> While -B behavior is already documented, it was not included in the
> usage docs for either the man page or the help text. This change fixes
> that and brings the usage docs in line with how the flags are documented
> in other commands such as git checkout.

Thanks. Some historical context...

Omission of -B from the synopsis was intentional[1] for the sake of brevity.

There was a previous "complaint"[2] about the omission of -B in the
synopsis. A response[3] to that message suggested that a patch
addressing the shortcoming would probably be welcome, though it
appears that the author of [2] never submitted such a patch.
Summarizing [3]:

    Whether or not the omission was actually a good decision is
    questionable. [...] Of course, that decision does not assist
    newcomers, so adding "-B" to the synopsis would help the page
    better stand on its own.

Which suggests that this patch is probably a good idea.

The patch itself looks fine (though the noise-change in the position
of the "\" line-splice seems unwarranted).

[1]: https://lore.kernel.org/git/1435969052-540-17-git-send-email-sunshine@sunshineco.com/
[2]: https://lore.kernel.org/git/alpine.LFD.2.21.1711140324580.12112@localhost.localdomain/
[3]: https://lore.kernel.org/git/CAPig+cRc7Yqeys=oPEgPnyR4qT7qKYLbH1ifnp+6F6N+mSzNVA@mail.gmail.com/

> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]]
> -                  [-b <new-branch>] <path> [<commit-ish>]
> +                  [[-b | -B] <new-branch>] <path> [<commit-ish>]
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -15,9 +15,9 @@
> -#define BUILTIN_WORKTREE_ADD_USAGE \
> +#define BUILTIN_WORKTREE_ADD_USAGE                                                        \

This is just a noise-change which (IMHO) makes it harder to spot the
line-splice. (Presumably this whitespace change was made by an
automated formatting tool or by a "too smart" editor?)

>         N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \
> -          "                 [-b <new-branch>] <path> [<commit-ish>]")
> +          "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree
  2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
@ 2022-11-04  3:58     ` Jacob Abel
  2022-11-04 20:45     ` Taylor Blau
  1 sibling, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  3:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git

On 22/11/04 02:32AM, Ævar Arnfjörð Bjarmason wrote:
>
> Won't we now start to run the post-checkout when doing this, and is that
> intended?

It appears it will. `git worktree add` runs the hook manually later after the
`done` label so I can suppress the initial hook run in v2. Is there a
sanctioned way to do this beyond `-c core.hooksPath=/dev/null`? This seems
hacky to put it lightly.

Alternatively would `git symbolic-ref HEAD "refs/heads/${new_branch_name}"`
work instead (along with reverting the change from reset to checkout)
when creating an orphan branch? It appears to work based on my quick tests but
there's something I might be missing.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
@ 2022-11-04  4:11     ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  4:11 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git

On 22/11/04 02:33AM, Ævar Arnfjörð Bjarmason wrote:
>
> > ...
>
> Here.
>
> > ...
>
> And here we have a stray style change, in this case conforming to our
> CodingGuidelines (it's agnostic on the former), but IMO better to keep
> this out, or split it into a "various style stuff" commit, makes this
> harder to review...

I believe those changes were introduced when I ran `make style`. I can revert
these changes in v2.

> > ...
>
> ditto, we don't usually \n\n split up varibale decls.

Noted. Will fix in v2.

> > ...
>
> This is just a stray refactoring of existing code to not-our-usual-style
> (first arg is on the same line as the "(", rest aligned with "(").
>
> > ...
>
> Ditto, these look like they're too-long in the pre-image, but please
> resist re-flowing existing code while at it.
>
> > ...
>
> ditto.

Ditto the comment above regarding `make style`.

> > +	if (!!opts.detach + !!new_branch + !!new_branch_force +
> > +		    !!opts.orphan_branch >
> > +	    1)
>
> The continued "if" is mis-indented, and that "1" is on a line of its
> own...

Ditto the comment above regarding `make style`. Also I'm not exactly sure
what the tool tried to do here but I was initially hesitant to override
the formatter.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 4/4] worktree add: Add unit tests for --orphan
  2022-11-04  1:37   ` Ævar Arnfjörð Bjarmason
@ 2022-11-04  4:17     ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  4:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git

On 22/11/04 02:37AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Nov 04 2022, Jacob Abel wrote:
>
> We usually add tests along with the feature, so this should be squashed
> into your 3/4.
>
> > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> > ---
> >  t/t2400-worktree-add.sh | 54 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >
> > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> > index d587e0b20d..064e1336e2 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 main
> > +'
> > +
> > +test_expect_success '"add" --orphan/-B mutually exclusive' '
> > +	test_must_fail git worktree add --orphan poodle -B poodle bamboo main
> > +'
> > +
> > +test_expect_success '"add" --orphan/--detach mutually exclusive' '
> > +	test_must_fail git worktree add --orphan poodle --detach bamboo main
> > +'
> > +
> > +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
> > +	test_must_fail git worktree add --orphan poodle --no-checkout 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"' '
> > +	git worktree add --orphan neworphan orphandir main &&
> > +	git -C orphandir symbolic-ref HEAD >actual &&
> > +	echo refs/heads/neworphan >expected &&
> > +	test_cmp expected actual &&
>
> nit: we usually do:
>
> 	echo ...
> 	git ...
> 	test_cmp
>
> So there's never doubt that the "expected" is before the command
> (i.e. not impacted by it).
>
> > +	(
> > +		cd orphandir &&
> > +		git diff main
> > +	)
>
> Here you can avoid the sub-shell, with:
>
> 	git -C orphandir diff main
>
> > +'
> > +
> > +test_expect_success '"add --orphan" fails if the branch already exists' '
> > +	git worktree add -b existingbranch orphandir2 main &&
> > +	test_must_fail git worktree add --orphan existingbranch orphandir3 main &&
> > +	[ ! -d orphandir3 ]
>
> Don't use "[", use "test", but in this case use one of the "test -d",
> "test -f" etc. wrappers in test-lib-functions.sh.
>
> > +'
> > +
> > +test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
> > +	test_must_fail git worktree add --orphan badcommitish orphandir4 eee2222 &&
> > +	[ ! -d orphandir4 ]
>
> ditto.

Noted. Will make the above requested changes in v2.

> > +'
> > +
> > +test_expect_success '"add --orphan" with empty repository' '
> > +	(
> > +		mkdir emptyorphanrepo &&
> > +		cd emptyorphanrepo &&
> > +		GIT_DIR=".git" git init --bare &&
> > +		git worktree add --orphan newbranch worktreedir &&
> > +		git -C worktreedir symbolic-ref HEAD >actual &&
> > +		echo refs/heads/newbranch >expected &&
> > +		test_cmp expected actual
> > +	)
>
> Ditto we can avoid sub-shelling here, also when using sub-shells, make
> the "cd" the first command in it, if you keep it the "mkdir" should go
> outside of it.
>
> But isn't this (untested, maybe I'm missing a subtlety):
>
> 	test_when_finished "rm -rf repo" &&
> 	GIT_DIR=.git git init --bare repo &&
> 	git -C worktree ...
> 	git -C worktree/subdir ..
> 	echo refs ...
> 	test_cmp ...
>
> I.e. do we need to create the dir manually before "git init --bare"?

Yes, these should be essentially equivalent. I'll update the tests accordingly.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 1/4] worktree add: Include -B in usage docs
  2022-11-04  3:05   ` Eric Sunshine
@ 2022-11-04  4:24     ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  4:24 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

On 22/11/03 11:05PM, Eric Sunshine wrote:
>
> Thanks. Some historical context...
>
> Omission of -B from the synopsis was intentional[1] for the sake of brevity.
>
> There was a previous "complaint"[2] about the omission of -B in the
> synopsis. A response[3] to that message suggested that a patch
> addressing the shortcoming would probably be welcome, though it
> appears that the author of [2] never submitted such a patch.
> Summarizing [3]:
>
>     Whether or not the omission was actually a good decision is
>     questionable. [...] Of course, that decision does not assist
>     newcomers, so adding "-B" to the synopsis would help the page
>     better stand on its own.
>
> Which suggests that this patch is probably a good idea.
>
> The patch itself looks fine (though the noise-change in the position
> of the "\" line-splice seems unwarranted).
>
> [1]: https://lore.kernel.org/git/1435969052-540-17-git-send-email-sunshine@sunshineco.com/
> [2]: https://lore.kernel.org/git/alpine.LFD.2.21.1711140324580.12112@localhost.localdomain/
> [3]: https://lore.kernel.org/git/CAPig+cRc7Yqeys=oPEgPnyR4qT7qKYLbH1ifnp+6F6N+mSzNVA@mail.gmail.com/
>
> > Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -10,7 +10,7 @@ SYNOPSIS
> >  'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]]
> > -                  [-b <new-branch>] <path> [<commit-ish>]
> > +                  [[-b | -B] <new-branch>] <path> [<commit-ish>]
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -15,9 +15,9 @@
> > -#define BUILTIN_WORKTREE_ADD_USAGE \
> > +#define BUILTIN_WORKTREE_ADD_USAGE                                                        \
>
> This is just a noise-change which (IMHO) makes it harder to spot the
> line-splice. (Presumably this whitespace change was made by an
> automated formatting tool or by a "too smart" editor?)
>
> >         N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \
> > -          "                 [-b <new-branch>] <path> [<commit-ish>]")
> > +          "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")

I appreciate the context and yes most of those noise-changes in this patchset
were courtesy of `make style`/`git clang-format`. For v2 I'll clean out the
noise to make it a bit easier to review.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
                   ` (3 preceding siblings ...)
  2022-11-04  1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
@ 2022-11-04  4:33 ` Eric Sunshine
  2022-11-04  4:47   ` Jacob Abel
  2022-11-04  4:50   ` Jacob Abel
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
  5 siblings, 2 replies; 53+ messages in thread
From: Eric Sunshine @ 2022-11-04  4:33 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git

On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> While working with the worktree based git workflow, I realised that setting
> up a new git repository required switching between the traditional and
> worktree based workflows. Searching online I found a SO answer [1] which
> seemed to support this and which indicated that adding support for this should
> not be technically difficult.

I've also found myself using an orphaned worktree on occasion, so I
can see the value in supporting this use-case directly. In fact, the
idea all along was that `git worktree add` would start with a small
set of options but gain additional options over time as the need
arose. It was foreseen in [1], for instance, that --orphan might one
day be added.

> This patchset has three parts:
>   * adding `-B` to the usage docs (noticed during dev and it seemed too small
>     to justify a separate submission)
>   * switching from `git reset --hard` to `git checkout` for worktree checkout
>   * adding orphan branch functionality (as is present in `git-checkout`)
>     to `git-worktree-add`

I'm pretty sure that we don't want to go the route of using
heavyweight and heavily-featured `git checkout` as a substitute for
lightweight `git reset --hard`. In fact, worktree functionality
started life as a special checkout mode invoked by `git checkout
--to`. A good deal of work[2][3][4] went into extracting that
functionality to a standalone `git worktree add` command, and
eventually ridding `git worktree add` of its unfortunate dependency
upon `git checkout` as a backend[5][6][7], and ridding `git checkout`
of its ugly intimate specialized knowledge of a newly-crafted
worktree.

The key motivation for rejecting `git checkout` as backend and
migrating to `git reset --hard` came from Junio[8][9], and I trust
that his observations are still pertinent.

So, rather than swapping out `git reset --hard` in favor of `git
checkout`, we probably want to stick with the already-established
approach of adding the necessary machinery to `git worktree add`
itself (or by refactoring `git checkout` machinery to be reusable),
just as we did for other `git worktree` options which have `git
checkout` counterparts, such as --track[10], --guess-remote[11],
--[no]-checkout[12], --quiet[13], etc.

[1]: https://lore.kernel.org/git/1436573146-3893-11-git-send-email-sunshine@sunshineco.com/
[2]: https://lore.kernel.org/git/1435640202-95945-1-git-send-email-sunshine@sunshineco.com/
[3]: https://lore.kernel.org/git/1435969052-540-1-git-send-email-sunshine@sunshineco.com/
[4]: https://lore.kernel.org/git/1436203860-846-1-git-send-email-sunshine@sunshineco.com/
[5]: https://lore.kernel.org/git/1436573146-3893-1-git-send-email-sunshine@sunshineco.com/
[6]: https://lore.kernel.org/git/1437034825-32054-1-git-send-email-sunshine@sunshineco.com/
[7]: https://lore.kernel.org/git/1437174017-81687-1-git-send-email-sunshine@sunshineco.com/
[8]: https://lore.kernel.org/git/xmqqh9physyu.fsf@gitster.dls.corp.google.com/
[9]: https://lore.kernel.org/git/CAPig+cSBp-U_jC3fcPXkZQ6kEPh7TRs2bAwKYQGGTtoGR3UYeg@mail.gmail.com/
[10]: https://lore.kernel.org/git/20171129200451.16856-4-t.gummerer@gmail.com/
[11]: https://lore.kernel.org/git/20171129200451.16856-6-t.gummerer@gmail.com/
[12]: https://lore.kernel.org/git/01020153bcda5e6c-2bae9b68-6669-4f29-a512-136c42722001-000000@eu-west-1.amazonses.com/
[13]: https://lore.kernel.org/git/20180815205630.32876-1-gitter.spiros@gmail.com/

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees
  2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
@ 2022-11-04  4:47   ` Jacob Abel
  2022-11-04  4:50   ` Jacob Abel
  1 sibling, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  4:47 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

On 22/11/04 12:33AM, Eric Sunshine wrote:
>
> I'm pretty sure that we don't want to go the route of using
> heavyweight and heavily-featured `git checkout` as a substitute for
> lightweight `git reset --hard`. In fact, worktree functionality
> started life as a special checkout mode invoked by `git checkout
> --to`. A good deal of work[2][3][4] went into extracting that
> functionality to a standalone `git worktree add` command, and
> eventually ridding `git worktree add` of its unfortunate dependency
> upon `git checkout` as a backend[5][6][7], and ridding `git checkout`
> of its ugly intimate specialized knowledge of a newly-crafted
> worktree.
>
> The key motivation for rejecting `git checkout` as backend and
> migrating to `git reset --hard` came from Junio[8][9], and I trust
> that his observations are still pertinent.
>
> So, rather than swapping out `git reset --hard` in favor of `git
> checkout`, we probably want to stick with the already-established
> approach of adding the necessary machinery to `git worktree add`
> itself (or by refactoring `git checkout` machinery to be reusable),
> just as we did for other `git worktree` options which have `git
> checkout` counterparts, such as --track[10], --guess-remote[11],
> --[no]-checkout[12], --quiet[13], etc.

I agree. I appreciate you sharing the history as it makes sense now why
it is this way in particular.

I think I found an alternative which was mentioned over in another reply [1]
(running `git symbolic-ref` after running `git reset`) and I'm going to try
implementing that for v2 and hope it doesn't introduce any complications.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees
  2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
  2022-11-04  4:47   ` Jacob Abel
@ 2022-11-04  4:50   ` Jacob Abel
  1 sibling, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04  4:50 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

On 22/11/04 12:47AM, Jacob Abel wrote:
> On 22/11/04 12:33AM, Eric Sunshine wrote:
> >
> > I'm pretty sure that we don't want to go the route of using
> > heavyweight and heavily-featured `git checkout` as a substitute for
> > lightweight `git reset --hard`. In fact, worktree functionality
> > started life as a special checkout mode invoked by `git checkout
> > --to`. A good deal of work[2][3][4] went into extracting that
> > functionality to a standalone `git worktree add` command, and
> > eventually ridding `git worktree add` of its unfortunate dependency
> > upon `git checkout` as a backend[5][6][7], and ridding `git checkout`
> > of its ugly intimate specialized knowledge of a newly-crafted
> > worktree.
> >
> > The key motivation for rejecting `git checkout` as backend and
> > migrating to `git reset --hard` came from Junio[8][9], and I trust
> > that his observations are still pertinent.
> >
> > So, rather than swapping out `git reset --hard` in favor of `git
> > checkout`, we probably want to stick with the already-established
> > approach of adding the necessary machinery to `git worktree add`
> > itself (or by refactoring `git checkout` machinery to be reusable),
> > just as we did for other `git worktree` options which have `git
> > checkout` counterparts, such as --track[10], --guess-remote[11],
> > --[no]-checkout[12], --quiet[13], etc.
>
> I agree. I appreciate you sharing the history as it makes sense now why
> it is this way in particular.
>
> I think I found an alternative which was mentioned over in another reply [1]
> (running `git symbolic-ref` after running `git reset`) and I'm going to try
> implementing that for v2 and hope it doesn't introduce any complications.

Oops. I forgot to link the other thread.

1. https://lore.kernel.org/git/20221104035751.nnrfpvbqlqheb57k@phi/


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
  2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
@ 2022-11-04  5:03   ` Eric Sunshine
  2022-11-04 16:41     ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-04  5:03 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git

On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> Adds support for creating an orphan branch when adding a new worktree.
> This functionality is equivalent to git checkout's --orphan flag.
> [...]
> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -95,6 +95,17 @@ exist, a new branch based on `HEAD` is automatically created as if
> +------------
> +$ git worktree add --orphan <branch> <path> [<commit-ish>]
> +------------
> ++
> +Create a worktree containing an orphan branch named `<branch>` based
> +on `<commit-ish>`. If `<commit-ish>` is not specified, the new orphan branch
> +will be created based on `HEAD`.
> ++
> +Note that unlike with `-b` or `-B`, this operation will succeed even if
> +`<commit-ish>` is a branch that is currently checked out somewhere else.

Are we sure we want to be modeling this after `git checkout --orphan`?
If I understand correctly, that option has long been considered (by
some) too clunky, which is why `git switch --orphan` was simplified to
accept only a branch name but no commit-ish, and to start the orphan
branch with an empty directory. My own feeling is that modeling it
after `git switch --orphan` is probably the way to go...

> @@ -222,6 +233,11 @@ This can also be set up as the default behaviour by using the
> +--orphan <new-branch>::
> +       With `add`, create a new orphan branch named `<new-branch>` in the new
> +       worktree based on `<commit-ish>`. If `<commit-ish>` is omitted, it
> +       defaults to `HEAD`.

...which would mean that this would no longer talk about `<commit-ish>`.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -608,33 +613,52 @@ static int add(int ac, const char **av, const char *prefix)
>         struct option options[] = {
> +               OPT_STRING(0, "orphan", &opts.orphan_branch, N_("branch"),
> +                          N_("create a new unparented branch")),

The short help message for `git switch --orphan` and `git checkout
--orphan` say simply "new unparented branch", so this message should
probably follow suit (or consistency and to ease the job of
translators).

> -       if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
> -               die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
> +               die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
> +                   "-b", "-B", "--orphan", "--detach");

Good to see this interlock updated for --orphan.

> +       if (opts.orphan_branch && opt_track)
> +               die(_("'%s' cannot be used with '%s'"), "--orphan", "--track");
> +       if (opts.orphan_branch && !opts.checkout)
> +               die(_("'%s' cannot be used with '%s'"), "--orphan",
> +                   "--no-checkout");

Good to have these additional interlocks. I think, however, for the
sake of translators, we should use the same terminology as the
existing message above (i.e. "options ... cannot be used together").

> +       /*
> +        * From here on, new_branch will contain the branch to be checked out,
> +        * and new_branch_force and opts.orphan_branch will tell us which one of
> +        * -b/-B/--orphan is being used.
> +        */

This can probably be worded a bit differently to make it clear that
from this point onward, those other variables are interpreted as if
they are booleans. Moreover, we can make this even clearer by
following the example of -B in which (by necessity due to
parse-options) the local variable in add() is a `const char *`, but
its counterpart in `struct add_opts` is a boolean (int).

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04  5:03   ` Eric Sunshine
@ 2022-11-04 16:41     ` Jacob Abel
  2022-11-10  4:13       ` Eric Sunshine
  0 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-04 16:41 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

On 22/11/04 01:03AM, Eric Sunshine wrote:
> On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> > ...
>
> Are we sure we want to be modeling this after `git checkout --orphan`?
> If I understand correctly, that option has long been considered (by
> some) too clunky, which is why `git switch --orphan` was simplified to
> accept only a branch name but no commit-ish, and to start the orphan
> branch with an empty directory. My own feeling is that modeling it
> after `git switch --orphan` is probably the way to go...

I would argue that the `git checkout --orphan` command format is preferable to
`git switch --orphan` when creating new worktrees. Reason being that in many
cases (except when working in a new repo), if you are trying to create a
worktree from an orphan you will be doing it with a different commit-ish
currently checked out in your worktree than the one you want to use for the
orphan (or you aren't in any worktree).

Requiring the commit-ish to be inferred would limit the user to checking out
an orphan from an existing worktree (in which case they could just create a
new worktree normally and use `git switch --orphan` to move that to an orphan
branch).

> > ...
>
> The short help message for `git switch --orphan` and `git checkout
> --orphan` say simply "new unparented branch", so this message should
> probably follow suit (or consistency and to ease the job of
> translators).

Noted.

> > ...
>
> Good to have these additional interlocks. I think, however, for the
> sake of translators, we should use the same terminology as the
> existing message above (i.e. "options ... cannot be used together").

Noted.

>
> > +       /*
> > +        * From here on, new_branch will contain the branch to be checked out,
> > +        * and new_branch_force and opts.orphan_branch will tell us which one of
> > +        * -b/-B/--orphan is being used.
> > +        */
>
> This can probably be worded a bit differently to make it clear that
> from this point onward, those other variables are interpreted as if
> they are booleans. Moreover, we can make this even clearer by
> following the example of -B in which (by necessity due to
> parse-options) the local variable in add() is a `const char *`, but
> its counterpart in `struct add_opts` is a boolean (int).

The one thing to note with `opts.orphan_branch` is that it is used as both a
string and a boolean later in `add_worktree()`. Since orphan branches don't
have any commits tied to them, we have to check out the original commit-ish
in `add_worktree()` and then convert it to an orphan of name
`opts.orphan_branch` instead of creating the branch prior to entering
`add_worktree()` (as is done for `-B` and `-b`).

I do agree that the comment should probably be re-worded. I'll update it to
be clearer in v2.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree
  2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
  2022-11-04  3:58     ` Jacob Abel
@ 2022-11-04 20:45     ` Taylor Blau
  1 sibling, 0 replies; 53+ messages in thread
From: Taylor Blau @ 2022-11-04 20:45 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jacob Abel, git

On Fri, Nov 04, 2022 at 02:32:11AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > @@ -357,7 +357,7 @@ static int checkout_worktree(const struct add_opts *opts,
> >  {
> >  	struct child_process cp = CHILD_PROCESS_INIT;
> >  	cp.git_cmd = 1;
> > -	strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
> > +	strvec_pushl(&cp.args, "checkout", "--no-recurse-submodules", NULL);
> >  	if (opts->quiet)
> >  		strvec_push(&cp.args, "--quiet");
> >  	strvec_pushv(&cp.env, child_env->v);
>
> Won't we now start to run the post-checkout when doing this, and is that
> intended?

Beyond that, does the change between `reset --hard` and `checkout`'s
treatment of modified files in the working copy matter?

I don't know enough about the worktree code off-hand to know if this
function will ever run in an already-populated worktree that may be
carrying its own modifications.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v2 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
                   ` (4 preceding siblings ...)
  2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
@ 2022-11-04 21:34 ` Jacob Abel
  2022-11-04 21:34   ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
                     ` (2 more replies)
  5 siblings, 3 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04 21:34 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

While working with the worktree based git workflow, I realised that setting
up a new git repository required switching between the traditional and
worktree based workflows. Searching online I found a SO answer [1] which
seemed to support this and which indicated that adding support for this should
not be technically difficult.

This patchset has two parts:

  * adding `-B` to the usage docs (noticed during dev and it seemed too small
    to justify a separate submission)
  * adding orphan branch functionality (as is present in `git-checkout`)
    to `git-worktree-add`

Changes from v1:

  * Reverted change to `checkout_worktree()` [2]. Instead we now change the
    HEAD after `git reset --hard` with a call to `git symbolic-ref`.
  * Removed noise-changes and weird formatting from the patchset.
  * Updated tests and squashed them into the main `--orphan` patch as
    requested [3].
  * Improved test cleanup.
  * Clarify comments regarding `new_branch_force` and `opts.orphan_branch` [4].

1. https://stackoverflow.com/a/68717229/15064705
2. https://lore.kernel.org/git/20221104010242.11555-3-jacobabel@nullpo.dev/
3. https://lore.kernel.org/git/221104.86k04bzeaa.gmgdl@evledraar.gmail.com/
4. https://lore.kernel.org/git/20221104164147.izizapz5mdwwalxu@phi/

Jacob Abel (2):
  worktree add: Include -B in usage docs
  worktree add: add --orphan flag

 Documentation/git-worktree.txt | 18 +++++++-
 builtin/worktree.c             | 81 ++++++++++++++++++++++++++++------
 t/t2400-worktree-add.sh        | 50 +++++++++++++++++++++
 3 files changed, 135 insertions(+), 14 deletions(-)

Range-diff against v1:
1:  d74a58b3bb ! 1:  f35d78cfb4 worktree add: Include -B in usage docs
    @@ Documentation/git-worktree.txt: SYNOPSIS

      ## builtin/worktree.c ##
     @@
    - #include "worktree.h"
    - #include "quote.h"

    --#define BUILTIN_WORKTREE_ADD_USAGE \
    -+#define BUILTIN_WORKTREE_ADD_USAGE                                                        \
    + #define BUILTIN_WORKTREE_ADD_USAGE \
      	N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \
     -	   "                 [-b <new-branch>] <path> [<commit-ish>]")
     +	   "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")
2:  4e56a9494e < -:  ---------- builtin/worktree.c: Update checkout_worktree() to use git-worktree
3:  b8b4098ff5 ! 2:  653be67e8a worktree add: add --orphan flag
    @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
      ## builtin/worktree.c ##
     @@

    - #define BUILTIN_WORKTREE_ADD_USAGE                                                        \
    + #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>]")
    @@ builtin/worktree.c: struct add_opts {
      };

     @@ builtin/worktree.c: static int checkout_worktree(const struct add_opts *opts,
    - 	strvec_pushl(&cp.args, "checkout", "--no-recurse-submodules", NULL);
    - 	if (opts->quiet)
    - 		strvec_push(&cp.args, "--quiet");
    -+	if (opts->orphan_branch)
    -+		strvec_pushl(&cp.args, "--orphan", opts->orphan_branch, NULL);
    - 	strvec_pushv(&cp.env, child_env->v);
      	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)
    + {
     @@ builtin/worktree.c: 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->implicit)
      		die(_("invalid reference: %s"), refname);

    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      		strvec_pushl(&cp.args, "symbolic-ref", "HEAD",
      			     symref.buf, NULL);
      		if (opts->quiet)
    +@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
    + 	if (ret)
    + 		goto done;
    +
    +-	if (opts->checkout &&
    +-	    (ret = checkout_worktree(opts, &child_env)))
    +-		goto done;
    ++	if (opts->checkout) {
    ++		ret = checkout_worktree(opts, &child_env);
    ++		if (opts->orphan_branch && !ret)
    ++			ret = make_worktree_orphan(opts, &child_env);
    ++		if (ret)
    ++			goto done;
    ++	}
    +
    + 	is_junk = 0;
    + 	FREE_AND_NULL(junk_work_tree);
     @@ builtin/worktree.c: 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.
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam

      		strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL);
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
    - 	const char *opt_track = NULL;
    - 	const char *lock_reason = NULL;
    - 	int keep_locked = 0;
    -+
    - 	struct option options[] = {
    --		OPT__FORCE(&opts.force,
    --			   N_("checkout <branch> even if already checked out in other worktree"),
    --			   PARSE_OPT_NOCOMPLETE),
    -+		OPT__FORCE(
    -+			&opts.force,
    -+			N_("checkout <branch> even if already checked out in other worktree"),
    -+			PARSE_OPT_NOCOMPLETE),
    - 		OPT_STRING('b', NULL, &new_branch, N_("branch"),
      			   N_("create a new branch")),
      		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
      			   N_("create or reset a 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")),
     +		OPT_STRING(0, "orphan", &opts.orphan_branch, N_("branch"),
    -+			   N_("create a 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")),
    - 		OPT_STRING(0, "reason", &lock_reason, N_("string"),
    - 			   N_("reason for locking")),
    - 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
    - 		OPT_PASSTHRU(0, "track", &opt_track, NULL,
    - 			     N_("set up tracking mode (see git-branch(1))"),
    - 			     PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
    --		OPT_BOOL(0, "guess-remote", &guess_remote,
    --			 N_("try to match the new branch name with a remote-tracking branch")),
    -+		OPT_BOOL(
    -+			0, "guess-remote", &guess_remote,
    -+			N_("try to match the new branch name with a remote-tracking branch")),
    - 		OPT_END()
    - 	};
    -
    ++			   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")),
    +@@ builtin/worktree.c: 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);
     -	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
     -		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
    -+
     +	opts.implicit = ac < 2;
     +
    -+	if (!!opts.detach + !!new_branch + !!new_branch_force +
    -+		    !!opts.orphan_branch >
    -+	    1)
    ++	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' cannot be used with '%s'"), "--orphan", "--track");
    ++		die(_("'%s' and '%s' cannot be used together"), "--orphan", "--track");
     +	if (opts.orphan_branch && !opts.checkout)
    -+		die(_("'%s' cannot be used with '%s'"), "--orphan",
    ++		die(_("'%s' and '%s' cannot be used together"), "--orphan",
     +		    "--no-checkout");
      	if (lock_reason && !keep_locked)
      		die(_("the option '%s' requires '%s'"), "--reason", "--lock");
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		branch = "@{-1}";

     +	/*
    -+	 * From here on, new_branch will contain the branch to be checked out,
    -+	 * and new_branch_force and opts.orphan_branch will tell us which one of
    -+	 * -b/-B/--orphan is being used.
    ++	 * 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;
    @@ builtin/worktree.c: 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.
    ++	 *
    ++	 * When creating a new orphan, force checkout regardless of whether
    ++	 * the existing branch is already checked out.
    ++	 */
     +	if (opts.orphan_branch) {
     +		new_branch = opts.orphan_branch;
     +		opts.force = 1;
     +	}
     +
    - 	if (ac < 2 && !new_branch && !opts.detach) {
    ++	if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) {
      		const char *s = dwim_branch(path, &new_branch);
      		if (s)
    + 			branch = s;
     @@ builtin/worktree.c: 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);
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		struct child_process cp = CHILD_PROCESS_INIT;
      		cp.git_cmd = 1;
      		strvec_push(&cp.args, "branch");
    +
    + ## t/t2400-worktree-add.sh ##
    +@@ t/t2400-worktree-add.sh: 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 main
    ++'
    ++
    ++test_expect_success '"add" --orphan/-B mutually exclusive' '
    ++	test_must_fail git worktree add --orphan poodle -B poodle bamboo main
    ++'
    ++
    ++test_expect_success '"add" --orphan/--detach mutually exclusive' '
    ++	test_must_fail git worktree add --orphan poodle --detach bamboo main
    ++'
    ++
    ++test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
    ++	test_must_fail git worktree add --orphan poodle --no-checkout 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" fails if the branch is checked out' '
    + 	git rev-parse newmain >before &&
    + 	test_must_fail git worktree add -B newmain bamboo main &&
    +@@ t/t2400-worktree-add.sh: 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 main &&
    ++	echo refs/heads/neworphan >expected &&
    ++	git -C orphandir symbolic-ref HEAD >actual &&
    ++	test_cmp expected actual &&
    ++	git -C orphandir diff main
    ++'
    ++
    ++test_expect_success '"add --orphan" fails if the branch already exists' '
    ++	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 main &&
    ++	test ! -d orphandir2
    ++'
    ++
    ++test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
    ++	test_must_fail git worktree add --orphan badcommitish orphandir eee2222 &&
    ++	test ! -d orphandir
    ++'
    ++
    ++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 )
4:  a167f440c3 < -:  ---------- worktree add: Add unit tests for --orphan
--
2.37.4



^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v2 1/2] worktree add: Include -B in usage docs
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
@ 2022-11-04 21:34   ` Jacob Abel
  2022-11-04 21:34   ` [PATCH v2 2/2] worktree add: add --orphan flag Jacob Abel
  2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04 21:34 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

While -B behavior is already documented, it was not included in the
usage docs for either the man page or the help text. This change fixes
that and brings the usage docs in line with how the flags are documented
in other commands such as git checkout.

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 Documentation/git-worktree.txt | 2 +-
 builtin/worktree.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 063d6eeb99..4dd658012b 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 <new-branch>] <path> [<commit-ish>]
+		   [[-b | -B] <new-branch>] <path> [<commit-ish>]
 '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 4a24d53be1..fccb17f070 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 <new-branch>] <path> [<commit-ish>]")
+	   "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")
 #define BUILTIN_WORKTREE_LIST_USAGE \
 	N_("git worktree list [-v | --porcelain [-z]]")
 #define BUILTIN_WORKTREE_LOCK_USAGE \
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v2 2/2] worktree add: add --orphan flag
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
  2022-11-04 21:34   ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
@ 2022-11-04 21:34   ` Jacob Abel
  2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-04 21:34 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

Adds support for creating an orphan branch when adding a new worktree.
This functionality is equivalent to git checkout'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 | 18 +++++++-
 builtin/worktree.c             | 81 ++++++++++++++++++++++++++++------
 t/t2400-worktree-add.sh        | 50 +++++++++++++++++++++
 3 files changed, 135 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 4dd658012b..92bd75564f 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,17 @@ 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> [<commit-ish>]
+------------
++
+Create a worktree containing an orphan branch named `<branch>` based
+on `<commit-ish>`. If `<commit-ish>` is not specified, the new orphan branch
+will be created based on `HEAD`.
++
+Note that unlike with `-b` or `-B`, this operation will succeed even if
+`<commit-ish>` is a branch that is currently checked out somewhere else.

 list::

@@ -222,6 +233,11 @@ 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 based on `<commit-ish>`. If `<commit-ish>` is omitted, it
+	defaults to `HEAD`.
+
 --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..303a8fb935 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,8 @@ struct add_opts {
 	int detach;
 	int quiet;
 	int checkout;
+	int implicit;
+	const char *orphan_branch;
 	const char *keep_locked;
 };

@@ -364,6 +366,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 +413,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->implicit)
 		die(_("invalid reference: %s"), refname);

 	name = worktree_basename(path, &len);
@@ -482,10 +502,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,9 +517,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;

-	if (opts->checkout &&
-	    (ret = checkout_worktree(opts, &child_env)))
-		goto done;
+	if (opts->checkout) {
+		ret = checkout_worktree(opts, &child_env);
+		if (opts->orphan_branch && !ret)
+			ret = make_worktree_orphan(opts, &child_env);
+		if (ret)
+			goto done;
+	}

 	is_junk = 0;
 	FREE_AND_NULL(junk_work_tree);
@@ -516,7 +540,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 +640,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")),
@@ -633,8 +659,16 @@ 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);
-	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
-		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
+	opts.implicit = ac < 2;
+
+	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)
@@ -646,11 +680,18 @@ static int add(int ac, const char **av, const char *prefix)
 		usage_with_options(git_worktree_add_usage, options);

 	path = prefix_filename(prefix, av[0]);
-	branch = ac < 2 ? "HEAD" : av[1];
+	branch = opts.implicit ? "HEAD" : av[1];

 	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 +704,21 @@ 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.
+	 *
+	 * When creating a new orphan, force checkout regardless of whether
+	 * the existing branch is already checked out.
+	 */
+	if (opts.orphan_branch) {
+		new_branch = opts.orphan_branch;
+		opts.force = 1;
+	}
+
+	if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) {
 		const char *s = dwim_branch(path, &new_branch);
 		if (s)
 			branch = s;
@@ -686,7 +741,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..72930432f9 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 main
+'
+
+test_expect_success '"add" --orphan/-B mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle -B poodle bamboo main
+'
+
+test_expect_success '"add" --orphan/--detach mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle --detach bamboo main
+'
+
+test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
+	test_must_fail git worktree add --orphan poodle --no-checkout 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" 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,36 @@ 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 main &&
+	echo refs/heads/neworphan >expected &&
+	git -C orphandir symbolic-ref HEAD >actual &&
+	test_cmp expected actual &&
+	git -C orphandir diff main
+'
+
+test_expect_success '"add --orphan" fails if the branch already exists' '
+	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 main &&
+	test ! -d orphandir2
+'
+
+test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
+	test_must_fail git worktree add --orphan badcommitish orphandir eee2222 &&
+	test ! -d orphandir
+'
+
+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 )
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-04 16:41     ` Jacob Abel
@ 2022-11-10  4:13       ` Eric Sunshine
  2022-11-10 21:21         ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-10  4:13 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git

On Fri, Nov 4, 2022 at 12:42 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> On 22/11/04 01:03AM, Eric Sunshine wrote:
> > On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> > Are we sure we want to be modeling this after `git checkout --orphan`?
> > If I understand correctly, that option has long been considered (by
> > some) too clunky, which is why `git switch --orphan` was simplified to
> > accept only a branch name but no commit-ish, and to start the orphan
> > branch with an empty directory. My own feeling is that modeling it
> > after `git switch --orphan` is probably the way to go...
>
> I would argue that the `git checkout --orphan` command format is preferable to
> `git switch --orphan` when creating new worktrees. Reason being that in many
> cases (except when working in a new repo), if you are trying to create a
> worktree from an orphan you will be doing it with a different commit-ish
> currently checked out in your worktree than the one you want to use for the
> orphan (or you aren't in any worktree).

I guess I'm not understanding the use-case being described here or
that this series is trying to address. In my own experience, the very,
very few times I've used --orphan was when I needed a branch with no
existing history (i.e. "orphan") and with no existing files. For that
use-case, `git switch --orphan` is ideal, whereas `git checkout
--orphan` is a bother since it requires manually removing all content
from the directory and clearing the index.

> Requiring the commit-ish to be inferred would limit the user to checking out
> an orphan from an existing worktree (in which case they could just create a
> new worktree normally and use `git switch --orphan` to move that to an orphan
> branch).

I'm not following what you mean by inferred commit-ish. `git switch
--orphan` does not infer any commit-ish; it starts the orphaned branch
with an empty directory, hence there is no commit-ish involved.

The `git switch --orphan` behavior was intentionally implemented to
"fix" what has long been considered (by some) a UX botch in the
behavior of `git checkout --orphan`. It was argued that in the vast
majority of cases, people wanted an orphan branch to mean both "no
history" and "no files". So, in that sense, it feels like a step
backward to adopt `git checkout --orphan` when introducing `git
worktree --orphan`.

But, as I said, I'm genuinely not grasping your use-case, so I'm
having trouble understanding why you consider `git checkout --orphan`
a better model. If you can elaborate your use-case more thoroughly,
perhaps it would help (at least me).

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 3/4] worktree add: add --orphan flag
  2022-11-10  4:13       ` Eric Sunshine
@ 2022-11-10 21:21         ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-10 21:21 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

On 22/11/09 11:13PM, Eric Sunshine wrote:
> On Fri, Nov 4, 2022 at 12:42 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> > On 22/11/04 01:03AM, Eric Sunshine wrote:
> > > On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> > > Are we sure we want to be modeling this after `git checkout --orphan`?
> > > If I understand correctly, that option has long been considered (by
> > > some) too clunky, which is why `git switch --orphan` was simplified to
> > > accept only a branch name but no commit-ish, and to start the orphan
> > > branch with an empty directory. My own feeling is that modeling it
> > > after `git switch --orphan` is probably the way to go...
> >
> > I would argue that the `git checkout --orphan` command format is preferable to
> > `git switch --orphan` when creating new worktrees. Reason being that in many
> > cases (except when working in a new repo), if you are trying to create a
> > worktree from an orphan you will be doing it with a different commit-ish
> > currently checked out in your worktree than the one you want to use for the
> > orphan (or you aren't in any worktree).
>
> I guess I'm not understanding the use-case being described here or
> that this series is trying to address. In my own experience, the very,
> very few times I've used --orphan was when I needed a branch with no
> existing history (i.e. "orphan") and with no existing files. For that
> use-case, `git switch --orphan` is ideal, whereas `git checkout
> --orphan` is a bother since it requires manually removing all content
> from the directory and clearing the index.
>
> > Requiring the commit-ish to be inferred would limit the user to checking out
> > an orphan from an existing worktree (in which case they could just create a
> > new worktree normally and use `git switch --orphan` to move that to an orphan
> > branch).
>
> I'm not following what you mean by inferred commit-ish. `git switch
> --orphan` does not infer any commit-ish; it starts the orphaned branch
> with an empty directory, hence there is no commit-ish involved.
>
> The `git switch --orphan` behavior was intentionally implemented to
> "fix" what has long been considered (by some) a UX botch in the
> behavior of `git checkout --orphan`. It was argued that in the vast
> majority of cases, people wanted an orphan branch to mean both "no
> history" and "no files". So, in that sense, it feels like a step
> backward to adopt `git checkout --orphan` when introducing `git
> worktree --orphan`.
>
> But, as I said, I'm genuinely not grasping your use-case, so I'm
> having trouble understanding why you consider `git checkout --orphan`
> a better model. If you can elaborate your use-case more thoroughly,
> perhaps it would help (at least me).

Ah I see where my misunderstanding was. I have significantly less experience
with `git switch` vs `git checkout` so prior to responding I was trying to
understand the difference in behaviour and I ended up misunderstanding what
`git switch --orphan` was doing.

I wrongly assumed that `git switch --orphan` was doing the same thing as
`git checkout --orphan` but using the currently checked out branch.
Additionally I had assumed that there was an important reason for being able
to create orphans from existing branches and that not being able to select
which branch to use would somehow be removing functionality.

After re-reading your replies, I can see that this is not the case and that
I jumped the gun on my reply prior to doing my research properly. I will make
the requested change (moving from `git checkout` to `git switch` semantics)
for v3. Apologies for the misunderstanding.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
  2022-11-04 21:34   ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
  2022-11-04 21:34   ` [PATCH v2 2/2] worktree add: add --orphan flag Jacob Abel
@ 2022-11-10 23:32   ` Jacob Abel
  2022-11-10 23:32     ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
                       ` (2 more replies)
  2 siblings, 3 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-10 23:32 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

While working with the worktree based git workflow, I realised that setting
up a new git repository required switching between the traditional and
worktree based workflows. Searching online I found a SO answer [1] which
seemed to support this and which indicated that adding support for this should
not be technically difficult.

This patchset has two parts:

  * adding `-B` to the usage docs (noticed during dev and it seemed too small
    to justify a separate submission)
  * adding orphan branch functionality (as is present in `git-switch`)
    to `git-worktree-add`

Changes from v2:

  * Changed orphan creation behavior to match `git switch --orphan` instead of
    `git checkout --orphan` [2][3]. As a result `--orphan` no longer accepts a
    `<commit-ish>` and creates the orphan branch with a clean working directory.
  * Removed the `opts.implicit` flag as it is no longer needed and
    `opts.orphan_branch` can be used instead.
  * No longer set `opts.force` when creating an orphan branch (as checkout can
    no longer fail in a way that `--force` would prevent).
  * Updated tests to no longer provide a `<commit-ish>`.
  * Removed no longer relevant test.
  * Added additional cleanup to tests.

1. https://stackoverflow.com/a/68717229/15064705/
2. https://lore.kernel.org/git/CAPig+cSVzewXpk+eDSC-W-+Q8X_7ikZXXeSQbmpHBcdLCU5svw@mail.gmail.com/
3. https://lore.kernel.org/git/20221110212132.3se4imsksjo3gsso@phi/

Jacob Abel (2):
  worktree add: Include -B in usage docs
  worktree add: add --orphan flag

 Documentation/git-worktree.txt | 14 +++++++-
 builtin/worktree.c             | 64 ++++++++++++++++++++++++++++++----
 t/t2400-worktree-add.sh        | 45 ++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 8 deletions(-)

Range-diff against v2:
1:  f35d78cfb4 = 1:  f35d78cfb4 worktree add: Include -B in usage docs
2:  653be67e8a ! 2:  c040c87c6d worktree add: add --orphan flag
    @@ Commit message
         worktree add: add --orphan flag

         Adds support for creating an orphan branch when adding a new worktree.
    -    This functionality is equivalent to git checkout's --orphan flag.
    +    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
    @@ Documentation/git-worktree.txt: exist, a new branch based on `HEAD` is automatic
      command will refuse to create the worktree (unless `--force` is used).
     ++
     +------------
    -+$ git worktree add --orphan <branch> <path> [<commit-ish>]
    ++$ git worktree add --orphan <branch> <path>
     +------------
     ++
    -+Create a worktree containing an orphan branch named `<branch>` based
    -+on `<commit-ish>`. If `<commit-ish>` is not specified, the new orphan branch
    -+will be created based on `HEAD`.
    -++
    -+Note that unlike with `-b` or `-B`, this operation will succeed even if
    -+`<commit-ish>` is a branch that is currently checked out somewhere else.
    ++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::

    @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour

     +--orphan <new-branch>::
     +	With `add`, create a new orphan branch named `<new-branch>` in the new
    -+	worktree based on `<commit-ish>`. If `<commit-ish>` is omitted, it
    -+	defaults to `HEAD`.
    ++	worktree. See `--orphan` in linkgit:git-switch[1] for details.
     +
      --porcelain::
      	With `list`, output in an easy-to-parse format for scripts.
    @@ builtin/worktree.c: struct add_opts {
      	int detach;
      	int quiet;
      	int checkout;
    -+	int implicit;
     +	const char *orphan_branch;
      	const char *keep_locked;
      };
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      	}
      	commit = lookup_commit_reference_by_name(refname);
     -	if (!commit)
    -+	if (!commit && !opts->implicit)
    ++	if (!commit && !opts->orphan_branch)
      		die(_("invalid reference: %s"), refname);

      	name = worktree_basename(path, &len);
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      	if (ret)
      		goto done;

    --	if (opts->checkout &&
    --	    (ret = checkout_worktree(opts, &child_env)))
    --		goto done;
    -+	if (opts->checkout) {
    -+		ret = checkout_worktree(opts, &child_env);
    -+		if (opts->orphan_branch && !ret)
    -+			ret = make_worktree_orphan(opts, &child_env);
    -+		if (ret)
    -+			goto done;
    -+	}
    -
    - 	is_junk = 0;
    - 	FREE_AND_NULL(junk_work_tree);
    ++	if (opts->orphan_branch &&
    ++	    (ret = make_worktree_orphan(opts, &child_env)))
    ++		goto done;
    ++
    + 	if (opts->checkout &&
    + 	    (ret = checkout_worktree(opts, &child_env)))
    + 		goto done;
     @@ builtin/worktree.c: 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.
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		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")),
     @@ builtin/worktree.c: 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);
    --	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
    --		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
    -+	opts.implicit = ac < 2;
    -+
    + 	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");
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		die(_("the option '%s' requires '%s'"), "--reason", "--lock");
      	if (lock_reason)
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
    - 		usage_with_options(git_worktree_add_usage, options);
    -
    - 	path = prefix_filename(prefix, av[0]);
    --	branch = ac < 2 ? "HEAD" : av[1];
    -+	branch = opts.implicit ? "HEAD" : av[1];
    -
      	if (!strcmp(branch, "-"))
      		branch = "@{-1}";

    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
     +	 * 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.
    -+	 *
    -+	 * When creating a new orphan, force checkout regardless of whether
    -+	 * the existing branch is already checked out.
     +	 */
     +	if (opts.orphan_branch) {
     +		new_branch = opts.orphan_branch;
    -+		opts.force = 1;
     +	}
     +
     +	if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) {
    @@ t/t2400-worktree-add.sh: test_expect_success '"add" -B/--detach mutually exclusi
      '

     +test_expect_success '"add" --orphan/-b mutually exclusive' '
    -+	test_must_fail git worktree add --orphan poodle -b poodle bamboo main
    ++	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 main
    ++	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 main
    ++	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 main
    ++	test_must_fail git worktree add --orphan poodle --no-checkout bamboo
     +'
     +
     +test_expect_success '"add" -B/--detach mutually exclusive' '
    @@ t/t2400-worktree-add.sh: test_expect_success 'add --quiet' '

     +test_expect_success '"add --orphan"' '
     +	test_when_finished "git worktree remove -f -f orphandir" &&
    -+	git worktree add --orphan neworphan orphandir main &&
    ++	git worktree add --orphan neworphan orphandir &&
     +	echo refs/heads/neworphan >expected &&
     +	git -C orphandir symbolic-ref HEAD >actual &&
    -+	test_cmp expected actual &&
    -+	git -C orphandir diff main
    ++	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 main &&
    ++	test_must_fail git worktree add --orphan existingbranch orphandir2 &&
     +	test ! -d orphandir2
     +'
     +
    -+test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
    -+	test_must_fail git worktree add --orphan badcommitish orphandir eee2222 &&
    -+	test ! -d orphandir
    -+'
    -+
     +test_expect_success '"add --orphan" with empty repository' '
     +	test_when_finished "rm -rf empty_repo" &&
     +	echo refs/heads/newbranch >expected &&
--
2.37.4



^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 1/2] worktree add: Include -B in usage docs
  2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
@ 2022-11-10 23:32     ` Jacob Abel
  2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
  2022-11-16  0:39     ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
  2 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-10 23:32 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

While -B behavior is already documented, it was not included in the
usage docs for either the man page or the help text. This change fixes
that and brings the usage docs in line with how the flags are documented
in other commands such as git checkout.

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 Documentation/git-worktree.txt | 2 +-
 builtin/worktree.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 063d6eeb99..4dd658012b 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 <new-branch>] <path> [<commit-ish>]
+		   [[-b | -B] <new-branch>] <path> [<commit-ish>]
 '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 4a24d53be1..fccb17f070 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 <new-branch>] <path> [<commit-ish>]")
+	   "                 [[-b | -B] <new-branch>] <path> [<commit-ish>]")
 #define BUILTIN_WORKTREE_LIST_USAGE \
 	N_("git worktree list [-v | --porcelain [-z]]")
 #define BUILTIN_WORKTREE_LOCK_USAGE \
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2022-11-10 23:32     ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
@ 2022-11-10 23:32     ` Jacob Abel
  2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
  2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
  2022-11-16  0:39     ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
  2 siblings, 2 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-10 23:32 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason,
	Eric Sunshine, Taylor Blau

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.

 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 )
--
2.37.4



^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
@ 2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
  2022-11-15 21:29         ` Eric Sunshine
  2022-11-19  1:44         ` Jacob Abel
  2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
  1 sibling, 2 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-15 21:08 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git, Eric Sunshine, Taylor Blau


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?

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
@ 2022-11-15 21:29         ` Eric Sunshine
  2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
  2022-11-19  1:44         ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-15 21:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jacob Abel, git, Taylor Blau

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.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
  2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
@ 2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
  2022-11-19  2:57         ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-15 22:09 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git, Eric Sunshine, Taylor Blau


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) {
		....
	}

?

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 21:29         ` Eric Sunshine
@ 2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
  2022-11-16  0:19             ` Eric Sunshine
  2022-11-19  3:09             ` Jacob Abel
  0 siblings, 2 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-15 22:35 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jacob Abel, git, Taylor Blau


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.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
@ 2022-11-16  0:19             ` Eric Sunshine
  2022-11-19  3:13               ` Jacob Abel
  2022-11-19  3:09             ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-16  0:19 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jacob Abel, git, Taylor Blau

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.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
  2022-11-10 23:32     ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
  2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
@ 2022-11-16  0:39     ` Eric Sunshine
  2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-16  0:39 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git, Ævar Arnfjörð Bjarmason, Taylor Blau

On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> While working with the worktree based git workflow, I realised that setting
> up a new git repository required switching between the traditional and
> worktree based workflows. Searching online I found a SO answer [1] which
> seemed to support this and which indicated that adding support for this should
> not be technically difficult.
>
>   * adding orphan branch functionality (as is present in `git-switch`)
>     to `git-worktree-add`

I haven't had a chance yet to read v3, but can we take a step back for
a moment and look at this topic from a slightly different angle?
Setting aside the value of adding --orphan to `git worktree add`
(which, I'm perfectly fine with, as mentioned earlier), I have a
question about whether the solution proposed by this series is the
best we can do.

As I understand it, the actual problem this series wants to solve is
that it's not possible to create a new worktree from an empty bare
repository; for instance:

    % git init --bare foo.git
    % git -C foo.git worktree add -b main bar
    Preparing worktree (new branch 'main')
    fatal: not a valid object name: 'HEAD'
    %

This series addresses that shortcoming by adding --orphan, so that the
following works:

    % git init --bare foo.git
    % git -C foo.git worktree add --orphan main bar
    Preparing worktree (new branch 'main')
    %

However, is this really the best and most user-friendly and most
discoverable solution? Is it likely that users are somehow going to
instinctively use --orphan when they see the "fatal: not a valid
object name: 'HEAD'" error message?

Wouldn't a better solution be to somehow fix `git worktree add -b
<branch>` so that it just works rather than erroring out? I haven't
delved into the implementation to determine if this is possible, but
if it is, it seems a far superior "fix" for the problem shown above
since it requires no extra effort on the user's part, and doesn't
raise any discoverability red-flags (since nothing needs to be
"discovered" if `-b <branch>` works as expected in the first place).

If fixing `-b <branch>` to "just work" is possible, then --orphan is
no longer a needed workaround but becomes "icing on the cake".

> Changes from v2:
>
>   * Changed orphan creation behavior to match `git switch --orphan` instead of
>     `git checkout --orphan` [2][3]. As a result `--orphan` no longer accepts a
>     `<commit-ish>` and creates the orphan branch with a clean working directory.

Thanks for making this change.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-16  0:39     ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
@ 2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
  2022-11-19  3:47         ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-17 10:00 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jacob Abel, git, Taylor Blau


On Tue, Nov 15 2022, Eric Sunshine wrote:

> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>> While working with the worktree based git workflow, I realised that setting
>> up a new git repository required switching between the traditional and
>> worktree based workflows. Searching online I found a SO answer [1] which
>> seemed to support this and which indicated that adding support for this should
>> not be technically difficult.
>>
>>   * adding orphan branch functionality (as is present in `git-switch`)
>>     to `git-worktree-add`
>
> I haven't had a chance yet to read v3, but can we take a step back for
> a moment and look at this topic from a slightly different angle?
> Setting aside the value of adding --orphan to `git worktree add`
> (which, I'm perfectly fine with, as mentioned earlier), I have a
> question about whether the solution proposed by this series is the
> best we can do.
>
> As I understand it, the actual problem this series wants to solve is
> that it's not possible to create a new worktree from an empty bare
> repository; for instance:
>
>     % git init --bare foo.git
>     % git -C foo.git worktree add -b main bar
>     Preparing worktree (new branch 'main')
>     fatal: not a valid object name: 'HEAD'
>     %
>
> This series addresses that shortcoming by adding --orphan, so that the
> following works:
>
>     % git init --bare foo.git
>     % git -C foo.git worktree add --orphan main bar
>     Preparing worktree (new branch 'main')
>     %
>
> However, is this really the best and most user-friendly and most
> discoverable solution? Is it likely that users are somehow going to
> instinctively use --orphan when they see the "fatal: not a valid
> object name: 'HEAD'" error message?
>
> Wouldn't a better solution be to somehow fix `git worktree add -b
> <branch>` so that it just works rather than erroring out? I haven't
> delved into the implementation to determine if this is possible, but
> if it is, it seems a far superior "fix" for the problem shown above
> since it requires no extra effort on the user's part, and doesn't
> raise any discoverability red-flags (since nothing needs to be
> "discovered" if `-b <branch>` works as expected in the first place).
>
> If fixing `-b <branch>` to "just work" is possible, then --orphan is
> no longer a needed workaround but becomes "icing on the cake".

That's a really good point, and we *could* "fix" that.

But I don't see how to do it without overloading "-b" even further, in a
way that some users either might not mean, or at least would be
confusing.

E.g. one script "manually clones" a repo because it does "git init",
"git remote set-url", "git fetch" etc. Another one makes worktrees from
those fresh checkouts once set up.

If we "DWYM" here that second step will carry forward the bad state
instead of erroring early.

I haven't fully thought this throuh, so maybe it's fine, just
wondering...

...an alternate way to perhaps to do this would be to detect this
situation in add(), and emit an advise() telling the user that maybe
they want to use "--orphan" for this?





^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
  2022-11-15 21:29         ` Eric Sunshine
@ 2022-11-19  1:44         ` Jacob Abel
  2022-11-22  6:00           ` Eric Sunshine
  1 sibling, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-19  1:44 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Sunshine, Taylor Blau

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?


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
@ 2022-11-19  2:57         ` Jacob Abel
  2022-11-19 11:50           ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-19  2:57 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Sunshine, Taylor Blau

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.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
  2022-11-16  0:19             ` Eric Sunshine
@ 2022-11-19  3:09             ` Jacob Abel
  2022-11-19 11:50               ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-19  3:09 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Eric Sunshine, git, Taylor Blau

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.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-16  0:19             ` Eric Sunshine
@ 2022-11-19  3:13               ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-19  3:13 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

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.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
@ 2022-11-19  3:47         ` Jacob Abel
  2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
  2022-11-22 14:45           ` Phillip Wood
  0 siblings, 2 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-19  3:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Eric Sunshine, git, Taylor Blau

On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Nov 15 2022, Eric Sunshine wrote:
>
> > On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> >> While working with the worktree based git workflow, I realised that setting
> >> up a new git repository required switching between the traditional and
> >> worktree based workflows. Searching online I found a SO answer [1] which
> >> seemed to support this and which indicated that adding support for this should
> >> not be technically difficult.
> >>
> >>   * adding orphan branch functionality (as is present in `git-switch`)
> >>     to `git-worktree-add`
> >
> > I haven't had a chance yet to read v3, but can we take a step back for
> > a moment and look at this topic from a slightly different angle?
> > Setting aside the value of adding --orphan to `git worktree add`
> > (which, I'm perfectly fine with, as mentioned earlier), I have a
> > question about whether the solution proposed by this series is the
> > best we can do.
> >
> > As I understand it, the actual problem this series wants to solve is
> > that it's not possible to create a new worktree from an empty bare
> > repository; for instance:
> >
> >     % git init --bare foo.git
> >     % git -C foo.git worktree add -b main bar
> >     Preparing worktree (new branch 'main')
> >     fatal: not a valid object name: 'HEAD'
> >     %
> >
> > This series addresses that shortcoming by adding --orphan, so that the
> > following works:
> >
> >     % git init --bare foo.git
> >     % git -C foo.git worktree add --orphan main bar
> >     Preparing worktree (new branch 'main')
> >     %
> >
> > However, is this really the best and most user-friendly and most
> > discoverable solution? Is it likely that users are somehow going to
> > instinctively use --orphan when they see the "fatal: not a valid
> > object name: 'HEAD'" error message?
> >
> > Wouldn't a better solution be to somehow fix `git worktree add -b
> > <branch>` so that it just works rather than erroring out? I haven't
> > delved into the implementation to determine if this is possible, but
> > if it is, it seems a far superior "fix" for the problem shown above
> > since it requires no extra effort on the user's part, and doesn't
> > raise any discoverability red-flags (since nothing needs to be
> > "discovered" if `-b <branch>` works as expected in the first place).
> >
> > If fixing `-b <branch>` to "just work" is possible, then --orphan is
> > no longer a needed workaround but becomes "icing on the cake".
>
> That's a really good point, and we *could* "fix" that.
>
> But I don't see how to do it without overloading "-b" even further, in a
> way that some users either might not mean, or at least would be
> confusing.
>
> E.g. one script "manually clones" a repo because it does "git init",
> "git remote set-url", "git fetch" etc. Another one makes worktrees from
> those fresh checkouts once set up.
>
> If we "DWYM" here that second step will carry forward the bad state
> instead of erroring early.
>
> I haven't fully thought this throuh, so maybe it's fine, just
> wondering...
>
> ...an alternate way to perhaps to do this would be to detect this
> situation in add(), and emit an advise() telling the user that maybe
> they want to use "--orphan" for this?
>

Prior to writing this patch, I tried to determine if there was a succinct way
to make `-b` "just work" however I wasn't able to find one that wouldn't
introduce unintuitive behavior. My conclusion was that it was probably best
to break it out into a separate command as the other tools had.

I'd support adding an `advise()` for at least the basic case where you try to
create a worktree and no branches currently exist in the repository.
i.e. something like this:

    % git init --bare foo.git
    % git -C foo.git branch --list

    % git -C foo.git worktree add foobar/
    hint: If you meant to create a new initial branch for this repository,
    hint: e.g. 'main', you can do so using the --orphan option:
    hint:
    hint:   git worktree add --orphan main main/
    hint:
    fatal: invalid reference: 'foobar'

and

    % git init --bare foo.git
    % git -C foo.git --no-pager branch --list

    % git -C foo.git worktree add -b foobar foobardir/
    hint: If you meant to create a new initial branch for this repository,
    hint: e.g. 'main', you can do so using the --orphan option:
    hint:
    hint:   git worktree add --orphan main main/
    hint:
    fatal: invalid reference: 'foobar'

but not in the following circumstances:

    % git init --bare foo.git
    % ...
    % git -C foo.git --no-pager branch --list
    + foo
      bar
    % git -C foo.git worktree add foobar/
    Preparing worktree (new branch 'foobar')
    HEAD is now at 319605f8f0 This is a commit message

or

    % git init --bare foo.git
    % ...
    % git -C foo.git --no-pager branch --list
    + foo
      bar
    % git -C foo.git worktree add -b foobar foobardir/
    Preparing worktree (new branch 'foobar)
    HEAD is now at 319605f8f0 This is a commit message

Would there be any other circumstances where we'd definitely want an `advise()`?
Generally I'd assume that outside of those two circumstances, most users will
rarely intend to make an orphan without already knowing they absolutely need to
make an orphan.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-19  3:47         ` Jacob Abel
@ 2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
  2022-11-22  5:16             ` Eric Sunshine
  2022-11-22 14:45           ` Phillip Wood
  1 sibling, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-19 11:48 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Eric Sunshine, git, Taylor Blau


On Sat, Nov 19 2022, Jacob Abel wrote:

> On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Nov 15 2022, Eric Sunshine wrote:
>>
>> > On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>> >> While working with the worktree based git workflow, I realised that setting
>> >> up a new git repository required switching between the traditional and
>> >> worktree based workflows. Searching online I found a SO answer [1] which
>> >> seemed to support this and which indicated that adding support for this should
>> >> not be technically difficult.
>> >>
>> >>   * adding orphan branch functionality (as is present in `git-switch`)
>> >>     to `git-worktree-add`
>> >
>> > I haven't had a chance yet to read v3, but can we take a step back for
>> > a moment and look at this topic from a slightly different angle?
>> > Setting aside the value of adding --orphan to `git worktree add`
>> > (which, I'm perfectly fine with, as mentioned earlier), I have a
>> > question about whether the solution proposed by this series is the
>> > best we can do.
>> >
>> > As I understand it, the actual problem this series wants to solve is
>> > that it's not possible to create a new worktree from an empty bare
>> > repository; for instance:
>> >
>> >     % git init --bare foo.git
>> >     % git -C foo.git worktree add -b main bar
>> >     Preparing worktree (new branch 'main')
>> >     fatal: not a valid object name: 'HEAD'
>> >     %
>> >
>> > This series addresses that shortcoming by adding --orphan, so that the
>> > following works:
>> >
>> >     % git init --bare foo.git
>> >     % git -C foo.git worktree add --orphan main bar
>> >     Preparing worktree (new branch 'main')
>> >     %
>> >
>> > However, is this really the best and most user-friendly and most
>> > discoverable solution? Is it likely that users are somehow going to
>> > instinctively use --orphan when they see the "fatal: not a valid
>> > object name: 'HEAD'" error message?
>> >
>> > Wouldn't a better solution be to somehow fix `git worktree add -b
>> > <branch>` so that it just works rather than erroring out? I haven't
>> > delved into the implementation to determine if this is possible, but
>> > if it is, it seems a far superior "fix" for the problem shown above
>> > since it requires no extra effort on the user's part, and doesn't
>> > raise any discoverability red-flags (since nothing needs to be
>> > "discovered" if `-b <branch>` works as expected in the first place).
>> >
>> > If fixing `-b <branch>` to "just work" is possible, then --orphan is
>> > no longer a needed workaround but becomes "icing on the cake".
>>
>> That's a really good point, and we *could* "fix" that.
>>
>> But I don't see how to do it without overloading "-b" even further, in a
>> way that some users either might not mean, or at least would be
>> confusing.
>>
>> E.g. one script "manually clones" a repo because it does "git init",
>> "git remote set-url", "git fetch" etc. Another one makes worktrees from
>> those fresh checkouts once set up.
>>
>> If we "DWYM" here that second step will carry forward the bad state
>> instead of erroring early.
>>
>> I haven't fully thought this throuh, so maybe it's fine, just
>> wondering...
>>
>> ...an alternate way to perhaps to do this would be to detect this
>> situation in add(), and emit an advise() telling the user that maybe
>> they want to use "--orphan" for this?
>>
>
> Prior to writing this patch, I tried to determine if there was a succinct way
> to make `-b` "just work" however I wasn't able to find one that wouldn't
> introduce unintuitive behavior. My conclusion was that it was probably best
> to break it out into a separate command as the other tools had.
>
> I'd support adding an `advise()` for at least the basic case where you try to
> create a worktree and no branches currently exist in the repository.
> i.e. something like this:
>
>     % git init --bare foo.git
>     % git -C foo.git branch --list
>
>     % git -C foo.git worktree add foobar/
>     hint: If you meant to create a new initial branch for this repository,
>     hint: e.g. 'main', you can do so using the --orphan option:
>     hint:
>     hint:   git worktree add --orphan main main/
>     hint:
>     fatal: invalid reference: 'foobar'
>
> and
>
>     % git init --bare foo.git
>     % git -C foo.git --no-pager branch --list
>
>     % git -C foo.git worktree add -b foobar foobardir/
>     hint: If you meant to create a new initial branch for this repository,
>     hint: e.g. 'main', you can do so using the --orphan option:
>     hint:
>     hint:   git worktree add --orphan main main/
>     hint:
>     fatal: invalid reference: 'foobar'

I think those would make sense, yes.

> but not in the following circumstances:
>
>     % git init --bare foo.git
>     % ...
>     % git -C foo.git --no-pager branch --list
>     + foo
>       bar
>     % git -C foo.git worktree add foobar/
>     Preparing worktree (new branch 'foobar')
>     HEAD is now at 319605f8f0 This is a commit message
>
> or
>
>     % git init --bare foo.git
>     % ...
>     % git -C foo.git --no-pager branch --list
>     + foo
>       bar
>     % git -C foo.git worktree add -b foobar foobardir/
>     Preparing worktree (new branch 'foobar)
>     HEAD is now at 319605f8f0 This is a commit message

*nod*

> Would there be any other circumstances where we'd definitely want an `advise()`?
> Generally I'd assume that outside of those two circumstances, most users will
> rarely intend to make an orphan without already knowing they absolutely need to
> make an orphan.

I'm not familiar enough with the use-cases & workflow around "worktree"
to say, sorry.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-19  3:09             ` Jacob Abel
@ 2022-11-19 11:50               ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-19 11:50 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Eric Sunshine, git, Taylor Blau


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!

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-19  2:57         ` Jacob Abel
@ 2022-11-19 11:50           ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-19 11:50 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git, Eric Sunshine, Taylor Blau


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!

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
@ 2022-11-22  5:16             ` Eric Sunshine
  2022-11-22 23:26               ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-22  5:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jacob Abel, git, Taylor Blau

On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sat, Nov 19 2022, Jacob Abel wrote:
> > I'd support adding an `advise()` for at least the basic case where you try to
> > create a worktree and no branches currently exist in the repository.
> > i.e. something like this:
> >
> >     % git -C foo.git worktree add foobar/
> >     hint: If you meant to create a new initial branch for this repository,
> >     hint: e.g. 'main', you can do so using the --orphan option:
> >     hint:
> >     hint:   git worktree add --orphan main main/
> >     hint:
> >     fatal: invalid reference: 'foobar'
> > and
> >     % git -C foo.git worktree add -b foobar foobardir/
> >     hint: If you meant to create a new initial branch for this repository,
> >     hint: e.g. 'main', you can do so using the --orphan option:
> >     hint:
> >     hint:   git worktree add --orphan main main/
> >     hint:
> >     fatal: invalid reference: 'foobar'
>
> I think those would make sense, yes.

Yes, this sort of advice could go a long way toward addressing my
discoverability concerns. (I think, too, we should be able to
dynamically customize the advice to mention "foobar" rather than
"main" in order to more directly help the user.) Along with that,
explaining this use-case in the git-worktree documentation would also
be valuable for improving discoverability.

Updating the commit message of patch [2/2] to explain this more fully
would also be helpful for reviewers. It wasn't clear to me, for
instance, during initial reviews and discussion that you were adding
--orphan to make this use-case possible. Simply including in the
commit message an example usage and associated error of the current
implementation:

    % git init --bare foo.git
    % git -C foo.git worktree add -b main bar
    Preparing worktree (new branch 'main')
    fatal: not a valid object name: 'HEAD'
    %

would go a long way to help reviewers understand what this series is
trying to achieve (at least it would have helped me).

> > Would there be any other circumstances where we'd definitely want an `advise()`?
> > Generally I'd assume that outside of those two circumstances, most users will
> > rarely intend to make an orphan without already knowing they absolutely need to
> > make an orphan.
>
> I'm not familiar enough with the use-cases & workflow around "worktree"
> to say, sorry.

It's probably fine to limit this advice to `git worktree add`,
certainly for an initial implementation.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-19  1:44         ` Jacob Abel
@ 2022-11-22  6:00           ` Eric Sunshine
  2022-11-22 23:09             ` Jacob Abel
  0 siblings, 1 reply; 53+ messages in thread
From: Eric Sunshine @ 2022-11-22  6:00 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

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").

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-19  3:47         ` Jacob Abel
  2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
@ 2022-11-22 14:45           ` Phillip Wood
  2022-11-23  4:21             ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2022-11-22 14:45 UTC (permalink / raw)
  To: Jacob Abel, Ævar Arnfjörð Bjarmason
  Cc: Eric Sunshine, git, Taylor Blau

On 19/11/2022 03:47, Jacob Abel wrote:
> On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Nov 15 2022, Eric Sunshine wrote:
>>
>>> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>>>> While working with the worktree based git workflow, I realised that setting
>>>> up a new git repository required switching between the traditional and
>>>> worktree based workflows. Searching online I found a SO answer [1] which
>>>> seemed to support this and which indicated that adding support for this should
>>>> not be technically difficult.
>>>>
>>>>    * adding orphan branch functionality (as is present in `git-switch`)
>>>>      to `git-worktree-add`
>>>
>>> I haven't had a chance yet to read v3, but can we take a step back for
>>> a moment and look at this topic from a slightly different angle?
>>> Setting aside the value of adding --orphan to `git worktree add`
>>> (which, I'm perfectly fine with, as mentioned earlier), I have a
>>> question about whether the solution proposed by this series is the
>>> best we can do.
>>>
>>> As I understand it, the actual problem this series wants to solve is
>>> that it's not possible to create a new worktree from an empty bare
>>> repository; for instance:
>>>
>>>      % git init --bare foo.git
>>>      % git -C foo.git worktree add -b main bar
>>>      Preparing worktree (new branch 'main')
>>>      fatal: not a valid object name: 'HEAD'
>>>      %
>>>
>>> This series addresses that shortcoming by adding --orphan, so that the
>>> following works:
>>>
>>>      % git init --bare foo.git
>>>      % git -C foo.git worktree add --orphan main bar
>>>      Preparing worktree (new branch 'main')
>>>      %
>>>
>>> However, is this really the best and most user-friendly and most
>>> discoverable solution? Is it likely that users are somehow going to
>>> instinctively use --orphan when they see the "fatal: not a valid
>>> object name: 'HEAD'" error message?
>>>
>>> Wouldn't a better solution be to somehow fix `git worktree add -b
>>> <branch>` so that it just works rather than erroring out? I haven't
>>> delved into the implementation to determine if this is possible, but
>>> if it is, it seems a far superior "fix" for the problem shown above
>>> since it requires no extra effort on the user's part, and doesn't
>>> raise any discoverability red-flags (since nothing needs to be
>>> "discovered" if `-b <branch>` works as expected in the first place).
>>>
>>> If fixing `-b <branch>` to "just work" is possible, then --orphan is
>>> no longer a needed workaround but becomes "icing on the cake".
>>
>> That's a really good point, and we *could* "fix" that.
>>
>> But I don't see how to do it without overloading "-b" even further, in a
>> way that some users either might not mean, or at least would be
>> confusing.
>>
>> E.g. one script "manually clones" a repo because it does "git init",
>> "git remote set-url", "git fetch" etc. Another one makes worktrees from
>> those fresh checkouts once set up.
>>
>> If we "DWYM" here that second step will carry forward the bad state
>> instead of erroring early.

Wouldn't the first script error out if there was a problem?

>> I haven't fully thought this throuh, so maybe it's fine, just
>> wondering...
>>
>> ...an alternate way to perhaps to do this would be to detect this
>> situation in add(), and emit an advise() telling the user that maybe
>> they want to use "--orphan" for this?
>>
> 
> Prior to writing this patch, I tried to determine if there was a succinct way
> to make `-b` "just work" however I wasn't able to find one that wouldn't
> introduce unintuitive behavior.

Can you say a bit more about what the unintuitive behavior was? As I 
understand it the problem is that "git branch" errors out when HEAD is a 
symbolic ref pointing to a ref that does not exist. I think we can use 
read_ref() to check for that before running "git branch" and act 
accordingly. We might want to check if HEAD matches init.defaultBranch 
and only do an orphan checkout in the new worktree in that case.

> My conclusion was that it was probably best
> to break it out into a separate command as the other tools had.
> 
> I'd support adding an `advise()` for at least the basic case where you try to
> create a worktree and no branches currently exist in the repository.
> i.e. something like this:
> 
>      % git init --bare foo.git
>      % git -C foo.git branch --list
> 
>      % git -C foo.git worktree add foobar/
>      hint: If you meant to create a new initial branch for this repository,
>      hint: e.g. 'main', you can do so using the --orphan option:
>      hint:
>      hint:   git worktree add --orphan main main/
>      hint:
>      fatal: invalid reference: 'foobar'
> 
> and
> 
>      % git init --bare foo.git
>      % git -C foo.git --no-pager branch --list
> 
>      % git -C foo.git worktree add -b foobar foobardir/
>      hint: If you meant to create a new initial branch for this repository,
>      hint: e.g. 'main', you can do so using the --orphan option:
>      hint:
>      hint:   git worktree add --orphan main main/
>      hint:
>      fatal: invalid reference: 'foobar'
> 
> but not in the following circumstances:
> 
>      % git init --bare foo.git
>      % ...
>      % git -C foo.git --no-pager branch --list
>      + foo
>        bar
>      % git -C foo.git worktree add foobar/
>      Preparing worktree (new branch 'foobar')
>      HEAD is now at 319605f8f0 This is a commit message
> 
> or
> 
>      % git init --bare foo.git
>      % ...
>      % git -C foo.git --no-pager branch --list
>      + foo
>        bar
>      % git -C foo.git worktree add -b foobar foobardir/
>      Preparing worktree (new branch 'foobar)
>      HEAD is now at 319605f8f0 This is a commit message
> 
> Would there be any other circumstances where we'd definitely want an `advise()`?
> Generally I'd assume that outside of those two circumstances, most users will
> rarely intend to make an orphan without already knowing they absolutely need to
> make an orphan.

I don't think it matters if the repository is bare so I think it would 
be good to advise() on

	% git init foo
	% git -C foo worktree add bar

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 2/2] worktree add: add --orphan flag
  2022-11-22  6:00           ` Eric Sunshine
@ 2022-11-22 23:09             ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-22 23:09 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

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.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22  5:16             ` Eric Sunshine
@ 2022-11-22 23:26               ` Jacob Abel
  2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
  2022-11-23  2:43                 ` Rubén Justo
  0 siblings, 2 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-22 23:26 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

On 22/11/22 12:16AM, Eric Sunshine wrote:
> On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Sat, Nov 19 2022, Jacob Abel wrote:
> > > I'd support adding an `advise()` for at least the basic case where you try to
> > > create a worktree and no branches currently exist in the repository.
> > > i.e. something like this:
> > >
> > >     % git -C foo.git worktree add foobar/
> > >     hint: If you meant to create a new initial branch for this repository,
> > >     hint: e.g. 'main', you can do so using the --orphan option:
> > >     hint:
> > >     hint:   git worktree add --orphan main main/
> > >     hint:
> > >     fatal: invalid reference: 'foobar'
> > > and
> > >     % git -C foo.git worktree add -b foobar foobardir/
> > >     hint: If you meant to create a new initial branch for this repository,
> > >     hint: e.g. 'main', you can do so using the --orphan option:
> > >     hint:
> > >     hint:   git worktree add --orphan main main/
> > >     hint:
> > >     fatal: invalid reference: 'foobar'
> >
> > I think those would make sense, yes.
>
> Yes, this sort of advice could go a long way toward addressing my
> discoverability concerns. (I think, too, we should be able to
> dynamically customize the advice to mention "foobar" rather than
> "main" in order to more directly help the user.) Along with that,
> explaining this use-case in the git-worktree documentation would also
> be valuable for improving discoverability.

Perfect. I think I've got this working already on my end using more or less
the following:

    diff --git a/builtin/worktree.c b/builtin/worktree.c
    index 71786b72f6..f65b63d9d2 100644
    --- a/builtin/worktree.c
    +++ b/builtin/worktree.c
    @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
    +	if (opts.orphan_branch) {
    +		branch = new_branch;
    +	} else if (!lookup_commit_reference_by_name("head")) {
    +		/*
    +		 * if head does not reference a valid commit, only worktrees
    +		 * based on orphan branches can be created.
    +		 */
    +		advise("if you meant to create a new orphan branch for this repository,\n"
    +			 "e.g. '%s', you can do so using the --orphan option:\n"
    +			 "\n"
    +			 "	git worktree add --orphan %s %s\n"
    +			 "\n",
    +			 new_branch, new_branch, path);
    +		die(_("invalid reference: %s"), new_branch);
    +	} else if (new_branch) {
            struct child_process cp = child_process_init;
            cp.git_cmd = 1;
            strvec_push(&cp.args, "branch");

>
> Updating the commit message of patch [2/2] to explain this more fully
> would also be helpful for reviewers. It wasn't clear to me, for
> instance, during initial reviews and discussion that you were adding
> --orphan to make this use-case possible. Simply including in the
> commit message an example usage and associated error of the current
> implementation:
>
>     % git init --bare foo.git
>     % git -C foo.git worktree add -b main bar
>     Preparing worktree (new branch 'main')
>     fatal: not a valid object name: 'HEAD'
>     %
>
> would go a long way to help reviewers understand what this series is
> trying to achieve (at least it would have helped me).

Will do.

>
> > > Would there be any other circumstances where we'd definitely want an `advise()`?
> > > Generally I'd assume that outside of those two circumstances, most users will
> > > rarely intend to make an orphan without already knowing they absolutely need to
> > > make an orphan.
> >
> > I'm not familiar enough with the use-cases & workflow around "worktree"
> > to say, sorry.
>
> It's probably fine to limit this advice to `git worktree add`,
> certainly for an initial implementation.

Perfect. I'll work on getting the next revision for the patchset out then.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 23:26               ` Jacob Abel
@ 2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
  2022-11-23  2:47                   ` Jacob Abel
  2022-11-23  2:43                 ` Rubén Justo
  1 sibling, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-22 23:55 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Eric Sunshine, git, Taylor Blau


On Tue, Nov 22 2022, Jacob Abel wrote:

> On 22/11/22 12:16AM, Eric Sunshine wrote:
>> On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> > On Sat, Nov 19 2022, Jacob Abel wrote:
>> > > I'd support adding an `advise()` for at least the basic case where you try to
>> > > create a worktree and no branches currently exist in the repository.
>> > > i.e. something like this:
>> > >
>> > >     % git -C foo.git worktree add foobar/
>> > >     hint: If you meant to create a new initial branch for this repository,
>> > >     hint: e.g. 'main', you can do so using the --orphan option:
>> > >     hint:
>> > >     hint:   git worktree add --orphan main main/
>> > >     hint:
>> > >     fatal: invalid reference: 'foobar'
>> > > and
>> > >     % git -C foo.git worktree add -b foobar foobardir/
>> > >     hint: If you meant to create a new initial branch for this repository,
>> > >     hint: e.g. 'main', you can do so using the --orphan option:
>> > >     hint:
>> > >     hint:   git worktree add --orphan main main/
>> > >     hint:
>> > >     fatal: invalid reference: 'foobar'
>> >
>> > I think those would make sense, yes.
>>
>> Yes, this sort of advice could go a long way toward addressing my
>> discoverability concerns. (I think, too, we should be able to
>> dynamically customize the advice to mention "foobar" rather than
>> "main" in order to more directly help the user.) Along with that,
>> explaining this use-case in the git-worktree documentation would also
>> be valuable for improving discoverability.
>
> Perfect. I think I've got this working already on my end using more or less
> the following:
>
>     diff --git a/builtin/worktree.c b/builtin/worktree.c
>     index 71786b72f6..f65b63d9d2 100644
>     --- a/builtin/worktree.c
>     +++ b/builtin/worktree.c
>     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
>     +	if (opts.orphan_branch) {
>     +		branch = new_branch;
>     +	} else if (!lookup_commit_reference_by_name("head")) {
>     +		/*
>     +		 * if head does not reference a valid commit, only worktrees
>     +		 * based on orphan branches can be created.
>     +		 */
>     +		advise("if you meant to create a new orphan branch for this repository,\n"
>     +			 "e.g. '%s', you can do so using the --orphan option:\n"
>     +			 "\n"
>     +			 "	git worktree add --orphan %s %s\n"
>     +			 "\n",
>     +			 new_branch, new_branch, path);

We don't consistently check for this, unfortunately (but I have some
local patches for it), but to add an advice you should:

 * Add it to Documentation/config/advice.txt (in sorted order)
 * Add the corresponding enum to advice.h
 * And to the advice.c listing
 * Then use advise_if_enabled(<that new enum>, ...) in cases such as this one.
 * End your message with a suggstion about how to disable the advice:
   git grep -W -F 'git config advice.' -- '*.c'

That's rather tedious, sorry, but that's the extent of the current
boilerplate...

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 23:26               ` Jacob Abel
  2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
@ 2022-11-23  2:43                 ` Rubén Justo
  2022-11-23  5:37                   ` Jacob Abel
  1 sibling, 1 reply; 53+ messages in thread
From: Rubén Justo @ 2022-11-23  2:43 UTC (permalink / raw)
  To: Jacob Abel, Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

On 22-nov-2022 23:26:57, Jacob Abel wrote:
> On 22/11/22 12:16AM, Eric Sunshine wrote:
> > On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> > > On Sat, Nov 19 2022, Jacob Abel wrote:
> > > > I'd support adding an `advise()` for at least the basic case where you try to
> > > > create a worktree and no branches currently exist in the repository.
> > > > i.e. something like this:
> > > >
> > > >     % git -C foo.git worktree add foobar/
> > > >     hint: If you meant to create a new initial branch for this repository,
> > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > >     hint:
> > > >     hint:   git worktree add --orphan main main/
> > > >     hint:
> > > >     fatal: invalid reference: 'foobar'
> > > > and
> > > >     % git -C foo.git worktree add -b foobar foobardir/
> > > >     hint: If you meant to create a new initial branch for this repository,
> > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > >     hint:
> > > >     hint:   git worktree add --orphan main main/
> > > >     hint:
> > > >     fatal: invalid reference: 'foobar'
> > >
> > > I think those would make sense, yes.
> >
> > Yes, this sort of advice could go a long way toward addressing my
> > discoverability concerns. (I think, too, we should be able to
> > dynamically customize the advice to mention "foobar" rather than
> > "main" in order to more directly help the user.) Along with that,
> > explaining this use-case in the git-worktree documentation would also
> > be valuable for improving discoverability.
> 
> Perfect. I think I've got this working already on my end using more or less
> the following:
> 
>     diff --git a/builtin/worktree.c b/builtin/worktree.c
>     index 71786b72f6..f65b63d9d2 100644
>     --- a/builtin/worktree.c
>     +++ b/builtin/worktree.c
>     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
>     +	if (opts.orphan_branch) {
>     +		branch = new_branch;
>     +	} else if (!lookup_commit_reference_by_name("head")) {

I haven't read the full thread and sorry to enter this way in the
conversation, but this line got my attention.  This needs to be "HEAD",
in capital letters.

Thank you for working on this, this is a thing that has hit me several
times.

The first impression got me thinking.. Why do we need this advise?
Why not make the orphan branch right away? And why the argument for the
--orphan option?

I like what this new flag allows: make a new orphan branch when we
are in any branch.  But if we are already in an orphan branch (like the
initial) what's the user's expectation?

Maybe we can use the new flag to indicate that the user unconditionally
wants an orphan branch, and use the rest of the arguments as they are,
'-b' included.

This needs more work, but something like this:

--- >8 ---

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d774ff192a..1ea8d05c2f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -406,7 +406,7 @@ static int add_worktree(const char *path, const char *refname,
 
 	/* is 'refname' a branch or commit? */
 	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
-	    ref_exists(symref.buf)) {
+	    (opts->orphan_branch || ref_exists(symref.buf))) {
 		is_branch = 1;
 		if (!opts->force)
 			die_if_checked_out(symref.buf, 0);
@@ -738,18 +738,8 @@ static int add(int ac, const char **av, const char *prefix)
 
 	if (opts.orphan_branch) {
 		branch = new_branch;
-	} else if (!lookup_commit_reference_by_name("head")) {
-		/*
-		 * if head does not reference a valid commit, only worktrees
-		 * based on orphan branches can be created.
-		 */
-		advise("if you meant to create a new orphan branch for this repository,\n"
-			 "e.g. '%s', you can do so using the --orphan option:\n"
-			 "\n"
-			 "	git worktree add --orphan %s %s\n"
-			 "\n",
-			 new_branch, new_branch, path);
-		die(_("invalid reference: %s"), new_branch);
+	} else if (new_branch && !lookup_commit_reference_by_name("HEAD")) {
+		branch = opts.orphan_branch = new_branch;
 	} else if (new_branch) {
 		struct child_process cp = CHILD_PROCESS_INIT;
 		cp.git_cmd = 1;

^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
@ 2022-11-23  2:47                   ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-23  2:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Eric Sunshine, git, Taylor Blau

On 22/11/23 12:55AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Nov 22 2022, Jacob Abel wrote:
>
> > On 22/11/22 12:16AM, Eric Sunshine wrote:
> >> On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> >> <avarab@gmail.com> wrote:
> >> > On Sat, Nov 19 2022, Jacob Abel wrote:
> >> > > I'd support adding an `advise()` for at least the basic case where you try to
> >> > > create a worktree and no branches currently exist in the repository.
> >> > > i.e. something like this:
> >> > >
> >> > >     % git -C foo.git worktree add foobar/
> >> > >     hint: If you meant to create a new initial branch for this repository,
> >> > >     hint: e.g. 'main', you can do so using the --orphan option:
> >> > >     hint:
> >> > >     hint:   git worktree add --orphan main main/
> >> > >     hint:
> >> > >     fatal: invalid reference: 'foobar'
> >> > > and
> >> > >     % git -C foo.git worktree add -b foobar foobardir/
> >> > >     hint: If you meant to create a new initial branch for this repository,
> >> > >     hint: e.g. 'main', you can do so using the --orphan option:
> >> > >     hint:
> >> > >     hint:   git worktree add --orphan main main/
> >> > >     hint:
> >> > >     fatal: invalid reference: 'foobar'
> >> >
> >> > I think those would make sense, yes.
> >>
> >> Yes, this sort of advice could go a long way toward addressing my
> >> discoverability concerns. (I think, too, we should be able to
> >> dynamically customize the advice to mention "foobar" rather than
> >> "main" in order to more directly help the user.) Along with that,
> >> explaining this use-case in the git-worktree documentation would also
> >> be valuable for improving discoverability.
> >
> > Perfect. I think I've got this working already on my end using more or less
> > the following:
> >
> >     diff --git a/builtin/worktree.c b/builtin/worktree.c
> >     index 71786b72f6..f65b63d9d2 100644
> >     --- a/builtin/worktree.c
> >     +++ b/builtin/worktree.c
> >     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
> >     +	if (opts.orphan_branch) {
> >     +		branch = new_branch;
> >     +	} else if (!lookup_commit_reference_by_name("head")) {
> >     +		/*
> >     +		 * if head does not reference a valid commit, only worktrees
> >     +		 * based on orphan branches can be created.
> >     +		 */
> >     +		advise("if you meant to create a new orphan branch for this repository,\n"
> >     +			 "e.g. '%s', you can do so using the --orphan option:\n"
> >     +			 "\n"
> >     +			 "	git worktree add --orphan %s %s\n"
> >     +			 "\n",
> >     +			 new_branch, new_branch, path);
>
> We don't consistently check for this, unfortunately (but I have some
> local patches for it), but to add an advice you should:
>
>  * Add it to Documentation/config/advice.txt (in sorted order)
>  * Add the corresponding enum to advice.h
>  * And to the advice.c listing
>  * Then use advise_if_enabled(<that new enum>, ...) in cases such as this one.
>  * End your message with a suggstion about how to disable the advice:
>    git grep -W -F 'git config advice.' -- '*.c'
>
> That's rather tedious, sorry, but that's the extent of the current
> boilerplate...

Noted. Will do.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 14:45           ` Phillip Wood
@ 2022-11-23  4:21             ` Jacob Abel
  0 siblings, 0 replies; 53+ messages in thread
From: Jacob Abel @ 2022-11-23  4:21 UTC (permalink / raw)
  To: phillip.wood
  Cc: Ævar Arnfjörð Bjarmason, Eric Sunshine, git, Taylor Blau

On 22/11/22 02:45PM, Phillip Wood wrote:
> On 19/11/2022 03:47, Jacob Abel wrote:
> > On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Tue, Nov 15 2022, Eric Sunshine wrote:
> >>
> >>> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> >>>> While working with the worktree based git workflow, I realised that setting
> >>>> up a new git repository required switching between the traditional and
> >>>> worktree based workflows. Searching online I found a SO answer [1] which
> >>>> seemed to support this and which indicated that adding support for this should
> >>>> not be technically difficult.
> >>>>
> >>>>    * adding orphan branch functionality (as is present in `git-switch`)
> >>>>      to `git-worktree-add`
> >>>
> >>> I haven't had a chance yet to read v3, but can we take a step back for
> >>> a moment and look at this topic from a slightly different angle?
> >>> Setting aside the value of adding --orphan to `git worktree add`
> >>> (which, I'm perfectly fine with, as mentioned earlier), I have a
> >>> question about whether the solution proposed by this series is the
> >>> best we can do.
> >>>
> >>> As I understand it, the actual problem this series wants to solve is
> >>> that it's not possible to create a new worktree from an empty bare
> >>> repository; for instance:
> >>>
> >>>      % git init --bare foo.git
> >>>      % git -C foo.git worktree add -b main bar
> >>>      Preparing worktree (new branch 'main')
> >>>      fatal: not a valid object name: 'HEAD'
> >>>      %
> >>>
> >>> This series addresses that shortcoming by adding --orphan, so that the
> >>> following works:
> >>>
> >>>      % git init --bare foo.git
> >>>      % git -C foo.git worktree add --orphan main bar
> >>>      Preparing worktree (new branch 'main')
> >>>      %
> >>>
> >>> However, is this really the best and most user-friendly and most
> >>> discoverable solution? Is it likely that users are somehow going to
> >>> instinctively use --orphan when they see the "fatal: not a valid
> >>> object name: 'HEAD'" error message?
> >>>
> >>> Wouldn't a better solution be to somehow fix `git worktree add -b
> >>> <branch>` so that it just works rather than erroring out? I haven't
> >>> delved into the implementation to determine if this is possible, but
> >>> if it is, it seems a far superior "fix" for the problem shown above
> >>> since it requires no extra effort on the user's part, and doesn't
> >>> raise any discoverability red-flags (since nothing needs to be
> >>> "discovered" if `-b <branch>` works as expected in the first place).
> >>>
> >>> If fixing `-b <branch>` to "just work" is possible, then --orphan is
> >>> no longer a needed workaround but becomes "icing on the cake".
> >>
> >> That's a really good point, and we *could* "fix" that.
> >>
> >> But I don't see how to do it without overloading "-b" even further, in a
> >> way that some users either might not mean, or at least would be
> >> confusing.
> >>
> >> E.g. one script "manually clones" a repo because it does "git init",
> >> "git remote set-url", "git fetch" etc. Another one makes worktrees from
> >> those fresh checkouts once set up.
> >>
> >> If we "DWYM" here that second step will carry forward the bad state
> >> instead of erroring early.
>
> Wouldn't the first script error out if there was a problem?
>
> >> I haven't fully thought this throuh, so maybe it's fine, just
> >> wondering...
> >>
> >> ...an alternate way to perhaps to do this would be to detect this
> >> situation in add(), and emit an advise() telling the user that maybe
> >> they want to use "--orphan" for this?
> >>
> >
> > Prior to writing this patch, I tried to determine if there was a succinct way
> > to make `-b` "just work" however I wasn't able to find one that wouldn't
> > introduce unintuitive behavior.
>
> Can you say a bit more about what the unintuitive behavior was? As I
> understand it the problem is that "git branch" errors out when HEAD is a
> symbolic ref pointing to a ref that does not exist. I think we can use
> read_ref() to check for that before running "git branch" and act
> accordingly. We might want to check if HEAD matches init.defaultBranch
> and only do an orphan checkout in the new worktree in that case.

The main issue is that creating an orphan branch is very rarely what the user
intends to do. To modify `-b` to automatically create an orphan would require
that you set the behavior so that `-b` (and DWYM) creates a new orphan branch
only when the repository has no branches (i.e. a fresh init repo).

This has the effect that the command will perform separate operations depending
on the state of the repository and when mixed in with other commands it quickly
becomes confusing.

In the directory shown below:

    ../
    ./
    .git/

with `.git` containing a bare repository with no branches,

    % git worktree add foobar/

would now create a worktree `foobar/` with orphan branch `foobar` (which
technically speaking doesn't exist until a commit is made).

This behavior continues to apply until your first commit.

However after the following:

    % git worktree add foo/
    % cd foo/
    # create files
    % git add .
    % cd ../
    % git worktree add bar/
    % cd bar/
    # create files
    % cd ../foo/
    % git commit -m "foo commit"
    % cd ../bar/
    % git add .
    % git commit -m "bar commit"
    % cd ../foobar/
    # create files
    % git add .
    % git commit -m "foobar commit"

In that same directory:

    ../
    ./
    .git/
    foobar/ <- on branch foobar @ "foobar commit"
    foo/    <- on branch foo    @ "foo commit"
    bar/    <- on branch bar    @ "bar commit"

that same command now creates a branch which is based on whichever reference
HEAD happens to now refer to. In the case of a directory which is not a working
tree, it's not always clear to me what HEAD should actually point to. So now
what should the following do:

    % git worktree add what_am_i/

The user has just created 3 worktrees containing orphan branches. Wouldn't the
user now reasonably expect that from this directory with no working tree that
the above command would also create an orphan branch?

And when it doesn't, which branch is the history based off of? Which one will
the user expect?

- worktree foobar/ which was created first but with the most recent initial commit?
- worktree foo/ where the user has been working the longest?
- worktree bar/ which was created last but which had the earliest initial commit?

This isn't necessarily due to this change in particular but rather that this
change would expose users to an edge case where they can run into really
unintuitive behavior.

Of course this is a bit of an unusual use example but I'd rather warn & direct
the user when they need to do something slightly different/unusual to handle an
edge case rather than risk users getting weird behavior that leaves them turning
to Stack Overflow when they encounter an edge case.

>
> > My conclusion was that it was probably best
> > to break it out into a separate command as the other tools had.
> >
> > I'd support adding an `advise()` for at least the basic case where you try to
> > create a worktree and no branches currently exist in the repository.
> > i.e. something like this:
> >
> >      % git init --bare foo.git
> >      % git -C foo.git branch --list
> >
> >      % git -C foo.git worktree add foobar/
> >      hint: If you meant to create a new initial branch for this repository,
> >      hint: e.g. 'main', you can do so using the --orphan option:
> >      hint:
> >      hint:   git worktree add --orphan main main/
> >      hint:
> >      fatal: invalid reference: 'foobar'
> >
> > and
> >
> >      % git init --bare foo.git
> >      % git -C foo.git --no-pager branch --list
> >
> >      % git -C foo.git worktree add -b foobar foobardir/
> >      hint: If you meant to create a new initial branch for this repository,
> >      hint: e.g. 'main', you can do so using the --orphan option:
> >      hint:
> >      hint:   git worktree add --orphan main main/
> >      hint:
> >      fatal: invalid reference: 'foobar'
> >
> > but not in the following circumstances:
> >
> >      % git init --bare foo.git
> >      % ...
> >      % git -C foo.git --no-pager branch --list
> >      + foo
> >        bar
> >      % git -C foo.git worktree add foobar/
> >      Preparing worktree (new branch 'foobar')
> >      HEAD is now at 319605f8f0 This is a commit message
> >
> > or
> >
> >      % git init --bare foo.git
> >      % ...
> >      % git -C foo.git --no-pager branch --list
> >      + foo
> >        bar
> >      % git -C foo.git worktree add -b foobar foobardir/
> >      Preparing worktree (new branch 'foobar)
> >      HEAD is now at 319605f8f0 This is a commit message
> >
> > Would there be any other circumstances where we'd definitely want an `advise()`?
> > Generally I'd assume that outside of those two circumstances, most users will
> > rarely intend to make an orphan without already knowing they absolutely need to
> > make an orphan.
>
> I don't think it matters if the repository is bare so I think it would
> be good to advise() on
>
> 	% git init foo
> 	% git -C foo worktree add bar
>
> Best Wishes
>
> Phillip

Correct. It shouldn't matter between bare and non-bare. I tend to prefer bare repos
when working with worktrees which is why I wrote it that way but I'm definitely
intending that the advise() works the same for both bare and non-bare.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-23  2:43                 ` Rubén Justo
@ 2022-11-23  5:37                   ` Jacob Abel
  2022-11-23  7:35                     ` Rubén Justo
  0 siblings, 1 reply; 53+ messages in thread
From: Jacob Abel @ 2022-11-23  5:37 UTC (permalink / raw)
  To: Rubén Justo
  Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason, git, Taylor Blau

On 22/11/23 03:43AM, Rubén Justo wrote:
> On 22-nov-2022 23:26:57, Jacob Abel wrote:
> > On 22/11/22 12:16AM, Eric Sunshine wrote:
> > > On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> > > <avarab@gmail.com> wrote:
> > > > On Sat, Nov 19 2022, Jacob Abel wrote:
> > > > > I'd support adding an `advise()` for at least the basic case where you try to
> > > > > create a worktree and no branches currently exist in the repository.
> > > > > i.e. something like this:
> > > > >
> > > > >     % git -C foo.git worktree add foobar/
> > > > >     hint: If you meant to create a new initial branch for this repository,
> > > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > > >     hint:
> > > > >     hint:   git worktree add --orphan main main/
> > > > >     hint:
> > > > >     fatal: invalid reference: 'foobar'
> > > > > and
> > > > >     % git -C foo.git worktree add -b foobar foobardir/
> > > > >     hint: If you meant to create a new initial branch for this repository,
> > > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > > >     hint:
> > > > >     hint:   git worktree add --orphan main main/
> > > > >     hint:
> > > > >     fatal: invalid reference: 'foobar'
> > > >
> > > > I think those would make sense, yes.
> > >
> > > Yes, this sort of advice could go a long way toward addressing my
> > > discoverability concerns. (I think, too, we should be able to
> > > dynamically customize the advice to mention "foobar" rather than
> > > "main" in order to more directly help the user.) Along with that,
> > > explaining this use-case in the git-worktree documentation would also
> > > be valuable for improving discoverability.
> >
> > Perfect. I think I've got this working already on my end using more or less
> > the following:
> >
> >     diff --git a/builtin/worktree.c b/builtin/worktree.c
> >     index 71786b72f6..f65b63d9d2 100644
> >     --- a/builtin/worktree.c
> >     +++ b/builtin/worktree.c
> >     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
> >     +	if (opts.orphan_branch) {
> >     +		branch = new_branch;
> >     +	} else if (!lookup_commit_reference_by_name("head")) {
>
> I haven't read the full thread and sorry to enter this way in the
> conversation, but this line got my attention.

No worries. It's always nice to have more eyes to catch mistakes.

> This needs to be "HEAD", in capital letters.

Ah yes. I wasn't paying attention when I copied it into my MUA and must have
accidentally typed `ggvGu` instead of `ggvGy` and lowercased it before I copied
it (thanks vim/user error). It should be:

    diff --git a/builtin/worktree.c b/builtin/worktree.c
    index 71786b72f6..f65b63d9d2 100644
    --- a/builtin/worktree.c
    +++ b/builtin/worktree.c
    @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
    +	if (opts.orphan_branch) {
    +		branch = new_branch;
    +	} else if (!lookup_commit_reference_by_name("HEAD")) {
    +		/*
    +		 * If HEAD does not reference a valid commit, only worktrees
    +		 * based on orphan branches can be created.
    +		 */
    +		advise("If you meant to create a new orphan branch for this repository,\n"
    +			 "e.g. '%s', you can do so using the --orphan option:\n"
    +			 "\n"
    +			 "	git worktree add --orphan %s %s\n"
    +			 "\n",
    +			 new_branch, new_branch, path);
    +		die(_("invalid reference: %s"), new_branch);
    +	} else if (new_branch) {
            struct child_process cp = CHILD_PROCESS_INIT;
            cp.git_cmd = 1;
            strvec_push(&cp.args, "branch");


>
> Thank you for working on this, this is a thing that has hit me several
> times.
>
> The first impression got me thinking.. Why do we need this advise?
> Why not make the orphan branch right away? And why the argument for the
> --orphan option?

I went into my concerns with further overloading `worktree add -b/-B` and
`worktree add` (DWYM) over on the other side of this thread [1]. I won't echo
it all here but I wanted to mention a few things.

As for why we want the advise, by not short circuiting with the advise and
instead just trying to DWYM, we can catch the following edge case:

A user less well acquainted with git tries out worktrees on a new project (no
branches). They create multiple worktrees and since there are no branches, they
are all orphans. Unless they've read the docs, they are now accustomed to this
"new worktrees have no history" behavior. Then they make a commit on one of the
orphans and the behavior changes and all new worktrees derive from that branch
unless `git worktree add` is run from inside another worktree with a non-orphan
branch.

There's more to it in the other thread but it gets kinda messy for the user if
they walk off the well trodden path inadvertently. I'd like to avoid that all
together where possible.

As for the argument, the reason is so that the syntax matches
`git switch --orphan <new_branch>` (and the `git checkout` variant).

> I like what this new flag allows: make a new orphan branch when we
> are in any branch.  But if we are already in an orphan branch (like the
> initial) what's the user's expectation?

Like mentioned above (and in [1]), further overloading DWYM and `-b` impacts the
already somewhat complex/unclear expectations for `git worktree add`.

When using the flag and not adding to `-b` and DWYM, we can short circuit this
confusion for the most part by requiring the user to explicitly request
`--orphan`.

As for creating a new orphan in a repo with existing branches but from a
worktree containing an orphan branch, that fails cleanly as shown below:

    # in worktree with orphan branch
    % git worktree add -b foobar ../foobar
    Preparing worktree (new branch 'foobar')
    fatal: invalid reference: foobar

and in the next revision should fail with the following:

    # in worktree with orphan branch
    % git worktree add -b foobar ../foobar
    Preparing worktree (new branch 'foobar')
    hint: If you meant to create a new orphan branch for this repository,
    hint: e.g. 'foobar', you can do so using the --orphan option:
    hint:
    hint:   git worktree add --orphan foobar ../foobar/
    hint:
    fatal: invalid reference: foobar

> Maybe we can use the new flag to indicate that the user unconditionally
> wants an orphan branch, and use the rest of the arguments as they are,
> '-b' included.

I wouldn't necessarily be opposed to this however I do think changing it from
`--orphan <new_branch>` to `--orphan -b <new_branch>` would be a departure from
the syntax used in `git switch` and `git checkout` and that may make it harder
for users already familar with those other commands.

>
> This needs more work, but something like this:
>
> --- >8 ---
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index d774ff192a..1ea8d05c2f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -406,7 +406,7 @@ static int add_worktree(const char *path, const char *refname,
>
>  	/* is 'refname' a branch or commit? */
>  	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
> -	    ref_exists(symref.buf)) {
> +	    (opts->orphan_branch || ref_exists(symref.buf))) {
>  		is_branch = 1;
>  		if (!opts->force)
>  			die_if_checked_out(symref.buf, 0);
> @@ -738,18 +738,8 @@ static int add(int ac, const char **av, const char *prefix)
>
>  	if (opts.orphan_branch) {
>  		branch = new_branch;
> -	} else if (!lookup_commit_reference_by_name("head")) {
> -		/*
> -		 * if head does not reference a valid commit, only worktrees
> -		 * based on orphan branches can be created.
> -		 */
> -		advise("if you meant to create a new orphan branch for this repository,\n"
> -			 "e.g. '%s', you can do so using the --orphan option:\n"
> -			 "\n"
> -			 "	git worktree add --orphan %s %s\n"
> -			 "\n",
> -			 new_branch, new_branch, path);
> -		die(_("invalid reference: %s"), new_branch);
> +	} else if (new_branch && !lookup_commit_reference_by_name("HEAD")) {
> +		branch = opts.orphan_branch = new_branch;
>  	} else if (new_branch) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
>  		cp.git_cmd = 1;

1. https://lore.kernel.org/git/20221123042052.t42jmsqjxgx2k3th@phi/


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-23  5:37                   ` Jacob Abel
@ 2022-11-23  7:35                     ` Rubén Justo
  0 siblings, 0 replies; 53+ messages in thread
From: Rubén Justo @ 2022-11-23  7:35 UTC (permalink / raw)
  To: Jacob Abel
  Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason, git, Taylor Blau

On 23-nov-2022 05:37:21, Jacob Abel wrote:
> On 22/11/23 03:43AM, Rubén Justo wrote:
> > On 22-nov-2022 23:26:57, Jacob Abel wrote:
> > > On 22/11/22 12:16AM, Eric Sunshine wrote:
> > > > On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> > > > <avarab@gmail.com> wrote:
> > > > > On Sat, Nov 19 2022, Jacob Abel wrote:
> > > > > > I'd support adding an `advise()` for at least the basic case where you try to
> > > > > > create a worktree and no branches currently exist in the repository.
> > > > > > i.e. something like this:
> > > > > >
> > > > > >     % git -C foo.git worktree add foobar/
> > > > > >     hint: If you meant to create a new initial branch for this repository,
> > > > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > > > >     hint:
> > > > > >     hint:   git worktree add --orphan main main/
> > > > > >     hint:
> > > > > >     fatal: invalid reference: 'foobar'
> > > > > > and
> > > > > >     % git -C foo.git worktree add -b foobar foobardir/
> > > > > >     hint: If you meant to create a new initial branch for this repository,
> > > > > >     hint: e.g. 'main', you can do so using the --orphan option:
> > > > > >     hint:
> > > > > >     hint:   git worktree add --orphan main main/
> > > > > >     hint:
> > > > > >     fatal: invalid reference: 'foobar'
> > > > >
> > > > > I think those would make sense, yes.
> > > >
> > > > Yes, this sort of advice could go a long way toward addressing my
> > > > discoverability concerns. (I think, too, we should be able to
> > > > dynamically customize the advice to mention "foobar" rather than
> > > > "main" in order to more directly help the user.) Along with that,
> > > > explaining this use-case in the git-worktree documentation would also
> > > > be valuable for improving discoverability.
> > >
> > > Perfect. I think I've got this working already on my end using more or less
> > > the following:
> > >
> > >     diff --git a/builtin/worktree.c b/builtin/worktree.c
> > >     index 71786b72f6..f65b63d9d2 100644
> > >     --- a/builtin/worktree.c
> > >     +++ b/builtin/worktree.c
> > >     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
> > >     +	if (opts.orphan_branch) {
> > >     +		branch = new_branch;
> > >     +	} else if (!lookup_commit_reference_by_name("head")) {
> >
> > I haven't read the full thread and sorry to enter this way in the
> > conversation, but this line got my attention.
> 
> No worries. It's always nice to have more eyes to catch mistakes.
> 
> > This needs to be "HEAD", in capital letters.
> 
> Ah yes. I wasn't paying attention when I copied it into my MUA and must have
> accidentally typed `ggvGu` instead of `ggvGy` and lowercased it before I copied
> it (thanks vim/user error). It should be:
> 
>     diff --git a/builtin/worktree.c b/builtin/worktree.c
>     index 71786b72f6..f65b63d9d2 100644
>     --- a/builtin/worktree.c
>     +++ b/builtin/worktree.c
>     @@ -736,7 +736,21 @@ 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 && !opts.orphan_branch) {
>     +	if (opts.orphan_branch) {
>     +		branch = new_branch;
>     +	} else if (!lookup_commit_reference_by_name("HEAD")) {
>     +		/*
>     +		 * If HEAD does not reference a valid commit, only worktrees
>     +		 * based on orphan branches can be created.
>     +		 */
>     +		advise("If you meant to create a new orphan branch for this repository,\n"
>     +			 "e.g. '%s', you can do so using the --orphan option:\n"
>     +			 "\n"
>     +			 "	git worktree add --orphan %s %s\n"
>     +			 "\n",
>     +			 new_branch, new_branch, path);
>     +		die(_("invalid reference: %s"), new_branch);
>     +	} else if (new_branch) {
>             struct child_process cp = CHILD_PROCESS_INIT;
>             cp.git_cmd = 1;
>             strvec_push(&cp.args, "branch");
> 
> 
> >
> > Thank you for working on this, this is a thing that has hit me several
> > times.
> >
> > The first impression got me thinking.. Why do we need this advise?
> > Why not make the orphan branch right away? And why the argument for the
> > --orphan option?
> 
> I went into my concerns with further overloading `worktree add -b/-B` and
> `worktree add` (DWYM) over on the other side of this thread [1]. I won't echo
> it all here but I wanted to mention a few things.
> 
> As for why we want the advise, by not short circuiting with the advise and
> instead just trying to DWYM, we can catch the following edge case:
> 
> A user less well acquainted with git tries out worktrees on a new project (no
> branches). They create multiple worktrees and since there are no branches, they
> are all orphans. Unless they've read the docs, they are now accustomed to this
> "new worktrees have no history" behavior. Then they make a commit on one of the
> orphans and the behavior changes and all new worktrees derive from that branch
> unless `git worktree add` is run from inside another worktree with a non-orphan
> branch.
> 
> There's more to it in the other thread but it gets kinda messy for the user if
> they walk off the well trodden path inadvertently. I'd like to avoid that all
> together where possible.
> 
> As for the argument, the reason is so that the syntax matches
> `git switch --orphan <new_branch>` (and the `git checkout` variant).
> 
> > I like what this new flag allows: make a new orphan branch when we
> > are in any branch.  But if we are already in an orphan branch (like the
> > initial) what's the user's expectation?
> 
> Like mentioned above (and in [1]), further overloading DWYM and `-b` impacts the
> already somewhat complex/unclear expectations for `git worktree add`.
> 
> When using the flag and not adding to `-b` and DWYM, we can short circuit this
> confusion for the most part by requiring the user to explicitly request
> `--orphan`.
> 
> As for creating a new orphan in a repo with existing branches but from a
> worktree containing an orphan branch, that fails cleanly as shown below:
> 
>     # in worktree with orphan branch
>     % git worktree add -b foobar ../foobar
>     Preparing worktree (new branch 'foobar')
>     fatal: invalid reference: foobar
> 
> and in the next revision should fail with the following:
> 
>     # in worktree with orphan branch
>     % git worktree add -b foobar ../foobar
>     Preparing worktree (new branch 'foobar')
>     hint: If you meant to create a new orphan branch for this repository,
>     hint: e.g. 'foobar', you can do so using the --orphan option:
>     hint:
>     hint:   git worktree add --orphan foobar ../foobar/
>     hint:
>     fatal: invalid reference: foobar
> 
> > Maybe we can use the new flag to indicate that the user unconditionally
> > wants an orphan branch, and use the rest of the arguments as they are,
> > '-b' included.
> 
> I wouldn't necessarily be opposed to this however I do think changing it from
> `--orphan <new_branch>` to `--orphan -b <new_branch>` would be a departure from
> the syntax used in `git switch` and `git checkout` and that may make it harder
> for users already familar with those other commands.
> 

Understood.  Maybe allowing a mixed DWYM...

	$ git worktree add --orphan foobar

I'll wait for your next version.

Thank you for the wrap up, and sorry again for reading the thread bottom
up.

^ permalink raw reply	[flat|nested] 53+ messages in thread

end of thread, other threads:[~2022-11-23  7:35 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
2022-11-04  3:05   ` Eric Sunshine
2022-11-04  4:24     ` Jacob Abel
2022-11-04  1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
2022-11-04  3:58     ` Jacob Abel
2022-11-04 20:45     ` Taylor Blau
2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
2022-11-04  4:11     ` Jacob Abel
2022-11-04  5:03   ` Eric Sunshine
2022-11-04 16:41     ` Jacob Abel
2022-11-10  4:13       ` Eric Sunshine
2022-11-10 21:21         ` Jacob Abel
2022-11-04  1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
2022-11-04  1:37   ` Ævar Arnfjörð Bjarmason
2022-11-04  4:17     ` Jacob Abel
2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-04  4:47   ` Jacob Abel
2022-11-04  4:50   ` Jacob Abel
2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
2022-11-04 21:34   ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-04 21:34   ` [PATCH v2 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-10 23:32     ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
2022-11-15 21:29         ` Eric Sunshine
2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
2022-11-16  0:19             ` Eric Sunshine
2022-11-19  3:13               ` Jacob Abel
2022-11-19  3:09             ` Jacob Abel
2022-11-19 11:50               ` Ævar Arnfjörð Bjarmason
2022-11-19  1:44         ` Jacob Abel
2022-11-22  6:00           ` Eric Sunshine
2022-11-22 23:09             ` Jacob Abel
2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
2022-11-19  2:57         ` Jacob Abel
2022-11-19 11:50           ` Ævar Arnfjörð Bjarmason
2022-11-16  0:39     ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
2022-11-19  3:47         ` Jacob Abel
2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
2022-11-22  5:16             ` Eric Sunshine
2022-11-22 23:26               ` Jacob Abel
2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
2022-11-23  2:47                   ` Jacob Abel
2022-11-23  2:43                 ` Rubén Justo
2022-11-23  5:37                   ` Jacob Abel
2022-11-23  7:35                     ` Rubén Justo
2022-11-22 14:45           ` Phillip Wood
2022-11-23  4:21             ` Jacob Abel

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).