All of lore.kernel.org
 help / color / mirror / 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>,
	"Elijah Newren" <newren@gmail.com>
Subject: [PATCH v2 0/8] Handle pull option precedence
Date: Wed, 21 Jul 2021 01:42:16 +0000	[thread overview]
Message-ID: <pull.1049.v2.git.git.1626831744.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1049.git.git.1626536507.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 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                  |  63 ++++++---
 t/t4013-diff-various.sh         |   2 +-
 t/t5520-pull.sh                 |  26 ++--
 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, 375 insertions(+), 93 deletions(-)


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

Range-diff vs v1:

  1:  6cb771297f5 !  1:  17560927211 t7601: add relative precedence tests for merge and rebase flags/options
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    t7601: add relative precedence tests for merge and rebase flags/options
     +    t7601: test interaction of merge/rebase/fast-forward flags and options
      
          The interaction of rebase and merge flags and options was not well
          tested.  Add several tests to check for correct behavior from the
          following rules:
     -        * --ff-only takes precedence over --[no-]rebase
     -          * Corollary: pull.ff=only overrides pull.rebase
     -        * --rebase[=!false] takes precedence over --no-ff and --ff
     -          * Corollary: pull.rebase=!false overrides pull.ff=!only
     +        * --ff-only vs. --[no-]rebase
     +          (and the related pull.ff=only vs. pull.rebase)
     +        * --rebase[=!false] vs. --no-ff and --ff
     +          (and the related pull.rebase=!false overrides pull.ff=!only)
              * command line flags take precedence over config, except:
                * --no-rebase heeds pull.ff=!only
     -          * pull.rebase=!false takes precedence over --no-ff and --ff
     +          * pull.rebase=!false vs --no-ff and --ff
      
          For more details behind these rules and a larger table of individual
          cases, refer to https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
       	test_i18ngrep ! "Pulling without specifying how to reconcile" err
       '
       
     -+test_does_rebase() {
     ++test_does_rebase () {
      +	git reset --hard c2 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a rebase
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_does_merge_noff() {
     ++# Prefers merge over fast-forward
     ++test_does_merge_when_ff_possible () {
      +	git reset --hard c0 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a merge
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_does_merge_ff() {
     ++# Prefers fast-forward over merge or rebase
     ++test_does_fast_forward () {
      +	git reset --hard c0 &&
      +	git "$@" . c1 &&
     -+	# Check that we actually did a merge
     ++
     ++	# Check that we did not get any merges
      +	git rev-list --count HEAD >actual &&
      +	git rev-list --merges --count HEAD >>actual &&
      +	test_write_lines 2 0 >expect &&
      +	test_cmp expect actual &&
     ++
     ++	# Check that we ended up at c1
     ++	git rev-parse HEAD >actual &&
     ++	git rev-parse c1^{commit} >expect &&
     ++	test_cmp actual expect &&
     ++
     ++	# Remove temporary files
      +	rm actual expect
      +}
      +
     -+test_does_need_full_merge() {
     ++# Doesn't fail when fast-forward not possible; does a merge
     ++test_falls_back_to_full_merge () {
      +	git reset --hard c2 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a merge
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_attempts_fast_forward() {
     ++# Attempts fast forward, which is impossible, and bails
     ++test_attempts_fast_forward () {
      +	git reset --hard c2 &&
      +	test_must_fail git "$@" . c1 2>err &&
      +	test_i18ngrep "Not possible to fast-forward, aborting" err
      +}
      +
      +#
     -+# Rule 1: --ff-only takes precedence over --[no-]rebase
     -+# (Corollary: pull.ff=only overrides pull.rebase)
     ++# Group 1: Interaction of --ff-only with --[no-]rebase
     ++# (And related interaction of pull.ff=only with pull.rebase)
      +#
     -+test_expect_failure '--ff-only takes precedence over --rebase' '
     ++test_expect_failure '--ff-only overrides --rebase' '
      +	test_attempts_fast_forward pull --rebase --ff-only
      +'
      +
     -+test_expect_failure '--ff-only takes precedence over --rebase even if first' '
     ++test_expect_failure '--ff-only overrides --rebase even if first' '
      +	test_attempts_fast_forward pull --ff-only --rebase
      +'
      +
     -+test_expect_success '--ff-only takes precedence over --no-rebase' '
     ++test_expect_success '--ff-only overrides --no-rebase' '
      +	test_attempts_fast_forward pull --ff-only --no-rebase
      +'
      +
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
      +'
      +
     -+# Rule 2: --rebase=[!false] takes precedence over --no-ff and --ff
     -+# (Corollary: pull.rebase=!false overrides pull.ff=!only)
     -+test_expect_success '--rebase takes precedence over --no-ff' '
     ++# Group 2: --rebase=[!false] overrides --no-ff and --ff
     ++# (And related interaction of pull.rebase=!false and pull.ff=!only)
     ++test_expect_success '--rebase overrides --no-ff' '
      +	test_does_rebase pull --rebase --no-ff
      +'
      +
     -+test_expect_success '--rebase takes precedence over --ff' '
     ++test_expect_success '--rebase overrides --ff' '
      +	test_does_rebase pull --rebase --ff
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over pull.ff=false' '
     ++test_expect_success '--rebase fast-forwards when possible' '
     ++	test_does_fast_forward pull --rebase --ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=true overrides pull.ff=false' '
      +	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
     ++test_expect_success 'pull.rebase=true overrides pull.ff=true' '
      +	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
      +'
      +
     -+# Rule 3: command line flags take precedence over config
     ++# Group 3: command line flags take precedence over config
      +test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
      +	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
      +'
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
      +'
      +
     -+test_expect_failure '--no-rebase overrides pull.ff=only' '
     -+	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     ++test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     ++	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
      +'
      +
      +test_expect_success '--rebase takes precedence over pull.ff=only' '
      +	test_does_rebase -c pull.ff=only pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff=true' '
     ++test_expect_success '--rebase overrides pull.ff=true' '
      +	test_does_rebase -c pull.ff=true pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff=false' '
     ++test_expect_success '--rebase overrides pull.ff=false' '
      +	test_does_rebase -c pull.ff=false pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff unset' '
     ++test_expect_success '--rebase overrides pull.ff unset' '
      +	test_does_rebase pull --rebase
      +'
      +
     -+# Rule 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
     ++# Group 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
      +
      +test_expect_success '--no-rebase works with --no-ff' '
     -+	test_does_merge_noff pull --no-rebase --no-ff
     ++	test_does_merge_when_ff_possible pull --no-rebase --no-ff
      +'
      +
      +test_expect_success '--no-rebase works with --ff' '
     -+	test_does_merge_ff pull --no-rebase --ff
     ++	test_does_fast_forward pull --no-rebase --ff
      +'
      +
      +test_expect_success '--no-rebase does ff if pull.ff unset' '
     -+	test_does_merge_ff pull --no-rebase
     ++	test_does_fast_forward pull --no-rebase
      +'
      +
      +test_expect_success '--no-rebase heeds pull.ff=true' '
     -+	test_does_merge_ff -c pull.ff=true pull --no-rebase
     ++	test_does_fast_forward -c pull.ff=true pull --no-rebase
      +'
      +
      +test_expect_success '--no-rebase heeds pull.ff=false' '
     -+	test_does_merge_noff -c pull.ff=false pull --no-rebase
     ++	test_does_merge_when_ff_possible -c pull.ff=false pull --no-rebase
      +'
      +
     -+# Rule 5: pull.rebase=!false takes precedence over --no-ff and --ff
     -+test_expect_success 'pull.rebase=true takes precedence over --no-ff' '
     ++# Group 5: pull.rebase=!false in combination with --no-ff or --ff
     ++test_expect_success 'pull.rebase=true and --no-ff' '
      +	test_does_rebase -c pull.rebase=true pull --no-ff
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over --ff' '
     ++test_expect_success 'pull.rebase=true and --ff' '
      +	test_does_rebase -c pull.rebase=true pull --ff
      +'
      +
     -+# End of precedence rules
     ++test_expect_success 'pull.rebase=false and --no-ff' '
     ++	test_does_merge_when_ff_possible -c pull.rebase=false pull --no-ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=false and --ff, ff possible' '
     ++	test_does_fast_forward -c pull.rebase=false pull --ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=false and --ff, ff not possible' '
     ++	test_falls_back_to_full_merge -c pull.rebase=false pull --ff
     ++'
     ++
     ++# End of groupings for conflicting merge vs. rebase flags/options
      +
       test_expect_success 'merge c1 with c2' '
       	git reset --hard c1 &&
  2:  329802382bf !  2:  66fe7f7f934 t7601: add tests of interactions with multiple merge heads and config
     @@ Commit message
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over --ff' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=false and --ff, ff not possible' '
       
     - # End of precedence rules
     + # End of groupings for conflicting merge vs. rebase flags/options
       
     -+test_expect_failure 'Multiple heads does not warn about fast forwarding' '
     ++test_expect_failure 'Multiple heads warns about inability to fast forward' '
      +	git reset --hard c1 &&
     -+	git pull . c2 c3 2>err &&
     -+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
     ++	test_must_fail git pull . c2 c3 2>err &&
     ++	test_i18ngrep "Pulling without specifying how to reconcile" err
      +'
      +
     -+test_expect_success 'Cannot fast-forward with multiple heads' '
     ++test_expect_failure 'Multiple can never be fast forwarded' '
      +	git reset --hard c0 &&
      +	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
      +	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     -+	test_i18ngrep "Not possible to fast-forward, aborting" err
     ++	# In addition to calling out "cannot fast-forward", we very much
     ++	# want the "multiple branches" piece to be called out to users.
     ++	test_i18ngrep "Cannot fast-forward to multiple branches" err
      +'
      +
      +test_expect_success 'Cannot rebase with multiple heads' '
  3:  ae54afd8b01 !  3:  c45cd239666 pull: abort if --ff-only is given and fast-forwarding is impossible
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       	if (opt_rebase) {
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_attempts_fast_forward() {
     - # Rule 1: --ff-only takes precedence over --[no-]rebase
     - # (Corollary: pull.ff=only overrides pull.rebase)
     +@@ t/t7601-merge-pull-config.sh: test_attempts_fast_forward () {
     + # Group 1: Interaction of --ff-only with --[no-]rebase
     + # (And related interaction of pull.ff=only with pull.rebase)
       #
     --test_expect_failure '--ff-only takes precedence over --rebase' '
     -+test_expect_success '--ff-only takes precedence over --rebase' '
     +-test_expect_failure '--ff-only overrides --rebase' '
     ++test_expect_success '--ff-only overrides --rebase' '
       	test_attempts_fast_forward pull --rebase --ff-only
       '
       
     --test_expect_failure '--ff-only takes precedence over --rebase even if first' '
     -+test_expect_success '--ff-only takes precedence over --rebase even if first' '
     +-test_expect_failure '--ff-only overrides --rebase even if first' '
     ++test_expect_success '--ff-only overrides --rebase even if first' '
       	test_attempts_fast_forward pull --ff-only --rebase
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence over --no-rebase' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only overrides --no-rebase' '
       	test_attempts_fast_forward pull --ff-only --no-rebase
       '
       
     @@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence ov
       	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true overrides pull.ff=true' '
       '
       
     - # Rule 3: command line flags take precedence over config
     + # Group 3: command line flags take precedence over config
      -test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
      +test_expect_success '--ff-only takes precedence over pull.rebase=true' '
       	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_failure '--no-rebase overrides pull.ff=only' '
     - 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     +@@ t/t7601-merge-pull-config.sh: test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     + 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
       '
       
      -test_expect_success '--rebase takes precedence over pull.ff=only' '
  4:  de4b460b09d <  -:  ----------- pull: since --ff-only overrides, handle it first
  5:  3d9ff69198e !  4:  1a821d3b1dd pull: ensure --rebase overrides ability to ff
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    pull: ensure --rebase overrides ability to ff
     +    pull: since --ff-only overrides, handle it first
      
     -    Now that the handling of fast-forward-only in combination with rebases
     -    has been moved before the merge-vs-rebase logic, we have an unnecessary
     -    special fast-forward case left within the rebase logic.  Actually, more
     -    than unnecessary, it's actually a violation of the rules.  As per
     -    https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/, --rebase is
     -    supposed to override all ff flags other than an explicit --ff-only.
     -    Ensure that it does so by removing the fast-forward special case that
     -    exists within the rebase logic.
     +    There are both merge and rebase branches in the logic, and previously
     +    both had to handle fast-forwarding.  Merge handled that implicitly
     +    (because git merge handles it directly), while in rebase it was
     +    explicit.  Given that the --ff-only flag is meant to override any
     +    --rebase or --no-rebase, make the code reflect that by handling
     +    --ff-only before the merge-vs-rebase logic.
     +
     +    It turns out that this also fixes a bug for submodules.  Previously,
     +    when --ff-only was given, the code would run `merge --ff-only` on the
     +    main module, and then run `submodule update --recursive --rebase` on the
     +    submodules.  With this change, we still run `merge --ff-only` on the
     +    main module, but now run `submodule update --recursive --checkout` on
     +    the submodules.  I believe this better reflects the intent of --ff-only
     +    to have it apply to both the main module and the submodules.
     +
     +    (Sidenote: It is somewhat interesting that all merges pass `--checkout`
     +    to submodule update, even when `--no-ff` is specified, meaning that it
     +    will only do fast-forward merges for submodules.  This was discussed in
     +    commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule
     +    changes only)", 2017-06-23).  The same limitations apply now as then, so
     +    we are not trying to fix this at this time.)
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/pull.c ##
      @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     + 
     + 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
     + 
     +-	if (!can_ff) {
     +-		if (opt_ff) {
     +-			if (!strcmp(opt_ff, "--ff-only"))
     +-				die_ff_impossible();
     +-		} else {
     +-			if (rebase_unspecified && opt_verbosity >= 0)
     +-				show_advice_pull_non_ff();
     +-		}
     ++	/* ff-only takes precedence over rebase */
     ++	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
     ++		if (!can_ff)
     ++			die_ff_impossible();
     ++		opt_rebase = REBASE_FALSE;
     + 	}
     ++	/* If no action specified and we can't fast forward, then warn. */
     ++	if (!opt_ff && rebase_unspecified && !can_ff)
     ++		show_advice_pull_non_ff();
     + 
     + 	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"));
       
  6:  b379fea097d !  5:  9b116f3d284 pull: make --rebase and --no-rebase override pull.ff=only
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
      -	if (!opt_ff)
      +	if (!opt_ff) {
       		opt_ff = xstrdup_or_null(config_get_ff());
     ++		/*
     ++		 * A subtle point: opt_ff was set on the line above via
     ++		 * reading from config.  opt_rebase, in contrast, is set
     ++		 * before this point via command line options.  The setting
     ++		 * of opt_rebase via reading from config (using
     ++		 * config_get_rebase()) does not happen until later.  We
     ++		 * are relying on the next if-condition happening before
     ++		 * the config_get_rebase() call so that an explicit
     ++		 * "--rebase" can override a config setting of
     ++		 * pull.ff=only.
     ++		 */
      +		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
      +			opt_ff = "--ff";
      +	}
     @@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence ov
       	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
       '
       
     --test_expect_failure '--no-rebase overrides pull.ff=only' '
     -+test_expect_success '--no-rebase overrides pull.ff=only' '
     - 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     +-test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     ++test_expect_success '--no-rebase takes precedence over pull.ff=only' '
     + 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
       '
       
      -test_expect_failure '--rebase takes precedence over pull.ff=only' '
  7:  dca0455898a !  6:  f061f8b4e75 pull: abort by default when fast-forwarding is not possible
     @@ builtin/pull.c: static int get_can_ff(struct object_id *orig_head, struct object
       		 "  git config pull.rebase false  # merge (the default strategy)\n"
       		 "  git config pull.rebase true   # rebase\n"
      @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     - 			return run_merge();
     + 		opt_rebase = REBASE_FALSE;
       	}
       	/* If no action specified and we can't fast forward, then warn. */
      -	if (!opt_ff && rebase_unspecified && !can_ff)
     @@ t/t6402-merge-rename.sh: test_expect_success 'setup' '
      -	test_expect_code 1 git pull . white &&
      +	test_expect_code 1 git pull --no-rebase . white &&
       	git ls-files -s &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into another renaming one' \
       	rm -f B &&
       	git reset --hard &&
       	git checkout red &&
      -	test_expect_code 1 git pull . white &&
      +	test_expect_code 1 git pull --no-rebase . white &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     - 	git ls-files -s N >n.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
     + 	sed -ne "/^g/{
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull unrenaming branch into renaming one' \
       '
       	git reset --hard &&
       	git show-branch &&
      -	test_expect_code 1 git pull . main &&
      +	test_expect_code 1 git pull --no-rebase . main &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     - 	git ls-files -s N >n.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
     + 	sed -ne "/^g/{
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull conflicting renames' \
       '
       	git reset --hard &&
       	git show-branch &&
      -	test_expect_code 1 git pull . blue &&
      +	test_expect_code 1 git pull --no-rebase . blue &&
     - 	git ls-files -u A >a.stages &&
     - 	test_line_count = 1 a.stages &&
     - 	git ls-files -u B >b.stages &&
     + 	test_stdout_line_count = 1 git ls-files -u A &&
     + 	test_stdout_line_count = 1 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -u C &&
      @@ t/t6402-merge-rename.sh: test_expect_success 'interference with untracked working tree file' '
       	git reset --hard &&
       	git show-branch &&
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'setup' '
      +	test_i18ngrep ! "You have divergent branches" err
       '
       
     - test_does_rebase() {
     + test_does_rebase () {
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=false and --ff, ff not possible' '
     + 
     + # End of groupings for conflicting merge vs. rebase flags/options
     + 
     +-test_expect_failure 'Multiple heads warns about inability to fast forward' '
     ++test_expect_success 'Multiple heads warns about inability to fast forward' '
     + 	git reset --hard c1 &&
     + 	test_must_fail git pull . c2 c3 2>err &&
     +-	test_i18ngrep "Pulling without specifying how to reconcile" err
     ++	test_i18ngrep "You have divergent branches" err
     + '
     + 
     + test_expect_failure 'Multiple can never be fast forwarded' '
     + 	git reset --hard c0 &&
     + 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
     +-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     ++	test_i18ngrep ! "You have divergent branches" err &&
     + 	# In addition to calling out "cannot fast-forward", we very much
     + 	# want the "multiple branches" piece to be called out to users.
     + 	test_i18ngrep "Cannot fast-forward to multiple branches" err
     +@@ t/t7601-merge-pull-config.sh: test_expect_failure 'Multiple can never be fast forwarded' '
     + test_expect_success 'Cannot rebase with multiple heads' '
     + 	git reset --hard c0 &&
     + 	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
     +-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     ++	test_i18ngrep ! "You have divergent branches" err &&
     + 	test_i18ngrep "Cannot rebase onto multiple branches." err
     + '
     + 
      
       ## t/t7603-merge-reduce-heads.sh ##
      @@ t/t7603-merge-reduce-heads.sh: test_expect_success 'merge c1 with c2, c3, c4, c5' '
  8:  d1952f014f2 !  7:  90d49e0fb78 pull: update docs & code for option compatibility with rebasing
     @@ Commit message
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     + ## Documentation/git-merge.txt ##
     +@@ Documentation/git-merge.txt: merge has resulted in conflicts.
     + 
     + OPTIONS
     + -------
     ++:git-merge: 1
     ++
     + include::merge-options.txt[]
     + 
     + -m <msg>::
     +
       ## Documentation/git-pull.txt ##
     -@@ Documentation/git-pull.txt: When false, merge the current branch into the upstream branch.
     - +
     - When `interactive`, enable the interactive mode of rebase.
     - +
     -+Note that `--ff-only` takes precedence over any `--rebase` flag.
     -++
     - See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
     - linkgit:git-config[1] if you want to make `git pull` always use
     - `--rebase` instead of merging.
     +@@ Documentation/git-pull.txt: Incorporates changes from a remote repository into the current branch.
     + If the current branch is behind the remote, then by default it will
     + fast-forward the current branch to match the remote.  If the current
     + branch and the remote have diverged, the user needs to specify how to
     +-reconcile the divergent branches with `--no-ff`, `--ff`, or `--rebase`
     +-(or the corresponding configuration options in `pull.ff` or
     +-`pull.rebase`).
     ++reconcile the divergent branches with `--rebase` or `--no-rebase` (or
     ++the corresponding configuration option in `pull.rebase`).
     + 
     + More precisely, `git pull` runs `git fetch` with the given parameters
     + and then depending on configuration options or command line flags,
     +-will call either `git merge` or `git rebase` to reconcile diverging
     ++will call either `git rebase` or `git merge` to reconcile diverging
     + branches.
     + 
     + <repository> should be the name of a remote repository as
     +@@ Documentation/git-pull.txt: published that history already.  Do *not* use this option
     + unless you have read linkgit:git-rebase[1] carefully.
     + 
     + --no-rebase::
     +-	Override earlier --rebase.
     ++	This is shorthand for --rebase=false.
     + 
     + Options related to fetching
     + ~~~~~~~~~~~~~~~~~~~~~~~~~~~
      
       ## Documentation/merge-options.txt ##
      @@
     @@ Documentation/merge-options.txt
       +
       With --no-commit perform the merge and stop just before creating
       a merge commit, to give the user a chance to inspect and further
     -@@ Documentation/merge-options.txt: could instead be resolved as a fast-forward.
     +@@ Documentation/merge-options.txt: set to `no` at the beginning of them.
     + 	to `MERGE_MSG` before being passed on to the commit machinery in the
     + 	case of a merge conflict.
     + 
     ++ifdef::git-merge[]
     + --ff::
     + --no-ff::
     + --ff-only::
     +@@ Documentation/merge-options.txt: set to `no` at the beginning of them.
     + 	default unless merging an annotated (and possibly signed) tag
     + 	that is not stored in its natural place in the `refs/tags/`
     + 	hierarchy, in which case `--no-ff` is assumed.
     ++endif::git-merge[]
     ++ifdef::git-pull[]
     ++--ff-only::
     ++	Only update to the new history if there is no divergent local
     ++	history.  This is the default when no method for reconciling
     ++	divergent histories is provided (via the --rebase=* flags).
     ++
     ++--ff::
     ++--no-ff::
     ++	When merging rather than rebasing, specifies how a merge is
     ++	handled when the merged-in history is already a descendant of
     ++	the current history.  If merging is requested, `--ff` is the
     ++	default unless merging an annotated (and possibly signed) tag
     ++	that is not stored in its natural place in the `refs/tags/`
     ++	hierarchy, in which case `--no-ff` is assumed.
     ++endif::git-pull[]
     + +
     + With `--ff`, when possible resolve the merge as a fast-forward (only
     + update the branch pointer to match the merged branch; do not create a
     +@@ Documentation/merge-options.txt: descendant of the current history), create a merge commit.
     + +
     + With `--no-ff`, create a merge commit in all cases, even when the merge
     + could instead be resolved as a fast-forward.
     ++ifdef::git-merge[]
       +
       With `--ff-only`, resolve the merge as a fast-forward when possible.
       When not possible, refuse to merge and exit with a non-zero status.
     -+ifdef::git-pull[]
     -++
     -+Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
     -+endif::git-pull[]
     ++endif::git-merge[]
       
       -S[<keyid>]::
       --gpg-sign[=<keyid>]::
  9:  3d8df246772 !  8:  f03b15b7eb0 pull: fix handling of multiple heads
     @@ Commit message
          pull: fix handling of multiple heads
      
          With multiple heads, we should not allow rebasing or fast-forwarding.
     -    Also, it seems wrong to have our can_ff computation return true, so fix
     -    that while we are at it too (we won't actually use the can_ff flag due
     -    to setting opt_ff to "--no-ff", but it's confusing to leave it as
     -    computed to be true).
     +    Make sure any fast-forward request calls out specifically the fact that
     +    multiple branches are in play.  Also, since we cannot fast-forward to
     +    multiple branches, fix our computation of can_ff.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
      +			die(_("Cannot rebase onto multiple branches."));
      +		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
      +			die(_("Cannot fast-forward to multiple branches."));
     -+		if (!opt_ff)
     -+			opt_ff = "--no-ff";
      +	}
       
      -	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over --ff' '
     - 
     - # End of precedence rules
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'Multiple heads warns about inability to fast forward' '
     + 	test_i18ngrep "You have divergent branches" err
     + '
       
     --test_expect_failure 'Multiple heads does not warn about fast forwarding' '
     -+test_expect_success 'Multiple heads does not warn about fast forwarding' '
     - 	git reset --hard c1 &&
     - 	git pull . c2 c3 2>err &&
     - 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'Cannot fast-forward with multiple heads' '
     +-test_expect_failure 'Multiple can never be fast forwarded' '
     ++test_expect_success 'Multiple can never be fast forwarded' '
       	git reset --hard c0 &&
       	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
     - 	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     --	test_i18ngrep "Not possible to fast-forward, aborting" err
     -+	test_i18ngrep "Cannot fast-forward to multiple branches" err
     - '
     - 
     - test_expect_success 'Cannot rebase with multiple heads' '
     + 	test_i18ngrep ! "You have divergent branches" err &&

-- 
gitgitgadget

  parent reply	other threads:[~2021-07-21  1:43 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-17 15:41 [PATCH 0/9] Handle pull option precedence 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 ` Elijah Newren via GitGitGadget [this message]
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   ` [PATCH v3 " Elijah Newren via GitGitGadget
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

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

  git send-email \
    --in-reply-to=pull.1049.v2.git.git.1626831744.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=alexhenrie24@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=matthias.baumgarten@aixigo.com \
    --cc=newren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=sunshine@sunshineco.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.