{"thread":{"id":"48079","subject":"Bad refspec messes up bundle.","startedAt":"2018-03-19T08:39:31Z","lastAt":"2018-04-03T14:38:53Z","messageCount":7,"participants":["Luciano Joublanc","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"342131","messageId":"CAO+-ZX_-COVpzqoAZLRGu4mvLMkLYiR3y7rkCtwmnvJ8kA7txQ@mail.gmail.com","threadId":"48079","inReplyTo":null,"subject":"Bad refspec messes up bundle.","fromName":"Luciano Joublanc","fromEmail":"ljoublanc@dinogroup.eu","sentAt":"2018-03-19T08:39:02Z","receivedAt":"2018-03-19T08:39:31Z","isPatch":false,"sender":{"key":"ljoublanc@dinogroup.eu","avatar":null},"body":"Yesterday I created a git bundle as best as I can remember like this\n\ngit bundle save chunk chunk.bundle --all master\n\nNote the 'master' I added accidentally at the end - this was a user\nerror but still the bundle was created.\n\nWhen I tried to clone this, I get\n\n~\\local\\src> git clone 'G:\\My Drive\\chunk.bundle' fs2-columns\nCloning into 'fs2-columns'...\nReceiving objects: 100% (31/31), done.\nResolving deltas: 100% (5/5), done.\nfatal: multiple updates for ref 'refs/remotes/origin/master' not allowed.\n~\\local\\src> git bundle verify chunk.bundle\nThe bundle contains these 3 refs:\n3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\n3c804437a5f8537db1bfb5d09b7bff4f9950605e HEAD\n3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\nThe bundle records a complete history.\nchunk.bundle is okay\n\nAfter trying a couple of things, I finally managed to clone it using\n\n~\\local\\src> git clone -b master --single-branch .\\chunk.bundle fs2-columns\n\ni.e. the '--single-branch' option saved me.\n\nIs this a bug? Should bundle allow providing multiple refspecs when\n`--all` is provided? I admit this was clearly a case of 'caveat\nemptor', but shouldn't this be disallowed (i.e. is there any situation\nwhen this is useful?)\n\nThanks!\n\nLuciano\n\n-- \nThis message is intended only for the personal and confidential use of the \ndesignated recipient(s) named above.  If you are not the intended recipient \nof this message you are hereby notified that any review, dissemination, \ndistribution or copying of this message is strictly prohibited.  This \ncommunication is for information purposes only and should not be regarded \nas an offer to sell or as a solicitation of an offer to buy any financial \nproduct, an official confirmation of any transaction, or as an official \nstatement of the Dinosaur Group.  Email transmission cannot be guaranteed \nto be secure or error-free.  Therefore, we do not represent that this \ninformation is complete or accurate and it should not be relied upon as \nsuch.  All information is subject to change without notice.\n"},{"id":"342215","messageId":"xmqq370wvugk.fsf@gitster-ct.c.googlers.com","threadId":"48079","inReplyTo":"CAO+-ZX_-COVpzqoAZLRGu4mvLMkLYiR3y7rkCtwmnvJ8kA7txQ@mail.gmail.com","subject":"Re: Bad refspec messes up bundle.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-19T17:36:11Z","receivedAt":"2018-03-19T17:36:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luciano Joublanc <ljoublanc@dinogroup.eu> writes:\n\n> Yesterday I created a git bundle as best as I can remember like this\n>\n> git bundle save chunk chunk.bundle --all master\n>\n> Note the 'master' I added accidentally at the end - this was a user\n> error but still the bundle was created.\n>\n> When I tried to clone this, I get\n>\n> ~\\local\\src> git clone 'G:\\My Drive\\chunk.bundle' fs2-columns\n> Cloning into 'fs2-columns'...\n> Receiving objects: 100% (31/31), done.\n> Resolving deltas: 100% (5/5), done.\n> fatal: multiple updates for ref 'refs/remotes/origin/master' not allowed.\n> ~\\local\\src> git bundle verify chunk.bundle\n> The bundle contains these 3 refs:\n> 3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\n> 3c804437a5f8537db1bfb5d09b7bff4f9950605e HEAD\n> 3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\n> The bundle records a complete history.\n> chunk.bundle is okay\n>\n> After trying a couple of things, I finally managed to clone it using\n>\n> ~\\local\\src> git clone -b master --single-branch .\\chunk.bundle fs2-columns\n>\n> i.e. the '--single-branch' option saved me.\n>\n> Is this a bug? Should bundle allow providing multiple refspecs when\n> `--all` is provided? I admit this was clearly a case of 'caveat\n> emptor', but shouldn't this be disallowed (i.e. is there any situation\n> when this is useful?)\n\nThanks for a report.\n\nJust like a remote repository that reports the same ref more than\nonce in its initial advertisement (i.e. \"git ls-remote $remote\"\ngives duplicate entries), a bundle file that records the same ref\nmore than once *is* a bug, I would think.\n\nA \"git bundle create\" command that creates such a bundle file\nshouldn't.  It is not very useful to diagnose it as an error; it\nprobably makes more sense to dedup the refs instead when writing the\nbundle file.  Of course, we should abort with an error *if* the code\never tries to store the same ref twice with different object name\n(i.e. attempt to dedup, in vain).\n\nAlso, \"git clone\" from such a bundle file (or for that matter, a\nremote repository that advertises the same ref twice) probably\nshould do a similar deduping, with a warning message.\n\n"},{"id":"343396","messageId":"nycvar.QRO.7.76.6.1803301102430.5026@qfpub.tvgsbejvaqbjf.bet","threadId":"48079","inReplyTo":"xmqq370wvugk.fsf@gitster-ct.c.googlers.com","subject":"Re: Bad refspec messes up bundle.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-30T10:20:21Z","receivedAt":"2018-03-30T10:20:33Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Luciano,\n\nOn Mon, 19 Mar 2018, Junio C Hamano wrote:\n\n> Luciano Joublanc <ljoublanc@dinogroup.eu> writes:\n> \n> > Yesterday I created a git bundle as best as I can remember like this\n> >\n> > git bundle save chunk chunk.bundle --all master\n> >\n> > Note the 'master' I added accidentally at the end - this was a user\n> > error but still the bundle was created.\n> >\n> > When I tried to clone this, I get\n> >\n> > ~\\local\\src> git clone 'G:\\My Drive\\chunk.bundle' fs2-columns\n> > Cloning into 'fs2-columns'...\n> > Receiving objects: 100% (31/31), done.\n> > Resolving deltas: 100% (5/5), done.\n> > fatal: multiple updates for ref 'refs/remotes/origin/master' not allowed.\n> > ~\\local\\src> git bundle verify chunk.bundle\n> > The bundle contains these 3 refs:\n> > 3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\n> > 3c804437a5f8537db1bfb5d09b7bff4f9950605e HEAD\n> > 3c804437a5f8537db1bfb5d09b7bff4f9950605e refs/heads/master\n> > The bundle records a complete history.\n> > chunk.bundle is okay\n> >\n> > After trying a couple of things, I finally managed to clone it using\n> >\n> > ~\\local\\src> git clone -b master --single-branch .\\chunk.bundle fs2-columns\n> >\n> > i.e. the '--single-branch' option saved me.\n> >\n> > Is this a bug? Should bundle allow providing multiple refspecs when\n> > `--all` is provided? I admit this was clearly a case of 'caveat\n> > emptor', but shouldn't this be disallowed (i.e. is there any situation\n> > when this is useful?)\n> \n> Thanks for a report.\n> \n> Just like a remote repository that reports the same ref more than\n> once in its initial advertisement (i.e. \"git ls-remote $remote\"\n> gives duplicate entries), a bundle file that records the same ref\n> more than once *is* a bug, I would think.\n> \n> A \"git bundle create\" command that creates such a bundle file\n> shouldn't.  It is not very useful to diagnose it as an error; it\n> probably makes more sense to dedup the refs instead when writing the\n> bundle file.  Of course, we should abort with an error *if* the code\n> ever tries to store the same ref twice with different object name\n> (i.e. attempt to dedup, in vain).\n> \n> Also, \"git clone\" from such a bundle file (or for that matter, a\n> remote repository that advertises the same ref twice) probably\n> should do a similar deduping, with a warning message.\n\nI agree that it is a bug if a bundle file records a ref multiple times.\nLuciano, here are some pointers so you can fix it:\n\n- probably the best way to start would be to add a new test case to\n  t/t5607-clone-bundle.sh. The script *should* be relatively easy to\n  understand and imitate. The new test case would probably look somewhat\n  like this:\n\n\ttest_expect_failure 'bundles must not contain multiple refs' '\n\t\tgit bundle create all.bundle --all master &&\n\t\tgrep master all.bundle >master.lines &&\n\t\ttest_line_count = 1 master.lines\n\t'\n\n- then, ensure that this test passes and reports this correctly as a known\n  breakage.\n\n- now it is time to fix the actual bug. The code in question is the\n  write_bundle_refs() function in bundle.c (careful, there are two files\n  of that name in Git's source code, one in the builtin/ subdirectory, the\n  other one in the toplevel directory, you will want to look at the\n  latter one).\n\n- notice that there is already a construct in the loop over the pending\n  refs where some are skipped:\n\n\t\tif (e->item->flags & UNINTERESTING)\n\t\t\tcontinue;\n\n  The \"uninteresting\" refs would be those on the excluded end of a commit\n  range, e.g. if you called `git bundle create x.bundle master..next`, the\n  `master` ref would be such an \"uninteresting\" ref.\n\n- one might be tempted to introduce another flag and set it once a ref was\n  shown and skip a ref for which that flag is set. And indeed, this was my\n  own first thought!\n\n  However, this would be incorrect, as the flags are stored with the\n  *commit*, not with the ref. So if two refs point to the same commit,\n  that new code would skip the second one by mistake!\n\n  By the way, this makes me think that there is another very real bug in\n  the bundle code, in the part I showed above. Suppose you have a `master`\n  and a `next` ref, and both point at the same commit, then you would want\n  `git bundle create next.bundle master..next` to list `next`, don't you\n  think? This one would be a lot harder to fix, though, as the\n  setup_revisions() function really only sets the flag on the respective\n  commits because it wants to set things up for revision walking, not\n  necessarily to list \"interesting\" refs. A possible solution might be to\n  abuse the `mode` field in the object_array (which is the type of the\n  `pending` field in the `rev_info` struct where the parsed command-line\n  parameters are held, when that `mode` makes only sense for blobs and\n  we store refs pointing to commits or tags).\n\n  That's a side-track, though.\n\n- most likely, the best way to avoid duplicate refs entries is to use the\n  actual ref name and put it into a hash set. Luckily, we do have code\n  for this, and examples how to use it, too. See e.g. fc65b00da7e\n  (merge-recursive: change current file dir string_lists to hashmap,\n  2017-09-07). So you would define something like\n\n\tstruct ref_name_entry {\n\t\tstruct hashmap_entry e; /* must be first field */\n\t\tchar ref_name[FLEX_ARRAY];\n\t}\n\n  and implement the cmp function similar to the path_hashmap_cmp() in\n  merge-recursive.c, then initialize a hashset in write_bundle_refs(),\n  add handled ref names via\n\n\tFLEX_ALLOC_STR(entry, ref_name, e->name);\n\thashmap_entry_init(entry, strhash(e->name));\n\thashmap_add(&shown_refs, entry);\n\n  and look up whether a ref should be skipped via\n\n\tif (hashmap_get_from_hash(&shown_refs, strhash(e->name), e->name))\n\t\tcontinue;\n\n  Oh, and before returning from write_bundle_refs(), the hashset\n  (including its entries) should be released via\n\n\thashmap_free(&shown_refs, 1);\n\n\nThese hints should get you started on this project.\n\nLooking forward to your contribution,\nJohannes\n"},{"id":"343439","messageId":"xmqq370h4h4b.fsf@gitster-ct.c.googlers.com","threadId":"48079","inReplyTo":"nycvar.QRO.7.76.6.1803301102430.5026@qfpub.tvgsbejvaqbjf.bet","subject":"Re: Bad refspec messes up bundle.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T17:18:12Z","receivedAt":"2018-03-30T17:18:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Luciano,\n>\n>> > Is this a bug? Should bundle allow providing multiple refspecs when\n> ...\n> I agree that it is a bug if a bundle file records a ref multiple times.\n> Luciano, here are some pointers so you can fix it:\n>\n> - probably the best way to start would be to add a new test case to\n>   t/t5607-clone-bundle.sh. The script *should* be relatively easy to\n>   understand and imitate. The new test case would probably look somewhat\n>   like this:\n>\n> \ttest_expect_failure 'bundles must not contain multiple refs' '\n\ns/multiple/duplicate/.  It is not unusual for a bundle to record\nmore than one ref; it is (1) useless and harmful to unsuspecting\nclients to record the same ref twice with the same value and (2)\nnonesnse to record the same ref twice with different value.\n\nOther than that, the outline seems to go in the right general\ndirection.\n\n"},{"id":"343459","messageId":"nycvar.QRO.7.76.6.1803302056570.5026@qfpub.tvgsbejvaqbjf.bet","threadId":"48079","inReplyTo":"xmqq370h4h4b.fsf@gitster-ct.c.googlers.com","subject":"Re: Bad refspec messes up bundle.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-30T18:58:23Z","receivedAt":"2018-03-30T18:58:34Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 30 Mar 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> > Is this a bug? Should bundle allow providing multiple refspecs when\n> > ...\n> > I agree that it is a bug if a bundle file records a ref multiple\n> > times.  Luciano, here are some pointers so you can fix it:\n> >\n> > - probably the best way to start would be to add a new test case to\n> >   t/t5607-clone-bundle.sh. The script *should* be relatively easy to\n> >   understand and imitate. The new test case would probably look somewhat\n> >   like this:\n> >\n> > \ttest_expect_failure 'bundles must not contain multiple refs' '\n> \n> s/multiple/duplicate/.  It is not unusual for a bundle to record\n> more than one ref; it is (1) useless and harmful to unsuspecting\n> clients to record the same ref twice with the same value and (2)\n> nonesnse to record the same ref twice with different value.\n\nOf course! Thanks for pointing this out.\n\n> Other than that, the outline seems to go in the right general\n> direction.\n\nExcellent.\n\nLuciano, the ball is in your court now. If you get stuck with anything\n(such as getting started with building Git), please let us know. We can\nhelp.\n\nCiao,\nJohannes\n"},{"id":"343501","messageId":"CAO+-ZX-DvjsOnpvfPuLkx2w2cR5FDb6Ww8xEyuZHMmC57=b2yQ@mail.gmail.com","threadId":"48079","inReplyTo":"nycvar.QRO.7.76.6.1803301102430.5026@qfpub.tvgsbejvaqbjf.bet","subject":"Re: Bad refspec messes up bundle.","fromName":"Luciano Joublanc","fromEmail":"ljoublanc@dinogroup.eu","sentAt":"2018-03-31T08:50:17Z","receivedAt":"2018-03-31T08:50:45Z","isPatch":false,"sender":{"key":"ljoublanc@dinogroup.eu","avatar":null},"body":"Hi Johannes,\n\nWith such a comprehensive reply, I would feel guilty not making a\ncontribution now :) Be forewarned though, It's been about ten years\nsince I wrote anything in `C`!\n\nI've cloned the `maint` branch, built the project, and added the test\nas you suggested - it's failing as expected.\n\nI'm somewhat confused though. I think it's m limited understanding of\n'ref' and 'commit'.\n\nOn 30 March 2018 at 11:20, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n>\n>   However, this would be incorrect, as the flags are stored with the\n>   *commit*, not with the ref. So if two refs point to the same commit,\n>   that new code would skip the second one by mistake!\n\n\nIsn't that the point here? to deduplicate commits?  My limited\nunderstanding is that at a 'ref' is like an alias or pointer to a\ncommit.\n\n>\n>   By the way, this makes me think that there is another very real bug in\n>   the bundle code, in the part I showed above. Suppose you have a `master`\n>   and a `next` ref, and both point at the same commit, then you would want\n>   `git bundle create next.bundle master..next` to list `next`, don't you\n>   think?\n\n\nDoesn't this contradict what you just said, that we don't want to skip\nrefs with the same commit #?\n\nIn fact, if you look in the calling function, there is a\n`    object_array_remove_duplicates(&revs.pending);`\nWhich to the best of my understanding removes duplicate refs (not\ncommits). However, I suspect this doesn't cover the `--all` case as\nit's a switch rather than a revspec? Would that be right?\n\n>\n>\n> - most likely, the best way to avoid duplicate refs entries is to use the\n>   actual ref name and put it into a hash set. Luckily, we do have code\n>   for this, and examples how to use it, too. See e.g. fc65b00da7e\n>   (merge-recursive: change current file dir string_lists to hashmap,\n>   2017-09-07). So you would define something like\n>\n\nSeparately, if I do end up including the hashmap code, it should be\nrefactored out into it's own file, right?\n\nThanks again,\n\nLuciano\n\n-- \nThis message is intended only for the personal and confidential use of the \ndesignated recipient(s) named above.  If you are not the intended recipient \nof this message you are hereby notified that any review, dissemination, \ndistribution or copying of this message is strictly prohibited.  This \ncommunication is for information purposes only and should not be regarded \nas an offer to sell or as a solicitation of an offer to buy any financial \nproduct, an official confirmation of any transaction, or as an official \nstatement of the Dinosaur Group.  Email transmission cannot be guaranteed \nto be secure or error-free.  Therefore, we do not represent that this \ninformation is complete or accurate and it should not be relied upon as \nsuch.  All information is subject to change without notice.\n"},{"id":"343697","messageId":"nycvar.QRO.7.76.6.1804031610550.5026@qfpub.tvgsbejvaqbjf.bet","threadId":"48079","inReplyTo":"CAO+-ZX-DvjsOnpvfPuLkx2w2cR5FDb6Ww8xEyuZHMmC57=b2yQ@mail.gmail.com","subject":"Re: Bad refspec messes up bundle.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-03T14:38:40Z","receivedAt":"2018-04-03T14:38:53Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Luciano,\n\nOn Sat, 31 Mar 2018, Luciano Joublanc wrote:\n\n> I've cloned the `maint` branch, built the project, and added the test\n> as you suggested - it's failing as expected.\n\nExcellent.\n\n> On 30 March 2018 at 11:20, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> >   However, this would be incorrect, as the flags are stored with the\n> >   *commit*, not with the ref. So if two refs point to the same commit,\n> >   that new code would skip the second one by mistake!\n> \n> Isn't that the point here? to deduplicate commits?  My limited\n> understanding is that at a 'ref' is like an alias or pointer to a\n> commit.\n\nThe point is to deduplicate refs, not commits ;-)\n\nImagine that you have a git.git clone, and then you work on some topic,\nsay, `bundle-refs` and then call `git checkout -b bundle-refs-wip` from\nthere. If you now say\n\n\tgit bundle create wip.bundle bundle-refs bundle-refs-wip\n\nyou will want both bundle-refs and bundle-refs-wip to show up in that\nbundle, not just bundle-refs, even if both refs point at the same commit.\n\n> >   By the way, this makes me think that there is another very real bug\n> >   in the bundle code, in the part I showed above. Suppose you have a\n> >   `master` and a `next` ref, and both point at the same commit, then\n> >   you would want `git bundle create next.bundle master..next` to list\n> >   `next`, don't you think?\n> \n> Doesn't this contradict what you just said, that we don't want to skip\n> refs with the same commit #?\n\nI would rather be able to generate such a wip.bundle as outlined above\nwhere calling `git ls-remote wip.bundle` would list *both* refs, with the\nsame commit.\n\n> In fact, if you look in the calling function, there is a\n> `    object_array_remove_duplicates(&revs.pending);`\n> Which to the best of my understanding removes duplicate refs (not\n> commits). However, I suspect this doesn't cover the `--all` case as\n> it's a switch rather than a revspec? Would that be right?\n\nOh, I missed that!\n\nAnd I also missed that this is implemented with something *else* than a\nhashmap, so it won't have linear complexity but instead quadratic. Gross.\n\nBut you got an interesting nugget there, as it indeed tries to\ndeduplicate, but not by object ID, otherwise the bug you reported would\nnot occur (but other bugs, as I outlined above).\n\nInstead, the object_array_remove_duplicates() code does this:\n\n-- snip --\nvoid object_array_remove_duplicates(struct object_array *array)\n{\n        unsigned nr = array->nr, src;\n        struct object_array_entry *objects = array->objects;\n\n        array->nr = 0;\n        for (src = 0; src < nr; src++) {\n                if (!contains_name(array, objects[src].name)) {\n                        if (src != array->nr)\n                                objects[array->nr] = objects[src];\n                        array->nr++;\n                } else {\n                        object_array_release_entry(&objects[src]);\n                }\n        }\n}\n-- snap --\n\nAnd indeed, the `contains_name()` function iterates through all of the\nre-added entries and compares the *name*.\n\nRunning this in a debugger shows the culprit, too: there is a\n`refs/heads/master`, a `HEAD` and a `master`. Note how the last entry\n(which was taken directly from the command-line arguments) lacks the\n`refs/heads/` prefix? *That* is the culprit...\n\n> > - most likely, the best way to avoid duplicate refs entries is to use the\n> >   actual ref name and put it into a hash set. Luckily, we do have code\n> >   for this, and examples how to use it, too. See e.g. fc65b00da7e\n> >   (merge-recursive: change current file dir string_lists to hashmap,\n> >   2017-09-07). So you would define something like\n> \n> Separately, if I do end up including the hashmap code, it should be\n> refactored out into it's own file, right?\n\nI do not think that is necessary. Personally, I'd just add the hashmap as\nlocal variable to `write_bundle_refs()`, initialize it before the loop, add\nthe struct for the hashmap entry and the _cmp function as file-local (i.e.\n`static`) function before `write_bundle_refs()`, then add all shown refs\n(as stored in the `display_ref` variable) to the hashmap, and add another\nconditional `goto skip_write_ref` after all the others, contingent on\n`display_ref` *not* being found in the hashmap via\n`hashmap_get_from_hash(&displayed_refs, strhash(display_ref),\ndisplay_ref)`.\n\nIn the same run, I would remove that `object_array_remove_duplicates()`\nfunction altogether, as its only caller is now no longer necessary.\n\nCiao,\nJohannes\n"}]}