Re: [PATCH] rebase: display an error if --root and --fork-point are both provided

[Alban Gruin <alban.gruin@gmail.com>]

Hi Elijah,

Le 27/04/2020 à 18:21, Elijah Newren via GitGitGadget a écrit :
> From: Elijah Newren <newren@gmail.com>
> 
> --root implies we want to rebase all commits since the beginning of
> history.  --fork-point means we want to use the reflog of the specified
> upstream to find the best common ancestor between <upstream> and
> <branch> and only rebase commits since that common ancestor.  These
> options are clearly contradictory, so throw an error (instead of
> segfaulting on a NULL pointer) if both are specified.
> 
> Reported-by: Alexander Berg <alexander.berg@atos.net>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     rebase: display an error if --root and --fork-point are both provided
>     
>     --root implies we want to rebase all commits since the beginning of
>     history. --fork-point means we want to use the reflog of the specified
>     upstream to find the best common ancestor between and and only rebase
>     commits since that common ancestor. These options are clearly
>     contradictory, so throw an error (instead of segfaulting on a NULL
>     pointer) if both are specified.
>     
>     Reported-by: Alexander Berg alexander.berg@atos.net
>     [alexander.berg@atos.net]Signed-off-by: Elijah Newren newren@gmail.com
>     [newren@gmail.com]
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-771%2Fnewren%2Frebase-fork-point-root-error-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-771/newren/rebase-fork-point-root-error-v1
> Pull-Request: https://github.com/git/git/pull/771
> 
>  builtin/rebase.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index bff53d5d167..287ac1aa21b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1652,6 +1652,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die(_("cannot combine '--keep-base' with '--root'"));
>  	}
>  
> +	if (options.root && fork_point > 0)
> +		die(_("cannot combine '--root' with '--fork-point'"));
> +
>  	if (action != ACTION_NONE && !in_progress)
>  		die(_("No rebase in progress?"));
>  	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
> 
> base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec
> 

The patch is fairly obvious and looks good to me.

The documentation must be updated, too, since it does not mention the
incompatibility between the two switches:

-- snip --
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index bed500f151..a30de5aa20 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -448,12 +448,14 @@ When --fork-point is active, 'fork_point' will be
used instead of
 <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
 ends up being empty, the <upstream> will be used as a fallback.
 +
-If either <upstream> or --root is given on the command line, then the
-default is `--no-fork-point`, otherwise the default is `--fork-point`.
+If <upstream> is given on the command line, then the default is
+`--no-fork-point`, otherwise the default is `--fork-point`.
 +
 If your branch was based on <upstream> but <upstream> was rewound and
 your branch contains commits which were dropped, this option can be used
 with `--keep-base` in order to drop those commits from your branch.
++
+See also INCOMPATIBLE OPTIONS below.

 --ignore-whitespace::
 --whitespace=<option>::
@@ -635,6 +637,7 @@ In addition, the following pairs of options are
incompatible:
  * --preserve-merges and --empty=
  * --keep-base and --onto
  * --keep-base and --root
+ * --fork-point and --root

 BEHAVIORAL DIFFERENCES
 -----------------------
-- snap --

Cheers,
Alban