[PATCH 0/1] worktree: delete branches auto-created by 'worktree add'

[PATCH 0/1] worktree: delete branches auto-created by 'worktree add'

[Pratyush Yadav]

Hi,

This fixes a small annoyance I had with git-worktree. Most of the
feature is explained in the patch, so I'm using the cover letter to
leave a couple of notes.

- Since the patch changes current behaviour of git-worktree-remove, it
  might break existing scripts. In that case, we might want to add a
  config variable and command line option to trigger that. But for the
  sake of simplicity, I went with making the behaviour default. I don't
  mind making it optional if people think that would be a better idea.

- To make sure no commits are lost, the branch is not deleted if it has
  moved since its creation. This is a more conservative approach. An
  alternative would be to run 'git branch -d' directly without checking if
  branch has moved. This will mean new commits not in upstream branch are
  still preserved but if the branch is simply moved to another commit it
  will still be deleted.

Pratyush Yadav (1):
  worktree: delete branches auto-created by 'worktree add'

 Documentation/git-worktree.txt |  9 ++++--
 builtin/worktree.c             | 52 ++++++++++++++++++++++++++++++++--
 t/t2403-worktree-move.sh       | 45 +++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 5 deletions(-)

--
2.24.1

[PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Pratyush Yadav]

When no branch name is supplied to 'worktree add', it creates a new
branch based on the name of the directory the new worktree is located
in. But when the worktree is later removed, that created branch is left
over.

Remove that branch when removing the worktree. To make sure no commits
are lost, the branch won't be deleted if it has moved.

An example use case of when something like this is useful is when the
user wants to check out a separate worktree to run and test on an older
version, but don't want to touch the current worktree. So, they create a
worktree, run some tests, and then remove it. But this leaves behind a
branch the user never created in the first place.

So, remove the branch if nothing was done on it.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 Documentation/git-worktree.txt |  9 ++++--
 builtin/worktree.c             | 52 ++++++++++++++++++++++++++++++++--
 t/t2403-worktree-move.sh       | 45 +++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 85d92c9761..87b84be608 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -73,8 +73,9 @@ If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, the new worktree is associated with a branch
 (call it `<branch>`) named after `$(basename <path>)`.  If `<branch>`
 doesn't 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
+if `-b <branch>` was given.  In this case, if `<branch>` is not moved, it is
+automatically deleted when the worktree is removed.  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).
 
@@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files
 and no modification in tracked files) can be removed. Unclean working
 trees or ones with submodules can be removed with `--force`. The main
 working tree cannot be removed.
++
+Removing a working tree might lead to its associated branch being deleted if
+it was auto-created and has not moved since. See `add` for more information on
+when exactly this can happen.
 
 unlock::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d6bc5263f1..c62811259a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -35,6 +35,7 @@ struct add_opts {
 static int show_only;
 static int verbose;
 static int guess_remote;
+static int auto_create;
 static timestamp_t expire;
 
 static int git_worktree_config(const char *var, const char *value, void *cb)
@@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct argv_array child_env = ARGV_ARRAY_INIT;
 	unsigned int counter = 0;
-	int len, ret;
+	int len, ret, fd;
 	struct strbuf symref = STRBUF_INIT;
 	struct commit *commit = NULL;
 	int is_branch = 0;
 	struct strbuf sb_name = STRBUF_INIT;
+	struct object_id oid;
+	char *hex;
 
 	validate_worktree_add(path, opts);
 
@@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "%s/auto_created", sb_repo.buf);
+	/* Mark this branch as an "auto-created" one. */
+	if (auto_create) {
+		fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+		get_oid("HEAD", &oid);
+		hex = oid_to_hex(&oid);
+		write_file_buf(sb.buf, hex, strlen(hex));
+
+		if (close(fd))
+			die(_("could not close '%s'"), sb.buf);
+	}
 
 	argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
@@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)
 		if (run_command(&cp))
 			return -1;
 		branch = new_branch;
+
+		auto_create = 1;
 	} else if (opt_track) {
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
@@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 		OPT_END()
 	};
 	struct worktree **worktrees, *wt;
-	struct strbuf errmsg = STRBUF_INIT;
+	struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;
 	const char *reason = NULL;
-	int ret = 0;
+	int ret = 0, delete_auto_created = 0;
+	struct object_id oid;
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (ac != 1)
@@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 		    errmsg.buf);
 	strbuf_release(&errmsg);
 
+	/*
+	 * Check if we auto-created a branch for this worktree and it hasn't
+	 * moved since. Do it before the contents of the worktree get wiped.
+	 * Delete the branch later because it is checked out right now.
+	 */
+	git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
+	if (file_exists(sb.buf)) {
+		strbuf_read_file(&hex, sb.buf, 0);
+		get_oid(wt->id, &oid);
+
+		if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
+			delete_auto_created = 1;
+	}
+
+	strbuf_release(&sb);
+	strbuf_release(&hex);
+
 	if (file_exists(wt->path)) {
 		if (!force)
 			check_clean_worktree(wt, av[0]);
@@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	ret |= delete_git_dir(wt->id);
 	delete_worktrees_dir_if_empty();
 
+	if (delete_auto_created) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		cp.git_cmd = 1;
+
+		argv_array_push(&cp.args, "branch");
+		argv_array_push(&cp.args, "-d");
+		argv_array_push(&cp.args, wt->id);
+
+		ret |= run_command(&cp);
+	}
+
 	free_worktrees(worktrees);
 	return ret;
 }
diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
index 939d18d728..c71c0bc1c7 100755
--- a/t/t2403-worktree-move.sh
+++ b/t/t2403-worktree-move.sh
@@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '
 	)
 '
 
+test_expect_success 'remove auto-created branch' '
+	(
+		git worktree add to-remove &&
+		git worktree remove to-remove &&
+		git branch -l to-remove >branch_list &&
+		test_line_count = 0 branch_list
+	)
+'
+
+test_expect_success 'do not remove a branch that was not auto-created' '
+	(
+		git worktree add -b new_branch to-remove &&
+		git worktree remove to-remove &&
+		git branch -l new_branch >branch_list &&
+		test_line_count = 1 branch_list &&
+		git branch -d new_branch &&
+		git branch foo &&
+		git worktree add to-remove foo &&
+		git worktree remove to-remove &&
+		git branch -l foo >branch_list &&
+		test_line_count = 1 branch_list &&
+		git branch -d foo &&
+		git branch to-remove &&
+		git worktree add to-remove &&
+		git worktree remove to-remove &&
+		git branch -l to-remove >branch_list &&
+		test_line_count = 1 branch_list &&
+		git branch -d to-remove
+	)
+'
+
+test_expect_success 'do not remove auto-created branch that was moved' '
+	(
+		git worktree add to-remove &&
+		cd to-remove &&
+		test_commit foo &&
+		cd ../ &&
+		git worktree remove to-remove &&
+		git branch -l to-remove >branch_list &&
+		test_line_count = 1 branch_list &&
+		git branch -D to-remove
+	)
+'
+
+
 test_done
-- 
2.24.1

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Pratyush Yadav]

On 14/12/19 09:44PM, Pratyush Yadav wrote:
> When no branch name is supplied to 'worktree add', it creates a new
> branch based on the name of the directory the new worktree is located
> in. But when the worktree is later removed, that created branch is left
> over.
> 
> Remove that branch when removing the worktree. To make sure no commits
> are lost, the branch won't be deleted if it has moved.
> 
> An example use case of when something like this is useful is when the
> user wants to check out a separate worktree to run and test on an older
> version, but don't want to touch the current worktree. So, they create a
> worktree, run some tests, and then remove it. But this leaves behind a
> branch the user never created in the first place.
> 
> So, remove the branch if nothing was done on it.
> 
> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>

Ping?

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Eric Sunshine]

