{"thread":{"id":"31984","subject":"[PATCH v3 0/4] fast-export: general fixes","startedAt":"2012-10-30T19:06:23Z","lastAt":"2012-11-02T15:35:29Z","messageCount":21,"participants":["Felipe Contreras","Jonathan Nieder","Sverre Rabbelier","Peter Baumann","Drew Northup","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"202225","messageId":"1351623987-21012-1-git-send-email-felipe.contreras@gmail.com","threadId":"31984","inReplyTo":null,"subject":"[PATCH v3 0/4] fast-export: general fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:06:23Z","receivedAt":"2012-10-30T19:06:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nNote: sorry for the noise, the first try (v2) was silently eaten by the mailing\nlist handler.\n\nFirst patches are general cleanups and fixes, the last patch fixes a real issue\nthat affects remote helpers.\n\nChanges since v2:\n\n * Actually send it to the ml\n\nChanges since v1:\n\n * Improved commit messages\n * Use /dev/null in tests\n * Add test for remote helpers\n\nFelipe Contreras (4):\n fast-export: trivial cleanup\n fast-export: fix comparisson in tests\n fast-export: don't handle uninteresting refs\n fast-export: make sure refs are updated properly\n\n builtin/fast-export.c | 16 +++++++++++-----\n t/t5800-remote-helpers.sh | 11 +++++++++++\n t/t9350-fast-export.sh | 26 +++++++++++++++++++++++---\n 3 files changed, 45 insertions(+), 8 deletions(-)\n\n-- \n1.8.0\n"},{"id":"202226","messageId":"1351623987-21012-2-git-send-email-felipe.contreras@gmail.com","threadId":"31984","inReplyTo":"1351623987-21012-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 1/4] fast-export: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:06:24Z","receivedAt":"2012-10-30T19:06:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Setting commit to commit is a no-op. It might have been there to avoid a\ncompiler warning, but if so, it was the compiler to blame.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 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-- \n1.8.0\n"},{"id":"202227","messageId":"1351623987-21012-3-git-send-email-felipe.contreras@gmail.com","threadId":"31984","inReplyTo":"1351623987-21012-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 2/4] fast-export: fix comparisson in tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:06:25Z","receivedAt":"2012-10-30T19:06:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"First the expected, then the actual, otherwise the diff would be the\nopposite of what we want.\n\nSigned-off-by: Felipe Contreras \n---\n t/t9350-fast-export.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 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-- \n1.8.0\n"},{"id":"202228","messageId":"1351623987-21012-4-git-send-email-felipe.contreras@gmail.com","threadId":"31984","inReplyTo":"1351623987-21012-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 3/4] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:06:26Z","receivedAt":"2012-10-30T19:06:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"They have been marked as UNINTERESTING for a reason, lets respect that.\n\nCurrently the first ref is handled properly, but not the rest, so:\n\n % git fast-export master ^master\n\nWould currently throw a reset for master (2nd ref), which is not what we\nwant.\n\n % git fast-export master ^foo ^bar ^roo\n % git fast-export master salsa..tacos\n\nEven if all these refs point to the same object; foo, bar, roo, salsa,\nand tacos would all get a reset.\n\nThis is most certainly not what we want. After this patch, nothing gets\nexported, because nothing was selected (everything is UNINTERESTING).\n\nSigned-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\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 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 }\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 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-- \n1.8.0\n"},{"id":"202229","messageId":"1351623987-21012-5-git-send-email-felipe.contreras@gmail.com","threadId":"31984","inReplyTo":"1351623987-21012-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-30T19:06:27Z","receivedAt":"2012-10-30T19:06:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When an object has already been exported (and thus is in the marks) it\nis flagged as SHOWN, so it will not be exported again, even if this time\nit's exported through a different ref.\n\nWe don't need the object to be exported again, but we want the ref\nupdated, which doesn't happen.\n\nSince we can't know if a ref was exported or not, let's just assume that\nif the commit was marked (flags & SHOWN), the user still wants the ref\nupdated.\n\nSo:\n\n % git branch test master\n % git fast-export $mark_flags master\n % git fast-export $mark_flags test\n\nWould export 'test' properly.\n\nAdditionally, this fixes issues with remote helpers; now they can push\nrefs wich objects have already been exported.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 11 ++++++++---\n t/t5800-remote-helpers.sh | 11 +++++++++++\n t/t9350-fast-export.sh | 14 ++++++++++++++\n 3 files changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 7fb6fe1..663a93d 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -523,11 +523,16 @@ 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\t\t/* more than one name for the same object */\n+\n+\t\t/*\n+\t\t * This ref will not be updated through a commit, lets make\n+\t\t * sure it gets properly upddated eventually.\n+\t\t */\n+\t\tif (commit->util || commit->object.flags & SHOWN) {\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}\n+\t\tif (!commit->util)\n \t\t\tcommit->util = full_name;\n \t}\n }\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex e7dc668..69a145a 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '\n \tcompare_refs clone HEAD server refs/heads/new-refspec\n '\n \n+test_expect_success 'push ref with existing object' '\n+\t(cd localclone &&\n+\tgit branch point-to-master master &&\n+\tgit push origin point-to-master\n+\t) &&\n+\n+\t(cd server &&\n+\tgit show-ref refs/heads/point-to-master\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 6ea8f6f..a4178e3 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -446,4 +446,18 @@ test_expect_success 'proper extra refs handling' '\n \ttest_cmp expected actual\n '\n \n+cat > expected << EOF\n+reset refs/heads/master\n+from :13\n+\n+EOF\n+\n+test_expect_success 'refs are updated even if no commits need to be exported' '\n+\tgit fast-export --import-marks=tmp-marks \\\n+\t\t--export-marks=tmp-marks master > /dev/null &&\n+\tgit fast-export --import-marks=tmp-marks \\\n+\t\t--export-marks=tmp-marks master > actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.0\n"},{"id":"202271","messageId":"20121031001117.GA29486@elie.Belkin","threadId":"31984","inReplyTo":"1351623987-21012-5-git-send-email-felipe.contreras@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-31T00:11:17Z","receivedAt":"2012-10-31T00:11:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing the git list)\nFelipe Contreras wrote:\n\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\n\nYes, makes perfect sense.\n\nFor what it's worth,\nAcked-by: Jonathan Nieder \n\n[...]\n> --- a/t/t5800-remote-helpers.sh\n> +++ b/t/t5800-remote-helpers.sh\n> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '\n> \tcompare_refs clone HEAD server refs/heads/new-refspec\n> '\n> \n> +test_expect_success 'push ref with existing object' '\n> +\t(cd localclone &&\n> +\tgit branch point-to-master master &&\n> +\tgit push origin point-to-master\n> +\t) &&\n> +\n> +\t(cd server &&\n> +\tgit show-ref refs/heads/point-to-master\n> +\t)\n\nStyle: if you indent like this, the test becomes clearer:\n\n\t(\n\t\tcd localclone &&\n\t\tgit branch point-to-master master &&\n\t\tgit push origin point-to-master\n\t) &&\n\t(\n\t\tcd server &&\n\t\tgit rev-parse --verify refs/heads/point-to-master\n\t)\n\n[...]\n> +test_expect_success 'refs are updated even if no commits need to be exported' '\n> +\tgit fast-export --import-marks=tmp-marks \\\n> +\t\t--export-marks=tmp-marks master > /dev/null &&\n\nThe redirect just makes the test log with \"-v\" less informative, so\nI'd drop it.\n\n> +\tgit fast-export --import-marks=tmp-marks \\\n> +\t\t--export-marks=tmp-marks master > actual &&\n> +\ttest_cmp expected actual\n\nRedirections in git shell scripts are generally spelled as\n\"do_something >actual\", without a space between the operator and\nfilename.\n\nHope that helps,\nJonathan\n"},{"id":"202272","messageId":"20121031003721.GV15167@elie.Belkin","threadId":"31984","inReplyTo":"1351623987-21012-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T00:37:21Z","receivedAt":"2012-10-31T00:37:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -523,11 +523,16 @@ 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\t\t/* more than one name for the same object */\n> +\n> +\t\t/*\n> +\t\t * This ref will not be updated through a commit, lets make\n> +\t\t * sure it gets properly upddated eventually.\n> +\t\t */\n> +\t\tif (commit->util || commit->object.flags & SHOWN) {\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}\n> +\t\tif (!commit->util)\n> \t\t\tcommit->util = full_name;\n\nHere's an explanation of why the above makes sense to me.\n\nget_tags_and_duplicates() gets called after the marks import and\nbefore the revision walk. It walks through the revs from the\ncommandline and for each one:\n\n - peels it to a refname, and then to a commit\n - stores the refname so fast-export knows what arg to pass to\n the \"commit\" command during the revision walk\n - if it already had a refname stored, instead adds the\n (refname, commit) pair to the extra_refs list, so fast-export\n knows to add a \"reset\" command later.\n\nIf the commit already has the SHOWN flag set because it was pointed to\nby a mark, it is not going to come up in the revision walk, so it will\nnot be mentioned in the output stream unless it is added to\nextra_refs. That's what this patch does.\n\nIncidentally, the change from \"else\" to \"if (!commit->util)\" is\nunnecessary because if a commit is already SHOWN then it will not be\nencountered in the revision walk so commit->util does not need to be\nset.\n\nIf the commit does not have the SHOWN or UNINTERESTING flag set but it\nis going to get the UNINTERESTING flag set during the walk because of\na negative commit listed on the command line, this patch won't help.\n\nJonathan\n"},{"id":"202278","messageId":"CAGdFq_jNM_48muXJ0BX2ehC=k8T9GLui_QtRO8D8C7h6b5jyHg@mail.gmail.com","threadId":"31984","inReplyTo":"20121031003721.GV15167@elie.Belkin","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-31T01:33:04Z","receivedAt":"2012-10-31T01:33:04Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> --- a/builtin/fast-export.c\n>> +++ b/builtin/fast-export.c\n>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,\n>> typename(e->item->type));\n>> continue;\n>> }\n>> - if (commit->util) {\n>> - /* more than one name for the same object */\n>> +\n>> + /*\n>> + * This ref will not be updated through a commit, lets make\n>> + * sure it gets properly upddated eventually.\n>> + */\n>> + if (commit->util || commit->object.flags & SHOWN) {\n>> if (!(commit->object.flags & UNINTERESTING))\n>> string_list_append(extra_refs, full_name)->util = commit;\n>> - } else\n>> + }\n>> + if (!commit->util)\n>> commit->util = full_name;\n>\n> Here's an explanation of why the above makes sense to me.\n>\n> get_tags_and_duplicates() gets called after the marks import and\n> before the revision walk. It walks through the revs from the\n> commandline and for each one:\n>\n> - peels it to a refname, and then to a commit\n> - stores the refname so fast-export knows what arg to pass to\n> the \"commit\" command during the revision walk\n> - if it already had a refname stored, instead adds the\n> (refname, commit) pair to the extra_refs list, so fast-export\n> knows to add a \"reset\" command later.\n>\n> If the commit already has the SHOWN flag set because it was pointed to\n> by a mark, it is not going to come up in the revision walk, so it will\n> not be mentioned in the output stream unless it is added to\n> extra_refs. That's what this patch does.\n>\n> Incidentally, the change from \"else\" to \"if (!commit->util)\" is\n> unnecessary because if a commit is already SHOWN then it will not be\n> encountered in the revision walk so commit->util does not need to be\n> set.\n>\n> If the commit does not have the SHOWN or UNINTERESTING flag set but it\n> is going to get the UNINTERESTING flag set during the walk because of\n> a negative commit listed on the command line, this patch won't help.\n\nThanks for the thorough explanation. Perhaps some of that could make\nit's way into the commit message?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"202283","messageId":"CAMP44s0jyvwmqD59o+cRgWc9vjxAdWO_4rORYrNcqU4VLJ9Kfg@mail.gmail.com","threadId":"31984","inReplyTo":"20121031001117.GA29486@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-31T02:08:32Z","receivedAt":"2012-10-31T02:08:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 31, 2012 at 1:11 AM, Jonathan Nieder wrote:\n> (cc-ing the git list)\n> Felipe Contreras wrote:\n>\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\n>\n> Yes, makes perfect sense.\n>\n> For what it's worth,\n> Acked-by: Jonathan Nieder \n\nYay!\n\n> [...]\n>> --- a/t/t5800-remote-helpers.sh\n>> +++ b/t/t5800-remote-helpers.sh\n>> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '\n>> compare_refs clone HEAD server refs/heads/new-refspec\n>> '\n>>\n>> +test_expect_success 'push ref with existing object' '\n>> + (cd localclone &&\n>> + git branch point-to-master master &&\n>> + git push origin point-to-master\n>> + ) &&\n>> +\n>> + (cd server &&\n>> + git show-ref refs/heads/point-to-master\n>> + )\n>\n> Style: if you indent like this, the test becomes clearer:\n\nAnd then it would become inconsistent with the rest of the file.\n\n>> + git fast-export --import-marks=tmp-marks \\\n>> + --export-marks=tmp-marks master > actual &&\n>> + test_cmp expected actual\n>\n> Redirections in git shell scripts are generally spelled as\n> \"do_something >actual\", without a space between the operator and\n> filename.\n\nI generally am OK with adapting to whatever code-style is used\n(sometimes under protest), but this is a place where I draw the\nline. Sorry, '>actual' is more annoying to me than a knife screeching\nglass. Fortunately, '> actual' is used in many other places in 't/',\nso I'm going to use the other people jumping over the bridge argument.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202284","messageId":"CAMP44s26chrdESPmQGRYBN4dy-C-fGjbgWbR7fBs71bViRYa-w@mail.gmail.com","threadId":"31984","inReplyTo":"20121031003721.GV15167@elie.Belkin","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-31T02:13:20Z","receivedAt":"2012-10-31T02:13:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> --- a/builtin/fast-export.c\n>> +++ b/builtin/fast-export.c\n>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,\n>> typename(e->item->type));\n>> continue;\n>> }\n>> - if (commit->util) {\n>> - /* more than one name for the same object */\n>> +\n>> + /*\n>> + * This ref will not be updated through a commit, lets make\n>> + * sure it gets properly upddated eventually.\n>> + */\n>> + if (commit->util || commit->object.flags & SHOWN) {\n>> if (!(commit->object.flags & UNINTERESTING))\n>> string_list_append(extra_refs, full_name)->util = commit;\n>> - } else\n>> + }\n>> + if (!commit->util)\n>> commit->util = full_name;\n>\n> Here's an explanation of why the above makes sense to me.\n>\n> get_tags_and_duplicates() gets called after the marks import and\n> before the revision walk. It walks through the revs from the\n> commandline and for each one:\n>\n> - peels it to a refname, and then to a commit\n> - stores the refname so fast-export knows what arg to pass to\n> the \"commit\" command during the revision walk\n> - if it already had a refname stored, instead adds the\n> (refname, commit) pair to the extra_refs list, so fast-export\n> knows to add a \"reset\" command later.\n>\n> If the commit already has the SHOWN flag set because it was pointed to\n> by a mark, it is not going to come up in the revision walk, so it will\n> not be mentioned in the output stream unless it is added to\n> extra_refs. That's what this patch does.\n\nThat is correct.\n\n> Incidentally, the change from \"else\" to \"if (!commit->util)\" is\n> unnecessary because if a commit is already SHOWN then it will not be\n> encountered in the revision walk so commit->util does not need to be\n> set.\n\nMaybe, but that's yet another change, and with more changes come more\npossibilities of regressions. I haven't verified this is the case.\n\nIf this makes sense, I would do it in another, separate patch.\n\n> If the commit does not have the SHOWN or UNINTERESTING flag set but it\n> is going to get the UNINTERESTING flag set during the walk because of\n> a negative commit listed on the command line, this patch won't help.\n\nI don't know what that means in practice.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202290","messageId":"20121031060529.GA30432@elie.Belkin","threadId":"31984","inReplyTo":"CAGdFq_jNM_48muXJ0BX2ehC=k8T9GLui_QtRO8D8C7h6b5jyHg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-31T06:05:29Z","receivedAt":"2012-10-31T06:05:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> Thanks for the thorough explanation. Perhaps some of that could make\n> it's way into the commit message?\n\nIt's fine with me if it doesn't, since the original commit message\ncovers the basics (current behavior and intent of the change) in its\nfirst two paragraphs and anyone wanting more detail can use\n\n\tGIT_NOTES_REF=refs/remotes/charon/notes/full \\\n\tgit show --show-notes \n\nto find more details.\n\nThanks,\nJonathan\n"},{"id":"202295","messageId":"20121031095327.GB18557@m62s10.vlinux.de","threadId":"31984","inReplyTo":"20121031060529.GA30432@elie.Belkin","subject":"[OT] How to get the discussion details via notes","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2012-10-31T09:53:27Z","receivedAt":"2012-10-31T09:53:27Z","isPatch":false,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"Dropping the Cc list, as this is off topic\n\nOn Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:\n> Sverre Rabbelier wrote:\n> \n> > Thanks for the thorough explanation. Perhaps some of that could make\n> > it's way into the commit message?\n> \n> It's fine with me if it doesn't, since the original commit message\n> covers the basics (current behavior and intent of the change) in its\n> first two paragraphs and anyone wanting more detail can use\n> \n> \tGIT_NOTES_REF=refs/remotes/charon/notes/full \\\n> \tgit show --show-notes \n> \n> to find more details.\n\nI seem to miss something here, but I don't get it how the notes ref\nbecomes magically filled with the details of this discussion.\n\nCare to explain?\n\n-Peter\n"},{"id":"202305","messageId":"CAM9Z-nnMF47p2aK+c4Nq=hYFfhYBsWqiKogW36Lre5o7vZonyw@mail.gmail.com","threadId":"31984","inReplyTo":"20121031095327.GB18557@m62s10.vlinux.de","subject":"Re: [OT] How to get the discussion details via notes","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-10-31T12:29:33Z","receivedAt":"2012-10-31T12:29:33Z","isPatch":false,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"On Wed, Oct 31, 2012 at 5:53 AM, Peter Baumann wrote:\n> Dropping the Cc list, as this is off topic\n>\n> On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:\n>> Sverre Rabbelier wrote:\n>>\n>> > Thanks for the thorough explanation. Perhaps some of that could make\n>> > it's way into the commit message?\n>>\n>> It's fine with me if it doesn't, since the original commit message\n>> covers the basics (current behavior and intent of the change) in its\n>> first two paragraphs and anyone wanting more detail can use\n>>\n>> GIT_NOTES_REF=refs/remotes/charon/notes/full \\\n>> git show --show-notes \n>>\n>> to find more details.\n>\n> I seem to miss something here, but I don't get it how the notes ref\n> becomes magically filled with the details of this discussion.\n>\n> Care to explain?\n\nIf I have an email thread I'd like to store alongside a commit I'll\nput that into a note, but I usually don't push that kind of thing out\nto a remote repo.\nDoes that help?\n\n-- \n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"202313","messageId":"20121031141024.GB24291@sigill.intra.peff.net","threadId":"31984","inReplyTo":"20121031095327.GB18557@m62s10.vlinux.de","subject":"Re: [OT] How to get the discussion details via notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-31T14:10:24Z","receivedAt":"2012-10-31T14:10:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 31, 2012 at 10:53:27AM +0100, Peter Baumann wrote:\n\n> > covers the basics (current behavior and intent of the change) in its\n> > first two paragraphs and anyone wanting more detail can use\n> > \n> > \tGIT_NOTES_REF=refs/remotes/charon/notes/full \\\n> > \tgit show --show-notes \n> > \n> > to find more details.\n> \n> I seem to miss something here, but I don't get it how the notes ref\n> becomes magically filled with the details of this discussion.\n\nThomas Rast (aka charon) keeps a mapping of commits to the email threads\nthat led to them. You can fetch it from:\n\n git://repo.or.cz/git/trast.git\n\n(try the notes/full and notes/terse refs).\n\n-Peff\n"},{"id":"202359","messageId":"20121101074940.GC18557@m62s10.vlinux.de","threadId":"31984","inReplyTo":"20121031141024.GB24291@sigill.intra.peff.net","subject":"Re: [OT] How to get the discussion details via notes","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2012-11-01T07:49:40Z","receivedAt":"2012-11-01T07:49:40Z","isPatch":false,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Wed, Oct 31, 2012 at 10:10:24AM -0400, Jeff King wrote:\n> On Wed, Oct 31, 2012 at 10:53:27AM +0100, Peter Baumann wrote:\n> \n> > > covers the basics (current behavior and intent of the change) in its\n> > > first two paragraphs and anyone wanting more detail can use\n> > > \n> > > \tGIT_NOTES_REF=refs/remotes/charon/notes/full \\\n> > > \tgit show --show-notes \n> > > \n> > > to find more details.\n> > \n> > I seem to miss something here, but I don't get it how the notes ref\n> > becomes magically filled with the details of this discussion.\n> \n> Thomas Rast (aka charon) keeps a mapping of commits to the email threads\n> that led to them. You can fetch it from:\n> \n> git://repo.or.cz/git/trast.git\n> \n> (try the notes/full and notes/terse refs).\n> \n\nNice! I didn't know about that.\n\n-Peter\n"},{"id":"202416","messageId":"20121102131255.GB2598@sigill.intra.peff.net","threadId":"31984","inReplyTo":"20121031003721.GV15167@elie.Belkin","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-02T13:12:55Z","receivedAt":"2012-11-02T13:12:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:\n\n> If the commit does not have the SHOWN or UNINTERESTING flag set but it\n> is going to get the UNINTERESTING flag set during the walk because of\n> a negative commit listed on the command line, this patch won't help.\n\nRight, so my understanding of the situation is that doing this:\n\n $ git branch foo master~1\n $ git fast-export foo master~1..master\n\nwon't show \"foo\", which seems wrong to me. _But_ we currently get that\nwrong already, so Felipe's patches are not making anything worse, but\nare fixing some situations (namely when master~1 is not mentioned on the\ncommand-line, but rather in a marks file).\n\nIs that correct?\n\nIf so, then this series isn't regressing behavior; the only downside is\nthat it's an incomplete fix. In theory this could get in the way of the\nfull fix later on, but given the commit messages and the archive of this\ndiscussion, it would be simple enough to revert it later in favor of a\nmore full fix. Is that accurate?\n\nSorry if I am belaboring the discussion. I just want to make sure I\nunderstand the situation before deciding what to do with the topic. It\nsounds like the consensus at this point is \"not perfect, but good enough\nto make forward progress\".\n\n-Peff\n"},{"id":"202432","messageId":"20121102145555.GA14774@elie.Belkin","threadId":"31984","inReplyTo":"20121102131255.GB2598@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-02T14:55:55Z","receivedAt":"2012-11-02T14:55:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> If so, then this series isn't regressing behavior; the only downside is\n> that it's an incomplete fix. In theory this could get in the way of the\n> full fix later on, but given the commit messages and the archive of this\n> discussion, it would be simple enough to revert it later in favor of a\n> more full fix. Is that accurate?\n>\n> Sorry if I am belaboring the discussion. I just want to make sure I\n> understand the situation before deciding what to do with the topic. It\n> sounds like the consensus at this point is \"not perfect, but good enough\n> to make forward progress\".\n\nPatch 1, 2, and 4 are good modulo their descriptions. They should\nwork fine without patch 3.\n\nPatch 3 is a regression in comprehensibility. I think we can do\nbetter. Maybe all it would take is a less confusing description, and\ntweaks to the code (to loop over revs->cmdline instead of\nrevs->pending) could come on top.\n"},{"id":"202435","messageId":"alpine.DEB.1.00.1211021612320.7256@s15462909.onlinehome-server.info","threadId":"31984","inReplyTo":"20121102131255.GB2598@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-11-02T15:17:14Z","receivedAt":"2012-11-02T15:17:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 2 Nov 2012, Jeff King wrote:\n\n> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:\n> \n> > If the commit does not have the SHOWN or UNINTERESTING flag set but it\n> > is going to get the UNINTERESTING flag set during the walk because of\n> > a negative commit listed on the command line, this patch won't help.\n> \n> Right, so my understanding of the situation is that doing this:\n> \n> $ git branch foo master~1\n> $ git fast-export foo master~1..master\n> \n> won't show \"foo\", which seems wrong to me. _But_ we currently get that\n> wrong already, so Felipe's patches are not making anything worse, but\n> are fixing some situations (namely when master~1 is not mentioned on the\n> command-line, but rather in a marks file).\n> \n> Is that correct?\n> \n> If so, then this series isn't regressing behavior; the only downside is\n> that it's an incomplete fix. In theory this could get in the way of the\n> full fix later on, but given the commit messages and the archive of this\n> discussion, it would be simple enough to revert it later in favor of a\n> more full fix. Is that accurate?\n\n>From my understanding, yes.\n\n> Sorry if I am belaboring the discussion. I just want to make sure I\n> understand the situation before deciding what to do with the topic. It\n> sounds like the consensus at this point is \"not perfect, but good enough\n> to make forward progress\".\n\nI appreciate that stance very much. The patch Sverre and I proposed was\nalso an incomplete fix (although I suspect it would fix the issue you\npointed out above), so I agree with the \"perfect is the enemy of the good\"\napproach, obviously.\n\nMay I just ask to include a summary of that rationale into the commit\nmessage rather than relying on people having internet access and knowing\nwhere to look? Adding the following to the commit message would be good\nenough for me:\n\n\tNote that\n\n\t\t$ git branch foo master~1\n\t\t$ git fast-export foo master~1..master\n\n\tstill does not update the \"foo\" ref, but a partial fix is better\n\tthan no fix.\n\nThanks,\nDscho\n"},{"id":"202437","messageId":"20121102151955.GA24622@sigill.intra.peff.net","threadId":"31984","inReplyTo":"alpine.DEB.1.00.1211021612320.7256@s15462909.onlinehome-server.info","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-02T15:19:55Z","receivedAt":"2012-11-02T15:19:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:\n\n> > If so, then this series isn't regressing behavior; the only downside is\n> > that it's an incomplete fix. In theory this could get in the way of the\n> > full fix later on, but given the commit messages and the archive of this\n> > discussion, it would be simple enough to revert it later in favor of a\n> > more full fix. Is that accurate?\n> \n> From my understanding, yes.\n> \n> > Sorry if I am belaboring the discussion. I just want to make sure I\n> > understand the situation before deciding what to do with the topic. It\n> > sounds like the consensus at this point is \"not perfect, but good enough\n> > to make forward progress\".\n> \n> I appreciate that stance very much. The patch Sverre and I proposed was\n> also an incomplete fix (although I suspect it would fix the issue you\n> pointed out above), so I agree with the \"perfect is the enemy of the good\"\n> approach, obviously.\n\nThanks for the response.\n\n> May I just ask to include a summary of that rationale into the commit\n> message rather than relying on people having internet access and knowing\n> where to look? Adding the following to the commit message would be good\n> enough for me:\n> \n> \tNote that\n> \n> \t\t$ git branch foo master~1\n> \t\t$ git fast-export foo master~1..master\n> \n> \tstill does not update the \"foo\" ref, but a partial fix is better\n> \tthan no fix.\n\nYes, I think that makes a lot of sense.\n\nFelipe, I notice that you sent out a big \"fast-export improvements\"\nseries. Does that supersede this?\n\n-Peff\n"},{"id":"202440","messageId":"CAMP44s2s-XjOqQQr4qvzeOhVDazKsLZ94p4QDz+2XJ9m_8A15A@mail.gmail.com","threadId":"31984","inReplyTo":"20121102131255.GB2598@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-02T15:34:38Z","receivedAt":"2012-11-02T15:34:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 2, 2012 at 2:12 PM, Jeff King wrote:\n> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:\n>\n>> If the commit does not have the SHOWN or UNINTERESTING flag set but it\n>> is going to get the UNINTERESTING flag set during the walk because of\n>> a negative commit listed on the command line, this patch won't help.\n>\n> Right, so my understanding of the situation is that doing this:\n>\n> $ git branch foo master~1\n> $ git fast-export foo master~1..master\n>\n> won't show \"foo\", which seems wrong to me. _But_ we currently get that\n> wrong already, so Felipe's patches are not making anything worse, but\n> are fixing some situations (namely when master~1 is not mentioned on the\n> command-line, but rather in a marks file).\n>\n> Is that correct?\n\nYes, that's correct. But my patch (\"make sure refs are updated\nproperly\") does _not_ change in any shape or form what happens with\nwhat you specify in the command line, only what happens with marks.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"202441","messageId":"CAMP44s1QPNCASyz3sVHPkJGv+PqrV5_Rt1ymVavupkaCtr-DJQ@mail.gmail.com","threadId":"31984","inReplyTo":"20121102151955.GA24622@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-02T15:35:29Z","receivedAt":"2012-11-02T15:35:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 2, 2012 at 4:19 PM, Jeff King wrote:\n> On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:\n\n>> May I just ask to include a summary of that rationale into the commit\n>> message rather than relying on people having internet access and knowing\n>> where to look? Adding the following to the commit message would be good\n>> enough for me:\n>>\n>> Note that\n>>\n>> $ git branch foo master~1\n>> $ git fast-export foo master~1..master\n>>\n>> still does not update the \"foo\" ref, but a partial fix is better\n>> than no fix.\n>\n> Yes, I think that makes a lot of sense.\n>\n> Felipe, I notice that you sent out a big \"fast-export improvements\"\n> series. Does that supersede this?\n\nYes. I noticed this patch fixes other tests.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}