Re: [PATCH] experiment: pull: change --ff-only and default mode
From: Elijah Newren <hidden>
Date: 2020-12-05 17:42:35
On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras [off-list ref] 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?)
quoted hunk ↗ jump to hunk
Signed-off-by: Felipe Contreras <redacted> --- 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?