On Wed, Dec 18, 2019 at 2:31 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> On 14/12/19 09:44PM, Pratyush Yadav wrote:
> > When no branch name is supplied to 'worktree add', it creates a new
> > branch based on the name of the directory the new worktree is located
> > in. But when the worktree is later removed, that created branch is left
> > over.
> >
> > Remove that branch when removing the worktree. To make sure no commits
> > are lost, the branch won't be deleted if it has moved.
>
> Ping?

I scanned the patch when you sent it and will have a number of
comments to make when I find time to review it formally.

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Eric Sunshine]

(Sorry for taking so long to review this patch; it ended up being a
quite lengthy review.)

On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> When no branch name is supplied to 'worktree add', it creates a new
> branch based on the name of the directory the new worktree is located
> in. But when the worktree is later removed, that created branch is left
> over.

This is describing the existing (intentional) behavior but doesn't
explain why this might be annoying or problematic. To help sell the
patch, it might make sense to say something about how the behavior can
trip up newcomers to git-worktree, leaving them to wonder why they are
accumulating so many branches that they weren't aware they created. A
comment about why you think "git worktree add -d foo" is not a viable
way to side-step the creation of unwanted branches might also be
worthwhile. For instance, you might say something about how newcomers
might not read the documentation thoroughly enough to know about
--detach or to understand what it means; indeed, some newcomers to Git
presumably have trouble with the concept of a detached HEAD and may
find it scary.

> Remove that branch when removing the worktree. To make sure no commits
> are lost, the branch won't be deleted if it has moved.

My knee-jerk reaction upon reading the first sentence of this
paragraph was that this is a significant and undesirable behavior
change, however, the second sentence helps to allay my fears about it.

It's possible, I suppose, that there is some existing tooling
somewhere which relies upon the current behavior, but it's hard to
imagine any good reason to do so. (That is, "git worktree add foo &&
git worktree remove foo" is just a glorified and expensive way to say
"git branch foo".) So, I don't look upon this change with disfavor; it
could well be beneficial for newcomers, and perhaps a nice convenience
in general.

However, there is a rather serious flaw in the implementation. My
expectation is that it should only automatically delete a branch if
the branch creation was inferred; it should never automatically delete
a branch which was created explicitly. You kind of have this covered
(and even have a test for it), but it doesn't work correctly when the
user explicitly requests branch creation via -b/-B and the branch name
matches the worktree name. For instance:

    git worktree add -b foo foo
    git worktree remove foo

incorrectly automatically removes branch "foo" even though the user
requested its creation explicitly.

Another big question: Should an automatically-created branch be
deleted automatically when a worktree is pruned? That is, although
this sequence will remove an automatically-created branch:

    git worktree add foo
    git worktree remove foo

the current patch will not clean up the branch given this sequence:

    git worktree add foo
    rm -rf foo
    git worktree prune

Either way, it might be worthwhile to update the documentation to mention this.

> An example use case of when something like this is useful is when the
> user wants to check out a separate worktree to run and test on an older
> version, but don't want to touch the current worktree. So, they create a
> worktree, run some tests, and then remove it. But this leaves behind a
> branch the user never created in the first place.

The last sentence isn't exactly accurate. The user _did_ create the
branch. It would be more accurate to say "...the user did not
necessarily _intentionally_ create..." or something like that.

> So, remove the branch if nothing was done on it.

By the way, the ordering of the commit message paragraphs is a bit
off; it somewhat tries to justifies the change before explaining what
the problem is. I'd suggest this order:

    - describe current behavior
    - explain why current behavior can be undesirable in some circumstances;
      cite your example use-case here, perhaps
    - describe how this patch improves the situation

The two paragraphs which talk about "remove the branch" are just
repeating one another. I would drop one of them and keep the other as
the final bullet point of the suggested commit message order.

> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -73,8 +73,9 @@ If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
>  doesn't 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
> +if `-b <branch>` was given.  In this case, if `<branch>` is not moved, it is
> +automatically deleted when the worktree is removed.  If `<branch>` does exist,
> +it will be checked out in the new worktree, if it's not checked out anywhere

I found it confusing to find automatic branch deletion described here
under the "worktree add" command...

> @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files
> +Removing a working tree might lead to its associated branch being deleted if
> +it was auto-created and has not moved since. See `add` for more information on
> +when exactly this can happen.

Subjectively, it seems more natural to fully discuss automatic branch
removal here rather than referring to the discussion of "worktree
add".

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -35,6 +35,7 @@ struct add_opts {
> +static int auto_create;

I think this variable belongs in the 'add_opts' structure rather than
being file-global.

> @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,
> -       int len, ret;
> +       int len, ret, fd;
> +       struct object_id oid;
> +       char *hex;

Rather than declaring 'fd', 'oid', and 'hex' here, how about declaring
them in the scope of the "if (auto_create) {" conditional below, which
is the only place they are used?

> @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,
> +       strbuf_reset(&sb);
> +       strbuf_addf(&sb, "%s/auto_created", sb_repo.buf);

Why aren't these two lines inside the "if (auto_create) {" conditional
below? They seem to be used only for that case.

I think this new worktree metadata file warrants a documentation
update. In particular, gitrepository-layout.txt talks about
worktree-specific metadata files, and the "Details" section of
git-worktree.txt may need an update.

A bit of bikeshedding regarding the filename: "auto_created" is rather
unusual. Most names in the .git hierarchy are short and sweet. Also,
with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
filenames seem to use hyphen rather than underscore, which suggests
"auto-created" would be a better choice. However, I'd probably drop
the hyphen altogether. Finally, "auto_created", alone, does not
necessarily convey that the branch was auto-created; someone could
misinterpret it as meaning the worktree itself was auto-created, so I
wonder if a better name can be found.

A bigger question, though, is whether we really want to see new files
like this springing up in the .git/worktrees/<id>/ directory for each
new piece of metadata which belongs to a worktree. I ask because this
isn't the first such case in which some additional worktree-specific
metadata was proposed (see, for instance, [1]). So, I'm wondering if
we should have a more generalized solution, such as introducing a new
file which can hold any sort of metadata which comes along in the
future. In particular, I'm thinking about a file containing an
extensible set of "key: value" tuples, in which case the "auto
created" metadata would be just one of possibly many keys. For
instance:

    branch-auto-created-at: deadbeef

The above is a genuine question. I'm not demanding that this patch
implement it, but I think it deserves discussion and thought before
making a decision.

[1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/

> +       /* Mark this branch as an "auto-created" one. */

This comment doesn't really say anything which the code itself isn't
already saying (especially if you move the strbuf_addf() call inside
the conditional), so the comment could be dropped.

> +       if (auto_create) {
> +               fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> +               get_oid("HEAD", &oid);

Unless I'm mistaken, this is just wrong. You're grabbing the OID of
HEAD from the worktree in which "worktree add" is being invoked,
however, if the new branch name is DWIM'd from an existing
tracking-branch, then the OID should be that of the tracking-branch,
not HEAD of the current worktree. So, you should be using the OID
already looked up earlier in the function, 'commit->object.oid', which
should be correct for either case.

> +               hex = oid_to_hex(&oid);
> +               write_file_buf(sb.buf, hex, strlen(hex));
> +
> +               if (close(fd))
> +                       die(_("could not close '%s'"), sb.buf);
> +       }

Is there a reason you're creating the file in this rather manual
fashion rather than using write_file() as is already heavily used in
this code for creating all the other files residing in
.git/worktrees/<id>/?

This code is correctly sandwiched within the "is_junk" scope, which
means that "auto_created" will be cleaned up automatically, along with
other .git/worktrees/<id>/ files, if "worktree add" fails for some
reason. Good.

>         argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
>         argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
> @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)
>                 if (run_command(&cp))
>                         return -1;
>                 branch = new_branch;
> +
> +               auto_create = 1;

Drop the unnecessary blank line.

By the way, this suffers from the problem that if "git worktree add
foo" fails for some reason, such as because path "foo" already exists,
then the new branch will _not_ be cleaned up automatically since that
failure will happen before "auto_created" is ever created (among other
reasons). But that's not a new issue; it's an existing flaw of
"worktree add" not cleaning up a branch it created before it discovers
that it can't actually create the target directory for some reason, so
I wouldn't expect you to fix that problem with this submission. (I'm
just mentioning it for completeness.)

