[PATCH 0/3] warn when unreachable commits are left behind

[PATCH 0/3] warn when unreachable commits are left behind

[Rubén Justo]

Warn the user when unreachable commits are being left behind.

Rubén Justo (3):
  checkout: move orphaned_commit_warning()
  worktree: warn when removing a worktree with orphan commits
  checkout: warn when unreachable commits after using --orphan

 builtin/checkout.c         | 132 ++-----------------------------------
 builtin/worktree.c         |   8 +++
 checkout.c                 | 132 +++++++++++++++++++++++++++++++++++++
 checkout.h                 |  10 +++
 t/t2020-checkout-detach.sh |   9 +++
 t/t2403-worktree-move.sh   |  10 +++
 6 files changed, 175 insertions(+), 126 deletions(-)


base-commit: b28a910c4c1e6d7cbdc0663e75c2f5bc6b11eb20
-- 
2.39.2

[PATCH 1/3] checkout: move orphaned_commit_warning()

[Rubén Justo]

In 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave
commits behind, 2011-02-18) we introduced orphaned_commit_warning() in
builtin/checkout.c.

In subsequent commits we're going to use orphaned_commit_warning() not
only from builtin/checkout.c, but from other builtin commands too.

Let's move the function and its helpers to checkout.c and make it an
API callable not just from builtin/checkout.c.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/checkout.c | 124 -------------------------------------------
 checkout.c         | 129 +++++++++++++++++++++++++++++++++++++++++++++
 checkout.h         |   9 ++++
 3 files changed, 138 insertions(+), 124 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6f5d82ed3d..991413ef1a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -656,24 +656,6 @@ static void show_local_changes(struct object *head,
 	release_revisions(&rev);
 }
 
-static void describe_detached_head(const char *msg, struct commit *commit)
-{
-	struct strbuf sb = STRBUF_INIT;
-
-	if (!repo_parse_commit(the_repository, commit))
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
-	if (print_sha1_ellipsis()) {
-		fprintf(stderr, "%s %s... %s\n", msg,
-			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),
-			sb.buf);
-	} else {
-		fprintf(stderr, "%s %s %s\n", msg,
-			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),
-			sb.buf);
-	}
-	strbuf_release(&sb);
-}
-
 static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 		      int worktree, int *writeout_error,
 		      struct branch_info *info)
@@ -1016,112 +998,6 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 		report_tracking(new_branch_info);
 }
 
-static int add_pending_uninteresting_ref(const char *refname,
-					 const struct object_id *oid,
-					 int flags UNUSED, void *cb_data)
-{
-	add_pending_oid(cb_data, refname, oid, UNINTERESTING);
-	return 0;
-}
-
-static void describe_one_orphan(struct strbuf *sb, struct commit *commit)
-{
-	strbuf_addstr(sb, "  ");
-	strbuf_add_unique_abbrev(sb, &commit->object.oid, DEFAULT_ABBREV);
-	strbuf_addch(sb, ' ');
-	if (!repo_parse_commit(the_repository, commit))
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, sb);
-	strbuf_addch(sb, '\n');
-}
-
-#define ORPHAN_CUTOFF 4
-static void suggest_reattach(struct commit *commit, struct rev_info *revs)
-{
-	struct commit *c, *last = NULL;
-	struct strbuf sb = STRBUF_INIT;
-	int lost = 0;
-	while ((c = get_revision(revs)) != NULL) {
-		if (lost < ORPHAN_CUTOFF)
-			describe_one_orphan(&sb, c);
-		last = c;
-		lost++;
-	}
-	if (ORPHAN_CUTOFF < lost) {
-		int more = lost - ORPHAN_CUTOFF;
-		if (more == 1)
-			describe_one_orphan(&sb, last);
-		else
-			strbuf_addf(&sb, _(" ... and %d more.\n"), more);
-	}
-
-	fprintf(stderr,
-		Q_(
-		/* The singular version */
-		"Warning: you are leaving %d commit behind, "
-		"not connected to\n"
-		"any of your branches:\n\n"
-		"%s\n",
-		/* The plural version */
-		"Warning: you are leaving %d commits behind, "
-		"not connected to\n"
-		"any of your branches:\n\n"
-		"%s\n",
-		/* Give ngettext() the count */
-		lost),
-		lost,
-		sb.buf);
-	strbuf_release(&sb);
-
-	if (advice_enabled(ADVICE_DETACHED_HEAD))
-		fprintf(stderr,
-			Q_(
-			/* The singular version */
-			"If you want to keep it by creating a new branch, "
-			"this may be a good time\nto do so with:\n\n"
-			" git branch <new-branch-name> %s\n\n",
-			/* The plural version */
-			"If you want to keep them by creating a new branch, "
-			"this may be a good time\nto do so with:\n\n"
-			" git branch <new-branch-name> %s\n\n",
-			/* Give ngettext() the count */
-			lost),
-			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
-}
-
-/*
- * We are about to leave commit that was at the tip of a detached
- * HEAD.  If it is not reachable from any ref, this is the last chance
- * for the user to do so without resorting to reflog.
- */
-static void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)
-{
-	struct rev_info revs;
-	struct object *object = &old_commit->object;
-
-	repo_init_revisions(the_repository, &revs, NULL);
-	setup_revisions(0, NULL, &revs, NULL);
-
-	object->flags &= ~UNINTERESTING;
-	add_pending_object(&revs, object, oid_to_hex(&object->oid));
-
-	for_each_ref(add_pending_uninteresting_ref, &revs);
-	if (new_commit)
-		add_pending_oid(&revs, "HEAD",
-				&new_commit->object.oid,
-				UNINTERESTING);
-
-	if (prepare_revision_walk(&revs))
-		die(_("internal error in revision walk"));
-	if (!(old_commit->object.flags & UNINTERESTING))
-		suggest_reattach(old_commit, &revs);
-	else
-		describe_detached_head(_("Previous HEAD position was"), old_commit);
-
-	/* Clean up objects used, as they will be reused. */
-	repo_clear_commit_marks(the_repository, ALL_REV_FLAGS);
-	release_revisions(&revs);
-}
-
 static int switch_branches(const struct checkout_opts *opts,
 			   struct branch_info *new_branch_info)
 {
diff --git a/checkout.c b/checkout.c
index 04238b2713..18e7362043 100644
--- a/checkout.c
+++ b/checkout.c
@@ -5,6 +5,11 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "environment.h"
+#include "revision.h"
+#include "advice.h"
+#include "hex.h"
+#include "refs.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;
@@ -70,3 +75,127 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,
 	}
 	return NULL;
 }
