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

["Rubén Justo" <rjusto@gmail.com>]

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.