| Message ID | 1897dff1-bb4d-9715-dd1c-86763c052589@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | warn when unreachable commits are left behind | expand |
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
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.
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.
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.)
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.
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.
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
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(-)