On 24-abr-2023 13:28:14, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