> @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>                 OPT_END()
>         };
> -       struct strbuf errmsg = STRBUF_INIT;
> +       struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;
> -       int ret = 0;
> +       int ret = 0, delete_auto_created = 0;
> +       struct object_id oid;

Perhaps move the declarations of 'hex' and 'oid' into the scope where
they are used rather than making them global to the function.

> @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> +       /*
> +        * Check if we auto-created a branch for this worktree and it hasn't
> +        * moved since. Do it before the contents of the worktree get wiped.
> +        * Delete the branch later because it is checked out right now.
> +        */

Good useful comment.

> +       git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> +       if (file_exists(sb.buf)) {
> +               strbuf_read_file(&hex, sb.buf, 0);

You can avoid an unnecessary race condition here by dropping the
file_exists() call altogether and just checking the return code of
strbuf_read_file() -- which you should probably be doing anyhow. If
strbuf_read_file() returns a non-negative value, then you know it
exists, so file_exists() is redundant.

> +               get_oid(wt->id, &oid);
> +

Drop the unnecessary blank line.

> +               if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> +                       delete_auto_created = 1;

I was wondering if it would be more semantically correct to parse
'hex' into an 'oid' and compare them with oidcmp() rather than doing a
string comparison of the hex values (though I'm not sure it will
matter in practice).

> +       }
> +
> +       strbuf_release(&sb);
> +       strbuf_release(&hex);

Drop the unnecessary blank line.

> @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> +       if (delete_auto_created) {
> +               struct child_process cp = CHILD_PROCESS_INIT;
> +               cp.git_cmd = 1;
> +
> +               argv_array_push(&cp.args, "branch");
> +               argv_array_push(&cp.args, "-d");
> +               argv_array_push(&cp.args, wt->id);
> +
> +               ret |= run_command(&cp);
> +       }

Alternately:

    argv_array_pushl(&cp.args, "branch", "-d", wt->id, NULL);

However, I don't think it is correct to use 'wt->id' here as the
branch name since there is no guarantee that the <id> in
.git/worktrees/<id>/ matches the branch name with which the worktree
was created. For instance:

    git worktree add foo/bar existing-branch
    git worktree add baz/bar

will, due to name conflicts, create worktree metadata directories:

    .git/worktrees/bar
    .git/worktrees/bar1

where the first is associated with branch "existing-branch", and the
second is associated with new branch "bar". When you then invoke "git
worktree remove baz/bar", it will try removing a branch named "bar1",
not "bar" as intended. To fix this, I think you need to record the
original auto-created branch name in the "auto_created" metadata file
too, not just the OID.

Finally, make this code consistent with other existing similar code in
this file by dropping the unnecessary blank lines in this hunk.

> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '
> +test_expect_success 'remove auto-created branch' '
> +       (
> +               git worktree add to-remove &&
> +               git worktree remove to-remove &&
> +               git branch -l to-remove >branch_list &&
> +               test_line_count = 0 branch_list
> +       )
> +'

I don't think there is any need for this test to be run in a subshell,
so you can drop the enclosing '(' and ')'.

I worry about using porcelain git-branch to check whether the branch
has actually been removed. Using git-rev-parse would likely be a more
direct and safe way to test it. For instance:

    git worktree add to-remove &&
    git worktree remove to-remove &&
    test_must_fail git rev-parse --verify -q to-remove

should be sufficient, I think. And, to be really thorough, you might say:

    test_might_fail git branch -D to-remove &&
    git worktree add to-remove &&
    git rev-parse --verify -q to-remove &&
    git worktree remove to-remove &&
    test_must_fail git rev-parse --verify -q to-remove

The above comments apply to the other new tests added by this patch, as well.

> +test_expect_success 'do not remove a branch that was not auto-created' '
> +       (
> +               git worktree add -b new_branch to-remove &&

Nit: The inconsistent mix of underscore and hyphen in names is odd.
Perhaps settle on one or the other (with a slight preference toward
hyphen).

> +               git worktree remove to-remove &&
> +               git branch -l new_branch >branch_list &&
> +               test_line_count = 1 branch_list &&

As noted earlier, although this particular case of a branch created
explicitly with -b works as expected by not deleting the branch, the
similar case:

    git worktree add -b to-remove to-remove &&

will incorrectly automatically delete the branch.

> +               git branch -d new_branch &&
> +               git branch foo &&
> +               git worktree add to-remove foo &&
> +               git worktree remove to-remove &&
> +               git branch -l foo >branch_list &&
> +               test_line_count = 1 branch_list &&
> +               git branch -d foo &&
> +               git branch to-remove &&
> +               git worktree add to-remove &&
> +               git worktree remove to-remove &&
> +               git branch -l to-remove >branch_list &&
> +               test_line_count = 1 branch_list &&
> +               git branch -d to-remove

If any code above this "git branch -d" fails, then it will never get
this far, thus won't remove "to-remove". To perform cleanup whether
the test succeeds or fails, you should use test_when_finished()
_early_ in the test:

    test_when_finished "git branch -d to-remove || :" &&

However, if you restructure the tests as suggested above, then you
might be able to get away without bothering with this cleanup.

> +       )
> +'

This test is checking three distinct cases of explicitly-created
branches. It would make it easier to debug a failing case if you split
it up into three tests -- one for each case.

> +test_expect_success 'do not remove auto-created branch that was moved' '
> +       (
> +               git worktree add to-remove &&
> +               cd to-remove &&
> +               test_commit foo &&
> +               cd ../ &&

We normally avoid cd'ing around in tests like this because it can
cause tests following this one to run in the wrong directory if
something above the "cd ../" fails. In this particular case, it
doesn't matter since the entire body of this test is within a
subshell.

However, if you take advantage of test_commits()'s -C argument, then
you can ditch the cd's and the subshell altogether:

    test_commit -C to-remove foo &&

> +               git worktree remove to-remove &&
> +               git branch -l to-remove >branch_list &&
> +               test_line_count = 1 branch_list &&
> +               git branch -D to-remove
> +       )
> +'

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Pratyush Yadav]

Hi Eric,

Thanks for the review.

On 27/12/19 06:05AM, Eric Sunshine wrote:
> (Sorry for taking so long to review this patch; it ended up being a
> quite lengthy review.)

No problem :-). And sorry for being so late to reply.
 
> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > When no branch name is supplied to 'worktree add', it creates a new
> > branch based on the name of the directory the new worktree is located
> > in. But when the worktree is later removed, that created branch is left
> > over.
> 
> This is describing the existing (intentional) behavior but doesn't
> explain why this might be annoying or problematic. To help sell the
> patch, it might make sense to say something about how the behavior can
> trip up newcomers to git-worktree, leaving them to wonder why they are
> accumulating so many branches that they weren't aware they created. A
> comment about why you think "git worktree add -d foo" is not a viable
> way to side-step the creation of unwanted branches might also be
> worthwhile. For instance, you might say something about how newcomers
> might not read the documentation thoroughly enough to know about
> --detach or to understand what it means; indeed, some newcomers to Git
> presumably have trouble with the concept of a detached HEAD and may
> find it scary.

Will do.
 
