Re: [RFC] add test cases for the --repo option to git push

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 26, 2009, 09:26 UTC
Message-ID: <49A66057.1050501@drmicha.warpmail.net>
In-Reply-To: <7vhc2iyy9y.fsf@gitster.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 25.02.2009 22:58:

Show 45 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> @@ -419,6 +419,41 @@ test_expect_success 'push with config remote.*.push = HEAD' '
>>  git config --remove-section remote.there
>>  git config --remove-section branch.master
>>  
>> +test_expect_success 'push with --repo=repourl from non-tracking branch' '
>> +
>> +	mk_test heads/master &&
>> +	git push --repo=testrepo &&
>> +	check_push_result $the_commit heads/master
>> +'
>> +
>> +# set up fake remote config
>> +test_expect_success 'push with --repo=remoterepo from non-tracking branch' '
>> +
>> +	mk_test heads/master &&
>> +	git config remote.testremote.url testrepo &&
>> +	git push --repo=testremote &&
>> +	check_push_result $the_commit heads/master
>> +'
>> +
>> +# set up fake tracking info; testrepo exists, origin does not.
>> +test_expect_failure 'push with --repo=repo from tracking branch with bad config' '
>> +
>> +	mk_test heads/master &&
>> +	git config branch.master.remote origin &&
>> +	test_must_fail git push --repo=testrepo
>> +'
> 
> At this point, you have:
> 
> 	branch.master.remote = origin
>         remote.testremote.url = testrepo
> 
> and nothing else related to push.  And you are asking a "git push but
> instead of origin please default to testrepo".
> 
> In response to that request, in order to figure out what refs to push by
> default, remote.testremote.push instead of remote.origin.push will be
> consulted, and we won't even have to notice nor complain the missing
> remote.origin.  remote.testremote.push is missing, so the push defaults to
> the "matching ref" behaviour.
> 
> I do not understand why you expect the above to fail.

First of all: I define good/bad as matching the documentation. Current behaviour does not match the doc: "git push--repo=repo" is equivalent to "git push repo" with the current code. One could simply document it as such, of course. I took the other approach: Test for documented behaviour and adjust the code.

Now for the test above: I expect: git push looks whether the branch tracks a remote. It finds "origin" due to branch.master.remote=origin It looks for a push refspec. It finds none thus uses the default. It pushes to origin with the default refspec and fails because there is no remote origin. This is why I have test_must_fail

I get with current behaviour: git takes the --repo=testrepo argument instead of the tracking info. push succeeds accordingly. This is why I have test_expect_failure.

After the fix in PATCH 2/2, the behaviour matches exactly what I described under expect. Note that I did not change the handling of tracking info etc. at all in the PATCH, I only changed the treatment of the --repo option and produced the expected behaviour.

Show 9 quoted lines

>> +test_expect_failure 'push with --repo=repo from tracking branch with good config' '
>> +
>> +	mk_test heads/master &&
>> +	git config branch.master.remote testrepo &&
>> +	git push --repo=origin &&
>> +	check_push_result $the_commit heads/master
>> +'
> 
> Likewise.

Uhm, yes, likewise ;) Expect: git push looks whether the branch tracks a remote. It finds "testrepo" due to branch.master.remote=testrepo It looks for a push refspec. It finds none thus uses the default. It pushes to testrepo with the default refspec and succeeds (exists, has matching refs). For extra safety, I check that the push produced the correct ref on the remote side. This is why I expect it to succeed.

I get with current behaviour: git takes the --repo=origin argument instead of the tracking info. push fails because there is no remote origin. This is why I have test_expect_failure.

Note that according to the doc, "git push --repo=origin" is equivalent to "git push", but this test shows that it is not currently.

After PATCH 2/2 the test succeeds as expected.

> I think the "good/bad config" labels given to your two test scripts are
> swapped and there is no bug or misfeature here.

Well, there's a discrepancy between doc and behaviour, as reported by Jay. The proposed patch brings them in line the hard way (fixing/adjusting the code).

Michael