git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* git push usage
@ 2009-02-20  9:16 Jay Soffian
  2009-02-21  9:32 ` Jay Soffian
  0 siblings, 1 reply; 13+ messages in thread
From: Jay Soffian @ 2009-02-20  9:16 UTC (permalink / raw)
  To: Git Mailing List

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: git push usage
  2009-02-20  9:16 git push usage Jay Soffian
@ 2009-02-21  9:32 ` Jay Soffian
  2009-02-24 17:40   ` [RFC] add test cases for the --repo option to git push Michael J Gruber
  0 siblings, 1 reply; 13+ messages in thread
From: Jay Soffian @ 2009-02-21  9:32 UTC (permalink / raw)
  To: Git Mailing List

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

On Fri, Feb 20, 2009 at 4:16 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> 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.
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC] add test cases for the --repo option to git push
  2009-02-21  9:32 ` Jay Soffian
@ 2009-02-24 17:40   ` Michael J Gruber
  2009-02-25 21:58     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Michael J Gruber @ 2009-02-24 17:40 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 89649e7..8393366 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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
+'
+
+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
+'
+
+# clean up fake remote and tracking info
+git config --unset-all branch.master.remote
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-- 
1.6.2.rc1.30.gd43c

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-24 17:40   ` [RFC] add test cases for the --repo option to git push Michael J Gruber
@ 2009-02-25 21:58     ` Junio C Hamano
  2009-02-26  9:26       ` Michael J Gruber
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2009-02-25 21:58 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-25 21:58     ` Junio C Hamano
@ 2009-02-26  9:26       ` Michael J Gruber
  2009-02-26 17:09         ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Michael J Gruber @ 2009-02-26  9:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano venit, vidit, dixit 25.02.2009 22:58:
> 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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-26  9:26       ` Michael J Gruber
@ 2009-02-26 17:09         ` Junio C Hamano
  2009-02-26 17:48           ` Michael J Gruber
  2009-02-27 10:42           ` Michael J Gruber
  0 siblings, 2 replies; 13+ messages in thread
From: Junio C Hamano @ 2009-02-26 17:09 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

    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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-26 17:09         ` Junio C Hamano
@ 2009-02-26 17:48           ` Michael J Gruber
  2009-02-26 22:11             ` Jay Soffian
  2009-02-27 10:42           ` Michael J Gruber
  1 sibling, 1 reply; 13+ messages in thread
From: Michael J Gruber @ 2009-02-26 17:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Linus Torvalds

Junio C Hamano venit, vidit, dixit 26.02.2009 18:09:
> 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
> 
>     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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-26 17:48           ` Michael J Gruber
@ 2009-02-26 22:11             ` Jay Soffian
  0 siblings, 0 replies; 13+ messages in thread
From: Jay Soffian @ 2009-02-26 22:11 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Junio C Hamano, git, Linus Torvalds

On Thu, Feb 26, 2009 at 12:48 PM, Michael J Gruber
<git@drmicha.warpmail.net> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-26 17:09         ` Junio C Hamano
  2009-02-26 17:48           ` Michael J Gruber
@ 2009-02-27 10:42           ` Michael J Gruber
  2009-02-27 17:34             ` Junio C Hamano
  2009-02-27 20:48             ` Jay Soffian
  1 sibling, 2 replies; 13+ messages in thread
From: Michael J Gruber @ 2009-02-27 10:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael J Gruber, git, Linus Torvalds

Junio C Hamano venit, vidit, dixit 26.02.2009 18:09:
> 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.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-27 10:42           ` Michael J Gruber
@ 2009-02-27 17:34             ` Junio C Hamano
  2009-02-27 20:48             ` Jay Soffian
  1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2009-02-27 17:34 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Linus Torvalds

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 26.02.2009 18:09:
>> 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.

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:

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-27 10:42           ` Michael J Gruber
  2009-02-27 17:34             ` Junio C Hamano
@ 2009-02-27 20:48             ` Jay Soffian
  2009-02-27 21:00               ` Linus Torvalds
  1 sibling, 1 reply; 13+ messages in thread
From: Jay Soffian @ 2009-02-27 20:48 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Junio C Hamano, git, Linus Torvalds

On Fri, Feb 27, 2009 at 5:42 AM, Michael J Gruber
<git@drmicha.warpmail.net> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-27 20:48             ` Jay Soffian
@ 2009-02-27 21:00               ` Linus Torvalds
  2009-02-27 21:21                 ` Jay Soffian
  0 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2009-02-27 21:00 UTC (permalink / raw)
  To: Jay Soffian; +Cc: Michael J Gruber, Junio C Hamano, git



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] add test cases for the --repo option to git push
  2009-02-27 21:00               ` Linus Torvalds
@ 2009-02-27 21:21                 ` Jay Soffian
  0 siblings, 0 replies; 13+ messages in thread
From: Jay Soffian @ 2009-02-27 21:21 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Michael J Gruber, Junio C Hamano, git

On Fri, Feb 27, 2009 at 4:00 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2009-02-27 21:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-20  9:16 git push usage Jay Soffian
2009-02-21  9:32 ` Jay Soffian
2009-02-24 17:40   ` [RFC] add test cases for the --repo option to git push Michael J Gruber
2009-02-25 21:58     ` Junio C Hamano
2009-02-26  9:26       ` Michael J Gruber
2009-02-26 17:09         ` Junio C Hamano
2009-02-26 17:48           ` Michael J Gruber
2009-02-26 22:11             ` Jay Soffian
2009-02-27 10:42           ` Michael J Gruber
2009-02-27 17:34             ` Junio C Hamano
2009-02-27 20:48             ` Jay Soffian
2009-02-27 21:00               ` Linus Torvalds
2009-02-27 21:21                 ` Jay Soffian

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).