> > Remove that branch when removing the worktree. To make sure no commits
> > are lost, the branch won't be deleted if it has moved.
> 
> My knee-jerk reaction upon reading the first sentence of this
> paragraph was that this is a significant and undesirable behavior
> change, however, the second sentence helps to allay my fears about it.
> 
> It's possible, I suppose, that there is some existing tooling
> somewhere which relies upon the current behavior, but it's hard to
> imagine any good reason to do so. (That is, "git worktree add foo &&
> git worktree remove foo" is just a glorified and expensive way to say
> "git branch foo".) So, I don't look upon this change with disfavor; it
> could well be beneficial for newcomers, and perhaps a nice convenience
> in general.

It is possible that some script somewhere does 

  git worktree add foo
  do_something # doesn't move the branch
  git worktree remove foo
  git branch -d foo

Branch deletion would fail here, which might be considered as an error 
by the script. Not sure how common that would be though.
 
> However, there is a rather serious flaw in the implementation. My
> expectation is that it should only automatically delete a branch if
> the branch creation was inferred; it should never automatically delete
> a branch which was created explicitly. You kind of have this covered
> (and even have a test for it), but it doesn't work correctly when the
> user explicitly requests branch creation via -b/-B and the branch name
> matches the worktree name. For instance:
> 
>     git worktree add -b foo foo
>     git worktree remove foo
> 
> incorrectly automatically removes branch "foo" even though the user
> requested its creation explicitly.

Thanks for pointing it out. Will fix.
 
> Another big question: Should an automatically-created branch be
> deleted automatically when a worktree is pruned? That is, although
> this sequence will remove an automatically-created branch:
> 
>     git worktree add foo
>     git worktree remove foo
> 
> the current patch will not clean up the branch given this sequence:
> 
>     git worktree add foo
>     rm -rf foo
>     git worktree prune

I see no problem with doing the same thing in 'prune' too.
 
> Either way, it might be worthwhile to update the documentation to mention this.

I'll see if I can make 'prune' delete the branch too. Otherwise, I'll 
mention it in the documentation.
 
> > An example use case of when something like this is useful is when the
> > user wants to check out a separate worktree to run and test on an older
> > version, but don't want to touch the current worktree. So, they create a
> > worktree, run some tests, and then remove it. But this leaves behind a
> > branch the user never created in the first place.
> 
> The last sentence isn't exactly accurate. The user _did_ create the
> branch. It would be more accurate to say "...the user did not
> necessarily _intentionally_ create..." or something like that.

Yes, that was the intention of the sentence. Will fix.
 
> > So, remove the branch if nothing was done on it.
> 
> By the way, the ordering of the commit message paragraphs is a bit
> off; it somewhat tries to justifies the change before explaining what
> the problem is. I'd suggest this order:
> 
>     - describe current behavior
>     - explain why current behavior can be undesirable in some circumstances;
>       cite your example use-case here, perhaps
>     - describe how this patch improves the situation
> 
> The two paragraphs which talk about "remove the branch" are just
> repeating one another. I would drop one of them and keep the other as
> the final bullet point of the suggested commit message order.

Will fix.
 
> > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -73,8 +73,9 @@ If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> >  doesn't 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
> > +if `-b <branch>` was given.  In this case, if `<branch>` is not moved, it is
> > +automatically deleted when the worktree is removed.  If `<branch>` does exist,
> > +it will be checked out in the new worktree, if it's not checked out anywhere
> 
> I found it confusing to find automatic branch deletion described here
> under the "worktree add" command...
> 
> > @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files
> > +Removing a working tree might lead to its associated branch being deleted if
> > +it was auto-created and has not moved since. See `add` for more information on
> > +when exactly this can happen.
> 
> Subjectively, it seems more natural to fully discuss automatic branch
> removal here rather than referring to the discussion of "worktree
> add".

I considered doing this but then left that part in 'add' because the 
conditions in which the branch is auto deleted are described pretty well 
in add's documentation. Will move it to 'remove'.
 
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -35,6 +35,7 @@ struct add_opts {
> > +static int auto_create;
> 
> I think this variable belongs in the 'add_opts' structure rather than
> being file-global.

Ok.
 
> > @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,
> > -       int len, ret;
> > +       int len, ret, fd;
> > +       struct object_id oid;
> > +       char *hex;
> 
> Rather than declaring 'fd', 'oid', and 'hex' here, how about declaring
> them in the scope of the "if (auto_create) {" conditional below, which
> is the only place they are used?

Will do. Some other projects I've contributed to in the past insist on 
declaring everything up-front, so I played it safe and put them here.
 
> > @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,
> > +       strbuf_reset(&sb);
> > +       strbuf_addf(&sb, "%s/auto_created", sb_repo.buf);
> 
> Why aren't these two lines inside the "if (auto_create) {" conditional
> below? They seem to be used only for that case.

Yes, they should be. Will fix.
 
> I think this new worktree metadata file warrants a documentation
> update. In particular, gitrepository-layout.txt talks about
> worktree-specific metadata files, and the "Details" section of
> git-worktree.txt may need an update.

Will fix.
 
> A bit of bikeshedding regarding the filename: "auto_created" is rather
> unusual. Most names in the .git hierarchy are short and sweet. Also,
> with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
> filenames seem to use hyphen rather than underscore, which suggests
> "auto-created" would be a better choice. However, I'd probably drop
> the hyphen altogether. Finally, "auto_created", alone, does not
> necessarily convey that the branch was auto-created; someone could
> misinterpret it as meaning the worktree itself was auto-created, so I
> wonder if a better name can be found.

Any suggestions? Does "implicitbranch"/"implicit-branch" sound any 
better? How about "branch-auto-created-at"? It is very clear but is a 
mouthful.
 
> A bigger question, though, is whether we really want to see new files
> like this springing up in the .git/worktrees/<id>/ directory for each
> new piece of metadata which belongs to a worktree. I ask because this
> isn't the first such case in which some additional worktree-specific
> metadata was proposed (see, for instance, [1]). So, I'm wondering if
> we should have a more generalized solution, such as introducing a new
> file which can hold any sort of metadata which comes along in the
> future. In particular, I'm thinking about a file containing an
> extensible set of "key: value" tuples, in which case the "auto
> created" metadata would be just one of possibly many keys. For
> instance:

Do you worry that the number of metadata files might grow to be too 
large? I can't say how worktrees will grow in the future, but right now 
there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD'). 
So, not a lot.

I chose to add a new file because from what I have noticed, Git keeps a 
lot of metadata in files like this (HEAD, refs, etc). Do other 
subsystems use a key-value store? What problems did they face?
 
>     branch-auto-created-at: deadbeef
> 
> The above is a genuine question. I'm not demanding that this patch
> implement it, but I think it deserves discussion and thought before
> making a decision.

I'd prefer to not take on this feature (since I expect it to be a lot of 
work), but if there are strong opinions on using a key-value store then 
I guess I'll bite the bullet.
 
