[v4,0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities

Message

Philippe Blain via GitGitGadget Jan. 22, 2023, 6:12 a.m. UTC

We had a report about --update-refs being ignored when --whitespace=fix was
passed, confusing an end user. These were already marked as incompatible in
the manual, but the code didn't check for the incompatibility and provide an
error to the user.

Folks brought up other flags tangentially when reviewing an earlier round of
this series, and I found we had more that were missing checks, and that we
were missing some testcases, and that the documentation was wrong on some of
the relevant options. So, I ended up getting lots of little fixes to
straighten these all out.

Changes since v3:

 * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
   the testcases (Thanks to Phillip for pointing out my error)
 * I went ahead and implemented the better error message when the merge
   backend is triggered solely via config options.

Changes since v2:

 * Remove the extra patch and reword the comment in t3422 more thoroughly.
 * Add tests for other flag incompatibilities that were missing
 * Add code that was missing for checking various flag incompatibilities
 * Fix incorrect claims in the documentation around incompatible options
 * A few other miscellaneous fixups noticed while doing all the above

Changes since v1:

 * Add a patch nuking the -C option to rebase (fixes confusion around the
   comment in t3422 and acknowledges the fact that the option is totally and
   utterly useless and always has been. It literally never affects the
   results of a rebase.)

Signed-off-by: Elijah Newren [e-mail]

Elijah Newren (9):
  rebase: mark --update-refs as requiring the merge backend
  rebase: flag --apply and --merge as incompatible
  rebase: remove --allow-empty-message from incompatible opts
  rebase: fix docs about incompatibilities with --root
  rebase: add coverage of other incompatible options
  rebase: clarify the OPT_CMDMODE incompatibilities
  rebase: fix formatting of rebase --reapply-cherry-picks option in docs
  rebase: put rebase_options initialization in single place
  rebase: provide better error message for apply options vs. merge
    config

 Documentation/git-rebase.txt           | 77 +++++++++++++-------------
 builtin/rebase.c                       | 72 +++++++++++++++++++-----
 t/t3422-rebase-incompatible-options.sh | 71 ++++++++++++++++++++++--
 3 files changed, 162 insertions(+), 58 deletions(-)


base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

Range-diff vs v3:

  1:  9089834adea =  1:  8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
  2:  a8b5a0e4fb0 =  2:  cc129b87185 rebase: flag --apply and --merge as incompatible
  3:  f4fbfd40d45 =  3:  9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
  4:  a1e61ac8f21 =  4:  b702f15ed7c rebase: fix docs about incompatibilities with --root
  5:  48c40c0dda0 !  5:  5e4851e611e rebase: add coverage of other incompatible options
     @@ Commit message
          code checks for some of these, which could result in command line
          options being silently ignored.
      
     +    Also, note that adding a check for autosquash means that using
     +    --whitespace=fix together with the config setting rebase.autosquash=true
     +    will trigger an error.  A subsequent commit will improve the error
     +    message.
     +
          Signed-off-by: Elijah Newren <[e-mail]>
      
     + ## Documentation/git-rebase.txt ##
     +@@ Documentation/git-rebase.txt: are incompatible with the following options:
     +  * --exec
     +  * --no-keep-empty
     +  * --empty=
     +- * --reapply-cherry-picks
     ++ * --[no-]reapply-cherry-picks
     +  * --edit-todo
     +  * --update-refs
     +  * --root when used without --onto
     +
       ## builtin/rebase.c ##
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
       		if (options.fork_point < 0)
       			options.fork_point = 0;
       	}
      +	/*
     -+	 * reapply_cherry_picks is slightly weird.  It starts out with a
     -+	 * value of -1.  It will be assigned a value of keep_base below and
     -+	 * then handled specially.  The apply backend is hardcoded to
     -+	 * behave like reapply_cherry_picks==1, so if it has that value, we
     -+	 * can just ignore the flag with the apply backend.  Thus, we only
     -+	 * really need to throw an error and require the merge backend if
     -+	 * reapply_cherry_picks==0.
     ++	 * The apply backend does not support --[no-]reapply-cherry-picks.
     ++	 * The behavior it implements by default is equivalent to
     ++	 * --no-reapply-cherry-picks (due to passing --cherry-picks to
     ++	 * format-patch), but --keep-base alters the upstream such that no
     ++	 * cherry-picks can be found (effectively making it act like
     ++	 * --reapply-cherry-picks).
     ++	 *
     ++	 * Now, if the user does specify --[no-]reapply-cherry-picks, but
     ++	 * does so in such a way that options.reapply_cherry_picks ==
     ++	 * keep_base, then the behavior they get will match what they
     ++	 * expect despite options.reapply_cherry_picks being ignored.  We
     ++	 * could just allow the flag in that case, but it seems better to
     ++	 * just alert the user that they've specified a flag that the
     ++	 * backend ignores.
      +	 */
     -+	if (options.reapply_cherry_picks == 0)
     -+		imply_merge(&options, "--no-reapply-cherry-picks");
     ++	if (options.reapply_cherry_picks >= 0)
     ++		imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
     ++								     "--no-reapply-cherry-picks");
     ++
       	/*
       	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
       	 * commits when using this option.
     @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
      +		git checkout B^0 &&
      +		test_must_fail git rebase $opt --no-reapply-cherry-picks A
      +	"
     ++
     ++	test_expect_success "$opt incompatible with --reapply-cherry-picks" "
     ++		git checkout B^0 &&
     ++		test_must_fail git rebase $opt --reapply-cherry-picks A
     ++	"
      +
       	test_expect_success "$opt incompatible with --update-refs" "
       		git checkout B^0 &&
  6:  8664cce6cf7 !  6:  21ae9e05313 rebase: clarify the OPT_CMDMODE incompatibilities
     @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
      @@ Documentation/git-rebase.txt: are incompatible with the following options:
        * --no-keep-empty
        * --empty=
     -  * --reapply-cherry-picks
     +  * --[no-]reapply-cherry-picks
      - * --edit-todo
        * --update-refs
        * --root when used without --onto
  7:  0e8b06163f2 =  7:  74a216bf0c0 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
  -:  ----------- >  8:  a8adf7fda61 rebase: put rebase_options initialization in single place
  -:  ----------- >  9:  5cb00e5103b rebase: provide better error message for apply options vs. merge config

Comments

Derrick Stolee Jan. 23, 2023, 3:56 p.m. UTC | #1

On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:
> We had a report about --update-refs being ignored when --whitespace=fix was
> passed, confusing an end user. These were already marked as incompatible in
> the manual, but the code didn't check for the incompatibility and provide an
> error to the user.
> 
> Folks brought up other flags tangentially when reviewing an earlier round of
> this series, and I found we had more that were missing checks, and that we
> were missing some testcases, and that the documentation was wrong on some of
> the relevant options. So, I ended up getting lots of little fixes to
> straighten these all out.

Wow, this really expanded since I last looked at it. Thanks for taking on all
of that extra work! (That was not my intention when recommending that you take
over the fix.)
 
> Changes since v3:
> 
>  * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
>    the testcases (Thanks to Phillip for pointing out my error)
>  * I went ahead and implemented the better error message when the merge
>    backend is triggered solely via config options.

I really appreciate this extra attention to detail. I'm also really happy with
how you implemented it, using different variables to signal how the option was
specified (and using -1 for "unset" in both cases).

While I had not been following version 2 or 3, I read this version in its
entirety and everything looked good to me. These improvements to our docs,
tests, and implementation will be felt by users.

Thanks!
-Stolee

Elijah Newren Jan. 24, 2023, 2:05 a.m. UTC | #2

On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee <[e-mail]> wrote:
>
> On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:
> > We had a report about --update-refs being ignored when --whitespace=fix was
> > passed, confusing an end user. These were already marked as incompatible in
> > the manual, but the code didn't check for the incompatibility and provide an
> > error to the user.
> >
> > Folks brought up other flags tangentially when reviewing an earlier round of
> > this series, and I found we had more that were missing checks, and that we
> > were missing some testcases, and that the documentation was wrong on some of
> > the relevant options. So, I ended up getting lots of little fixes to
> > straighten these all out.
>
> Wow, this really expanded since I last looked at it. Thanks for taking on all
> of that extra work! (That was not my intention when recommending that you take
> over the fix.)

Yeah, I know you were willing to let some of the work wait for some
future series, and I intended to take that route.  But...
  * both you and Phillip brought up --autosquash, and it turns out we
didn't check it
  * the above made me check the whole list of incompatibilities and
discover other missing checks
  * adding tests made me discover that the documented
incompatibilities had a few issues
  * you mentioned that an explicit --apply wasn't checked either, and
since I was already diving in I figured I might as well handle that
too
  * even though you mentioned the config fix wasn't needed, both Junio
and Phillip brought it up as well,
    and based on the various feedback gotten so far, I started think
that just addressing it might require
    less work than going through more back and forth on review of the
feature without that implementation.

> > Changes since v3:
> >
> >  * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
> >    the testcases (Thanks to Phillip for pointing out my error)
> >  * I went ahead and implemented the better error message when the merge
> >    backend is triggered solely via config options.
>
> I really appreciate this extra attention to detail. I'm also really happy with
> how you implemented it, using different variables to signal how the option was
> specified (and using -1 for "unset" in both cases).
>
> While I had not been following version 2 or 3, I read this version in its
> entirety and everything looked good to me. These improvements to our docs,
> tests, and implementation will be felt by users.

Thanks for reading!