+
+void describe_detached_head(const char *msg, struct commit *commit)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!repo_parse_commit(the_repository, commit))
+		pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
+	if (print_sha1_ellipsis()) {
+		fprintf(stderr, "%s %s... %s\n", msg,
+			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),
+			sb.buf);
+	} else {
+		fprintf(stderr, "%s %s %s\n", msg,
+			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),
+			sb.buf);
+	}
+	strbuf_release(&sb);
+}
+
+static int add_pending_uninteresting_ref(const char *refname,
+					 const struct object_id *oid,
+					 int flags UNUSED, void *cb_data)
+{
+	add_pending_oid(cb_data, refname, oid, UNINTERESTING);
+	return 0;
+}
+
+static void describe_one_orphan(struct strbuf *sb, struct commit *commit)
+{
+	strbuf_addstr(sb, "  ");
+	strbuf_add_unique_abbrev(sb, &commit->object.oid, DEFAULT_ABBREV);
+	strbuf_addch(sb, ' ');
+	if (!repo_parse_commit(the_repository, commit))
+		pp_commit_easy(CMIT_FMT_ONELINE, commit, sb);
+	strbuf_addch(sb, '\n');
+}
+
+#define ORPHAN_CUTOFF 4
+static void suggest_reattach(struct commit *commit, struct rev_info *revs)
+{
+	struct commit *c, *last = NULL;
+	struct strbuf sb = STRBUF_INIT;
+	int lost = 0;
+	while ((c = get_revision(revs)) != NULL) {
+		if (lost < ORPHAN_CUTOFF)
+			describe_one_orphan(&sb, c);
+		last = c;
+		lost++;
+	}
+	if (ORPHAN_CUTOFF < lost) {
+		int more = lost - ORPHAN_CUTOFF;
+		if (more == 1)
+			describe_one_orphan(&sb, last);
+		else
+			strbuf_addf(&sb, _(" ... and %d more.\n"), more);
+	}
+
+	fprintf(stderr,
+		Q_(
+		/* The singular version */
+		"Warning: you are leaving %d commit behind, "
+		"not connected to\n"
+		"any of your branches:\n\n"
+		"%s\n",
+		/* The plural version */
+		"Warning: you are leaving %d commits behind, "
+		"not connected to\n"
+		"any of your branches:\n\n"
+		"%s\n",
+		/* Give ngettext() the count */
+		lost),
+		lost,
+		sb.buf);
+	strbuf_release(&sb);
+
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
+		fprintf(stderr,
+			Q_(
+			/* The singular version */
+			"If you want to keep it by creating a new branch, "
+			"this may be a good time\nto do so with:\n\n"
+			" git branch <new-branch-name> %s\n\n",
+			/* The plural version */
+			"If you want to keep them by creating a new branch, "
+			"this may be a good time\nto do so with:\n\n"
+			" git branch <new-branch-name> %s\n\n",
+			/* Give ngettext() the count */
+			lost),
+			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
+}
+
+/*
+ * We are about to leave commit that was at the tip of a detached
+ * HEAD.  If it is not reachable from any ref, this is the last chance
+ * for the user to do so without resorting to reflog.
+ */
+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)
+{
+	struct rev_info revs;
+	struct object *object = &old_commit->object;
+
+	repo_init_revisions(the_repository, &revs, NULL);
+	setup_revisions(0, NULL, &revs, NULL);
+
+	object->flags &= ~UNINTERESTING;
+	add_pending_object(&revs, object, oid_to_hex(&object->oid));
+
+	for_each_ref(add_pending_uninteresting_ref, &revs);
+	if (new_commit)
+		add_pending_oid(&revs, "HEAD",
+				&new_commit->object.oid,
+				UNINTERESTING);
+
+	if (prepare_revision_walk(&revs))
+		die(_("internal error in revision walk"));
+	if (!(old_commit->object.flags & UNINTERESTING))
+		suggest_reattach(old_commit, &revs);
+	else
+		describe_detached_head(_("Previous HEAD position was"), old_commit);
+
+	/* Clean up objects used, as they will be reused. */
+	repo_clear_commit_marks(the_repository, ALL_REV_FLAGS);
+	release_revisions(&revs);
+}
diff --git a/checkout.h b/checkout.h
index 1917f3b323..c7dc056544 100644
--- a/checkout.h
+++ b/checkout.h
@@ -2,6 +2,7 @@
 #define CHECKOUT_H
 
 #include "hash.h"