> [1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/
> 
> > +       /* Mark this branch as an "auto-created" one. */
> 
> This comment doesn't really say anything which the code itself isn't
> already saying (especially if you move the strbuf_addf() call inside
> the conditional), so the comment could be dropped.

Ok.
 
> > +       if (auto_create) {
> > +               fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > +               get_oid("HEAD", &oid);
> 
> Unless I'm mistaken, this is just wrong. You're grabbing the OID of
> HEAD from the worktree in which "worktree add" is being invoked,
> however, if the new branch name is DWIM'd from an existing
> tracking-branch, then the OID should be that of the tracking-branch,
> not HEAD of the current worktree. So, you should be using the OID
> already looked up earlier in the function, 'commit->object.oid', which
> should be correct for either case.

Oops! Thanks for pointing it out. Will fix.
 
> > +               hex = oid_to_hex(&oid);
> > +               write_file_buf(sb.buf, hex, strlen(hex));
> > +
> > +               if (close(fd))
> > +                       die(_("could not close '%s'"), sb.buf);
> > +       }
> 
> Is there a reason you're creating the file in this rather manual
> fashion rather than using write_file() as is already heavily used in
> this code for creating all the other files residing in
> .git/worktrees/<id>/?

No particular reason. I didn't look around enough to catch the pattern 
of using write_file() for this. Will fix.
 
> This code is correctly sandwiched within the "is_junk" scope, which
> means that "auto_created" will be cleaned up automatically, along with
> other .git/worktrees/<id>/ files, if "worktree add" fails for some
> reason. Good.
> 
> >         argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
> >         argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
> > @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)
> >                 if (run_command(&cp))
> >                         return -1;
> >                 branch = new_branch;
> > +
> > +               auto_create = 1;
> 
> Drop the unnecessary blank line.
> 
> By the way, this suffers from the problem that if "git worktree add
> foo" fails for some reason, such as because path "foo" already exists,
> then the new branch will _not_ be cleaned up automatically since that
> failure will happen before "auto_created" is ever created (among other
> reasons). But that's not a new issue; it's an existing flaw of
> "worktree add" not cleaning up a branch it created before it discovers
> that it can't actually create the target directory for some reason, so
> I wouldn't expect you to fix that problem with this submission. (I'm
> just mentioning it for completeness.)

I'll see if I can come up with a fix for this as a follow-up patch.
 
> > @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> >                 OPT_END()
> >         };
> > -       struct strbuf errmsg = STRBUF_INIT;
> > +       struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;
> > -       int ret = 0;
> > +       int ret = 0, delete_auto_created = 0;
> > +       struct object_id oid;
> 
> Perhaps move the declarations of 'hex' and 'oid' into the scope where
> they are used rather than making them global to the function.

Will do.
 
> > @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > +       /*
> > +        * Check if we auto-created a branch for this worktree and it hasn't
> > +        * moved since. Do it before the contents of the worktree get wiped.
> > +        * Delete the branch later because it is checked out right now.
> > +        */
> 
> Good useful comment.

Thanks.
 
> > +       git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> > +       if (file_exists(sb.buf)) {
> > +               strbuf_read_file(&hex, sb.buf, 0);
> 
> You can avoid an unnecessary race condition here by dropping the
> file_exists() call altogether and just checking the return code of
> strbuf_read_file() -- which you should probably be doing anyhow. If
> strbuf_read_file() returns a non-negative value, then you know it
> exists, so file_exists() is redundant.

Will fix. Though I don't see how it would be a "race condition". Is 
file_exists() asynchronous in some way? Otherwise, how would a race 
happen and between what?
 
> > +               get_oid(wt->id, &oid);
> > +
> 
> Drop the unnecessary blank line.
> 
> > +               if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> > +                       delete_auto_created = 1;
> 
> I was wondering if it would be more semantically correct to parse
> 'hex' into an 'oid' and compare them with oidcmp() rather than doing a
> string comparison of the hex values (though I'm not sure it will
> matter in practice).

Since I haven't spent too much time in the Git internals, the string 
representation feels more natural to me. And that's why I went this way 
subconsciously. While I don't mind either, I wonder if it would make a 
difference in practice. Anyway, if you have a preference for the other 
way round, I'll trust your gut feeling.
 
> > +       }
> > +
> > +       strbuf_release(&sb);
> > +       strbuf_release(&hex);
> 
> Drop the unnecessary blank line.
> 
> > @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > +       if (delete_auto_created) {
> > +               struct child_process cp = CHILD_PROCESS_INIT;
> > +               cp.git_cmd = 1;
> > +
> > +               argv_array_push(&cp.args, "branch");
> > +               argv_array_push(&cp.args, "-d");
> > +               argv_array_push(&cp.args, wt->id);
> > +
> > +               ret |= run_command(&cp);
> > +       }
> 
> Alternately:
> 
>     argv_array_pushl(&cp.args, "branch", "-d", wt->id, NULL);

Ok.
 
> However, I don't think it is correct to use 'wt->id' here as the
> branch name since there is no guarantee that the <id> in
> .git/worktrees/<id>/ matches the branch name with which the worktree
> was created. For instance:
> 
>     git worktree add foo/bar existing-branch
>     git worktree add baz/bar
> 
> will, due to name conflicts, create worktree metadata directories:
> 
>     .git/worktrees/bar
>     .git/worktrees/bar1
> 
> where the first is associated with branch "existing-branch", and the
> second is associated with new branch "bar". When you then invoke "git
> worktree remove baz/bar", it will try removing a branch named "bar1",
> not "bar" as intended. To fix this, I think you need to record the
> original auto-created branch name in the "auto_created" metadata file
> too, not just the OID.

Interesting! Didn't think of a situation like this. Thanks for pointing 
it out. Will fix.
 
> Finally, make this code consistent with other existing similar code in
> this file by dropping the unnecessary blank lines in this hunk.

The blank lines are a personal preference for me mostly. I am not a huge 
fan of seeing large chunks of code with do blank lines in between. IMO 
it hurts readability. But, I think staying consistent with the code that 
already exists is more important. Will remove all them.
 
> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> > @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '
> > +test_expect_success 'remove auto-created branch' '
> > +       (
> > +               git worktree add to-remove &&
> > +               git worktree remove to-remove &&
> > +               git branch -l to-remove >branch_list &&
> > +               test_line_count = 0 branch_list
> > +       )
> > +'
> 
> I don't think there is any need for this test to be run in a subshell,
> so you can drop the enclosing '(' and ')'.

I was following the pattern in the two tests above. Will drop the 
parentheses.
 
> I worry about using porcelain git-branch to check whether the branch
> has actually been removed. Using git-rev-parse would likely be a more
> direct and safe way to test it. For instance:
> 
>     git worktree add to-remove &&
>     git worktree remove to-remove &&
>     test_must_fail git rev-parse --verify -q to-remove
> 
> should be sufficient, I think. And, to be really thorough, you might say:
> 
>     test_might_fail git branch -D to-remove &&
>     git worktree add to-remove &&
>     git rev-parse --verify -q to-remove &&
>     git worktree remove to-remove &&
>     test_must_fail git rev-parse --verify -q to-remove
> 
> The above comments apply to the other new tests added by this patch, as well.

Will fix.
 
> > +test_expect_success 'do not remove a branch that was not auto-created' '
> > +       (
> > +               git worktree add -b new_branch to-remove &&
> 
> Nit: The inconsistent mix of underscore and hyphen in names is odd.
> Perhaps settle on one or the other (with a slight preference toward
> hyphen).

I'll change 'new_branch' to 'new-branch'.
 
> > +               git worktree remove to-remove &&
> > +               git branch -l new_branch >branch_list &&
> > +               test_line_count = 1 branch_list &&
> 
> As noted earlier, although this particular case of a branch created
> explicitly with -b works as expected by not deleting the branch, the
> similar case:
> 
>     git worktree add -b to-remove to-remove &&
> 
> will incorrectly automatically delete the branch.
> 
> > +               git branch -d new_branch &&
> > +               git branch foo &&
> > +               git worktree add to-remove foo &&
> > +               git worktree remove to-remove &&
> > +               git branch -l foo >branch_list &&
> > +               test_line_count = 1 branch_list &&
> > +               git branch -d foo &&
> > +               git branch to-remove &&
> > +               git worktree add to-remove &&
> > +               git worktree remove to-remove &&
> > +               git branch -l to-remove >branch_list &&
> > +               test_line_count = 1 branch_list &&
> > +               git branch -d to-remove
> 
> If any code above this "git branch -d" fails, then it will never get
> this far, thus won't remove "to-remove". To perform cleanup whether
> the test succeeds or fails, you should use test_when_finished()
> _early_ in the test:
> 
>     test_when_finished "git branch -d to-remove || :" &&
> 
> However, if you restructure the tests as suggested above, then you
> might be able to get away without bothering with this cleanup.
> 
> > +       )
> > +'
> 
> This test is checking three distinct cases of explicitly-created
> branches. It would make it easier to debug a failing case if you split
> it up into three tests -- one for each case.

