{"thread":{"id":"52458","subject":"[PATCH 0/1] worktree: delete branches auto-created by 'worktree add'","startedAt":"2019-12-14T16:15:16Z","lastAt":"2020-01-09T09:47:12Z","messageCount":10,"participants":["Pratyush Yadav","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"388210","messageId":"20191214161438.16157-1-me@yadavpratyush.com","threadId":"52458","inReplyTo":null,"subject":"[PATCH 0/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-12-14T16:14:37Z","receivedAt":"2019-12-14T16:15:16Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi,\n\nThis fixes a small annoyance I had with git-worktree. Most of the\nfeature is explained in the patch, so I'm using the cover letter to\nleave a couple of notes.\n\n- Since the patch changes current behaviour of git-worktree-remove, it\n might break existing scripts. In that case, we might want to add a\n config variable and command line option to trigger that. But for the\n sake of simplicity, I went with making the behaviour default. I don't\n mind making it optional if people think that would be a better idea.\n\n- To make sure no commits are lost, the branch is not deleted if it has\n moved since its creation. This is a more conservative approach. An\n alternative would be to run 'git branch -d' directly without checking if\n branch has moved. This will mean new commits not in upstream branch are\n still preserved but if the branch is simply moved to another commit it\n will still be deleted.\n\nPratyush Yadav (1):\n worktree: delete branches auto-created by 'worktree add'\n\n Documentation/git-worktree.txt | 9 ++++--\n builtin/worktree.c | 52 ++++++++++++++++++++++++++++++++--\n t/t2403-worktree-move.sh | 45 +++++++++++++++++++++++++++++\n 3 files changed, 101 insertions(+), 5 deletions(-)\n\n--\n2.24.1\n\n"},{"id":"388211","messageId":"20191214161438.16157-2-me@yadavpratyush.com","threadId":"52458","inReplyTo":"20191214161438.16157-1-me@yadavpratyush.com","subject":"[PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-12-14T16:14:38Z","receivedAt":"2019-12-14T16:15:29Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"When no branch name is supplied to 'worktree add', it creates a new\nbranch based on the name of the directory the new worktree is located\nin. But when the worktree is later removed, that created branch is left\nover.\n\nRemove that branch when removing the worktree. To make sure no commits\nare lost, the branch won't be deleted if it has moved.\n\nAn example use case of when something like this is useful is when the\nuser wants to check out a separate worktree to run and test on an older\nversion, but don't want to touch the current worktree. So, they create a\nworktree, run some tests, and then remove it. But this leaves behind a\nbranch the user never created in the first place.\n\nSo, remove the branch if nothing was done on it.\n\nSigned-off-by: Pratyush Yadav \n---\n Documentation/git-worktree.txt | 9 ++++--\n builtin/worktree.c | 52 ++++++++++++++++++++++++++++++++--\n t/t2403-worktree-move.sh | 45 +++++++++++++++++++++++++++++\n 3 files changed, 101 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 85d92c9761..87b84be608 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -73,8 +73,9 @@ If `` is omitted and neither `-b` nor `-B` nor `--detach` used,\n then, as a convenience, the new worktree is associated with a branch\n (call it ``) named after `$(basename )`. If ``\n doesn't exist, a new branch based on HEAD is automatically created as\n-if `-b ` was given. If `` does exist, it will be\n-checked out in the new worktree, if it's not checked out anywhere\n+if `-b ` was given. In this case, if `` is not moved, it is\n+automatically deleted when the worktree is removed. If `` does exist,\n+it will be checked out in the new worktree, if it's not checked out anywhere\n else, otherwise the command will refuse to create the worktree (unless\n `--force` is used).\n \n@@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files\n and no modification in tracked files) can be removed. Unclean working\n trees or ones with submodules can be removed with `--force`. The main\n working tree cannot be removed.\n++\n+Removing a working tree might lead to its associated branch being deleted if\n+it was auto-created and has not moved since. See `add` for more information on\n+when exactly this can happen.\n \n unlock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d6bc5263f1..c62811259a 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -35,6 +35,7 @@ struct add_opts {\n static int show_only;\n static int verbose;\n static int guess_remote;\n+static int auto_create;\n static timestamp_t expire;\n \n static int git_worktree_config(const char *var, const char *value, void *cb)\n@@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n \tunsigned int counter = 0;\n-\tint len, ret;\n+\tint len, ret, fd;\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n \tstruct strbuf sb_name = STRBUF_INIT;\n+\tstruct object_id oid;\n+\tchar *hex;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s/auto_created\", sb_repo.buf);\n+\t/* Mark this branch as an \"auto-created\" one. */\n+\tif (auto_create) {\n+\t\tfd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\t\tget_oid(\"HEAD\", &oid);\n+\t\thex = oid_to_hex(&oid);\n+\t\twrite_file_buf(sb.buf, hex, strlen(hex));\n+\n+\t\tif (close(fd))\n+\t\t\tdie(_(\"could not close '%s'\"), sb.buf);\n+\t}\n \n \targv_array_pushf(&child_env, \"%s=%s\", GIT_DIR_ENVIRONMENT, sb_git.buf);\n \targv_array_pushf(&child_env, \"%s=%s\", GIT_WORK_TREE_ENVIRONMENT, path);\n@@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tif (run_command(&cp))\n \t\t\treturn -1;\n \t\tbranch = new_branch;\n+\n+\t\tauto_create = 1;\n \t} else if (opt_track) {\n \t\tdie(_(\"--[no-]track can only be used if a new branch is created\"));\n \t}\n@@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\tOPT_END()\n \t};\n \tstruct worktree **worktrees, *wt;\n-\tstruct strbuf errmsg = STRBUF_INIT;\n+\tstruct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;\n \tconst char *reason = NULL;\n-\tint ret = 0;\n+\tint ret = 0, delete_auto_created = 0;\n+\tstruct object_id oid;\n \n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n \tif (ac != 1)\n@@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n \n+\t/*\n+\t * Check if we auto-created a branch for this worktree and it hasn't\n+\t * moved since. Do it before the contents of the worktree get wiped.\n+\t * Delete the branch later because it is checked out right now.\n+\t */\n+\tgit_path_buf(&sb, \"worktrees/%s/auto_created\", wt->id);\n+\tif (file_exists(sb.buf)) {\n+\t\tstrbuf_read_file(&hex, sb.buf, 0);\n+\t\tget_oid(wt->id, &oid);\n+\n+\t\tif (strcmp(hex.buf, oid_to_hex(&oid)) == 0)\n+\t\t\tdelete_auto_created = 1;\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\tstrbuf_release(&hex);\n+\n \tif (file_exists(wt->path)) {\n \t\tif (!force)\n \t\t\tcheck_clean_worktree(wt, av[0]);\n@@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \tret |= delete_git_dir(wt->id);\n \tdelete_worktrees_dir_if_empty();\n \n+\tif (delete_auto_created) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tcp.git_cmd = 1;\n+\n+\t\targv_array_push(&cp.args, \"branch\");\n+\t\targv_array_push(&cp.args, \"-d\");\n+\t\targv_array_push(&cp.args, wt->id);\n+\n+\t\tret |= run_command(&cp);\n+\t}\n+\n \tfree_worktrees(worktrees);\n \treturn ret;\n }\ndiff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\nindex 939d18d728..c71c0bc1c7 100755\n--- a/t/t2403-worktree-move.sh\n+++ b/t/t2403-worktree-move.sh\n@@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '\n \t)\n '\n \n+test_expect_success 'remove auto-created branch' '\n+\t(\n+\t\tgit worktree add to-remove &&\n+\t\tgit worktree remove to-remove &&\n+\t\tgit branch -l to-remove >branch_list &&\n+\t\ttest_line_count = 0 branch_list\n+\t)\n+'\n+\n+test_expect_success 'do not remove a branch that was not auto-created' '\n+\t(\n+\t\tgit worktree add -b new_branch to-remove &&\n+\t\tgit worktree remove to-remove &&\n+\t\tgit branch -l new_branch >branch_list &&\n+\t\ttest_line_count = 1 branch_list &&\n+\t\tgit branch -d new_branch &&\n+\t\tgit branch foo &&\n+\t\tgit worktree add to-remove foo &&\n+\t\tgit worktree remove to-remove &&\n+\t\tgit branch -l foo >branch_list &&\n+\t\ttest_line_count = 1 branch_list &&\n+\t\tgit branch -d foo &&\n+\t\tgit branch to-remove &&\n+\t\tgit worktree add to-remove &&\n+\t\tgit worktree remove to-remove &&\n+\t\tgit branch -l to-remove >branch_list &&\n+\t\ttest_line_count = 1 branch_list &&\n+\t\tgit branch -d to-remove\n+\t)\n+'\n+\n+test_expect_success 'do not remove auto-created branch that was moved' '\n+\t(\n+\t\tgit worktree add to-remove &&\n+\t\tcd to-remove &&\n+\t\ttest_commit foo &&\n+\t\tcd ../ &&\n+\t\tgit worktree remove to-remove &&\n+\t\tgit branch -l to-remove >branch_list &&\n+\t\ttest_line_count = 1 branch_list &&\n+\t\tgit branch -D to-remove\n+\t)\n+'\n+\n+\n test_done\n-- \n2.24.1\n\n"},{"id":"388468","messageId":"20191218193129.hnvaetebih4y2slt@yadavpratyush.com","threadId":"52458","inReplyTo":"20191214161438.16157-2-me@yadavpratyush.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-12-18T19:31:29Z","receivedAt":"2019-12-18T19:31:38Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 14/12/19 09:44PM, Pratyush Yadav wrote:\n> When no branch name is supplied to 'worktree add', it creates a new\n> branch based on the name of the directory the new worktree is located\n> in. But when the worktree is later removed, that created branch is left\n> over.\n> \n> Remove that branch when removing the worktree. To make sure no commits\n> are lost, the branch won't be deleted if it has moved.\n> \n> An example use case of when something like this is useful is when the\n> user wants to check out a separate worktree to run and test on an older\n> version, but don't want to touch the current worktree. So, they create a\n> worktree, run some tests, and then remove it. But this leaves behind a\n> branch the user never created in the first place.\n> \n> So, remove the branch if nothing was done on it.\n> \n> Signed-off-by: Pratyush Yadav \n\nPing?\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"388470","messageId":"CAPig+cQjL3Q0JFFK86+5Ee_-g_gmuiAD8f_kx5Z5NxG2ZEP7+Q@mail.gmail.com","threadId":"52458","inReplyTo":"20191218193129.hnvaetebih4y2slt@yadavpratyush.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-18T19:34:06Z","receivedAt":"2019-12-18T19:34:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 18, 2019 at 2:31 PM Pratyush Yadav wrote:\n> On 14/12/19 09:44PM, Pratyush Yadav wrote:\n> > When no branch name is supplied to 'worktree add', it creates a new\n> > branch based on the name of the directory the new worktree is located\n> > in. But when the worktree is later removed, that created branch is left\n> > over.\n> >\n> > Remove that branch when removing the worktree. To make sure no commits\n> > are lost, the branch won't be deleted if it has moved.\n>\n> Ping?\n\nI scanned the patch when you sent it and will have a number of\ncomments to make when I find time to review it formally.\n"},{"id":"388968","messageId":"CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com","threadId":"52458","inReplyTo":"20191214161438.16157-2-me@yadavpratyush.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-27T11:05:05Z","receivedAt":"2019-12-27T11:05:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"(Sorry for taking so long to review this patch; it ended up being a\nquite lengthy review.)\n\nOn Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav wrote:\n> When no branch name is supplied to 'worktree add', it creates a new\n> branch based on the name of the directory the new worktree is located\n> in. But when the worktree is later removed, that created branch is left\n> over.\n\nThis is describing the existing (intentional) behavior but doesn't\nexplain why this might be annoying or problematic. To help sell the\npatch, it might make sense to say something about how the behavior can\ntrip up newcomers to git-worktree, leaving them to wonder why they are\naccumulating so many branches that they weren't aware they created. A\ncomment about why you think \"git worktree add -d foo\" is not a viable\nway to side-step the creation of unwanted branches might also be\nworthwhile. For instance, you might say something about how newcomers\nmight not read the documentation thoroughly enough to know about\n--detach or to understand what it means; indeed, some newcomers to Git\npresumably have trouble with the concept of a detached HEAD and may\nfind it scary.\n\n> Remove that branch when removing the worktree. To make sure no commits\n> are lost, the branch won't be deleted if it has moved.\n\nMy knee-jerk reaction upon reading the first sentence of this\nparagraph was that this is a significant and undesirable behavior\nchange, however, the second sentence helps to allay my fears about it.\n\nIt's possible, I suppose, that there is some existing tooling\nsomewhere which relies upon the current behavior, but it's hard to\nimagine any good reason to do so. (That is, \"git worktree add foo &&\ngit worktree remove foo\" is just a glorified and expensive way to say\n\"git branch foo\".) So, I don't look upon this change with disfavor; it\ncould well be beneficial for newcomers, and perhaps a nice convenience\nin general.\n\nHowever, there is a rather serious flaw in the implementation. My\nexpectation is that it should only automatically delete a branch if\nthe branch creation was inferred; it should never automatically delete\na branch which was created explicitly. You kind of have this covered\n(and even have a test for it), but it doesn't work correctly when the\nuser explicitly requests branch creation via -b/-B and the branch name\nmatches the worktree name. For instance:\n\n git worktree add -b foo foo\n git worktree remove foo\n\nincorrectly automatically removes branch \"foo\" even though the user\nrequested its creation explicitly.\n\nAnother big question: Should an automatically-created branch be\ndeleted automatically when a worktree is pruned? That is, although\nthis sequence will remove an automatically-created branch:\n\n git worktree add foo\n git worktree remove foo\n\nthe current patch will not clean up the branch given this sequence:\n\n git worktree add foo\n rm -rf foo\n git worktree prune\n\nEither way, it might be worthwhile to update the documentation to mention this.\n\n> An example use case of when something like this is useful is when the\n> user wants to check out a separate worktree to run and test on an older\n> version, but don't want to touch the current worktree. So, they create a\n> worktree, run some tests, and then remove it. But this leaves behind a\n> branch the user never created in the first place.\n\nThe last sentence isn't exactly accurate. The user _did_ create the\nbranch. It would be more accurate to say \"...the user did not\nnecessarily _intentionally_ create...\" or something like that.\n\n> So, remove the branch if nothing was done on it.\n\nBy the way, the ordering of the commit message paragraphs is a bit\noff; it somewhat tries to justifies the change before explaining what\nthe problem is. I'd suggest this order:\n\n - describe current behavior\n - explain why current behavior can be undesirable in some circumstances;\n cite your example use-case here, perhaps\n - describe how this patch improves the situation\n\nThe two paragraphs which talk about \"remove the branch\" are just\nrepeating one another. I would drop one of them and keep the other as\nthe final bullet point of the suggested commit message order.\n\n> Signed-off-by: Pratyush Yadav \n> ---\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -73,8 +73,9 @@ If `` is omitted and neither `-b` nor `-B` nor `--detach` used,\n> doesn't exist, a new branch based on HEAD is automatically created as\n> -if `-b ` was given. If `` does exist, it will be\n> -checked out in the new worktree, if it's not checked out anywhere\n> +if `-b ` was given. In this case, if `` is not moved, it is\n> +automatically deleted when the worktree is removed. If `` does exist,\n> +it will be checked out in the new worktree, if it's not checked out anywhere\n\nI found it confusing to find automatic branch deletion described here\nunder the \"worktree add\" command...\n\n> @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files\n> +Removing a working tree might lead to its associated branch being deleted if\n> +it was auto-created and has not moved since. See `add` for more information on\n> +when exactly this can happen.\n\nSubjectively, it seems more natural to fully discuss automatic branch\nremoval here rather than referring to the discussion of \"worktree\nadd\".\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -35,6 +35,7 @@ struct add_opts {\n> +static int auto_create;\n\nI think this variable belongs in the 'add_opts' structure rather than\nbeing file-global.\n\n> @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,\n> - int len, ret;\n> + int len, ret, fd;\n> + struct object_id oid;\n> + char *hex;\n\nRather than declaring 'fd', 'oid', and 'hex' here, how about declaring\nthem in the scope of the \"if (auto_create) {\" conditional below, which\nis the only place they are used?\n\n> @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,\n> + strbuf_reset(&sb);\n> + strbuf_addf(&sb, \"%s/auto_created\", sb_repo.buf);\n\nWhy aren't these two lines inside the \"if (auto_create) {\" conditional\nbelow? They seem to be used only for that case.\n\nI think this new worktree metadata file warrants a documentation\nupdate. In particular, gitrepository-layout.txt talks about\nworktree-specific metadata files, and the \"Details\" section of\ngit-worktree.txt may need an update.\n\nA bit of bikeshedding regarding the filename: \"auto_created\" is rather\nunusual. Most names in the .git hierarchy are short and sweet. Also,\nwith the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word\nfilenames seem to use hyphen rather than underscore, which suggests\n\"auto-created\" would be a better choice. However, I'd probably drop\nthe hyphen altogether. Finally, \"auto_created\", alone, does not\nnecessarily convey that the branch was auto-created; someone could\nmisinterpret it as meaning the worktree itself was auto-created, so I\nwonder if a better name can be found.\n\nA bigger question, though, is whether we really want to see new files\nlike this springing up in the .git/worktrees// directory for each\nnew piece of metadata which belongs to a worktree. I ask because this\nisn't the first such case in which some additional worktree-specific\nmetadata was proposed (see, for instance, [1]). So, I'm wondering if\nwe should have a more generalized solution, such as introducing a new\nfile which can hold any sort of metadata which comes along in the\nfuture. In particular, I'm thinking about a file containing an\nextensible set of \"key: value\" tuples, in which case the \"auto\ncreated\" metadata would be just one of possibly many keys. For\ninstance:\n\n branch-auto-created-at: deadbeef\n\nThe above is a genuine question. I'm not demanding that this patch\nimplement it, but I think it deserves discussion and thought before\nmaking a decision.\n\n[1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/\n\n> + /* Mark this branch as an \"auto-created\" one. */\n\nThis comment doesn't really say anything which the code itself isn't\nalready saying (especially if you move the strbuf_addf() call inside\nthe conditional), so the comment could be dropped.\n\n> + if (auto_create) {\n> + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> + get_oid(\"HEAD\", &oid);\n\nUnless I'm mistaken, this is just wrong. You're grabbing the OID of\nHEAD from the worktree in which \"worktree add\" is being invoked,\nhowever, if the new branch name is DWIM'd from an existing\ntracking-branch, then the OID should be that of the tracking-branch,\nnot HEAD of the current worktree. So, you should be using the OID\nalready looked up earlier in the function, 'commit->object.oid', which\nshould be correct for either case.\n\n> + hex = oid_to_hex(&oid);\n> + write_file_buf(sb.buf, hex, strlen(hex));\n> +\n> + if (close(fd))\n> + die(_(\"could not close '%s'\"), sb.buf);\n> + }\n\nIs there a reason you're creating the file in this rather manual\nfashion rather than using write_file() as is already heavily used in\nthis code for creating all the other files residing in\n.git/worktrees//?\n\nThis code is correctly sandwiched within the \"is_junk\" scope, which\nmeans that \"auto_created\" will be cleaned up automatically, along with\nother .git/worktrees// files, if \"worktree add\" fails for some\nreason. Good.\n\n> argv_array_pushf(&child_env, \"%s=%s\", GIT_DIR_ENVIRONMENT, sb_git.buf);\n> argv_array_pushf(&child_env, \"%s=%s\", GIT_WORK_TREE_ENVIRONMENT, path);\n> @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)\n> if (run_command(&cp))\n> return -1;\n> branch = new_branch;\n> +\n> + auto_create = 1;\n\nDrop the unnecessary blank line.\n\nBy the way, this suffers from the problem that if \"git worktree add\nfoo\" fails for some reason, such as because path \"foo\" already exists,\nthen the new branch will _not_ be cleaned up automatically since that\nfailure will happen before \"auto_created\" is ever created (among other\nreasons). But that's not a new issue; it's an existing flaw of\n\"worktree add\" not cleaning up a branch it created before it discovers\nthat it can't actually create the target directory for some reason, so\nI wouldn't expect you to fix that problem with this submission. (I'm\njust mentioning it for completeness.)\n\n> @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> OPT_END()\n> };\n> - struct strbuf errmsg = STRBUF_INIT;\n> + struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;\n> - int ret = 0;\n> + int ret = 0, delete_auto_created = 0;\n> + struct object_id oid;\n\nPerhaps move the declarations of 'hex' and 'oid' into the scope where\nthey are used rather than making them global to the function.\n\n> @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> + /*\n> + * Check if we auto-created a branch for this worktree and it hasn't\n> + * moved since. Do it before the contents of the worktree get wiped.\n> + * Delete the branch later because it is checked out right now.\n> + */\n\nGood useful comment.\n\n> + git_path_buf(&sb, \"worktrees/%s/auto_created\", wt->id);\n> + if (file_exists(sb.buf)) {\n> + strbuf_read_file(&hex, sb.buf, 0);\n\nYou can avoid an unnecessary race condition here by dropping the\nfile_exists() call altogether and just checking the return code of\nstrbuf_read_file() -- which you should probably be doing anyhow. If\nstrbuf_read_file() returns a non-negative value, then you know it\nexists, so file_exists() is redundant.\n\n> + get_oid(wt->id, &oid);\n> +\n\nDrop the unnecessary blank line.\n\n> + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)\n> + delete_auto_created = 1;\n\nI was wondering if it would be more semantically correct to parse\n'hex' into an 'oid' and compare them with oidcmp() rather than doing a\nstring comparison of the hex values (though I'm not sure it will\nmatter in practice).\n\n> + }\n> +\n> + strbuf_release(&sb);\n> + strbuf_release(&hex);\n\nDrop the unnecessary blank line.\n\n> @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> + if (delete_auto_created) {\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + cp.git_cmd = 1;\n> +\n> + argv_array_push(&cp.args, \"branch\");\n> + argv_array_push(&cp.args, \"-d\");\n> + argv_array_push(&cp.args, wt->id);\n> +\n> + ret |= run_command(&cp);\n> + }\n\nAlternately:\n\n argv_array_pushl(&cp.args, \"branch\", \"-d\", wt->id, NULL);\n\nHowever, I don't think it is correct to use 'wt->id' here as the\nbranch name since there is no guarantee that the in\n.git/worktrees// matches the branch name with which the worktree\nwas created. For instance:\n\n git worktree add foo/bar existing-branch\n git worktree add baz/bar\n\nwill, due to name conflicts, create worktree metadata directories:\n\n .git/worktrees/bar\n .git/worktrees/bar1\n\nwhere the first is associated with branch \"existing-branch\", and the\nsecond is associated with new branch \"bar\". When you then invoke \"git\nworktree remove baz/bar\", it will try removing a branch named \"bar1\",\nnot \"bar\" as intended. To fix this, I think you need to record the\noriginal auto-created branch name in the \"auto_created\" metadata file\ntoo, not just the OID.\n\nFinally, make this code consistent with other existing similar code in\nthis file by dropping the unnecessary blank lines in this hunk.\n\n> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\n> @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '\n> +test_expect_success 'remove auto-created branch' '\n> + (\n> + git worktree add to-remove &&\n> + git worktree remove to-remove &&\n> + git branch -l to-remove >branch_list &&\n> + test_line_count = 0 branch_list\n> + )\n> +'\n\nI don't think there is any need for this test to be run in a subshell,\nso you can drop the enclosing '(' and ')'.\n\nI worry about using porcelain git-branch to check whether the branch\nhas actually been removed. Using git-rev-parse would likely be a more\ndirect and safe way to test it. For instance:\n\n git worktree add to-remove &&\n git worktree remove to-remove &&\n test_must_fail git rev-parse --verify -q to-remove\n\nshould be sufficient, I think. And, to be really thorough, you might say:\n\n test_might_fail git branch -D to-remove &&\n git worktree add to-remove &&\n git rev-parse --verify -q to-remove &&\n git worktree remove to-remove &&\n test_must_fail git rev-parse --verify -q to-remove\n\nThe above comments apply to the other new tests added by this patch, as well.\n\n> +test_expect_success 'do not remove a branch that was not auto-created' '\n> + (\n> + git worktree add -b new_branch to-remove &&\n\nNit: The inconsistent mix of underscore and hyphen in names is odd.\nPerhaps settle on one or the other (with a slight preference toward\nhyphen).\n\n> + git worktree remove to-remove &&\n> + git branch -l new_branch >branch_list &&\n> + test_line_count = 1 branch_list &&\n\nAs noted earlier, although this particular case of a branch created\nexplicitly with -b works as expected by not deleting the branch, the\nsimilar case:\n\n git worktree add -b to-remove to-remove &&\n\nwill incorrectly automatically delete the branch.\n\n> + git branch -d new_branch &&\n> + git branch foo &&\n> + git worktree add to-remove foo &&\n> + git worktree remove to-remove &&\n> + git branch -l foo >branch_list &&\n> + test_line_count = 1 branch_list &&\n> + git branch -d foo &&\n> + git branch to-remove &&\n> + git worktree add to-remove &&\n> + git worktree remove to-remove &&\n> + git branch -l to-remove >branch_list &&\n> + test_line_count = 1 branch_list &&\n> + git branch -d to-remove\n\nIf any code above this \"git branch -d\" fails, then it will never get\nthis far, thus won't remove \"to-remove\". To perform cleanup whether\nthe test succeeds or fails, you should use test_when_finished()\n_early_ in the test:\n\n test_when_finished \"git branch -d to-remove || :\" &&\n\nHowever, if you restructure the tests as suggested above, then you\nmight be able to get away without bothering with this cleanup.\n\n> + )\n> +'\n\nThis test is checking three distinct cases of explicitly-created\nbranches. It would make it easier to debug a failing case if you split\nit up into three tests -- one for each case.\n\n> +test_expect_success 'do not remove auto-created branch that was moved' '\n> + (\n> + git worktree add to-remove &&\n> + cd to-remove &&\n> + test_commit foo &&\n> + cd ../ &&\n\nWe normally avoid cd'ing around in tests like this because it can\ncause tests following this one to run in the wrong directory if\nsomething above the \"cd ../\" fails. In this particular case, it\ndoesn't matter since the entire body of this test is within a\nsubshell.\n\nHowever, if you take advantage of test_commits()'s -C argument, then\nyou can ditch the cd's and the subshell altogether:\n\n test_commit -C to-remove foo &&\n\n> + git worktree remove to-remove &&\n> + git branch -l to-remove >branch_list &&\n> + test_line_count = 1 branch_list &&\n> + git branch -D to-remove\n> + )\n> +'\n"},{"id":"389227","messageId":"20200104214703.5su6h4edzkwjv2kx@yadavpratyush.com","threadId":"52458","inReplyTo":"CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2020-01-04T21:47:03Z","receivedAt":"2020-01-04T21:47:43Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi Eric,\n\nThanks for the review.\n\nOn 27/12/19 06:05AM, Eric Sunshine wrote:\n> (Sorry for taking so long to review this patch; it ended up being a\n> quite lengthy review.)\n\nNo problem :-). And sorry for being so late to reply.\n \n> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav wrote:\n> > When no branch name is supplied to 'worktree add', it creates a new\n> > branch based on the name of the directory the new worktree is located\n> > in. But when the worktree is later removed, that created branch is left\n> > over.\n> \n> This is describing the existing (intentional) behavior but doesn't\n> explain why this might be annoying or problematic. To help sell the\n> patch, it might make sense to say something about how the behavior can\n> trip up newcomers to git-worktree, leaving them to wonder why they are\n> accumulating so many branches that they weren't aware they created. A\n> comment about why you think \"git worktree add -d foo\" is not a viable\n> way to side-step the creation of unwanted branches might also be\n> worthwhile. For instance, you might say something about how newcomers\n> might not read the documentation thoroughly enough to know about\n> --detach or to understand what it means; indeed, some newcomers to Git\n> presumably have trouble with the concept of a detached HEAD and may\n> find it scary.\n\nWill do.\n \n> > Remove that branch when removing the worktree. To make sure no commits\n> > are lost, the branch won't be deleted if it has moved.\n> \n> My knee-jerk reaction upon reading the first sentence of this\n> paragraph was that this is a significant and undesirable behavior\n> change, however, the second sentence helps to allay my fears about it.\n> \n> It's possible, I suppose, that there is some existing tooling\n> somewhere which relies upon the current behavior, but it's hard to\n> imagine any good reason to do so. (That is, \"git worktree add foo &&\n> git worktree remove foo\" is just a glorified and expensive way to say\n> \"git branch foo\".) So, I don't look upon this change with disfavor; it\n> could well be beneficial for newcomers, and perhaps a nice convenience\n> in general.\n\nIt is possible that some script somewhere does \n\n git worktree add foo\n do_something # doesn't move the branch\n git worktree remove foo\n git branch -d foo\n\nBranch deletion would fail here, which might be considered as an error \nby the script. Not sure how common that would be though.\n \n> However, there is a rather serious flaw in the implementation. My\n> expectation is that it should only automatically delete a branch if\n> the branch creation was inferred; it should never automatically delete\n> a branch which was created explicitly. You kind of have this covered\n> (and even have a test for it), but it doesn't work correctly when the\n> user explicitly requests branch creation via -b/-B and the branch name\n> matches the worktree name. For instance:\n> \n> git worktree add -b foo foo\n> git worktree remove foo\n> \n> incorrectly automatically removes branch \"foo\" even though the user\n> requested its creation explicitly.\n\nThanks for pointing it out. Will fix.\n \n> Another big question: Should an automatically-created branch be\n> deleted automatically when a worktree is pruned? That is, although\n> this sequence will remove an automatically-created branch:\n> \n> git worktree add foo\n> git worktree remove foo\n> \n> the current patch will not clean up the branch given this sequence:\n> \n> git worktree add foo\n> rm -rf foo\n> git worktree prune\n\nI see no problem with doing the same thing in 'prune' too.\n \n> Either way, it might be worthwhile to update the documentation to mention this.\n\nI'll see if I can make 'prune' delete the branch too. Otherwise, I'll \nmention it in the documentation.\n \n> > An example use case of when something like this is useful is when the\n> > user wants to check out a separate worktree to run and test on an older\n> > version, but don't want to touch the current worktree. So, they create a\n> > worktree, run some tests, and then remove it. But this leaves behind a\n> > branch the user never created in the first place.\n> \n> The last sentence isn't exactly accurate. The user _did_ create the\n> branch. It would be more accurate to say \"...the user did not\n> necessarily _intentionally_ create...\" or something like that.\n\nYes, that was the intention of the sentence. Will fix.\n \n> > So, remove the branch if nothing was done on it.\n> \n> By the way, the ordering of the commit message paragraphs is a bit\n> off; it somewhat tries to justifies the change before explaining what\n> the problem is. I'd suggest this order:\n> \n> - describe current behavior\n> - explain why current behavior can be undesirable in some circumstances;\n> cite your example use-case here, perhaps\n> - describe how this patch improves the situation\n> \n> The two paragraphs which talk about \"remove the branch\" are just\n> repeating one another. I would drop one of them and keep the other as\n> the final bullet point of the suggested commit message order.\n\nWill fix.\n \n> > Signed-off-by: Pratyush Yadav \n> > ---\n> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> > @@ -73,8 +73,9 @@ If `` is omitted and neither `-b` nor `-B` nor `--detach` used,\n> > doesn't exist, a new branch based on HEAD is automatically created as\n> > -if `-b ` was given. If `` does exist, it will be\n> > -checked out in the new worktree, if it's not checked out anywhere\n> > +if `-b ` was given. In this case, if `` is not moved, it is\n> > +automatically deleted when the worktree is removed. If `` does exist,\n> > +it will be checked out in the new worktree, if it's not checked out anywhere\n> \n> I found it confusing to find automatic branch deletion described here\n> under the \"worktree add\" command...\n> \n> > @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files\n> > +Removing a working tree might lead to its associated branch being deleted if\n> > +it was auto-created and has not moved since. See `add` for more information on\n> > +when exactly this can happen.\n> \n> Subjectively, it seems more natural to fully discuss automatic branch\n> removal here rather than referring to the discussion of \"worktree\n> add\".\n\nI considered doing this but then left that part in 'add' because the \nconditions in which the branch is auto deleted are described pretty well \nin add's documentation. Will move it to 'remove'.\n \n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > @@ -35,6 +35,7 @@ struct add_opts {\n> > +static int auto_create;\n> \n> I think this variable belongs in the 'add_opts' structure rather than\n> being file-global.\n\nOk.\n \n> > @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,\n> > - int len, ret;\n> > + int len, ret, fd;\n> > + struct object_id oid;\n> > + char *hex;\n> \n> Rather than declaring 'fd', 'oid', and 'hex' here, how about declaring\n> them in the scope of the \"if (auto_create) {\" conditional below, which\n> is the only place they are used?\n\nWill do. Some other projects I've contributed to in the past insist on \ndeclaring everything up-front, so I played it safe and put them here.\n \n> > @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,\n> > + strbuf_reset(&sb);\n> > + strbuf_addf(&sb, \"%s/auto_created\", sb_repo.buf);\n> \n> Why aren't these two lines inside the \"if (auto_create) {\" conditional\n> below? They seem to be used only for that case.\n\nYes, they should be. Will fix.\n \n> I think this new worktree metadata file warrants a documentation\n> update. In particular, gitrepository-layout.txt talks about\n> worktree-specific metadata files, and the \"Details\" section of\n> git-worktree.txt may need an update.\n\nWill fix.\n \n> A bit of bikeshedding regarding the filename: \"auto_created\" is rather\n> unusual. Most names in the .git hierarchy are short and sweet. Also,\n> with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word\n> filenames seem to use hyphen rather than underscore, which suggests\n> \"auto-created\" would be a better choice. However, I'd probably drop\n> the hyphen altogether. Finally, \"auto_created\", alone, does not\n> necessarily convey that the branch was auto-created; someone could\n> misinterpret it as meaning the worktree itself was auto-created, so I\n> wonder if a better name can be found.\n\nAny suggestions? Does \"implicitbranch\"/\"implicit-branch\" sound any \nbetter? How about \"branch-auto-created-at\"? It is very clear but is a \nmouthful.\n \n> A bigger question, though, is whether we really want to see new files\n> like this springing up in the .git/worktrees// directory for each\n> new piece of metadata which belongs to a worktree. I ask because this\n> isn't the first such case in which some additional worktree-specific\n> metadata was proposed (see, for instance, [1]). So, I'm wondering if\n> we should have a more generalized solution, such as introducing a new\n> file which can hold any sort of metadata which comes along in the\n> future. In particular, I'm thinking about a file containing an\n> extensible set of \"key: value\" tuples, in which case the \"auto\n> created\" metadata would be just one of possibly many keys. For\n> instance:\n\nDo you worry that the number of metadata files might grow to be too \nlarge? I can't say how worktrees will grow in the future, but right now \nthere are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD'). \nSo, not a lot.\n\nI chose to add a new file because from what I have noticed, Git keeps a \nlot of metadata in files like this (HEAD, refs, etc). Do other \nsubsystems use a key-value store? What problems did they face?\n \n> branch-auto-created-at: deadbeef\n> \n> The above is a genuine question. I'm not demanding that this patch\n> implement it, but I think it deserves discussion and thought before\n> making a decision.\n\nI'd prefer to not take on this feature (since I expect it to be a lot of \nwork), but if there are strong opinions on using a key-value store then \nI guess I'll bite the bullet.\n \n> [1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/\n> \n> > + /* Mark this branch as an \"auto-created\" one. */\n> \n> This comment doesn't really say anything which the code itself isn't\n> already saying (especially if you move the strbuf_addf() call inside\n> the conditional), so the comment could be dropped.\n\nOk.\n \n> > + if (auto_create) {\n> > + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> > + get_oid(\"HEAD\", &oid);\n> \n> Unless I'm mistaken, this is just wrong. You're grabbing the OID of\n> HEAD from the worktree in which \"worktree add\" is being invoked,\n> however, if the new branch name is DWIM'd from an existing\n> tracking-branch, then the OID should be that of the tracking-branch,\n> not HEAD of the current worktree. So, you should be using the OID\n> already looked up earlier in the function, 'commit->object.oid', which\n> should be correct for either case.\n\nOops! Thanks for pointing it out. Will fix.\n \n> > + hex = oid_to_hex(&oid);\n> > + write_file_buf(sb.buf, hex, strlen(hex));\n> > +\n> > + if (close(fd))\n> > + die(_(\"could not close '%s'\"), sb.buf);\n> > + }\n> \n> Is there a reason you're creating the file in this rather manual\n> fashion rather than using write_file() as is already heavily used in\n> this code for creating all the other files residing in\n> .git/worktrees//?\n\nNo particular reason. I didn't look around enough to catch the pattern \nof using write_file() for this. Will fix.\n \n> This code is correctly sandwiched within the \"is_junk\" scope, which\n> means that \"auto_created\" will be cleaned up automatically, along with\n> other .git/worktrees// files, if \"worktree add\" fails for some\n> reason. Good.\n> \n> > argv_array_pushf(&child_env, \"%s=%s\", GIT_DIR_ENVIRONMENT, sb_git.buf);\n> > argv_array_pushf(&child_env, \"%s=%s\", GIT_WORK_TREE_ENVIRONMENT, path);\n> > @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)\n> > if (run_command(&cp))\n> > return -1;\n> > branch = new_branch;\n> > +\n> > + auto_create = 1;\n> \n> Drop the unnecessary blank line.\n> \n> By the way, this suffers from the problem that if \"git worktree add\n> foo\" fails for some reason, such as because path \"foo\" already exists,\n> then the new branch will _not_ be cleaned up automatically since that\n> failure will happen before \"auto_created\" is ever created (among other\n> reasons). But that's not a new issue; it's an existing flaw of\n> \"worktree add\" not cleaning up a branch it created before it discovers\n> that it can't actually create the target directory for some reason, so\n> I wouldn't expect you to fix that problem with this submission. (I'm\n> just mentioning it for completeness.)\n\nI'll see if I can come up with a fix for this as a follow-up patch.\n \n> > @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> > OPT_END()\n> > };\n> > - struct strbuf errmsg = STRBUF_INIT;\n> > + struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;\n> > - int ret = 0;\n> > + int ret = 0, delete_auto_created = 0;\n> > + struct object_id oid;\n> \n> Perhaps move the declarations of 'hex' and 'oid' into the scope where\n> they are used rather than making them global to the function.\n\nWill do.\n \n> > @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> > + /*\n> > + * Check if we auto-created a branch for this worktree and it hasn't\n> > + * moved since. Do it before the contents of the worktree get wiped.\n> > + * Delete the branch later because it is checked out right now.\n> > + */\n> \n> Good useful comment.\n\nThanks.\n \n> > + git_path_buf(&sb, \"worktrees/%s/auto_created\", wt->id);\n> > + if (file_exists(sb.buf)) {\n> > + strbuf_read_file(&hex, sb.buf, 0);\n> \n> You can avoid an unnecessary race condition here by dropping the\n> file_exists() call altogether and just checking the return code of\n> strbuf_read_file() -- which you should probably be doing anyhow. If\n> strbuf_read_file() returns a non-negative value, then you know it\n> exists, so file_exists() is redundant.\n\nWill fix. Though I don't see how it would be a \"race condition\". Is \nfile_exists() asynchronous in some way? Otherwise, how would a race \nhappen and between what?\n \n> > + get_oid(wt->id, &oid);\n> > +\n> \n> Drop the unnecessary blank line.\n> \n> > + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)\n> > + delete_auto_created = 1;\n> \n> I was wondering if it would be more semantically correct to parse\n> 'hex' into an 'oid' and compare them with oidcmp() rather than doing a\n> string comparison of the hex values (though I'm not sure it will\n> matter in practice).\n\nSince I haven't spent too much time in the Git internals, the string \nrepresentation feels more natural to me. And that's why I went this way \nsubconsciously. While I don't mind either, I wonder if it would make a \ndifference in practice. Anyway, if you have a preference for the other \nway round, I'll trust your gut feeling.\n \n> > + }\n> > +\n> > + strbuf_release(&sb);\n> > + strbuf_release(&hex);\n> \n> Drop the unnecessary blank line.\n> \n> > @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> > + if (delete_auto_created) {\n> > + struct child_process cp = CHILD_PROCESS_INIT;\n> > + cp.git_cmd = 1;\n> > +\n> > + argv_array_push(&cp.args, \"branch\");\n> > + argv_array_push(&cp.args, \"-d\");\n> > + argv_array_push(&cp.args, wt->id);\n> > +\n> > + ret |= run_command(&cp);\n> > + }\n> \n> Alternately:\n> \n> argv_array_pushl(&cp.args, \"branch\", \"-d\", wt->id, NULL);\n\nOk.\n \n> However, I don't think it is correct to use 'wt->id' here as the\n> branch name since there is no guarantee that the in\n> .git/worktrees// matches the branch name with which the worktree\n> was created. For instance:\n> \n> git worktree add foo/bar existing-branch\n> git worktree add baz/bar\n> \n> will, due to name conflicts, create worktree metadata directories:\n> \n> .git/worktrees/bar\n> .git/worktrees/bar1\n> \n> where the first is associated with branch \"existing-branch\", and the\n> second is associated with new branch \"bar\". When you then invoke \"git\n> worktree remove baz/bar\", it will try removing a branch named \"bar1\",\n> not \"bar\" as intended. To fix this, I think you need to record the\n> original auto-created branch name in the \"auto_created\" metadata file\n> too, not just the OID.\n\nInteresting! Didn't think of a situation like this. Thanks for pointing \nit out. Will fix.\n \n> Finally, make this code consistent with other existing similar code in\n> this file by dropping the unnecessary blank lines in this hunk.\n\nThe blank lines are a personal preference for me mostly. I am not a huge \nfan of seeing large chunks of code with do blank lines in between. IMO \nit hurts readability. But, I think staying consistent with the code that \nalready exists is more important. Will remove all them.\n \n> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\n> > @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '\n> > +test_expect_success 'remove auto-created branch' '\n> > + (\n> > + git worktree add to-remove &&\n> > + git worktree remove to-remove &&\n> > + git branch -l to-remove >branch_list &&\n> > + test_line_count = 0 branch_list\n> > + )\n> > +'\n> \n> I don't think there is any need for this test to be run in a subshell,\n> so you can drop the enclosing '(' and ')'.\n\nI was following the pattern in the two tests above. Will drop the \nparentheses.\n \n> I worry about using porcelain git-branch to check whether the branch\n> has actually been removed. Using git-rev-parse would likely be a more\n> direct and safe way to test it. For instance:\n> \n> git worktree add to-remove &&\n> git worktree remove to-remove &&\n> test_must_fail git rev-parse --verify -q to-remove\n> \n> should be sufficient, I think. And, to be really thorough, you might say:\n> \n> test_might_fail git branch -D to-remove &&\n> git worktree add to-remove &&\n> git rev-parse --verify -q to-remove &&\n> git worktree remove to-remove &&\n> test_must_fail git rev-parse --verify -q to-remove\n> \n> The above comments apply to the other new tests added by this patch, as well.\n\nWill fix.\n \n> > +test_expect_success 'do not remove a branch that was not auto-created' '\n> > + (\n> > + git worktree add -b new_branch to-remove &&\n> \n> Nit: The inconsistent mix of underscore and hyphen in names is odd.\n> Perhaps settle on one or the other (with a slight preference toward\n> hyphen).\n\nI'll change 'new_branch' to 'new-branch'.\n \n> > + git worktree remove to-remove &&\n> > + git branch -l new_branch >branch_list &&\n> > + test_line_count = 1 branch_list &&\n> \n> As noted earlier, although this particular case of a branch created\n> explicitly with -b works as expected by not deleting the branch, the\n> similar case:\n> \n> git worktree add -b to-remove to-remove &&\n> \n> will incorrectly automatically delete the branch.\n> \n> > + git branch -d new_branch &&\n> > + git branch foo &&\n> > + git worktree add to-remove foo &&\n> > + git worktree remove to-remove &&\n> > + git branch -l foo >branch_list &&\n> > + test_line_count = 1 branch_list &&\n> > + git branch -d foo &&\n> > + git branch to-remove &&\n> > + git worktree add to-remove &&\n> > + git worktree remove to-remove &&\n> > + git branch -l to-remove >branch_list &&\n> > + test_line_count = 1 branch_list &&\n> > + git branch -d to-remove\n> \n> If any code above this \"git branch -d\" fails, then it will never get\n> this far, thus won't remove \"to-remove\". To perform cleanup whether\n> the test succeeds or fails, you should use test_when_finished()\n> _early_ in the test:\n> \n> test_when_finished \"git branch -d to-remove || :\" &&\n> \n> However, if you restructure the tests as suggested above, then you\n> might be able to get away without bothering with this cleanup.\n> \n> > + )\n> > +'\n> \n> This test is checking three distinct cases of explicitly-created\n> branches. It would make it easier to debug a failing case if you split\n> it up into three tests -- one for each case.\n\nI considered doing it, but then I thought maybe I shouldn't add so many \ntests. And since there are only 3 rather independent cases, it would not \nbe that difficult to figure out which one is the culprit. Will split \nthem.\n \n> > +test_expect_success 'do not remove auto-created branch that was moved' '\n> > + (\n> > + git worktree add to-remove &&\n> > + cd to-remove &&\n> > + test_commit foo &&\n> > + cd ../ &&\n> \n> We normally avoid cd'ing around in tests like this because it can\n> cause tests following this one to run in the wrong directory if\n> something above the \"cd ../\" fails. In this particular case, it\n> doesn't matter since the entire body of this test is within a\n> subshell.\n> \n> However, if you take advantage of test_commits()'s -C argument, then\n> you can ditch the cd's and the subshell altogether:\n> \n> test_commit -C to-remove foo &&\n\nOk.\n \n> > + git worktree remove to-remove &&\n> > + git branch -l to-remove >branch_list &&\n> > + test_line_count = 1 branch_list &&\n> > + git branch -D to-remove\n> > + )\n> > +'\n\nI'll send out the v2 as soon as I can. Thanks.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"389241","messageId":"CAPig+cT9z5HQqbyFKpb0sQ7Hh=hbcu8TwNdN1smK74HtLzw1cg@mail.gmail.com","threadId":"52458","inReplyTo":"CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-05T05:32:57Z","receivedAt":"2020-01-05T05:33:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jan 4, 2020 at 4:47 PM Pratyush Yadav wrote:\n> On 27/12/19 06:05AM, Eric Sunshine wrote:\n> > On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav wrote:\n> > > Remove that branch when removing the worktree. To make sure no commits\n> > > are lost, the branch won't be deleted if it has moved.\n> >\n> > My knee-jerk reaction upon reading the first sentence of this\n> > paragraph was that this is a significant and undesirable behavior\n> > change, however, the second sentence helps to allay my fears about it.\n> > It's possible, I suppose, that there is some existing tooling\n> > somewhere which relies upon the current behavior, but it's hard to\n> > imagine any good reason to do so.\n>\n> It is possible that some script somewhere does\n>\n> git worktree add foo\n> do_something # doesn't move the branch\n> git worktree remove foo\n> git branch -d foo\n>\n> Branch deletion would fail here, which might be considered as an error\n> by the script. Not sure how common that would be though.\n\nGood point. That's a quite believable scenario for a scripting case.\nEven if the script itself doesn't check for an error from git-branch,\npeople could get annoyed if their scripts suddenly start complaining\n\"error: branch 'foo' not found\". So, that's a genuine concern.\n\n> > However, there is a rather serious flaw in the implementation. My\n> > expectation is that it should only automatically delete a branch if\n> > the branch creation was inferred; it should never automatically delete\n> > a branch which was created explicitly. You kind of have this covered\n> > (and even have a test for it), but it doesn't work correctly when the\n> > user explicitly requests branch creation via -b/-B and the branch name\n> > matches the worktree name. For instance:\n> >\n> > git worktree add -b foo foo\n> > git worktree remove foo\n> >\n> > incorrectly automatically removes branch \"foo\" even though the user\n> > requested its creation explicitly.\n>\n> Thanks for pointing it out. Will fix.\n\nNote that this almost certainly deserves an extra test in t2403 since\nthe tests added by v1 didn't catch this problem.\n\n> > Subjectively, it seems more natural to fully discuss automatic branch\n> > removal here rather than referring to the discussion of \"worktree\n> > add\".\n>\n> I considered doing this but then left that part in 'add' because the\n> conditions in which the branch is auto deleted are described pretty well\n> in add's documentation. Will move it to 'remove'.\n\nIn retrospect, I don't feel strongly about it one way or the other. It\njust surprised me to find it discussed under \"add\" on my first\nread-through. And if you do patch \"prune\" to also auto-remove an\nauto-created branch, then \"add\" might be the better place for the\ndiscussion anyhow.\n\n> > A bit of bikeshedding regarding the filename: \"auto_created\" is rather\n> > unusual. Most names in the .git hierarchy are short and sweet. Also,\n> > with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word\n> > filenames seem to use hyphen rather than underscore, which suggests\n> > \"auto-created\" would be a better choice. However, I'd probably drop\n> > the hyphen altogether. Finally, \"auto_created\", alone, does not\n> > necessarily convey that the branch was auto-created; someone could\n> > misinterpret it as meaning the worktree itself was auto-created, so I\n> > wonder if a better name can be found.\n>\n> Any suggestions? Does \"implicitbranch\"/\"implicit-branch\" sound any\n> better? How about \"branch-auto-created-at\"? It is very clear but is a\n> mouthful.\n\nTaking into account the suggestion from my review that you likely also\nwill need to store in this file the name of the auto-created branch\n(not just the original OID of that branch), then the nature of this\nfile changes a bit, which might help suggest a better name.\n\"implicitbranch\" and \"implicit-branch\" are not bad, though a bit of a\nmouthful. What about \"autobranch\"?\n\n> > A bigger question, though, is whether we really want to see new files\n> > like this springing up in the .git/worktrees// directory for each\n> > new piece of metadata which belongs to a worktree. I ask because this\n> > isn't the first such case in which some additional worktree-specific\n> > metadata was proposed (see, for instance, [1]). So, I'm wondering if\n> > we should have a more generalized solution, such as introducing a new\n> > file which can hold any sort of metadata which comes along in the\n> > future. In particular, I'm thinking about a file containing an\n> > extensible set of \"key: value\" tuples, in which case the \"auto\n> > created\" metadata would be just one of possibly many keys.\n>\n> Do you worry that the number of metadata files might grow to be too\n> large? I can't say how worktrees will grow in the future, but right now\n> there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD').\n> So, not a lot.\n\nI'm not particularly worried about the number of files. A couple\nthoughts I had in mind are: (1) other tools or non-canonical Git\nimplementations (jgit, libgit, etc.) may be poking around inside the\n.git/worktrees// directory, and (2) the information represented by\nthis new file may deserve inclusion in the output of \"git worktree\nlist --porcelain\".\n\nIt was #1, in particular, I think, which got me thinking of having a\nstandardized format (i.e. extensible \"key: value\" list) for worktree\nmetainfo added in the future. It would require a one-time cost for\neach tool/library to implement, and would then effectively be free as\nmore metainfo is added to the worktree. Compare that with having to\nwrite a reader/parser for each new metainfo file added (not that these\nfiles are terribly difficult to parse).\n\nSimilarly, a standardized format simplifies #2, extending \"git\nworktree list --porcelain\" to output additional metainfo.\n\nBy the way, I wouldn't mind seeing \"git worktree list --porcelain\"\nextended to output this new information, but I don't insist upon it as\na requirement of this patch; it can easily be done later if the need\narises. (In fact, the --porcelain documentation is so woefully lacking\nand under-specified that it needs an overhaul, which I think deserves\na patch series of its own, thus is another reason I don't really\nexpect/want to see that change made by this patch.)\n\n> I chose to add a new file because from what I have noticed, Git keeps a\n> lot of metadata in files like this (HEAD, refs, etc). Do other\n> subsystems use a key-value store? What problems did they face?\n>\n> I'd prefer to not take on this feature (since I expect it to be a lot of\n> work), but if there are strong opinions on using a key-value store then\n> I guess I'll bite the bullet.\n\nI brought up the idea of a standardized extensible \"key: value\" store\nbecause, having that older email thread in mind, it was the first\nthought which popped into my mind when I saw this patch introducing a\nnew file in .git/worktrees//. However, one can make a good\nargument that Git already has such a standard, and that that standard\nis simply using individual files like \"HEAD\", \"gitdir\", \"commondir\",\netc. So, I think I'm pretty comfortable with the idea of storing this\ninformation in a new file as this patch does. After all, there's\nplenty of precedent in Git for doing it that way.\n\n> > > + if (auto_create) {\n> > > + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> > > + get_oid(\"HEAD\", &oid);\n> >\n> > Unless I'm mistaken, this is just wrong. You're grabbing the OID of\n> > HEAD from the worktree in which \"worktree add\" is being invoked,\n> > however, if the new branch name is DWIM'd from an existing\n> > tracking-branch, then the OID should be that of the tracking-branch,\n> > not HEAD of the current worktree. So, you should be using the OID\n> > already looked up earlier in the function, 'commit->object.oid', which\n> > should be correct for either case.\n>\n> Oops! Thanks for pointing it out. Will fix.\n\nThis deserves a new test in t2403, as well (or perhaps two new tests\nsince there are a couple different ways the starting OID can be\nDWIM'd, if I'm reading the code correctly).\n\n> > By the way, this suffers from the problem that if \"git worktree add\n> > foo\" fails for some reason, such as because path \"foo\" already exists,\n> > then the new branch will _not_ be cleaned up automatically since that\n> > failure will happen before \"auto_created\" is ever created (among other\n> > reasons). But that's not a new issue; it's an existing flaw of\n> > \"worktree add\" not cleaning up a branch it created before it discovers\n> > that it can't actually create the target directory for some reason, so\n> > I wouldn't expect you to fix that problem with this submission. (I'm\n> > just mentioning it for completeness.)\n>\n> I'll see if I can come up with a fix for this as a follow-up patch.\n\nIf fixing this ends up being more involved than a relatively minor\nchange -- and I think it will be quite a bit more involved due to all\nthe die()ing going on inside add_worktree() -- then I could easily see\n(and probably would prefer) it being a separate patch series (since\nthe changes made by the patch under discussion are already\nsufficiently involved as to eat up a good deal of reviewer time).\n\n> > > + git_path_buf(&sb, \"worktrees/%s/auto_created\", wt->id);\n> > > + if (file_exists(sb.buf)) {\n> > > + strbuf_read_file(&hex, sb.buf, 0);\n> >\n> > You can avoid an unnecessary race condition here by dropping the\n> > file_exists() call altogether and just checking the return code of\n> > strbuf_read_file() -- which you should probably be doing anyhow. If\n> > strbuf_read_file() returns a non-negative value, then you know it\n> > exists, so file_exists() is redundant.\n>\n> Will fix. Though I don't see how it would be a \"race condition\". Is\n> file_exists() asynchronous in some way? Otherwise, how would a race\n> happen and between what?\n\nI think I was remembering an earlier[1] issue with someone running a\nbunch of git-worktree commands in parallel and encountering races, but\nthat shouldn't apply here. Still, the code will be cleaner by dropping\nfile_exists() altogether.\n\n[1]: https://lore.kernel.org/git/cover.1550508544.git.msuchanek@suse.de/T/\n\n> > > + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)\n> > > + delete_auto_created = 1;\n> >\n> > I was wondering if it would be more semantically correct to parse\n> > 'hex' into an 'oid' and compare them with oidcmp() rather than doing a\n> > string comparison of the hex values (though I'm not sure it will\n> > matter in practice).\n>\n> Since I haven't spent too much time in the Git internals, the string\n> representation feels more natural to me. And that's why I went this way\n> subconsciously. While I don't mind either, I wonder if it would make a\n> difference in practice. Anyway, if you have a preference for the other\n> way round, I'll trust your gut feeling.\n\nIt may not matter in practice, but having considered it further, I\nreally would prefer it to be semantically correct by comparing the\nOIDs directly rather than the string representations. With the process\nunderway of updating Git to be able to work with multiple hash\nfunctions (and moving away from SHA-1), making use of get_oid_hex()\nand oideq() to perform this comparison may make the job of auditing\nthe code for multi-hash-function friendliness easier.\n\n> > However, I don't think it is correct to use 'wt->id' here as the\n> > branch name since there is no guarantee that the in\n> > .git/worktrees// matches the branch name with which the worktree\n> > was created. For instance:\n> >\n> > git worktree add foo/bar existing-branch\n> > git worktree add baz/bar\n> >\n> > will, due to name conflicts, create worktree metadata directories:\n> >\n> > .git/worktrees/bar\n> > .git/worktrees/bar1\n> >\n> > where the first is associated with branch \"existing-branch\", and the\n> > second is associated with new branch \"bar\". When you then invoke \"git\n> > worktree remove baz/bar\", it will try removing a branch named \"bar1\",\n> > not \"bar\" as intended. To fix this, I think you need to record the\n> > original auto-created branch name in the \"auto_created\" metadata file\n> > too, not just the OID.\n>\n> Interesting! Didn't think of a situation like this. Thanks for pointing\n> it out. Will fix.\n\nDefinitely deserves a test in t2403.\n\n> > > +test_expect_success 'remove auto-created branch' '\n> > > + (\n> > > + git worktree add to-remove &&\n> > > + git worktree remove to-remove &&\n> > > + git branch -l to-remove >branch_list &&\n> > > + test_line_count = 0 branch_list\n> > > + )\n> > > +'\n> >\n> > I don't think there is any need for this test to be run in a subshell,\n> > so you can drop the enclosing '(' and ')'.\n>\n> I was following the pattern in the two tests above. Will drop the\n> parentheses.\n\nThe existing tests use a subshell (the parentheses) because they 'cd'\naround, and use of a subshell ensures that subsequent tests won't\nadversely run in the wrong directory if the test fails for some reason\n(since the affect of the 'cd' does not last beyond the end of the\nsubshell). As long as you're not cd'ing around (or doing a few other\nquestionable things), there's no need for the subshell.\n\n> > > +test_expect_success 'do not remove a branch that was not auto-created' '\n> > > + (\n> > > + git worktree add -b new_branch to-remove &&\n> >\n> > Nit: The inconsistent mix of underscore and hyphen in names is odd.\n> > Perhaps settle on one or the other (with a slight preference toward\n> > hyphen).\n>\n> I'll change 'new_branch' to 'new-branch'.\n\nAs mentioned above, also add a test in which the explicitly-created\nnew branch has the same name as the worktree itself (since that case\nwas implemented wrong in v1 but the tests didn't catch the failure).\n\n> I'll send out the v2 as soon as I can. Thanks.\n\nThere's no rush. Let's get the details and any lingering questions\nworked out before subjecting reviewers to a new version (especially,\nas my review time is somewhat limited these days).\n"},{"id":"389259","messageId":"CAPig+cQmqKiYWDWFH5eK2S6XPOi2t2+8Oas8yZa8R=bKLym3wQ@mail.gmail.com","threadId":"52458","inReplyTo":"CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-06T04:20:41Z","receivedAt":"2020-01-06T04:20:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine wrote:\n> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav wrote:\n> > When no branch name is supplied to 'worktree add', it creates a new\n> > branch based on the name of the directory the new worktree is located\n> > in. But when the worktree is later removed, that created branch is left\n> > over.\n>\n> This is describing the existing (intentional) behavior but doesn't\n> explain why this might be annoying or problematic. To help sell the\n> patch, it might make sense to say something about how the behavior can\n> trip up newcomers to git-worktree, leaving them to wonder why they are\n> accumulating so many branches that they weren't aware they created. A\n> comment about why you think \"git worktree add -d foo\" is not a viable\n> way to side-step the creation of unwanted branches might also be\n> worthwhile.\n\nAs an alternative to this patch, would the simpler approach of\nimproving git-worktree documentation to do a better job of pointing\npeople at -d/--detach as a way to side-step unwanted branch creation\nmake sense? That is, at minimum, enhance the \"Description\" section to\nprominently talk about throwaway worktrees (created with -d/--detach),\nand add an example to the \"Examples\" section (perhaps as the first\nexample) showing creation/use/deletion of a throwaway worktree.\n\nSome points in favor of just updating the documentation to address\nthis issue (rather than implementing the new behavior suggested by\nthis patch) include:\n\n* far simpler; no code to implement or debug\n\n* no (surprising) behavior changes\n\n* \"git worktree add -d foo\" is about as easy to type and remember as\n \"git worktree add foo\"\n\nOf lesser importance, it might make sense, as a followup, to add a\nconfiguration which changes the default behavior to detach instead of\nauto-creating a branch. I wonder if this could be piggybacked on the\nexisting \"worktree.guessremote\" configuration. Or rather,\nretire/deprecate that configuration and add a new one which affects\nDWIM'ing behavior such that it becomes multi-state. Some possible\nvalues for the new configuration: \"auto\" (or \"dwim\" or whatever),\n\"guessremote\", \"detach\". (I haven't thought this through thoroughly,\nso there might be holes in my suggestion.)\n\nThere's at least one point not in favor of merely updating the\ndocumentation to promote -d/--detach more heavily, and that is that\n(presumably) the concept of detached HEAD is perceived as an advanced\ntopic, so it may not be suitable for the newcomer or casual user.\n"},{"id":"389273","messageId":"20200106180101.wwznvthla35x3qd2@yadavpratyush.com","threadId":"52458","inReplyTo":"CAPig+cQmqKiYWDWFH5eK2S6XPOi2t2+8Oas8yZa8R=bKLym3wQ@mail.gmail.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2020-01-06T18:01:01Z","receivedAt":"2020-01-06T18:01:21Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 05/01/20 11:20PM, Eric Sunshine wrote:\n> On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine wrote:\n> > On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav wrote:\n> > > When no branch name is supplied to 'worktree add', it creates a new\n> > > branch based on the name of the directory the new worktree is located\n> > > in. But when the worktree is later removed, that created branch is left\n> > > over.\n> >\n> > This is describing the existing (intentional) behavior but doesn't\n> > explain why this might be annoying or problematic. To help sell the\n> > patch, it might make sense to say something about how the behavior can\n> > trip up newcomers to git-worktree, leaving them to wonder why they are\n> > accumulating so many branches that they weren't aware they created. A\n> > comment about why you think \"git worktree add -d foo\" is not a viable\n> > way to side-step the creation of unwanted branches might also be\n> > worthwhile.\n> \n> As an alternative to this patch, would the simpler approach of\n> improving git-worktree documentation to do a better job of pointing\n> people at -d/--detach as a way to side-step unwanted branch creation\n> make sense? That is, at minimum, enhance the \"Description\" section to\n> prominently talk about throwaway worktrees (created with -d/--detach),\n> and add an example to the \"Examples\" section (perhaps as the first\n> example) showing creation/use/deletion of a throwaway worktree.\n> \n> Some points in favor of just updating the documentation to address\n> this issue (rather than implementing the new behavior suggested by\n> this patch) include:\n> \n> * far simpler; no code to implement or debug\n> \n> * no (surprising) behavior changes\n> \n> * \"git worktree add -d foo\" is about as easy to type and remember as\n> \"git worktree add foo\"\n\nFYI, I'm running Git v2.24.1 and 'git worktree add' doesn't accept the \noption '-d'. It only accepts '--detach'. And looking at the current \n'next', I don't see the option mentioned in git-worktree.txt. So at the \nvery least, we should start by actually adding the option.\n \n> Of lesser importance, it might make sense, as a followup, to add a\n> configuration which changes the default behavior to detach instead of\n> auto-creating a branch. I wonder if this could be piggybacked on the\n> existing \"worktree.guessremote\" configuration. Or rather,\n> retire/deprecate that configuration and add a new one which affects\n> DWIM'ing behavior such that it becomes multi-state. Some possible\n> values for the new configuration: \"auto\" (or \"dwim\" or whatever),\n> \"guessremote\", \"detach\". (I haven't thought this through thoroughly,\n> so there might be holes in my suggestion.)\n\nHonestly, coupled with a configuration variable this alternative fits my \nuse-case really well.\n\nI think 'guessremote' does not describe very well what the config \nvariable would actually do. So I think deprecating it would be a better \nidea.\n\nDoes 'worktree.newBranch' sound like a good name? (Disclaimer: I am \nterrible at naming things).\n \n> There's at least one point not in favor of merely updating the\n> documentation to promote -d/--detach more heavily, and that is that\n> (presumably) the concept of detached HEAD is perceived as an advanced\n> topic, so it may not be suitable for the newcomer or casual user.\n\nI'm basing this off no data so take it with a grain of salt, but I think \npeople who know Git enough to understand the concept of multiple \nworktrees should also understand what a detached HEAD is. And even if \nthey already don't know what it is, they should have no trouble quickly \nreading one of the many great explanations available with a simple \nGoogle search.\n\nMy argument in favor of auto-deletion is that we should still try to \nhave sane defaults. Leaving behind a branch the user didn't explicitly \ncreate and didn't use doesn't sound like a sane default to me.\n\nThe configuration variable path is easier and suits my needs really \nwell, so I am inclined to just go with it. But making the whole user \nexperience better for everyone is still something worthwhile. But then \nagain, introducing a backwards-incompatible change might not be the best \nidea. So, I dunno.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"389524","messageId":"CAPig+cTva89t8Zco-Ke0oD5xDZF_uuGH-gSkLXE2r31NtSE8nw@mail.gmail.com","threadId":"52458","inReplyTo":"20200106180101.wwznvthla35x3qd2@yadavpratyush.com","subject":"Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-09T09:46:57Z","receivedAt":"2020-01-09T09:47:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 6, 2020 at 1:01 PM Pratyush Yadav wrote:\n> On 05/01/20 11:20PM, Eric Sunshine wrote:\n> > On Fri, Dec 27, 2019 at 6:05 AM Eric Sunshine wrote:\n> > As an alternative to this patch, would the simpler approach of\n> > improving git-worktree documentation to do a better job of pointing\n> > people at -d/--detach as a way to side-step unwanted branch creation\n> > make sense? That is, at minimum, enhance the \"Description\" section to\n> > prominently talk about throwaway worktrees (created with -d/--detach),\n> > and add an example to the \"Examples\" section (perhaps as the first\n> > example) showing creation/use/deletion of a throwaway worktree.\n>\n> FYI, I'm running Git v2.24.1 and 'git worktree add' doesn't accept the\n> option '-d'. It only accepts '--detach'. And looking at the current\n> 'next', I don't see the option mentioned in git-worktree.txt. So at the\n> very least, we should start by actually adding the option.\n\nI forgot that -d was never added as shorthand for --detach, and didn't\nbother checking the man page. But, yes, adding -d would be a good\nstart.\n\n> > Of lesser importance, it might make sense, as a followup, to add a\n> > configuration which changes the default behavior to detach instead of\n> > auto-creating a branch. I wonder if this could be piggybacked on the\n> > existing \"worktree.guessremote\" configuration. Or rather,\n> > retire/deprecate that configuration and add a new one which affects\n> > DWIM'ing behavior such that it becomes multi-state. Some possible\n> > values for the new configuration: \"auto\" (or \"dwim\" or whatever),\n> > \"guessremote\", \"detach\". (I haven't thought this through thoroughly,\n> > so there might be holes in my suggestion.)\n>\n> Honestly, coupled with a configuration variable this alternative fits my\n> use-case really well.\n>\n> I think 'guessremote' does not describe very well what the config\n> variable would actually do. So I think deprecating it would be a better\n> idea.\n>\n> Does 'worktree.newBranch' sound like a good name? (Disclaimer: I am\n> terrible at naming things).\n\nMaybe 'worktree.addFlags' or something? I'm thinking that this might\nbe a multi-value configuration variable which is a combination of the\nvarious option flags which can be used with \"git worktree add\". For\ninstance: 'worktree.addFlags=detach' or\nworktree.addFlags=auto-create-branch,guess-remote. Possible values\nmight include:\n\n[no-]auto-create-branch\n enable/disable automatic branch creation when is\n omitted\n\ndetach\n create worktree with detached HEAD\n\n[no-]checkout\n perform/suppress checkout of in the new worktree\n\n[no-]guess-remote\n create local branch from remote-tracking branch if present and\n omitted\n\n[no-]track\n make new branch track if the latter is a branch name\n\n[no-]lock\n keep worktree locked after creation\n\nIn fact, I'd like to see 'auto-create-branch' incorporate\n'guess-remote' behavior by default since \"remote guessing\" should have\nbeen the default behavior from day one, but it was overlooked. The\n--guess-remote option was added simply to avoid backward compatibility\nproblems, but it would be nice to one day make it the default. Since\nthis configuration variable is new, we don't have to worry about\nbackward compatibility with it, thus can make 'auto-create-branch'\nwork like it should have from inception -- that is, performing \"remote\nguessing\" DWIMing (just like \"git checkout\" does by default).\n\nA command-line option would (as expected) override a flag set via\n'worktree.addFlags'. So, for instance, --no-detach would override\n'worktree.addFlags=detach'.\n\nAnyhow, this is just a rough idea. I haven't thought through all the\nramifications, or even if this is a sane interface.\n\n> > There's at least one point not in favor of merely updating the\n> > documentation to promote -d/--detach more heavily, and that is that\n> > (presumably) the concept of detached HEAD is perceived as an advanced\n> > topic, so it may not be suitable for the newcomer or casual user.\n>\n> I'm basing this off no data so take it with a grain of salt, but I think\n> people who know Git enough to understand the concept of multiple\n> worktrees should also understand what a detached HEAD is. And even if\n> they already don't know what it is, they should have no trouble quickly\n> reading one of the many great explanations available with a simple\n> Google search.\n\nI don't necessarily share that opinion, but I do think that if we add\n-d as shorthand for --detach, and do a really good job of updating the\ndocumentation to promote the idea of \"throwaway worktrees\" (which also\nhappen to be detached), then we have a good path forward.\n\n> My argument in favor of auto-deletion is that we should still try to\n> have sane defaults. Leaving behind a branch the user didn't explicitly\n> create and didn't use doesn't sound like a sane default to me.\n>\n> The configuration variable path is easier and suits my needs really\n> well, so I am inclined to just go with it. But making the whole user\n> experience better for everyone is still something worthwhile. But then\n> again, introducing a backwards-incompatible change might not be the best\n> idea. So, I dunno.\n\nYep, the different ideas can co-exist, and each can be implemented\nwithout promising to implement the others. A good first step would be\nto add -d as alias for --detach and update the documentation to\npromote the concept of \"throwaway worktrees\". An optional second step\n(if needed) would be that new configuration variable (though it still\nneeds more thought). And, a really optional third step (if anyone\ncares strongly enough) would be to implement auto-deletion of\nauto-created branches.\n"}]}