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

Re: [PATCH 11/14] pull: teach git pull about --rebase

From
Stefan Beller <sbeller@google.com>
Date
May 18, 2015, 23:36 UTC
Message-ID
<CAGZ79kaY6vr6EqRs7DzZnSBDVhB0oW4=LRg8+cGqKzm45aVypg@mail.gmail.com>
In-Reply-To
<1431961571-20370-12-git-send-email-pyokagan@gmail.com>
On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 20 quoted lines
> +enum rebase_type {
> +       REBASE_FALSE,
> +       REBASE_TRUE,
> +       REBASE_PRESERVE
> +};
> +
> +/**
> + * Parses the value of --rebase, branch.*.rebase or pull.rebase. If value is a
> + * false value, returns REBASE_FALSE. If value is a true value, returns
> + * REBASE_TRUE. If value is "preserve", returns REBASE_PRESERVE. Otherwise,
> + * returns -1 to signify an invalid value.
> + */
> +static int parse_config_rebase(const char *value)
> +{
> +       int v = git_config_maybe_bool("pull.rebase", value);
> +       if (v >= 0)
> +               return v;
> +       if (!strcmp(value, "preserve"))
> +               return REBASE_PRESERVE;
> +       return -1;

There is no REBASE_FALSE nor REBASE_TRUE in this function, how do you make sure it returns the correct value then?

Currently this is probably in sync with what git_config_maybe_bool returns, but think of a future refactoring/change. Then it will be hard to have the connection between what the code in git_config_maybe_bool does, what the code here does and the comment here.

You could get rid of the enum all together, as you're using integer as a return type anyway. (so you lose type safety, a future change may introduce a "return 42" which is not part of the enum)

Or you could convert the code to use the enum more strictly and having it as the return type:

enum rebase_type parse_config_rebase(const char *value) {
    int v = git_config_maybe_bool("pull.rebase", value);
    switch (v) {
        case 0:
            return REBASE_FALSE;
        case 1:
            return REBASE_TRUE;
        default:
             if (!strcmp(value, "preserve"))
                return  REBASE_PRESERVE;
             else {
                die("value pull.rebase must be one of {false, true, preserve}");
                // or if you're more tolerant:
                return REBASE_VALUE_UNINTERPRETABLE;
       }
   }
Show 14 quoted lines
> +}
> +
> +/**
> + * Callback for --rebase, which parses arg with parse_config_rebase().
> + */
> +static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
> +{
> +       int *value = (int*) opt->value;
> +
> +       if (arg)
> +               *value = parse_config_rebase(arg);
> +       else
> +               *value = unset ? REBASE_FALSE : REBASE_TRUE;
> +       return *value >= 0 ? 0 : -1;

This is repeating the check from above. If you'd go the way of using stricter enums, you'd just check for the REBASE_VALUE_UNINTERPRETABLE value here.

Show 53 quoted lines
> +}
> +
>  static const char * const pull_usage[] = {
>         N_("git pull [options] [<repo> [<refspec>...]]"),
>         NULL
> @@ -48,7 +84,8 @@ static const char * const pull_usage[] = {
>  static int opt_verbosity;
>  static char *opt_progress;
>
> -/* Options passed to git-merge */
> +/* Options passed to git-merge or git-rebase */
> +static int opt_rebase;
>  static char *opt_diffstat;
>  static char *opt_log;
>  static char *opt_squash;
> @@ -82,8 +119,12 @@ static struct option pull_options[] = {
>           N_("force progress reporting"),
>           PARSE_OPT_NOARG, parse_opt_passthru},
>
> -       /* Options passed to git-merge */
> +       /* Options passed to git-merge or git-rebase */
>         OPT_GROUP(N_("Options related to merging")),
> +       { OPTION_CALLBACK, 'r', "rebase", &opt_rebase,
> +         N_("false|true|preserve"),
> +         N_("incorporate changes by rebasing rather than merging"),
> +         PARSE_OPT_OPTARG, parse_opt_rebase, 0},
>         { OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL,
>           N_("do not show a diffstat at the end of the merge"),
>           PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_passthru, (intptr_t) "no-stat" },
> @@ -506,11 +547,185 @@ static int run_merge(void)
>         return ret;
>  }
>
> +/**
> + * Returns the merge branch for the current branch. Returns NULL if repo is not
> + * a valid remote, HEAD does not point to a branch, repo is not the branch's
> + * configured remote or the branch does not have any configured merge branch.
> + */
> +static char *get_merge_branch_1(const char *repo)
> +{
> +       struct remote *rm;
> +       struct branch *curr_branch;
> +
> +       if (repo && !(rm = remote_get(repo)))
> +               return NULL;
> +       if (!(curr_branch = branch_get("HEAD")))
> +               return NULL;
> +       if (repo && curr_branch->remote != rm)
> +               return NULL;
> +       if (!curr_branch->merge_nr)
> +               return NULL;
> +       return xstrdup(curr_branch->merge[0]->dst);
> +}

This feels a bit tangled because of the repetition of checking for repo and curr_branch. Also we do not do assignments inside of conditions (if, while, for), so maybe start with a

    if (!repo)
        return NULL;
and then do the assignments and test on the assigned value.
Show 351 quoted lines
> +
> +/**
> + * Given a refspec, returns the merge branch. Returns NULL if the refspec src
> + * does not refer to a branch.
> + *
> + * FIXME: It should return the tracking branch. Currently only works with the
> + * default mapping.
> + */
> +static char *get_merge_branch_2(const char *repo, const char *refspec)
> +{
> +       struct refspec *spec;
> +       const char *remote;
> +       char *merge_branch;
> +
> +       spec = parse_fetch_refspec(1, &refspec);
> +       remote = spec->src;
> +       if (!*remote || !strcmp(remote, "HEAD"))
> +               remote = "HEAD";
> +       else if (skip_prefix(remote, "heads/", &remote))
> +               ;
> +       else if (skip_prefix(remote, "refs/heads/", &remote))
> +               ;
> +       else if (starts_with(remote, "refs/") ||
> +               starts_with(remote, "tags/") ||
> +               starts_with(remote, "remotes/"))
> +               remote = "";
> +
> +       if (*remote) {
> +               if (!strcmp(repo, "."))
> +                       merge_branch = mkpathdup("refs/heads/%s", remote);
> +               else
> +                       merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
> +       } else
> +               merge_branch = NULL;
> +
> +       free_refspec(1, spec);
> +       return merge_branch;
> +}
> +
> +/**
> + * Sets fork_point to the point at which the current branch forked from its
> + * remote merge branch. Returns 0 on success, -1 on failure.
> + */
> +static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
> +               const char *repo, const char *refspec)
> +{
> +       int ret;
> +       struct branch *curr_branch;
> +       char *remote_merge_branch;
> +       struct argv_array args = ARGV_ARRAY_INIT;
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       if (!(curr_branch = branch_get("HEAD")))
> +               return -1;
> +
> +       if (refspec)
> +               remote_merge_branch = get_merge_branch_2(repo, refspec);
> +       else
> +               remote_merge_branch = get_merge_branch_1(repo);
> +
> +       if (!remote_merge_branch)
> +               return -1;
> +
> +       argv_array_pushl(&args, "merge-base", "--fork-point",
> +                       remote_merge_branch, curr_branch->name, NULL);
> +       cp.argv = args.argv;
> +       cp.no_stdin = 1;
> +       cp.no_stderr = 1;
> +       cp.git_cmd = 1;
> +
> +       if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
> +               goto cleanup;
> +
> +       if ((ret = get_sha1_hex(sb.buf, fork_point)))
> +               goto cleanup;
> +
> +cleanup:
> +       free(remote_merge_branch);
> +       strbuf_release(&sb);
> +       return ret ? -1 : 0;
> +}
> +
> +/**
> + * Sets merge_base to the octopus merge base of curr_head, merge_head and
> + * fork_point. Returns 0 if a merge base is found, 1 otherwise.
> + */
> +static int get_octopus_merge_base(unsigned char merge_base[GIT_SHA1_HEXSZ],
> +               unsigned char curr_head[GIT_SHA1_RAWSZ],
> +               unsigned char merge_head[GIT_SHA1_RAWSZ],
> +               unsigned char fork_point[GIT_SHA1_RAWSZ])
> +{
> +       struct commit_list *revs = NULL, *result;
> +
> +       commit_list_insert(lookup_commit_reference(curr_head), &revs);
> +       commit_list_insert(lookup_commit_reference(merge_head), &revs);
> +       if (!is_null_sha1(fork_point))
> +               commit_list_insert(lookup_commit_reference(fork_point), &revs);
> +
> +       result = reduce_heads(get_octopus_merge_bases(revs));
> +       free_commit_list(revs);
> +       if (!result)
> +               return 1;
> +
> +       hashcpy(merge_base, result->item->object.sha1);
> +       return 0;
> +}
> +
> +/**
> + * Given the current HEAD SHA1, the merge head returned from git-fetch and the
> + * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
> + * appropriate arguments and returns its exit status.
> + */
> +static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
> +               unsigned char merge_head[GIT_SHA1_RAWSZ],
> +               unsigned char fork_point[GIT_SHA1_RAWSZ])
> +{
> +       int ret;
> +       unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
> +       struct argv_array args = ARGV_ARRAY_INIT;
> +
> +       if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head, fork_point))
> +               if (!is_null_sha1(fork_point) && !hashcmp(oct_merge_base, fork_point))
> +                       hashclr(fork_point);
> +
> +       argv_array_push(&args, "rebase");
> +
> +       /* Shared options */
> +       argv_push_verbosity(&args);
> +
> +       /* Options passed to git-rebase */
> +       if (opt_rebase == REBASE_PRESERVE)
> +               argv_array_push(&args, "--preserve-merges");
> +       if (opt_diffstat)
> +               argv_array_push(&args, opt_diffstat);
> +       argv_push_strategies(&args);
> +       argv_push_strategy_opts(&args);
> +       if (opt_gpg_sign)
> +               argv_array_push(&args, opt_gpg_sign);
> +
> +       argv_array_push(&args, "--onto");
> +       argv_array_push(&args, sha1_to_hex(merge_head));
> +
> +       if (!is_null_sha1(fork_point))
> +               argv_array_push(&args, sha1_to_hex(fork_point));
> +       else
> +               argv_array_push(&args, sha1_to_hex(merge_head));
> +
> +       ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
> +       argv_array_clear(&args);
> +       return ret;
> +}
> +
>  int cmd_pull(int argc, const char **argv, const char *prefix)
>  {
>         const char *repo, **refspecs;
>         struct sha1_array merge_heads = SHA1_ARRAY_INIT;
>         unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
> +       unsigned char rebase_fork_point[GIT_SHA1_RAWSZ];
>
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
> @@ -532,6 +747,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (get_sha1("HEAD", orig_head))
>                 hashclr(orig_head);
>
> +       if (opt_rebase)
> +               if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
> +                       hashclr(rebase_fork_point);
> +
>         if (run_fetch(repo, refspecs))
>                 return 1;
>
> @@ -573,6 +792,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 if (merge_heads.nr > 1)
>                         die(_("Cannot merge multiple branches into empty head."));
>                 return pull_into_void(*merge_heads.sha1, curr_head);
> +       } else if (opt_rebase) {
> +               if (merge_heads.nr > 1)
> +                       die(_("Cannot rebase onto multiple branches."));
> +               return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
>         } else
>                 return run_merge();
>  }
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9414cc1..3798b96 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -212,7 +212,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' '
>         test "$(git rev-parse third)" = "$(git rev-parse second)"
>  '
>
> -test_expect_failure '--rebase' '
> +test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
>         git commit -m file file &&
> @@ -226,7 +226,7 @@ test_expect_failure '--rebase' '
>         test new = "$(git show HEAD:file2)"
>  '
>
> -test_expect_failure '--rebase fails with multiple branches' '
> +test_expect_success '--rebase fails with multiple branches' '
>         git reset --hard before-rebase &&
>         test_must_fail git pull --rebase . copy master 2>err &&
>         test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
> @@ -310,7 +310,7 @@ test_expect_failure 'pull.rebase=invalid fails' '
>         ! git pull . copy
>  '
>
> -test_expect_failure '--rebase=false create a new merge commit' '
> +test_expect_success '--rebase=false create a new merge commit' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase true &&
>         git pull --rebase=false . copy &&
> @@ -319,7 +319,7 @@ test_expect_failure '--rebase=false create a new merge commit' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase=true rebases and flattens keep-merge' '
> +test_expect_success '--rebase=true rebases and flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase preserve &&
>         git pull --rebase=true . copy &&
> @@ -327,7 +327,7 @@ test_expect_failure '--rebase=true rebases and flattens keep-merge' '
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase=preserve rebases and merges keep-merge' '
> +test_expect_success '--rebase=preserve rebases and merges keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase true &&
>         git pull --rebase=preserve . copy &&
> @@ -340,7 +340,7 @@ test_expect_success '--rebase=invalid fails' '
>         ! git pull --rebase=invalid . copy
>  '
>
> -test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
> +test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' '
>         git reset --hard before-preserve-rebase &&
>         test_config pull.rebase preserve &&
>         git pull --rebase . copy &&
> @@ -348,7 +348,7 @@ test_expect_failure '--rebase overrides pull.rebase=preserve and flattens keep-m
>         test file3 = "$(git show HEAD:file3.t)"
>  '
>
> -test_expect_failure '--rebase with rebased upstream' '
> +test_expect_success '--rebase with rebased upstream' '
>
>         git remote add -f me . &&
>         git checkout copy &&
> @@ -366,7 +366,7 @@ test_expect_failure '--rebase with rebased upstream' '
>
>  '
>
> -test_expect_failure '--rebase with rebased default upstream' '
> +test_expect_success '--rebase with rebased default upstream' '
>
>         git update-ref refs/remotes/me/copy copy-orig &&
>         git checkout --track -b to-rebase2 me/copy &&
> @@ -377,7 +377,7 @@ test_expect_failure '--rebase with rebased default upstream' '
>
>  '
>
> -test_expect_failure 'rebased upstream + fetch + pull --rebase' '
> +test_expect_success 'rebased upstream + fetch + pull --rebase' '
>
>         git update-ref refs/remotes/me/copy copy-orig &&
>         git reset --hard to-rebase-orig &&
> @@ -409,7 +409,7 @@ test_expect_failure 'pull --rebase dies early with dirty working directory' '
>
>  '
>
> -test_expect_failure 'pull --rebase works on branch yet to be born' '
> +test_expect_success 'pull --rebase works on branch yet to be born' '
>         git rev-parse master >expect &&
>         mkdir empty_repo &&
>         (cd empty_repo &&
> @@ -456,14 +456,14 @@ test_expect_success 'setup for detecting upstreamed changes' '
>         )
>  '
>
> -test_expect_failure 'git pull --rebase detects upstreamed changes' '
> +test_expect_success 'git pull --rebase detects upstreamed changes' '
>         (cd dst &&
>          git pull --rebase &&
>          test -z "$(git ls-files -u)"
>         )
>  '
>
> -test_expect_failure 'setup for avoiding reapplying old patches' '
> +test_expect_success 'setup for avoiding reapplying old patches' '
>         (cd dst &&
>          test_might_fail git rebase --abort &&
>          git reset --hard origin/master
> @@ -485,7 +485,7 @@ test_expect_failure 'setup for avoiding reapplying old patches' '
>         )
>  '
>
> -test_expect_failure 'git pull --rebase does not reapply old patches' '
> +test_expect_success 'git pull --rebase does not reapply old patches' '
>         (cd dst &&
>          test_must_fail git pull --rebase &&
>          test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 4176e11..56e7377 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -19,7 +19,7 @@ test_expect_success 'git pull -q' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull -q --rebase' '
> +test_expect_success 'git pull -q --rebase' '
>         mkdir clonedqrb &&
>         (cd clonedqrb && git init &&
>         git pull -q --rebase "../parent" >out 2>err &&
> @@ -38,7 +38,7 @@ test_expect_success 'git pull' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull --rebase' '
> +test_expect_success 'git pull --rebase' '
>         mkdir clonedrb &&
>         (cd clonedrb && git init &&
>         git pull --rebase "../parent" >out 2>err &&
> @@ -54,7 +54,7 @@ test_expect_success 'git pull -v' '
>         test_must_be_empty out)
>  '
>
> -test_expect_failure 'git pull -v --rebase' '
> +test_expect_success 'git pull -v --rebase' '
>         mkdir clonedvrb &&
>         (cd clonedvrb && git init &&
>         git pull -v --rebase "../parent" >out 2>err &&
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 1ffd837..dda3929 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -11,8 +11,6 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
>
>  . ./test-lib.sh
>
> -skip_all='skipping submodule update tests, requires git pull --rebase'
> -test_done
>
>  compare_head()
>  {
> --
> 2.1.4
>
Previous: Paul TanNext: Johannes Schindelin
Message 26 of 43 in “Make git-pull a builtin”
  1. 00/14 Make git-pull a builtinPaul Tan, May 18, 2015
  2. 01/14 pull: implement fetch + mergePaul Tan, May 18, 2015
  3. Johannes SchindelinMay 18, 2015
  4. 02/14 pull: pass verbosity, --progress flags to fetch and mergePaul Tan, May 18, 2015
  5. Johannes SchindelinMay 18, 2015
  6. Paul TanMay 21, 2015
  7. Johannes SchindelinMay 21, 2015
  8. Paul TanMay 22, 2015
  9. 03/14 pull: pass git-merge's options to git-mergePaul Tan, May 18, 2015
  10. 04/14 pull: pass git-fetch's options to git-fetchPaul Tan, May 18, 2015
  11. 05/14 pull: error on no merge candidatesPaul Tan, May 18, 2015
  12. Johannes SchindelinMay 18, 2015
  13. 06/14 pull: support pull.ff configPaul Tan, May 18, 2015
  14. Johannes SchindelinMay 18, 2015
  15. Paul TanMay 21, 2015
  16. 07/14 pull: check if in unresolved merge statePaul Tan, May 18, 2015
  17. Johannes SchindelinMay 18, 2015
  18. 08/14 pull: fast-forward working tree if head is updatedPaul Tan, May 18, 2015
  19. Johannes SchindelinMay 18, 2015
  20. 09/14 pull: implement pulling into an unborn branchPaul Tan, May 18, 2015
  21. 10/14 pull: set reflog messagePaul Tan, May 18, 2015
  22. Johannes SchindelinMay 18, 2015
  23. Junio C HamanoMay 18, 2015
  24. Paul TanMay 21, 2015
  25. 11/14 pull: teach git pull about --rebasePaul Tan, May 18, 2015
  26. Stefan BellerMay 18, 2015
  27. Johannes SchindelinMay 19, 2015
  28. Paul TanMay 31, 2015
  29. Paul TanJun 2, 2015
  30. 12/14 pull: configure --rebase via branch.<name>.rebase or pull.rebasePaul Tan, May 18, 2015
  31. Stefan BellerMay 18, 2015
  32. 13/14 pull --rebase: exit early when the working directory is dirtyPaul Tan, May 18, 2015
  33. 14/14 pull --rebase: error on no merge candidate casesPaul Tan, May 18, 2015
  34. Stefan BellerMay 19, 2015
  35. Johannes SchindelinMay 19, 2015
  36. Junio C HamanoMay 19, 2015
  37. Paul TanMay 22, 2015
  38. Johannes SchindelinMay 22, 2015
  39. Stefan BellerMay 22, 2015
  40. Junio C HamanoMay 18, 2015
  41. Paul TanMay 30, 2015
  42. Paul TanMay 30, 2015
  43. Johannes SchindelinMay 18, 2015

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.