{"thread":{"id":"41798","subject":"[PATCH] git-send-pack: Fix --all option when used with directory","startedAt":"2016-03-23T16:24:22Z","lastAt":"2016-04-01T10:42:51Z","messageCount":10,"participants":["Stanislav Kolotinskiy","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281589","messageId":"1458750262-25765-1-git-send-email-stanislav@assembla.com","threadId":"41798","inReplyTo":null,"subject":"[PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Stanislav Kolotinskiy","fromEmail":"stanislav@assembla.com","sentAt":"2016-03-23T16:24:22Z","receivedAt":"2016-03-23T16:24:22Z","isPatch":true,"sender":{"key":"stanislav@assembla.com","avatar":"https://gravatar.com/avatar/dca9d54f97b9c9bb1c41538bcc64a933d063c1f8fe3cac0936a4d4b09e4c45ff?d=mp&s=160"},"body":"When using git send-pack with --all option\nand a target directory, usage message is being\ndisplayed instead of performing the actual transmission.\n\nThe reason for this issue is that refspecs variable is being\ncalculated in a different way comparing to previous versions,\nand even though the number of refspecs (nr_refspecs) is 0,\nrefspecs contain all the arguments and switches passed to send-pack.\n\nThis ensures that send-pack will stop execution only when --all\nor --mirror switch is used in conjunction with any refs passed.\n\nSigned-off-by: Stanislav Kolotinskiy <stanislav@assembla.com>\n---\n builtin/send-pack.c      |  2 +-\n t/t9904-send-pack-all.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n create mode 100755 t/t9904-send-pack-all.sh\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f6e5d64..19f0577 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -225,7 +225,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t * --all and --mirror are incompatible; neither makes sense\n \t * with any refspecs.\n \t */\n-\tif ((refspecs && (send_all || args.send_mirror)) ||\n+\tif ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||\n \t    (send_all && args.send_mirror))\n \t\tusage_with_options(send_pack_usage, options);\n \ndiff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh\nnew file mode 100755\nindex 0000000..f68d004\n--- /dev/null\n+++ b/t/t9904-send-pack-all.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='Make sure that send-pack --all copies all refs'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\tgit init --bare bare_repo && git init repo && (\n+\t\tcd repo &&\n+\n+\t\tgit remote add origin ../bare_repo &&\n+\t\tdate >file1 && git add file1 && test_tick &&\n+\t\tgit commit -m Initial &&\n+\t\tgit push origin master &&\n+\n+\t\tgit checkout -b other && date >file2 &&\n+\t\tgit add file2 && test_tick &&\n+\t\tgit commit -m Other &&\n+\t\tgit push origin other\n+\t) && git init another && (\n+\t\tcd another &&\n+\n+\t\tgit config receive.denyCurrentBranch ignore\n+\t)\n+'\n+\n+test_expect_success 'send-pack --all should copy all refs' '\n+\tcd bare_repo && git send-pack --all ../another\n+'\n+\n+test_done\n-- \n2.7.4\n"},{"id":"281608","messageId":"20160323212213.GA19920@sigill.intra.peff.net","threadId":"41798","inReplyTo":"1458750262-25765-1-git-send-email-stanislav@assembla.com","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-23T21:22:13Z","receivedAt":"2016-03-23T21:22:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 06:24:22PM +0200, Stanislav Kolotinskiy wrote:\n\n> When using git send-pack with --all option\n> and a target directory, usage message is being\n> displayed instead of performing the actual transmission.\n\nYeah, that seems very wrong.\n\nNot that it matters for this bug, but for my own curiosity, what do you\nuse \"send-pack --all\" for? I've generally assumed that nobody directly\ncalls send-pack themselves these days, but of course we have no data to\nsupport that either way. So I am always interested to hear about unusual\nuse cases.\n\n> The reason for this issue is that refspecs variable is being\n> calculated in a different way comparing to previous versions,\n> and even though the number of refspecs (nr_refspecs) is 0,\n> refspecs contain all the arguments and switches passed to send-pack.\n\nLooks like this bisects to 068c77a (builtin/send-pack.c: use\nparse_options API, 2015-08-19). Thanks for including a test, which made\nthe bisection easy.\n\nI wondered how the original ever worked, since it also points to argv\nwith the \"refspecs\" variable. But we only do so when we see an actual\nrefspec argument, and otherwise leave it as NULL. Whereas the new code\nlumps the destination and the refspecs into the same conditional.\n\nThat made me wonder if any other code cared about refspecs being NULL. I\ndon't think so, though. The other spots I looked at seem to use\nnr_refspec, which is good. There is one interesting interaction with\n--stdin, which _does_ always set refspecs to a non-NULL value.\n\nIn the original code (prior to 068c77a), doing this:\n\n  git send-pack --stdin --all </dev/null\n\ncomplained. If though we did not specify any refspecs, it saw that\n\"refspecs\" was non-NULL.\n\nWith your patch, that works (since nr_refspecs ends up as 0, it is OK\nwith --all). I think that is probably OK, though of course it is a\nsomewhat nonsensical request.\n\nArguably the \"if (stdin)\" conditional should also check for --all or\n--mirror and complain, but I doubt it really matters in practice.\n\n> diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh\n\nThe tests are roughly grouped by functionality. send-pack tests are in\nthe t540x range, and this should probably go there. Though I also\nsuspect it could easily be added to the end of an existing test script,\nwhich is preferable.\n\n> +test_expect_success setup '\n\nThis setup seems a bit more complicated than it needs to be. It's nice\nto keep tests as simple as possible, so a reader can understand exactly\nwhat is being tested.\n\nHere are a few things I think we can simplify:\n\n> +\tgit init --bare bare_repo && git init repo && (\n> +\t\tcd repo &&\n\nWe're in a repository already, so we should just be able to push\ndirectly out of the \"main\" test repository.\n\n> +\t\tgit remote add origin ../bare_repo &&\n\nWe don't need to define remotes; we can just push directly to paths.\n\n> +\t\tdate >file1 && git add file1 && test_tick &&\n> +\t\tgit commit -m Initial &&\n\nYou can use \"test_commit\" to make this simpler.\n\n> +\t\tgit push origin master &&\n> +\n> +\t\tgit checkout -b other && date >file2 &&\n> +\t\tgit add file2 && test_tick &&\n> +\t\tgit commit -m Other &&\n> +\t\tgit push origin other\n\nI guess you have multiple branches here so that we can be sure that\n\"--all\" is pushing all of them. But your later test doesn't actually\ncheck that.\n\n> +\t) && git init another && (\n> +\t\tcd another &&\n> +\n> +\t\tgit config receive.denyCurrentBranch ignore\n> +\t)\n> +'\n\nIf you make the destination repository bare, then you do not have to\nworry about denyCurrentBranch.\n\n> +test_expect_success 'send-pack --all should copy all refs' '\n> +\tcd bare_repo && git send-pack --all ../another\n> +'\n\nWe try to keep mutations of the test script state (like \"cd\") inside a\nsubshell, so they don't influence later tests. There aren't any later\ntests now, of course, but it's one less thing for somebody coming along\nlater to have to worry about.\n\n-Peff\n"},{"id":"281610","messageId":"20160323213034.GB19920@sigill.intra.peff.net","threadId":"41798","inReplyTo":"20160323212213.GA19920@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-23T21:30:34Z","receivedAt":"2016-03-23T21:30:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2016 at 05:22:13PM -0400, Jeff King wrote:\n\n> > diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh\n> \n> The tests are roughly grouped by functionality. send-pack tests are in\n> the t540x range, and this should probably go there. Though I also\n> suspect it could easily be added to the end of an existing test script,\n> which is preferable.\n> \n> > +test_expect_success setup '\n> \n> This setup seems a bit more complicated than it needs to be. It's nice\n> to keep tests as simple as possible, so a reader can understand exactly\n> what is being tested.\n> \n> Here are a few things I think we can simplify:\n> [...]\n\nSo I think we could replace your t9904 with something like this:\n\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 04cea97..305ca7a 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -128,6 +128,18 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n \ttest \"$victim_orig\" = \"$victim_head\"\n '\n \n+test_expect_success 'send-pack --all sends all branches' '\n+\t# make sure we have at least 2 branches with different\n+\t# values, just to be thorough\n+\tgit branch other-branch HEAD^ &&\n+\n+\tgit init --bare all.git &&\n+\tgit send-pack --all all.git &&\n+\tgit for-each-ref refs/heads >expect &&\n+\tgit -C all.git for-each-ref refs/heads >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \tmkdir parent &&\n \t(\n"},{"id":"281669","messageId":"56F3F267.30900@assembla.com","threadId":"41798","inReplyTo":"20160323212213.GA19920@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Stanislav Kolotinskiy","fromEmail":"stanislav@assembla.com","sentAt":"2016-03-24T13:57:59Z","receivedAt":"2016-03-24T13:57:59Z","isPatch":true,"sender":{"key":"stanislav@assembla.com","avatar":"https://gravatar.com/avatar/dca9d54f97b9c9bb1c41538bcc64a933d063c1f8fe3cac0936a4d4b09e4c45ff?d=mp&s=160"},"body":"On 23/03/16 23:22, Jeff King wrote:\n> Not that it matters for this bug, but for my own curiosity, what do you\n> use \"send-pack --all\" for? I've generally assumed that nobody directly\n> calls send-pack themselves these days, but of course we have no data to\n> support that either way. So I am always interested to hear about unusual\n> use cases.\nWell, here at Assembla we're using send-pack --all for creating forks\nfrom repos in a quick and efficient way.\n> The tests are roughly grouped by functionality. send-pack tests are in\n> the t540x range, and this should probably go there. Though I also\n> suspect it could easily be added to the end of an existing test script,\n> which is preferable.\nI'm not really comfortable (yet) with git tests, so thanks for pointing \nto that.\nI did see t5400, but thought that bug fixes should bring their own, \nseparate,\ntest files. Also, thanks for all the explanations and for the adaptation \nof my\ntest to a way better version!\n\nI'm going to update the patch and will send another version. Thanks again!\n\n--\nRegards,\nStanislav\n"},{"id":"281679","messageId":"xmqqpoujama8.fsf@gitster.mtv.corp.google.com","threadId":"41798","inReplyTo":"1458750262-25765-1-git-send-email-stanislav@assembla.com","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-24T16:11:27Z","receivedAt":"2016-03-24T16:11:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stanislav Kolotinskiy <stanislav@assembla.com> writes:\n\n> Subject: Re: [PATCH] git-send-pack: Fix --all option when used with directory\n\nPlease see \"git shortlog --no-merges\" output from recent history and\nnotice that s/Fix/fix/ would make things more consistent.\n\n> When using git send-pack with --all option and a target directory,\n> usage message is being displayed instead of performing the actual\n> transmission.\n>\n> The reason for this issue is that refspecs variable is being\n> calculated in a different way comparing to previous versions, and\n> even though the number of refspecs (nr_refspecs) is 0, refspecs\n> contain all the arguments and switches passed to send-pack.\n\nThanks for noticing; the above explanation however does not make it\nvery clear why the symptom exhibits itself only when \"directory\" is\ngiven (it also is unclear if \"target\" being a directory is special,\nor if any remote repository specification, e.g. host:/path/to/dir,\ntriggers the same problem).\n"},{"id":"281693","messageId":"20160324173731.GD6341@sigill.intra.peff.net","threadId":"41798","inReplyTo":"56F3F267.30900@assembla.com","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-24T17:37:31Z","receivedAt":"2016-03-24T17:37:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2016 at 03:57:59PM +0200, Stanislav Kolotinskiy wrote:\n\n> On 23/03/16 23:22, Jeff King wrote:\n> >Not that it matters for this bug, but for my own curiosity, what do you\n> >use \"send-pack --all\" for? I've generally assumed that nobody directly\n> >calls send-pack themselves these days, but of course we have no data to\n> >support that either way. So I am always interested to hear about unusual\n> >use cases.\n> Well, here at Assembla we're using send-pack --all for creating forks\n> from repos in a quick and efficient way.\n\nI guess a follow-up to my question is: what does \"git send-pack\" do that\n\"git push\" does not? Again, this is just for my curiosity (the answer\nmay be \"nothing, we just happened to build our scripts around\nsend-pack\", and that is perfectly fine).\n\n-Peff\n"},{"id":"281696","messageId":"56F42820.5000003@assembla.com","threadId":"41798","inReplyTo":"xmqqpoujama8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Stanislav Kolotinskiy","fromEmail":"stanislav@assembla.com","sentAt":"2016-03-24T17:47:12Z","receivedAt":"2016-03-24T17:47:12Z","isPatch":true,"sender":{"key":"stanislav@assembla.com","avatar":"https://gravatar.com/avatar/dca9d54f97b9c9bb1c41538bcc64a933d063c1f8fe3cac0936a4d4b09e4c45ff?d=mp&s=160"},"body":"On 24/03/16 18:11, Junio C Hamano wrote:\n> Please see \"git shortlog --no-merges\" output from recent history and\n> notice that s/Fix/fix/ would make things more consistent.\nThanks for letting me know, I'll update that.\n> Thanks for noticing; the above explanation however does not make it\n> very clear why the symptom exhibits itself only when \"directory\" is\n> given (it also is unclear if \"target\" being a directory is special,\n> or if any remote repository specification, e.g. host:/path/to/dir,\n> triggers the same problem).\nI'll think about a better explanation and will post it here - thanks.\n\n--\nRegards,\nStanislav\n"},{"id":"281697","messageId":"56F42899.9090909@assembla.com","threadId":"41798","inReplyTo":"20160324173731.GD6341@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Stanislav Kolotinskiy","fromEmail":"stanislav@assembla.com","sentAt":"2016-03-24T17:49:13Z","receivedAt":"2016-03-24T17:49:13Z","isPatch":true,"sender":{"key":"stanislav@assembla.com","avatar":"https://gravatar.com/avatar/dca9d54f97b9c9bb1c41538bcc64a933d063c1f8fe3cac0936a4d4b09e4c45ff?d=mp&s=160"},"body":"On 24/03/16 19:37, Jeff King wrote:\n> I guess a follow-up to my question is: what does \"git send-pack\" do that\n> \"git push\" does not? Again, this is just for my curiosity (the answer\n> may be \"nothing, we just happened to build our scripts around\n> send-pack\", and that is perfectly fine).\nWell, this is some really old code (eight years old), and git version \nused at the moment was around 1.6. So I'm pretty much sure that it just \nhappened to be the decision of the developer that wrote this piece of code.\n\n--\nStanislav\n"},{"id":"281700","messageId":"20160324180223.GA9817@sigill.intra.peff.net","threadId":"41798","inReplyTo":"56F42820.5000003@assembla.com","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-24T18:02:23Z","receivedAt":"2016-03-24T18:02:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote:\n\n> >Thanks for noticing; the above explanation however does not make it\n> >very clear why the symptom exhibits itself only when \"directory\" is\n> >given (it also is unclear if \"target\" being a directory is special,\n> >or if any remote repository specification, e.g. host:/path/to/dir,\n> >triggers the same problem).\n> I'll think about a better explanation and will post it here - thanks.\n\nFeel free to steal from the explanation I posted earlier.\n\n-Peff\n"},{"id":"282441","messageId":"56FE50AB.2090109@assembla.com","threadId":"41798","inReplyTo":"20160324180223.GA9817@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-pack: Fix --all option when used with directory","fromName":"Stanislav Kolotinskiy","fromEmail":"stanislav@assembla.com","sentAt":"2016-04-01T10:42:51Z","receivedAt":"2016-04-01T10:42:51Z","isPatch":true,"sender":{"key":"stanislav@assembla.com","avatar":"https://gravatar.com/avatar/dca9d54f97b9c9bb1c41538bcc64a933d063c1f8fe3cac0936a4d4b09e4c45ff?d=mp&s=160"},"body":"Thanks a lot, Jeff, your explanation really helped!\n\n--\nRegards,\nStanislav\n\nOn 24/03/16 20:02, Jeff King wrote:\n> On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote:\n>\n>>> Thanks for noticing; the above explanation however does not make it\n>>> very clear why the symptom exhibits itself only when \"directory\" is\n>>> given (it also is unclear if \"target\" being a directory is special,\n>>> or if any remote repository specification, e.g. host:/path/to/dir,\n>>> triggers the same problem).\n>> I'll think about a better explanation and will post it here - thanks.\n> Feel free to steal from the explanation I posted earlier.\n>\n> -Peff\n"}]}