git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Alex Henrie" <alexhenrie24@gmail.com>,
	"Son Luong Ngoc" <sluongng@gmail.com>,
	"Matthias Baumgarten" <matthias.baumgarten@aixigo.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Elijah Newren" <newren@gmail.com>
Subject: [PATCH v3 0/8] Handle pull option precedence
Date: Thu, 22 Jul 2021 05:04:42 +0000	[thread overview]
Message-ID: <pull.1049.v3.git.git.1626930290.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1049.v2.git.git.1626831744.gitgitgadget@gmail.com>

Based on a recent list of rules for flag/option precedence for git-pull[1]
from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
here's an attempt to implement and document it. Given multiple recent
surprises from users about some of these behaviors[2][3] and a coworker just
yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
seems like a good time to address some of these issues.

Since the handling of conflicting options was holding up two of Alex's
patches[4][5], and his patches fix some of the tests, I also include those
two patches in my series, with a few small changes to the first (so I've
kept him as author) and more substantial changes to the second (so I've
given him an Initial-patch-by attribution).

Changes since v2:

 * Remove some unnecessary changes in patch 4, pointed out by Junio.

Changes since v1:

 * Rebased on latest master (resolved a simple conflict with
   dd/test-stdout-count-lines)
 * Patch 1: based on feedback from Junio, fixed some style issues, clarified
   function names, added a few new tests, and took a stab at fixing up the
   comments and test descriptions (but still unsure if I hit the mark on the
   last point)
 * Patch 2: changed the test expectations for one of the multiple head tests
   as per Junio's suggestion, and made one of the other tests expect a more
   specific error message
 * Patches 4 & 5 were squashed and fixed: these now address a submodule bug
   interaction with --ff-only
 * Old patch 6 (now 5): added a code comment explaining a subtle point
 * Old patch 8 (now 7): a few more documentation updates, especially making
   --ff-only not sound merge-specific
 * Old patch 9 (now 8): Updates for new test expectation from patch 2

Quick overview:

 * Patches 1-2: new testcases (see the commit messages for the rules)
 * Patch 3: Alex's recent patch (abort if --ff-only but can't do so)
 * Patches 4-6: fix the precedence parts Alex didn't cover
 * Patch 7: Alex's other patch, abort if rebase vs. merge not specified
 * Patch 8: Compatibility of git-pull with merge-options.txt (think
   rebasing)
 * Patch 9: Fix multiple heads handling too

[1] https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/ [2]
https://lore.kernel.org/git/CAL3xRKdOyVWvcLXK7zoXtFPiHBjgL24zi5hhg+3yjowwSUPgmg@mail.gmail.com/
[3]
https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/
[4]
https://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/
[5]
https://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/

Alex Henrie (1):
  pull: abort if --ff-only is given and fast-forwarding is impossible

Elijah Newren (7):
  t7601: test interaction of merge/rebase/fast-forward flags and options
  t7601: add tests of interactions with multiple merge heads and config
  pull: since --ff-only overrides, handle it first
  pull: make --rebase and --no-rebase override pull.ff=only
  pull: abort by default when fast-forwarding is not possible
  pull: update docs & code for option compatibility with rebasing
  pull: fix handling of multiple heads

 Documentation/git-merge.txt     |   2 +
 Documentation/git-pull.txt      |  21 +--
 Documentation/merge-options.txt |  40 ++++++
 advice.c                        |   5 +
 advice.h                        |   1 +
 builtin/merge.c                 |   2 +-
 builtin/pull.c                  |  55 +++++--
 t/t4013-diff-various.sh         |   2 +-
 t/t5520-pull.sh                 |  20 +--
 t/t5521-pull-options.sh         |   4 +-
 t/t5524-pull-msg.sh             |   4 +-
 t/t5553-set-upstream.sh         |  14 +-
 t/t5604-clone-reference.sh      |   4 +-
 t/t6402-merge-rename.sh         |  18 +--
 t/t6409-merge-subtree.sh        |   6 +-
 t/t6417-merge-ours-theirs.sh    |  10 +-
 t/t7601-merge-pull-config.sh    | 244 +++++++++++++++++++++++++++++---
 t/t7603-merge-reduce-heads.sh   |   2 +-
 18 files changed, 371 insertions(+), 83 deletions(-)


