git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "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>
Subject: Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options
Date: Mon, 19 Jul 2021 11:23:34 -0700	[thread overview]
Message-ID: <xmqq35samac9.fsf@gitster.g> (raw)
In-Reply-To: <6cb771297f5f7d5bb0c6734bcb3fe6d3b8bb4c88.1626536508.git.gitgitgadget@gmail.com> (Elijah Newren via GitGitGadget's message of "Sat, 17 Jul 2021 15:41:39 +0000")

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_does_rebase() {

Style: missing SP before ().

> +	git reset --hard c2 &&
> +	git "$@" . c1 &&

OK.

It is clear, from the "." (this repository), that this is designed
to test nothing but "git pull", and it is somewhat unfortunate that
we cannot spell out 'git pull' here (instead we ask the callers to
always pass 'pull' subcommand from the command line), but that is
understandable, as the primary reason we lack 'pull' from this
command line is because we expect to pass one-shot configuration to
the command via "-c pull.rebase=X".

> +	# Check that we actually did a rebase
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 3 0 >expect &&

The answer is 3 and zero.  OK.  The point of having this helper is
not because we want to do "pull --rebase" of different histories on
top of different base, but because we want to ensure that with
different configuration and command line options, the same "pull"
will result in the same flattening rebase.  So it is not a problem
at all that these numbers are hardcoded (it might make it less
fragile to count only commits above c2, but it probably would not
matter).

> +	test_cmp expect actual &&
> +	rm actual expect
> +}

OK.

> +test_does_merge_noff() {
> +	git reset --hard c0 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 3 1 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_does_merge_ff() {
> +	git reset --hard c0 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 2 0 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_does_need_full_merge() {
> +	git reset --hard c2 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 4 1 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_attempts_fast_forward() {
> +	git reset --hard c2 &&
> +	test_must_fail git "$@" . c1 2>err &&
> +	test_i18ngrep "Not possible to fast-forward, aborting" err
> +}

The same reasoning says these test_does_X helpers make sense.  I am
not sure about the name does_need_full_merge though---what does it
want to ensure is not very clear to me.  Is it named that way because
you found "test_does_merge" (when contrasted to "test_does_merge_ff")
sounds too weak?

> +#
> +# Rule 1: --ff-only takes precedence over --[no-]rebase
> +# (Corollary: pull.ff=only overrides pull.rebase)
> +#
> +test_expect_failure '--ff-only takes precedence over --rebase' '
> +	test_attempts_fast_forward pull --rebase --ff-only
> +'
> +
> +test_expect_failure '--ff-only takes precedence over --rebase even if first' '
> +	test_attempts_fast_forward pull --ff-only --rebase
> +'
> +
> +test_expect_success '--ff-only takes precedence over --no-rebase' '
> +	test_attempts_fast_forward pull --ff-only --no-rebase
> +'
> +
> +test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
> +'
> +
> +test_expect_success 'pull.ff=only overrides pull.rebase=false' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
> +'

OK.  These all ensure that when the history does not fast-forward,
the command will fail when --ff-only tells us to allow only
fast-forward.  I am not sure "takes precedence" is a meaningful
label, though.  It is more like "ff-only means ff-only and fails
when the upstream history is not a descendant, no matter how the
possible integration is set to be performed".

> +# 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' '
> +	test_does_rebase pull --rebase --no-ff
> +'
> +
> +test_expect_success '--rebase takes precedence over --ff' '
> +	test_does_rebase pull --rebase --ff
> +'
> +
> +test_expect_success 'pull.rebase=true takes precedence over 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_does_rebase -c pull.rebase=true -c pull.ff=true pull
> +'

Sounds sensible.  Again, I do not view this as precedence, though.
"--ff" is merely "if there is nothing else needs to be done, it is
OK to fast-forward to their history", so with --rebase, it either
(1) gets ignored when we have something to be done, i.e. our own
development to replay on top of their history, or (2) becomes a
no-op as there truly isn't any development of our own.

And "--no-ff" is more or less a meaningless thing to say ("I do not
want to just fast-forward when I do not have anything meaningful to
add, I want an empty merge commit to mark where I was") in the
context of "--rebase".  Erroring out only when their histroy is
descendant of ours and "--no-ff" and "--rebase=<set>" are given
explicitly from the command line might make sense, but I do not
think of a sensible corrective action the end-user wants to do after
seeing such an error (after all, there was nothing to rebase on top
of their history), so I think ignoring is a more acceptable outcome
when we have nothing to replay.

Do we ensure that "pull --rebase --ff" fast-forwards when the
history truly fast-forwards?  test_does_rebase only and always
checks what happens when pulling c1 into c2 and nothing else, so I
do not think the above are testing that case.

IOW, I think "test_does_merge_ff pull --rebase --ff" wants to be
there somewhere?

> +# Rule 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
> +'
> +
> +test_expect_success '--ff-only takes precedence over pull.rebase=false' '
> +	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
> +'

Both are good.

> +test_expect_failure '--no-rebase overrides pull.ff=only' '
> +	test_does_need_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
> +'

OK.

> +test_expect_success '--rebase takes precedence over pull.ff=true' '
> +	test_does_rebase -c pull.ff=true pull --rebase
> +'
> +
> +test_expect_success '--rebase takes precedence over pull.ff=false' '
> +	test_does_rebase -c pull.ff=false pull --rebase
> +'
> +
> +test_expect_success '--rebase takes precedence over pull.ff unset' '
> +	test_does_rebase pull --rebase
> +'

These three are correct but again I do not see them as precedence
matter.

> +# Rule 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
> +'

OK.

> +test_expect_success '--no-rebase works with --ff' '
> +	test_does_merge_ff pull --no-rebase --ff
> +'

OK.

> +test_expect_success '--no-rebase does ff if pull.ff unset' '
> +	test_does_merge_ff pull --no-rebase
> +'

OK.

> +test_expect_success '--no-rebase heeds pull.ff=true' '
> +	test_does_merge_ff -c pull.ff=true pull --no-rebase
> +'

OK (pull.ff=true is the default anyway).

> +test_expect_success '--no-rebase heeds pull.ff=false' '
> +	test_does_merge_noff -c pull.ff=false pull --no-rebase
> +'

OK.

> +# Rule 5: pull.rebase=!false takes precedence over --no-ff and --ff
> +test_expect_success 'pull.rebase=true takes precedence over --no-ff' '
> +	test_does_rebase -c pull.rebase=true pull --no-ff
> +'

OK.  When we do have our own commits they do not have, there is no
point in letting --no-ff do anything special.  It would be sensible
to replay ours on top of theirs.


> +test_expect_success 'pull.rebase=true takes precedence over --ff' '
> +	test_does_rebase -c pull.rebase=true pull --ff
> +'

Again, I am not sure if this is "precedence" issue.  "ff" merely
means "fast-forwarding is allowed, when we do not have anything to
add to their history", and we do have our own work in the test
scenario test_does_rebase presents us, so rebasing would be quite
sensible.  Similarly

    test_does_need_full_merge -c pull.rebase=false pull --ff

would be true, right?

> +# End of precedence rules
> +
>  test_expect_success 'merge c1 with c2' '
>  	git reset --hard c1 &&
>  	test -f c0.c &&

The series of new tests makes me wonder if there is a good way to
ensure we covered full matrix, but I didn't see any that smelled
wrong.

Thanks.


  parent reply	other threads:[~2021-07-19 20:59 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 [this message]
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   ` [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

  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=xmqq35samac9.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=alexhenrie24@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=matthias.baumgarten@aixigo.com \
    --cc=newren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options' \
    /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).