I considered doing it, but then I thought maybe I shouldn't add so many 
tests. And since there are only 3 rather independent cases, it would not 
be that difficult to figure out which one is the culprit. Will split 
them.
 
> > +test_expect_success 'do not remove auto-created branch that was moved' '
> > +       (
> > +               git worktree add to-remove &&
> > +               cd to-remove &&
> > +               test_commit foo &&
> > +               cd ../ &&
> 
> We normally avoid cd'ing around in tests like this because it can
> cause tests following this one to run in the wrong directory if
> something above the "cd ../" fails. In this particular case, it
> doesn't matter since the entire body of this test is within a
> subshell.
> 
> However, if you take advantage of test_commits()'s -C argument, then
> you can ditch the cd's and the subshell altogether:
> 
>     test_commit -C to-remove foo &&

Ok.
 
> > +               git worktree remove to-remove &&
> > +               git branch -l to-remove >branch_list &&
> > +               test_line_count = 1 branch_list &&
> > +               git branch -D to-remove
> > +       )
> > +'

I'll send out the v2 as soon as I can. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Eric Sunshine]

On Sat, Jan 4, 2020 at 4:47 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> On 27/12/19 06:05AM, Eric Sunshine wrote:
> > On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > > Remove that branch when removing the worktree. To make sure no commits
> > > are lost, the branch won't be deleted if it has moved.
> >
> > My knee-jerk reaction upon reading the first sentence of this
> > paragraph was that this is a significant and undesirable behavior
> > change, however, the second sentence helps to allay my fears about it.
> > It's possible, I suppose, that there is some existing tooling
> > somewhere which relies upon the current behavior, but it's hard to
> > imagine any good reason to do so.
>
> It is possible that some script somewhere does
>
>  git worktree add foo
>  do_something # doesn't move the branch
>  git worktree remove foo
>  git branch -d foo
>
> Branch deletion would fail here, which might be considered as an error
> by the script. Not sure how common that would be though.

Good point. That's a quite believable scenario for a scripting case.
Even if the script itself doesn't check for an error from git-branch,
people could get annoyed if their scripts suddenly start complaining
"error: branch 'foo' not found". So, that's a genuine concern.

> > However, there is a rather serious flaw in the implementation. My
> > expectation is that it should only automatically delete a branch if
> > the branch creation was inferred; it should never automatically delete
> > a branch which was created explicitly. You kind of have this covered
> > (and even have a test for it), but it doesn't work correctly when the
> > user explicitly requests branch creation via -b/-B and the branch name
> > matches the worktree name. For instance:
> >
> >   git worktree add -b foo foo
> >   git worktree remove foo
> >
> > incorrectly automatically removes branch "foo" even though the user
> > requested its creation explicitly.
>
> Thanks for pointing it out. Will fix.

Note that this almost certainly deserves an extra test in t2403 since
the tests added by v1 didn't catch this problem.

> > Subjectively, it seems more natural to fully discuss automatic branch
> > removal here rather than referring to the discussion of "worktree
> > add".
>
> I considered doing this but then left that part in 'add' because the
> conditions in which the branch is auto deleted are described pretty well
> in add's documentation. Will move it to 'remove'.

In retrospect, I don't feel strongly about it one way or the other. It
just surprised me to find it discussed under "add" on my first
read-through. And if you do patch "prune" to also auto-remove an
auto-created branch, then "add" might be the better place for the
discussion anyhow.

> > A bit of bikeshedding regarding the filename: "auto_created" is rather
> > unusual. Most names in the .git hierarchy are short and sweet. Also,
> > with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
> > filenames seem to use hyphen rather than underscore, which suggests
> > "auto-created" would be a better choice. However, I'd probably drop
> > the hyphen altogether. Finally, "auto_created", alone, does not
> > necessarily convey that the branch was auto-created; someone could
> > misinterpret it as meaning the worktree itself was auto-created, so I
> > wonder if a better name can be found.
>
> Any suggestions? Does "implicitbranch"/"implicit-branch" sound any
> better? How about "branch-auto-created-at"? It is very clear but is a
> mouthful.

Taking into account the suggestion from my review that you likely also
will need to store in this file the name of the auto-created branch
(not just the original OID of that branch), then the nature of this
file changes a bit, which might help suggest a better name.
"implicitbranch" and "implicit-branch" are not bad, though a bit of a
mouthful. What about "autobranch"?

> > A bigger question, though, is whether we really want to see new files
> > like this springing up in the .git/worktrees/<id>/ directory for each
> > new piece of metadata which belongs to a worktree. I ask because this
> > isn't the first such case in which some additional worktree-specific
> > metadata was proposed (see, for instance, [1]). So, I'm wondering if
> > we should have a more generalized solution, such as introducing a new
> > file which can hold any sort of metadata which comes along in the
> > future. In particular, I'm thinking about a file containing an
> > extensible set of "key: value" tuples, in which case the "auto
> > created" metadata would be just one of possibly many keys.
>
> Do you worry that the number of metadata files might grow to be too
> large? I can't say how worktrees will grow in the future, but right now
> there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD').
> So, not a lot.

I'm not particularly worried about the number of files. A couple
thoughts I had in mind are: (1) other tools or non-canonical Git
implementations (jgit, libgit, etc.) may be poking around inside the
.git/worktrees/<id>/ directory, and (2) the information represented by
this new file may deserve inclusion in the output of "git worktree
list --porcelain".

It was #1, in particular, I think, which got me thinking of having a
standardized format (i.e. extensible "key: value" list) for worktree
metainfo added in the future. It would require a one-time cost for
each tool/library to implement, and would then effectively be free as
more metainfo is added to the worktree. Compare that with having to
write a reader/parser for each new metainfo file added (not that these
files are terribly difficult to parse).

Similarly, a standardized format simplifies #2, extending "git
worktree list --porcelain" to output additional metainfo.

By the way, I wouldn't mind seeing "git worktree list --porcelain"
extended to output this new information, but I don't insist upon it as
a requirement of this patch; it can easily be done later if the need
arises. (In fact, the --porcelain documentation is so woefully lacking
and under-specified that it needs an overhaul, which I think deserves
a patch series of its own, thus is another reason I don't really
expect/want to see that change made by this patch.)

> I chose to add a new file because from what I have noticed, Git keeps a
> lot of metadata in files like this (HEAD, refs, etc). Do other
> subsystems use a key-value store? What problems did they face?
>
> I'd prefer to not take on this feature (since I expect it to be a lot of
> work), but if there are strong opinions on using a key-value store then
> I guess I'll bite the bullet.

I brought up the idea of a standardized extensible "key: value" store
because, having that older email thread in mind, it was the first
thought which popped into my mind when I saw this patch introducing a
new file in .git/worktrees/<id>/. However, one can make a good
argument that Git already has such a standard, and that that standard
is simply using individual files like "HEAD", "gitdir", "commondir",
etc. So, I think I'm pretty comfortable with the idea of storing this
information in a new file as this patch does. After all, there's
plenty of precedent in Git for doing it that way.

