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

Michael J Gruber [off-list ref] writes:

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.

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

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

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

Michael J Gruber [off-list ref] writes:

quoted

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

quoted

+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

Michael J Gruber [off-list ref] writes:

First of all: I define good/bad as matching the documentation.

Ok, I was primarily working from this:

commit bcc785f611dc6084be75999a3b6bafcc950e21d6
Author: Linus Torvalds [off-list ref]
Date:   Mon Oct 30 08:28:59 2006 -0800

    git push: add verbose flag and allow overriding of default target repository
    
    This adds a command line flag "-v" to enable a more verbose mode, and
    "--repo=" to override the default target repository for "git push" (which
    otherwise always defaults to "origin").
    
    This, together with the patch to allow dashes in config variable names,
    allows me to do
    
    	[alias]
    		push-all = push -v --repo=all
    
    in my user-global config file, and then I can (for any project I maintain)
    add to the project-local config file
    
    	[remote "all"]
    		url=one.target.repo:/directory
    		url=another.target:/pub/somewhere/else
    
    and now "git push-all" just updates all the target repositories, and shows
    me what it does - regardless of which repo I am in.
    
    Signed-off-by: Linus Torvalds [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

If documentation does not match it, we need to figure out why J6t with
bf07cc5 (git-push.txt: Describe --repo option in more detail, 2008-10-07)
needed to update the documentation.

It could be that the behaviour changed (perhaps by accident, perhaps by
design) after Linus introduced --repo with bcc785f (git push: add verbose
flag and allow overriding of default target repository, 2006-10-30) and
J6t documented that updated behaviour.  And since then there was another
behaviour change (again, perhaps by accident, perhaps by design) that made
you notice the description does not match the behaviour.

You will see that:

 (1) bf07cc5 (i.e. J6t's documentation) passes your tests;

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

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

Michael J Gruber [off-list ref] writes:

quoted

First of all: I define good/bad as matching the documentation.

Ok, I was primarily working from this:

commit bcc785f611dc6084be75999a3b6bafcc950e21d6
Author: Linus Torvalds [off-list ref]
Date:   Mon Oct 30 08:28:59 2006 -0800

    git push: add verbose flag and allow overriding of default target repository
    
    This adds a command line flag "-v" to enable a more verbose mode, and
    "--repo=" to override the default target repository for "git push" (which
    otherwise always defaults to "origin").
    
    This, together with the patch to allow dashes in config variable names,
    allows me to do
    
    	[alias]
    		push-all = push -v --repo=all
    
    in my user-global config file, and then I can (for any project I maintain)
    add to the project-local config file
    
    	[remote "all"]
    		url=one.target.repo:/directory
    		url=another.target:/pub/somewhere/else
    
    and now "git push-all" just updates all the target repositories, and shows
    me what it does - regardless of which repo I am in.
    
    Signed-off-by: Linus Torvalds [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

If documentation does not match it, we need to figure out why J6t with
bf07cc5 (git-push.txt: Describe --repo option in more detail, 2008-10-07)
needed to update the documentation.

It could be that the behaviour changed (perhaps by accident, perhaps by
design) after Linus introduced --repo with bcc785f (git push: add verbose
flag and allow overriding of default target repository, 2006-10-30) and
J6t documented that updated behaviour.  And since then there was another
behaviour change (again, perhaps by accident, perhaps by design) that made
you notice the description does not match the behaviour.

You will see that:

 (1) bf07cc5 (i.e. J6t's documentation) passes your tests;

 (2) somewhere between that and v1.6.2-rc2, there is a regression to make
     your test fail.

I see. Back then I checked whether there was a change to git-push at or
after J6t's doc commit, and there was none, but I didn't test. I'll do now.

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.

From Linus' description it's not clear to me what should happen when

there is no explicit repo argument but the branch is tracking a remote
and there is a --repo option. And I think this case is the only open
question: Should the option win or the tracking config? Code does
option, doc says tracking config.

[Also, I don't see immediately what's wrong with "alias.push-all = push
-v all", in the current situation where option and arg are equivalent,
but I haven't tried.]

I'm cc'ing Linus to make sure J6t's (current) doc describes the original
intent for --repo and my patch isn't stepping on /some/one's toes...

Sorry, but I do not have time today to look into this.  Could you help?

Of course. I ursurpated someone else's itch here, but now it's mine ;)

I'll bisect. Seems to be the right thing to do while watching soccer
later in the evening...

Michael

On Thu, Feb 26, 2009 at 12:48 PM, Michael J Gruber
[off-list ref] wrote:

Of course. I ursurpated someone else's itch here, but now it's mine ;)

Well then, I owe you one. In the mean time, thank you. :-)

j.

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

Michael J Gruber [off-list ref] writes:

quoted

First of all: I define good/bad as matching the documentation.

Ok, I was primarily working from this:

commit bcc785f611dc6084be75999a3b6bafcc950e21d6
Author: Linus Torvalds [off-list ref]
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.

 (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

Michael J Gruber [off-list ref] writes:

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

quoted

Michael J Gruber [off-list ref] writes:

quoted

First of all: I define good/bad as matching the documentation.

Ok, I was primarily working from this:

commit bcc785f611dc6084be75999a3b6bafcc950e21d6
Author: Linus Torvalds [off-list ref]
Date:   Mon Oct 30 08:28:59 2006 -0800

[snip]

quoted

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.

No, I didn't even know if the commit passed your tests.  Before the [snip]
I think I had something like:

	If your claim that this is a bug is true, it might have happened
	this way: Linus originally wrote it, behaviour changed, J6t
	documented the updated behaviour, behaviour changed again.

and after (1)/(2)/..., I have:

quoted

if the above conjecture is true, and we may want to fix that regression to
match the documentation.

So if you don't see that, it merely proves that the conjecture is false.
The updated documentation didn't describe correctly what should happen and
there is no bug other than in the documentation.

It's a different matter if we want to update the semantics, but I am not
sure if people agree with "the documented version is more natural".  I'm
neutral right now.  I may have more to say after I re-read the detailed
analysis part of your message.

On Fri, Feb 27, 2009 at 5:42 AM, Michael J Gruber
[off-list ref] wrote:

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

So I still don't get why Linus introduced the option. I'm looking at
bcc785f:builtin-push.c and AFAICT, the following are exactly
equivalent:

$ git push [options]... <repo>
$ git push [options]... --repo=<repo>

Which is why I sent the original message that has spawned this saga. :-)

Here's the abbreviated code in question:

        const char *repo = "origin";    /* default repository */

        for (i = 1; i < argc; i++) {
                const char *arg = argv[i];
                if (arg[0] != '-') {
                        repo = arg;
                        i++;
                        break;
                }
                if (!strncmp(arg, "--repo=", 7)) {
                        repo = arg+7;
                        continue;
                }
        }
        set_refspecs(argv + i, argc - i);
        return do_push(repo);

--repo can be placed anywhere on the command line, but other than
that, it's identical in effect to specifying the repo as the first
non-dashed argument.

Or am I completely blind?

j.

On Fri, 27 Feb 2009, Jay Soffian wrote:

So I still don't get why Linus introduced the option. I'm looking at
bcc785f:builtin-push.c and AFAICT, the following are exactly
equivalent:

$ git push [options]... <repo>
$ git push [options]... --repo=<repo>

Yes. 

But now do

	[alias]
		push-all=push all

and then try to add those options AFTERWARDS!

--repo can be placed anywhere on the command line, but other than
that, it's identical in effect to specifying the repo as the first
non-dashed argument.

Or am I completely blind?

It's the "placed anywhere on the command line" that is the important part.

Try 

	git push-all --tags

and it didn't use to work without "--repo=all".

Of course, I think it works now, because I think "git push" uses 
"parse_options" these days, so now "--tags" actually works even after the 
repository definition. So _these_ days, you can just do

	git push all --tags

but that was not true historically. Back then, if you wanted to use an 
alias (which mean that the repo was named _before_ the arguments), you 
needed to do

	git push --repo=all --tags

because putting "--tags" after the repository name wouldn't work.

So _today_, we could remove the use of "--repo". But today, we have 
another reason to do "--repo" - compatibility.

		Linus

On Fri, Feb 27, 2009 at 4:00 PM, Linus Torvalds
[off-list ref] wrote:

So _today_, we could remove the use of "--repo". But today, we have
another reason to do "--repo" - compatibility.

I was not suggesting removing --repo. I just didn't understand why it existed.

Thanks for the clarification.

j.