| Message ID | 20230706040111.81110-2-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | advise about force-pushing as an alternative to reconciliation | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > Also, don't imply that `git pull` is only for merging. > > Co-authored-by: Junio C Hamano <gitster@pobox.com> I appreciate, but do not need, the credit; in any way, I didn't co-author this one. > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > remote.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/remote.c b/remote.c > index a81f2e2f17..1fe86f8b23 100644 > --- a/remote.c > +++ b/remote.c > @@ -2323,7 +2323,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb, > base, ours, theirs); > if (advice_enabled(ADVICE_STATUS_HINTS)) > strbuf_addstr(sb, > - _(" (use \"git pull\" to merge the remote branch into yours)\n")); > + _(" (To reconcile your local changes with the work at the remote, you can\n" > + " use 'git pull' and then 'git push'. To discard the work at the remote\n" > + " and replace it with what you did (alone), you can use\n" > + " 'git push --force'.)\n")); > } Since wt-status.c:wt_longstatus_print_tracking() calls this function, I would expect that this change would manifest as test breakage in "git status" (or "git commit" whose commit log edit buffer is examined) tests. Are we lacking test coverage? Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> diff --git a/remote.c b/remote.c >> index a81f2e2f17..1fe86f8b23 100644 >> --- a/remote.c >> +++ b/remote.c >> @@ -2323,7 +2323,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb, >> base, ours, theirs); >> if (advice_enabled(ADVICE_STATUS_HINTS)) >> strbuf_addstr(sb, >> - _(" (use \"git pull\" to merge the remote branch into yours)\n")); >> + _(" (To reconcile your local changes with the work at the remote, you can\n" >> + " use 'git pull' and then 'git push'. To discard the work at the remote\n" >> + " and replace it with what you did (alone), you can use\n" >> + " 'git push --force'.)\n")); >> } > > Since wt-status.c:wt_longstatus_print_tracking() calls this > function, I would expect that this change would manifest as test > breakage in "git status" (or "git commit" whose commit log edit > buffer is examined) tests. Are we lacking test coverage? The other callsite of format_tracking_info() is "git checkout". When you start working on your own topic forked from upstream by switching to it, if Git notices that your topic's base has become behind (so that you would later need to merge or rebase to avoid losing others' work), the "git pull" message is given to tell you that it is OK if you want to catch up first before working on it. But the new message does not fit well in the workflow. It is primarily targetted for the users who are about to push out. They are at the point where they are way before being ready to "discard the work at the remote". I guess the updated message in the context of "git status" has exactly the same issue. The user is about to make a commit, which will later be pushed out. So, while I agree that new users may need to be made aware of situations where they should not afraid of overwriting the remote repository by forcing a non-ff push, I am not sure if this is a good advice message to convey it.
On Thu, Jul 6, 2023 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > >> diff --git a/remote.c b/remote.c > >> index a81f2e2f17..1fe86f8b23 100644 > >> --- a/remote.c > >> +++ b/remote.c > >> @@ -2323,7 +2323,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb, > >> base, ours, theirs); > >> if (advice_enabled(ADVICE_STATUS_HINTS)) > >> strbuf_addstr(sb, > >> - _(" (use \"git pull\" to merge the remote branch into yours)\n")); > >> + _(" (To reconcile your local changes with the work at the remote, you can\n" > >> + " use 'git pull' and then 'git push'. To discard the work at the remote\n" > >> + " and replace it with what you did (alone), you can use\n" > >> + " 'git push --force'.)\n")); > >> } > > > > Since wt-status.c:wt_longstatus_print_tracking() calls this > > function, I would expect that this change would manifest as test > > breakage in "git status" (or "git commit" whose commit log edit > > buffer is examined) tests. Are we lacking test coverage? Because I was only changing advice messages and not any functionality, I didn't think to run the tests. They are indeed failing, sorry. I will fix that in v4. > The other callsite of format_tracking_info() is "git checkout". > When you start working on your own topic forked from upstream by > switching to it, if Git notices that your topic's base has become > behind (so that you would later need to merge or rebase to avoid > losing others' work), the "git pull" message is given to tell you > that it is OK if you want to catch up first before working on it. > > But the new message does not fit well in the workflow. It is > primarily targetted for the users who are about to push out. They > are at the point where they are way before being ready to "discard > the work at the remote". If the branch is merely behind, format_tracking_info prints "(use "git pull" to update your local branch)", which is perfectly reasonable. The problem is only with the message that appears when the branches are divergent, "(use "git pull" to merge the remote branch into yours)", which is bad advice for the common GitHub/GitLab workflow that expects force-pushing. > I guess the updated message in the context of "git status" has > exactly the same issue. The user is about to make a commit, which > will later be pushed out. > > So, while I agree that new users may need to be made aware of > situations where they should not afraid of overwriting the remote > repository by forcing a non-ff push, I am not sure if this is a good > advice message to convey it. For more context, the coworker who most recently had this problem tried to pull because he looked at `git status` _after_ committing. Git can't assume that certain commands go with certain workflows (at least, not when it comes to divergent branches). Even if the user switches to a different branch and switches back, the first branch might be divergent simply because the user forgot to force-push before switching off of it. So, let's please give the user all of the information (two ways forward: reconcile or delete) and encourage them to make the most appropriate decision for their particular workflow. -Alex
Hi Alex On 06/07/2023 05:01, Alex Henrie wrote: > Also, don't imply that `git pull` is only for merging. While the cover letter gives some background for the reason behind this change the commit message does not explain why the proposed changes are desirable. > @@ -2323,7 +2323,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb, > base, ours, theirs); > if (advice_enabled(ADVICE_STATUS_HINTS)) > strbuf_addstr(sb, > - _(" (use \"git pull\" to merge the remote branch into yours)\n")); > + _(" (To reconcile your local changes with the work at the remote, you can\n" This is a welcome improvement but I think it would be better to say "integrate" rather than "reconcile" to keep the wording aligned with the advice is builtin/push.c. > + " use 'git pull' and then 'git push'. To discard the work at the remote\n" > + " and replace it with what you did (alone), you can use\n" > + " 'git push --force'.)\n")); I share Junio's concerns about giving this advice after "git status" or "git checkout" especially as we don't know if our remote tracking ref accurately reflects the current state of the remote branch. Best Wishes Phillip > } > free(base); > return 1;
Alex Henrie <alexhenrie24@gmail.com> writes: >> When you start working on your own topic forked from upstream by >> switching to it, if Git notices that your topic's base has become >> behind (so that you would later need to merge or rebase to avoid >> losing others' work), the "git pull" message is given to tell you >> that it is OK if you want to catch up first before working on it. >> >> But the new message does not fit well in the workflow. It is >> primarily targetted for the users who are about to push out. They >> are at the point where they are way before being ready to "discard >> the work at the remote". > > If the branch is merely behind, format_tracking_info prints "(use "git > pull" to update your local branch)", which is perfectly reasonable. Correct. The message you are changing is not the "your topic has become behind" case, and it is exactly why I said "your topic's base has become behind", i.e. your upstream has diverged. > The problem is only with the message that appears when the branches > are divergent, "(use "git pull" to merge the remote branch into > yours)", which is bad advice for the common GitHub/GitLab workflow > that expects force-pushing. We are in agreement in that "you must always reconcile" is not a good message in general to give, but I do not think "git checkout" and "git status" are good places to give the new advice "depending on your workflow, you do not necessarily have to pay attention to what the upstream has and just overwrite it may be good". That is about how to "push", but the user is a few steps before they are ready to start thinking about how to "push" when they get this message. These places in "checkout" and "status", where the message is given, were perfectly good places to say "by the way, you are divergent and even long before you are ready to push your work out, you may want to refresh your work to work better with the updated upstream", which was the "use git pull to reconcile" message was all about.
Alex Henrie <alexhenrie24@gmail.com> writes: > So, let's please give the user all of the > information (two ways forward: reconcile or delete) and encourage them > to make the most appropriate decision for their particular workflow. It may be OK to do so in "git status". It does not make any sense in "git checkout" to talk about "you can force push". That happens AFTER the work is done, and a message that tells them BEFORE they start the work and asking them to remember doing the right thing is an unnecessary noise. I would rather see us toning the message down, e.g. "Your branches have diverged. **IF** you intend to eventually reconcile the work on the remote with yours, you could use `git pull` to do so now" is all we should say. If they do not want to keep the work on the remote, at the point of seeing "you have diverged", there is nothing they need to do. There is no need to talk about "push --force" and force the user to remember that they have to do so later. When they try "git push", an appropriate message should be given anyway, but that is not the message you are touching in this patch. For that matter, it does not make ANY sense to give "you can pull to reconcile" message in the comment you are editing the log message while running "git commit". It would be the most inconvenient time to do so. So it might be necessary to first tweak the code so that different messages depending on the codepath are shown, perhaps by teaching format_tracking_info() who is calling. Thanks.
On Fri, Jul 7, 2023 at 11:52 AM Junio C Hamano <gitster@pobox.com> wrote: > I would rather see us toning the message down, e.g. "Your branches > have diverged. **IF** you intend to eventually reconcile the work on > the remote with yours, you could use `git pull` to do so now" is all > we should say. If they do not want to keep the work on the remote, > at the point of seeing "you have diverged", there is nothing they > need to do. There is no need to talk about "push --force" and force > the user to remember that they have to do so later. When they try > "git push", an appropriate message should be given anyway, but that > is not the message you are touching in this patch. I would be satisfied with toning down this message as you suggest; you're right that we don't necessarily have to mention force-pushing here. To keep the message short, we could just replace "(use "git pull" to merge the remote branch into yours)" with "(use "git pull" if you want to integrate the remote branch into yours)". > For that matter, it does not make ANY sense to give "you can pull to > reconcile" message in the comment you are editing the log message > while running "git commit". It would be the most inconvenient time > to do so. So it might be necessary to first tweak the code so that > different messages depending on the codepath are shown, perhaps by > teaching format_tracking_info() who is calling. I agree, showing this message in the middle of `git commit` is not ideal. However, that's a separate issue that can be fixed later; it's not part of the problem I'm trying to solve in this series. Thanks for the feedback, -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > I agree, showing this message in the middle of `git commit` is not > ideal. However, that's a separate issue that can be fixed later; it's > not part of the problem I'm trying to solve in this series. That is debatable. Even "by the way you can pull and reconcile early before you have fully finished working on the topic and are ready to push back" is irrelevant during `git commit`. "Reconciling the differences is not the only way to deal with divergence; you may decide to simply discard what they have with push --force" is even less relevant at that time. So it seems to be very much an integral part of the problem you are tackling, at least to me.
On Sat, Jul 8, 2023 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > I agree, showing this message in the middle of `git commit` is not > > ideal. However, that's a separate issue that can be fixed later; it's > > not part of the problem I'm trying to solve in this series. > > That is debatable. Even "by the way you can pull and reconcile > early before you have fully finished working on the topic and are > ready to push back" is irrelevant during `git commit`. "Reconciling > the differences is not the only way to deal with divergence; you may > decide to simply discard what they have with push --force" is even > less relevant at that time. So it seems to be very much an integral > part of the problem you are tackling, at least to me. I thought we just agreed that we don't need to mention force-pushing in this particular message? I guess you're saying that we'd still be over-encouraging `git pull` if we don't remove this message from `git commit` altogether? -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > On Sat, Jul 8, 2023 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Alex Henrie <alexhenrie24@gmail.com> writes: >> >> > I agree, showing this message in the middle of `git commit` is not >> > ideal. However, that's a separate issue that can be fixed later; it's >> > not part of the problem I'm trying to solve in this series. >> >> That is debatable. Even "by the way you can pull and reconcile >> early before you have fully finished working on the topic and are >> ready to push back" is irrelevant during `git commit`. "Reconciling >> the differences is not the only way to deal with divergence; you may >> decide to simply discard what they have with push --force" is even >> less relevant at that time. So it seems to be very much an integral >> part of the problem you are tackling, at least to me. > > I thought we just agreed that we don't need to mention force-pushing > in this particular message? I guess you're saying that we'd still be > over-encouraging `git pull` if we don't remove this message from `git > commit` altogether? I do not think so. I was saying that, when the user during `git commit` is wondering what to write in the log message of the commit they are working on (which may not yet make the current branch ready to be pushed to or integrated with the remote), the user is not ready to even choose between "forcing push to overwrite" and "integrate and then push". It can be fixed later, but it is a part of "how to avoid giving confusing message to users, especially the new ones" theme. After all, "do not make it sound like they always have to integrate" is how you started this journey, no? Thanks.
On Mon, Jul 10, 2023 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > On Sat, Jul 8, 2023 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Alex Henrie <alexhenrie24@gmail.com> writes: > >> > >> > I agree, showing this message in the middle of `git commit` is not > >> > ideal. However, that's a separate issue that can be fixed later; it's > >> > not part of the problem I'm trying to solve in this series. > >> > >> That is debatable. Even "by the way you can pull and reconcile > >> early before you have fully finished working on the topic and are > >> ready to push back" is irrelevant during `git commit`. "Reconciling > >> the differences is not the only way to deal with divergence; you may > >> decide to simply discard what they have with push --force" is even > >> less relevant at that time. So it seems to be very much an integral > >> part of the problem you are tackling, at least to me. > > > > I thought we just agreed that we don't need to mention force-pushing > > in this particular message? I guess you're saying that we'd still be > > over-encouraging `git pull` if we don't remove this message from `git > > commit` altogether? > > I do not think so. > > I was saying that, when the user during `git commit` is wondering > what to write in the log message of the commit they are working on > (which may not yet make the current branch ready to be pushed to or > integrated with the remote), the user is not ready to even choose > between "forcing push to overwrite" and "integrate and then push". > > It can be fixed later, but it is a part of "how to avoid giving > confusing message to users, especially the new ones" theme. After > all, "do not make it sound like they always have to integrate" is > how you started this journey, no? To me, one of those things is bad advice, and the other is irrelevant advice. They're both confusing, but one of them is more likely to cause trouble than the other. Omitting this message from `git commit` isn't technically difficult, my main worry is that that change will be picked to death in code review and it will hold up the more important changes. I have to find time for Git outside of work and I'm already feeling pretty burned out trying to communicate the problem and integrate the feedback on this series so far. Even so, because it's important to you, and because I appreciate your willingness to work with me on this problem, I'm willing to take a stab at fixing both the bad advice and the irrelevant advice in the same series. Just to be sure that we're on the same page, when I said "I thought we just agreed that we don't need to mention force-pushing..." and you replied "I do not think so", were you only saying that you think that changes to `git commit` are essential, or were you also saying that we have not come to an agreement about whether to include force-pushing advice in this message? Thanks, -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > Just to be sure that we're on the same page, when I said "I thought we > just agreed that we don't need to mention force-pushing..." and you > replied "I do not think so", were you only saying that you think that > changes to `git commit` are essential, or were you also saying that we > have not come to an agreement about whether to include force-pushing > advice in this message? None of the above ;-) With that "I do not think so", I meant that I do not agree with "I guess you're saying that we'd still be over-encouraging `git pull`" that was in your message. In the message you were responding to, I was saying that the time the user runs `git commit` is not a good time for the user to decide how to eventually update the remote target, and it does not matter which one we encourage more between "`git pull [--rebase]` then `git push`" and "`git push --force`". I am fine dropping patch [1/2]; we would not be touching output from "git status", "git commit", or "git checkout", and "we should not talk about 'git pull' (or how the eventual remote update should go, for that matter) when we notice that the base of the user's branch has become stale" becomes totally out of the scope of this topic. I think that we all are in agreement that [2/2] is the more important part of this topic, as it more directly improves the guidance for the end-users when their "push" triggers the non-ff check. Thanks.
On Wed, Jul 12, 2023 at 9:18 AM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > Just to be sure that we're on the same page, when I said "I thought we > > just agreed that we don't need to mention force-pushing..." and you > > replied "I do not think so", were you only saying that you think that > > changes to `git commit` are essential, or were you also saying that we > > have not come to an agreement about whether to include force-pushing > > advice in this message? > > None of the above ;-) > > With that "I do not think so", I meant that I do not agree with "I > guess you're saying that we'd still be over-encouraging `git pull`" > that was in your message. In the message you were responding to, I > was saying that the time the user runs `git commit` is not a good > time for the user to decide how to eventually update the remote > target, and it does not matter which one we encourage more between > "`git pull [--rebase]` then `git push`" and "`git push --force`". > > I am fine dropping patch [1/2]; we would not be touching output from > "git status", "git commit", or "git checkout", and "we should not > talk about 'git pull' (or how the eventual remote update should go, > for that matter) when we notice that the base of the user's branch > has become stale" becomes totally out of the scope of this topic. I > think that we all are in agreement that [2/2] is the more important > part of this topic, as it more directly improves the guidance for > the end-users when their "push" triggers the non-ff check. Thanks for the clarification. This all started because of the message in `git status`, so despite it being the less important message, I feel pretty strongly that that message does need to be toned down slightly. There's also the problem of that message assuming that `git pull` will do a merge when it can do either a merge or a rebase, depending on the user's Git config. I've already written a patch to suppress the irrelevant advice in `git commit`, so I might as well send it. I'm hoping that we can agree to make a few tweaks to these advice messages without going as far as I originally proposed. -Alex
diff --git a/remote.c b/remote.c index a81f2e2f17..1fe86f8b23 100644 --- a/remote.c +++ b/remote.c @@ -2323,7 +2323,10 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb, base, ours, theirs); if (advice_enabled(ADVICE_STATUS_HINTS)) strbuf_addstr(sb, - _(" (use \"git pull\" to merge the remote branch into yours)\n")); + _(" (To reconcile your local changes with the work at the remote, you can\n" + " use 'git pull' and then 'git push'. To discard the work at the remote\n" + " and replace it with what you did (alone), you can use\n" + " 'git push --force'.)\n")); } free(base); return 1;
Also, don't imply that `git pull` is only for merging. Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- remote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)