> > > +    if (auto_create) {
> > > +        fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > > +        get_oid("HEAD", &oid);
> >
> > Unless I'm mistaken, this is just wrong. You're grabbing the OID of
> > HEAD from the worktree in which "worktree add" is being invoked,
> > however, if the new branch name is DWIM'd from an existing
> > tracking-branch, then the OID should be that of the tracking-branch,
> > not HEAD of the current worktree. So, you should be using the OID
> > already looked up earlier in the function, 'commit->object.oid', which
> > should be correct for either case.
>
> Oops! Thanks for pointing it out. Will fix.

This deserves a new test in t2403, as well (or perhaps two new tests
since there are a couple different ways the starting OID can be
DWIM'd, if I'm reading the code correctly).

> > By the way, this suffers from the problem that if "git worktree add
> > foo" fails for some reason, such as because path "foo" already exists,
> > then the new branch will _not_ be cleaned up automatically since that
> > failure will happen before "auto_created" is ever created (among other
> > reasons). But that's not a new issue; it's an existing flaw of
> > "worktree add" not cleaning up a branch it created before it discovers
> > that it can't actually create the target directory for some reason, so
> > I wouldn't expect you to fix that problem with this submission. (I'm
> > just mentioning it for completeness.)
>
> I'll see if I can come up with a fix for this as a follow-up patch.

If fixing this ends up being more involved than a relatively minor
change -- and I think it will be quite a bit more involved due to all
the die()ing going on inside add_worktree() -- then I could easily see
(and probably would prefer) it being a separate patch series (since
the changes made by the patch under discussion are already
sufficiently involved as to eat up a good deal of reviewer time).