base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1049%2Fnewren%2Fhandle-pull-option-precedence-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1049/newren/handle-pull-option-precedence-v3
Pull-Request: https://github.com/git/git/pull/1049

Range-diff vs v2:

 1:  17560927211 = 1:  17560927211 t7601: test interaction of merge/rebase/fast-forward flags and options
 2:  66fe7f7f934 = 2:  66fe7f7f934 t7601: add tests of interactions with multiple merge heads and config
 3:  c45cd239666 = 3:  c45cd239666 pull: abort if --ff-only is given and fast-forwarding is impossible
 4:  1a821d3b1dd ! 4:  0682b2250f4 pull: since --ff-only overrides, handle it first
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       
       	if (opt_rebase) {
       		int ret = 0;
     -@@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     - 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
     - 			die(_("cannot rebase with locally recorded submodule modifications"));
     - 
     --		if (can_ff) {
     --			/* we can fast-forward this without invoking rebase */
     --			opt_ff = "--ff-only";
     --			ret = run_merge();
     --		} else {
     --			ret = run_rebase(&newbase, &upstream);
     --		}
     -+		ret = run_rebase(&newbase, &upstream);
     - 
     - 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
     - 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
     -
     - ## t/t5520-pull.sh ##
     -@@ t/t5520-pull.sh: test_expect_success '--rebase (merge) fast forward' '
     - 	# The above only validates the result.  Did we actually bypass rebase?
     - 	git reflog -1 >reflog.actual &&
     - 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
     --	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
     -+	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
     - 	test_cmp reflog.expected reflog.fuzzy
     - '
     - 
     -@@ t/t5520-pull.sh: test_expect_success '--rebase (am) fast forward' '
     - 
     - 	# The above only validates the result.  Did we actually bypass rebase?
     - 	git reflog -1 >reflog.actual &&
     --	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
     --	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
     -+	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
     -+	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
     - 	test_cmp reflog.expected reflog.fuzzy
     - '
     - 
 5:  9b116f3d284 = 5:  b242d001132 pull: make --rebase and --no-rebase override pull.ff=only
 6:  f061f8b4e75 = 6:  7447c719bd7 pull: abort by default when fast-forwarding is not possible
 7:  90d49e0fb78 = 7:  726082f2e79 pull: update docs & code for option compatibility with rebasing
 8:  f03b15b7eb0 = 8:  768dec71558 pull: fix handling of multiple heads

-- 
gitgitgadget

  parent reply	other threads:[~2021-07-22  5:05 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-17 15:41 [PATCH 0/9] " Elijah Newren via GitGitGadget
2021-07-17 15:41 ` [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options Elijah Newren via GitGitGadget
2021-07-17 18:03   ` Felipe Contreras
2021-07-19 18:23   ` Junio C Hamano
2021-07-20 17:10     ` Elijah Newren
2021-07-20 18:22       ` Junio C Hamano
2021-07-20 20:29     ` Felipe Contreras
2021-07-17 15:41 ` [PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config Elijah Newren via GitGitGadget
2021-07-17 18:04   ` Felipe Contreras
2021-07-20 23:11   ` Junio C Hamano
2021-07-21  0:45     ` Elijah Newren
2021-07-17 15:41 ` [PATCH 3/9] pull: abort if --ff-only is given and fast-forwarding is impossible Alex Henrie via GitGitGadget
2021-07-17 18:14   ` Felipe Contreras
2021-07-17 15:41 ` [PATCH 4/9] pull: since --ff-only overrides, handle it first Elijah Newren via GitGitGadget
2021-07-17 18:22   ` Felipe Contreras
2021-07-17 15:41 ` [PATCH 5/9] pull: ensure --rebase overrides ability to ff Elijah Newren via GitGitGadget
2021-07-17 18:25   ` Felipe Contreras
2021-07-20 23:35   ` Junio C Hamano
2021-07-21  0:14     ` Elijah Newren
2021-07-17 15:41 ` [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only Elijah Newren via GitGitGadget
2021-07-17 18:28   ` Felipe Contreras
2021-07-20 23:53   ` Junio C Hamano
2021-07-21  0:09     ` Felipe Contreras
2021-07-17 15:41 ` [PATCH 7/9] pull: abort by default when fast-forwarding is not possible Elijah Newren via GitGitGadget
2021-07-17 18:31   ` Felipe Contreras
2021-07-17 15:41 ` [PATCH 8/9] pull: update docs & code for option compatibility with rebasing Elijah Newren via GitGitGadget
2021-07-21  0:17   ` Junio C Hamano
2021-07-21  0:44     ` Elijah Newren
2021-07-21  1:25       ` Junio C Hamano
2021-07-17 15:41 ` [PATCH 9/9] pull: fix handling of multiple heads Elijah Newren via GitGitGadget
2021-07-17 18:39 ` [PATCH 0/9] Handle pull option precedence Felipe Contreras
2021-07-21  1:42 ` [PATCH v2 0/8] " Elijah Newren via GitGitGadget
2021-07-21  1:42   ` [PATCH v2 1/8] t7601: test interaction of merge/rebase/fast-forward flags and options Elijah Newren via GitGitGadget
2021-07-21 12:24     ` Felipe Contreras
2021-07-21  1:42   ` [PATCH v2 2/8] t7601: add tests of interactions with multiple merge heads and config Elijah Newren via GitGitGadget
2021-07-21  1:42   ` [PATCH v2 3/8] pull: abort if --ff-only is given and fast-forwarding is impossible Alex Henrie via GitGitGadget
2021-07-21 12:25     ` Felipe Contreras
2021-07-21  1:42   ` [PATCH v2 4/8] pull: since --ff-only overrides, handle it first Elijah Newren via GitGitGadget
2021-07-21 19:18     ` Matthias Baumgarten
2021-07-21 21:18       ` Felipe Contreras
2021-07-21 20:18     ` Junio C Hamano
2021-07-22  3:42       ` Elijah Newren
2021-07-21  1:42   ` [PATCH v2 5/8] pull: make --rebase and --no-rebase override pull.ff=only Elijah Newren via GitGitGadget
2021-07-21 12:26     ` Felipe Contreras
2021-07-21  1:42   ` [PATCH v2 6/8] pull: abort by default when fast-forwarding is not possible Elijah Newren via GitGitGadget
2021-07-21 12:27     ` Felipe Contreras
2021-07-21  1:42   ` [PATCH v2 7/8] pull: update docs & code for option compatibility with rebasing Elijah Newren via GitGitGadget
2021-07-21  1:42   ` [PATCH v2 8/8] pull: fix handling of multiple heads Elijah Newren via GitGitGadget
2021-07-21 12:15   ` [PATCH v2 0/8] Handle pull option precedence Felipe Contreras
2021-07-22  5:04   ` Elijah Newren via GitGitGadget [this message]
2021-07-22  5:04     ` [PATCH v3 1/8] t7601: test interaction of merge/rebase/fast-forward flags and options Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 2/8] t7601: add tests of interactions with multiple merge heads and config Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 3/8] pull: abort if --ff-only is given and fast-forwarding is impossible Alex Henrie via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 4/8] pull: since --ff-only overrides, handle it first Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 5/8] pull: make --rebase and --no-rebase override pull.ff=only Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 6/8] pull: abort by default when fast-forwarding is not possible Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 7/8] pull: update docs & code for option compatibility with rebasing Elijah Newren via GitGitGadget
2021-07-22  5:04     ` [PATCH v3 8/8] pull: fix handling of multiple heads Elijah Newren via GitGitGadget
2021-07-22  7:09     ` [PATCH v3 0/8] Handle pull option precedence Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1049.v3.git.git.1626930290.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=alexhenrie24@gmail.com \
    --cc=avarab@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=matthias.baumgarten@aixigo.com \
    --cc=newren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v3 0/8] Handle pull option precedence' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).