| Message ID | 20230706040111.81110-3-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | advise about force-pushing as an alternative to reconciliation | expand |
Hi Alex On 06/07/2023 05:01, Alex Henrie wrote: > Also, don't put `git pull` in an awkward parenthetical, because > `git pull` can always be used to reconcile branches and is the normal > way to do so. This message would also benefit from adding explanation as to why this change is desirable. > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > builtin/push.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/builtin/push.c b/builtin/push.c > index 6f8a8dc711..b2f0a64e7c 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -301,21 +301,24 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote) > > static const char message_advice_pull_before_push[] = > N_("Updates were rejected because the tip of your current branch is behind\n" > - "its remote counterpart. Integrate the remote changes (e.g.\n" > - "'git pull ...') before pushing again.\n" > + "its remote counterpart. Use 'git pull' to integrate the remote changes\n" This is much clearer than "(e.g. 'git pull ...')" > + "before pushing again, or use 'git push --force' to delete the remote\n" > + "changes and replace them with your own.\n" I think it would be good to give a bit more context here as to when force pushing is a good idea. For example something like If you have rebased the branch since you last integrated remote changes then you can use 'git push --force-with-lease=<branch-ref> --force-if-includes' to safely replace the remote branch. If you have deleted and then recreated the branch since you last integrated remote changes then you can use 'git push +<branch>' to replace the remote. Note that if anyone else has pushed work to this branch it will be deleted. It makes the advice longer but the user get a specific suggestion for their current situation rather than a generic suggestion to delete the remote changes without discussing the implications. In this case we know that it was the current branch that was rejected and so should fill in the branch name in the advice as well. My main issue with the changes in this series is that they seem to assume the user is (a) pushing a single branch and (b) they are the only person who works on that branch. That is a common but narrow case where force pushing is perfectly sensible but there are many other scenarios where suggesting "push --force" would not be a good idea. Best Wishes Phillip > "See the 'Note about fast-forwards' in 'git push --help' for details."); > > static const char message_advice_checkout_pull_push[] = > N_("Updates were rejected because a pushed branch tip is behind its remote\n" > - "counterpart. Check out this branch and integrate the remote changes\n" > - "(e.g. 'git pull ...') before pushing again.\n" > + "counterpart. Check out this branch and use 'git pull' to integrate the\n" > + "remote changes before pushing again, or use 'git push --force' to delete\n" > + "the remote changes and replace them with your own.\n" > "See the 'Note about fast-forwards' in 'git push --help' for details."); > > static const char message_advice_ref_fetch_first[] = > - N_("Updates were rejected because the remote contains work that you do\n" > - "not have locally. This is usually caused by another repository pushing\n" > - "to the same ref. You may want to first integrate the remote changes\n" > - "(e.g., 'git pull ...') before pushing again.\n" > + N_("Updates were rejected because the remote contains work that you do not\n" > + "have locally. This is usually caused by another repository pushing to\n" > + "the same ref. Use 'git pull' to integrate the remote changes before\n" > + "pushing again, or use 'git push --force' to delete the remote changes\n" > + "and replace them with your own.\n" > "See the 'Note about fast-forwards' in 'git push --help' for details."); > > static const char message_advice_ref_already_exists[] = > @@ -327,10 +330,10 @@ static const char message_advice_ref_needs_force[] = > "without using the '--force' option.\n"); > > static const char message_advice_ref_needs_update[] = > - N_("Updates were rejected because the tip of the remote-tracking\n" > - "branch has been updated since the last checkout. You may want\n" > - "to integrate those changes locally (e.g., 'git pull ...')\n" > - "before forcing an update.\n"); > + N_("Updates were rejected because the tip of the remote-tracking branch has\n" > + "been updated since the last checkout. Use 'git pull' to integrate the\n" > + "remote changes before pushing again, or use 'git push --force' to delete\n" > + "the remote changes and replace them with your own.\n"); > > static void advise_pull_before_push(void) > {
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Alex > > On 06/07/2023 05:01, Alex Henrie wrote: >> Also, don't put `git pull` in an awkward parenthetical, because >> `git pull` can always be used to reconcile branches and is the normal >> way to do so. > > This message would also benefit from adding explanation as to why this > change is desirable. Yes, at least some essence from the lengthy discussion we had in the review threads for the expected use cases deserve to be summarized to help future developers who run "git log" (and "git blame") to find this commit. Unlike the [1/2] step, where the commands like "status" and "checkout" that are detached far away from the actual "push" are affected, this is exactly about "push has failed, now what" situation, where a change from "you must reconcile" to "if you want to reconcile, you could do this, but it may be that discarding the work on the other side is the right thing, if that is just a stale copy of what you are pushing" is very much welcome. > It makes the advice longer but the user get a specific suggestion for > their current situation rather than a generic suggestion to delete the > remote changes without discussing the implications. In this case we > know that it was the current branch that was rejected and so should > fill in the branch name in the advice as well. > > My main issue with the changes in this series is that they seem to > assume the user is (a) pushing a single branch and (b) they are the > only person who works on that branch. That is a common but narrow case > where force pushing is perfectly sensible but there are many other > scenarios where suggesting "push --force" would not be a good idea. Yup. Thanks for a review.
On Fri, Jul 7, 2023 at 2:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > This message would also benefit from adding explanation as to why this > change is desirable. OK, in v5 I'll add more explanation to the commit messages, including points brought up in this discussion. > > static const char message_advice_pull_before_push[] = > > N_("Updates were rejected because the tip of your current branch is behind\n" > > - "its remote counterpart. Integrate the remote changes (e.g.\n" > > - "'git pull ...') before pushing again.\n" > > + "its remote counterpart. Use 'git pull' to integrate the remote changes\n" > > This is much clearer than "(e.g. 'git pull ...')" > > > + "before pushing again, or use 'git push --force' to delete the remote\n" > > + "changes and replace them with your own.\n" > > I think it would be good to give a bit more context here as to when > force pushing is a good idea. For example something like > > If you have rebased the branch since you last integrated remote > changes then you can use > 'git push --force-with-lease=<branch-ref> --force-if-includes' to > safely replace the remote branch. > > If you have deleted and then recreated the branch since you last > integrated remote changes then you can use 'git push +<branch>' to > replace the remote. Note that if anyone else has pushed work to > this branch it will be deleted. > > It makes the advice longer but the user get a specific suggestion for > their current situation rather than a generic suggestion to delete the > remote changes without discussing the implications. In this case we know > that it was the current branch that was rejected and so should fill in > the branch name in the advice as well. Even if we could fill in <branch-ref> automatically, it's too much to ask the user to type out --force-with-lease=<branch-ref> --force-if-includes. Mentioning `git push --force` with a fat warning about how it only makes sense in a narrow (but common) case would be enough to make users aware of it while deterring them from abusing it. The advice already refers the user to the man page for more information, which includes a discussion of --force-with-lease and --force-if-includes as alternatives to plain --force. > My main issue with the changes in this series is that they seem to > assume the user is (a) pushing a single branch and (b) they are the only > person who works on that branch. That is a common but narrow case where > force pushing is perfectly sensible but there are many other scenarios > where suggesting "push --force" would not be a good idea. The goal of the series is not to assume that the user's situation is that narrow but common case, but rather to not assume that the user's situation is not that case. The most important thing is to make the user aware that integration/reconciliation is not the only possible way forward. Thanks for the feedback, -Alex
Hi Alex On 08/07/2023 19:56, Alex Henrie wrote: > On Fri, Jul 7, 2023 at 2:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >>> + "before pushing again, or use 'git push --force' to delete the remote\n" >>> + "changes and replace them with your own.\n" >> >> I think it would be good to give a bit more context here as to when >> force pushing is a good idea. For example something like >> >> If you have rebased the branch since you last integrated remote >> changes then you can use >> 'git push --force-with-lease=<branch-ref> --force-if-includes' to >> safely replace the remote branch. >> >> If you have deleted and then recreated the branch since you last >> integrated remote changes then you can use 'git push +<branch>' to >> replace the remote. Note that if anyone else has pushed work to >> this branch it will be deleted. >> >> It makes the advice longer but the user get a specific suggestion for >> their current situation rather than a generic suggestion to delete the >> remote changes without discussing the implications. In this case we know >> that it was the current branch that was rejected and so should fill in >> the branch name in the advice as well. > > Even if we could fill in <branch-ref> automatically, it's too much to > ask the user to type out --force-with-lease=<branch-ref> > --force-if-includes. Can't they just copy and paste the command from the advice message? Even if the user does not copy and paste it is not that hard to type it out with the benefit of the shell's tab completion. You're basically saying this combination of options is unusable in practice because it is too much effort to type them. We could look to see if we can make it less unwieldy by changing push to allow --force-if-includes=ref imply --force-with-lease for instance. > Mentioning `git push --force` with a fat warning > about how it only makes sense in a narrow (but common) case would be > enough to make users aware of it while deterring them from abusing it. Having a warning in the advice message would definitely help > The advice already refers the user to the man page for more > information, which includes a discussion of --force-with-lease and > --force-if-includes as alternatives to plain --force. It is good to mention the man page in the advice but we shouldn't assume that users will actually go and read it before running the suggested command. >> My main issue with the changes in this series is that they seem to >> assume the user is (a) pushing a single branch and (b) they are the only >> person who works on that branch. That is a common but narrow case where >> force pushing is perfectly sensible but there are many other scenarios >> where suggesting "push --force" would not be a good idea. > > The goal of the series is not to assume that the user's situation is > that narrow but common case, but rather to not assume that the user's > situation is not that case. The most important thing is to make the > user aware that integration/reconciliation is not the only possible > way forward. Thanks for clarifying, that is the sort of thing that should be in the commit message. Best Wishes Phillip > Thanks for the feedback, > > -Alex
On Tue, Jul 11, 2023 at 12:33 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 08/07/2023 19:56, Alex Henrie wrote: > > On Fri, Jul 7, 2023 at 2:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >>> + "before pushing again, or use 'git push --force' to delete the remote\n" > >>> + "changes and replace them with your own.\n" > >> > >> I think it would be good to give a bit more context here as to when > >> force pushing is a good idea. For example something like > >> > >> If you have rebased the branch since you last integrated remote > >> changes then you can use > >> 'git push --force-with-lease=<branch-ref> --force-if-includes' to > >> safely replace the remote branch. > >> > >> If you have deleted and then recreated the branch since you last > >> integrated remote changes then you can use 'git push +<branch>' to > >> replace the remote. Note that if anyone else has pushed work to > >> this branch it will be deleted. > >> > >> It makes the advice longer but the user get a specific suggestion for > >> their current situation rather than a generic suggestion to delete the > >> remote changes without discussing the implications. In this case we know > >> that it was the current branch that was rejected and so should fill in > >> the branch name in the advice as well. > > > > Even if we could fill in <branch-ref> automatically, it's too much to > > ask the user to type out --force-with-lease=<branch-ref> > > --force-if-includes. > > Can't they just copy and paste the command from the advice message? Even > if the user does not copy and paste it is not that hard to type it out > with the benefit of the shell's tab completion. You're basically saying > this combination of options is unusable in practice because it is too > much effort to type them. We could look to see if we can make it less > unwieldy by changing push to allow --force-if-includes=ref imply > --force-with-lease for instance. Yes, `git push --force-with-lease=<branch-ref> --force-if-includes` is cryptic and unwieldy, and even asking users to copy and paste a command is a bit much. If that's what's presented as the alternative to integration via `git pull`, it could make users who want to overwrite the remote branch think that force-pushing isn't what they want because what they want is conceptually very simple, so they expect it to have a simple user interface. It's possible that improvements will be made to this user interface in the future, but that's definitely not something that I'm going to tackle. I just want Git to give decent advice about what is available right now. If we can't agree on what specific command to recommend, maybe we can at least agree to tone down these messages to not sound so prescriptive. Just changing "Use 'git pull' to integrate..." to "You can use 'git pull' to integrate...' would be a big improvement. Thanks, -Alex
On Tue, Jul 11, 2023 at 10:47 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Tue, Jul 11, 2023 at 12:33 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > > On 08/07/2023 19:56, Alex Henrie wrote: > > > On Fri, Jul 7, 2023 at 2:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > >>> + "before pushing again, or use 'git push --force' to delete the remote\n" > > >>> + "changes and replace them with your own.\n" > > >> > > >> I think it would be good to give a bit more context here as to when > > >> force pushing is a good idea. For example something like > > >> > > >> If you have rebased the branch since you last integrated remote > > >> changes then you can use > > >> 'git push --force-with-lease=<branch-ref> --force-if-includes' to > > >> safely replace the remote branch. > > >> > > >> If you have deleted and then recreated the branch since you last > > >> integrated remote changes then you can use 'git push +<branch>' to > > >> replace the remote. Note that if anyone else has pushed work to > > >> this branch it will be deleted. > > >> > > >> It makes the advice longer but the user get a specific suggestion for > > >> their current situation rather than a generic suggestion to delete the > > >> remote changes without discussing the implications. In this case we know > > >> that it was the current branch that was rejected and so should fill in > > >> the branch name in the advice as well. > > > > > > Even if we could fill in <branch-ref> automatically, it's too much to > > > ask the user to type out --force-with-lease=<branch-ref> > > > --force-if-includes. > > > > Can't they just copy and paste the command from the advice message? Even > > if the user does not copy and paste it is not that hard to type it out > > with the benefit of the shell's tab completion. You're basically saying > > this combination of options is unusable in practice because it is too > > much effort to type them. We could look to see if we can make it less > > unwieldy by changing push to allow --force-if-includes=ref imply > > --force-with-lease for instance. > > Yes, `git push --force-with-lease=<branch-ref> --force-if-includes` is > cryptic and unwieldy, and even asking users to copy and paste a > command is a bit much. If that's what's presented as the alternative > to integration via `git pull`, it could make users who want to > overwrite the remote branch think that force-pushing isn't what they > want because what they want is conceptually very simple, so they > expect it to have a simple user interface. > > It's possible that improvements will be made to this user interface in > the future, but that's definitely not something that I'm going to > tackle. I just want Git to give decent advice about what is available > right now. If we can't agree on what specific command to recommend, > maybe we can at least agree to tone down these messages to not sound > so prescriptive. Just changing "Use 'git pull' to integrate..." to > "You can use 'git pull' to integrate...' would be a big improvement. Whoops, I accidentally quoted my own proposed text as if it were the current text. The current text is in fact "Integrate the remote changes..." which is stronger still. -Alex
diff --git a/builtin/push.c b/builtin/push.c index 6f8a8dc711..b2f0a64e7c 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -301,21 +301,24 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote) static const char message_advice_pull_before_push[] = N_("Updates were rejected because the tip of your current branch is behind\n" - "its remote counterpart. Integrate the remote changes (e.g.\n" - "'git pull ...') before pushing again.\n" + "its remote counterpart. Use 'git pull' to integrate the remote changes\n" + "before pushing again, or use 'git push --force' to delete the remote\n" + "changes and replace them with your own.\n" "See the 'Note about fast-forwards' in 'git push --help' for details."); static const char message_advice_checkout_pull_push[] = N_("Updates were rejected because a pushed branch tip is behind its remote\n" - "counterpart. Check out this branch and integrate the remote changes\n" - "(e.g. 'git pull ...') before pushing again.\n" + "counterpart. Check out this branch and use 'git pull' to integrate the\n" + "remote changes before pushing again, or use 'git push --force' to delete\n" + "the remote changes and replace them with your own.\n" "See the 'Note about fast-forwards' in 'git push --help' for details."); static const char message_advice_ref_fetch_first[] = - N_("Updates were rejected because the remote contains work that you do\n" - "not have locally. This is usually caused by another repository pushing\n" - "to the same ref. You may want to first integrate the remote changes\n" - "(e.g., 'git pull ...') before pushing again.\n" + N_("Updates were rejected because the remote contains work that you do not\n" + "have locally. This is usually caused by another repository pushing to\n" + "the same ref. Use 'git pull' to integrate the remote changes before\n" + "pushing again, or use 'git push --force' to delete the remote changes\n" + "and replace them with your own.\n" "See the 'Note about fast-forwards' in 'git push --help' for details."); static const char message_advice_ref_already_exists[] = @@ -327,10 +330,10 @@ static const char message_advice_ref_needs_force[] = "without using the '--force' option.\n"); static const char message_advice_ref_needs_update[] = - N_("Updates were rejected because the tip of the remote-tracking\n" - "branch has been updated since the last checkout. You may want\n" - "to integrate those changes locally (e.g., 'git pull ...')\n" - "before forcing an update.\n"); + N_("Updates were rejected because the tip of the remote-tracking branch has\n" + "been updated since the last checkout. Use 'git pull' to integrate the\n" + "remote changes before pushing again, or use 'git push --force' to delete\n" + "the remote changes and replace them with your own.\n"); static void advise_pull_before_push(void) {
Also, don't put `git pull` in an awkward parenthetical, because `git pull` can always be used to reconcile branches and is the normal way to do so. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- builtin/push.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)