Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Elijah

On 25/01/2023 04:03, 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.

This version looks good to me. Thanks for working on it - it turned out 
to be quite a bit of work in the end but it is nice improvement, 
especially the last patch.

Best Wishes

Phillip


> Changes since v4:
> 
>   * Pulled out the changes regarding incompatibility detection for
>     --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
>     with understanding the behavior, suggesting changes, and getting the
>     wording right, and I think it deserves its own patch with its own
>     explanation.
> 
> 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 newren@gmail.com
> 
> Elijah Newren (10):
>    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: fix incompatiblity checks for --[no-]reapply-cherry-picks
>    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                       | 79 +++++++++++++++++++-------
>   t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--
>   3 files changed, 163 insertions(+), 64 deletions(-)
> 
> 
> base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1466
> 
> Range-diff vs v4:
> 
>    1:  8a676e6ec1a =  1:  8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
>    2:  cc129b87185 =  2:  cc129b87185 rebase: flag --apply and --merge as incompatible
>    3:  9464bbbe9ba =  3:  9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
>    4:  b702f15ed7c =  4:  b702f15ed7c rebase: fix docs about incompatibilities with --root
>    -:  ----------- >  5:  3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
>    5:  5e4851e611e !  6:  2777dd2788a rebase: add coverage of other incompatible options
>       @@ Commit message
>        
>            The git-rebase manual noted several sets of incompatible options, but
>            we were missing tests for a few of these.  Further, we were missing
>       -    code checks for some of these, which could result in command line
>       +    code checks for one of these, which could result in command line
>            options being silently ignored.
>        
>            Also, note that adding a check for autosquash means that using
>       @@ Commit message
>        
>            Signed-off-by: Elijah Newren <newren@gmail.com>
>        
>       - ## 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;
>       - 	}
>       -+	/*
>       -+	 * 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, 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.
>       -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>       - 	if (options.empty != EMPTY_UNSPECIFIED)
>       - 		imply_merge(&options, "--empty");
>       -
>       --	/*
>       --	 * --keep-base implements --reapply-cherry-picks by altering upstream so
>       --	 * it works with both backends.
>       --	 */
>       --	if (options.reapply_cherry_picks && !keep_base)
>       --		imply_merge(&options, "--reapply-cherry-picks");
>       --
>       - 	if (gpg_sign)
>       - 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>       -
>        @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>         	if (options.update_refs)
>         		imply_merge(&options, "--update-refs");
>       @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
>        +		test_must_fail git rebase $opt --empty=ask A
>        +	"
>        +
>       -+	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
>       -+		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" "
>       + 	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
>         		git checkout B^0 &&
>       - 		test_must_fail git rebase $opt --update-refs A
>       + 		test_must_fail git rebase $opt --no-reapply-cherry-picks A
>    6:  21ae9e05313 !  7:  0d0541ea243 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=
>       -  * --[no-]reapply-cherry-picks
>       +  * --[no-]reapply-cherry-picks when used without --keep-base
>        - * --edit-todo
>          * --update-refs
>          * --root when used without --onto
>    7:  74a216bf0c0 =  8:  01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
>    8:  a8adf7fda61 =  9:  f646abee524 rebase: put rebase_options initialization in single place
>    9:  5cb00e5103b = 10:  328f5a1d534 rebase: provide better error message for apply options vs. merge config
>