+#include "commit.h"
 
 /*
  * Check if the branch name uniquely matches a branch name on a remote
@@ -12,4 +13,12 @@ const char *unique_tracking_name(const char *name,
 				 struct object_id *oid,
 				 int *dwim_remotes_matched);
 
+/*
+ * We are about to leave commit that was at the tip of a detached
+ * HEAD.  If it is not reachable from any ref, this is the last chance
+ * for the user to do so without resorting to reflog.
+ */
+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);
+
+void describe_detached_head(const char *msg, struct commit *commit);
 #endif /* CHECKOUT_H */
-- 
2.39.2

[PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Rubén Justo]

While working in a detached worktree, the user can create some commits
which won't be automatically connected to any ref.

Eventually, that worktree can be removed and, if the user has not
created any ref connected to the HEAD in that worktree (e.g. branch,
tag), those commits will become unreachable.

Let's issue a warning to remind the user for safety, when deleting a
worktree whose HEAD is not connected to an existing ref.

Let's also add an option to modify the message we show in
orphaned_commit_warning(): "Previous HEAD position was..."; allowing to
omit the word "Previous" as it may cause confusion, erroneously
suggesting that there is a "Current HEAD" while the worktree has been
removed.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/checkout.c       |  2 +-
 builtin/worktree.c       |  8 ++++++++
 checkout.c               |  7 +++++--
 checkout.h               |  3 ++-
 t/t2403-worktree-move.sh | 10 ++++++++++
 5 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 991413ef1a..85ac4bca00 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1051,7 +1051,7 @@ static int switch_branches(const struct checkout_opts *opts,
 	}
 
 	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
-		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
+		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);
 
 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a61bc32189..df269bccc8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 
 		ret |= delete_git_work_tree(wt);
 	}
+
+	if (!wt->head_ref && !is_null_oid(&wt->head_oid)) {
+		struct commit* wt_commit = lookup_commit_reference_gently(the_repository,
+									  &wt->head_oid, 1);
+		if (wt_commit)
+			orphaned_commit_warning(wt_commit, NULL, 0);
+	}
+
 	/*
 	 * continue on even if ret is non-zero, there's no going back
 	 * from here.
diff --git a/checkout.c b/checkout.c
index 18e7362043..5f7b0b3c49 100644
--- a/checkout.c
+++ b/checkout.c
@@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
  * HEAD.  If it is not reachable from any ref, this is the last chance
  * for the user to do so without resorting to reflog.
  */
-void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)
+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
+			     int show_previous_position)
 {
 	struct rev_info revs;
 	struct object *object = &old_commit->object;
@@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi
 		die(_("internal error in revision walk"));
 	if (!(old_commit->object.flags & UNINTERESTING))
 		suggest_reattach(old_commit, &revs);
-	else
+	else if (show_previous_position)
 		describe_detached_head(_("Previous HEAD position was"), old_commit);
+	else
+		describe_detached_head(_("HEAD position was"), old_commit);
 
 	/* Clean up objects used, as they will be reused. */
 	repo_clear_commit_marks(the_repository, ALL_REV_FLAGS);
diff --git a/checkout.h b/checkout.h
index c7dc056544..ee400376d5 100644
--- a/checkout.h
+++ b/checkout.h
@@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,
  * HEAD.  If it is not reachable from any ref, this is the last chance
  * for the user to do so without resorting to reflog.
  */
-void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);
+void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
+			     int show_previous_position);
 
 void describe_detached_head(const char *msg, struct commit *commit);
 #endif /* CHECKOUT_H */
diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
index 230a55e99a..f2756f7137 100755
--- a/t/t2403-worktree-move.sh
+++ b/t/t2403-worktree-move.sh
@@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '
 	)
 '
 
