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

[Junio C Hamano <gitster@pobox.com>]

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