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

> BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr> writes:
>
>> Adding the --set-upstream option to git pull/fetch
>
> We usually write commit messages with imperative tone, hence "add", not
> "adding".

>> +		/*
>> +		 * We want to set the current branch config following the 
>> +		 * ref_map entry which fetches on FETCH_HEAD
>
> fetches _to_? And period at end of sentence.

>> +		 * In case of "git pull <remote> --set-upstream" we
>> +		 * 	don't want to set all branches' config.
>> +		 * If there is no local ref which points on FETCH_HEAD
>
> Indentation is weird. If you're just writting sentences, just wrap the
> text 1 column away from the "*", and to make paragraphs, add blank lines
> (containing just "*") between paragraphs.

>> +		 * 	we don't set the config for the current branch
>> +		 * 	and warn the user.
>> +		 * If there is a fetch of more than one branch for example: 
>> +		 * 	"git pull <remote> <branch> <branch> --set-upstream"
>> +		 *	setting the current branch's config makes no sense.
>> +		 * Where we are in case of "git pull <remote> <branch>:<branch>" we
>> +		 * 	don't want to set the config for the local branch
>> +		 * 	can be improved in the future to set local branch's config.
>> +		 */
>
> I'm biaised because we talked about this in real-life, but I find the
> explanation unclear. I'd write stg like
> /*
> * We're setting the upstream configuration for the current branch. The
> * relevant upstream is the fetched branch that is meant to be merged with
> * the current one, i.e. the one fetched to FETCH_HEAD.
> * 
> * When there are several such branches, consider the request ambiguous and
> * err on the safe side by doing nothing and just emit a warning.
> */
>
> I think the discussion about the various use-case that may lead to
> different cases (0, 1 or >1 branches fetched to FETCH_HEAD) is not
> needed here, but can be relevant comments in the tests.

>> +		for (rm = ref_map; rm; rm = rm->next) {
>> +			fprintf(stderr, "\n -%s", rm->name);
>> +			if (rm->peer_ref) {
>> +				fprintf(stderr, " -> %s", rm->peer_ref->name);
>> +			} else {
>> +				if (target) {
>> +					fprintf(stderr, " -> FETCH_HEAD\n");
>> +					warning(_("Multiple FETCH_HEAD"));
>
> Is this a debug statement or a real warning? In the later case, it
> should be made clearer to the user.

>> +					target = NULL;
>> +					break;
>> +				} else {
>> +					target = rm;
>
> This is the branch you're fetching from, right? If so, "target" is a
> misleading name. Perhaps source_ref?

>> +					fprintf(stderr, " -> FETCH_HEAD");
>> +				}
>> +			}
>> +		}
>> +		fprintf(stderr, "\n\n");
>> +		if (target) {
>> +			if (!strcmp(ref_map->name, "HEAD") ||
>> +					starts_with(ref_map->name, "refs/heads/")) {
>
> Weird indentation. Perhaps you have a tab-width != 8?

>> +				install_branch_config(0, branch->name,
>> +							 transport->remote->name,
>> +							 target->name);
>> +			} else if (starts_with(ref_map->name, "refs/remotes/")) {
>> +				warning(_("Not setting upstream for a remote remote-tracking branch"));
>> +			} else if (starts_with(ref_map->name, "refs/tags/")) {
>> +				warning(_("Tag upstream not set"));
>> +			} else {
>> +				warning(_("Unknown branch type"));
>> +			}
>> +		} else {
>> +			warning(_("Fetching more than one branch. Current branch's upstream not set"));
>
> The warning seems misleading to me: this else branch is executed in many
> cases (described in the comment above), not only when there's more than
> one branch, right?

>> --- /dev/null
>> +++ b/t/t5553-set-upstream.sh
>> @@ -0,0 +1,141 @@
>> +#!/bin/sh
>> +
>> +test_description='"git fetch/pull --set-upstream" basic tests.
>> +
>> +'
>> +. ./test-lib.sh
>> +
>> +
>> +
>> +check_config() {
>> +	(echo $2; echo $3) >expect.$1
>> +	(git config branch.$1.remote
>> +	 git config branch.$1.merge) >actual.$1
>> +	test_cmp expect.$1 actual.$1
>> +}
>> +
>> +check_config_empty() {
>> +	git config branch.$1.remote >remote.$1
>> +	test_must_be_empty remote.$1
>> +	git config branch.$1.merge >merge.$1
>> +	test_must_be_empty merge.$1
>> +}
>
> Broken &&-chain (in both functions, but most importantly in the second,
> where the first test_must_be_empty is useless without &&.

>> +test_expect_success 'fetch --set-upstream does not set branch other' '
>
> Misleading test name: "set branch" -> "set upstream"? And here it's not
> just about "other" but about all branches.
>
> 'fetch --set-upstream does not set upstream w/o branch'
> ?

>> +	git checkout master &&
>> +	git fetch --set-upstream upstream &&
>> +	check_config_empty master &&
>> +	check_config_empty other
>> +'
>
>> +#test_expect_success 'fetch --set-upstream does not set branch other' '
>> +#	git checkout master &&
>> +#	git fetch --set-upstream upstream &&
>> +#	check_config master upstream refs/heads/master &&
>> +#	check_config_empty other
>> +#'
>
> Avoid leaving leftovers like this, even in WIP patches, they distract
> the reader.

>> +test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
>> +	git fetch --set-upstream upstream master &&
>> +	check_config master upstream refs/heads/master &&
>> +	check_config_empty other
>> +'
>> +
>> +
>
> Style: you sometimes leave 2 blank lines, sometimes 1 between tests. Try
> to be consistent.

>> +test_expect_success 'pull --set-upstream upstream other sets branch other' '
>
> Test title and content say the opposite of each other.
>
>> +	git pull --set-upstream upstream other &&
>> +	check_config master upstream refs/heads/other &&
>> +	check_config_empty other
>> +'

>> +test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
>> +	test_must_fail git pull --set-upstream http://nosuchdomain.example.com
>> +'
>
> You should check that it doesn't touch the config. That it fails is not
> a surprise regardless of the correctness of your code, but the thing to
> check is that it does not touch the config before failing.