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

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

From
Paul Tan <pyokagan@gmail.com>
Date
May 31, 2015, 08:18 UTC
Message-ID
<CACRoPnRvjyjtbdT4CY7f4kFqq1rTiksp7eUnpFza+h8ZAq-4gg@mail.gmail.com>
In-Reply-To
<ea07c7ecec761a0bb2d9f4936a8d2411@www.dscho.org>
Hi Johannes,

On Tue, May 19, 2015 at 9:04 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 46 quoted lines
> On 2015-05-18 17:06, Paul Tan wrote:
>> +/**
>> + * 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;
>> +}
>
> I have to admit that it took me a substantial amount of time to deduce from the code what `get_merge_branch_2()` really does (judging from the description, I thought that it would be essentially the same as `get_merge_branch_1()` without the hard-coded "HEAD"). If the comment above the function would have said something like:
>
> /**
>  * Given a refspec, returns the name of the local tracking ref.
>  */
>
> I would have had an easier time. Also, I wonder if something like this would do the job:

Yeah whoops, this came from a confusion over the difference over "merge branch" and "remote tracking branch". A merge branch would be a remote tracking branch, but a remote tracking branch is not necessarily a merge branch.

>         spec = parse_fetch_refspec(1, &refspec);
>         if (spec->dst)
>                 return spec->dst;

Hmm, I notice that get_remote_merge_branch() only looks at the src part of the refspec. However, I guess it is true that if the dst part is provided, the user may be wishing for that to be interpreted as the "remote tracking branch", so we should be looking at it to calculate the fork point.

>         if (!(remote = get_remote(remote_name)))
>                 return NULL;
>         if (remote_find_tracking(remote, spec))
>                 return spec->dst;

... and if the dst part of the refspec is not provided, we fall back to see if there is any remote tracking branch in the repo for the src part of the ref, which matches the intention of get_remote_merge_branch() I think, while being better because remote_find_tracking() takes into account the actual configured fetch refspecs for the remote.

However, we also need to consider if the user provided a wildcard refspec, as it will not make sense in this case. From my reading, remote_find_tracking(), which calls query_refspecs(), would just match the src part literally, so I guess we should explicitly detect and error out in this case.

>         return NULL;
>
> (I guess we'd have to `xstrdup()` the return values because the return value of `get_merge_branch_1()` needs to be `free()`d, but maybe we can avoid so much `malloc()/free()`ing.) Again, the function name should probably be changed to something clearer, maybe to `get_tracking_branch()`.

Yeah, it should be changed to get_tracking_branch(), since it is different from get_merge_branch().

Show 5 quoted lines
> One thing that is not clear at all to me is whether
>
>         git pull --rebase origin master next
>
> would error out as expected, or simply rebase to `origin/master`.

In git-pull.sh, for the rebase fork point calculation it just used the first refspec. However, when running git-rebase it checked to see if there was only one merge base, which is replicated here:

    @@ -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();
     }

Since this is just a 1:1 rewrite I just tried to keep as close to the original as possible. However, thinking about it, since we *are* just using the first refspec for fork point calculation, I do agree that we should probably give an warning() here as well if the user provided more than one refspec, like "Cannot calculate rebase fork point as you provided more than one refspec. git-pull will not be able to handle a rebased upstream". I do not think it is fatal enough that we should error() or die(), as e.g. the first refspec may be a wildcard refspec that matches nothing, and the second refspec that matches one merge head for rebasing. This is pretty contrived though, but still technically legitimate. I dunno.

Show 26 quoted lines
>> +/**
>> + * 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;
>
> We should probably `return error(_"No tracking branch found for %s@, refspec ? refspec : "HEAD");` so that the user has a chance to understand that there has been a problem and how to solve it.

Just like the above, I don't think this is serious enough to be considered an error() though. Sure, this means that we cannot properly handle the case where the upstream has been rebased, but this is not always the case. We could probably have a warning() here, but I think the message should be improved to tell the user what exactly she is losing out on. e.g. "No tracking branch found for %s. git-pull will not be able to handle a rebased upstream."

Show 5 quoted lines
>> +     argv_array_pushl(&args, "merge-base", "--fork-point",
>> +                     remote_merge_branch, curr_branch->name, NULL);
>> +     cp.argv = args.argv;
>
> Let's just use `cp.args` directly...
Yeah, whoops.
Show 34 quoted lines
>> +     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;
>> +}
>> +
>> +[...]
>> +/**
>> + * 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))
>
> It might be my mail program only that mangled the diff here. But it could also be that this line is a little long (by my count, it is 81 columns wide).
Yeah, it is, but I felt that breaking the line would reduce readability.
Thanks, these were some really hard questions that you raised ;-).

Regards, Paul

Previous: Johannes SchindelinNext: Paul Tan
Message 28 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.