+test_expect_success 'warn when removing a worktree with orphan commits' '
+	git worktree add --detach foo &&
+	git -C foo commit -m one --allow-empty &&
+	git -C foo commit -m two --allow-empty &&
+	git worktree remove foo 2>err &&
+	test_i18ngrep "you are leaving 2 commits behind" err &&
+	test_i18ngrep ! "Previous HEAD position was" err
+	test_i18ngrep "HEAD position was" err
+'
+
 test_done
-- 
2.39.2

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> While working in a detached worktree, the user can create some commits
> which won't be automatically connected to any ref.
>
> Eventually, that worktree can be removed and, if the user has not
> created any ref connected to the HEAD in that worktree (e.g. branch,
> tag), those commits will become unreachable.

The latter half of the first sentence feels a bit awkward, primarily
it sounds as if it almost wants to hint that it is good if we
connected these commits to some ref automatically, and it is far
from obvious why it is a good idea.  Perhaps

    ... the user can create some commits on detached HEAD, that are
    not connected to any ref.  If the user hasn't pointed at these
    commits by refs before removing the worktree, those commits will
    become unreachable.

That would be in line with the comment you moved in 1/3 that
describes why orphaned_commit_warning() helper is there, i.e.

    /*
     * We are about to leave commit that was at the tip of a detached
     * HEAD.  If it is not reachable from any ref, this is the last chance
     * for the user to do so without resorting to reflog.
     */

> Let's issue a warning to remind the user for safety, when deleting a
> worktree whose HEAD is not connected to an existing ref.

Good idea.  "Let's issue" -> "Issue" (or "Give", "Show").

> Let's also add an option to modify the message we show in
> orphaned_commit_warning(): "Previous HEAD position was..."; allowing to
> omit the word "Previous" as it may cause confusion, erroneously
> suggesting that there is a "Current HEAD" while the worktree has been
> removed.

Yes, it is absolutely necessary to adjust the message if you are to
reuse the orphaned_commit_warning() helper so that it matches the
situation as the end-user experiences.

>  	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
> -		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
> +		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);

The magic number "1" looks iffy.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index a61bc32189..df269bccc8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>  
>  		ret |= delete_git_work_tree(wt);
>  	}
> +
> +	if (!wt->head_ref && !is_null_oid(&wt->head_oid)) {
> +		struct commit* wt_commit = lookup_commit_reference_gently(the_repository,

Asterisk sticks to the variable, not to type, in C.  If you write

	struct commit *pointer, structure;

it is clear only one is the pointer.  It misleads people if you wrote

	struct commit* one, two;

instead.

> +									  &wt->head_oid, 1);

Also, lines around here look overly long.  Would it help to fold the
line after the initialization assignment, i.e.

		struct commit *wt_commit =
			lookup_commit_reference_gently(the_repository, ...);


> +		if (wt_commit)
> +			orphaned_commit_warning(wt_commit, NULL, 0);

Again, the magic number "0" looks iffy.

> diff --git a/checkout.c b/checkout.c
> index 18e7362043..5f7b0b3c49 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
>   * HEAD.  If it is not reachable from any ref, this is the last chance
>   * for the user to do so without resorting to reflog.
>   */
> -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)
> +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
> +			     int show_previous_position)
>  {
>  	struct rev_info revs;
>  	struct object *object = &old_commit->object;
> @@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi
>  		die(_("internal error in revision walk"));
>  	if (!(old_commit->object.flags & UNINTERESTING))
>  		suggest_reattach(old_commit, &revs);
> -	else
> +	else if (show_previous_position)
>  		describe_detached_head(_("Previous HEAD position was"), old_commit);
> +	else
> +		describe_detached_head(_("HEAD position was"), old_commit);

Can we think of a single way to phrase this batter?  It's not like
the reason why the user wants to save the orphaned history is
because it was at the PREVIOUS HEAD, or at the HEAD of a now-lost
working tree.  It is because the history leading to that commit is
now about to be lost.  So perhaps "history leading to commit X has
become unreachable" or something would apply to both situation and
we do not have to pass the mysterious "0"/"1" that are hardcoded?

If the situation were the opposite and there were many ways that
lead to lost history (i.e. not just the original "switch out of the
detached HEAD", we are now adding "discarding a worktree with HEAD
detached", and there may be more cases added in the future) that
need to be described differently, I would have instead suggested to
use an enum and use different phrasing for each case, but it does
not seem that the original "Previous HEAD position was" is so
superbly phrased that we do not want to lose it, and the second one
being added in the above hunk is not all that different.  If we can
get away with just a single universal message, it would make things
simpler.

> diff --git a/checkout.h b/checkout.h
> index c7dc056544..ee400376d5 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,
>   * HEAD.  If it is not reachable from any ref, this is the last chance
>   * for the user to do so without resorting to reflog.
>   */
> -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);
> +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
> +			     int show_previous_position);
>  
>  void describe_detached_head(const char *msg, struct commit *commit);
>  #endif /* CHECKOUT_H */
> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> index 230a55e99a..f2756f7137 100755
> --- a/t/t2403-worktree-move.sh
> +++ b/t/t2403-worktree-move.sh
> @@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '
>  	)
>  '
>  
> +test_expect_success 'warn when removing a worktree with orphan commits' '
> +	git worktree add --detach foo &&
> +	git -C foo commit -m one --allow-empty &&
> +	git -C foo commit -m two --allow-empty &&
> +	git worktree remove foo 2>err &&
> +	test_i18ngrep "you are leaving 2 commits behind" err &&
> +	test_i18ngrep ! "Previous HEAD position was" err
> +	test_i18ngrep "HEAD position was" err
> +'
> +
>  test_done

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Rubén Justo]

