git push usage

The man page for git push claims:

 --repo=<repository>
     This option is only relevant if no <repository> argument is passed
     in the invocation. In this case, git-push derives the remote name
     from the current branch: If it tracks a remote branch, then that
     remote repository is pushed to. Otherwise, the name "origin" is
     used. For this latter case, this option can be used to override the
     name "origin". In other words, the difference between these two
     commands

         git push public         #1
         git push --repo=public  #2

     is that #1 always pushes to "public" whereas #2 pushes to "public"
     only if the current branch does not track a remote branch. This is
     useful if you write an alias or script around git-push.

However, I'm sitting here looking at the code and I don't see how this is possible. I've also done some testing. So I think the man page lies and that forms (1) and (2) are equivalent as shown.

cmd_push() is:

  const char *repo = NULL; /* default repository */
  struct option options[] = {
    ...
    OPT_STRING( 0 , "repo", &repo, "repository", "repository"),
    ...
  }

  argc = parse_options(argc, argv, options, push_usage, 0);

  if (argc > 0) {
    repo = argv[0];
    set_refspecs(argv + 1, argc - 1);
  }

  rc = do_push(repo, flags);

So if the user specifies --repo, then its value is assigned to *repo by parse_options. If the user otherwise specifies a repository w/o --repo, that will be argv[0] after parse_options, so it will get assigned to *repo. Assuming no other arguments, set_refspecs gets called with argc = 0 and returns w/o doing anything.

So the only difference I can see is that form #1 allows the user to specify a refspec on the command line. Form #2 does not since the first non-dashed argument gets assigned to *repo, so:

$ git push --repo src:dst

would assign src:dst to *repo, which would choke.

So, what's the point of the --repo dashed-option then?

j.

Tap...tap...tap... is this thing on? :-)

On Fri, Feb 20, 2009 at 4:16 AM, Jay Soffian <jaysoffian@gmail.com> wrote:

The --repo=myorigin option is supposed to change the default fallback remote from "origin" to "myorigin", but not override any direct argument nor config info of tracking branches. Add tests for this. (currently 2 known breakages)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Tap tap tap....

This is an R for Comments on the desired behaviour of git push with respect to --repo. I think the tests below expose that the current behaviour does not match the current doc. I'm messing around in the code but can't quite produce a match with the doc yet. Before investing more time I'm wondering whether the code should adjusted to the doc or vice versa...

The code change I'm experimenting with right now is making default_remote_name a global and passing it from push. Does not look nice (esp. w.r.t. libification) but seems to ibe the minimally invasive solution.

 t/t5516-fetch-push.sh |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

Michael J Gruber <git@drmicha.warpmail.net> 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.

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:

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.

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.

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 <git@drmicha.warpmail.net> writes:

Ok, I was primarily working from this:

commit bcc785f611dc6084be75999a3b6bafcc950e21d6
Author: Linus Torvalds <torvalds@osdl.org>
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 <torvalds@osdl.org>
    Signed-off-by: Junio C Hamano <junkio@cox.net>

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:

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.

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

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 <git@drmicha.warpmail.net> wrote:

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

j.

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

[snip]

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

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 <git@drmicha.warpmail.net> writes:

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:

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 <git@drmicha.warpmail.net> 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>

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:

Yes. 

But now do

	[alias]
		push-all=push all

and then try to add those options AFTERWARDS!

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 <torvalds@linux-foundation.org> wrote:

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

Thanks for the clarification.

j.