git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Dec 5, 2020, 17:29 UTC
Message-ID
<CABPp-BE7B5hn3Fc4zD1o+qoqihJqCut=R1TP_fxMXW42+6iL+w@mail.gmail.com>
In-Reply-To
<20201205040644.1259845-1-felipe.contreras@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?)

Show 109 quoted lines
> 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.
Show 21 quoted lines
> +       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?
Previous: Felipe ContrerasNext: Felipe Contreras
Message 70 of 79 in “pull: default warning improvements”
  1. 00/14 pull: default warning improvementsFelipe Contreras, Dec 4, 2020
  2. 03/14 pull: refactor fast-forward checkFelipe Contreras, Dec 4, 2020
  3. 04/14 pull: cleanup autostash checkFelipe Contreras, Dec 4, 2020
  4. Elijah NewrenDec 4, 2020
  5. Felipe ContrerasDec 5, 2020
  6. Elijah NewrenDec 5, 2020
  7. 01/14 doc: pull: explain what is a fast-forwardFelipe Contreras, Dec 4, 2020
  8. Elijah NewrenDec 4, 2020
  9. Jacob KellerDec 5, 2020
  10. 02/14 pull: improve default warningFelipe Contreras, Dec 4, 2020
  11. Elijah NewrenDec 4, 2020
  12. Felipe ContrerasDec 5, 2020
  13. Elijah NewrenDec 5, 2020
  14. Felipe ContrerasDec 5, 2020
  15. Chris TorekDec 5, 2020
  16. Felipe ContrerasDec 5, 2020
  17. Elijah NewrenDec 5, 2020
  18. Felipe ContrerasDec 5, 2020
  19. Elijah NewrenDec 6, 2020
  20. Felipe ContrerasDec 6, 2020
  21. Junio C HamanoDec 7, 2020
  22. Felipe ContrerasDec 7, 2020
  23. Junio C HamanoDec 7, 2020
  24. Felipe ContrerasDec 7, 2020
  25. Junio C HamanoDec 7, 2020
  26. Felipe ContrerasDec 7, 2020
  27. Jacob KellerDec 7, 2020
  28. Junio C HamanoDec 8, 2020
  29. Felipe ContrerasDec 8, 2020
  30. Junio C HamanoDec 8, 2020
  31. Felipe ContrerasDec 9, 2020
  32. Elijah NewrenDec 9, 2020
  33. Felipe ContrerasDec 10, 2020
  34. Junio C HamanoDec 10, 2020
  35. Felipe ContrerasDec 10, 2020
  36. Felipe ContrerasDec 10, 2020
  37. Junio C HamanoDec 11, 2020
  38. Felipe ContrerasDec 11, 2020
  39. Junio C HamanoDec 14, 2020
  40. Felipe ContrerasDec 14, 2020
  41. Junio C HamanoDec 14, 2020
  42. Felipe ContrerasDec 15, 2020
  43. Jeff KingDec 15, 2020
  44. Felipe ContrerasDec 15, 2020
  45. Junio C HamanoDec 15, 2020
  46. Felipe ContrerasDec 15, 2020
  47. Felipe ContrerasDec 7, 2020
  48. 06/14 pull: move default warningFelipe Contreras, Dec 4, 2020
  49. Elijah NewrenDec 4, 2020
  50. Felipe ContrerasDec 4, 2020
  51. 05/14 pull: trivial cleanupFelipe Contreras, Dec 4, 2020
  52. Elijah NewrenDec 4, 2020
  53. Felipe ContrerasDec 5, 2020
  54. 08/14 pull: trivial whitespace style fixFelipe Contreras, Dec 4, 2020
  55. 10/14 pull: add proper error with --ff-onlyFelipe Contreras, Dec 4, 2020
  56. Elijah NewrenDec 4, 2020
  57. Felipe ContrerasDec 5, 2020
  58. 12/14 pull: show warning with --ffFelipe Contreras, Dec 4, 2020
  59. Elijah NewrenDec 4, 2020
  60. Felipe ContrerasDec 5, 2020
  61. 14/14 test: pull-options: revert unnecessary changesFelipe Contreras, Dec 4, 2020
  62. Elijah NewrenDec 4, 2020
  63. Felipe ContrerasDec 5, 2020
  64. 13/14 test: merge-pull-config: trivial cleanupFelipe Contreras, Dec 4, 2020
  65. Elijah NewrenDec 4, 2020
  66. 11/14 tentative: pull: change the semantics of --ff-onlyFelipe Contreras, Dec 4, 2020
  67. Elijah NewrenDec 4, 2020
  68. Felipe ContrerasDec 5, 2020
  69. experiment: pull: change --ff-only and default modeFelipe Contreras, Dec 5, 2020
  70. Elijah NewrenDec 5, 2020
  71. Felipe ContrerasDec 5, 2020
  72. Junio C HamanoDec 7, 2020
  73. Felipe ContrerasDec 5, 2020
  74. 07/14 pull: display default warning only when non-ffFelipe Contreras, Dec 4, 2020
  75. Elijah NewrenDec 4, 2020
  76. Felipe ContrerasDec 5, 2020
  77. 09/14 pull: introduce --merge optionFelipe Contreras, Dec 4, 2020
  78. Elijah NewrenDec 4, 2020
  79. Felipe ContrerasDec 5, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.