Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

From: Matthieu Moy <matthieu.moy@univ-lyon1.fr>
Date: Apr 18, 2019, 09:51 UTC
Message-ID: <86h8av7ian.fsf@univ-lyon1.fr>
In-Reply-To: <36559daca9d84f7a91933add734020cd@BPMBX2013-01.univ-lyon1.fr>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -165,6 +165,11 @@ ifndef::git-pull[]
>>  	Disable recursive fetching of submodules (this has the same effect as
>>  	using the `--recurse-submodules=no` option).
>>  
>> +--set-upstream::
>> +	If the new URL remote is correct, pull and add upstream (tracking) 
>> +	reference, used by argument-less linkgit:git-push[1] and other commands.
>
> git-push and other commands?

I think this is taken from the documentation of --set-upstream for push, which says:

-u::
--set-upstream::
	For every branch that is up to date or successfully pushed, add
	upstream (tracking) reference, used by argument-less
	linkgit:git-pull[1] and other commands. For more information,
	see `branch.<name>.merge` in linkgit:git-config[1].

Probably the reasoning was to make a symmetry between "git push --set-upstream", which mentions "pull" in the doc, and the new "git pull --set-upstream". However, I do not think there should be such symmetry:

Actually, the way I see it, the notion of uptream (i.e. branch.<branch>.remote and branch.<branch>.merge) is primarily about "pull" and friends, and "push" happens to use it also by default. But when branch.<branch>.pushRemote is set, upstream is really about pulling, and pushing goes to the pushRemote.

>> +	/* TODO: remove debug trace */
>
> Perhaps do so before sending it out for the review?

Yes. This is WIP for now, but it's time to get closer to a real patch, and these debug statements are counter-productive for that.

Show 6 quoted lines

>> +	test_must_be_empty merge.$1
>> +}
>
> If this wanted to say "It is OK for the variable to be missing, and
> it also is OK for the variable to have an empty string as its value;
> all other cases are unacceptable",

Actually, I don't think the "present but empty" case makes sense here, so just test_must_fail git config "$1" should do the trick.

I agree with all other remarks.

-- 
Matthieu Moy
https://matthieu-moy.fr/