{"thread":{"id":"31983","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","startedAt":"2012-10-30T18:12:24Z","lastAt":"2012-10-31T02:22:40Z","messageCount":29,"participants":["Sverre Rabbelier","Felipe Contreras","Jonathan Nieder","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"202216","messageId":"CAGdFq_j1RROOwxDi1FfJZJ6wiP9y9FWzSpc7MXVSvRmgk0sF9A@mail.gmail.com","threadId":"31983","inReplyTo":"1351617089-13036-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-30T18:12:24Z","receivedAt":"2012-10-30T18:12:24Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras\n wrote:\n> When an object has already been exported (and thus is in the marks) it\n> is flagged as SHOWN, so it will not be exported again, even if this time\n> it's exported through a different ref.\n>\n> We don't need the object to be exported again, but we want the ref\n> updated, which doesn't happen.\n>\n> Since we can't know if a ref was exported or not, let's just assume that\n> if the commit was marked (flags & SHOWN), the user still wants the ref\n> updated.\n>\n> So:\n>\n> % git branch test master\n> % git fast-export $mark_flags master\n> % git fast-export $mark_flags test\n>\n> Would export 'test' properly.\n>\n> Additionally, this fixes issues with remote helpers; now they can push\n> refs wich objects have already been exported.\n\nWon't this also export child (or maybe parent) branches that weren't\nmentioned? For example:\n\n$ git branch one\n$ echo foo > content\n$ git commit -m two\n$ git fast-export one\n$ git fast-export two\n\nI suspect that one of those will export both one and two. If not, this\nseems like a great solution to the fast-export problem.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"202221","messageId":"CAMP44s3MHrG_XeZEodnxemrW-V18+NHnFvi7koyx9mH8XuHc6w@mail.gmail.com","threadId":"31983","inReplyTo":"CAGdFq_j1RROOwxDi1FfJZJ6wiP9y9FWzSpc7MXVSvRmgk0sF9A@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T18:47:42Z","receivedAt":"2012-10-30T18:47:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 7:12 PM, Sverre Rabbelier wrote:\n> On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras\n> wrote:\n>> When an object has already been exported (and thus is in the marks) it\n>> is flagged as SHOWN, so it will not be exported again, even if this time\n>> it's exported through a different ref.\n>>\n>> We don't need the object to be exported again, but we want the ref\n>> updated, which doesn't happen.\n>>\n>> Since we can't know if a ref was exported or not, let's just assume that\n>> if the commit was marked (flags & SHOWN), the user still wants the ref\n>> updated.\n>>\n>> So:\n>>\n>> % git branch test master\n>> % git fast-export $mark_flags master\n>> % git fast-export $mark_flags test\n>>\n>> Would export 'test' properly.\n>>\n>> Additionally, this fixes issues with remote helpers; now they can push\n>> refs wich objects have already been exported.\n>\n> Won't this also export child (or maybe parent) branches that weren't\n> mentioned? For example:\n>\n> $ git branch one\n> $ echo foo > content\n> $ git commit -m two\n> $ git fast-export one\n> $ git fast-export two\n>\n> I suspect that one of those will export both one and two. If not, this\n> seems like a great solution to the fast-export problem.\n\nWhy would it? We are not changing the way objects are exported, the\nonly difference is what happens at the end\n(handle_tags_and_duplicates()).\n\nAnd if you are talking about the ref for the reset at the end, it has\nto be both in the list of refs selected by the user (initially in\n&revs.pending), either marked or the object already referenced by\nanother ref in the list selected by the user (e.g. fast-export one\ntwo, where one^{commit} == two^{commit}, and not marked as\nUNINTERESTING (e.g. ^two).\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202222","messageId":"20121030185542.GF15167@elie.Belkin","threadId":"31983","inReplyTo":"1351617089-13036-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/4] fast-export: trivial cleanup","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T18:55:43Z","receivedAt":"2012-10-30T18:55:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Setting commit to commit is a no-op.\n\nWrong description. This should say:\n\n\tThe code uses the idiom of assigning commit to itself to quench a\n\t\"may be used uninitialized\" warning. Luckily at least modern\n\tversions of gcc do not produce that warning here, so we can drop\n\tthe self-assignment.\n\n\tThis makes the code clearer to human beings, makes static\n\tanalyzers that do not know that idiom happier, and means that\n\tif the code some day evolves to use this variable uninitialized\n\tthen we will catch it.\n\n> Signed-off-by: Felipe Contreras \n\nWith that change, for what it's worth,\nReviewed-by: Jonathan Nieder \n\nPatch left unsnipped since it doesn't seem to have hit the list.\n\n> ---\n> builtin/fast-export.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 12220ad..065f324 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -483,7 +483,7 @@ static void get_tags_and_duplicates(struct object_array *pending,\n> \tfor (i = 0; i < pending->nr; i++) {\n> \t\tstruct object_array_entry *e = pending->objects + i;\n> \t\tunsigned char sha1[20];\n> -\t\tstruct commit *commit = commit;\n> +\t\tstruct commit *commit;\n> \t\tchar *full_name;\n> \n> \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n"},{"id":"202223","messageId":"20121030185731.GH15167@elie.Belkin","threadId":"31983","inReplyTo":"1351617089-13036-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/4] fast-export: fix comparisson in tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T18:57:31Z","receivedAt":"2012-10-30T18:57:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(actually cc-ing the git list this time. Sorry for the noise, all.)\nFelipe Contreras wrote:\n\n> [Subject: [PATCH v2 2/4] fast-export: fix comparisson in tests]\n>\n> First the expected, then the actual, otherwise the diff would be the\n> opposite of what we want.\n\nSpelling: s/comparisson/comparison/.\n\nSemantics: this isn't actually fixing anything --- it's a cosmetic\nthing. It would be clearer to say:\n\n\tfast-export test: swap arguments to test_cmp\n\n\tThis way if diff output is produced, it describes how the\n\tactual output differs from what was expected rather than the\n\tother way around.\n\n> Signed-off-by: Felipe Contreras \n\nFor what it's worth, with amended message,\nReviewed-by: Jonathan Nieder \n\nPatch left unsnipped because it hadn't hit the list.\n\n> ---\n> t/t9350-fast-export.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 3e821f9..49bdb44 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -303,7 +303,7 @@ test_expect_success 'dropping tag of filtered out object' '\n> (\n> \tcd limit-by-paths &&\n> \tgit fast-export --tag-of-filtered-object=drop mytag -- there > output &&\n> -\ttest_cmp output expected\n> +\ttest_cmp expected output\n> )\n> '\n> \n> @@ -320,7 +320,7 @@ test_expect_success 'rewriting tag of filtered out object' '\n> (\n> \tcd limit-by-paths &&\n> \tgit fast-export --tag-of-filtered-object=rewrite mytag -- there > output &&\n> -\ttest_cmp output expected\n> +\ttest_cmp expected output\n> )\n> '\n> \n> @@ -351,7 +351,7 @@ test_expect_failure 'no exact-ref revisions included' '\n> \t(\n> \t\tcd limit-by-paths &&\n> \t\tgit fast-export master~2..master~1 > output &&\n> -\t\ttest_cmp output expected\n> +\t\ttest_cmp expected output\n> \t)\n> '\n> \n> -- \n> 1.8.0\n"},{"id":"202224","messageId":"20121030185914.GI15167@elie.Belkin","threadId":"31983","inReplyTo":"1351617089-13036-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T18:59:14Z","receivedAt":"2012-10-30T18:59:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> They have been marked as UNINTERESTING for a reason, lets respect that.\n\nThis patch looks unsafe, and in the examples listed in the patch\ndescription the changed behavior does not look like an improvement.\nWorse, the description lists a few examples but gives no convincing\nexplanation to reassure about the lack of bad behavior for examples\nnot listed.\n\nPerhaps this patch has a prerequisite and has come out of order.\n\nHope that helps,\nJonathan\n\nPatch left unsnipped so we can get a copy in the list archive.\n\n> Currently the first ref is handled properly, but not the rest, so:\n> \n> % git fast-export master ^master\n> \n> Would currently throw a reset for master (2nd ref), which is not what we\n> want.\n> \n> % git fast-export master ^foo ^bar ^roo\n> % git fast-export master salsa..tacos\n> \n> Even if all these refs point to the same object; foo, bar, roo, salsa,\n> and tacos would all get a reset.\n> \n> This is most certainly not what we want. After this patch, nothing gets\n> exported, because nothing was selected (everything is UNINTERESTING).\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/fast-export.c | 7 ++++---\n> t/t9350-fast-export.sh | 6 ++++++\n> 2 files changed, 10 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 065f324..7fb6fe1 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array *pending,\n> \t\t\t\ttypename(e->item->type));\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tif (commit->util)\n> +\t\tif (commit->util) {\n> \t\t\t/* more than one name for the same object */\n> -\t\t\tstring_list_append(extra_refs, full_name)->util = commit;\n> -\t\telse\n> +\t\t\tif (!(commit->object.flags & UNINTERESTING))\n> +\t\t\t\tstring_list_append(extra_refs, full_name)->util = commit;\n> +\t\t} else\n> \t\t\tcommit->util = full_name;\n> \t}\n> }\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 49bdb44..6ea8f6f 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '\n> \t)\n> '\n> \n> +test_expect_success 'proper extra refs handling' '\n> +\tgit fast-export master ^master master..master > actual &&\n> +\techo -n > expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n> -- \n> 1.8.0\n"},{"id":"202231","messageId":"CAMP44s3LP65XOYFg-tBe_rzT1+gXp=714C-u14mkwxY26r4b=g@mail.gmail.com","threadId":"31983","inReplyTo":"20121030185914.GI15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:17:53Z","receivedAt":"2012-10-30T19:17:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"(again to the mailing list)\n\nOn Tue, Oct 30, 2012 at 7:59 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> They have been marked as UNINTERESTING for a reason, lets respect that.\n\nThat doesn't say anything.\n\n> and in the examples listed in the patch\n> description the changed behavior does not look like an improvement.\n\nI disagree.\n\n% git log master ^master\n\nWhat do you expect? Nothing.\n\n% git fast-export master ^master\n\nWhat do you expect? Nothing.\n\n> Worse, the description lists a few examples but gives no convincing\n> explanation to reassure about the lack of bad behavior for examples\n> not listed.\n\nWhat examples not listed?\n\n-- \nFelipe Contreras\n"},{"id":"202233","messageId":"alpine.DEB.1.00.1210302040560.7256@s15462909.onlinehome-server.info","threadId":"31983","inReplyTo":"CAMP44s1W4mwK+cNwBqu2S0=Aw04XX9KBan8w4ghyzqbODdmiLQ@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-10-30T19:41:37Z","receivedAt":"2012-10-30T19:41:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Felipe,\n\nOn Tue, 30 Oct 2012, Felipe Contreras wrote:\n\n> On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote:\n> > Felipe Contreras wrote:\n> >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder wrote:\n> >\n> >>> and in the examples listed in the patch\n> >>> description the changed behavior does not look like an improvement.\n> >>\n> >> I disagree.\n> >>\n> >> % git log master ^master\n> >>\n> >> What do you expect? Nothing.\n> >\n> > Yep.\n> >\n> >> % git fast-export master ^master\n> >>\n> >> What do you expect? Nothing.\n> >\n> > Nope.\n> \n> That's _your_ opinion. I would like to see what others think.\n\nIf you wanted to prove that you can work with others without offending\nthem, I think that failed.\n\nCiao,\nJohannes\n"},{"id":"202237","messageId":"CAGdFq_jJwZMLq=3co13hs7gas6y9kZRTKwcT+CP=n6-24Uv5Og@mail.gmail.com","threadId":"31983","inReplyTo":"CAMP44s3MHrG_XeZEodnxemrW-V18+NHnFvi7koyx9mH8XuHc6w@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-30T21:17:57Z","receivedAt":"2012-10-30T21:17:57Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras\n wrote:\n> Why would it? We are not changing the way objects are exported, the\n> only difference is what happens at the end\n> (handle_tags_and_duplicates()).\n\nBecause the marking is per-commit, not per-ref, right? Perhaps you\ncould add a simple test case to make sure it works as expected?\nSomething along the lines of the scenario I described in my previous\nemail?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"202244","messageId":"CAMP44s2QwdZKqJq0BZ5HOtZYiCMxCxycui9EmxxfL+Sa6M_6+g@mail.gmail.com","threadId":"31983","inReplyTo":"CAGdFq_jJwZMLq=3co13hs7gas6y9kZRTKwcT+CP=n6-24Uv5Og@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T21:35:51Z","receivedAt":"2012-10-30T21:35:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier wrote:\n> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras\n> wrote:\n>> Why would it? We are not changing the way objects are exported, the\n>> only difference is what happens at the end\n>> (handle_tags_and_duplicates()).\n>\n> Because the marking is per-commit, not per-ref, right?\n\nOh, you meant using marks?\n\nIt doesn't matter anyway, because get_tags_and_duplicates() would get\n'one' on the first run, and 'two' on the second.\n\nIf you meant something like this:\n% git fast-export $marks_args one\n% git fast-export $marks_args one two\n\nThen yeah, 'one' will be updated once again in the second command, but\nthere's nothing fatal about it, and your patch series had the same\nresult.\n\n> Perhaps you\n> could add a simple test case to make sure it works as expected?\n> Something along the lines of the scenario I described in my previous\n> email?\n\nI'm not sure what that test should be doing.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202245","messageId":"20121030213827.GM15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s2QwdZKqJq0BZ5HOtZYiCMxCxycui9EmxxfL+Sa6M_6+g@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T21:38:28Z","receivedAt":"2012-10-30T21:38:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> % git fast-export $marks_args one\n> % git fast-export $marks_args one two\n>\n> Then yeah, 'one' will be updated once again in the second command,\n\nThat's probably worth a mention in the commit message and tests\n(test_expect_failure), to save future readers from some confusion.\n\nThanks,\nJonathan\n"},{"id":"202246","messageId":"CAMP44s1tFhh3Xqe9tqoDAdtwnGc=kFT6OmAreeP1nbTstweaQQ@mail.gmail.com","threadId":"31983","inReplyTo":"CAMP44s3LP65XOYFg-tBe_rzT1+gXp=714C-u14mkwxY26r4b=g@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T21:40:12Z","receivedAt":"2012-10-30T21:40:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder wrote:\n>\n>>> and in the examples listed in the patch\n>>> description the changed behavior does not look like an improvement.\n>>\n>> I disagree.\n>>\n>> % git log master ^master\n>>\n>> What do you expect? Nothing.\n>\n> Yep.\n>\n>> % git fast-export master ^master\n>>\n>> What do you expect? Nothing.\n\nSo you think what we have now is the correct behavior:\n\n% git fast-export master ^master\nreset refs/heads/master\nfrom :0\n\nThat of course would crash fast-import. But hey, it's your opinion.\n\nWould be interesting to see if other people think the above is correct.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202247","messageId":"CAMP44s0dxW_FGGFQF_gDduFcGr3xri41SF0mvTjxbN-jbYWZ0w@mail.gmail.com","threadId":"31983","inReplyTo":"20121030213827.GM15167@elie.Belkin","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T21:41:55Z","receivedAt":"2012-10-30T21:41:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 10:38 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> % git fast-export $marks_args one\n>> % git fast-export $marks_args one two\n>>\n>> Then yeah, 'one' will be updated once again in the second command,\n>\n> That's probably worth a mention in the commit message and tests\n> (test_expect_failure), to save future readers from some confusion.\n\nIt is mentioned in the commit message.\n\n-- \nFelipe Contreras\n"},{"id":"202250","messageId":"20121030214531.GN15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s1tFhh3Xqe9tqoDAdtwnGc=kFT6OmAreeP1nbTstweaQQ@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T21:45:31Z","receivedAt":"2012-10-30T21:45:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> So you think what we have now is the correct behavior:\n>\n> % git fast-export master ^master\n> reset refs/heads/master\n> from :0\n\nNo, I don't think that, either.\n\nHope that helps,\nJonathan\n"},{"id":"202252","messageId":"CAGdFq_h3L-1rPvb=dSYeXqEea+f+g2kRHp7aAjaU-AxjZHB7dQ@mail.gmail.com","threadId":"31983","inReplyTo":"CAMP44s2QwdZKqJq0BZ5HOtZYiCMxCxycui9EmxxfL+Sa6M_6+g@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-30T21:59:57Z","receivedAt":"2012-10-30T21:59:57Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras\n wrote:\n> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier wrote:\n>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras\n>> wrote:\n>>> Why would it? We are not changing the way objects are exported, the\n>>> only difference is what happens at the end\n>>> (handle_tags_and_duplicates()).\n>>\n>> Because the marking is per-commit, not per-ref, right?\n>\n> Oh, you meant using marks?\n\nNo, I meant the 'SHOWN' flag, doesn't it get added per commit, not per\nref? That is, commit->object.flags & SHOWN refers to the object\nunderlying the ref. So I suspect this scenario doesn't pass the tests:\n\ngit init &&\necho first > content &&\ngit add content &&\ngit commit -m \"first\" &&\ngit branch first &&\necho two > content &&\ngit commit -m \"second\" &&\ngit branch second &&\ngit fast-export first > actual &&\ntest_cmp actual expected_first &&\ngit fast-export second > actual &&\ntest_cmp actual expected_second\n\nWith expected_first being something like:\n\n\n\nAnd expected_second being something like\n\n\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"202253","messageId":"CAMP44s1b+E8a0kdSgREbGzRTFy+nCw4VcjHadd3soQAXRkNzZw@mail.gmail.com","threadId":"31983","inReplyTo":"20121030214531.GN15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T22:01:47Z","receivedAt":"2012-10-30T22:01:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> So you think what we have now is the correct behavior:\n>>\n>> % git fast-export master ^master\n>> reset refs/heads/master\n>> from :0\n>\n> No, I don't think that, either.\n\nWell, that's what we have now, and you want to preserve this \"feature\"\n(aka bug), right?\n\nAnd I still haven't why this is \"unsafe\", and what are those \"examples\nnot listed\".\n\n-- \nFelipe Contreras\n"},{"id":"202255","messageId":"20121030220717.GO15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s1b+E8a0kdSgREbGzRTFy+nCw4VcjHadd3soQAXRkNzZw@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T22:07:17Z","receivedAt":"2012-10-30T22:07:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Well, that's what we have now, and you want to preserve this \"feature\"\n> (aka bug), right?\n\nNope. I just don't want regressions, and found a patch description\nthat did nothing to explain to the reader how it avoids regressions\nmore than a little disturbing.\n\nI also think the proposed behavior is wrong.\n\nI don't think there's much benefit to be gained from continuing to\ndiscuss this. Consider it a single data point: I would be deeply\nworried if this patch were applied without at least a clearer\ndescription of the change in behavior. Maybe I'm the only one!\n\nHope that helps,\nJonathan\n"},{"id":"202258","messageId":"CAMP44s2KNmr7zAvFo2gOR8G=YaoBWiGPCjPY47x00eev6MOAFw@mail.gmail.com","threadId":"31983","inReplyTo":"CAGdFq_h3L-1rPvb=dSYeXqEea+f+g2kRHp7aAjaU-AxjZHB7dQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T22:18:04Z","receivedAt":"2012-10-30T22:18:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 10:59 PM, Sverre Rabbelier wrote:\n> On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras\n> wrote:\n>> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier wrote:\n>>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras\n>>> wrote:\n>>>> Why would it? We are not changing the way objects are exported, the\n>>>> only difference is what happens at the end\n>>>> (handle_tags_and_duplicates()).\n>>>\n>>> Because the marking is per-commit, not per-ref, right?\n>>\n>> Oh, you meant using marks?\n>\n> No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per\n> ref? That is, commit->object.flags & SHOWN refers to the object\n> underlying the ref. So I suspect this scenario doesn't pass the tests:\n\nWithout marks you cannot have the SHOWN mark at that point; we haven't\ntraversed the commits.\n\n> git init &&\n> echo first > content &&\n> git add content &&\n> git commit -m \"first\" &&\n> git branch first &&\n> echo two > content &&\n> git commit -m \"second\" &&\n> git branch second &&\n> git fast-export first > actual &&\n> test_cmp actual expected_first &&\n> git fast-export second > actual &&\n> test_cmp actual expected_second\n>\n> With expected_first being something like:\n> \n> \n\nWhy would a 'reset' command be expected if the 'first' branch is\nalready pointing to the 'first' commit?\n\n> And expected_second being something like\n> \n> \n\nDitto, plus, why would 'git fast-export second' do anything regarding\n'first'? It wasn't specified in the committish; it's not relevant.\n\nBefore an after my patch the output is the same:\n\n% git fast-export first:\nreset refs/heads/first\ncommit refs/heads/first\n\n% git fast-export second:\nreset refs/heads/second\ncommit refs/heads/second\ncommit refs/heads/second\n\nWhich is expected and correct; the branch already points to the right\ncommit, no need for an extra reset.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202260","messageId":"CAMP44s3ArAQXH+-EbH4MHYaV6fTAWdwGzBdZwzn_qtCABHyonQ@mail.gmail.com","threadId":"31983","inReplyTo":"20121030220717.GO15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T22:22:44Z","receivedAt":"2012-10-30T22:22:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> Well, that's what we have now, and you want to preserve this \"feature\"\n>> (aka bug), right?\n>\n> Nope. I just don't want regressions, and found a patch description\n> that did nothing to explain to the reader how it avoids regressions\n> more than a little disturbing.\n\nI see, so you don't have any specific case where this could cause\nregressions, you are just saying it _might_ (like all patches).\n\n> I also think the proposed behavior is wrong.\n\nThat's a different matter, lets see what others think.\n\n-- \nFelipe Contreras\n"},{"id":"202261","messageId":"CAGdFq_iiGpYW-txPaa6mZrxg3mYdOX-Ez9uLF-rB5bAjZd5rWg@mail.gmail.com","threadId":"31983","inReplyTo":"CAMP44s2KNmr7zAvFo2gOR8G=YaoBWiGPCjPY47x00eev6MOAFw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-30T22:35:19Z","receivedAt":"2012-10-30T22:35:19Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras\n wrote:\n> Which is expected and correct; the branch already points to the right\n> commit, no need for an extra reset.\n\nI think you're correct. Thanks for confirming.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"202264","messageId":"CAMP44s2fgB=ruuVBdG6QjF6yviQAxVWFyhN6Vh3DWMGgmOKzyQ@mail.gmail.com","threadId":"31983","inReplyTo":"CAGdFq_iiGpYW-txPaa6mZrxg3mYdOX-Ez9uLF-rB5bAjZd5rWg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T22:56:20Z","receivedAt":"2012-10-30T22:56:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 30, 2012 at 11:35 PM, Sverre Rabbelier wrote:\n> On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras\n> wrote:\n>> Which is expected and correct; the branch already points to the right\n>> commit, no need for an extra reset.\n>\n> I think you're correct. Thanks for confirming.\n\nThanks for reviewing. If you are still not convinced, I could pull the\npatches from msysgit and simplify them, I'm sure the end result would\nbe pretty similar, if not exactly the same as this patch (plus other\northogonal changes). I saw some patches that were not part of the\npatch series you sent before, so maybe that's why you expected certain\nbehavior that wasn't actually there in that particular patch series.\n\nBut hopefully that's not needed.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202270","messageId":"20121030235506.GT15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s3ArAQXH+-EbH4MHYaV6fTAWdwGzBdZwzn_qtCABHyonQ@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-30T23:55:06Z","receivedAt":"2012-10-30T23:55:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote:\n\n>> Nope. I just don't want regressions, and found a patch description\n>> that did nothing to explain to the reader how it avoids regressions\n>> more than a little disturbing.\n>\n> I see, so you don't have any specific case where this could cause\n> regressions, you are just saying it _might_ (like all patches).\n\nYes, exactly. The commit log needs a description of the current\nbehavior, the intent behind the current code, the change the patch\nmakes, and the motivation behind that change, like all patches.\nDespite the nice examples, it doesn't currently have that.\n\nThe patch description just raises more questions for the reader. From\nthe description, one might imagine that this patch causes\n\n\tgit fast-export master\n\nnot to emit anything when another branch that has already been\nexported is ahead of \"master\". If I understand correctly (though\nI haven't tested), this patch does cause\n\n\tgit fast-export ^next master\n\nnot to emit anything when next is ahead of \"master\". That doesn't\nseem like progress.\n\nI haven't reviewed the later patches in the series; maybe they fix\nthese things. But in the long term it is much easier to understand\nand maintain a patch series that does not introduce regressions in the\nfirst place, and the context one might use to convincingly explain\nthat a patch is not introducing a regression turns out to be essential\nfor many other purposes as well.\n\nJonathan\n"},{"id":"202273","messageId":"20121031005748.GW15167@elie.Belkin","threadId":"31983","inReplyTo":"20121030185914.GI15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T00:57:48Z","receivedAt":"2012-10-31T00:57:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nFelipe Contreras wrote:\n\n> They have been marked as UNINTERESTING for a reason, lets respect that.\n\nSo, the above description conveyed zero information, as you mentioned.\n\nA clearer explanation would be the following:\n\n\tfast-export: don't emit \"reset\" command for negative refs\n\n\tWhen \"git fast-export\" encounters two refs on the commandline\n\treferring to the same commit, it exports the first during the usual\n\tcommit walk and the second using a \"reset\" command in a final pass\n\tover extra_refs:\n\n\t\t$ git fast-export master next\n\t\treset refs/heads/master\n\t\tcommit refs/heads/master\n\t\tmark :1\n\t\tauthor Jonathan Nieder 1351644412 -0700\n\t\tcommitter Jonathan Nieder 1351644412 -0700\n\t\tdata 17\n\t\tMy first commit!\n\n\t\treset refs/heads/next\n\t\tfrom :1\n\n\tUnfortunately the code to do this doesn't distinguish between positive\n\tand negative refs, producing confusing results:\n\n\t\t$ git fast-export ^master next\n\t\treset refs/heads/next\n\t\tfrom :0\n\n\t\t$ git fast-export master ^next\n\t\treset refs/heads/next\n\t\tfrom :0\n\n\tUse revs->cmdline instead of revs->pending to iterate over the rev-list\n\targuments, checking the UNINTERESTING flag bit to distinguish between\n\tpositive (master, --all, etc) and negative (next.., --not --all, etc)\n\trevs and avoid enqueueing negative revs in extra_revs.\n\n\tThis does not affect revs that were excluded from the revision walk\n\tbecause pointed to by a mark, since those use the SHOWN bit on the\n\tcommit object itself and not UNINTERESTING on the rev_cmdline_entry.\n\nA patch meeting the above description would make perfect sense to me.\nExcept for the somewhat strange testcase, the patch I am replying to\nwould also be fine in the short term, as long as it had an analagous\ndescription (i.e., with an appropriate replacement for the\nsecond-to-last paragraph).\n\nThanks for your patience, and hoping that helps,\nJonathan\n"},{"id":"202274","messageId":"CAMP44s1ftDijYpZW_Reu5qNi1T_L52_353ngNaRW3W1gz+k9jw@mail.gmail.com","threadId":"31983","inReplyTo":"20121030235506.GT15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-31T01:03:40Z","receivedAt":"2012-10-31T01:03:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote:\n>\n>>> Nope. I just don't want regressions, and found a patch description\n>>> that did nothing to explain to the reader how it avoids regressions\n>>> more than a little disturbing.\n>>\n>> I see, so you don't have any specific case where this could cause\n>> regressions, you are just saying it _might_ (like all patches).\n>\n> Yes, exactly. The commit log needs a description of the current\n> behavior, the intent behind the current code, the change the patch\n> makes, and the motivation behind that change, like all patches.\n> Despite the nice examples, it doesn't currently have that.\n>\n> The patch description just raises more questions for the reader. From\n> the description, one might imagine that this patch causes\n>\n> git fast-export master\n>\n> not to emit anything when another branch that has already been\n> exported is ahead of \"master\".\n\nThis is already the case.\n\nI don't see what part of my patch description would give you the idea\nthat this would change in any way how the objects are flagged, or how\nget_revision() decides how to traverse them.\n\nI clearly stated that this doesn't affect *the first* ref, which is\nhandled properly already; this patch affects *the rest* of the refs,\nof which you have none in that command above.\n\n> If I understand correctly (though\n> I haven't tested), this patch does cause\n>\n> git fast-export ^next master\n>\n> not to emit anything when next is ahead of \"master\". That doesn't\n> seem like progress.\n\nAgain, this is already the case RIGHT NOW.\n\nAnd nothing in my description should give you an idea that anything\nwould change for this case because the 2nd ref (*the first* doesn't\nget affected), is not marked as UNINTERESTING.\n\nNot only you are not reading what is in the description, but I don't\nthink you understand what the code actually does, and how it behaves.\n\nLet me give you some examples:\n\n% git fast-export ^next next\nreset refs/heads/next\nfrom :0\n\n% git fast-export ^next next^{commit}\n# nothing\n% git fast-export ^next next~0\n# nothing\n% git fast-export ^next next~1\n# nothing\n% git fast-export ^next next~2\n# nothing\n...\n# you get the idea\n\nThe *only time* when this patch would have any effect is when you\nspecify more than *one ref*, and they both point to *exactly the same\nobject*.\n\nAdditionally, and this is something I just found out; when the are\npure refs (e.g. 'next'), and not refs to objects (e.g.\n'next^{commit}').\n\nIn any other case; *there would be no change*.\n\nAfter my patch:\n\n% git fast-export ^next next\n# nothing\n% git fast-export ^next next^{commit}\n# nothing\n% git fast-export ^next next~0\n# nothing\n% git fast-export ^next next~1\n# nothing\n% git fast-export ^next next~2\n# nothing\n...\n# you get the idea\n\n> But in the long term it is much easier to understand\n> and maintain a patch series that does not introduce regressions in the\n> first place\n\nIt does not introduce regressions.\n\nI don't think it's my job to explain to you how 'git fast-export'\nworks. Above you made too many assumptions of what get broken, when in\nfact that's the current behavior already... maybe, just maybe, you are\nalso making wrong assumptions about this patch as well.\n\n-- \nFelipe Contreras\n"},{"id":"202275","messageId":"20121031010823.GX15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s1ftDijYpZW_Reu5qNi1T_L52_353ngNaRW3W1gz+k9jw@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T01:08:23Z","receivedAt":"2012-10-31T01:08:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> I don't think it's my job to explain to you how 'git fast-export'\n> works.\n\nActually, if you are submitting a patch for inclusion, it is your job\nto explain to future readers what the patch does. Yes, the reader\nmight not be deeply familiar with the part of fast-export you are\nmodifying. It might have even been modified since then, by the time\nthe reader is looking at the change!\n\nSad but true.\n\nThanks,\nJonathan\n"},{"id":"202276","messageId":"CAMP44s3pZsDa8w46JWmxFt=BdrxDxnB_r1p50p7eOiaVcjNs-w@mail.gmail.com","threadId":"31983","inReplyTo":"20121031005748.GW15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-31T01:23:08Z","receivedAt":"2012-10-31T01:23:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nOn Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> They have been marked as UNINTERESTING for a reason, lets respect that.\n>\n> So, the above description conveyed zero information, as you mentioned.\n\nI meant, this, of course:\n>> They have been marked as UNINTERESTING for a reason, lets respect that.\n>\n> This patch looks unsafe,\n\nWhich you know, because you received that message without the mistake.\n\n> A clearer explanation would be the following:\n>\n> fast-export: don't emit \"reset\" command for negative refs\n\nWhat is a negative ref?\n\n> When \"git fast-export\" encounters two refs on the commandline\n\ncommandline?\n\nOnly two refs? How about four?\n\n> referring to the same commit, it exports the first during the usual\n> commit walk and the second using a \"reset\" command in a final pass\n> over extra_refs:\n\nThat is not exactly true: (next^{commit}).\n\n> $ git fast-export master next\n> reset refs/heads/master\n> commit refs/heads/master\n> mark :1\n> author Jonathan Nieder 1351644412 -0700\n> committer Jonathan Nieder 1351644412 -0700\n> data 17\n> My first commit!\n>\n> reset refs/heads/next\n> from :1\n\nI don't think this example is good. Where does it say that 'next'\npoints to master? Using 'points-to-master' or a 'git branch stable\nmaster' and using 'master stable'.\n\nEven simpler would be to use 'git fast-export master master'; it would\nshow the same behavior.\n\n> Unfortunately the code to do this doesn't distinguish between positive\n> and negative refs, producing confusing results:\n>\n> $ git fast-export ^master next\n> reset refs/heads/next\n> from :0\n>\n> $ git fast-export master ^next\n> reset refs/heads/next\n> from :0\n>\n> Use revs->cmdline instead of revs->pending to iterate over the rev-list\n> arguments, checking the UNINTERESTING flag bit to distinguish between\n> positive (master, --all, etc) and negative (next.., --not --all, etc)\n> revs and avoid enqueueing negative revs in extra_revs.\n\nUse what? You mean, \"To solve the problem, lets use\".\n\nBut this is not correct, cmdline is not being used. Have you even\nlooked at the patch?\n\n> This does not affect revs that were excluded from the revision walk\n> because pointed to by a mark, since those use the SHOWN bit on the\n> commit object itself and not UNINTERESTING on the rev_cmdline_entry.\n\nrevs? You mean commits?\n\n\"excluded because point to by a mark\"? Doesn't sound like proper\ngrammar. Maybe \"excluded because they were pointed to by a mark\".\n\nAnd I don't see why this paragraph is needed at all. Why would the\nreader think marks have anything to do with this? There's no mention\nof marks before.\n\nThis might help you, or other people involved in the problem, but not\nanybody else. Anything related to marks is completely orthogonal to\nthis patch, and there's no point in mentioning that.\n\n-- \nFelipe Contreras\n"},{"id":"202279","messageId":"20121031013556.GZ15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s3pZsDa8w46JWmxFt=BdrxDxnB_r1p50p7eOiaVcjNs-w@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T01:35:56Z","receivedAt":"2012-10-31T01:35:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> This might help you, or other people involved in the problem, but not\n> anybody else.\n\nOk, I give up. Bye.\n\nSometimes the author of some code and the right person to interact\nwith the development community by submitting and maintaining it are\nnot the same person. Hopefully others more patient than we two can\npick up where we left off.\n\nThanks,\nJonathan\n"},{"id":"202280","messageId":"CAMP44s0RcbAiUmvGACxO+H-b-anQSPXxUqUuZwYRKWfrpXYeew@mail.gmail.com","threadId":"31983","inReplyTo":"20121031010823.GX15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-31T01:39:53Z","receivedAt":"2012-10-31T01:39:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> I don't think it's my job to explain to you how 'git fast-export'\n>> works.\n>\n> Actually, if you are submitting a patch for inclusion, it is your job\n> to explain to future readers what the patch does.\n\nThat's already explained.\n\n> Yes, the reader\n> might not be deeply familiar with the part of fast-export you are\n> modifying.\n\nThis has nothing to do with what you said. I'm literally explaining to\nyou how 'git fast-export' works in situations that are completely\northogonal to this patch, because you are using wrong examples as\ngrounds to prevent this patch from being accepted. It's not my job to\nexplain to you that 'git fast-export' doesn't work this way, you have\na command line to type those commands and see for yourself if they do\nwhat you think they do with a vanilla version of git. That's exactly\nwhat I did, to make sure I'm not using assumptions as basis for\narguing, it took me a few minutes.\n\nThat being said, if your problem is that it's not clear to people not\ndeeply familiar with that part of fast-export, this extra paragraph in\naddition to the current commit message should do the trick:\n\n---\nThe reason this happens is that before traversing the commits,\nfast-export checks if any of the refs point to the same object, and\nany duplicated ref gets added to a list in order to issue 'reset'\ncommands after the traversing. Unfortunately, it's not even checking\nif the commit is flagged as UNINTERESTING. The fix of course, is to do\nprecisely that.\n---\n\nAnd to get that all had to do is ask: \"Can you please add an\nexplanation of what this part of the code does? For the ones of us not\nfamiliar with it\".\n\nNot; \"This patch looks unsafe\", \"This patch makes Sally mad\", \"This\npatch causes regressions\", and so on.\n\nBut hey, at least we are not arguing about what is wrong with this\npatch (or so I hope).\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202281","messageId":"20121031015103.GA15167@elie.Belkin","threadId":"31983","inReplyTo":"CAMP44s0RcbAiUmvGACxO+H-b-anQSPXxUqUuZwYRKWfrpXYeew@mail.gmail.com","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T01:51:03Z","receivedAt":"2012-10-31T01:51:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> It's not my job to\n> explain to you that 'git fast-export' doesn't work this way, you have\n> a command line to type those commands and see for yourself if they do\n> what you think they do with a vanilla version of git. That's exactly\n> what I did, to make sure I'm not using assumptions as basis for\n> arguing, it took me a few minutes.\n\nWell no, when I run \"git blame\" 10 years down the line and do not\nunderstand what your code is doing, it is not at all reasonable to\nexpect me to checkout the parent commit, get it to compile with a\nmodern toolchain, and type those commands for myself.\n\nInstead, the commit message should be self-contained and explain what\nthe patch does.\n\nThat has multiple parts:\n\n - first, what the current behavior is\n\n - second, what the intent behind the current behavior is. This is\n crucial information because presumably we want the change not to\n break that.\n\n - third, what change the patch makes\n\n - fourth, what the consequences of that are, in terms of new use\n cases that become possible and old use cases that become less\n convenient\n\n - fifth, optionally, how the need for this change was discovered\n (real-life usage, code inspection, or something else)\n\n - sixth, optionally, implementation considerations and alternate\n approaches that were discarded\n\nIf you run \"git log\", you'll see many good and bad examples to think\nover and compare to this goal. It's hard work to describe one's work\nwell in terms that other people can understand, but I think it can be\nsatisfying, and in any event, it's just as necessary as including\ncomments near confusing code.\n\nSincerely,\nJonathan\n"},{"id":"202286","messageId":"CAMP44s1EX8AJgFyOjbr0v5mrQooCwQ_gbr2HYf32qwU_Xf7HfA@mail.gmail.com","threadId":"31983","inReplyTo":"20121031015103.GA15167@elie.Belkin","subject":"Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-31T02:22:40Z","receivedAt":"2012-10-31T02:22:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> It's not my job to\n>> explain to you that 'git fast-export' doesn't work this way, you have\n>> a command line to type those commands and see for yourself if they do\n>> what you think they do with a vanilla version of git. That's exactly\n>> what I did, to make sure I'm not using assumptions as basis for\n>> arguing, it took me a few minutes.\n>\n> Well no, when I run \"git blame\" 10 years down the line and do not\n> understand what your code is doing, it is not at all reasonable to\n> expect me to checkout the parent commit, get it to compile with a\n> modern toolchain, and type those commands for myself.\n>\n> Instead, the commit message should be self-contained and explain what\n> the patch does.\n>\n> That has multiple parts:\n>\n> - first, what the current behavior is\n>\n> - second, what the intent behind the current behavior is. This is\n> crucial information because presumably we want the change not to\n> break that.\n>\n> - third, what change the patch makes\n>\n> - fourth, what the consequences of that are, in terms of new use\n> cases that become possible and old use cases that become less\n> convenient\n>\n> - fifth, optionally, how the need for this change was discovered\n> (real-life usage, code inspection, or something else)\n>\n> - sixth, optionally, implementation considerations and alternate\n> approaches that were discarded\n\nI don't see any \"Explain in detail what different commands do, even if\nthey are irrelevant to the patch in question because someone might\nthink they would get broken by this patch when in fact they wouldn't\",\nthat might belong in the discussion, but not in the commit message,\nand certainly not in the form of any entitlement.\n\nAgain, it's _your_ responsibility to make sure the commands you say\nmight get broken do actually work with your current git, it's not mine\nto run them for you, even though that's exactly what I did, because\nI'm interested in getting things correctly on record.\n\nAnd FTR, since you removed it, here is what I proposed to add to the\ncommit message:\n\n---\nThe reason this happens is that before traversing the commits,\nfast-export checks if any of the refs point to the same object, and\nany duplicated ref gets added to a list in order to issue 'reset'\ncommands after the traversing. Unfortunately, it's not even checking\nif the commit is flagged as UNINTERESTING. The fix of course, is to do\nprecisely that.\n---\n\nWith that, all the points above are tackled, except fourth, because\nthere aren't any.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}