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

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 27, 2009, 10:42 UTC
Message-ID: <49A7C3A7.6060202@drmicha.warpmail.net>
In-Reply-To: <7vfxi1t99u.fsf@gitster.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 26.02.2009 18:09:

Show 9 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> First of all: I define good/bad as matching the documentation.
> 
> Ok, I was primarily working from this:
> 
> commit bcc785f611dc6084be75999a3b6bafcc950e21d6
> Author: Linus Torvalds <torvalds@osdl.org>
> Date:   Mon Oct 30 08:28:59 2006 -0800

[snip]

> You will see that:
> 
>  (1) bf07cc5 (i.e. J6t's documentation) passes your tests;

Hmm, I don't see that, we must be doing something differently, see below.

Show 11 quoted lines

>  (2) somewhere between that and v1.6.2-rc2, there is a regression to make
>      your test fail.
> 
> if the above conjecture is true, and we may want to fix that regression to
> match the documentation.
> 
> On the other hand, if bf07cc5 does not pass your tests, it means that the
> documentation update was the cause of the confusion, and it is not the
> behaviour that needs to be fixed.
> 
> Sorry, but I do not have time today to look into this.  Could you help?

I'll try and provide the necessary info below, but I think there is a decision to be made about the desired order of priorities for "repo argument", "repo option (--repo=)" and "tracking info config (branch.$branch.remote=)".

Current code: argument > option > track config > 'origin' Current doc: argument > track config > option > 'origin'

I'd say the documented version is more natural, meaning --repo=$repo is fully equivalent to changing "origin" to "$repo".

Now, here's if you want to know the details:

When Linus introduced "--repo" there was no heeding of the tracking info at all:

bcc785f (git push: add verbose flag and allow overriding of default target repository, 2006-10-30) Linus introduces --repo.

5751f49 (Move remote parsing into a library file out of builtin-push., 2007-05-12) Daniel Barkalow introduces the use of branch.$branch.remote info for the repo if git push has no other repo arguments nor --repo options.

378c483 (Use parseopts in builtin-push, 2007-11-04) Daniel switches git-push to parseopts, without changing the behaviour.

bf07cc5 (git-push.txt: Describe --repo option in more detail, 2008-10-07) J6t writes the current version of the doc.

Using a trimmed down ./t5516-fetch-push.sh from my patch (and a copy of current test-lib), all 4 of them give the same results:

*   ok 1: setup
*   ok 2: push with --repo=repourl from non-tracking branch
*   ok 3: push with --repo=remoterepo from non-tracking branch
* FAIL 4: 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

* FAIL 5: 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

[testrepo exists, origin does not]

Note that bcc785f was not even supposed to heed the value of branch.master.remote, so it's clear they fail 4&5. 2&3 only test whether --repo is considered if there's no other info, so it's clear they pass (versions without bcc785f would fail).

Cheers, Michael