{"thread":{"id":"60196","subject":"[PATCH] advice: improve hint for diverging branches.","startedAt":"2023-09-04T20:59:28Z","lastAt":"2023-09-06T00:03:56Z","messageCount":4,"participants":["Konstantin Pereiaslov via GitGitGadget","Junio C Hamano","Konstantin Pereiaslov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481378","messageId":"pull.1570.git.git.1693861162353.gitgitgadget@gmail.com","threadId":"60196","inReplyTo":null,"subject":"[PATCH] advice: improve hint for diverging branches.","fromName":"Konstantin Pereiaslov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-04T20:59:22Z","receivedAt":"2023-09-04T20:59:28Z","isPatch":true,"sender":{"key":"perk11@perk11.info","avatar":"https://gravatar.com/avatar/77e4d66b1556cce3926adece19658ea7701cf44e37c71e49f709a440267115be?d=mp&s=160"},"body":"From: Konstantin Pereiaslov <perk11@perk11.info>\n\nAdded a description of what the offered options will do and for pull\nalso offered a 3rd option during a pull - a hard reset.\nThis option should be helpful for the new users that accidentally\ncommitted into the wrong branch which is a scenario I saw very\noften.\n\nThe resulting tooltip looks like this for pull:\n\nhint: Diverging branches can't be fast-forwarded.\nConsider the following options:\nhint:\nhint: To merge remote changes into your branch:\nhint:   git merge --no-ff\nhint:\nhint: To apply your changes on top of remote changes:\nhint:   git rebase\nhint:\nhint: To discard your local changes and apply the remote changes:\nhint:   git reset --hard refs/remotes/upstream/branch-name\nhint:\nhint: Disable this message with \"git config advice.diverging false\"\n\nThere is some danger because it's semi-destructive, but so are\nother options offered if user doesn't know the commands to\nrevert back. Additionally, I think \"To discard your local changes\"\nwording describes the danger clearly enough.\n\nAnd for merge I improved the wording and added a description of what\nthe commands do:\n\nhint: Diverging branches can't be fast-forwarded.\nhint: Consider the following options:\nhint:\nhint: To merge changes into your branch:\nhint:   git merge --no-ff\nhint:\nhint: To apply your changes on top:\nhint:   git rebase\nhint:\nhint: Disable this message with \"git config advice.diverging false\"\n\nSigned-off-by: Konstantin Pereiaslov <perk11@perk11.info>\n---\n    Improve hint for diverging branches.\n    \n    I have seen a lot of developers not know what to do when they try to do\n    a git pull on a master branch with the intention of updating that branch\n    to the latest version, but see an error about branches diverging because\n    they accidentally committed their changes to that branch. They then\n    spend their time resolving conflicts and still not getting the intended\n    result. The suggestion to do a hard reset should be something that helps\n    in this situation.\n    \n    I'm not sure if a new config option needs to be created as technically\n    these are two different advice now. I'm also not sure if \"refs/remotes\"\n    part of the refspec is necessary, that is what I found the functions in\n    pull.c are returning. I think \"upstream/branch-name\" should be the same\n    thing, but kept it as is ( git reset --hard\n    refs/remotes/upstream/branch-name) for now. Please feel free to chime\n    in.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1570%2Fperk11%2Fdiverging-advice-improvements-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1570/perk11/diverging-advice-improvements-v1\nPull-Request: https://github.com/git/git/pull/1570\n\n Documentation/config/advice.txt |  3 ++-\n advice.c                        | 27 +++++++++++++++++++++++----\n advice.h                        |  3 ++-\n builtin/merge.c                 |  2 +-\n builtin/pull.c                  | 20 ++++++++++++++++++--\n 5 files changed, 46 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c548a91e676..f3daa232ace 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -137,7 +137,8 @@ advice.*::\n \t\tis asked to update index entries outside the current sparse\n \t\tcheckout.\n \tdiverging::\n-\t\tAdvice shown when a fast-forward is not possible.\n+\t\tAdvice shown when a fast-forward is not possible during merge\n+\t\tor pull operation.\n \tworktreeAddOrphan::\n \t\tAdvice shown when a user tries to create a worktree from an\n \t\tinvalid reference, to instruct how to create a new orphan\ndiff --git a/advice.c b/advice.c\nindex 50c79443ba7..8fc4fb19932 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -220,19 +220,38 @@ void NORETURN die_conclude_merge(void)\n \tdie(_(\"Exiting because of unfinished merge.\"));\n }\n \n-void NORETURN die_ff_impossible(void)\n+void NORETURN die_ff_impossible_during_merge(void)\n {\n \tadvise_if_enabled(ADVICE_DIVERGING,\n-\t\t_(\"Diverging branches can't be fast-forwarded, you need to either:\\n\"\n+\t\t_(\"Diverging branches can't be fast-forwarded.\\n\"\n+\t\t\"Consider the following options:\\n\"\n \t\t\"\\n\"\n+\t\t\"To merge changes into your branch:\\n\"\n \t\t\"\\tgit merge --no-ff\\n\"\n \t\t\"\\n\"\n-\t\t\"or:\\n\"\n-\t\t\"\\n\"\n+\t\t\"To apply your changes on top:\\n\"\n \t\t\"\\tgit rebase\\n\"));\n \tdie(_(\"Not possible to fast-forward, aborting.\"));\n }\n \n+void NORETURN die_ff_impossible_during_pull(const char *upstream_branch_spec)\n+{\n+\tadvise_if_enabled(ADVICE_DIVERGING,\n+\t\t\t  _(\"Diverging branches can't be fast-forwarded. \"\n+\t\t\t    \"Consider the following options:\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"To merge remote changes into your branch:\\n\"\n+\t\t\t    \"\\tgit merge --no-ff\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"To apply your changes on top of remote changes:\\n\"\n+\t\t\t    \"\\tgit rebase\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"To discard your local changes and apply the remote changes:\\n\"\n+\t\t\t    \"\\tgit reset --hard %s\\n\"), upstream_branch_spec);\n+\tdie(_(\"Not possible to fast-forward, aborting.\"));\n+}\n+\n+\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list)\n {\n \tstruct string_list_item *item;\ndiff --git a/advice.h b/advice.h\nindex 2affbe14261..f87369a5471 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -71,7 +71,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);\n int error_resolve_conflict(const char *me);\n void NORETURN die_resolve_conflict(const char *me);\n void NORETURN die_conclude_merge(void);\n-void NORETURN die_ff_impossible(void);\n+void NORETURN die_ff_impossible_during_merge(void);\n+void NORETURN die_ff_impossible_during_pull(const char *upstream_branch_spec);\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list);\n void detach_advice(const char *new_name);\n void advise_on_moving_dirty_path(struct string_list *pathspec_list);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex de68910177f..8358f137f1d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1675,7 +1675,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (fast_forward == FF_ONLY)\n-\t\tdie_ff_impossible();\n+\t\tdie_ff_impossible_during_merge();\n \n \tif (autostash)\n \t\tcreate_autostash(the_repository,\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex be2b2c9ebc9..51d30e6f918 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -765,6 +765,20 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)\n \trefspec_item_clear(&spec);\n \treturn merge_branch;\n }\n+/**\n+ * Returns the branch the pull is performed from.\n+ * If remote is NULL or refspec is NULL, configured upstream remote of the\n+ * current branch is used.\n+ * If refspec is NULL, the current upstream branch is used.\n+ */\n+static const char *get_pull_branch(const char *remote, const char *refspec)\n+{\n+\tif (refspec == NULL || remote == NULL) {\n+\t\treturn get_upstream_branch(remote);\n+\t}\n+\n+\treturn get_tracking_branch(remote, refspec);\n+}\n \n /**\n  * Given the repo and refspecs, sets fork_point to the point at which the\n@@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t/* ff-only takes precedence over rebase */\n \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n-\t\tif (divergent)\n-\t\t\tdie_ff_impossible();\n+\t\tif (divergent) {\n+\t\t\tconst char* pull_branch_spec = get_pull_branch(repo, *refspecs);\n+\t\t\tdie_ff_impossible_during_pull(pull_branch_spec);\n+\t\t}\n \t\topt_rebase = REBASE_FALSE;\n \t}\n \t/* If no action specified and we can't fast forward, then warn. */\n\nbase-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf\n-- \ngitgitgadget\n"},{"id":"481420","messageId":"xmqqh6o8p5pl.fsf@gitster.g","threadId":"60196","inReplyTo":"pull.1570.git.git.1693861162353.gitgitgadget@gmail.com","subject":"Re: [PATCH] advice: improve hint for diverging branches.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-05T23:20:54Z","receivedAt":"2023-09-05T23:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Konstantin Pereiaslov via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Konstantin Pereiaslov <perk11@perk11.info>\n>\n> Added a description of what the offered options will do and for pull\n> also offered a 3rd option during a pull - a hard reset.\n> This option should be helpful for the new users that accidentally\n> committed into the wrong branch which is a scenario I saw very\n> often.\n\ncf. Documentation/SubmittingPatches:[[describe-changes]]\n\n> The resulting tooltip looks like this for pull:\n>\n> hint: Diverging branches can't be fast-forwarded.\n> Consider the following options:\n\nWe do not give \"hint:\" prefix to this line???\n\n> hint:\n> hint: To merge remote changes into your branch:\n> hint:   git merge --no-ff\n> hint:\n> hint: To apply your changes on top of remote changes:\n> hint:   git rebase\n\nHmph, \"apply\" -> \"replay\" perhaps?\n\n> hint: To discard your local changes and apply the remote changes:\n\nHere \"apply\" is definitely a misnomer.  Nothing is applied; you just\ndiscard your work and adopt (or \"accept\") the state of the remote as\na whole.\n\n> hint:   git reset --hard refs/remotes/upstream/branch-name\n> hint:\n> hint: Disable this message with \"git config advice.diverging false\"\n\nOK.\n\nOverall, \"... looks like this\" should be shown a bit indented so\nthat the example stands out from the text that explains the example.\n\n> There is some danger because it's semi-destructive, but so are\n> other options offered if user doesn't know the commands to\n> revert back.\n\nSorry, but I do not quite understand what you want to say here.\n\n> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>  \n>  \t/* ff-only takes precedence over rebase */\n>  \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n> -\t\tif (divergent)\n> -\t\t\tdie_ff_impossible();\n> +\t\tif (divergent) {\n> +\t\t\tconst char* pull_branch_spec = get_pull_branch(repo, *refspecs);\n\nIn this codebase, asterisk sticks to the variable/function\nidentifier, not types.\n\nBut more importantly, what guarantees your recomputation using\n'*refspecs' here will match the result of the logic that computed\n'divergent', which certainly would have already known what commit we\ntried to fast-forward our branch to, and where that commit came\nfrom?  We shouldn't be computing the same thing twice, and in\ndifferent ways; that is a sure way to introduce inconsistent\nresults.\n\n> +\t\t\tdie_ff_impossible_during_pull(pull_branch_spec);\n> +\t\t}\n>  \t\topt_rebase = REBASE_FALSE;\n>  \t}\n>  \t/* If no action specified and we can't fast forward, then warn. */\n>\n> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf\n\nThanks.\n"},{"id":"481422","messageId":"e8af1e31-793c-4623-bc97-13b80ac98774@perk11.info","threadId":"60196","inReplyTo":"xmqqh6o8p5pl.fsf@gitster.g","subject":"Re: [PATCH] advice: improve hint for diverging branches.","fromName":"Konstantin Pereiaslov","fromEmail":"perk11@perk11.info","sentAt":"2023-09-05T23:55:56Z","receivedAt":"2023-09-06T00:02:58Z","isPatch":true,"sender":{"key":"perk11@perk11.info","avatar":"https://gravatar.com/avatar/77e4d66b1556cce3926adece19658ea7701cf44e37c71e49f709a440267115be?d=mp&s=160"},"body":"Thank you, all good points. I will work on the wording\nimprovements based on your suggestions.\n\n\"Consider the following options:\" should be on a new line and\nget a \"hint:\" prefix, I will fix that.\n\n\nAs far as the danger to the user, I was talking about the fact that\nthe user doing \"git reset --hard\" is going to lose any uncommitted\nwork as well as any commits currently in the branch.\n\n > \"But more importantly, what guarantees your recomputation using\n > '*refspecs' here will match the result of the logic that computed\n > 'divergent', which certainly would have already known what commit we\n > tried to fast-forward our branch to, and where that commit came\n > from?  We shouldn't be computing the same thing twice, and in\n > different ways; that is a sure way to introduce inconsistent\n > results.\n\nThis makes sens, I did it this way because I wanted to get a remote and\nbranch name, not just hash id and it was difficult to get this information.\nI will try to rework it so that it shares the code with the logic that \ndetermines\nthe divergence status.\nAny tips on what's the best way to do that would be highly appreciated.\n\nOn 9/5/23 18:20, Junio C Hamano wrote:\n> \"Konstantin Pereiaslov via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n>> From: Konstantin Pereiaslov <perk11@perk11.info>\n>>\n>> Added a description of what the offered options will do and for pull\n>> also offered a 3rd option during a pull - a hard reset.\n>> This option should be helpful for the new users that accidentally\n>> committed into the wrong branch which is a scenario I saw very\n>> often.\n> cf. Documentation/SubmittingPatches:[[describe-changes]]\n>\n>> The resulting tooltip looks like this for pull:\n>>\n>> hint: Diverging branches can't be fast-forwarded.\n>> Consider the following options:\n> We do not give \"hint:\" prefix to this line???\n>\n>> hint:\n>> hint: To merge remote changes into your branch:\n>> hint:   git merge --no-ff\n>> hint:\n>> hint: To apply your changes on top of remote changes:\n>> hint:   git rebase\n> Hmph, \"apply\" -> \"replay\" perhaps?\n>\n>> hint: To discard your local changes and apply the remote changes:\n> Here \"apply\" is definitely a misnomer.  Nothing is applied; you just\n> discard your work and adopt (or \"accept\") the state of the remote as\n> a whole.\n>\n>> hint:   git reset --hard refs/remotes/upstream/branch-name\n>> hint:\n>> hint: Disable this message with \"git config advice.diverging false\"\n> OK.\n>\n> Overall, \"... looks like this\" should be shown a bit indented so\n> that the example stands out from the text that explains the example.\n>\n>> There is some danger because it's semi-destructive, but so are\n>> other options offered if user doesn't know the commands to\n>> revert back.\n> Sorry, but I do not quite understand what you want to say here.\n>\n>> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>>   \n>>   \t/* ff-only takes precedence over rebase */\n>>   \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n>> -\t\tif (divergent)\n>> -\t\t\tdie_ff_impossible();\n>> +\t\tif (divergent) {\n>> +\t\t\tconst char* pull_branch_spec = get_pull_branch(repo, *refspecs);\n> In this codebase, asterisk sticks to the variable/function\n> identifier, not types.\n>\n> But more importantly, what guarantees your recomputation using\n> '*refspecs' here will match the result of the logic that computed\n> 'divergent', which certainly would have already known what commit we\n> tried to fast-forward our branch to, and where that commit came\n> from?  We shouldn't be computing the same thing twice, and in\n> different ways; that is a sure way to introduce inconsistent\n> results.\n>\n>> +\t\t\tdie_ff_impossible_during_pull(pull_branch_spec);\n>> +\t\t}\n>>   \t\topt_rebase = REBASE_FALSE;\n>>   \t}\n>>   \t/* If no action specified and we can't fast forward, then warn. */\n>>\n>> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf\n> Thanks.\n"},{"id":"481423","messageId":"55aff183-c514-498d-89ce-93654d3cde97@perk11.info","threadId":"60196","inReplyTo":"xmqqh6o8p5pl.fsf@gitster.g","subject":"Re: [PATCH] advice: improve hint for diverging branches.","fromName":"Konstantin Pereiaslov","fromEmail":"perk11@perk11.info","sentAt":"2023-09-06T00:03:46Z","receivedAt":"2023-09-06T00:03:56Z","isPatch":true,"sender":{"key":"perk11@perk11.info","avatar":"https://gravatar.com/avatar/77e4d66b1556cce3926adece19658ea7701cf44e37c71e49f709a440267115be?d=mp&s=160"},"body":"Thank you, all good points. I will work on the wording\nimprovements based on your suggestions.\n\n\"Consider the following options:\" should be on a new line and\nget a \"hint:\" prefix, I will fix that.\n\n\nAs far as the danger to the user, I was talking about the fact that\nthe user doing \"git reset --hard\" is going to lose any uncommitted\nwork as well as any commits currently in the branch.\n\n > \"But more importantly, what guarantees your recomputation using\n > '*refspecs' here will match the result of the logic that computed\n > 'divergent', which certainly would have already known what commit we\n > tried to fast-forward our branch to, and where that commit came\n > from?  We shouldn't be computing the same thing twice, and in\n > different ways; that is a sure way to introduce inconsistent\n > results.\n\nThis makes sens, I did it this way because I wanted to get a remote and\nbranch name, not just hash id and it was difficult to get this information.\nI will try to rework it so that it shares the code with the logic that \ndetermines\nthe divergence status.\nAny tips on what's the best way to do that would be highly appreciated.\n\nOn 9/5/23 18:20, Junio C Hamano wrote:\n> \"Konstantin Pereiaslov via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n>> From: Konstantin Pereiaslov <perk11@perk11.info>\n>>\n>> Added a description of what the offered options will do and for pull\n>> also offered a 3rd option during a pull - a hard reset.\n>> This option should be helpful for the new users that accidentally\n>> committed into the wrong branch which is a scenario I saw very\n>> often.\n> cf. Documentation/SubmittingPatches:[[describe-changes]]\n>\n>> The resulting tooltip looks like this for pull:\n>>\n>> hint: Diverging branches can't be fast-forwarded.\n>> Consider the following options:\n> We do not give \"hint:\" prefix to this line???\n>\n>> hint:\n>> hint: To merge remote changes into your branch:\n>> hint:   git merge --no-ff\n>> hint:\n>> hint: To apply your changes on top of remote changes:\n>> hint:   git rebase\n> Hmph, \"apply\" -> \"replay\" perhaps?\n>\n>> hint: To discard your local changes and apply the remote changes:\n> Here \"apply\" is definitely a misnomer.  Nothing is applied; you just\n> discard your work and adopt (or \"accept\") the state of the remote as\n> a whole.\n>\n>> hint:   git reset --hard refs/remotes/upstream/branch-name\n>> hint:\n>> hint: Disable this message with \"git config advice.diverging false\"\n> OK.\n>\n> Overall, \"... looks like this\" should be shown a bit indented so\n> that the example stands out from the text that explains the example.\n>\n>> There is some danger because it's semi-destructive, but so are\n>> other options offered if user doesn't know the commands to\n>> revert back.\n> Sorry, but I do not quite understand what you want to say here.\n>\n>> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>>   \n>>   \t/* ff-only takes precedence over rebase */\n>>   \tif (opt_ff && !strcmp(opt_ff, \"--ff-only\")) {\n>> -\t\tif (divergent)\n>> -\t\t\tdie_ff_impossible();\n>> +\t\tif (divergent) {\n>> +\t\t\tconst char* pull_branch_spec = get_pull_branch(repo, *refspecs);\n> In this codebase, asterisk sticks to the variable/function\n> identifier, not types.\n>\n> But more importantly, what guarantees your recomputation using\n> '*refspecs' here will match the result of the logic that computed\n> 'divergent', which certainly would have already known what commit we\n> tried to fast-forward our branch to, and where that commit came\n> from?  We shouldn't be computing the same thing twice, and in\n> different ways; that is a sure way to introduce inconsistent\n> results.\n>\n>> +\t\t\tdie_ff_impossible_during_pull(pull_branch_spec);\n>> +\t\t}\n>>   \t\topt_rebase = REBASE_FALSE;\n>>   \t}\n>>   \t/* If no action specified and we can't fast forward, then warn. */\n>>\n>> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf\n> Thanks.\n>\n"}]}