Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +test_does_rebase() {
>
> Style: missing SP before ().

> > +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?

> > +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.
>

>
> > +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.