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

On Sun, May 31, 2015 at 4:18 PM, Paul Tan [off-list ref] wrote:

On Tue, May 19, 2015 at 9:04 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

Also, I wonder if something like this would do the job:
        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.

quoted

        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.

With all that said, after thinking about it I feel that this patch
series should focus solely on rewriting git-pull.sh 1:1. While I do
agree with the above suggested improvements, I think they should be
implemented as separated patch(es) on top of this series since we
would be technically changing git-pull's behavior, even if we are
improving it.

As such, the issue that I think should be focused on for this patch
is: does get_merge_branch_1() and get_merge_branch_2() in this patch
implement the same behavior as get_remote_merge_branch() in
git-parse.remote.sh? If it does, then its purpose is fulfilled.

So, I'll keep the overall logic of get_merge_branch_2() the same for
the next re-roll. (Other than renaming the function and fixing code
style issues). Once this series is okay, I'll look into doing a
separate patch on top that changes the function to use
remote_find_tracking() so that we fix the assumption that the default
fetch mapping is used.

The other possibility is that we fix this in git-parse-remote.sh, but
I'm personally getting a bit tired from having to re-implement the
same thing in shell script and C. Furthermore, the only script using
get_remote_merge_branch() is git-pull.sh.

[...]
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.
[...]

quoted

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."

Likewise, I won't introduce the error()s or warning()s for now.

Other than that, all other code style related issues have been/will be
fixed. Thanks for the reviews.

Regards,
Paul