Re: [RFC/PATCH v2] pull: add --set-upstream

[Erwan Mathoniere <erwan.mathoniere@grenoble-inp.org>]

On 06/06/2016 17:54, Matthieu Moy wrote:
> Erwan Mathoniere <erwan.mathoniere@grenoble-inp.org> writes:
>
>> @@ -497,6 +504,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
>>  		fprintf(stderr, "\n");
>>  		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
>>  		fprintf(stderr, "\n");
>> +		fprintf_ln(stderr, "    git pull -u %s %s", _("<remote>"), _("<branch>"));
>
> I'd rather use the long syntax "--set-upstream" in the advice. It gives
> a hint to the user about what the command is actually going to do.

You're right, I'll change it.

>> +static void set_pull_upstream(const char *repo, const char **refspecs_name)
>> +{
>> +	unsigned char sha1[GIT_SHA1_RAWSZ];
>
> The trend in the codebase is to use object_id instead of these char
> sha1[] arrays. See the output of "git log --grep object_id" for details.

I'll dig into it, thanks.

>> +	strbuf_init(&buf, 0);
>> +	refspec = parse_fetch_refspec(nr_refspec, refspecs_name);
>> +
>> +	for (i = 0; i < nr_refspec; i++) {
>> +		if (refspec[i].pattern) {
>> +			warning(_("upstream cannot be set for patterns"));
>> +			continue;
>> +		}
>> +
>> +		branch = branch_get(refspec[i].dst);
>> +		if (!branch || !ref_exists(branch->refname)) {
>> +			if (!refspec[i].dst || !*refspec[i].dst)
>> +				warning(_("could not set upstream of HEAD when "
>> +					"it does not point to any branch."));
>> +			else
>> +				warning(_("cannot set upstream: "
>> +					"'%s' is not a branch"), refspec[i].dst);
>
> Inconsistent message: "could not"/"cannot".

I copied/pasted the first warning from another portion of code in order 
to avoid useless translation work, but it isn't really relevant in that 
case.

> For this kind of code, it greatly helps to have comments refering to the
> input syntax for each branch of each conditionals. I'm not familiar with
> this part of the code, but if I understood correctly, the above would be
>
> if ()
> 	/* refspec: <branch>: */
>         warning(...);
> else
> 	/* refspec: <branch>:<no-such-branch> */
>         warning(...);

Good idea, some part aren't so easy to link to the input.

> I think it would make sense to actually raise an error (i.e. set the
> exit status of the "git pull" process to a non-zero value) when such
> issue occur. OTOH, "git push --set-upstream" does not even issue a
> warning when trying to --set-upstream to a tag for example, so not
> raising an error is consistant.

Since the idea is to have a similar behavior to `git push 
--set-upstream`, I think it's not relevant to raise an error when `git 
push` doesn't. And for a minor feature, it seems disproportionate to 
consider a `git pull -u ...` as a failure when the entire process 
succeeded except for the upstream part.

>>  int cmd_pull(int argc, const char **argv, const char *prefix)
>>  {
>> +	int ret;
>>  	const char *repo, **refspecs;
>>  	struct sha1_array merge_heads = SHA1_ARRAY_INIT;
>>  	unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
>> @@ -918,11 +1013,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>  	if (is_null_sha1(orig_head)) {
>>  		if (merge_heads.nr > 1)
>>  			die(_("Cannot merge multiple branches into empty head."));
>> -		return pull_into_void(*merge_heads.sha1, curr_head);
>> +		ret = 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);
>> +		ret = run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
>>  	} else
>> -		return run_merge();
>> +		ret = run_merge();
>> +
>> +	if (opt_set_upstream && ret < 128)
>
> Shouldn't this be "ret == 0"?

The feature activates after rebasing/merging. When there are merge 
conflicts, `run_merge` doesn't return a non-null error code and we want 
to set upstream even in that case.
On the other hand, when merge fails (e.g. histories aren't related) it 
stops itself by invoking `die` and returning a >= 128 error code. We 
don't want to proceed in that case.

>> +check_config () {
>
> Perhaps "check_upstream" is more expressive.

I took this procedure from t5523-push-upstream, but you're right it's 
more relevant.

>> +	(echo "$2"; echo "$3") >expect
>
> What happened to Junio's suggestion with test_write_lines?

Oups,  small oversight.

>> +test_expect_success 'setup repos' '
>> +	git init parent &&
>> +	(
>> +		cd parent &&
>> +		echo content >file &&
>> +		git add file &&
>> +		git commit -am one &&
>
> test_commit can make this code simpler.
>
>> +		echo content_modified >file &&
>> +		git commit -am "file modification"
>
> Likewise.
>

Thanks, I'll take a look at it.

>> +test_expect_success 'pull -u with a tag should not work' '
>> +	git checkout master &&
>> +	test_config_unchanged git pull -u upstream initial_tag
>> +'
>> +
>> +test_expect_success 'pull -u on detached head should not work' '
>> +	git checkout HEAD^0 &&
>> +	test_config_unchanged git pull -u upstream master2 &&
>> +	git checkout -
>> +'
>
> For all these "test_config_unchanged", it would be nice to check that
> the error message is the right one too.

Ok, I'll add checks on that.