{"thread":{"id":"54827","subject":"[RFC/PATCH 0/3] pull: warning improvements part 2","startedAt":"2020-12-15T08:47:13Z","lastAt":"2020-12-15T08:47:47Z","messageCount":4,"participants":["Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"412246","messageId":"20201215084619.1631180-1-felipe.contreras@gmail.com","threadId":"54827","inReplyTo":null,"subject":"[RFC/PATCH 0/3] pull: warning improvements part 2","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T08:46:16Z","receivedAt":"2020-12-15T08:47:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This patch series goes on top of fc/pull-merge-rebase, however, I don't\nknow how I should communicate that clearly, so I'm sending this as RFC\nto avoid any possible merging issues. It's not really an RFC, and in\nreality this is not v1, but the end result of many iterations, simply\nrebased on top of Junio's changes.\n\nWhile updating the documentation (yet again) I noticed it didn't follow\nfrom one paragraph to the next (not a good story). So I decided to\nrewrite it adding a lot more detail.\n\nThe warning has also been changed completely taking into consideration\nthe imput Jeff King shared with us in 2013 from GitHub trainers.\n\nAnd finally one low-hanging fruit patch.\n\nI have many more in the queue though.\n\n\nFelipe Contreras (3):\n  doc: pull: explain what is a fast-forward\n  pull: improve default warning\n  pull: cleanup autostash check\n\n Documentation/git-pull.txt | 38 +++++++++++++++++++++++++++++++++-----\n builtin/pull.c             | 38 +++++++++++++++++++-------------------\n 2 files changed, 52 insertions(+), 24 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"412247","messageId":"20201215084619.1631180-2-felipe.contreras@gmail.com","threadId":"54827","inReplyTo":"20201215084619.1631180-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 1/3] doc: pull: explain what is a fast-forward","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T08:46:17Z","receivedAt":"2020-12-15T08:47:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We want users to know what is a fast-forward in order to understand the\ndefault warning.\n\nLet's expand the explanation in order to cover both the simple, and the\ncomplex cases with as much detail as possible.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/git-pull.txt | 38 +++++++++++++++++++++++++++++++++-----\n 1 file changed, 33 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 5c3fb67c01..e89d391b3b 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -41,16 +41,41 @@ Assume the following history exists and the current branch is\n ------------\n \t  A---B---C master on origin\n \t /\n-    D---E---F---G master\n+    D---E master\n \t^\n \torigin/master in your repository\n ------------\n \n Then \"`git pull`\" will fetch and replay the changes from the remote\n `master` branch since it diverged from the local `master` (i.e., `E`)\n-until its current commit (`C`) on top of `master` and record the\n-result in a new commit along with the names of the two parent commits\n-and a log message from the user describing the changes.\n+until its current commit (`C`) on top of `master`.\n+\n+After the remote changes have been synchronized, the local `master` will\n+be fast-forwarded to the same commit as the remote one, therefore\n+creating a linear history.\n+\n+------------\n+    D---E---A---B---C master, origin/master\n+------------\n+\n+However, a non-fast-forward case looks very different:\n+\n+------------\n+\t  A---B---C origin/master\n+\t /\n+    D---E---F---G master\n+------------\n+\n+If there are additional changes in the local `master`, it's\n+not possible to fast-forward, so a decision must be made how to\n+synchronize the local, and remote brances.\n+\n+In these situations `git pull` will warn you about your possible\n+options, which are either merge, or rebase. However, by default it will\n+continue doing a merge.\n+\n+A merge will create a new commit with two parent commits (`G` and `C`)\n+and a log message describing the changes, which you can edit.\n \n ------------\n \t  A---B---C origin/master\n@@ -58,8 +83,11 @@ and a log message from the user describing the changes.\n     D---E---F---G---H master\n ------------\n \n+Once the merge commit is created (`H`), your local `master` branch has\n+incorporated the changes of the remote `master` branch.\n+\n See linkgit:git-merge[1] for details, including how conflicts\n-are presented and handled.\n+are presented and handled, and also linkgit:git-rebase[1].\n \n In Git 1.7.0 or later, to cancel a conflicting merge, use\n `git reset --merge`.  *Warning*: In older versions of Git, running 'git pull'\n-- \n2.29.2\n\n"},{"id":"412248","messageId":"20201215084619.1631180-3-felipe.contreras@gmail.com","threadId":"54827","inReplyTo":"20201215084619.1631180-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 2/3] pull: improve default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T08:46:18Z","receivedAt":"2020-12-15T08:47:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"According to feedback from GitHub trainers [1], most newcomers don't\nunderstand what a rebase is. So in the default warning we want to\nprovide our users with a command that does the most sensible thing,\nfixes the divergence, gets rid of the warning, with the minimum mental\neffort, and happens to be the default:\n\n  git pull --no-rebase (later --merge)\n\nIn addition, we don't want to start by recommending a permanent\nconfiguration, but a temporary solution so they start training their\nfingers and maybe learn how to do a rebase. So we start with the commands.\n\nAlso, we need to be clear about what we mean by \"specifying\"; merge, or\nrebase.\n\nMoreover, thanks to the previous patch now \"git pull --help\" explains\nwhat a fast-forward is, let's mention that reference.\n\nAnd finally, use --global in the configuration commands like we did with\npush.default.\n\n[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/pull.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e8927fc2ff..a766d9762c 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -927,18 +927,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n \n static void show_advice_pull_non_ff(void)\n {\n-\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n-\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n-\t\t \"commands sometime before your next pull:\\n\"\n+\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is discouraged;\\n\"\n+\t\t \"you need to specify if you want a merge, or a rebase.\\n\"\n \t\t \"\\n\"\n-\t\t \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n-\t\t \"  git config pull.rebase true   # rebase\\n\"\n-\t\t \"  git config pull.ff only       # fast-forward only\\n\"\n+\t\t \"  git pull --no-rebase # the default (merge)\\n\"\n+\t\t \"  git pull --rebase\\n\"\n \t\t \"\\n\"\n-\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n-\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n-\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n-\t\t \"invocation.\\n\"));\n+\t\t \"You can squelch this message by running one of the following commands:\\n\"\n+\t\t \"\\n\"\n+\t\t \"  git config --global pull.rebase false  # merge\\n\"\n+\t\t \"  git config --global pull.rebase true   # rebase\\n\"\n+\t\t \"  git config --global pull.ff only       # fast-forward only\\n\"\n+\t\t \"\\n\"\n+\t\t \"If unsure, run \\\"git pull --no-rebase\\\".\\n\"\n+\t\t \"Read \\\"git pull --help\\\" for more information.\"));\n }\n \n int cmd_pull(int argc, const char **argv, const char *prefix)\n-- \n2.29.2\n\n"},{"id":"412249","messageId":"20201215084619.1631180-4-felipe.contreras@gmail.com","threadId":"54827","inReplyTo":"20201215084619.1631180-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 3/3] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T08:46:19Z","receivedAt":"2020-12-15T08:47:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently \"git pull --rebase\" takes a shortcut in the case a\nfast-forward merge is possible; run_merge() is called with --ff-only.\n\nHowever, \"git merge\" didn't have an --autostash option, so, when \"git\npull --rebase --autostash\" was called *and* the fast-forward merge\nshortcut was taken, then the pull failed.\n\nThis was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash\nworks in dirty repo, 2017-06-01) by simply skipping the fast-forward\nmerge shortcut.\n\nLater on \"git merge\" learned the --autostash option [a03b55530a\n(merge: teach --autostash option, 2020-04-07)], and so did \"git pull\"\n[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].\n\nTherefore it's not necessary to skip the fast-forward merge shortcut\nanymore when called with --rebase --autostash.\n\nLet's always take the fast-forward merge shortcut by essentially\nreverting f15e7cf5cc.\n\nReviewed-by: Elijah Newren <newren@gmail.com>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/pull.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex a766d9762c..42cd6c38d8 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -949,7 +949,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct oid_array merge_heads = OID_ARRAY_INIT;\n \tstruct object_id orig_head, curr_head;\n \tstruct object_id rebase_fork_point;\n-\tint autostash;\n \tint rebase_unspecified = 0;\n \tint can_ff;\n \n@@ -984,8 +983,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_oid(\"HEAD\", &orig_head))\n \t\toidclr(&orig_head);\n \n-\tautostash = config_autostash;\n \tif (opt_rebase) {\n+\t\tint autostash = config_autostash;\n \t\tif (opt_autostash != -1)\n \t\t\tautostash = opt_autostash;\n \n@@ -1067,13 +1066,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t    submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n-\t\tif (!autostash) {\n-\t\t\tif (can_ff) {\n-\t\t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\t\topt_ff = \"--ff-only\";\n-\t\t\t\tran_ff = 1;\n-\t\t\t\tret = run_merge();\n-\t\t\t}\n+\n+\t\tif (can_ff) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\tran_ff = 1;\n+\t\t\tret = run_merge();\n \t\t}\n \t\tif (!ran_ff)\n \t\t\tret = run_rebase(&newbase, &upstream);\n-- \n2.29.2\n\n"}]}