Re: [PATCH] git-send-pack: Fix --all option when used with directory
Thanks a lot, Jeff, your explanation really helped! -- Regards, Stanislav On 24/03/16 20:02, Jeff King wrote: On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote: Thanks for noticing; the above explanation however does not make it very clear why the symptom exhibits itself only when "directory" is given (it also is unclear if "target" being a directory is special, or if any remote repository specification, e.g. host:/path/to/dir, triggers the same problem). I'll think about a better explanation and will post it here - thanks. Feel free to steal from the explanation I posted earlier. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote: > >Thanks for noticing; the above explanation however does not make it > >very clear why the symptom exhibits itself only when "directory" is > >given (it also is unclear if "target" being a directory is special, > >or if any remote repository specification, e.g. host:/path/to/dir, > >triggers the same problem). > I'll think about a better explanation and will post it here - thanks. Feel free to steal from the explanation I posted earlier. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On 24/03/16 19:37, Jeff King wrote: I guess a follow-up to my question is: what does "git send-pack" do that "git push" does not? Again, this is just for my curiosity (the answer may be "nothing, we just happened to build our scripts around send-pack", and that is perfectly fine). Well, this is some really old code (eight years old), and git version used at the moment was around 1.6. So I'm pretty much sure that it just happened to be the decision of the developer that wrote this piece of code. -- Stanislav -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On 24/03/16 18:11, Junio C Hamano wrote: Please see "git shortlog --no-merges" output from recent history and notice that s/Fix/fix/ would make things more consistent. Thanks for letting me know, I'll update that. Thanks for noticing; the above explanation however does not make it very clear why the symptom exhibits itself only when "directory" is given (it also is unclear if "target" being a directory is special, or if any remote repository specification, e.g. host:/path/to/dir, triggers the same problem). I'll think about a better explanation and will post it here - thanks. -- Regards, Stanislav -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On Thu, Mar 24, 2016 at 03:57:59PM +0200, Stanislav Kolotinskiy wrote: > On 23/03/16 23:22, Jeff King wrote: > >Not that it matters for this bug, but for my own curiosity, what do you > >use "send-pack --all" for? I've generally assumed that nobody directly > >calls send-pack themselves these days, but of course we have no data to > >support that either way. So I am always interested to hear about unusual > >use cases. > Well, here at Assembla we're using send-pack --all for creating forks > from repos in a quick and efficient way. I guess a follow-up to my question is: what does "git send-pack" do that "git push" does not? Again, this is just for my curiosity (the answer may be "nothing, we just happened to build our scripts around send-pack", and that is perfectly fine). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
Stanislav Kolotinskiy writes: > Subject: Re: [PATCH] git-send-pack: Fix --all option when used with directory Please see "git shortlog --no-merges" output from recent history and notice that s/Fix/fix/ would make things more consistent. > When using git send-pack with --all option and a target directory, > usage message is being displayed instead of performing the actual > transmission. > > The reason for this issue is that refspecs variable is being > calculated in a different way comparing to previous versions, and > even though the number of refspecs (nr_refspecs) is 0, refspecs > contain all the arguments and switches passed to send-pack. Thanks for noticing; the above explanation however does not make it very clear why the symptom exhibits itself only when "directory" is given (it also is unclear if "target" being a directory is special, or if any remote repository specification, e.g. host:/path/to/dir, triggers the same problem). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On 23/03/16 23:22, Jeff King wrote: Not that it matters for this bug, but for my own curiosity, what do you use "send-pack --all" for? I've generally assumed that nobody directly calls send-pack themselves these days, but of course we have no data to support that either way. So I am always interested to hear about unusual use cases. Well, here at Assembla we're using send-pack --all for creating forks from repos in a quick and efficient way. The tests are roughly grouped by functionality. send-pack tests are in the t540x range, and this should probably go there. Though I also suspect it could easily be added to the end of an existing test script, which is preferable. I'm not really comfortable (yet) with git tests, so thanks for pointing to that. I did see t5400, but thought that bug fixes should bring their own, separate, test files. Also, thanks for all the explanations and for the adaptation of my test to a way better version! I'm going to update the patch and will send another version. Thanks again! -- Regards, Stanislav -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On Wed, Mar 23, 2016 at 05:22:13PM -0400, Jeff King wrote: > > diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh > > The tests are roughly grouped by functionality. send-pack tests are in > the t540x range, and this should probably go there. Though I also > suspect it could easily be added to the end of an existing test script, > which is preferable. > > > +test_expect_success setup ' > > This setup seems a bit more complicated than it needs to be. It's nice > to keep tests as simple as possible, so a reader can understand exactly > what is being tested. > > Here are a few things I think we can simplify: > [...] So I think we could replace your t9904 with something like this: diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh index 04cea97..305ca7a 100755 --- a/t/t5400-send-pack.sh +++ b/t/t5400-send-pack.sh @@ -128,6 +128,18 @@ test_expect_success 'denyNonFastforwards trumps --force' ' test "$victim_orig" = "$victim_head" ' +test_expect_success 'send-pack --all sends all branches' ' + # make sure we have at least 2 branches with different + # values, just to be thorough + git branch other-branch HEAD^ && + + git init --bare all.git && + git send-pack --all all.git && + git for-each-ref refs/heads >expect && + git -C all.git for-each-ref refs/heads >actual && + test_cmp expect actual +' + test_expect_success 'push --all excludes remote-tracking hierarchy' ' mkdir parent && ( -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-pack: Fix --all option when used with directory
On Wed, Mar 23, 2016 at 06:24:22PM +0200, Stanislav Kolotinskiy wrote: > When using git send-pack with --all option > and a target directory, usage message is being > displayed instead of performing the actual transmission. Yeah, that seems very wrong. Not that it matters for this bug, but for my own curiosity, what do you use "send-pack --all" for? I've generally assumed that nobody directly calls send-pack themselves these days, but of course we have no data to support that either way. So I am always interested to hear about unusual use cases. > The reason for this issue is that refspecs variable is being > calculated in a different way comparing to previous versions, > and even though the number of refspecs (nr_refspecs) is 0, > refspecs contain all the arguments and switches passed to send-pack. Looks like this bisects to 068c77a (builtin/send-pack.c: use parse_options API, 2015-08-19). Thanks for including a test, which made the bisection easy. I wondered how the original ever worked, since it also points to argv with the "refspecs" variable. But we only do so when we see an actual refspec argument, and otherwise leave it as NULL. Whereas the new code lumps the destination and the refspecs into the same conditional. That made me wonder if any other code cared about refspecs being NULL. I don't think so, though. The other spots I looked at seem to use nr_refspec, which is good. There is one interesting interaction with --stdin, which _does_ always set refspecs to a non-NULL value. In the original code (prior to 068c77a), doing this: git send-pack --stdin --all diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh The tests are roughly grouped by functionality. send-pack tests are in the t540x range, and this should probably go there. Though I also suspect it could easily be added to the end of an existing test script, which is preferable. > +test_expect_success setup ' This setup seems a bit more complicated than it needs to be. It's nice to keep tests as simple as possible, so a reader can understand exactly what is being tested. Here are a few things I think we can simplify: > + git init --bare bare_repo && git init repo && ( > + cd repo && We're in a repository already, so we should just be able to push directly out of the "main" test repository. > + git remote add origin ../bare_repo && We don't need to define remotes; we can just push directly to paths. > + date >file1 && git add file1 && test_tick && > + git commit -m Initial && You can use "test_commit" to make this simpler. > + git push origin master && > + > + git checkout -b other && date >file2 && > + git add file2 && test_tick && > + git commit -m Other && > + git push origin other I guess you have multiple branches here so that we can be sure that "--all" is pushing all of them. But your later test doesn't actually check that. > + ) && git init another && ( > + cd another && > + > + git config receive.denyCurrentBranch ignore > + ) > +' If you make the destination repository bare, then you do not have to worry about denyCurrentBranch. > +test_expect_success 'send-pack --all should copy all refs' ' > + cd bare_repo && git send-pack --all ../another > +' We try to keep mutations of the test script state (like "cd") inside a subshell, so they don't influence later tests. There aren't any later tests now, of course, but it's one less thing for somebody coming along later to have to worry about. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-send-pack: Fix --all option when used with directory
When using git send-pack with --all option and a target directory, usage message is being displayed instead of performing the actual transmission. The reason for this issue is that refspecs variable is being calculated in a different way comparing to previous versions, and even though the number of refspecs (nr_refspecs) is 0, refspecs contain all the arguments and switches passed to send-pack. This ensures that send-pack will stop execution only when --all or --mirror switch is used in conjunction with any refs passed. Signed-off-by: Stanislav Kolotinskiy --- builtin/send-pack.c | 2 +- t/t9904-send-pack-all.sh | 32 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100755 t/t9904-send-pack-all.sh diff --git a/builtin/send-pack.c b/builtin/send-pack.c index f6e5d64..19f0577 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -225,7 +225,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) * --all and --mirror are incompatible; neither makes sense * with any refspecs. */ - if ((refspecs && (send_all || args.send_mirror)) || + if ((nr_refspecs > 0 && (send_all || args.send_mirror)) || (send_all && args.send_mirror)) usage_with_options(send_pack_usage, options); diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh new file mode 100755 index 000..f68d004 --- /dev/null +++ b/t/t9904-send-pack-all.sh @@ -0,0 +1,32 @@ +#!/bin/sh + +test_description='Make sure that send-pack --all copies all refs' + +. ./test-lib.sh + +test_expect_success setup ' + + git init --bare bare_repo && git init repo && ( + cd repo && + + git remote add origin ../bare_repo && + date >file1 && git add file1 && test_tick && + git commit -m Initial && + git push origin master && + + git checkout -b other && date >file2 && + git add file2 && test_tick && + git commit -m Other && + git push origin other + ) && git init another && ( + cd another && + + git config receive.denyCurrentBranch ignore + ) +' + +test_expect_success 'send-pack --all should copy all refs' ' + cd bare_repo && git send-pack --all ../another +' + +test_done -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
