{"thread":{"id":"59637","subject":"[PATCH 0/3] warn when unreachable commits are left behind","startedAt":"2023-04-22T22:10:48Z","lastAt":"2023-04-28T00:49:58Z","messageCount":12,"participants":["Rubén Justo","Junio C Hamano","Andrei Rybak","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"475881","messageId":"f702476a-543a-da9b-ccd9-4431c80471e1@gmail.com","threadId":"59637","inReplyTo":null,"subject":"[PATCH 0/3] warn when unreachable commits are left behind","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-22T22:10:41Z","receivedAt":"2023-04-22T22:10:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Warn the user when unreachable commits are being left behind.\n\nRubén Justo (3):\n  checkout: move orphaned_commit_warning()\n  worktree: warn when removing a worktree with orphan commits\n  checkout: warn when unreachable commits after using --orphan\n\n builtin/checkout.c         | 132 ++-----------------------------------\n builtin/worktree.c         |   8 +++\n checkout.c                 | 132 +++++++++++++++++++++++++++++++++++++\n checkout.h                 |  10 +++\n t/t2020-checkout-detach.sh |   9 +++\n t/t2403-worktree-move.sh   |  10 +++\n 6 files changed, 175 insertions(+), 126 deletions(-)\n\n\nbase-commit: b28a910c4c1e6d7cbdc0663e75c2f5bc6b11eb20\n-- \n2.39.2\n"},{"id":"475882","messageId":"bf4835ac-4d39-8bfa-47e9-057d97fa0fff@gmail.com","threadId":"59637","inReplyTo":"f702476a-543a-da9b-ccd9-4431c80471e1@gmail.com","subject":"[PATCH 1/3] checkout: move orphaned_commit_warning()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-22T22:19:05Z","receivedAt":"2023-04-22T22:19:13Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave\ncommits behind, 2011-02-18) we introduced orphaned_commit_warning() in\nbuiltin/checkout.c.\n\nIn subsequent commits we're going to use orphaned_commit_warning() not\nonly from builtin/checkout.c, but from other builtin commands too.\n\nLet's move the function and its helpers to checkout.c and make it an\nAPI callable not just from builtin/checkout.c.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/checkout.c | 124 -------------------------------------------\n checkout.c         | 129 +++++++++++++++++++++++++++++++++++++++++++++\n checkout.h         |   9 ++++\n 3 files changed, 138 insertions(+), 124 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6f5d82ed3d..991413ef1a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -656,24 +656,6 @@ static void show_local_changes(struct object *head,\n \trelease_revisions(&rev);\n }\n \n-static void describe_detached_head(const char *msg, struct commit *commit)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n-\tif (!repo_parse_commit(the_repository, commit))\n-\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);\n-\tif (print_sha1_ellipsis()) {\n-\t\tfprintf(stderr, \"%s %s... %s\\n\", msg,\n-\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),\n-\t\t\tsb.buf);\n-\t} else {\n-\t\tfprintf(stderr, \"%s %s %s\\n\", msg,\n-\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),\n-\t\t\tsb.buf);\n-\t}\n-\tstrbuf_release(&sb);\n-}\n-\n static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \t\t      int worktree, int *writeout_error,\n \t\t      struct branch_info *info)\n@@ -1016,112 +998,6 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\treport_tracking(new_branch_info);\n }\n \n-static int add_pending_uninteresting_ref(const char *refname,\n-\t\t\t\t\t const struct object_id *oid,\n-\t\t\t\t\t int flags UNUSED, void *cb_data)\n-{\n-\tadd_pending_oid(cb_data, refname, oid, UNINTERESTING);\n-\treturn 0;\n-}\n-\n-static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n-{\n-\tstrbuf_addstr(sb, \"  \");\n-\tstrbuf_add_unique_abbrev(sb, &commit->object.oid, DEFAULT_ABBREV);\n-\tstrbuf_addch(sb, ' ');\n-\tif (!repo_parse_commit(the_repository, commit))\n-\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, sb);\n-\tstrbuf_addch(sb, '\\n');\n-}\n-\n-#define ORPHAN_CUTOFF 4\n-static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n-{\n-\tstruct commit *c, *last = NULL;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint lost = 0;\n-\twhile ((c = get_revision(revs)) != NULL) {\n-\t\tif (lost < ORPHAN_CUTOFF)\n-\t\t\tdescribe_one_orphan(&sb, c);\n-\t\tlast = c;\n-\t\tlost++;\n-\t}\n-\tif (ORPHAN_CUTOFF < lost) {\n-\t\tint more = lost - ORPHAN_CUTOFF;\n-\t\tif (more == 1)\n-\t\t\tdescribe_one_orphan(&sb, last);\n-\t\telse\n-\t\t\tstrbuf_addf(&sb, _(\" ... and %d more.\\n\"), more);\n-\t}\n-\n-\tfprintf(stderr,\n-\t\tQ_(\n-\t\t/* The singular version */\n-\t\t\"Warning: you are leaving %d commit behind, \"\n-\t\t\"not connected to\\n\"\n-\t\t\"any of your branches:\\n\\n\"\n-\t\t\"%s\\n\",\n-\t\t/* The plural version */\n-\t\t\"Warning: you are leaving %d commits behind, \"\n-\t\t\"not connected to\\n\"\n-\t\t\"any of your branches:\\n\\n\"\n-\t\t\"%s\\n\",\n-\t\t/* Give ngettext() the count */\n-\t\tlost),\n-\t\tlost,\n-\t\tsb.buf);\n-\tstrbuf_release(&sb);\n-\n-\tif (advice_enabled(ADVICE_DETACHED_HEAD))\n-\t\tfprintf(stderr,\n-\t\t\tQ_(\n-\t\t\t/* The singular version */\n-\t\t\t\"If you want to keep it by creating a new branch, \"\n-\t\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n-\t\t\t\" git branch <new-branch-name> %s\\n\\n\",\n-\t\t\t/* The plural version */\n-\t\t\t\"If you want to keep them by creating a new branch, \"\n-\t\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n-\t\t\t\" git branch <new-branch-name> %s\\n\\n\",\n-\t\t\t/* Give ngettext() the count */\n-\t\t\tlost),\n-\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));\n-}\n-\n-/*\n- * We are about to leave commit that was at the tip of a detached\n- * HEAD.  If it is not reachable from any ref, this is the last chance\n- * for the user to do so without resorting to reflog.\n- */\n-static void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)\n-{\n-\tstruct rev_info revs;\n-\tstruct object *object = &old_commit->object;\n-\n-\trepo_init_revisions(the_repository, &revs, NULL);\n-\tsetup_revisions(0, NULL, &revs, NULL);\n-\n-\tobject->flags &= ~UNINTERESTING;\n-\tadd_pending_object(&revs, object, oid_to_hex(&object->oid));\n-\n-\tfor_each_ref(add_pending_uninteresting_ref, &revs);\n-\tif (new_commit)\n-\t\tadd_pending_oid(&revs, \"HEAD\",\n-\t\t\t\t&new_commit->object.oid,\n-\t\t\t\tUNINTERESTING);\n-\n-\tif (prepare_revision_walk(&revs))\n-\t\tdie(_(\"internal error in revision walk\"));\n-\tif (!(old_commit->object.flags & UNINTERESTING))\n-\t\tsuggest_reattach(old_commit, &revs);\n-\telse\n-\t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old_commit);\n-\n-\t/* Clean up objects used, as they will be reused. */\n-\trepo_clear_commit_marks(the_repository, ALL_REV_FLAGS);\n-\trelease_revisions(&revs);\n-}\n-\n static int switch_branches(const struct checkout_opts *opts,\n \t\t\t   struct branch_info *new_branch_info)\n {\ndiff --git a/checkout.c b/checkout.c\nindex 04238b2713..18e7362043 100644\n--- a/checkout.c\n+++ b/checkout.c\n@@ -5,6 +5,11 @@\n #include \"checkout.h\"\n #include \"config.h\"\n #include \"strbuf.h\"\n+#include \"environment.h\"\n+#include \"revision.h\"\n+#include \"advice.h\"\n+#include \"hex.h\"\n+#include \"refs.h\"\n \n struct tracking_name_data {\n \t/* const */ char *src_ref;\n@@ -70,3 +75,127 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,\n \t}\n \treturn NULL;\n }\n+\n+void describe_detached_head(const char *msg, struct commit *commit)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!repo_parse_commit(the_repository, commit))\n+\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);\n+\tif (print_sha1_ellipsis()) {\n+\t\tfprintf(stderr, \"%s %s... %s\\n\", msg,\n+\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),\n+\t\t\tsb.buf);\n+\t} else {\n+\t\tfprintf(stderr, \"%s %s %s\\n\", msg,\n+\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),\n+\t\t\tsb.buf);\n+\t}\n+\tstrbuf_release(&sb);\n+}\n+\n+static int add_pending_uninteresting_ref(const char *refname,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t int flags UNUSED, void *cb_data)\n+{\n+\tadd_pending_oid(cb_data, refname, oid, UNINTERESTING);\n+\treturn 0;\n+}\n+\n+static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n+{\n+\tstrbuf_addstr(sb, \"  \");\n+\tstrbuf_add_unique_abbrev(sb, &commit->object.oid, DEFAULT_ABBREV);\n+\tstrbuf_addch(sb, ' ');\n+\tif (!repo_parse_commit(the_repository, commit))\n+\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, sb);\n+\tstrbuf_addch(sb, '\\n');\n+}\n+\n+#define ORPHAN_CUTOFF 4\n+static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n+{\n+\tstruct commit *c, *last = NULL;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint lost = 0;\n+\twhile ((c = get_revision(revs)) != NULL) {\n+\t\tif (lost < ORPHAN_CUTOFF)\n+\t\t\tdescribe_one_orphan(&sb, c);\n+\t\tlast = c;\n+\t\tlost++;\n+\t}\n+\tif (ORPHAN_CUTOFF < lost) {\n+\t\tint more = lost - ORPHAN_CUTOFF;\n+\t\tif (more == 1)\n+\t\t\tdescribe_one_orphan(&sb, last);\n+\t\telse\n+\t\t\tstrbuf_addf(&sb, _(\" ... and %d more.\\n\"), more);\n+\t}\n+\n+\tfprintf(stderr,\n+\t\tQ_(\n+\t\t/* The singular version */\n+\t\t\"Warning: you are leaving %d commit behind, \"\n+\t\t\"not connected to\\n\"\n+\t\t\"any of your branches:\\n\\n\"\n+\t\t\"%s\\n\",\n+\t\t/* The plural version */\n+\t\t\"Warning: you are leaving %d commits behind, \"\n+\t\t\"not connected to\\n\"\n+\t\t\"any of your branches:\\n\\n\"\n+\t\t\"%s\\n\",\n+\t\t/* Give ngettext() the count */\n+\t\tlost),\n+\t\tlost,\n+\t\tsb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tif (advice_enabled(ADVICE_DETACHED_HEAD))\n+\t\tfprintf(stderr,\n+\t\t\tQ_(\n+\t\t\t/* The singular version */\n+\t\t\t\"If you want to keep it by creating a new branch, \"\n+\t\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n+\t\t\t\" git branch <new-branch-name> %s\\n\\n\",\n+\t\t\t/* The plural version */\n+\t\t\t\"If you want to keep them by creating a new branch, \"\n+\t\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n+\t\t\t\" git branch <new-branch-name> %s\\n\\n\",\n+\t\t\t/* Give ngettext() the count */\n+\t\t\tlost),\n+\t\t\trepo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));\n+}\n+\n+/*\n+ * We are about to leave commit that was at the tip of a detached\n+ * HEAD.  If it is not reachable from any ref, this is the last chance\n+ * for the user to do so without resorting to reflog.\n+ */\n+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)\n+{\n+\tstruct rev_info revs;\n+\tstruct object *object = &old_commit->object;\n+\n+\trepo_init_revisions(the_repository, &revs, NULL);\n+\tsetup_revisions(0, NULL, &revs, NULL);\n+\n+\tobject->flags &= ~UNINTERESTING;\n+\tadd_pending_object(&revs, object, oid_to_hex(&object->oid));\n+\n+\tfor_each_ref(add_pending_uninteresting_ref, &revs);\n+\tif (new_commit)\n+\t\tadd_pending_oid(&revs, \"HEAD\",\n+\t\t\t\t&new_commit->object.oid,\n+\t\t\t\tUNINTERESTING);\n+\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(_(\"internal error in revision walk\"));\n+\tif (!(old_commit->object.flags & UNINTERESTING))\n+\t\tsuggest_reattach(old_commit, &revs);\n+\telse\n+\t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old_commit);\n+\n+\t/* Clean up objects used, as they will be reused. */\n+\trepo_clear_commit_marks(the_repository, ALL_REV_FLAGS);\n+\trelease_revisions(&revs);\n+}\ndiff --git a/checkout.h b/checkout.h\nindex 1917f3b323..c7dc056544 100644\n--- a/checkout.h\n+++ b/checkout.h\n@@ -2,6 +2,7 @@\n #define CHECKOUT_H\n \n #include \"hash.h\"\n+#include \"commit.h\"\n \n /*\n  * Check if the branch name uniquely matches a branch name on a remote\n@@ -12,4 +13,12 @@ const char *unique_tracking_name(const char *name,\n \t\t\t\t struct object_id *oid,\n \t\t\t\t int *dwim_remotes_matched);\n \n+/*\n+ * We are about to leave commit that was at the tip of a detached\n+ * HEAD.  If it is not reachable from any ref, this is the last chance\n+ * for the user to do so without resorting to reflog.\n+ */\n+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);\n+\n+void describe_detached_head(const char *msg, struct commit *commit);\n #endif /* CHECKOUT_H */\n-- \n2.39.2\n"},{"id":"475883","messageId":"1897dff1-bb4d-9715-dd1c-86763c052589@gmail.com","threadId":"59637","inReplyTo":"f702476a-543a-da9b-ccd9-4431c80471e1@gmail.com","subject":"[PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-22T22:19:21Z","receivedAt":"2023-04-22T22:19:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"While working in a detached worktree, the user can create some commits\nwhich won't be automatically connected to any ref.\n\nEventually, that worktree can be removed and, if the user has not\ncreated any ref connected to the HEAD in that worktree (e.g. branch,\ntag), those commits will become unreachable.\n\nLet's issue a warning to remind the user for safety, when deleting a\nworktree whose HEAD is not connected to an existing ref.\n\nLet's also add an option to modify the message we show in\norphaned_commit_warning(): \"Previous HEAD position was...\"; allowing to\nomit the word \"Previous\" as it may cause confusion, erroneously\nsuggesting that there is a \"Current HEAD\" while the worktree has been\nremoved.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/checkout.c       |  2 +-\n builtin/worktree.c       |  8 ++++++++\n checkout.c               |  7 +++++--\n checkout.h               |  3 ++-\n t/t2403-worktree-move.sh | 10 ++++++++++\n 5 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 991413ef1a..85ac4bca00 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1051,7 +1051,7 @@ static int switch_branches(const struct checkout_opts *opts,\n \t}\n \n \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n-\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);\n+\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);\n \n \tupdate_refs_for_switch(opts, &old_branch_info, new_branch_info);\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex a61bc32189..df269bccc8 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \n \t\tret |= delete_git_work_tree(wt);\n \t}\n+\n+\tif (!wt->head_ref && !is_null_oid(&wt->head_oid)) {\n+\t\tstruct commit* wt_commit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t  &wt->head_oid, 1);\n+\t\tif (wt_commit)\n+\t\t\torphaned_commit_warning(wt_commit, NULL, 0);\n+\t}\n+\n \t/*\n \t * continue on even if ret is non-zero, there's no going back\n \t * from here.\ndiff --git a/checkout.c b/checkout.c\nindex 18e7362043..5f7b0b3c49 100644\n--- a/checkout.c\n+++ b/checkout.c\n@@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n  * HEAD.  If it is not reachable from any ref, this is the last chance\n  * for the user to do so without resorting to reflog.\n  */\n-void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)\n+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n+\t\t\t     int show_previous_position)\n {\n \tstruct rev_info revs;\n \tstruct object *object = &old_commit->object;\n@@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi\n \t\tdie(_(\"internal error in revision walk\"));\n \tif (!(old_commit->object.flags & UNINTERESTING))\n \t\tsuggest_reattach(old_commit, &revs);\n-\telse\n+\telse if (show_previous_position)\n \t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old_commit);\n+\telse\n+\t\tdescribe_detached_head(_(\"HEAD position was\"), old_commit);\n \n \t/* Clean up objects used, as they will be reused. */\n \trepo_clear_commit_marks(the_repository, ALL_REV_FLAGS);\ndiff --git a/checkout.h b/checkout.h\nindex c7dc056544..ee400376d5 100644\n--- a/checkout.h\n+++ b/checkout.h\n@@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,\n  * HEAD.  If it is not reachable from any ref, this is the last chance\n  * for the user to do so without resorting to reflog.\n  */\n-void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);\n+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n+\t\t\t     int show_previous_position);\n \n void describe_detached_head(const char *msg, struct commit *commit);\n #endif /* CHECKOUT_H */\ndiff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\nindex 230a55e99a..f2756f7137 100755\n--- a/t/t2403-worktree-move.sh\n+++ b/t/t2403-worktree-move.sh\n@@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '\n \t)\n '\n \n+test_expect_success 'warn when removing a worktree with orphan commits' '\n+\tgit worktree add --detach foo &&\n+\tgit -C foo commit -m one --allow-empty &&\n+\tgit -C foo commit -m two --allow-empty &&\n+\tgit worktree remove foo 2>err &&\n+\ttest_i18ngrep \"you are leaving 2 commits behind\" err &&\n+\ttest_i18ngrep ! \"Previous HEAD position was\" err\n+\ttest_i18ngrep \"HEAD position was\" err\n+'\n+\n test_done\n-- \n2.39.2\n"},{"id":"475884","messageId":"417ae16c-9ba7-1e6d-c8d7-5b20a188b4fe@gmail.com","threadId":"59637","inReplyTo":"f702476a-543a-da9b-ccd9-4431c80471e1@gmail.com","subject":"[PATCH 3/3] checkout: warn when unreachable commits after using --orphan","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-22T22:19:42Z","receivedAt":"2023-04-22T22:19:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave\ncommits behind, 2011-02-18) we introduced a warning to be issued when,\nwhile checking out, the tip commit being left behind is not connected to\nany ref.\n\nWe assumed that if the commit to be checked out is the same commit\ncurrently checked out, we would omit the warning.  This makes sense\nbecause we're going to have HEAD pointing to the same commit anyway, so\nthere is nothing to warn about.\n\nHowever, with \"--orphan\" the target commit is not going to be used as\nHEAD in the worktree, but a new orphan branch being created, which is\nnot going to be connected to the previous commit.  Therefore, we need\nto check if the commit it is reachable and warn otherwise.\n\nLet's fix the condition we introduced in 8e2dc6ac06, considering the\n\"--orphan\" flag situation.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/checkout.c         | 8 ++++++--\n t/t2020-checkout-detach.sh | 9 +++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 85ac4bca00..7fad3161b4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1050,8 +1050,12 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\t}\n \t}\n \n-\tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n-\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);\n+\tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit) {\n+\t\tif (new_branch_info->commit != old_branch_info.commit)\n+\t\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);\n+\t\telse if (opts->new_orphan_branch)\n+\t\t\torphaned_commit_warning(old_branch_info.commit, NULL, 1);\n+\t}\n \n \tupdate_refs_for_switch(opts, &old_branch_info, new_branch_info);\n \ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 2eab6474f8..6762a9a572 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '\n \tcheck_orphan_warning stderr \"2 commits\"\n '\n \n+test_expect_success 'checkout --orphan warns on orphan commits' '\n+\tgit checkout \"$orphan2\" &&\n+\tgit checkout --orphan orphan 2>stderr\n+'\n+\n+test_expect_success 'checkout --orphan warns on orphan commits: output' '\n+\tcheck_orphan_warning stderr \"2 commits\"\n+'\n+\n test_expect_success 'checkout warns orphaning 1 of 2 commits' '\n \tgit checkout \"$orphan2\" &&\n \tgit checkout HEAD^ 2>stderr\n-- \n2.39.2\n"},{"id":"475971","messageId":"xmqq5y9lc9ep.fsf@gitster.g","threadId":"59637","inReplyTo":"1897dff1-bb4d-9715-dd1c-86763c052589@gmail.com","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T20:28:14Z","receivedAt":"2023-04-24T20:28:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> While working in a detached worktree, the user can create some commits\n> which won't be automatically connected to any ref.\n>\n> Eventually, that worktree can be removed and, if the user has not\n> created any ref connected to the HEAD in that worktree (e.g. branch,\n> tag), those commits will become unreachable.\n\nThe latter half of the first sentence feels a bit awkward, primarily\nit sounds as if it almost wants to hint that it is good if we\nconnected these commits to some ref automatically, and it is far\nfrom obvious why it is a good idea.  Perhaps\n\n    ... the user can create some commits on detached HEAD, that are\n    not connected to any ref.  If the user hasn't pointed at these\n    commits by refs before removing the worktree, those commits will\n    become unreachable.\n\nThat would be in line with the comment you moved in 1/3 that\ndescribes why orphaned_commit_warning() helper is there, i.e.\n\n    /*\n     * We are about to leave commit that was at the tip of a detached\n     * HEAD.  If it is not reachable from any ref, this is the last chance\n     * for the user to do so without resorting to reflog.\n     */\n\n> Let's issue a warning to remind the user for safety, when deleting a\n> worktree whose HEAD is not connected to an existing ref.\n\nGood idea.  \"Let's issue\" -> \"Issue\" (or \"Give\", \"Show\").\n\n> Let's also add an option to modify the message we show in\n> orphaned_commit_warning(): \"Previous HEAD position was...\"; allowing to\n> omit the word \"Previous\" as it may cause confusion, erroneously\n> suggesting that there is a \"Current HEAD\" while the worktree has been\n> removed.\n\nYes, it is absolutely necessary to adjust the message if you are to\nreuse the orphaned_commit_warning() helper so that it matches the\nsituation as the end-user experiences.\n\n>  \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n> -\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);\n> +\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);\n\nThe magic number \"1\" looks iffy.\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index a61bc32189..df269bccc8 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n>  \n>  \t\tret |= delete_git_work_tree(wt);\n>  \t}\n> +\n> +\tif (!wt->head_ref && !is_null_oid(&wt->head_oid)) {\n> +\t\tstruct commit* wt_commit = lookup_commit_reference_gently(the_repository,\n\nAsterisk sticks to the variable, not to type, in C.  If you write\n\n\tstruct commit *pointer, structure;\n\nit is clear only one is the pointer.  It misleads people if you wrote\n\n\tstruct commit* one, two;\n\ninstead.\n\n> +\t\t\t\t\t\t\t\t\t  &wt->head_oid, 1);\n\nAlso, lines around here look overly long.  Would it help to fold the\nline after the initialization assignment, i.e.\n\n\t\tstruct commit *wt_commit =\n\t\t\tlookup_commit_reference_gently(the_repository, ...);\n\n\n> +\t\tif (wt_commit)\n> +\t\t\torphaned_commit_warning(wt_commit, NULL, 0);\n\nAgain, the magic number \"0\" looks iffy.\n\n> diff --git a/checkout.c b/checkout.c\n> index 18e7362043..5f7b0b3c49 100644\n> --- a/checkout.c\n> +++ b/checkout.c\n> @@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n>   * HEAD.  If it is not reachable from any ref, this is the last chance\n>   * for the user to do so without resorting to reflog.\n>   */\n> -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)\n> +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n> +\t\t\t     int show_previous_position)\n>  {\n>  \tstruct rev_info revs;\n>  \tstruct object *object = &old_commit->object;\n> @@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi\n>  \t\tdie(_(\"internal error in revision walk\"));\n>  \tif (!(old_commit->object.flags & UNINTERESTING))\n>  \t\tsuggest_reattach(old_commit, &revs);\n> -\telse\n> +\telse if (show_previous_position)\n>  \t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old_commit);\n> +\telse\n> +\t\tdescribe_detached_head(_(\"HEAD position was\"), old_commit);\n\nCan we think of a single way to phrase this batter?  It's not like\nthe reason why the user wants to save the orphaned history is\nbecause it was at the PREVIOUS HEAD, or at the HEAD of a now-lost\nworking tree.  It is because the history leading to that commit is\nnow about to be lost.  So perhaps \"history leading to commit X has\nbecome unreachable\" or something would apply to both situation and\nwe do not have to pass the mysterious \"0\"/\"1\" that are hardcoded?\n\nIf the situation were the opposite and there were many ways that\nlead to lost history (i.e. not just the original \"switch out of the\ndetached HEAD\", we are now adding \"discarding a worktree with HEAD\ndetached\", and there may be more cases added in the future) that\nneed to be described differently, I would have instead suggested to\nuse an enum and use different phrasing for each case, but it does\nnot seem that the original \"Previous HEAD position was\" is so\nsuperbly phrased that we do not want to lose it, and the second one\nbeing added in the above hunk is not all that different.  If we can\nget away with just a single universal message, it would make things\nsimpler.\n\n> diff --git a/checkout.h b/checkout.h\n> index c7dc056544..ee400376d5 100644\n> --- a/checkout.h\n> +++ b/checkout.h\n> @@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,\n>   * HEAD.  If it is not reachable from any ref, this is the last chance\n>   * for the user to do so without resorting to reflog.\n>   */\n> -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);\n> +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n> +\t\t\t     int show_previous_position);\n>  \n>  void describe_detached_head(const char *msg, struct commit *commit);\n>  #endif /* CHECKOUT_H */\n> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\n> index 230a55e99a..f2756f7137 100755\n> --- a/t/t2403-worktree-move.sh\n> +++ b/t/t2403-worktree-move.sh\n> @@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'warn when removing a worktree with orphan commits' '\n> +\tgit worktree add --detach foo &&\n> +\tgit -C foo commit -m one --allow-empty &&\n> +\tgit -C foo commit -m two --allow-empty &&\n> +\tgit worktree remove foo 2>err &&\n> +\ttest_i18ngrep \"you are leaving 2 commits behind\" err &&\n> +\ttest_i18ngrep ! \"Previous HEAD position was\" err\n> +\ttest_i18ngrep \"HEAD position was\" err\n> +'\n> +\n>  test_done\n"},{"id":"476158","messageId":"51adbbcb-bbfa-58cc-03a4-82809c0c555e@gmail.com","threadId":"59637","inReplyTo":"xmqq5y9lc9ep.fsf@gitster.g","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-26T22:29:12Z","receivedAt":"2023-04-26T22:29:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 24-abr-2023 13:28:14, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > While working in a detached worktree, the user can create some commits\n> > which won't be automatically connected to any ref.\n> >\n> > Eventually, that worktree can be removed and, if the user has not\n> > created any ref connected to the HEAD in that worktree (e.g. branch,\n> > tag), those commits will become unreachable.\n> \n> The latter half of the first sentence feels a bit awkward, primarily\n> it sounds as if it almost wants to hint that it is good if we\n> connected these commits to some ref automatically, and it is far\n> from obvious why it is a good idea.  Perhaps\n> \n>     ... the user can create some commits on detached HEAD, that are\n>     not connected to any ref.  If the user hasn't pointed at these\n>     commits by refs before removing the worktree, those commits will\n>     become unreachable.\n> \n> That would be in line with the comment you moved in 1/3 that\n> describes why orphaned_commit_warning() helper is there, i.e.\n> \n>     /*\n>      * We are about to leave commit that was at the tip of a detached\n>      * HEAD.  If it is not reachable from any ref, this is the last chance\n>      * for the user to do so without resorting to reflog.\n>      */\n> \n\nOK.  I'll reword the message with that.\n\n> > Let's issue a warning to remind the user for safety, when deleting a\n> > worktree whose HEAD is not connected to an existing ref.\n> \n> Good idea.  \"Let's issue\" -> \"Issue\" (or \"Give\", \"Show\").\n\nOK.\n\n> \n> > Let's also add an option to modify the message we show in\n> > orphaned_commit_warning(): \"Previous HEAD position was...\"; allowing to\n> > omit the word \"Previous\" as it may cause confusion, erroneously\n> > suggesting that there is a \"Current HEAD\" while the worktree has been\n> > removed.\n> \n> Yes, it is absolutely necessary to adjust the message if you are to\n> reuse the orphaned_commit_warning() helper so that it matches the\n> situation as the end-user experiences.\n> \n> >  \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n> > -\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);\n> > +\t\torphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);\n> \n> The magic number \"1\" looks iffy.\n> \n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index a61bc32189..df269bccc8 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> >  \n> >  \t\tret |= delete_git_work_tree(wt);\n> >  \t}\n> > +\n> > +\tif (!wt->head_ref && !is_null_oid(&wt->head_oid)) {\n> > +\t\tstruct commit* wt_commit = lookup_commit_reference_gently(the_repository,\n> \n> Asterisk sticks to the variable, not to type, in C.  If you write\n> \n> \tstruct commit *pointer, structure;\n> \n> it is clear only one is the pointer.  It misleads people if you wrote\n> \n> \tstruct commit* one, two;\n> \n> instead.\n\nOK, sorry.\n\n> \n> > +\t\t\t\t\t\t\t\t\t  &wt->head_oid, 1);\n> \n> Also, lines around here look overly long.  Would it help to fold the\n> line after the initialization assignment, i.e.\n> \n> \t\tstruct commit *wt_commit =\n> \t\t\tlookup_commit_reference_gently(the_repository, ...);\n\nOK.\n\n> \n> \n> > +\t\tif (wt_commit)\n> > +\t\t\torphaned_commit_warning(wt_commit, NULL, 0);\n> \n> Again, the magic number \"0\" looks iffy.\n> \n> > diff --git a/checkout.c b/checkout.c\n> > index 18e7362043..5f7b0b3c49 100644\n> > --- a/checkout.c\n> > +++ b/checkout.c\n> > @@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n> >   * HEAD.  If it is not reachable from any ref, this is the last chance\n> >   * for the user to do so without resorting to reflog.\n> >   */\n> > -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)\n> > +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n> > +\t\t\t     int show_previous_position)\n> >  {\n> >  \tstruct rev_info revs;\n> >  \tstruct object *object = &old_commit->object;\n> > @@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi\n> >  \t\tdie(_(\"internal error in revision walk\"));\n> >  \tif (!(old_commit->object.flags & UNINTERESTING))\n> >  \t\tsuggest_reattach(old_commit, &revs);\n> > -\telse\n> > +\telse if (show_previous_position)\n> >  \t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old_commit);\n> > +\telse\n> > +\t\tdescribe_detached_head(_(\"HEAD position was\"), old_commit);\n> \n> Can we think of a single way to phrase this batter?  It's not like\n\nOK.\n\nThis is the current situation:\n\n   $ git checkout --detach \n   HEAD is now at 2efe05c commit-a\n\n   $ git checkout HEAD~1\n   Previous HEAD position was 2efe05c commit-a\n   HEAD is now at 7906992 commit-b\n\n   $ git worktree add test --detach && git worktree remove test\n   Preparing worktree (detached HEAD 7906992)\n   HEAD is now at 7906992 commit-b\n\nMaybe \"HEAD position was\" fits for both usages.  This is how it would\nlook like:\n\n   $ git checkout -\n   HEAD position was 7906992 commit-b\n   HEAD is now at 2efe05c commit-a\n\n   $ git worktree add test --detach && git worktree remove test\n   Preparing worktree (detached HEAD 2efe05c)\n   HEAD is now at 2efe05c commit-a\n   HEAD position was 2efe05c commit-a\n\nOr just \"HEAD was at\":\n\n   $ git checkout -\n   HEAD was at 2efe05c commit-a\n   HEAD is now at 7906992 commit-b\n\n   $ git worktree add test --detach && git worktree remove test\n   Preparing worktree (detached HEAD 7906992)\n   HEAD is now at 7906992 commit-b\n   HEAD was at 7906992 commit-b\n\nI think, if there are no objections or better suggestions, I'll re-roll\nwith \"HEAD was at\". \n\n> \n> > diff --git a/checkout.h b/checkout.h\n> > index c7dc056544..ee400376d5 100644\n> > --- a/checkout.h\n> > +++ b/checkout.h\n> > @@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,\n> >   * HEAD.  If it is not reachable from any ref, this is the last chance\n> >   * for the user to do so without resorting to reflog.\n> >   */\n> > -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);\n> > +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,\n> > +\t\t\t     int show_previous_position);\n> >  \n> >  void describe_detached_head(const char *msg, struct commit *commit);\n> >  #endif /* CHECKOUT_H */\n> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\n> > index 230a55e99a..f2756f7137 100755\n> > --- a/t/t2403-worktree-move.sh\n> > +++ b/t/t2403-worktree-move.sh\n> > @@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '\n> >  \t)\n> >  '\n> >  \n> > +test_expect_success 'warn when removing a worktree with orphan commits' '\n> > +\tgit worktree add --detach foo &&\n> > +\tgit -C foo commit -m one --allow-empty &&\n> > +\tgit -C foo commit -m two --allow-empty &&\n> > +\tgit worktree remove foo 2>err &&\n> > +\ttest_i18ngrep \"you are leaving 2 commits behind\" err &&\n> > +\ttest_i18ngrep ! \"Previous HEAD position was\" err\n> > +\ttest_i18ngrep \"HEAD position was\" err\n> > +'\n> > +\n> >  test_done\n\nThanks.\n"},{"id":"476160","messageId":"033bd734-9c32-e5a2-3f5c-c851e4e3bc5d@gmail.com","threadId":"59637","inReplyTo":"417ae16c-9ba7-1e6d-c8d7-5b20a188b4fe@gmail.com","subject":"Re: [PATCH 3/3] checkout: warn when unreachable commits after using --orphan","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-27T00:28:24Z","receivedAt":"2023-04-27T00:28:32Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 23/04/2023 00:19, Rubén Justo wrote:\n> diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\n> index 2eab6474f8..6762a9a572 100755\n> --- a/t/t2020-checkout-detach.sh\n> +++ b/t/t2020-checkout-detach.sh\n> @@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '\n>   \tcheck_orphan_warning stderr \"2 commits\"\n>   '\n>   \n> +test_expect_success 'checkout --orphan warns on orphan commits' '\n> +\tgit checkout \"$orphan2\" &&\n> +\tgit checkout --orphan orphan 2>stderr\n> +'\n> +\n> +test_expect_success 'checkout --orphan warns on orphan commits: output' '\n> +\tcheck_orphan_warning stderr \"2 commits\"\n> +'\n\nThese two tests could be a single test.\n\n\ttest_expect_success 'checkout --orphan warns on orphan commits' '\n\t\tgit checkout \"$orphan2\" &&\n\t\tgit checkout --orphan orphan 2>stderr &&\n\t\tcheck_orphan_warning stderr \"2 commits\"\n\t'\n\nValidating output like this in a separate step is an artifact of\nthe old way of checking localized strings.  Tests were split into\ntwo in f06f08b78c (\"i18n: mark checkout plural warning for\ntranslation\", 2011-04-10) and then prerequisite C_LOCALE_OUTPUT\nwas removed in f2c8c8007c (\"i18n: use test_i18ngrep in t2020,\nt2204, t3030, and t3200\", 2011-04-12).  Usage of test_i18ngrep\nwas then removed in 1108cea7f8 (\"tests: remove most uses of\ntest_i18ncmp\", 2021-02-11).\n\n> +\n>   test_expect_success 'checkout warns orphaning 1 of 2 commits' '\n>   \tgit checkout \"$orphan2\" &&\n>   \tgit checkout HEAD^ 2>stderr\n\n\n"},{"id":"476169","messageId":"xmqqwn1xnahy.fsf@gitster.g","threadId":"59637","inReplyTo":"51adbbcb-bbfa-58cc-03a4-82809c0c555e@gmail.com","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-27T05:46:01Z","receivedAt":"2023-04-27T05:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Maybe \"HEAD position was\" fits for both usages.  This is how it would\n> look like:\n> ...\n> I think, if there are no objections or better suggestions, I'll re-roll\n> with \"HEAD was at\". \n\nBut does it convey the more important point?  The reason why \"HEAD\nWAS at\" may matter is because the user is about to lose history\nleading to it.  I wonder if we want to be more direct and alarming,\ne.g.\n\n    $ git checkout -\n    About to lose history leading to 2efe05c commit-a\n    HEAD is now at 7906992 commit-b\n\nWhichever phrasing you end up using, I think the order of messages\nshould be made consistent between the two cases.  That is,\n\n> Maybe \"HEAD position was\" fits for both usages.  This is how it would\n> look like:\n>\n>    $ git checkout -\n>    HEAD position was 7906992 commit-b\n>    HEAD is now at 2efe05c commit-a\n\nHere \"git checkout\" reports the lost HEAD and then the end result.\n\n>    $ git worktree add test --detach && git worktree remove test\n>    Preparing worktree (detached HEAD 2efe05c)\n>    HEAD is now at 2efe05c commit-a\n>    HEAD position was 2efe05c commit-a\n\nBut here \"git worktree add\" reports the end resultfirst and then\nreports the lost HEAD.  It probably should report them in reverse.\n\nThanks.\n\n"},{"id":"476171","messageId":"CAPig+cRm1XaMzgDQZGS8vwpD_k8srQH0NGmziT_J7mtTHnR7OA@mail.gmail.com","threadId":"59637","inReplyTo":"xmqqwn1xnahy.fsf@gitster.g","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-04-27T06:16:28Z","receivedAt":"2023-04-27T06:16:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 27, 2023 at 1:50 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Whichever phrasing you end up using, I think the order of messages\n> should be made consistent between the two cases.  That is,\n>\n> > Maybe \"HEAD position was\" fits for both usages.  This is how it would\n> > look like:\n> >\n> >    $ git checkout -\n> >    HEAD position was 7906992 commit-b\n> >    HEAD is now at 2efe05c commit-a\n>\n> Here \"git checkout\" reports the lost HEAD and then the end result.\n>\n> >    $ git worktree add test --detach && git worktree remove test\n> >    Preparing worktree (detached HEAD 2efe05c)\n> >    HEAD is now at 2efe05c commit-a\n> >    HEAD position was 2efe05c commit-a\n>\n> But here \"git worktree add\" reports the end resultfirst and then\n> reports the lost HEAD.  It probably should report them in reverse.\n\nThere may be a misunderstanding here due to the unfortunate\nconstruction of Rubén's example which muddles together the output of\n`git worktree add` and `git worktree remove`. For clarity, his example\nshould probably have been written:\n\n  $ git worktree add test --detach\n  Preparing worktree (detached HEAD 2efe05c)\n  HEAD is now at 2efe05c commit-a\n  $ git worktree remove test\n  HEAD position was 2efe05c commit-a\n\nalthough showing only the `git worktree remove` command would probably\nhave been even clearer.\n\nSuch example output does a good job of arguing in favor of your\nsuggestion to use phrasing which is more alarming:\n\n  $ git checkout -\n  Commit 2efe05c \"commit-a\" left dangling\n  HEAD is now at 7906992 commit-b\n\n  $ git worktree remove test\n  Commit 2efe05c \"commit-a\" left dangling\n\n(Hopefully someone can come up with better wording than \"About to lose\nhistory leading to\" and \"Commit ... left dangling\", neither of which\nsound quite right.)\n"},{"id":"476248","messageId":"592c7265-c8af-bd3c-561d-be22e34d61d6@gmail.com","threadId":"59637","inReplyTo":"xmqqwn1xnahy.fsf@gitster.g","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-27T23:08:00Z","receivedAt":"2023-04-27T23:08:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 26-abr-2023 22:46:01, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n\nThe message: \"Previous HEAD position was\", which we have since dc9195ac78\n(Let git-checkout always drop any detached head, 2007-02-03), describes\na detached HEAD that has been left behind.\n\nIn 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave\ncommits behind, 2011-02-18) we moved this message to a new function,\norphaned_commit_warning().  We still show the message if the HEAD left\nbehind is detached.  However, if the HEAD left behind is detached _and_\n_not connected_ to any ref, instead of the original message, we show a\nwarning.\n\nIn this series, we want to use that function to show the same warning\nwhen the user removes a worktree whose HEAD is detached and _not\nconnected_ to any ref.  However, if the HEAD is detached but connected,\nthe original message introduced in dc9195ac78 needs to be adjusted.\n\n> > Maybe \"HEAD position was\" fits for both usages.  This is how it would\n> > look like:\n> > ...\n> > I think, if there are no objections or better suggestions, I'll re-roll\n> > with \"HEAD was at\". \n\nThis is about the message introduced in dc9195ac78, but...\n\n> But does it convey the more important point?  The reason why \"HEAD\n\nI think you are referring to the warning.\n\nStarting from a situation like:\n\n   $ git checkout -b foo\n   Switched to a new branch 'foo'\n\n   $ git checkout --detach\n   HEAD is now at 47ab99a\n\n   $ git commit --allow-empty -m dangling\n   [detached HEAD 398a1b0] dangling\n\n   $ git worktree add --detach foo-wt\n   Preparing worktree (detached HEAD 398a1b0)\n   HEAD is now at 398a1b0 dangling\n\nIf we switch to 'foo' in the current worktree, the message is:\n\n   $ git checkout foo\n   Warning: you are leaving 1 commit behind, not connected to\n   any of your branches:\n   \n     398a1b0 dangling \n   \n   If you want to keep it by creating a new branch, this may be a good time\n   to do so with:\n   \n    git branch <new-branch-name> 398a1b0\n   \n   Switched to branch 'foo'\n\nAnd -- this is what we are adding in this series -- the same message if\nwe remove the worktree 'foo-wt':\n\n   $ git worktree remove foo-wt\n   Warning: you are leaving 1 commit behind, not connected to\n   any of your branches:\n   \n     398a1b0 dangling \n   \n   If you want to keep it by creating a new branch, this may be a good time\n   to do so with:\n   \n    git branch <new-branch-name> 398a1b0\n\n> > Maybe \"HEAD position was\" fits for both usages.  This is how it would\n> > look like:\n> >\n> >    $ git checkout -\n> >    HEAD position was 7906992 commit-b\n> >    HEAD is now at 2efe05c commit-a\n> \n> Here \"git checkout\" reports the lost HEAD and then the end result.\n> \n> >    $ git worktree add test --detach && git worktree remove test\n> >    Preparing worktree (detached HEAD 2efe05c)\n> >    HEAD is now at 2efe05c commit-a\n> >    HEAD position was 2efe05c commit-a\n\nI apologize, the examples were confusing.  I though it was a good idea\nto show the new message next to other messages where we also refer to\nthe HEAD position.\n"},{"id":"476249","messageId":"a746089f-945e-df64-d261-61a212f79c7b@gmail.com","threadId":"59637","inReplyTo":"033bd734-9c32-e5a2-3f5c-c851e4e3bc5d@gmail.com","subject":"Re: [PATCH 3/3] checkout: warn when unreachable commits after using --orphan","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-27T23:09:36Z","receivedAt":"2023-04-27T23:09:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 27/4/23 2:28, Andrei Rybak wrote:\n> On 23/04/2023 00:19, Rubén Justo wrote:\n>> diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\n>> index 2eab6474f8..6762a9a572 100755\n>> --- a/t/t2020-checkout-detach.sh\n>> +++ b/t/t2020-checkout-detach.sh\n>> @@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '\n>>       check_orphan_warning stderr \"2 commits\"\n>>   '\n>>   +test_expect_success 'checkout --orphan warns on orphan commits' '\n>> +    git checkout \"$orphan2\" &&\n>> +    git checkout --orphan orphan 2>stderr\n>> +'\n>> +\n>> +test_expect_success 'checkout --orphan warns on orphan commits: output' '\n>> +    check_orphan_warning stderr \"2 commits\"\n>> +'\n> \n> These two tests could be a single test.\n> \n>     test_expect_success 'checkout --orphan warns on orphan commits' '\n>         git checkout \"$orphan2\" &&\n>         git checkout --orphan orphan 2>stderr &&\n>         check_orphan_warning stderr \"2 commits\"\n> \n\nOK\n    '\n> \n> Validating output like this in a separate step is an artifact of\n> the old way of checking localized strings.  Tests were split into\n> two in f06f08b78c (\"i18n: mark checkout plural warning for\n> translation\", 2011-04-10) and then prerequisite C_LOCALE_OUTPUT\n> was removed in f2c8c8007c (\"i18n: use test_i18ngrep in t2020,\n> t2204, t3030, and t3200\", 2011-04-12).  Usage of test_i18ngrep\n> was then removed in 1108cea7f8 (\"tests: remove most uses of\n> test_i18ncmp\", 2021-02-11).\n\nThank you!\n"},{"id":"476252","messageId":"xmqqpm7ohluf.fsf@gitster.g","threadId":"59637","inReplyTo":"CAPig+cRm1XaMzgDQZGS8vwpD_k8srQH0NGmziT_J7mtTHnR7OA@mail.gmail.com","subject":"Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T00:49:44Z","receivedAt":"2023-04-28T00:49:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> There may be a misunderstanding here due to the unfortunate\n> construction of Rubén's example which muddles together the output of\n> `git worktree add` and `git worktree remove`. For clarity, his example\n> should probably have been written:\n>\n>   $ git worktree add test --detach\n>   Preparing worktree (detached HEAD 2efe05c)\n>   HEAD is now at 2efe05c commit-a\n>   $ git worktree remove test\n>   HEAD position was 2efe05c commit-a\n>\n> although showing only the `git worktree remove` command would probably\n> have been even clearer.\n\nAh, you are absolutely right.\n\nMy \"huh?\" against the apparent inconsistency between \"checkout\" and\n\"worktree\" regarding the order of \"this is the end result\" vs \"this\nis what we left behind\" does not exist, as \"worktree remove\" does\nnot involve being newly on a detached HEAD and it is the one that\nmay introduce a newly abandoned line of history.  So everything\nmakes sense.\n\n> Such example output does a good job of arguing in favor of your\n> suggestion to use phrasing which is more alarming:\n>\n>   $ git checkout -\n>   Commit 2efe05c \"commit-a\" left dangling\n>   HEAD is now at 7906992 commit-b\n>\n>   $ git worktree remove test\n>   Commit 2efe05c \"commit-a\" left dangling\n>\n> (Hopefully someone can come up with better wording than \"About to lose\n> history leading to\" and \"Commit ... left dangling\", neither of which\n> sound quite right.)\n\nYup, I am obviously worse at phrasing this than you are ;-) We'd\nneed a good wording that is alarming, even for those who squelch\nmost of the warning given via the advise system, without becoming\ntoo verbose.\n\nThanks.\n"}]}