On 24-abr-2023 13:28:14, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > While working in a detached worktree, the user can create some commits
> > which won't be automatically connected to any ref.
> >
> > Eventually, that worktree can be removed and, if the user has not
> > created any ref connected to the HEAD in that worktree (e.g. branch,
> > tag), those commits will become unreachable.
> 
> The latter half of the first sentence feels a bit awkward, primarily
> it sounds as if it almost wants to hint that it is good if we
> connected these commits to some ref automatically, and it is far
> from obvious why it is a good idea.  Perhaps
> 
>     ... the user can create some commits on detached HEAD, that are
>     not connected to any ref.  If the user hasn't pointed at these
>     commits by refs before removing the worktree, those commits will
>     become unreachable.
> 
> That would be in line with the comment you moved in 1/3 that
> describes why orphaned_commit_warning() helper is there, i.e.
> 
>     /*
>      * We are about to leave commit that was at the tip of a detached
>      * HEAD.  If it is not reachable from any ref, this is the last chance
>      * for the user to do so without resorting to reflog.
>      */
> 

OK.  I'll reword the message with that.

> > Let's issue a warning to remind the user for safety, when deleting a
> > worktree whose HEAD is not connected to an existing ref.
> 
> Good idea.  "Let's issue" -> "Issue" (or "Give", "Show").

OK.

> 
> > Let's also add an option to modify the message we show in
> > orphaned_commit_warning(): "Previous HEAD position was..."; allowing to
> > omit the word "Previous" as it may cause confusion, erroneously
> > suggesting that there is a "Current HEAD" while the worktree has been
> > removed.
> 
> Yes, it is absolutely necessary to adjust the message if you are to
> reuse the orphaned_commit_warning() helper so that it matches the
> situation as the end-user experiences.
> 
> >  	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
> > -		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
> > +		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);
> 
> The magic number "1" looks iffy.
> 
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index a61bc32189..df269bccc8 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -1138,6 +1138,14 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> >  
> >  		ret |= delete_git_work_tree(wt);
> >  	}
> > +
> > +	if (!wt->head_ref && !is_null_oid(&wt->head_oid)) {
> > +		struct commit* wt_commit = lookup_commit_reference_gently(the_repository,
> 
> Asterisk sticks to the variable, not to type, in C.  If you write
> 
> 	struct commit *pointer, structure;
> 
> it is clear only one is the pointer.  It misleads people if you wrote
> 
> 	struct commit* one, two;
> 
> instead.

OK, sorry.

> 
> > +									  &wt->head_oid, 1);
> 
> Also, lines around here look overly long.  Would it help to fold the
> line after the initialization assignment, i.e.
> 
> 		struct commit *wt_commit =
> 			lookup_commit_reference_gently(the_repository, ...);

OK.

> 
> 
> > +		if (wt_commit)
> > +			orphaned_commit_warning(wt_commit, NULL, 0);
> 
> Again, the magic number "0" looks iffy.
> 
> > diff --git a/checkout.c b/checkout.c
> > index 18e7362043..5f7b0b3c49 100644
> > --- a/checkout.c
> > +++ b/checkout.c
> > @@ -171,7 +171,8 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
> >   * HEAD.  If it is not reachable from any ref, this is the last chance
> >   * for the user to do so without resorting to reflog.
> >   */
> > -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit)
> > +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
> > +			     int show_previous_position)
> >  {
> >  	struct rev_info revs;
> >  	struct object *object = &old_commit->object;
> > @@ -192,8 +193,10 @@ void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commi
> >  		die(_("internal error in revision walk"));
> >  	if (!(old_commit->object.flags & UNINTERESTING))
> >  		suggest_reattach(old_commit, &revs);
> > -	else
> > +	else if (show_previous_position)
> >  		describe_detached_head(_("Previous HEAD position was"), old_commit);
> > +	else
> > +		describe_detached_head(_("HEAD position was"), old_commit);
> 
> Can we think of a single way to phrase this batter?  It's not like

OK.

This is the current situation:

   $ git checkout --detach 
   HEAD is now at 2efe05c commit-a

   $ git checkout HEAD~1
   Previous HEAD position was 2efe05c commit-a
   HEAD is now at 7906992 commit-b

   $ git worktree add test --detach && git worktree remove test
   Preparing worktree (detached HEAD 7906992)
   HEAD is now at 7906992 commit-b

Maybe "HEAD position was" fits for both usages.  This is how it would
look like:

   $ git checkout -
   HEAD position was 7906992 commit-b
   HEAD is now at 2efe05c commit-a

   $ git worktree add test --detach && git worktree remove test
   Preparing worktree (detached HEAD 2efe05c)
   HEAD is now at 2efe05c commit-a
   HEAD position was 2efe05c commit-a

Or just "HEAD was at":

   $ git checkout -
   HEAD was at 2efe05c commit-a
   HEAD is now at 7906992 commit-b

   $ git worktree add test --detach && git worktree remove test
   Preparing worktree (detached HEAD 7906992)
   HEAD is now at 7906992 commit-b
   HEAD was at 7906992 commit-b

I think, if there are no objections or better suggestions, I'll re-roll
with "HEAD was at". 

> 
> > diff --git a/checkout.h b/checkout.h
> > index c7dc056544..ee400376d5 100644
> > --- a/checkout.h
> > +++ b/checkout.h
> > @@ -18,7 +18,8 @@ const char *unique_tracking_name(const char *name,
> >   * HEAD.  If it is not reachable from any ref, this is the last chance
> >   * for the user to do so without resorting to reflog.
> >   */
> > -void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit);
> > +void orphaned_commit_warning(struct commit *old_commit, struct commit *new_commit,
> > +			     int show_previous_position);
> >  
> >  void describe_detached_head(const char *msg, struct commit *commit);
> >  #endif /* CHECKOUT_H */
> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> > index 230a55e99a..f2756f7137 100755
> > --- a/t/t2403-worktree-move.sh
> > +++ b/t/t2403-worktree-move.sh
> > @@ -247,4 +247,14 @@ test_expect_success 'not remove a repo with initialized submodule' '
> >  	)
> >  '
> >  
> > +test_expect_success 'warn when removing a worktree with orphan commits' '
> > +	git worktree add --detach foo &&
> > +	git -C foo commit -m one --allow-empty &&
> > +	git -C foo commit -m two --allow-empty &&
> > +	git worktree remove foo 2>err &&
> > +	test_i18ngrep "you are leaving 2 commits behind" err &&
> > +	test_i18ngrep ! "Previous HEAD position was" err
> > +	test_i18ngrep "HEAD position was" err
> > +'
> > +
> >  test_done

