threads / rfc / 54827

RFC patch, 3 partspull: warning improvements part 2

Subject: [RFC/PATCH 0/3] pull: warning improvements part 2

## tl;dr

4 messages between Dec 15, 2020 and Dec 15, 2020. Diffs are folded; open one to read it.

replies: 3people: 1as markdown or json

Felipe Contreras· Dec 15, 2020, 08:46 UTC · lore

This patch series goes on top of fc/pull-merge-rebase, however, I don't know how I should communicate that clearly, so I'm sending this as RFC to avoid any possible merging issues. It's not really an RFC, and in reality this is not v1, but the end result of many iterations, simply rebased on top of Junio's changes.

While updating the documentation (yet again) I noticed it didn't follow from one paragraph to the next (not a good story). So I decided to rewrite it adding a lot more detail.

The warning has also been changed completely taking into consideration the imput Jeff King shared with us in 2013 from GitHub trainers.

And finally one low-hanging fruit patch.
I have many more in the queue though.
Felipe Contreras (3):
  doc: pull: explain what is a fast-forward
  pull: improve default warning
  pull: cleanup autostash check
 Documentation/git-pull.txt | 38 +++++++++++++++++++++++++++++++++-----
 builtin/pull.c             | 38 +++++++++++++++++++-------------------
 2 files changed, 52 insertions(+), 24 deletions(-)
-- 
2.29.2
Felipe Contreras· Dec 15, 2020, 08:46 UTC · re: Felipe Contreras · lore

[RFC/PATCH 1/3] doc: pull: explain what is a fast-forward

We want users to know what is a fast-forward in order to understand the default warning.

Let's expand the explanation in order to cover both the simple, and the complex cases with as much detail as possible.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)
Show changes to Documentation/git-pull.txt +33 −5
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c01..e89d391b3b 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -41,16 +41,41 @@ Assume the following history exists and the current branch is
 ------------
 	  A---B---C master on origin
 	 /
-    D---E---F---G master
+    D---E master
 	^
 	origin/master in your repository
 ------------
 
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
-until its current commit (`C`) on top of `master` and record the
-result in a new commit along with the names of the two parent commits
-and a log message from the user describing the changes.
+until its current commit (`C`) on top of `master`.
+
+After the remote changes have been synchronized, the local `master` will
+be fast-forwarded to the same commit as the remote one, therefore
+creating a linear history.
+
+------------
+    D---E---A---B---C master, origin/master
+------------
+
+However, a non-fast-forward case looks very different:
+
+------------
+	  A---B---C origin/master
+	 /
+    D---E---F---G master
+------------
+
+If there are additional changes in the local `master`, it's
+not possible to fast-forward, so a decision must be made how to
+synchronize the local, and remote brances.
+
+In these situations `git pull` will warn you about your possible
+options, which are either merge, or rebase. However, by default it will
+continue doing a merge.
+
+A merge will create a new commit with two parent commits (`G` and `C`)
+and a log message describing the changes, which you can edit.
 
 ------------
 	  A---B---C origin/master
@@ -58,8 +83,11 @@ and a log message from the user describing the changes.
     D---E---F---G---H master
 ------------
 
+Once the merge commit is created (`H`), your local `master` branch has
+incorporated the changes of the remote `master` branch.
+
 See linkgit:git-merge[1] for details, including how conflicts
-are presented and handled.
+are presented and handled, and also linkgit:git-rebase[1].
 
 In Git 1.7.0 or later, to cancel a conflicting merge, use
 `git reset --merge`.  *Warning*: In older versions of Git, running 'git pull'
-- 
2.29.2
Felipe Contreras· Dec 15, 2020, 08:46 UTC · re: Felipe Contreras · lore

[RFC/PATCH 2/3] pull: improve default warning

According to feedback from GitHub trainers [1], most newcomers don't understand what a rebase is. So in the default warning we want to provide our users with a command that does the most sensible thing, fixes the divergence, gets rid of the warning, with the minimum mental effort, and happens to be the default:

  git pull --no-rebase (later --merge)

In addition, we don't want to start by recommending a permanent configuration, but a temporary solution so they start training their fingers and maybe learn how to do a rebase. So we start with the commands.

Also, we need to be clear about what we mean by "specifying"; merge, or rebase.

Moreover, thanks to the previous patch now "git pull --help" explains what a fast-forward is, let's mention that reference.

And finally, use --global in the configuration commands like we did with push.default.

[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
Show changes to builtin/pull.c +12 −10
diff --git a/builtin/pull.c b/builtin/pull.c
index e8927fc2ff..a766d9762c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,18 +927,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 
 static void show_advice_pull_non_ff(void)
 {
-	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
-		 "discouraged. You can squelch this message by running one of the following\n"
-		 "commands sometime before your next pull:\n"
+	advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
+		 "you need to specify if you want a merge, or a rebase.\n"
 		 "\n"
-		 "  git config pull.rebase false  # merge (the default strategy)\n"
-		 "  git config pull.rebase true   # rebase\n"
-		 "  git config pull.ff only       # fast-forward only\n"
+		 "  git pull --no-rebase # the default (merge)\n"
+		 "  git pull --rebase\n"
 		 "\n"
-		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
-		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-		 "or --ff-only on the command line to override the configured default per\n"
-		 "invocation.\n"));
+		 "You can squelch this message by running one of the following commands:\n"
+		 "\n"
+		 "  git config --global pull.rebase false  # merge\n"
+		 "  git config --global pull.rebase true   # rebase\n"
+		 "  git config --global pull.ff only       # fast-forward only\n"
+		 "\n"
+		 "If unsure, run \"git pull --no-rebase\".\n"
+		 "Read \"git pull --help\" for more information."));
 }
 
 int cmd_pull(int argc, const char **argv, const char *prefix)
-- 
2.29.2
Felipe Contreras· Dec 15, 2020, 08:46 UTC · re: Felipe Contreras · lore

[RFC/PATCH 3/3] pull: cleanup autostash check

Currently "git pull --rebase" takes a shortcut in the case a fast-forward merge is possible; run_merge() is called with --ff-only.

However, "git merge" didn't have an --autostash option, so, when "git pull --rebase --autostash" was called *and* the fast-forward merge shortcut was taken, then the pull failed.

This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash works in dirty repo, 2017-06-01) by simply skipping the fast-forward merge shortcut.

Later on "git merge" learned the --autostash option [a03b55530a (merge: teach --autostash option, 2020-04-07)], and so did "git pull" [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].

Therefore it's not necessary to skip the fast-forward merge shortcut anymore when called with --rebase --autostash.

Let's always take the fast-forward merge shortcut by essentially reverting f15e7cf5cc.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
Show changes to builtin/pull.c +7 −9
diff --git a/builtin/pull.c b/builtin/pull.c
index a766d9762c..42cd6c38d8 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -949,7 +949,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
-	int autostash;
 	int rebase_unspecified = 0;
 	int can_ff;
 
@@ -984,8 +983,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	autostash = config_autostash;
 	if (opt_rebase) {
+		int autostash = config_autostash;
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
 
@@ -1067,13 +1066,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
-		if (!autostash) {
-			if (can_ff) {
-				/* we can fast-forward this without invoking rebase */
-				opt_ff = "--ff-only";
-				ran_ff = 1;
-				ret = run_merge();
-			}
+
+		if (can_ff) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			ran_ff = 1;
+			ret = run_merge();
 		}
 		if (!ran_ff)
 			ret = run_rebase(&newbase, &upstream);
-- 
2.29.2

← back to recent threads