> > > +    git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> > > +    if (file_exists(sb.buf)) {
> > > +        strbuf_read_file(&hex, sb.buf, 0);
> >
> > You can avoid an unnecessary race condition here by dropping the
> > file_exists() call altogether and just checking the return code of
> > strbuf_read_file() -- which you should probably be doing anyhow. If
> > strbuf_read_file() returns a non-negative value, then you know it
> > exists, so file_exists() is redundant.
>
> Will fix. Though I don't see how it would be a "race condition". Is
> file_exists() asynchronous in some way? Otherwise, how would a race
> happen and between what?

I think I was remembering an earlier[1] issue with someone running a
bunch of git-worktree commands in parallel and encountering races, but
that shouldn't apply here. Still, the code will be cleaner by dropping
file_exists() altogether.

[1]: https://lore.kernel.org/git/cover.1550508544.git.msuchanek@suse.de/T/

> > > +        if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> > > +            delete_auto_created = 1;
> >
> > I was wondering if it would be more semantically correct to parse
> > 'hex' into an 'oid' and compare them with oidcmp() rather than doing a
> > string comparison of the hex values (though I'm not sure it will
> > matter in practice).
>
> Since I haven't spent too much time in the Git internals, the string
> representation feels more natural to me. And that's why I went this way
> subconsciously. While I don't mind either, I wonder if it would make a
> difference in practice. Anyway, if you have a preference for the other
> way round, I'll trust your gut feeling.

It may not matter in practice, but having considered it further, I
really would prefer it to be semantically correct by comparing the
OIDs directly rather than the string representations. With the process
underway of updating Git to be able to work with multiple hash
functions (and moving away from SHA-1), making use of get_oid_hex()
and oideq() to perform this comparison may make the job of auditing
the code for multi-hash-function friendliness easier.

> > However, I don't think it is correct to use 'wt->id' here as the
> > branch name since there is no guarantee that the <id> in
> > .git/worktrees/<id>/ matches the branch name with which the worktree
> > was created. For instance:
> >
> >   git worktree add foo/bar existing-branch
> >   git worktree add baz/bar
> >
> > will, due to name conflicts, create worktree metadata directories:
> >
> >   .git/worktrees/bar
> >   .git/worktrees/bar1
> >
> > where the first is associated with branch "existing-branch", and the
> > second is associated with new branch "bar". When you then invoke "git
> > worktree remove baz/bar", it will try removing a branch named "bar1",
> > not "bar" as intended. To fix this, I think you need to record the
> > original auto-created branch name in the "auto_created" metadata file
> > too, not just the OID.
>
> Interesting! Didn't think of a situation like this. Thanks for pointing
> it out. Will fix.

Definitely deserves a test in t2403.

> > > +test_expect_success 'remove auto-created branch' '
> > > +    (
> > > +        git worktree add to-remove &&
> > > +        git worktree remove to-remove &&
> > > +        git branch -l to-remove >branch_list &&
> > > +        test_line_count = 0 branch_list
> > > +    )
> > > +'
> >
> > I don't think there is any need for this test to be run in a subshell,
> > so you can drop the enclosing '(' and ')'.
>
> I was following the pattern in the two tests above. Will drop the
> parentheses.

The existing tests use a subshell (the parentheses) because they 'cd'
around, and use of a subshell ensures that subsequent tests won't
adversely run in the wrong directory if the test fails for some reason
(since the affect of the 'cd' does not last beyond the end of the
subshell). As long as you're not cd'ing around (or doing a few other
questionable things), there's no need for the subshell.

> > > +test_expect_success 'do not remove a branch that was not auto-created' '
> > > +    (
> > > +        git worktree add -b new_branch to-remove &&
> >
> > Nit: The inconsistent mix of underscore and hyphen in names is odd.
> > Perhaps settle on one or the other (with a slight preference toward
> > hyphen).
>
> I'll change 'new_branch' to 'new-branch'.

As mentioned above, also add a test in which the explicitly-created
new branch has the same name as the worktree itself (since that case
was implemented wrong in v1 but the tests didn't catch the failure).

> I'll send out the v2 as soon as I can. Thanks.

There's no rush. Let's get the details and any lingering questions
worked out before subjecting reviewers to a new version (especially,
as my review time is somewhat limited these days).

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Eric Sunshine]

On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > When no branch name is supplied to 'worktree add', it creates a new
> > branch based on the name of the directory the new worktree is located
> > in. But when the worktree is later removed, that created branch is left
> > over.
>
> This is describing the existing (intentional) behavior but doesn't
> explain why this might be annoying or problematic. To help sell the
> patch, it might make sense to say something about how the behavior can
> trip up newcomers to git-worktree, leaving them to wonder why they are
> accumulating so many branches that they weren't aware they created. A
> comment about why you think "git worktree add -d foo" is not a viable
> way to side-step the creation of unwanted branches might also be
> worthwhile.

As an alternative to this patch, would the simpler approach of
improving git-worktree documentation to do a better job of pointing
people at -d/--detach as a way to side-step unwanted branch creation
make sense? That is, at minimum, enhance the "Description" section to
prominently talk about throwaway worktrees (created with -d/--detach),
and add an example to the "Examples" section (perhaps as the first
example) showing creation/use/deletion of a throwaway worktree.

Some points in favor of just updating the documentation to address
this issue (rather than implementing the new behavior suggested by
this patch) include:

* far simpler; no code to implement or debug

* no (surprising) behavior changes

* "git worktree add -d foo" is about as easy to type and remember as
  "git worktree add foo"

Of lesser importance, it might make sense, as a followup, to add a
configuration which changes the default behavior to detach instead of
auto-creating a branch. I wonder if this could be piggybacked on the
existing "worktree.guessremote" configuration. Or rather,
retire/deprecate that configuration and add a new one which affects
DWIM'ing behavior such that it becomes multi-state. Some possible
values for the new configuration: "auto" (or "dwim" or whatever),
"guessremote", "detach". (I haven't thought this through thoroughly,
so there might be holes in my suggestion.)

There's at least one point not in favor of merely updating the
documentation to promote -d/--detach more heavily, and that is that
(presumably) the concept of detached HEAD is perceived as an advanced
topic, so it may not be suitable for the newcomer or casual user.

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Pratyush Yadav]

On 05/01/20 11:20PM, Eric Sunshine wrote:
> On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > > When no branch name is supplied to 'worktree add', it creates a new
> > > branch based on the name of the directory the new worktree is located
> > > in. But when the worktree is later removed, that created branch is left
> > > over.
> >
> > This is describing the existing (intentional) behavior but doesn't
> > explain why this might be annoying or problematic. To help sell the
> > patch, it might make sense to say something about how the behavior can
> > trip up newcomers to git-worktree, leaving them to wonder why they are
> > accumulating so many branches that they weren't aware they created. A
> > comment about why you think "git worktree add -d foo" is not a viable
> > way to side-step the creation of unwanted branches might also be
> > worthwhile.
> 
> As an alternative to this patch, would the simpler approach of
> improving git-worktree documentation to do a better job of pointing
> people at -d/--detach as a way to side-step unwanted branch creation
> make sense? That is, at minimum, enhance the "Description" section to
> prominently talk about throwaway worktrees (created with -d/--detach),
> and add an example to the "Examples" section (perhaps as the first
> example) showing creation/use/deletion of a throwaway worktree.
> 
> Some points in favor of just updating the documentation to address
> this issue (rather than implementing the new behavior suggested by
> this patch) include:
> 
> * far simpler; no code to implement or debug
> 
> * no (surprising) behavior changes
> 
> * "git worktree add -d foo" is about as easy to type and remember as
>   "git worktree add foo"

FYI, I'm running Git v2.24.1 and 'git worktree add' doesn't accept the 
option '-d'. It only accepts '--detach'. And looking at the current 
'next', I don't see the option mentioned in git-worktree.txt. So at the 
very least, we should start by actually adding the option.
 
> Of lesser importance, it might make sense, as a followup, to add a
> configuration which changes the default behavior to detach instead of
> auto-creating a branch. I wonder if this could be piggybacked on the
> existing "worktree.guessremote" configuration. Or rather,
> retire/deprecate that configuration and add a new one which affects
> DWIM'ing behavior such that it becomes multi-state. Some possible
> values for the new configuration: "auto" (or "dwim" or whatever),
> "guessremote", "detach". (I haven't thought this through thoroughly,
> so there might be holes in my suggestion.)

Honestly, coupled with a configuration variable this alternative fits my 
use-case really well.

I think 'guessremote' does not describe very well what the config 
variable would actually do. So I think deprecating it would be a better 
idea.

Does 'worktree.newBranch' sound like a good name? (Disclaimer: I am 
terrible at naming things).
 
> There's at least one point not in favor of merely updating the
> documentation to promote -d/--detach more heavily, and that is that
> (presumably) the concept of detached HEAD is perceived as an advanced
> topic, so it may not be suitable for the newcomer or casual user.

I'm basing this off no data so take it with a grain of salt, but I think 
people who know Git enough to understand the concept of multiple 
worktrees should also understand what a detached HEAD is. And even if 
they already don't know what it is, they should have no trouble quickly 
reading one of the many great explanations available with a simple 
Google search.

My argument in favor of auto-deletion is that we should still try to 
have sane defaults. Leaving behind a branch the user didn't explicitly 
create and didn't use doesn't sound like a sane default to me.

The configuration variable path is easier and suits my needs really 
well, so I am inclined to just go with it. But making the whole user 
experience better for everyone is still something worthwhile. But then 
again, introducing a backwards-incompatible change might not be the best 
idea. So, I dunno.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'

[Eric Sunshine]

On Mon, Jan 6, 2020 at 1:01 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> On 05/01/20 11:20PM, Eric Sunshine wrote:
> > On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > As an alternative to this patch, would the simpler approach of
> > improving git-worktree documentation to do a better job of pointing
> > people at -d/--detach as a way to side-step unwanted branch creation
> > make sense? That is, at minimum, enhance the "Description" section to
> > prominently talk about throwaway worktrees (created with -d/--detach),
> > and add an example to the "Examples" section (perhaps as the first
> > example) showing creation/use/deletion of a throwaway worktree.
>
> FYI, I'm running Git v2.24.1 and 'git worktree add' doesn't accept the
> option '-d'. It only accepts '--detach'. And looking at the current
> 'next', I don't see the option mentioned in git-worktree.txt. So at the
> very least, we should start by actually adding the option.

I forgot that -d was never added as shorthand for --detach, and didn't
bother checking the man page. But, yes, adding -d would be a good
start.

> > Of lesser importance, it might make sense, as a followup, to add a
> > configuration which changes the default behavior to detach instead of
> > auto-creating a branch. I wonder if this could be piggybacked on the
> > existing "worktree.guessremote" configuration. Or rather,
> > retire/deprecate that configuration and add a new one which affects
> > DWIM'ing behavior such that it becomes multi-state. Some possible
> > values for the new configuration: "auto" (or "dwim" or whatever),
> > "guessremote", "detach". (I haven't thought this through thoroughly,
> > so there might be holes in my suggestion.)
>
> Honestly, coupled with a configuration variable this alternative fits my
> use-case really well.
>
> I think 'guessremote' does not describe very well what the config
> variable would actually do. So I think deprecating it would be a better
> idea.
>
> Does 'worktree.newBranch' sound like a good name? (Disclaimer: I am
> terrible at naming things).

Maybe 'worktree.addFlags' or something? I'm thinking that this might
be a multi-value configuration variable which is a combination of the
various option flags which can be used with "git worktree add". For
instance: 'worktree.addFlags=detach' or
worktree.addFlags=auto-create-branch,guess-remote. Possible values
might include:

[no-]auto-create-branch
    enable/disable automatic branch creation when <commit-ish> is
    omitted

detach
    create worktree with detached HEAD

[no-]checkout
    perform/suppress checkout of <commit-ish> in the new worktree

[no-]guess-remote
    create local branch from remote-tracking branch if present and
    <commit-ish> omitted

[no-]track
    make new branch track <commit-ish> if the latter is a branch name

[no-]lock
    keep worktree locked after creation

In fact, I'd like to see 'auto-create-branch' incorporate
'guess-remote' behavior by default since "remote guessing" should have
been the default behavior from day one, but it was overlooked. The
--guess-remote option was added simply to avoid backward compatibility
problems, but it would be nice to one day make it the default. Since
this configuration variable is new, we don't have to worry about
backward compatibility with it, thus can make 'auto-create-branch'
work like it should have from inception -- that is, performing "remote
guessing" DWIMing (just like "git checkout" does by default).

A command-line option would (as expected) override a flag set via
'worktree.addFlags'. So, for instance, --no-detach would override
'worktree.addFlags=detach'.

Anyhow, this is just a rough idea. I haven't thought through all the
ramifications, or even if this is a sane interface.

> > There's at least one point not in favor of merely updating the
> > documentation to promote -d/--detach more heavily, and that is that
> > (presumably) the concept of detached HEAD is perceived as an advanced
> > topic, so it may not be suitable for the newcomer or casual user.
>
> I'm basing this off no data so take it with a grain of salt, but I think
> people who know Git enough to understand the concept of multiple
> worktrees should also understand what a detached HEAD is. And even if
> they already don't know what it is, they should have no trouble quickly
> reading one of the many great explanations available with a simple
> Google search.

I don't necessarily share that opinion, but I do think that if we add
-d as shorthand for --detach, and do a really good job of updating the
documentation to promote the idea of "throwaway worktrees" (which also
happen to be detached), then we have a good path forward.

> My argument in favor of auto-deletion is that we should still try to
> have sane defaults. Leaving behind a branch the user didn't explicitly
> create and didn't use doesn't sound like a sane default to me.
>
> The configuration variable path is easier and suits my needs really
> well, so I am inclined to just go with it. But making the whole user
> experience better for everyone is still something worthwhile. But then
> again, introducing a backwards-incompatible change might not be the best
> idea. So, I dunno.

Yep, the different ideas can co-exist, and each can be implemented
without promising to implement the others. A good first step would be
to add -d as alias for --detach and update the documentation to
promote the concept of "throwaway worktrees". An optional second step
(if needed) would be that new configuration variable (though it still
needs more thought). And, a really optional third step (if anyone
cares strongly enough) would be to implement auto-deletion of
auto-created branches.