Thanks.

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Maybe "HEAD position was" fits for both usages.  This is how it would
> look like:
> ...
> I think, if there are no objections or better suggestions, I'll re-roll
> with "HEAD was at". 

But does it convey the more important point?  The reason why "HEAD
WAS at" may matter is because the user is about to lose history
leading to it.  I wonder if we want to be more direct and alarming,
e.g.

    $ git checkout -
    About to lose history leading to 2efe05c commit-a
    HEAD is now at 7906992 commit-b

Whichever phrasing you end up using, I think the order of messages
should be made consistent between the two cases.  That is,

> Maybe "HEAD position was" fits for both usages.  This is how it would
> look like:
>
>    $ git checkout -
>    HEAD position was 7906992 commit-b
>    HEAD is now at 2efe05c commit-a

Here "git checkout" reports the lost HEAD and then the end result.

>    $ git worktree add test --detach && git worktree remove test
>    Preparing worktree (detached HEAD 2efe05c)
>    HEAD is now at 2efe05c commit-a
>    HEAD position was 2efe05c commit-a

But here "git worktree add" reports the end resultfirst and then
reports the lost HEAD.  It probably should report them in reverse.

Thanks.

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Eric Sunshine]

On Thu, Apr 27, 2023 at 1:50 AM Junio C Hamano <gitster@pobox.com> wrote:
> Whichever phrasing you end up using, I think the order of messages
> should be made consistent between the two cases.  That is,
>
> > Maybe "HEAD position was" fits for both usages.  This is how it would
> > look like:
> >
> >    $ git checkout -
> >    HEAD position was 7906992 commit-b
> >    HEAD is now at 2efe05c commit-a
>
> Here "git checkout" reports the lost HEAD and then the end result.
>
> >    $ git worktree add test --detach && git worktree remove test
> >    Preparing worktree (detached HEAD 2efe05c)
> >    HEAD is now at 2efe05c commit-a
> >    HEAD position was 2efe05c commit-a
>
> But here "git worktree add" reports the end resultfirst and then
> reports the lost HEAD.  It probably should report them in reverse.

There may be a misunderstanding here due to the unfortunate
construction of Rubén's example which muddles together the output of
`git worktree add` and `git worktree remove`. For clarity, his example
should probably have been written:

  $ git worktree add test --detach
  Preparing worktree (detached HEAD 2efe05c)
  HEAD is now at 2efe05c commit-a
  $ git worktree remove test
  HEAD position was 2efe05c commit-a

although showing only the `git worktree remove` command would probably
have been even clearer.

Such example output does a good job of arguing in favor of your
suggestion to use phrasing which is more alarming:

  $ git checkout -
  Commit 2efe05c "commit-a" left dangling
  HEAD is now at 7906992 commit-b

  $ git worktree remove test
  Commit 2efe05c "commit-a" left dangling

