Re: [PATCH v3 2/2] push: advise about force-pushing as an alternative to reconciliation

[Phillip Wood <phillip.wood123@gmail.com>]

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)
>   {