Re: [PATCH v3 5/8] push: add an advice on unqualified <dst> push

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2018, 05:14 UTC
Message-ID: <xmqqva5l49np.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181026230741.23321-6-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 13 quoted lines

> +	if (!advice_push_unqualified_ref_name)
> +		return;
> +
> +	if (get_oid(matched_src_name, &oid))
> +		BUG("'%s' is not a valid object, "
> +		    "match_explicit_lhs() should catch this!",
> +		    matched_src_name);
> +	type = oid_object_info(the_repository, &oid, NULL);
> +	if (type == OBJ_COMMIT) {
> +		advise(_("The <src> part of the refspec is a commit object.\n"
> +			 "Did you mean to create a new branch by pushing to\n"
> +			 "'%s:refs/heads/%s'?"),
> +		       matched_src_name, dst_value);

Good, except that "git push $there notes/commits^0:newnotes" may not want to become a branch and neither may "git push $there stash:wip", I think it is a reasonable piece of advice we'd give by default.

I do not think it is worth the effort of inspecting the tree of the commit object to special case notes and stash ;-)

Show 5 quoted lines

> +	} else if (type == OBJ_TAG) {
> +		advise(_("The <src> part of the refspec is a tag object.\n"
> +			 "Did you mean to create a new tag by pushing to\n"
> +			 "'%s:refs/tags/%s'?"),
> +		       matched_src_name, dst_value);

Good.

Show 10 quoted lines

> +	} else if (type == OBJ_TREE) {
> +		advise(_("The <src> part of the refspec is a tree object.\n"
> +			 "Did you mean to tag a new tree by pushing to\n"
> +			 "'%s:refs/tags/%s'?"),
> +		       matched_src_name, dst_value);
> +	} else if (type == OBJ_BLOB) {
> +		advise(_("The <src> part of the refspec is a blob object.\n"
> +			 "Did you mean to tag a new blob by pushing to\n"
> +			 "'%s:refs/tags/%s'?"),
> +		       matched_src_name, dst_value);

These two are questionable, but assuming that heads and tags are the only two hiearchies people would push into, they are acceptable choices.

Show 35 quoted lines

> +	} else {
> +		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> +		    matched_src_name, type);
> +	}
>  }
>  
>  static int match_explicit(struct ref *src, struct ref *dst,
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index d2a2cdd453..2e58721f98 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1222,4 +1222,29 @@ test_expect_success 'add remote matching the "insteadOf" URL' '
>  	git remote add backup xyz@example.com
>  '
>  
> +test_expect_success 'unqualified <dst> refspec DWIM and advice' '
> +	test_when_finished "(cd test && git tag -d some-tag)" &&
> +	(
> +		cd test &&
> +		git tag -a -m "Some tag" some-tag master &&
> +		for type in commit tag tree blob
> +		do
> +			if test "$type" = "blob"
> +			then
> +				oid=$(git rev-parse some-tag:file)
> +			else
> +				oid=$(git rev-parse some-tag^{$type})
> +			fi &&
> +			test_must_fail git push origin $oid:dst 2>err &&
> +			test_i18ngrep "error: The destination you" err &&
> +			test_i18ngrep "hint: Did you mean" err &&
> +			test_must_fail git -c advice.pushUnqualifiedRefName=false \
> +				push origin $oid:dst 2>err &&
> +			test_i18ngrep "error: The destination you" err &&
> +			test_i18ngrep ! "hint: Did you mean" err

Any failure in the &&-chain (or the last grep) would not terminate the for loop, so for the purpose of determining the success of this test_expect_success, the last "blob" iteration is the only thing that matters.

Which is probably not what you want. If testing all of these is important, then you can do this:

	(
		exit_with=true &&
		for type in ...
		do
			... many ... &&
			... many ... &&
			... tests ... ||
			exit_with=false
		done
		$exit_with
	)

or just say "|| exit" and leave the loop (and the subprocess running it) early on the first failure.

Show 6 quoted lines

> +		done
> +	)
> +'
> +
> +
>  test_done