(Hopefully someone can come up with better wording than "About to lose
history leading to" and "Commit ... left dangling", neither of which
sound quite right.)

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> There may be a misunderstanding here due to the unfortunate
> construction of Rubén's example which muddles together the output of
> `git worktree add` and `git worktree remove`. For clarity, his example
> should probably have been written:
>
>   $ git worktree add test --detach
>   Preparing worktree (detached HEAD 2efe05c)
>   HEAD is now at 2efe05c commit-a
>   $ git worktree remove test
>   HEAD position was 2efe05c commit-a
>
> although showing only the `git worktree remove` command would probably
> have been even clearer.

Ah, you are absolutely right.

My "huh?" against the apparent inconsistency between "checkout" and
"worktree" regarding the order of "this is the end result" vs "this
is what we left behind" does not exist, as "worktree remove" does
not involve being newly on a detached HEAD and it is the one that
may introduce a newly abandoned line of history.  So everything
makes sense.

> Such example output does a good job of arguing in favor of your
> suggestion to use phrasing which is more alarming:
>
>   $ git checkout -
>   Commit 2efe05c "commit-a" left dangling
>   HEAD is now at 7906992 commit-b
>
>   $ git worktree remove test
>   Commit 2efe05c "commit-a" left dangling
>
> (Hopefully someone can come up with better wording than "About to lose
> history leading to" and "Commit ... left dangling", neither of which
> sound quite right.)

Yup, I am obviously worse at phrasing this than you are ;-) We'd
need a good wording that is alarming, even for those who squelch
most of the warning given via the advise system, without becoming
too verbose.

Thanks.

Re: [PATCH 2/3] worktree: warn when removing a worktree with orphan commits

[Rubén Justo]

On 26-abr-2023 22:46:01, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:

The message: "Previous HEAD position was", which we have since dc9195ac78
(Let git-checkout always drop any detached head, 2007-02-03), describes
a detached HEAD that has been left behind.

In 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave
commits behind, 2011-02-18) we moved this message to a new function,
orphaned_commit_warning().  We still show the message if the HEAD left
behind is detached.  However, if the HEAD left behind is detached _and_
_not connected_ to any ref, instead of the original message, we show a
warning.

In this series, we want to use that function to show the same warning
when the user removes a worktree whose HEAD is detached and _not
connected_ to any ref.  However, if the HEAD is detached but connected,
the original message introduced in dc9195ac78 needs to be adjusted.

> > Maybe "HEAD position was" fits for both usages.  This is how it would
> > look like:
> > ...
> > I think, if there are no objections or better suggestions, I'll re-roll
> > with "HEAD was at". 

This is about the message introduced in dc9195ac78, but...

> But does it convey the more important point?  The reason why "HEAD

I think you are referring to the warning.

Starting from a situation like:

   $ git checkout -b foo
   Switched to a new branch 'foo'

   $ git checkout --detach
   HEAD is now at 47ab99a

   $ git commit --allow-empty -m dangling
   [detached HEAD 398a1b0] dangling

   $ git worktree add --detach foo-wt
   Preparing worktree (detached HEAD 398a1b0)
   HEAD is now at 398a1b0 dangling

If we switch to 'foo' in the current worktree, the message is:

   $ git checkout foo
   Warning: you are leaving 1 commit behind, not connected to
   any of your branches:
   
     398a1b0 dangling 
   
   If you want to keep it by creating a new branch, this may be a good time
   to do so with:
   
    git branch <new-branch-name> 398a1b0
   
   Switched to branch 'foo'

And -- this is what we are adding in this series -- the same message if
we remove the worktree 'foo-wt':

   $ git worktree remove foo-wt
   Warning: you are leaving 1 commit behind, not connected to
   any of your branches:
   
     398a1b0 dangling 
   
   If you want to keep it by creating a new branch, this may be a good time
   to do so with:
   
    git branch <new-branch-name> 398a1b0

> > Maybe "HEAD position was" fits for both usages.  This is how it would
> > look like:
> >
> >    $ git checkout -
> >    HEAD position was 7906992 commit-b
> >    HEAD is now at 2efe05c commit-a
> 
> Here "git checkout" reports the lost HEAD and then the end result.
> 
> >    $ git worktree add test --detach && git worktree remove test
> >    Preparing worktree (detached HEAD 2efe05c)
> >    HEAD is now at 2efe05c commit-a
> >    HEAD position was 2efe05c commit-a

I apologize, the examples were confusing.  I though it was a good idea
to show the new message next to other messages where we also refer to
the HEAD position.

[PATCH 3/3] checkout: warn when unreachable commits after using --orphan

[Rubén Justo]

In 8e2dc6ac06 (commit: give final warning when reattaching HEAD to leave
commits behind, 2011-02-18) we introduced a warning to be issued when,
while checking out, the tip commit being left behind is not connected to
any ref.

We assumed that if the commit to be checked out is the same commit
currently checked out, we would omit the warning.  This makes sense
because we're going to have HEAD pointing to the same commit anyway, so
there is nothing to warn about.

