{"thread":{"id":"27908","subject":"[PATCH 4/5] fast-export: do not export negative refs","startedAt":"2011-07-24T14:21:17Z","lastAt":"2011-08-08T23:39:50Z","messageCount":29,"participants":["Sverre Rabbelier","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"171952","messageId":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":null,"subject":"[PATCH 0/5] fast-export: prepare for remote helpers awesomeness","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:17Z","receivedAt":"2011-07-24T14:21:17Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"During a funathon with a lot of sheep baaah-ing in the background, we\ndiscovered this ancient fast-export bug while working on finishing\ngit-remote-hg. So, here it goes.\n\nJohannes Schindelin (4):\n fast-export: do not refer to non-existing marks\n setup_revisions: remember whether a ref was positive or not\n fast-export: do not export negative refs\n setup_revisions: remember whether a ref was positive or not\n\nSverre Rabbelier (1):\n t9350: point out that refs are not updated correctly\n\n builtin/fast-export.c | 55 +++++++++++++++++++++++++++++++++++++++--------\n object.h | 2 +-\n revision.c | 12 ++++++----\n t/t9350-fast-export.sh | 11 +++++++++\n 4 files changed, 64 insertions(+), 16 deletions(-)\n\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171953","messageId":"1311517282-24831-2-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 1/5] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:18Z","receivedAt":"2011-07-24T14:21:18Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"This happens only when the corresponding commits are not exported in\nthe current fast-export run. This can happen either when the relevant\ncommit is already marked, or when the commit is explicitly marked\nas UNINTERESTING with a negative ref by another argument.\n\nThis breaks fast-export basec remote helpers.\n\nSigned-off-by: Sverre Rabbelier \n---\n\n Obviously the real occurance of the breakage is not as artificial\n as this test case, but it's a very simple minimal reproducer.\n\n t/t9350-fast-export.sh | 11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex f823c05..ed0417a 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -414,4 +414,15 @@ test_expect_success SYMLINKS 'directory becomes symlink' '\n \t(cd result && git show master:foo)\n '\n \n+cat > expected << EOF\n+reset refs/heads/master\n+from $(git rev-parse master)\n+\n+EOF\n+\n+test_expect_failure 'refs are updated even if no commits need to be exported' '\n+\tgit fast-export master..master > actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171950","messageId":"1311517282-24831-3-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 2/5] fast-export: do not refer to non-existing marks","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:19Z","receivedAt":"2011-07-24T14:21:19Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"From: Johannes Schindelin \n\nWhen calling `git fast-export a..a b` when a and b refer to the same\ncommit, nothing would be exported, and an incorrect reset line would\nbe printed for b ('from :0').\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \n---\n\n We encountered this one while staring at the code and trying to\n figure out if our existing solution was sufficient.\n\n builtin/fast-export.c | 17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex becef85..92743c8 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -512,9 +512,20 @@ static void get_tags_and_duplicates(struct object_array *pending,\n \t}\n }\n \n+static void handle_reset(const char *name, struct object *object)\n+{\n+\tint mark = get_object_mark(object);\n+\n+\tif (mark)\n+\t\tprintf(\"reset %s\\nfrom :%d\\n\\n\", name,\n+\t\t get_object_mark(object));\n+\telse\n+\t\tprintf(\"reset %s\\nfrom %s\\n\\n\", name,\n+\t\t sha1_to_hex(object->sha1));\n+}\n+\n static void handle_tags_and_duplicates(struct string_list *extra_refs)\n {\n-\tstruct commit *commit;\n \tint i;\n \n \tfor (i = extra_refs->nr - 1; i >= 0; i--) {\n@@ -526,9 +537,7 @@ static void handle_tags_and_duplicates(struct string_list *extra_refs)\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\n \t\t\t/* create refs pointing to already seen commits */\n-\t\t\tcommit = (struct commit *)object;\n-\t\t\tprintf(\"reset %s\\nfrom :%d\\n\\n\", name,\n-\t\t\t get_object_mark(&commit->object));\n+\t\t\thandle_reset(name, object);\n \t\t\tshow_progress();\n \t\t\tbreak;\n \t\t}\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171951","messageId":"1311517282-24831-4-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:20Z","receivedAt":"2011-07-24T14:21:20Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"From: Johannes Schindelin \n\nThis will be required by fast-export, when no commits were\nexported, but the refs should be set, of course.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \n---\n\n This is just in preperation of fixing the actual bug.\n\n object.h | 2 +-\n revision.c | 12 +++++++-----\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/object.h b/object.h\nindex b6618d9..a28cd45 100644\n--- a/object.h\n+++ b/object.h\n@@ -12,7 +12,7 @@ struct object_array {\n \tstruct object_array_entry {\n \t\tstruct object *item;\n \t\tconst char *name;\n-\t\tunsigned mode;\n+\t\tunsigned mode, flags;\n \t} *objects;\n };\n \ndiff --git a/revision.c b/revision.c\nindex c46cfaa..72c3ee5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -131,7 +131,7 @@ void mark_parents_uninteresting(struct commit *commit)\n \t}\n }\n \n-static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)\n+static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode, unsigned flags)\n {\n \tif (!obj)\n \t\treturn;\n@@ -152,11 +152,12 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o\n \t\t\treturn;\n \t}\n \tadd_object_array_with_mode(obj, name, &revs->pending, mode);\n+\trevs->pending.objects[revs->pending.nr-1].flags = flags;\n }\n \n void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n {\n-\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n+\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n }\n \n void add_head_to_pending(struct rev_info *revs)\n@@ -1073,7 +1074,8 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\t\t} else\n \t\t\t\ta->object.flags |= flags_exclude;\n \t\t\tb->object.flags |= flags;\n-\t\t\tadd_pending_object(revs, &a->object, this);\n+\t\t\tadd_pending_object_with_mode(revs, &a->object, this,\n+\t\t\t\t\t\t S_IFINVALID, flags_exclude);\n \t\t\tadd_pending_object(revs, &b->object, next);\n \t\t\treturn 0;\n \t\t}\n@@ -1103,7 +1105,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \tif (!cant_be_filename)\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n-\tadd_pending_object_with_mode(revs, object, arg, mode);\n+\tadd_pending_object_with_mode(revs, object, arg, mode, local_flags);\n \treturn 0;\n }\n \n@@ -1708,7 +1710,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tif (get_sha1_with_mode(revs->def, sha1, &mode))\n \t\t\tdie(\"bad default revision '%s'\", revs->def);\n \t\tobject = get_reference(revs, revs->def, sha1, 0);\n-\t\tadd_pending_object_with_mode(revs, object, revs->def, mode);\n+\t\tadd_pending_object_with_mode(revs, object, revs->def, mode, 0);\n \t}\n \n \t/* Did the user ask for any diff output? Run the diff! */\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171949","messageId":"1311517282-24831-5-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 4/5] fast-export: do not export negative refs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:21Z","receivedAt":"2011-07-24T14:21:21Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"From: Johannes Schindelin \n\nWhen calling `git fast-export master..next` we want to export\nrefs/heads/next, but not refs/heads/master.\n\nCurrently this is not a problem, because negative refs' commits\nare never shown. In the next commit this will be changed in order\nto make sure that 'master..master' does export master. I.e. even\nrefs whose commits are not shown are exported.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \n---\n\n We noticed this while trying to come up with a test case for the\n bug we found.\n\n builtin/fast-export.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 92743c8..628eab0 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -467,7 +467,7 @@ static void get_tags_and_duplicates(struct object_array *pending,\n \t\tstruct commit *commit = commit;\n \t\tchar *full_name;\n \n-\t\tif (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n+\t\tif ((e->flags & UNINTERESTING) || dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)\n \t\t\tcontinue;\n \n \t\tswitch (e->item->type) {\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171954","messageId":"1311517282-24831-6-git-send-email-srabbelier@gmail.com","threadId":"27908","inReplyTo":"1311517282-24831-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 5/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-07-24T14:21:22Z","receivedAt":"2011-07-24T14:21:22Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"From: Johannes Schindelin \n\nThis will be required by fast-export, when no commits were\nexported, but the refs should be set, of course.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \n---\n\n And then finally, the fix. Thanks for reading.\n\n builtin/fast-export.c | 36 +++++++++++++++++++++++++++++++-----\n t/t9350-fast-export.sh | 2 +-\n 2 files changed, 32 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 628eab0..9c24f4e 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -17,6 +17,8 @@\n #include \"utf8.h\"\n #include \"parse-options.h\"\n \n+#define REF_HANDLED (ALL_REV_FLAGS + 1)\n+\n static const char *fast_export_usage[] = {\n \t\"git fast-export [rev-list-opts]\",\n \tNULL\n@@ -456,6 +458,7 @@ static void handle_tag(const char *name, struct tag *tag)\n }\n \n static void get_tags_and_duplicates(struct object_array *pending,\n+\t\t\t\t struct string_list *refs,\n \t\t\t\t struct string_list *extra_refs)\n {\n \tstruct tag *tag;\n@@ -507,8 +510,11 @@ static void get_tags_and_duplicates(struct object_array *pending,\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\telse {\n \t\t\tcommit->util = full_name;\n+\t\t\t/* we might need to set this ref explicitly */\n+\t\t\tstring_list_append(refs, full_name)->util = commit;\n+\t\t}\n \t}\n }\n \n@@ -524,10 +530,29 @@ static void handle_reset(const char *name, struct object *object)\n \t\t sha1_to_hex(object->sha1));\n }\n \n-static void handle_tags_and_duplicates(struct string_list *extra_refs)\n+static void handle_tags_and_duplicates(struct string_list *refs, struct string_list *extra_refs)\n {\n \tint i;\n \n+\t/* even if no commits were exported, we need to export the ref */\n+\tfor (i = refs->nr - 1; i >= 0; i--) {\n+\t\tconst char *name = refs->items[i].string;\n+\t\tstruct object *object = refs->items[i].util;\n+\n+\t\tif (!(object->flags & REF_HANDLED)) {\n+\t\t\tif (object->type & OBJ_TAG)\n+\t\t\t\thandle_tag(name, (struct tag *)object);\n+\t\t\telse {\n+\t\t\t\tif (!prefixcmp(name, \"refs/tags/\") &&\n+\t\t\t\t (tag_of_filtered_mode != REWRITE ||\n+\t\t\t\t !get_object_mark(object)))\n+\t\t\t\t\tcontinue;\n+\t\t\t\thandle_reset(name, object);\n+\t\t\t\tobject->flags |= REF_HANDLED;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tfor (i = extra_refs->nr - 1; i >= 0; i--) {\n \t\tconst char *name = extra_refs->items[i].string;\n \t\tstruct object *object = extra_refs->items[i].util;\n@@ -617,7 +642,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n \tstruct object_array commits = OBJECT_ARRAY_INIT;\n-\tstruct string_list extra_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list refs = STRING_LIST_INIT_NODUP, extra_refs = STRING_LIST_INIT_NODUP;\n \tstruct commit *commit;\n \tchar *export_filename = NULL, *import_filename = NULL;\n \tstruct option options[] = {\n@@ -669,7 +694,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.pending, &refs, &extra_refs);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n@@ -681,11 +706,12 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\telse {\n \t\t\thandle_commit(commit, &revs);\n+\t\t\tcommit->object.flags |= REF_HANDLED;\n \t\t\thandle_tail(&commits, &revs);\n \t\t}\n \t}\n \n-\thandle_tags_and_duplicates(&extra_refs);\n+\thandle_tags_and_duplicates(&refs, &extra_refs);\n \n \tif (export_filename)\n \t\texport_marks(export_filename);\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex ed0417a..d2d7ef8 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -420,7 +420,7 @@ from $(git rev-parse master)\n \n EOF\n \n-test_expect_failure 'refs are updated even if no commits need to be exported' '\n+test_expect_success 'refs are updated even if no commits need to be exported' '\n \tgit fast-export master..master > actual &&\n \ttest_cmp expected actual\n '\n-- \n1.7.6.385.g91185.dirty\n"},{"id":"171965","messageId":"7v39hvts4i.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"1311517282-24831-5-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 4/5] fast-export: do not export negative refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-24T19:07:09Z","receivedAt":"2011-07-24T19:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> ... In the next commit this will be changed in order\n> to make sure that 'master..master' does export master.\n\nSorry, but you need to be much more clear why this could be a good\nthing.\n\nEverywhere in git a range A..B means \"reachable from B but exclude\neverything that is reachable from A\", and with that knowledge it is\nnatural to expect that master..master should yield an empty set. If you\nneed to change it for some reason, we need to know why, and if you are\ndoing it for a narrow special case, we need to know why that case is so\nspecial.\n"},{"id":"171968","messageId":"7vy5znscst.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"1311517282-24831-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-24T19:23:30Z","receivedAt":"2011-07-24T19:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n> {\n> -\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n> +\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n> }\n\nThis seems utterly broken. For example, fmt-merge-msg.c adds \"^HEAD\" and\nof course the flags on the object is UNINTERESTING. Has all the callers of\nadd_pending_object() been verified? Why is it passing an unconditional 0\nand not !!(obj->flags & UNINTERESTING) or something?\n\nIf the excuse is \"this is only to help fast-export and other callers of\nadd_pending_object() does not care\", that is a sloppy attitude that breaks\nmaintainability of the code (because it forgets to add \"in the current\ncode nobody looks at the new 'flags' field\" to that excuse, and also does\nnot have any comments around this code that says so); it is questionable\nif such a hack belongs to a patch that touches object.h.\n\n> @@ -1073,7 +1074,8 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n> \t\t\t} else\n> \t\t\t\ta->object.flags |= flags_exclude;\n> \t\t\tb->object.flags |= flags;\n> -\t\t\tadd_pending_object(revs, &a->object, this);\n> +\t\t\tadd_pending_object_with_mode(revs, &a->object, this,\n> +\t\t\t\t\t\t S_IFINVALID, flags_exclude);\n> ...\n> @@ -1103,7 +1105,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n> \tif (!cant_be_filename)\n> \t\tverify_non_filename(revs->prefix, arg);\n> \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n> -\tadd_pending_object_with_mode(revs, object, arg, mode);\n> +\tadd_pending_object_with_mode(revs, object, arg, mode, local_flags);\n> \treturn 0;\n> }\n\nQuestionable. Did the user mean to say Z is positive when he said\n\n\t$ git rev-list A B ^C ... --not G H ... ^Z\n"},{"id":"171973","messageId":"7vr55fs1z0.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"7vy5znscst.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-24T23:17:23Z","receivedAt":"2011-07-24T23:17:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Questionable. Did the user mean to say Z is positive when he said\n>\n> \t$ git rev-list A B ^C ... --not G H ... ^Z\n\nHaving said all that, I think you wanted a way to reconstruct various\ndifferent command lines that ends up in the same set of \"pending objects\",\nand I do think that is something we need to address, as our command line\nparsing heavily depend on the preprocessing phase of the revision\ntraversal machinery, and some commands do want to act differently between\nthe case when they are given \"master\" vs \"master^0\" for example (i.e. does\nthe user mean the branch as a whole or the commit at the tip? If the\nformer the command may want to do something to the refs/heads/master while\nthe latter the command may work only on the commit or outright reject the\nrequest saying \"I only work on a branch\").\n\nIn other words, I am not opposed to an effort to give the callers to the\n\"pending objects\" machinery a better way to discover what the user told us\nfrom the command line, giving them more than just \"at the end of the\nUNINTERESTING marking here are the objects listed on the command line and\nyou can look at their flags\". For example, some commands may want to tell\n\"a..b\" and \"^a b\" apart, and other commands may want to tell what \"a\" was\nwhen the user asked for exotic things like \"a^@\" or \"a^!\".\n\nIt may make sense to change the new \"flag\" field that can express only one\nbit in your implementation to something more descriptive to express \"what\ncommand line option did this come from\". For example,\n\n\tgit cmd a...b\n\nthat calls setup_revisions() may put \"a\" (positive), \"b\" (positive), and\nzero or more commits that are their merge bases (negative) in pending\nobjects queue. Right now, these pending object entries may have some part\nof the string being parsed as their name, but we may want to annotate all\nthe object array entries that result from \"a...b\" with the same \"a...b\"\n(or a structure that represents its parsed form), so that the caller can\nnotice things like (1) they came from the single command line argument,\n(2) the user didn't list random negative commits but they are meant as\nmerge bases between a and b, (3) a and b were written as such, not as\ntheir abbreviated object names, etc.\n\nThen \"git fast-export master..master\" can notice that there are two object\narray entries in the pending object queue, they came from the same command\nline argument, whose parse tree is a \"unsymmetric difference between\nstring 'master' and string 'master'\", and take the last piece to convince\nitself that the user is talking about the master branch, even though as\nthe set arithmetic the result is an empty set, and do something\nintelligent about the master branch.\n"},{"id":"172084","messageId":"alpine.DEB.1.00.1107261709070.1534@s15462909.onlinehome-server.info","threadId":"27908","inReplyTo":"7v39hvts4i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/5] fast-export: do not export negative refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-26T15:11:03Z","receivedAt":"2011-07-26T15:11:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 24 Jul 2011, Junio C Hamano wrote:\n\n> Sverre Rabbelier writes:\n> \n> > ... In the next commit this will be changed in order to make sure that \n> > 'master..master' does export master.\n> \n> Sorry, but you need to be much more clear why this could be a good \n> thing.\n\nMaybe we should have written 'master^0..master' or 'blub..blah' where \n'blah' points to the same commit as 'blub'.\n\nThe problem is that we need to be able to tell which _ref_ is \nuninteresting, not which _commit_, since one and the same commit can be \nreferenced by an uninteresting and an interesting ref.\n\nHopefully you will excuse our being a little incoherent after a codathon,\nDscho\n"},{"id":"172085","messageId":"alpine.DEB.1.00.1107261713040.1534@s15462909.onlinehome-server.info","threadId":"27908","inReplyTo":"7vy5znscst.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-26T15:16:06Z","receivedAt":"2011-07-26T15:16:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 24 Jul 2011, Junio C Hamano wrote:\n\n> Sverre Rabbelier writes:\n> \n> > void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n> > {\n> > -\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n> > +\tadd_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n> > }\n> \n> This seems utterly broken. For example, fmt-merge-msg.c adds \"^HEAD\" \n> and of course the flags on the object is UNINTERESTING. Has all the \n> callers of add_pending_object() been verified? Why is it passing an \n> unconditional 0 and not !!(obj->flags & UNINTERESTING) or something?\n> \n> If the excuse is \"this is only to help fast-export and other callers of \n> add_pending_object() does not care\", that is a sloppy attitude that \n> breaks maintainability of the code (because it forgets to add \"in the \n> current code nobody looks at the new 'flags' field\" to that excuse, and \n> also does not have any comments around this code that says so); it is \n> questionable if such a hack belongs to a patch that touches object.h.\n\nI hope you appreciate our effort not to hack this into the 'mode' \nvariable. That variable is exactly in the spirit you mentioned, so I \nfigured we'd be fine with a similar approach.\n\nNow that you have made clear that it is not fine, would you please care to \nenlighten me how you would prefer the change to look like that makes the \nargument parsing in revision.c remember which _refs_ (as opposed to \n_commits_) were marked uninteresting?\n\nThank you very much,\nJohannes\n"},{"id":"172766","messageId":"CAGdFq_ghxFdpjxCgTNbqXWGpt0rpJaGZ1_h+ZC71PzaPzbQ-0A@mail.gmail.com","threadId":"27908","inReplyTo":"7vr55fs1z0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-03T13:52:16Z","receivedAt":"2011-08-03T13:52:16Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jul 25, 2011 at 01:17, Junio C Hamano wrote:\n> In other words, I am not opposed to an effort to give the callers to the\n> \"pending objects\" machinery a better way to discover what the user told us\n> from the command line, giving them more than just \"at the end of the\n> UNINTERESTING marking here are the objects listed on the command line and\n> you can look at their flags\".  For example, some commands may want to tell\n> \"a..b\" and \"^a b\" apart, and other commands may want to tell what \"a\" was\n> when the user asked for exotic things like \"a^@\" or \"a^!\".\n\nI agree that such might be useful, but since we currently do not need\nsomething as advanced as that, I'm hesitant to implement something\n(far) more advanced than what we need (YAGNI [0]). So I'm thinking\nthat perhaps there's a suitable middle ground where we stick with the\ncurrent flags approach until someone needs something more advanced so\nas to make sure that it's implemented in a way that meets an actual\nimplementer need?\n\nOn Sun, Jul 24, 2011 at 21:23, Junio C Hamano wrote:\n> Sverre Rabbelier writes:\n>\n>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n>>  {\n>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n>>  }\n>\n> This seems utterly broken.  For example, fmt-merge-msg.c adds \"^HEAD\" and\n> of course the flags on the object is UNINTERESTING. Has all the callers of\n> add_pending_object() been verified? Why is it passing an unconditional 0\n> and not !!(obj->flags & UNINTERESTING) or something?\n\nIf I understand correctly (and it's not unlikely that I don't), the\n'flags' field is used to store the actual flags (not just a boolean).\nWould the following be appropriate?\n\n+     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);\n\nHopefully Dscho remembers enough of our hacking to be able to answer\nthat question.\n\nIf it is, do you still think it's \"utterly broken\" or would that be\nsomething we can work with?\n\n[0] http://c2.com/xp/YouArentGonnaNeedIt.html\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"172789","messageId":"7vy5zabbz7.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_ghxFdpjxCgTNbqXWGpt0rpJaGZ1_h+ZC71PzaPzbQ-0A@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-03T18:12:44Z","receivedAt":"2011-08-03T18:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Sun, Jul 24, 2011 at 21:23, Junio C Hamano wrote:\n>> Sverre Rabbelier writes:\n>>\n>>> void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n>>> {\n>>> - add_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n>>> + add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n>>> }\n>>\n>> This seems utterly broken. For example, fmt-merge-msg.c adds \"^HEAD\" and\n>> of course the flags on the object is UNINTERESTING. Has all the callers of\n>> add_pending_object() been verified? Why is it passing an unconditional 0\n>> and not !!(obj->flags & UNINTERESTING) or something?\n>\n> If I understand correctly (and it's not unlikely that I don't), the\n> 'flags' field is used to store the actual flags (not just a boolean).\n> Would the following be appropriate?\n>\n> + add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);\n\nI would think that the information you are trying to convey is more in\nline with the spirit of \"name\" field, not \"mode\". Originally, the pending\nobject array was to only collect the given arguments at the object level\nwith \"is this interesting or uninteresting?\" flag and nothing more, but we\nlater wanted to have richer information to deduce what the user wanted\nfrom how the user gave the object information to us (i.e. \"not just the\ntree object named by the object name, but I meant the object at this\npath\"), primarily to give a better error message.\n\nLet's clarify what I hinted as \"some parsed representation of how the revs\nwere specified\" in an earlier message with a concrete example. There is a\ncode block at the end of builtin/diff.c that says something like:\n\n\tIf we have more than 2 tree-ishes, and the first one is marked as\n\tUNINTERESTING, it must have come from \"diff A...B\". In this case,\n\twe know ent[0] is one of the merge-bases, ent[ents-2] is A, and\n\tent[ents-1] is B, so turn it into \"diff ent[0] B\".\n\nThese entries in ent[] come from the pending objects array, and because\nthe revision machinery loses information regarding how the command line\narguments were spelled, we have to play such games to _guess_ what the\nuser meant to say. This will lead to \"diff ^C A B\" running \"diff C B\",\ninstead of barfing with \"What are you talking about???\", which should\nhappen instead.\n\nWouldn't it be wonderful if the revision machinery left richer clue in\neach element of the pending object array while parsing, so that the caller\ndoes not have to guess? For example, suppose that it parsed \"A...B\" into\nthis N element array of pending objects:\n\n\t^MB0 (the first merge base of A and B)\n\t^MB1 (the second merge base of A and B)\n ...\n\tA\n\tB\n\nIn addition to a single \"mode\" integer, which says if it is supposed to be\na tree or a blob, we could allocate a single structure that records\nsomething like this:\n\n\tstruct parsed_rev {\n enum {\n\t\t\tSHA1, REF, RANGE, SYMMETRIC_RANGE, REFLOG_ENT, ...\n // there are others like OBJ^!, OBJ@!, ...\n } kind;\n const char *string;\n union {\n\t\t\tstruct {\n\t\t\t\tconst char *real_ref;\n\t\t\t} ref;\n\t\t\tstruct {\n\t\t\t\tstruct parsed_rev *bottom;\n\t\t\t\tstruct parsed_rev *top;\n } range;\n\t\t\t...\n } u;\n\t};\n\nAnd then all the elements in the pending object array resulting from\nparsing \"maint...master\" would all point at a single instance of this\n\"struct parsed_rev\" that may read like this:\n\n\t{\n \t.kind = SYMMETRIC_RANGE;\n\t\t.string = \"maint...master\";\n .u.range = {\n \t.bottom = {\n \t.kind = REF;\n .string = \"maint\";\n \t\t.u.ref.real_ref = \"refs/heads/maint\";\n\t\t\t};\n \t.top = {\n \t.kind = REF;\n .string = \"master\";\n \t\t.u.ref.real_ref = \"refs/heads/master\";\n\t\t\t};\n\t\t};\n\t};\n\nIn a similar fashion, if you wanted to make sure that you do not discard\n\"master\" as totally uninteresting, even though the end user gave you a\nlist of arguments that ends up marking the commit object \"master\" as\nuninteresting, e.g. \"master^0..master\", you could do so if we updated the\nrevision parsing machinery so that the resulting two elements in the\npending array would both point at (in addition to the \"uninteresting\ncommit object 'master^0'\") a structure that would look like this:\n\n\t{\n \t.kind = RANGE;\n .string = \"master^0..master\";\n .u.range = {\n\t\t\t...\n .top = {\n\t\t\t\t.kind = REF;\n .string = \"master\";\n \t\t.u.ref.real_ref = \"refs/heads/master\";\n\t\t\t};\n\t\t};\n\t};\n\nAnd by looking at .u.range.top, you know that the user meant to do\nsomething to the \"master\" branch.\n"},{"id":"173137","messageId":"alpine.DEB.1.00.1108081748060.7748@s15462909.onlinehome-server.info","threadId":"27908","inReplyTo":"7vy5zabbz7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-08-08T16:22:48Z","receivedAt":"2011-08-08T16:22:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 3 Aug 2011, Junio C Hamano wrote:\n\n> Sverre Rabbelier writes:\n> \n> > On Sun, Jul 24, 2011 at 21:23, Junio C Hamano wrote:\n> >> Sverre Rabbelier writes:\n> >>\n> >>> void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n> >>> {\n> >>> - add_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n> >>> + add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n> >>> }\n> >>\n> >> This seems utterly broken. For example, fmt-merge-msg.c adds \"^HEAD\" \n> >> and of course the flags on the object is UNINTERESTING. Has all the \n> >> callers of add_pending_object() been verified? Why is it passing an \n> >> unconditional 0 and not !!(obj->flags & UNINTERESTING) or something?\n\nIf you do that, you're back to start. Since obj has not the faintest clue \nwhether the pending object was added from a negative or a positive ref.\n\nAnd yes, all the callers have been verified: I don't want them to change. \nI only need to catch the callers inside the revision machinery's argument \nparsing, since that is what I need anyway, to traverse the objects later \non. And rather than reimplementing the wheel^Wargument parsing, I'd like \nto reuse what is already there and just forgets the single bit of \ninformation I need.\n\nThe other users of the argument parsing can change their callers if \nnecessary, when they also need the information about whether a ref was \npositive/negative.\n\n> > If I understand correctly (and it's not unlikely that I don't), the \n> > 'flags' field is used to store the actual flags (not just a boolean). \n> > Would the following be appropriate?\n> >\n> > + add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);\n> \n> I would think that the information you are trying to convey is more in\n> line with the spirit of \"name\" field, not \"mode\".\n\nNo, it is a flag. The parsed argument is a negative ref. It has a name \n(not including the \"^\") and it has a flag (\"negative\").\n\nThe mode is indeed only relevant for non-commitishs: If you look where a \npending object is added _with_ a mode other than S_IFINVALID, it is only \nwhen calling get_sha1_with_mode(). That function indirectly calls \nget_sha1_with_context_1() which sets the mode to ce_mode. In other words, \nthe mode is the file mode when specifying a blob/tree as :. All \ncommits (and tags) have a mode set to S_IFINVALID.\n\n> [...]\n>\n> Wouldn't it be wonderful if the revision machinery left richer clue in\n> each element of the pending object array while parsing, so that the caller\n> does not have to guess?\n\nThat is what we are trying to solve, and rather than to reuse the \"mode\" I \nthought that it'd be wiser to add new \"flags\".\n\nMany of the richer clues you refer to could be expressed as such flags, \nincluding the problem I need to address.\n\nFor the only itch I have is to get remote-hg working nicely for me, and \nSverre said I can only have that if fast-export is running correctly, i.e. \nupdating all the refs, even if some other ref already points at the same \ncommit.\n\n> In addition to a single \"mode\" integer, which says if it is supposed to \n> be a tree or a blob, we could allocate a single structure that records \n> something like this:\n> \n> \tstruct parsed_rev {\n> enum {\n> \t\t\tSHA1, REF, RANGE, SYMMETRIC_RANGE, REFLOG_ENT, ...\n> // there are others like OBJ^!, OBJ@!, ...\n> } kind;\n> const char *string;\n> union {\n> \t\t\tstruct {\n> \t\t\t\tconst char *real_ref;\n> \t\t\t} ref;\n> \t\t\tstruct {\n> \t\t\t\tstruct parsed_rev *bottom;\n> \t\t\t\tstruct parsed_rev *top;\n> } range;\n> \t\t\t...\n> } u;\n> \t};\n\nIs this not a little bit of a big, huge, tremendous overkill? All I need \nright now is a simple flag. Let's not cross bridges at which we haven't \narrived for another 50 miles yet.\n\nOr in other words: I'd rather stay with a simple, elegant, minimal patch \nthat solves the problem at hand while not preventing future enhancements.\n\nAlso remember that the pending objects array is used for the complete \nobject traversal. If you make that bigger you might end up with a \nsubstantial increase in memory consumption (which made me think of \nreusing the \"mode\", but then I thought about the possible side effects in \nthe future and rejected that idea).\n\nThe only way to convince me to do that complicated stuff is by \nblackmailing me, stating that I cannot have a working remote-hg without \ndoing all this nested/union/self-referential struct work I don't need.\n\nCiao,\nJohannes\n"},{"id":"173141","messageId":"7vfwlbztfg.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"alpine.DEB.1.00.1108081748060.7748@s15462909.onlinehome-server.info","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T17:47:47Z","receivedAt":"2011-08-08T17:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Wed, 3 Aug 2011, Junio C Hamano wrote:\n>\n>> Sverre Rabbelier writes:\n>> \n>> > On Sun, Jul 24, 2011 at 21:23, Junio C Hamano wrote:\n>> >> Sverre Rabbelier writes:\n>> >>\n>> >>> void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)\n>> >>> {\n>> >>> - add_pending_object_with_mode(revs, obj, name, S_IFINVALID);\n>> >>> + add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);\n>> >>> }\n> ...\n> If you do that, you're back to start. Since obj has not the faintest clue \n> whether the pending object was added from a negative or a positive ref.\n\nBut the point is that this codepath does not have a faintest clue whether\nthe \"obj\" parameter is something the end user actively asked for (which\nmight have been marked as uninteresting for other reasons, namely, because\nit is reachable from other negative refs). So passing unconditional 0 is\njust as bad.\n\n>> Wouldn't it be wonderful if the revision machinery left richer clue in\n>> each element of the pending object array while parsing, so that the caller\n>> does not have to guess?\n>\n> That is what we are trying to solve, and rather than to reuse the \"mode\" I \n> thought that it'd be wiser to add new \"flags\".\n\nI never thought about nor suggested touching \"mode\" ;-) It does not have\nenough width for the necessary information.\n\n> Many of the richer clues you refer to could be expressed as such flags, \n> including the problem I need to address.\n>\n>> In addition to a single \"mode\" integer, which says if it is supposed to \n>> be a tree or a blob, we could allocate a single structure that records... \n>\n> Is this not a little bit of a big, huge, tremendous overkill?\n\nAs long as you can show your \"flags\" can (be extended to) express the same\nrichness to solve sample problems I mentioned in my response, as well as\nyour immediate issue, I wouldn't insist implementing a parsed struct/union\nthat may be a more (and unnecessarily) verbose way to say the same thing.\n\n> Or in other words: I'd rather stay with a simple, elegant, minimal patch \n> that solves the problem at hand while not preventing future enhancements.\n\nWe are on the same page, but what I read from the patch didn't show a\nclear way forward to extend the \"flags\" to allow the stuff I mentioned\n(and the stuff I didn't but obviously fall into the same category of \"we\nwish revision parsing machinery left us richer information\").\n\n> Also remember that the pending objects array is used for the complete \n> object traversal.\n\nMy reading and rememberance of the code around add_parents_to_list() must\nbe quite different from yours. It is not 2006 anymore.\n\nWe didn't even have a separate pending object list type until 1f1e895 (Add\n\"named object array\" concept, 2006-06-19) and used the same object_list,\nwhose element size _did_ matter as you mention. But that commit allowed us\nto give elements on the pending list that came directly from the end user\nricher semantics than those of the object_list that we discover during the\ntraversal, and that is why back at e5709a4 (add add_object_array_with_mode,\n2007-04-22) we gave more information to it without having to fatten\nobject_list elements.\n\n> The only way to convince me to do that complicated stuff is by \n> blackmailing me, stating that I cannot have a working remote-hg without \n> doing all this nested/union/self-referential struct work I don't need.\n\nI would be reluctant to accept a myopic hack that is only good for one\ncaller and that needs to be ripped out and re-done, especially when we\nalready know other issues that can be solved cleanly if you go a little\nfurther in the initial round.\n\nAs I said, I am not married to the verbose struct/union representation\n(the only reason I showed that verbosity was because it allowed me to do\naway without having to enumerate all the syntax sugars we already\nsupport); if your \"flags\" can express the same thing (it may needs to\nbecome a bitfield with enough width, but I highly suspect that you would\nalso need at least a component that says \"this is the string the user gave\nus --- the user said 'master', not 'master^0', for example) and is a lot\nmore compact, that is definitely we want to go with.\n\nThanks.\n"},{"id":"173165","messageId":"CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com","threadId":"27908","inReplyTo":"7vfwlbztfg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T21:27:34Z","receivedAt":"2011-08-08T21:27:34Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Aug 8, 2011 at 19:47, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>> If you do that, you're back to start. Since obj has not the faintest clue\n>> whether the pending object was added from a negative or a positive ref.\n>\n> But the point is that this codepath does not have a faintest clue whether\n> the \"obj\" parameter is something the end user actively asked for (which\n> might have been marked as uninteresting for other reasons, namely, because\n> it is reachable from other negative refs). So passing unconditional 0 is\n> just as bad.\n\nDoesn't passing 0 indicate that we _did not_ receive any explicit user\ninput on this ref, which is exactly what we want to record? If the\nuser passed us any explicit input on the ref, we record it at the\nother call-sites, here, the user told us nothing, so we record exactly\nthat, nothing.\n\n>> Is this not a little bit of a big, huge, tremendous overkill?\n>\n> As long as you can show your \"flags\" can (be extended to) express the same\n> richness to solve sample problems I mentioned in my response, as well as\n> your immediate issue, I wouldn't insist implementing a parsed struct/union\n> that may be a more (and unnecessarily) verbose way to say the same thing.\n\nI cannot recall you ever asking somebody to implement some feature\n_nobody needs right now_ while trying to fix a _bug_, why now? I do\nnot know this code well enough to implement it, and Dscho doesn't have\nthe time to do it.\n\n>> Or in other words: I'd rather stay with a simple, elegant, minimal patch\n>> that solves the problem at hand while not preventing future enhancements.\n>\n> We are on the same page, but what I read from the patch didn't show a\n> clear way forward to extend the \"flags\" to allow the stuff I mentioned\n\nNobody needs the stuff you mentioned right now. We do need this to fix\nthis bug. If someone else does want it at a later date, replacing it\nwill be exactly as much work with or without this patch.\n\n> I would be reluctant to accept a myopic hack that is only good for one\n> caller and that needs to be ripped out and re-done, especially when we\n> already know other issues that can be solved cleanly if you go a little\n> further in the initial round.\n\nWhile I understand this reluctance, remember that this \"one caller\" is\nrequired to fix a bug in the current code. If you had a similar\ncomplaint about the remote-hg.py patches that I haven't sent yet, I\nwould be more than willing to invest the extra time in addressing\nthose concerns, since I'm adding new functionality anyway, this is\ndifferent.\n\n> As I said, I am not married to the verbose struct/union representation\n> (the only reason I showed that verbosity was because it allowed me to do\n> away without having to enumerate all the syntax sugars we already\n> support); if your \"flags\" can express the same thing (it may needs to\n> become a bitfield with enough width, but I highly suspect that you would\n> also need at least a component that says \"this is the string the user gave\n> us --- the user said 'master', not 'master^0', for example) and is a lot\n> more compact, that is definitely we want to go with.\n\nDon't we already store that in the name field?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173168","messageId":"7vty9rv9p2.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T22:07:37Z","receivedAt":"2011-08-08T22:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Nobody needs the stuff you mentioned right now.\n\nWrong.\n\n\"diff ^C A B\" is a bug, isn't it? And that is a bug in the current code.\n\n>> I would be reluctant to accept a myopic hack that is only good for one\n>> caller and that needs to be ripped out and re-done, especially when we\n>> already know other issues that can be solved cleanly if you go a little\n>> further in the initial round.\n>\n> While I understand this reluctance, remember that this \"one caller\" is\n> required to fix a bug in the current code.\n"},{"id":"173170","messageId":"7vpqkfv8x1.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T22:24:26Z","receivedAt":"2011-08-08T22:24:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n>> But the point is that this codepath does not have a faintest clue whether\n>> the \"obj\" parameter is something the end user actively asked for (which\n>> might have been marked as uninteresting for other reasons, namely, because\n>> it is reachable from other negative refs). So passing unconditional 0 is\n>> just as bad.\n>\n> Doesn't passing 0 indicate that we _did not_ receive any explicit user\n> input on this ref, which is exactly what we want to record? If the\n> user passed us any explicit input on the ref, we record it at the\n> other call-sites, here, the user told us nothing, so we record exactly\n> that, nothing.\n\nHmm, which means you have a way to say \"explicitly affirmative\" vs \"no\ninformation\", but no way to say \"explicitly negative\", for example, and\nthe worse part is that it is unclear if the approach the patch takes is\nextensible enough to allow that in the future. That is the kind of \"myopic\nhack\" attitude I did not particularly like in this patch.\n\n\"The next person who needs more generic framework can rip out what this\npatch does and the work required is the same amount\" is not a convincing\nargument---it would mean you are burdening that other person with an extra\nwork to _redo_ what this series does properly, and it is not likely to be\nof help for that person after your interest in this codepath has long\nwaned.\n\n>> As I said, I am not married to the verbose struct/union representation\n>> (the only reason I showed that verbosity was because it allowed me to do\n>> away without having to enumerate all the syntax sugars we already\n>> support); if your \"flags\" can express the same thing (it may needs to\n>> become a bitfield with enough width, but I highly suspect that you would\n>> also need at least a component that says \"this is the string the user gave\n>> us --- the user said 'master', not 'master^0', for example) and is a lot\n>> more compact, that is definitely we want to go with.\n>\n> Don't we already store that in the name field?\n\nPlease remind yourself why then it is not sufficient for your patch to\nread from the name field please?\n\nAfter all, wasn't the issue that \"master^0..master\" yields an empty set\nbut you somehow wanted to know that the RHS of that dotdot was given as a\npositive ref?\n"},{"id":"173171","messageId":"CAGdFq_jzraS-_afjs+MmRS4ML805h73hXAjrRkrQ=tGUdSQqkQ@mail.gmail.com","threadId":"27908","inReplyTo":"7vpqkfv8x1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T22:28:56Z","receivedAt":"2011-08-08T22:28:56Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 00:24, Junio C Hamano wrote:\n> Hmm, which means you have a way to say \"explicitly affirmative\" vs \"no\n> information\", but no way to say \"explicitly negative\", for example, and\n> the worse part is that it is unclear if the approach the patch takes is\n> extensible enough to allow that in the future. That is the kind of \"myopic\n> hack\" attitude I did not particularly like in this patch.\n\nDo we have a way to say explicitly negative on the commandline? Is\nthere a way to say \"I don't want you to decorate this commit as\nanything at all\"?\n\n> \"The next person who needs more generic framework can rip out what this\n> patch does and the work required is the same amount\" is not a convincing\n> argument---it would mean you are burdening that other person with an extra\n> work to _redo_ what this series does properly, and it is not likely to be\n> of help for that person after your interest in this codepath has long\n> waned.\n\nBe fair. How is \"2 files changed, 8 insertions(+), 6 deletions(-)\"\ngoing to make it any harder at all for someone who is going to be\ndoing that huge patch series you described earlier?\n\n>> Don't we already store that in the name field?\n>\n> Please remind yourself why then it is not sufficient for your patch to\n> read from the name field please?\n\nSure, we could do it. But it would be duplicating all the effort\nalready being done in rev-parse!\n\n> After all, wasn't the issue that \"master^0..master\" yields an empty set\n> but you somehow wanted to know that the RHS of that dotdot was given as a\n> positive ref?\n\nThe issue was that if I push \"master\" to origin but I already pushed a\n\"next\" which points at the same commit nothing happens because\ngit-fast-export doesn't know to emit the \"reset master :13878\" line.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173172","messageId":"CAGdFq_joHskwhp=934OjirmXiRMR3NbGd4s-hSjJc-gCFT_Jew@mail.gmail.com","threadId":"27908","inReplyTo":"7vty9rv9p2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T22:30:04Z","receivedAt":"2011-08-08T22:30:04Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 00:07, Junio C Hamano wrote:\n> Sverre Rabbelier writes:\n>> Nobody needs the stuff you mentioned right now.\n>\n> Wrong.\n>\n> \"diff ^C A B\" is a bug, isn't it? And that is a bug in the current code.\n\nI think I missed something, can you point me to the relevant thread\nabout this bug in diff? Maybe I can convince the people involved in\nfixing that to help out.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173173","messageId":"7vliv3v8cx.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_joHskwhp=934OjirmXiRMR3NbGd4s-hSjJc-gCFT_Jew@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T22:36:30Z","receivedAt":"2011-08-08T22:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Tue, Aug 9, 2011 at 00:07, Junio C Hamano wrote:\n>> Sverre Rabbelier writes:\n>>> Nobody needs the stuff you mentioned right now.\n>>\n>> Wrong.\n>>\n>> \"diff ^C A B\" is a bug, isn't it? And that is a bug in the current code.\n>\n> I think I missed something, can you point me to the relevant thread\n> about this bug in diff? Maybe I can convince the people involved in\n> fixing that to help out.\n\nIt is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>\n"},{"id":"173174","messageId":"CAGdFq_iHBE7eESpsX_doyfJu6EAkPOJpBgqkw1psMhqATf2oCw@mail.gmail.com","threadId":"27908","inReplyTo":"7vliv3v8cx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T22:38:24Z","receivedAt":"2011-08-08T22:38:24Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 00:36, Junio C Hamano wrote:\n> It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>\n\nThen I don't understand why you're mentioning it in reference to a\nproblem someone is actually having?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173175","messageId":"7vhb5rv7x0.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_iHBE7eESpsX_doyfJu6EAkPOJpBgqkw1psMhqATf2oCw@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T22:46:03Z","receivedAt":"2011-08-08T22:46:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Tue, Aug 9, 2011 at 00:36, Junio C Hamano wrote:\n>> It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>\n>\n> Then I don't understand why you're mentioning it in reference to a\n> problem someone is actually having?\n\nWhat?\n\nWhy isn't it a real problem when I have it in the currently released code,\nand why is it a real problem if you have it with your wip series that uses\nthe current code? Both are real problems that somebody is actually\nhaving.\n"},{"id":"173176","messageId":"CAGdFq_ioxeZUCnn-fFKuiT-6eVzVoph8eQE2j0hEbAxtDZzGnQ@mail.gmail.com","threadId":"27908","inReplyTo":"7vhb5rv7x0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T22:48:58Z","receivedAt":"2011-08-08T22:48:58Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 00:46, Junio C Hamano wrote:\n> Why isn't it a real problem when I have it in the currently released code,\n> and why is it a real problem if you have it with your wip series that uses\n> the current code?  Both are real problems that somebody is actually\n> having.\n\nBecause no actual users are actually having this problem (e.g., at\ntimes typing \"git diff ^C A B\" and are seriously troubled by it), or\nat least haven't spoken up? Or do you meant that you are that user?\nI'm confused.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173177","messageId":"7vd3gfv7gd.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_jzraS-_afjs+MmRS4ML805h73hXAjrRkrQ=tGUdSQqkQ@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T22:56:02Z","receivedAt":"2011-08-08T22:56:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n>> Please remind yourself why then it is not sufficient for your patch to\n>> read from the name field please?\n>\n> Sure, we could do it. But it would be duplicating all the effort\n> already being done in rev-parse!\n>\n>> After all, wasn't the issue that \"master^0..master\" yields an empty set\n>> but you somehow wanted to know that the RHS of that dotdot was given as a\n>> positive ref?\n>\n> The issue was that if I push \"master\" to origin but I already pushed a\n> \"next\" which points at the same commit nothing happens...\n\nWould we have the same issue with \"git bundle\", by the way, caused by the\nsame setup_revisions() limitation? Over there we would also need to\ndecide the set of refs that the user wanted to record their values while\ncomputing the set of commits we would need to transfer. \n\nIn this message I am not interested in talking about what implementation\nto use to fix it, but just wondering the scope of the same issue and\ntrying to figure out the (positive) impact of _a_ fix.\n"},{"id":"173180","messageId":"CAGdFq_gY3-1fv8TiHj=8ppS=AE4B-rwwUL-iKHcjvVtqQa8UAA@mail.gmail.com","threadId":"27908","inReplyTo":"7vd3gfv7gd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T23:04:19Z","receivedAt":"2011-08-08T23:04:19Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 00:56, Junio C Hamano wrote:\n> Would we have the same issue with \"git bundle\", by the way, caused by the\n> same setup_revisions() limitation?  Over there we would also need to\n> decide the set of refs that the user wanted to record their values while\n> computing the set of commits we would need to transfer.\n\nI think so.\n\n$ git init test && cd test\nInitialized empty Git repository in /home/sverre/code/test/test/.git/\n$ echo content >> file\n$ git add file && git commit -m \"first\"\n[master (root-commit) c7f5a27] first\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 file\n$ (git init ../another && cd ../another && git pull ../test)\nInitialized empty Git repository in /home/sverre/code/another/.git/\nremote: Counting objects: 3, done.\nremote: Total 3 (delta 0), reused 0 (delta 0)\nUnpacking objects: 100% (3/3), done.\nFrom ../test\n * branch HEAD -> FETCH_HEAD\nsverre@laptop-sverre:~/code/test\n$ git branch second\n$ git bundle create bundle second\nCounting objects: 3, done.\nWriting objects: 100% (3/3), 215 bytes, done.\nTotal 3 (delta 0), reused 0 (delta 0)\nsverre@laptop-sverre:~/code/test\n$ mv bundle ../another/\n$ (cd ../another/ && git bundle unbundle bundle)\nc7f5a273b11a438bf78aecff0dfcbb8b16344555 refs/heads/second\n$ (cd ../another/ && git for-each-ref)\nc7f5a273b11a438bf78aecff0dfcbb8b16344555 commit\trefs/heads/master\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173182","messageId":"7vy5z3trwu.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_ioxeZUCnn-fFKuiT-6eVzVoph8eQE2j0hEbAxtDZzGnQ@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T23:17:05Z","receivedAt":"2011-08-08T23:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Because no actual users are actually having this problem (e.g., at\n> times typing \"git diff ^C A B\" and are seriously troubled by it), or\n> at least haven't spoken up? Or do you meant that you are that user?\n\nYes, I am that user who have been disturbed by the command line revision\nparser limitation for a loooooooong time and have been wanting to see it\nproperly fixed for quite a while (by myself or by somebody else).\n\nOtherwise why would I have bothered to spend 93 lines to describe it and\nits one possible solution that I _think_ is general enough to cover both\nof the real issue identified in this thread?\n\nI may probably be being slightly unfair, for two reasons. The primary\nreason is because the affected codepath is so central to the UI that\nfixing this properly is more important than various random tweaks we make\nin other areas (which makes me particularly picky with respect to ensuring\nthat the future expansion would be easy). Seeing a single-bit \"hack\" not\nfrom a complete newbie but from two known-to-be-competent long timers of\nGit is another.\n\nThere are numerous hacks that try to work around the same revision parser\nlimitation (lossage of information), and I have been hoping that a series\nthat touch the parser to leave more information during parsing to be\ncleanly enhancible to get rid of them (e.g. \"git checkout $commit\" vs \"git\ncheckout $branch\" looks at the command line itself, but it should be able\nto call into the revision parser and inspect what the parser gives it\nback).\n"},{"id":"173184","messageId":"CAGdFq_hO-MYC_kXZZhoqXhTRvVhCDfTT4EHPoYDyHjGtRiSB9g@mail.gmail.com","threadId":"27908","inReplyTo":"7vy5z3trwu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-08T23:25:24Z","receivedAt":"2011-08-08T23:25:24Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Aug 9, 2011 at 01:17, Junio C Hamano wrote:\n> Yes, I am that user who have been disturbed by the command line revision\n> parser limitation for a loooooooong time and have been wanting to see it\n> properly fixed for quite a while (by myself or by somebody else).\n\nAaaah, well that does explain a thing or two :).\n\n> Seeing a single-bit \"hack\" not\n> from a complete newbie but from two known-to-be-competent long timers of\n> Git is another.\n\nPerhaps known-to-be-competent, but time-constrained nonetheless.\nSometimes one is forced to chose between fixing something you care\nabout and doing something in a non-hacky way... There's only so much\ntime in a day to work on side-projects such as this after all :).\n\n> There are numerous hacks that try to work around the same revision parser\n> limitation (lossage of information), and I have been hoping that a series\n> that touch the parser to leave more information during parsing to be\n> cleanly enhancible to get rid of them (e.g. \"git checkout $commit\" vs \"git\n> checkout $branch\" looks at the command line itself, but it should be able\n> to call into the revision parser and inspect what the parser gives it\n> back).\n\nOk, knowing that, I am somewhat more inclined to have a look at this.\nI'm still not sure that I know how to implement the design you\ndescribed though. I mean, I understand the general concept, but I have\nno idea in what places it would need to hook in to make it work (ditto\non how to fix the diff bug).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"173187","messageId":"7vipq7tqux.fsf@alter.siamese.dyndns.org","threadId":"27908","inReplyTo":"CAGdFq_hO-MYC_kXZZhoqXhTRvVhCDfTT4EHPoYDyHjGtRiSB9g@mail.gmail.com","subject":"Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-08T23:39:50Z","receivedAt":"2011-08-08T23:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Ok, knowing that, I am somewhat more inclined to have a look at this.\n> I'm still not sure that I know how to implement the design you\n> described though. I mean, I understand the general concept, but I have\n> no idea in what places it would need to hook in to make it work (ditto\n> on how to fix the diff bug).\n\nIt is entirely possible, as Dscho hinted in his response, that your \"flag\"\nword may be sufficient to encode \"where did this object (whether it is\nmarked as uninteresting or interesting) come from? what did the user want\nto do with it?\" information for all practical purposes, even though I\ndoubt one bit would be sufficient. So how about ignoring \"diff A...B\" vs\n\"diff ^C A B\" in the meantime and see if your change to revision.c is\napplicable to the same issue in \"git bundle\" as a starter?\n\nI would imagine that \"diff A...B\" vs \"diff ^C A B\" case would be solved by\nthe former (^C is the common ancestor between A and B) add a flag\nCOMMON_ANCESTOR to pending object array entry that holds ^C,\nSYMMETRIC_LEFT to the one with A and SYMMETRIC_RIGHT to the one with B,\nwhile the latter would stuff one SINGLE_NEGATIVE (^C), and two\nSINGLE_POSITIVE (A and B), and the caller would tell between the two, or\nsomething like that. And \"bundle master^0..master\" (and similarly your\nexport stream) would note the RHS of the dot operator with SYMMETRIC_RIGHT\nso that \"master\", even though the commit object gets marked as\nuninteresting at the end, can be recovered to be of interest to the end\nuser.\n\nI am not suggesting the exact flag word names above, but just trying to\nenumerate how many kinds would be needed). Your single-bit in the patch\nwould be SYMMETRIC_RIGHT in the above ugly notation, but you can call it\nREF_INTERESTING or whatever.\n"}]}