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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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.

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

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

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(...);

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.

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

> --- /dev/null
> +++ b/t/t5544-pull-upstream.sh
> @@ -0,0 +1,164 @@
> +#!/bin/sh
> +
> +test_description='pull with --set-upstream'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-terminal.sh
> +
> +test_config_unchanged () {
> +	git config --list --local >original
> +	"$@"
> +	git config --list --local >modified
> +	test_cmp original modified
> +}

The test passes if "$@" fails. You should &&-chain the lines here to
catch things like crashes or unexpected "exit 1" in git.

> +check_config () {

Perhaps "check_upstream" is more expressive.

> +	(echo "$2"; echo "$3") >expect

What happened to Junio's suggestion with test_write_lines?

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

> +test_expect_success 'pull -u master' '
> +	git pull -u upstream master &&
> +	check_config master upstream refs/heads/master
> +'
> +
> +test_expect_success 'pull -u takes the last branch as upstream' '
> +	test_might_fail git config --unset branch.master.merge &&
> +	test_might_fail git config --unset branch.master.remote &&
> +	git pull -u upstream master master2 &&
> +	check_config master upstream refs/heads/master2
> +'
> +
> +test_expect_success 'pull -u master:other' '
> +	git pull -u upstream master:other &&
> +	check_config other upstream refs/heads/master
> +'
> +
> +

Nit: two blank lines instead of one.

> +test_expect_success 'pull -u refs/heads/*:refs/remotes/origin/* should not work' '
> +	git checkout master &&
> +	test_config_unchanged git pull -u upstream "refs/heads/*:refs/remotes/upstream/*"
> +'
> +
> +test_expect_success 'pull -u master:refs/remotes/origin/master should not work' '
> +	test_config_unchanged git pull -u upstream master:refs/remotes/upstream/master
> +'
> +
> +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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/