However, with "--orphan" the target commit is not going to be used as
HEAD in the worktree, but a new orphan branch being created, which is
not going to be connected to the previous commit.  Therefore, we need
to check if the commit it is reachable and warn otherwise.

Let's fix the condition we introduced in 8e2dc6ac06, considering the
"--orphan" flag situation.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/checkout.c         | 8 ++++++--
 t/t2020-checkout-detach.sh | 9 +++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 85ac4bca00..7fad3161b4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1050,8 +1050,12 @@ static int switch_branches(const struct checkout_opts *opts,
 		}
 	}
 
-	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
-		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);
+	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit) {
+		if (new_branch_info->commit != old_branch_info.commit)
+			orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit, 1);
+		else if (opts->new_orphan_branch)
+			orphaned_commit_warning(old_branch_info.commit, NULL, 1);
+	}
 
 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 2eab6474f8..6762a9a572 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '
 	check_orphan_warning stderr "2 commits"
 '
 
+test_expect_success 'checkout --orphan warns on orphan commits' '
+	git checkout "$orphan2" &&
+	git checkout --orphan orphan 2>stderr
+'
+
+test_expect_success 'checkout --orphan warns on orphan commits: output' '
+	check_orphan_warning stderr "2 commits"
+'
+
 test_expect_success 'checkout warns orphaning 1 of 2 commits' '
 	git checkout "$orphan2" &&
 	git checkout HEAD^ 2>stderr
-- 
2.39.2

Re: [PATCH 3/3] checkout: warn when unreachable commits after using --orphan

[Andrei Rybak]

On 23/04/2023 00:19, Rubén Justo wrote:
> diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
> index 2eab6474f8..6762a9a572 100755
> --- a/t/t2020-checkout-detach.sh
> +++ b/t/t2020-checkout-detach.sh
> @@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '
>   	check_orphan_warning stderr "2 commits"
>   '
>   
> +test_expect_success 'checkout --orphan warns on orphan commits' '
> +	git checkout "$orphan2" &&
> +	git checkout --orphan orphan 2>stderr
> +'
> +
> +test_expect_success 'checkout --orphan warns on orphan commits: output' '
> +	check_orphan_warning stderr "2 commits"
> +'

These two tests could be a single test.

	test_expect_success 'checkout --orphan warns on orphan commits' '
		git checkout "$orphan2" &&
		git checkout --orphan orphan 2>stderr &&
		check_orphan_warning stderr "2 commits"
	'

Validating output like this in a separate step is an artifact of
the old way of checking localized strings.  Tests were split into
two in f06f08b78c ("i18n: mark checkout plural warning for
translation", 2011-04-10) and then prerequisite C_LOCALE_OUTPUT
was removed in f2c8c8007c ("i18n: use test_i18ngrep in t2020,
t2204, t3030, and t3200", 2011-04-12).  Usage of test_i18ngrep
was then removed in 1108cea7f8 ("tests: remove most uses of
test_i18ncmp", 2021-02-11).

> +
>   test_expect_success 'checkout warns orphaning 1 of 2 commits' '
>   	git checkout "$orphan2" &&
>   	git checkout HEAD^ 2>stderr

Re: [PATCH 3/3] checkout: warn when unreachable commits after using --orphan

[Rubén Justo]

On 27/4/23 2:28, Andrei Rybak wrote:
> On 23/04/2023 00:19, Rubén Justo wrote:
>> diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
>> index 2eab6474f8..6762a9a572 100755
>> --- a/t/t2020-checkout-detach.sh
>> +++ b/t/t2020-checkout-detach.sh
>> @@ -124,6 +124,15 @@ test_expect_success 'checkout warns on orphan commits: output' '
>>       check_orphan_warning stderr "2 commits"
>>   '
>>   +test_expect_success 'checkout --orphan warns on orphan commits' '
>> +    git checkout "$orphan2" &&
>> +    git checkout --orphan orphan 2>stderr
>> +'
>> +
>> +test_expect_success 'checkout --orphan warns on orphan commits: output' '
>> +    check_orphan_warning stderr "2 commits"
>> +'
> 
> These two tests could be a single test.
> 
>     test_expect_success 'checkout --orphan warns on orphan commits' '
>         git checkout "$orphan2" &&
>         git checkout --orphan orphan 2>stderr &&
>         check_orphan_warning stderr "2 commits"
> 

OK
    '
> 
> Validating output like this in a separate step is an artifact of
> the old way of checking localized strings.  Tests were split into
> two in f06f08b78c ("i18n: mark checkout plural warning for
> translation", 2011-04-10) and then prerequisite C_LOCALE_OUTPUT
> was removed in f2c8c8007c ("i18n: use test_i18ngrep in t2020,
> t2204, t3030, and t3200", 2011-04-12).  Usage of test_i18ngrep
> was then removed in 1108cea7f8 ("tests: remove most uses of
> test_i18ncmp", 2021-02-11).

Thank you!