Re: [PATCH] experiment: pull: change --ff-only and default mode

[Elijah Newren <newren@gmail.com>]

On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:

This commit message should say something more than just "change x", it
should have some words about what it is change from or to.  I think
the thrust of the patch is allowing --ff-only, --merge, and --rebase
to countermand an earlier otherwise-conflicting option.  Perhaps a
commit message of the form:

In git, we sometimes allow conflicting command line options with the
last one winning, e.g.
  git log --patch --no-patch
  git log --no-patch --patch
other times we just error out when conflicting options are given, e.g.
  git checkout -b --orphan NEWBRANCH
  git checkout --orphan -b NEWBRANCH
Previously, we did neither with --no-ff, --merge, and --rebase.
Change these options to have a last-one-wins behavior.


(Although, after writing this out, I wonder if we want to die with a
conflict message instead of going the route I suggested yesterday.
I'm not certain if one is better than the other, but worth
considering.  Thoughts?)

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c  | 40 ++++++++++++++++++++++++++--------------
>  t/t5520-pull.sh | 23 +++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index e389ffcdc3..95ecbdaad5 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -62,6 +62,7 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset
>                 *value = parse_config_rebase("--rebase", arg, 0);
>         else
>                 *value = unset ? REBASE_FALSE : REBASE_TRUE;
> +       if (*value != REBASE_INVALID) default_mode = 0;
>         return *value == REBASE_INVALID ? -1 : 0;
>  }
>
> @@ -114,6 +115,23 @@ static int opt_show_forced_updates = -1;
>  static char *set_upstream;
>  static struct strvec opt_fetch = STRVEC_INIT;
>
> +static int parse_opt_ff_only(const struct option *opt, const char *arg, int unset)
> +{
> +       char **value = opt->value;
> +       opt_rebase = REBASE_FALSE;
> +       *value = "--ff-only";
> +       return 0;
> +}
> +
> +static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
> +{
> +       char **value = opt->value;
> +       opt_ff = NULL;
> +       *value = REBASE_FALSE;
> +       default_mode = 0;
> +       return 0;
> +}
> +
>  static struct option pull_options[] = {
>         /* Shared options */
>         OPT__VERBOSITY(&opt_verbosity),
> @@ -131,8 +149,9 @@ static struct option pull_options[] = {
>                 "(false|true|merges|preserve|interactive)",
>                 N_("incorporate changes by rebasing rather than merging"),
>                 PARSE_OPT_OPTARG, parse_opt_rebase),
> -       OPT_SET_INT('m', "merge", &opt_rebase,
> -               N_("incorporate changes by merging"), 0),
> +       OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL,
> +               N_("incorporate changes by merging"),
> +               PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge),
>         OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
>                 N_("do not show a diffstat at the end of the merge"),
>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> @@ -161,9 +180,9 @@ static struct option pull_options[] = {
>         OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
>                 N_("allow fast-forward"),
>                 PARSE_OPT_NOARG),
> -       OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
> +       OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
>                 N_("abort if fast-forward is not possible"),
> -               PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> +               PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
>         OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>                 N_("verify that the named commit has a valid GPG signature"),
>                 PARSE_OPT_NOARG),
> @@ -924,6 +943,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         git_config(git_pull_config, NULL);
>
> +       opt_ff = xstrdup_or_null(config_get_ff());
> +       opt_rebase = config_get_rebase();
> +
>         argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>
>         if (cleanup_arg)
> @@ -935,12 +957,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         parse_repo_refspecs(argc, argv, &repo, &refspecs);
>
> -       if (!opt_ff)
> -               opt_ff = xstrdup_or_null(config_get_ff());
> -
> -       if (opt_rebase < 0)
> -               opt_rebase = config_get_rebase();
> -
>         if (read_cache_unmerged())
>                 die_resolve_conflict("pull");
>
> @@ -1037,10 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 }
>         }
>
> -       /* Disable --ff-only when --merge is specified */
> -       if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only"))
> -               opt_ff = NULL;
> -
>         if (opt_rebase) {
>                 int ret = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index f48d0f8d50..c0cfde54e1 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -881,4 +881,27 @@ test_expect_success 'git pull non-fast-forward error message' '
>         grep -q "The pull was not fast-forward" error
>  '
>
> +test_expect_success 'git pull --merge overrides --ffonly' '

You're missing a hyphen there; it should be --ff-only.

> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       git pull --ff-only --merge
> +'
> +
> +test_expect_success 'git pull --ff-only overrides --merge' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       test_must_fail git pull --merge --ff-only
> +'

What about --ff-only and --rebase?  Are there already tests for those?