Re: [PATCH v2 15/19] pull: teach git pull about --rebase

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 10, 2015, 15:35 UTC
Message-ID: <CACRoPnSgVPVDTC=nY6FQHoVRPF1HZhRYXaKoQ1oJrJCoG1W8xg@mail.gmail.com>
In-Reply-To: <xmqqmw07oc72.fsf@gitster.dls.corp.google.com>

On Wed, Jun 10, 2015 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Paul Tan <pyokagan@gmail.com> writes:
>
>>> Hmph, it is somewhat surprising that we do not have such a helper
>>> already. Wouldn't we need this logic to implement $branch@{upstream}
>>> syntax?
>>
>> Right, the @{upstream} syntax is implemented by branch_get_upstream()
>> in remote.c. It, however, does not check to see if the branch's remote
>> matches what is provided on the command-line, so we still have to
>> implement this check ourselves, which means this helper function is
>> still required.
>>
>> I guess we could still use branch_get_upstream() in this function though.
>
> It is entirely expected that existing function may not do exactly
> what the new caller you introduce might want to do, or may do more
> than what it wants.  That is where refactoring of existing code
> comes in.
>
> It somewhat feels strange that you have to write more than "shim"
> code to glue existing helpers and API functions together to
> re-implement what a scripted Porcelain is already doing, though.
> It can't be that git-pull.sh implements this logic as shell script,
> and it must be asking existing code in Git to do what the callers
> you added for this function would want to do, right?

Not git-pull.sh, but get_remote_merge_branch() git-parse-remote.sh. The shell code that get_upstream_branch() in this patch implements is:

    0|1)
        origin="$1"
        default=$(get_default_remote)
        test -z "$origin" && origin=$default
        curr_branch=$(git symbolic-ref -q HEAD) &&
        [ "$origin" = "$default" ] &&

^ This here is where it checks to see if the branch's configured remote matches the remote provided on the command line.

        echo $(git for-each-ref --format='%(upstream)' $curr_branch)
        ;;

^ While here it calls git to get the upstream branch, which is implemented by branch_get_upstream() on the C side.

So yes, we can use branch_get_upstream(), but we still need to implement some code on top.

Just to add on, the shell code that get_tracking_branch() in this patch implements is:

    *)
        repo=$1
        shift
        ref=$1
        # FIXME: It should return the tracking branch
        #        Currently only works with the default mapping
        case "$ref" in
        +*)
        ref=$(expr "z$ref" : 'z+\(.*\)')
        ;;
        esac
        expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
        remote=$(expr "z$ref" : 'z\([^:]*\):')
        case "$remote" in
        '' | HEAD ) remote=HEAD ;;
        heads/*) remote=${remote#heads/} ;;
        refs/heads/*) remote=${remote#refs/heads/} ;;
        refs/* | tags/* | remotes/* ) remote=
        esac
        [ -n "$remote" ] && case "$repo" in
        .)
            echo "refs/heads/$remote"
            ;;
        *)
            echo "refs/remotes/$repo/$remote"
            ;;
        esac

so it's more or less a direct translation of the shell script, and we can be sure it will have the same behavior. I'm definitely in favor of switching this to use remote_find_tracking(), the question is whether we want to do it in this patch or in a future patch on top.

Thanks, Paul