{"thread":{"id":"32234","subject":"[PATCH v7 p2 0/2] fast-export fixes","startedAt":"2012-11-28T22:23:58Z","lastAt":"2012-12-02T02:07:38Z","messageCount":6,"participants":["Felipe Contreras","Max Horn","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":2},"messages":[{"id":"204224","messageId":"1354141440-26534-1-git-send-email-felipe.contreras@gmail.com","threadId":"32234","inReplyTo":null,"subject":"[PATCH v7 p2 0/2] fast-export fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-28T22:23:58Z","receivedAt":"2012-11-28T22:23:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nHere's version 7 part 2; I've dropped all the unnecessary patches, nobody seems\nto care about the current brokedness, and I did them only to show that these\nare correct, and that remote helpers without marks just don't work. These are\nthe ones I care about.\n\nBelow is a summary of what happens when you apply both patches, and how the new\nbehavior is obviously correct. All the refs point to the same object.\n\n== before ==\n\n % git fast-export --export--marks=marks master\n # exported stuff\n % git fast-export --{import,export}-marks=marks test\n # nothing\n % git fast-export --{import,export}-marks=marks master ^uninteresting\n reset refs/heads/uninteresting\n from :6\n\n % git fast-export --{import,export}-marks=marks ^uninteresting master ^foo test\n reset refs/heads/test\n from :6\n\n reset refs/heads/foo\n from :6\n\n reset refs/heads/master\n from :6\n\n % git fast-export --{import,export}-marks=marks uninteresting..master\n\n== after ==\n\n % git fast-export --export--marks=marks master\n # exported stuff\n % git fast-export --{import,export}-marks=marks test\n reset refs/heads/test\n from :6\n\n % git fast-export --{import,export}-marks=marks master ^uninteresting\n reset refs/heads/master\n from :6\n\n % git fast-export --{import,export}-marks=marks ^uninteresting master ^foo test\n reset refs/heads/test\n from :6\n\n reset refs/heads/master\n from :6\n\n % git fast-export --{import,export}-marks=marks uninteresting..master\n reset refs/heads/master\n from :6\n\nChanges since v6:\n\n * Drop all the extra patches\n * Reorder patches so tests never fail\n\nFelipe Contreras (2):\n fast-export: don't handle uninteresting refs\n fast-export: make sure updated refs get updated\n\n builtin/fast-export.c | 21 ++++++++++++++-------\n t/t5801-remote-helpers.sh | 28 ++++++++++++++++------------\n t/t9350-fast-export.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 75 insertions(+), 19 deletions(-)\n\n-- \n1.8.0.1\n"},{"id":"204225","messageId":"1354141440-26534-2-git-send-email-felipe.contreras@gmail.com","threadId":"32234","inReplyTo":"1354141440-26534-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-28T22:23:59Z","receivedAt":"2012-11-28T22:23:59Z","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:\n\n % git fast-export master ^uninteresting ^foo ^bar\n reset refs/heads/bar\n from :0\n\n reset refs/heads/foo\n from :0\n\n reset refs/heads/uninteresting\n from :0\n\n % git fast-export ^uninteresting ^foo ^bar master\n reset refs/heads/master\n from :0\n\n reset refs/heads/bar\n from :0\n\n reset refs/heads/foo\n from :0\n\nClearly this is wrong; the negative refs should be ignored.\n\nAfter this patch:\n\n % git fast-export ^uninteresting ^foo ^bar master\n # nothing\n % git fast-export master ^uninteresting ^foo ^bar\n # nothing\n\nAnd even more, it would only happen if the ref is pointing to exactly\nthe same commit, but not otherwise:\n\n % git fast-export ^next next\n reset refs/heads/next\n from :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\nThe reason this happens is that before traversing the commits,\nfast-export checks if any of the refs point to the same object, and any\nduplicated ref gets added to a list in order to issue 'reset' commands\nafter the traversing. Unfortunately, it's not even checking if the\ncommit is flagged as UNINTERESTING. The fix of course, is to do\nprecisely that.\n\nHowever, in order to do it properly we need to get the UNINTERESTING flag\nfrom the command line ref, not from the commit object. Fortunately we\ncan simply use revs.pending, which contains all the information we need\nfor get_tags_and_duplicates(), plus the ref flag. This way the rest of\nthe positive refs will remain untouched; it's only the negative ones\nthat change in behavior.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 11 +++++++----\n t/t5801-remote-helpers.sh | 8 ++++++++\n t/t9350-fast-export.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 45 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 191936c..2547e6c 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -474,18 +474,21 @@ static void handle_tag(const char *name, struct tag *tag)\n \t (int)message_size, (int)message_size, message ? message : \"\");\n }\n \n-static void get_tags_and_duplicates(struct object_array *pending,\n+static void get_tags_and_duplicates(struct rev_cmdline_info *info,\n \t\t\t\t struct string_list *extra_refs)\n {\n \tstruct tag *tag;\n \tint i;\n \n-\tfor (i = 0; i < pending->nr; i++) {\n-\t\tstruct object_array_entry *e = pending->objects + i;\n+\tfor (i = 0; i < info->nr; i++) {\n+\t\tstruct rev_cmdline_entry *e = info->rev + i;\n \t\tunsigned char sha1[20];\n \t\tstruct commit *commit;\n \t\tchar *full_name;\n \n+\t\tif (e->flags & UNINTERESTING)\n+\t\t\tcontinue;\n+\n \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n \t\t\tcontinue;\n \n@@ -681,7 +684,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (import_filename && revs.prune_data.nr)\n \t\tfull_tree = 1;\n \n-\tget_tags_and_duplicates(&revs.pending, &extra_refs);\n+\tget_tags_and_duplicates(&revs.cmdline, &extra_refs);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 12ae256..ece8fd5 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -162,4 +162,12 @@ test_expect_failure 'pushing without marks' '\n \tcompare_refs local2 HEAD server HEAD\n '\n \n+test_expect_success 'push all with existing object' '\n+\t(cd local &&\n+\tgit branch dup2 master &&\n+\tgit push origin --all\n+\t) &&\n+\tcompare_refs local dup2 server dup2\n+'\n+\n test_done\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 1f59862..c8e41c1 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -454,4 +454,34 @@ test_expect_success 'test bidirectionality' '\n \tgit fast-import --export-marks=marks-cur --import-marks=marks-cur\n '\n \n+cat > expected << EOF\n+blob\n+mark :13\n+data 5\n+bump\n+\n+commit refs/heads/master\n+mark :14\n+author A U Thor 1112912773 -0700\n+committer C O Mitter 1112912773 -0700\n+data 5\n+bump\n+from :12\n+M 100644 :13 file\n+\n+EOF\n+\n+test_expect_success 'avoid uninteresting refs' '\n+\t> tmp-marks &&\n+\tgit fast-export --import-marks=tmp-marks \\\n+\t\t--export-marks=tmp-marks master > /dev/null &&\n+\tgit tag v1.0 &&\n+\tgit branch uninteresting &&\n+\techo bump > file &&\n+\tgit commit -a -m bump &&\n+\tgit fast-export --import-marks=tmp-marks \\\n+\t\t--export-marks=tmp-marks ^uninteresting ^v1.0 master > actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.0.1\n"},{"id":"204226","messageId":"1354141440-26534-3-git-send-email-felipe.contreras@gmail.com","threadId":"32234","inReplyTo":"1354141440-26534-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v7 p2 2/2] fast-export: make sure updated refs get updated","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-28T22:24:00Z","receivedAt":"2012-11-28T22:24:00Z","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's\nflagged as SHOWN, so it will not be exported again, even if in a later\ntime it'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\nIOW: If it's specified in the command line, it will get updated,\nregardless of whether or not the object was marked.\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 whose objects have already been exported, and a few other issues as\nwell. Update the tests accordingly.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 10 +++++++---\n t/t5801-remote-helpers.sh | 20 ++++++++------------\n t/t9350-fast-export.sh | 15 +++++++++++++++\n 3 files changed, 30 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 2547e6c..77dffd1 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -526,10 +526,14 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,\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 updated eventually.\n+\t\t */\n+\t\tif (commit->util || commit->object.flags & SHOWN)\n \t\t\tstring_list_append(extra_refs, full_name)->util = commit;\n-\t\telse\n+\t\tif (!commit->util)\n \t\t\tcommit->util = full_name;\n \t}\n }\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex ece8fd5..f387027 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -63,18 +63,6 @@ test_expect_success 'fetch new branch' '\n \tcompare_refs server HEAD local FETCH_HEAD\n '\n \n-#\n-# This is only needed because of a bug not detected by this script. It will be\n-# fixed shortly, but for now lets not cause regressions.\n-#\n-test_expect_success 'bump commit in server' '\n-\t(cd server &&\n-\tgit checkout master &&\n-\techo content >>file &&\n-\tgit commit -a -m four) &&\n-\tcompare_refs server HEAD server HEAD\n-'\n-\n test_expect_success 'fetch multiple branches' '\n \t(cd local &&\n \t git fetch\n@@ -170,4 +158,12 @@ test_expect_success 'push all with existing object' '\n \tcompare_refs local dup2 server dup2\n '\n \n+test_expect_success 'push ref with existing object' '\n+\t(cd local &&\n+\tgit branch dup master &&\n+\tgit push origin dup\n+\t) &&\n+\tcompare_refs local dup server dup\n+'\n+\n test_done\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex c8e41c1..9320b4f 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -484,4 +484,19 @@ test_expect_success 'avoid uninteresting refs' '\n \ttest_cmp expected actual\n '\n \n+cat > expected << EOF\n+reset refs/heads/master\n+from :14\n+\n+EOF\n+\n+test_expect_success 'refs are updated even if no commits need to be exported' '\n+\t> tmp-marks &&\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.1\n"},{"id":"204230","messageId":"8FA492C2-71B0-44AB-B816-AFB6C91DC01C@quendi.de","threadId":"32234","inReplyTo":"1354141440-26534-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs","fromName":"Max Horn","fromEmail":"postbox@quendi.de","sentAt":"2012-11-29T01:16:28Z","receivedAt":"2012-11-29T01:16:28Z","isPatch":true,"sender":{"key":"postbox@quendi.de","avatar":null},"body":"\nOn 28.11.2012, at 23:23, Felipe Contreras wrote:\n\n> They have been marked as UNINTERESTING for a reason, lets respect that.\n> \n> Currently the first ref is handled properly, but not the rest:\n> \n> % git fast-export master ^uninteresting ^foo ^bar\n\nAll these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-).\n\n\n\n\n> The reason this happens is that before traversing the commits,\n> fast-export checks if any of the refs point to the same object, and any\n> duplicated ref gets added to a list in order to issue 'reset' commands\n> after the traversing. Unfortunately, it's not even checking if the\n> commit is flagged as UNINTERESTING. The fix of course, is to do\n> precisely that.\n\nHm... So this might be me being a stupid n00b (I am not yet that familiar with the internal rep of things in git and all...)... but I found the \"precisely that\" par very confusing, because right afterwards, you say:\n\n> \n> However, in order to do it properly we need to get the UNINTERESTING flag\n> from the command line ref, not from the commit object.\n\nSo this sounds like you are saying \"we do *precisely* that, except we don't, because it is more complicated, so we actually don't do this *precisely*, just manner of speaking...\"\n\nSome details here are beyond my knowledge, I am afraid, so I have to resort to guess: In particular it is not clear to me why the \"however\" part pops up: Reading it makes it sound as if the commit object also carries an UNINTERESTING flag, but we can't use it because of some reason (perhaps it doesn't have the semantics we need?), so we have to look at revs.pending instead. Right? Wrong? Or is it because the commit objects actually do *not* carry the UNINTERESTING bits, hence we need to look at revs.pending. Or is it due to yet another reason?\n\nI would find it helpful if that could be clarified. E.g. like so:\n\n \"The fix is to add such a check. However, we cannot just use the UNINTERESTING flag of the commit object, because INSERT-REASON.\"\n\nor\n\n \"The fix is to add such a check. However, the commit object does not contain the UNINTERESTING flag directly.\"\n\nor something.\n\nAnyway, other than these nitpicky questions, this whole thing looks very logical to me, description and code alike. I also played around with tons of \"fast-export\" invocations, with and without this patch, and it seems to do what the description says. Finally, I went to the various long threads discussion prior versions of this patch, in particular those starting at\n http://thread.gmane.org/gmane.comp.version-control.git/208725\nand\n http://thread.gmane.org/gmane.comp.version-control.git/209355/focus=209370\n\nThese contained some concerns. Sadly, several of those discussions ultimately degenerated into not-so-pleasant exchanges :-(, and my impression is that as a result some people are not so inclined to comment on these patches anymore at all. Which is a pity :-(. But overall, it seems this patch makes nothing worse, but fixes some things; and it is simple enough that it shouldn't make future improvements harder.\n\nSo *I* at least am quite happy with this, it helps me! My impression is that Felipe's latest patch addresses most concerns people raised by means of an improved description. I couldn't find any in those threads that I feel still applies -- but of course those people should speak for themselves, I am simply afraid they don't want to be part of this anymore :-(.\n\n\nStill, for what little it might be worth, I think this patch is good and a real improvement. I hope it can be merged soon.\n\n\nCheers,\nMax\n\n\n> Fortunately we\n> can simply use revs.pending, which contains all the information we need\n> for get_tags_and_duplicates(), plus the ref flag. This way the rest of\n> the positive refs will remain untouched; it's only the negative ones\n> that change in behavior.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/fast-export.c | 11 +++++++----\n> t/t5801-remote-helpers.sh | 8 ++++++++\n> t/t9350-fast-export.sh | 30 ++++++++++++++++++++++++++++++\n> 3 files changed, 45 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 191936c..2547e6c 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -474,18 +474,21 @@ static void handle_tag(const char *name, struct tag *tag)\n> \t (int)message_size, (int)message_size, message ? message : \"\");\n> }\n> \n> -static void get_tags_and_duplicates(struct object_array *pending,\n> +static void get_tags_and_duplicates(struct rev_cmdline_info *info,\n> \t\t\t\t struct string_list *extra_refs)\n> {\n> \tstruct tag *tag;\n> \tint i;\n> \n> -\tfor (i = 0; i < pending->nr; i++) {\n> -\t\tstruct object_array_entry *e = pending->objects + i;\n> +\tfor (i = 0; i < info->nr; i++) {\n> +\t\tstruct rev_cmdline_entry *e = info->rev + i;\n> \t\tunsigned char sha1[20];\n> \t\tstruct commit *commit;\n> \t\tchar *full_name;\n> \n> +\t\tif (e->flags & UNINTERESTING)\n> +\t\t\tcontinue;\n> +\n> \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n> \t\t\tcontinue;\n> \n> @@ -681,7 +684,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> \tif (import_filename && revs.prune_data.nr)\n> \t\tfull_tree = 1;\n> \n> -\tget_tags_and_duplicates(&revs.pending, &extra_refs);\n> +\tget_tags_and_duplicates(&revs.cmdline, &extra_refs);\n> \n> \tif (prepare_revision_walk(&revs))\n> \t\tdie(\"revision walk setup failed\");\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 12ae256..ece8fd5 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -162,4 +162,12 @@ test_expect_failure 'pushing without marks' '\n> \tcompare_refs local2 HEAD server HEAD\n> '\n> \n> +test_expect_success 'push all with existing object' '\n> +\t(cd local &&\n> +\tgit branch dup2 master &&\n> +\tgit push origin --all\n> +\t) &&\n> +\tcompare_refs local dup2 server dup2\n> +'\n> +\n> test_done\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 1f59862..c8e41c1 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -454,4 +454,34 @@ test_expect_success 'test bidirectionality' '\n> \tgit fast-import --export-marks=marks-cur --import-marks=marks-cur\n> '\n> \n> +cat > expected << EOF\n> +blob\n> +mark :13\n> +data 5\n> +bump\n> +\n> +commit refs/heads/master\n> +mark :14\n> +author A U Thor 1112912773 -0700\n> +committer C O Mitter 1112912773 -0700\n> +data 5\n> +bump\n> +from :12\n> +M 100644 :13 file\n> +\n> +EOF\n> +\n> +test_expect_success 'avoid uninteresting refs' '\n> +\t> tmp-marks &&\n> +\tgit fast-export --import-marks=tmp-marks \\\n> +\t\t--export-marks=tmp-marks master > /dev/null &&\n> +\tgit tag v1.0 &&\n> +\tgit branch uninteresting &&\n> +\techo bump > file &&\n> +\tgit commit -a -m bump &&\n> +\tgit fast-export --import-marks=tmp-marks \\\n> +\t\t--export-marks=tmp-marks ^uninteresting ^v1.0 master > actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n> -- \n> 1.8.0.1\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"204323","messageId":"CAMP44s08Jfu08oeABHcy=xPtn=LZfKTdbaRZuDbf7g+RiP7xAA@mail.gmail.com","threadId":"32234","inReplyTo":"8FA492C2-71B0-44AB-B816-AFB6C91DC01C@quendi.de","subject":"Re: [PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-30T05:57:18Z","receivedAt":"2012-11-30T05:57:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 29, 2012 at 2:16 AM, Max Horn wrote:\n>\n> On 28.11.2012, at 23:23, Felipe Contreras wrote:\n>\n>> They have been marked as UNINTERESTING for a reason, lets respect that.\n>>\n>> Currently the first ref is handled properly, but not the rest:\n>>\n>> % git fast-export master ^uninteresting ^foo ^bar\n>\n> All these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-).\n\nYeah, they point to the same object.\n\n> \n>\n>> The reason this happens is that before traversing the commits,\n>> fast-export checks if any of the refs point to the same object, and any\n>> duplicated ref gets added to a list in order to issue 'reset' commands\n>> after the traversing. Unfortunately, it's not even checking if the\n>> commit is flagged as UNINTERESTING. The fix of course, is to do\n>> precisely that.\n>\n> Hm... So this might be me being a stupid n00b (I am not yet that familiar with the internal rep of things in git and all...)... but I found the \"precisely that\" par very confusing, because right afterwards, you say:\n\nYeah, the next part was added afterwards.\n\n>> However, in order to do it properly we need to get the UNINTERESTING flag\n>> from the command line ref, not from the commit object.\n>\n> So this sounds like you are saying \"we do *precisely* that, except we don't, because it is more complicated, so we actually don't do this *precisely*, just manner of speaking...\"\n\nWell, we do check fro the UNINTERESTING flag, but on the ref, not on the commit.\n\n> Some details here are beyond my knowledge, I am afraid, so I have to resort to guess: In particular it is not clear to me why the \"however\" part pops up: Reading it makes it sound as if the commit object also carries an UNINTERESTING flag, but we can't use it because of some reason (perhaps it doesn't have the semantics we need?), so we have to look at revs.pending instead. Right? Wrong? Or is it because the commit objects actually do *not* carry the UNINTERESTING bits, hence we need to look at revs.pending. Or is it due to yet another reason?\n\nIt's actually revs.cmdline, I typed the wrong one.\n\nIf you have two refs pointing to the same object, and you do 'one\n^two', the object (e.g. 8c7a786) will get the UNINTERESTING flag, but\nthat doesn't tell us anything about the ref being a positive or a\nnegative one, and revs.pending only has the object flags. On the other\nhand revs.cmdline does have the flags for the refs.\n\nDoes that explain it?\n\n> Anyway, other than these nitpicky questions, this whole thing looks very logical to me, description and code alike. I also played around with tons of \"fast-export\" invocations, with and without this patch, and it seems to do what the description says. Finally, I went to the various long threads discussion prior versions of this patch, in particular those starting at\n> http://thread.gmane.org/gmane.comp.version-control.git/208725\n> and\n> http://thread.gmane.org/gmane.comp.version-control.git/209355/focus=209370\n>\n> These contained some concerns. Sadly, several of those discussions ultimately degenerated into not-so-pleasant exchanges :-(, and my impression is that as a result some people are not so inclined to comment on these patches anymore at all. Which is a pity :-(. But overall, it seems this patch makes nothing worse, but fixes some things; and it is simple enough that it shouldn't make future improvements harder.\n>\n> So *I* at least am quite happy with this, it helps me! My impression is that Felipe's latest patch addresses most concerns people raised by means of an improved description. I couldn't find any in those threads that I feel still applies -- but of course those people should speak for themselves, I am simply afraid they don't want to be part of this anymore :-(.\n\nIndeed. For all the concerns given I made a response to how that\neither is not true, or doesn't really matter, and in the case of the\nlatter, I asked for examples where it would matter, only to receive\nnothing. For whatever reason involved people are not responding, not a\nsingle valid concern has been raised and remained.\n\nSo I think it's good.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"204402","messageId":"7vsj7pmck5.fsf@alter.siamese.dyndns.org","threadId":"32234","inReplyTo":"CAMP44s08Jfu08oeABHcy=xPtn=LZfKTdbaRZuDbf7g+RiP7xAA@mail.gmail.com","subject":"Re: [PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-02T02:07:38Z","receivedAt":"2012-12-02T02:07:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Thu, Nov 29, 2012 at 2:16 AM, Max Horn wrote:\n>>\n>> On 28.11.2012, at 23:23, Felipe Contreras wrote:\n>>\n>>> They have been marked as UNINTERESTING for a reason, lets respect that.\n>>>\n>>> Currently the first ref is handled properly, but not the rest:\n>>>\n>>> % git fast-export master ^uninteresting ^foo ^bar\n>>\n>> All these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-).\n>\n> Yeah, they point to the same object.\n\nDo you want me to amend the log message of that commit to clarify\nthis?\n\n>> \n>>\n> ...\n> It's actually revs.cmdline, I typed the wrong one.\n> ...\n> So I think it's good.\n\nWait.\n\nI at least read two points above you said what you wrote in the\ncommit was not corrrect and misleading to later readers. And then I\nhear \"it's good\". Which one?\n\nAre you merely saying that it is easily fixable to become good? If\nso, what do you want to do with these not-so-good part?\n\nIf you want to ask me to amend, that is fine, but do so in a more\nexplicit way, not in a message at the tail of long thread that is\nnot even CC'ed to me.\n\nOf course, a proper re-roll like everybody else does is just fine.\n\nThanks.\n"}]}