{"thread":{"id":"28857","subject":"[PATCH 0/3] fast-export fixes","startedAt":"2011-11-05T23:23:24Z","lastAt":"2019-01-29T19:42:42Z","messageCount":42,"participants":["Sverre Rabbelier","Jonathan Nieder","Junio C Hamano","Thomas Rast","Felipe Contreras","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"178925","messageId":"1320535407-4933-1-git-send-email-srabbelier@gmail.com","threadId":"28857","inReplyTo":null,"subject":"[PATCH 0/3] fast-export fixes","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-05T23:23:24Z","receivedAt":"2011-11-05T23:23:24Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Dscho and I worked on this patch series during a mini-hackathon in\nlate July, but Junio held the series back since he saw a more elegant\nway to tackle the problem that would pave the way to solve a problem\nhe was having. Since then I've had very little time to work on git,\nso I was very glad to have the chance to work on this during another\nmini-hackathon in Amsterdam today.\n\nI've used the new rev_info mechanism Junio introduced, and while I\ncan't say I completely understand how the tag_of_filtered_mode bit\nworks, I'm happy to say that all the tests pass now :).\n\nJohannes Schindelin (2):\n fast-export: do not refer to non-existing marks\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 | 48 ++++++++++++++++++++++++++++++++++++++++++------\n t/t9350-fast-export.sh | 11 +++++++++++\n 2 files changed, 53 insertions(+), 6 deletions(-)\n\n-- \n1.7.8.rc0.36.g67522.dirty\n"},{"id":"178927","messageId":"1320535407-4933-2-git-send-email-srabbelier@gmail.com","threadId":"28857","inReplyTo":"1320535407-4933-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-05T23:23:25Z","receivedAt":"2011-11-05T23:23:25Z","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 based remote helpers, as they use marks\nfiles to store which commits have already been seen. The call graph\nis something as follows:\n\n$ # push master to remote repo\n$ git fast-export --{im,ex}port-marks=marksfile master\n$ # make a commit on master and push it to remote\n$ git fast-export --{im,ex}port-marks=marksfile master\n$ # run `git branch foo` and push it to remote\n$ git fast-export --{im,ex}port-marks=marksfile foo\n\nWhen fast-export imports the marksfile and sees that all commits in\nfoo are marked as UNINTERESTING (they have already been exported\nwhile pushing master), it exits without doing anything. However,\nwhat we want is for it to reset 'foo' to the already-exported commit.\n\nEither way demonstrates the problem, and since this is the most\nsuccint way to demonstrate the problem it is implemented by passing\nmaster..master on the commandline.\n\nSigned-off-by: Sverre Rabbelier \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 950d0ff..74914dc 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -440,4 +440,15 @@ test_expect_success 'fast-export quotes pathnames' '\n \t)\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.8.rc0.36.g67522.dirty\n"},{"id":"178926","messageId":"1320535407-4933-3-git-send-email-srabbelier@gmail.com","threadId":"28857","inReplyTo":"1320535407-4933-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 2/3] fast-export: do not refer to non-existing marks","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-05T23:23:26Z","receivedAt":"2011-11-05T23:23:26Z","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\nExtract a handle_reset function that deals with this, which can then\nbe re-used in a later commit.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \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 9836e6b..c4c4391 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -529,9 +529,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@@ -543,9 +554,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.8.rc0.36.g67522.dirty\n"},{"id":"178928","messageId":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","threadId":"28857","inReplyTo":"1320535407-4933-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-05T23:23:27Z","receivedAt":"2011-11-05T23:23:27Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"When a revision is specified on the commandline we explicitly output\na 'reset' command for it if it was not handled already. This allows\nfor example the remote-helper protocol to use fast-export to create\nbranches that point to a commit that has already been exported.\n\nInitial-patch-by: Johannes Schindelin \nSigned-off-by: Sverre Rabbelier \n---\n\n Most of the hard work for this patch was done by Dscho. The rest of\n it was basically me applying the technique used by jch in c3502fa\n (25-08-2011 do not include sibling history in --ancestry-path).\n\n The if statement dealing with tag_of_filtered_mode is not as\n elegant as either me or Dscho would have liked, but we couldn't\n find a better way to determine if a ref is a tag at this point\n in the code.\n\n Additionally, the elem->whence != REV_CMD_RIGHT case should really\n check if REV_CMD_RIGHT_REF, but as this is not provided by the\n ref_info structure this is left as is. A result of this is that\n incorrect input will result in incorrect output, rather than an\n error message. That is: `git fast-export a..` will\n incorrectly generate a `reset ` statement in the fast-export\n stream.\n\n The dwim_ref bit is a double work (it has already been done by the\n caller of this function), but I decided it would be more work to\n pass this information along than to recompute it for the few\n commandline refs that were relevant.\n\n builtin/fast-export.c | 31 +++++++++++++++++++++++++++++--\n t/t9350-fast-export.sh | 2 +-\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex c4c4391..bcfec38 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -18,6 +18,8 @@\n #include \"parse-options.h\"\n #include \"quote.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@@ -541,10 +543,34 @@ 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 rev_info *revs, 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 = 0; i < revs->cmdline.nr; i++) {\n+\t\tstruct rev_cmdline_entry *elem = &revs->cmdline.rev[i];\n+\n+\t\tif (elem->flags & UNINTERESTING)\n+\t\t\tcontinue;\n+\n+\t\tif (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n+\t\t\tcontinue;\n+\n+\t\tchar *full_name;\n+\t\tdwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);\n+\n+\t\tif (!prefixcmp(full_name, \"refs/tags/\") &&\n+\t\t\t(tag_of_filtered_mode != REWRITE ||\n+\t\t\t!get_object_mark(elem->item)))\n+\t\t\tcontinue;\n+\n+\t\tif (!(elem->flags & REF_HANDLED)) {\n+\t\t\thandle_reset(full_name, elem->item);\n+\t\t\telem->flags |= REF_HANDLED;\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@@ -698,11 +724,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(&revs, &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 74914dc..ea7dc21 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -446,7 +446,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.8.rc0.36.g67522.dirty\n"},{"id":"178949","messageId":"20111106043157.GM27272@elie.hsd1.il.comcast.net","threadId":"28857","inReplyTo":"1320535407-4933-2-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T04:31:57Z","receivedAt":"2011-11-06T04:31:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> This happens only when the corresponding commits are not exported in\n> the current fast-export run. This can happen either when the relevant\n> commit is already marked, or when the commit is explicitly marked\n> as UNINTERESTING with a negative ref by another argument.\n\nThe above \"This\" has no antecedent. I guess you mean that\nfast-export writes no output when passed a range of the form A..A.\n\n> This breaks fast-export based remote helpers,\n\nMakes sense.\n\n> as they use marks\n> files to store which commits have already been seen. The call graph\n> is something as follows:\n>\n> $ # push master to remote repo\n> $ git fast-export --{im,ex}port-marks=marksfile master\n> $ # make a commit on master and push it to remote\n> $ git fast-export --{im,ex}port-marks=marksfile master\n> $ # run `git branch foo` and push it to remote\n> $ git fast-export --{im,ex}port-marks=marksfile foo\n>\n> When fast-export imports the marksfile and sees that all commits in\n> foo are marked as UNINTERESTING\n\nHmm, I didn't know about this behavior. Would it be possible to add\na test for it, too?\n\n> t/t9350-fast-export.sh | 11 +++++++++++\n> 1 files changed, 11 insertions(+), 0 deletions(-)\n\nWith or without the change suggested above, this new test seems to me\nlike a good thing, even though in the longer term it might be nicer to\nteach fast-export to understand a syntax like\n\n\tgit fast-import ^master master:master\n\nPut another way, the possibility of something nicer later shouldn't\nstop us from adding an incremental refinement that improves things\ntoday.\n\nThanks for working on this,\nJonathan\n"},{"id":"178950","messageId":"20111106044514.GN27272@elie.hsd1.il.comcast.net","threadId":"28857","inReplyTo":"1320535407-4933-3-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 2/3] fast-export: do not refer to non-existing marks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T04:45:14Z","receivedAt":"2011-11-06T04:45:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSverre Rabbelier wrote:\n\n> When calling `git fast-export a..a b` when a and b refer to the same\n> commit, nothing would be exported, and an incorrect reset line would\n> be printed for b ('from :0').\n\nHm, seems problematic indeed.\n\n> Extract a handle_reset function that deals with this, which can then\n> be re-used in a later commit.\n\nSo, does this patch drop the confusing behavior and add one that is\nmore intuitive for remote helpers? It's not clear from this\ndescription what sort of deal the patch makes and whether it is a good\nor bad one.\n\n[...]\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -529,9 +529,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\nNit: the other handle_* functions are about acting on objects\nencountered during revision traversal from the object store. In other\nwords, the things being handled are the git objects.\n\nBy contrast, this function is about writing a \"reset\" command to the\nfast-import stream. I'd be tempted to call it reset_ref() or\nsomething like that.\n\n> +{\n> +\tint mark = get_object_mark(object);\n> +\n> -\tcommit = (struct commit *)object;\n> -\tprintf(\"reset %s\\nfrom :%d\\n\\n\", name,\n> -\t get_object_mark(&commit->object));\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\nAh --- the functional change is to use a sha1 when there is no mark\ncorresponding to the object.\n\nWhy is this codepath being run at all when b is excluded by the\nrevision range (a..a a = ^a a a)? Is this the same bug tested\nfor in patch 1/3 or something separate?\n"},{"id":"178951","messageId":"20111106050126.GO27272@elie.hsd1.il.comcast.net","threadId":"28857","inReplyTo":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T05:01:26Z","receivedAt":"2011-11-06T05:01:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> When a revision is specified on the commandline we explicitly output\n> a 'reset' command for it if it was not handled already. This allows\n> for example the remote-helper protocol to use fast-export to create\n> branches that point to a commit that has already been exported.\n>\n> Initial-patch-by: Johannes Schindelin \n> Signed-off-by: Sverre Rabbelier \n\nThanks. I'd suggest squashing in the test from patch 1/3 for easy\nreference (since each patch makes the other easier to understand).\n\n> ---\n> The if statement dealing with tag_of_filtered_mode is not as\n> elegant as either me or Dscho would have liked, but we couldn't\n> find a better way to determine if a ref is a tag at this point\n> in the code.\n>\n> Additionally, the elem->whence != REV_CMD_RIGHT case should really\n> check if REV_CMD_RIGHT_REF, but as this is not provided by the\n> ref_info structure this is left as is. A result of this is that\n> incorrect input will result in incorrect output, rather than an\n> error message. That is: `git fast-export a..` will\n> incorrectly generate a `reset ` statement in the fast-export\n> stream.\n>\n> The dwim_ref bit is a double work (it has already been done by the\n> caller of this function), but I decided it would be more work to\n> pass this information along than to recompute it for the few\n> commandline refs that were relevant.\n\nThese details seem like good details for the commit message, so the\nnext puzzled person looking at the code can see what behavior is\ndeliberate and what are the incidental side-effects.\n\nThe \"git fast-export a..$(git rev-parse HEAD^{commit})\" case sounds\nworth a test.\n\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -18,6 +18,8 @@\n> #include \"parse-options.h\"\n> #include \"quote.h\"\n> \n> +#define REF_HANDLED (ALL_REV_FLAGS + 1)\n\nCould TMP_MARK be used for this?\n\n[...]\n> @@ -541,10 +543,34 @@ 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 rev_info *revs, 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 = 0; i < revs->cmdline.nr; i++) {\n\nMight be clearer in a new function.\n\n> +\t\tstruct rev_cmdline_entry *elem = &revs->cmdline.rev[i];\n> +\n> +\t\tif (elem->flags & UNINTERESTING)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n> +\t\t\tcontinue;\n\nOh, neat.\n\n> +\n> +\t\tchar *full_name;\n\ndeclaration-after-statement\n\n> +\t\tdwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);\n> +\n> +\t\tif (!prefixcmp(full_name, \"refs/tags/\") &&\n\nWhat happens if dwim_ref fails, perhaps because a ref was deleted in\nthe meantime?\n\n> +\t\t\t(tag_of_filtered_mode != REWRITE ||\n> +\t\t\t!get_object_mark(elem->item)))\n> +\t\t\tcontinue;\n\nStyle nit: this would be easier to read if the \"if\" condition doesn't\nline up with the code below it:\n\n\t\tif (!prefixcmp(full_name, \"refs/tags/\")) {\n\t\t\tif (tag_of_filtered_mode != REWRITE ||\n\t\t\t !get_object_mark(elem->item))\n\t\t\t\tcontinue;\n\t\t}\n\nIf tag_of_filtered_mode == ABORT, we are going to die() soon, right?\nSo this seems to be about tag_of_filtered_mode == DROP --- makes\nsense.\n\nWhen does the !get_object_mark() case come up?\n\n> +\n> +\t\tif (!(elem->flags & REF_HANDLED)) {\n> +\t\t\thandle_reset(full_name, elem->item);\n> +\t\t\telem->flags |= REF_HANDLED;\n> +\t\t}\n\nJust curious: is the REF_HANDLED handling actually needed? What\nwould happen if fast-export included the redundant resets?\n\n> +\t}\n[...]\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -446,7 +446,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\nThanks for a pleasant read.\n"},{"id":"178977","messageId":"CAGdFq_hmF8xDA8PdDUPygSSAVsvrA=BRVKp+eCVRggHxLZzBsQ@mail.gmail.com","threadId":"28857","inReplyTo":"20111106043157.GM27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T19:38:08Z","receivedAt":"2011-11-06T19:38:08Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 05:31, Jonathan Nieder wrote:\n>> This happens only when the corresponding commits are not exported in\n>> the current fast-export run. This can happen either when the relevant\n>> commit is already marked, or when the commit is explicitly marked\n>> as UNINTERESTING with a negative ref by another argument.\n>\n> The above \"This\" has no antecedent.  I guess you mean that\n> fast-export writes no output when passed a range of the form A..A.\n\nWell, it's referring to the subject, how about:\n\n\"When a commit has not been exported in the current fast-export run\nits ref is not updated correctly. This can happen ...\".\n\n>> as they use marks\n>> files to store which commits have already been seen. The call graph\n>> is something as follows:\n>>\n>> $ # push master to remote repo\n>> $ git fast-export --{im,ex}port-marks=marksfile master\n>> $ # make a commit on master and push it to remote\n>> $ git fast-export --{im,ex}port-marks=marksfile master\n>> $ # run `git branch foo` and push it to remote\n>> $ git fast-export --{im,ex}port-marks=marksfile foo\n>>\n>> When fast-export imports the marksfile and sees that all commits in\n>> foo are marked as UNINTERESTING\n>\n> Hmm, I didn't know about this behavior.  Would it be possible to add\n> a test for it, too?\n\nWhat behavior are you referring to here? What kind of test would you want added?\n\n>>  t/t9350-fast-export.sh |   11 +++++++++++\n>>  1 files changed, 11 insertions(+), 0 deletions(-)\n>\n> With or without the change suggested above, this new test seems to me\n> like a good thing, even though in the longer term it might be nicer to\n> teach fast-export to understand a syntax like\n>\n>        git fast-import ^master master:master\n>\n> Put another way, the possibility of something nicer later shouldn't\n> stop us from adding an incremental refinement that improves things\n> today.\n\nYes, extending the capabilities of fast-export is needed if we want\nthe remote-helpers to be as powerful as native git.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"178978","messageId":"CAGdFq_i=f+ZD7pdN0D-hFBeq6TejXtt15Rb07UDViv1=nnXkmg@mail.gmail.com","threadId":"28857","inReplyTo":"20111106044514.GN27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 2/3] fast-export: do not refer to non-existing marks","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T19:40:51Z","receivedAt":"2011-11-06T19:40:51Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 05:45, Jonathan Nieder wrote:\n>> Extract a handle_reset function that deals with this, which can then\n>> be re-used in a later commit.\n>\n> So, does this patch drop the confusing behavior and add one that is\n> more intuitive for remote helpers?  It's not clear from this\n> description what sort of deal the patch makes and whether it is a good\n> or bad one.\n\nAh, yes. Perhaps something like:\n\n\"Extract a reset_ref function that deals with this situation by\nprinting the commit sha1 when no mark has been written yet.\"\n\n> Ah --- the functional change is to use a sha1 when there is no mark\n> corresponding to the object.\n>\n> Why is this codepath being run at all when b is excluded by the\n> revision range (a..a a = ^a a a)?  Is this the same bug tested\n> for in patch 1/3 or something separate?\n\nI must admit that I don't recall how exactly we stumbled on this case.\nIt might even make sense to instead die when we run into this corner\ncase, but I'm not convinced that there's no valid use case for this\n(which we would block by die-ing).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"178979","messageId":"CAGdFq_gkSxvw9Di_mUqS5N0bgCWh-dygMe_DWcR+ENAo=A-3=A@mail.gmail.com","threadId":"28857","inReplyTo":"20111106050126.GO27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T19:48:02Z","receivedAt":"2011-11-06T19:48:02Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 06:01, Jonathan Nieder wrote:\n> Thanks.  I'd suggest squashing in the test from patch 1/3 for easy\n> reference (since each patch makes the other easier to understand).\n\nYes, agreed. The initial series was 5 patches in total, but splitting\nit out for such a small series (and small patch at that) makes less\nsense.\n\n\n> These details seem like good details for the commit message, so the\n> next puzzled person looking at the code can see what behavior is\n> deliberate and what are the incidental side-effects.\n\nAll of it? I wasn't sure what part should go in the commit message.\n\n> The \"git fast-export a..$(git rev-parse HEAD^{commit})\" case sounds\n> worth a test.\n\nA test_must_fail?\n\n>> +#define REF_HANDLED (ALL_REV_FLAGS + 1)\n>\n> Could TMP_MARK be used for this?\n\nI don't know its usage, is it?\n\n> -static void handle_tags_and_duplicates(struct string_list *extra_refs)\n>> +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs)\n>>  {\n>>       int i;\n>>\n>> +     /* even if no commits were exported, we need to export the ref */\n>> +     for (i = 0; i < revs->cmdline.nr; i++) {\n>\n> Might be clearer in a new function.\n\nYes, probably. handle_cmdline_refs?\n\n>> +             struct rev_cmdline_entry *elem = &revs->cmdline.rev[i];\n>> +\n>> +             if (elem->flags & UNINTERESTING)\n>> +                     continue;\n>> +\n>> +             if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n>> +                     continue;\n>\n> Oh, neat.\n\nYes, I must admit that this bit was easier than I dreaded it would be\n(I must admit that's been a large reason that I haven't taken the time\nto work on this till now). With the fast-export and remote-helper\ntests to guide me, I was able to code-by-accident the right conditions\nhere :).\n\n>> +\n>> +             char *full_name;\n>\n> declaration-after-statement\n\nAh, yes.\n\n>> +             dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);\n>> +\n>> +             if (!prefixcmp(full_name, \"refs/tags/\") &&\n>\n> What happens if dwim_ref fails, perhaps because a ref was deleted in\n> the meantime?\n\nThat would be bad. I assumed that we have a lock on the refs, should I\nadd back the die check that's done by the other dwim_ref caller?\n\n>> +                     (tag_of_filtered_mode != REWRITE ||\n>> +                     !get_object_mark(elem->item)))\n>> +                     continue;\n>\n> Style nit: this would be easier to read if the \"if\" condition doesn't\n> line up with the code below it:\n>\n>                if (!prefixcmp(full_name, \"refs/tags/\")) {\n>                        if (tag_of_filtered_mode != REWRITE ||\n>                            !get_object_mark(elem->item))\n>                                continue;\n>                }\n\nYeah, that does look better :).\n\n> If tag_of_filtered_mode == ABORT, we are going to die() soon, right?\n\nI don't know to be honest, perhaps we would have already died by now?\nI don't know the details of how the tag_of_filtered_mode part is\nimplemented.\n\n> So this seems to be about tag_of_filtered_mode == DROP --- makes\n> sense.\n>\n> When does the !get_object_mark() case come up?\n\nEh, it has something to do with it being a replacement (rather than\nthe same), maybe? This is mostly just taken from Dscho's original\npatch.\n\n>> +             if (!(elem->flags & REF_HANDLED)) {\n>> +                     handle_reset(full_name, elem->item);\n>> +                     elem->flags |= REF_HANDLED;\n>> +             }\n>\n> Just curious: is the REF_HANDLED handling actually needed?  What\n> would happen if fast-export included the redundant resets?\n\nThat would just be sloppy :). I don't think anything particularly bad\nwould happen.\n\n> Thanks for a pleasant read.\n\nThanks for the review.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"179003","messageId":"7v1utk4gym.fsf@alter.siamese.dyndns.org","threadId":"28857","inReplyTo":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T05:52:33Z","receivedAt":"2011-11-07T05:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Additionally, the elem->whence != REV_CMD_RIGHT case should really\n> check if REV_CMD_RIGHT_REF, but as this is not provided by the\n> ref_info structure this is left as is.\n\nI am not sure what you mean by REV_CMD_RIGHT_REF here. Do you mean \"We are\nonly interested in the RHS endpoint of A...B syntax (i.e. B) but only when\nit is a refname and not an arbitrary SHA-1 expression (e.g. even though\nnext~4 in \"master...next~4\" is a RHS endpoint, it is not a ref, and we do\nnot want it)\"?\n\nI think the distinction you are trying to express (\"is it a ref and if so\nwhat exact refname resolve_ref() would produce, or is it just the name of\na random commit?\") is a very useful thing in general, but it is orthogonal\nto what existing REV_CMD_* are trying to express, which is \"where did they\ncome from\", that you can read from the name of the field \"whence\".\n\nPerhaps we would want to add a new field \"const char *ref\" to \"struct\nrev_cmdline_entry\" to record the additional information you want perhaps\nby storing the result of resolve_ref() if it is a ref and NULL otherwise.\nWould it be too much work to add it to perfect this series?\n\nBy the way, REV_CMD_REF is meant to mean \"the user did not explicitly name\nthis but it came as a result of iterating over refs/something/ namespace\",\nand does not mean \"this is a tip of some ref\" (they happen to be all refs,\nbut \"obtained by iteration, not by explicit naming\" is the more important\nreason for marking them as such). As they are numerous, if you are going\nto add that \"const char *ref\" field to rev_cmdline_entry, we may want to\neither leave it NULL for REV_CMD_REF entries (the name field already has\nthat information anyway), or have it point at its name field (we need to\naudit the codepath to free the name and ref fields if we go that route).\n"},{"id":"179004","messageId":"7vr51k32cj.fsf@alter.siamese.dyndns.org","threadId":"28857","inReplyTo":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T05:53:32Z","receivedAt":"2011-11-07T05:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> +static void handle_tags_and_duplicates(struct rev_info *revs, 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 = 0; i < revs->cmdline.nr; i++) {\n> +\t\tstruct rev_cmdline_entry *elem = &revs->cmdline.rev[i];\n> +\n> +\t\tif (elem->flags & UNINTERESTING)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n> +\t\t\tcontinue;\n> +\n> +\t\tchar *full_name;\n> +\t\tdwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);\n\nJust a nit I've already fixed locally (iow no need to resend only to fix\nthis) but this is decl-after-stmt.\n"},{"id":"179013","messageId":"20111107085846.GA30641@elie.hsd1.il.comcast.net","threadId":"28857","inReplyTo":"CAGdFq_gkSxvw9Di_mUqS5N0bgCWh-dygMe_DWcR+ENAo=A-3=A@mail.gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-07T08:58:47Z","receivedAt":"2011-11-07T08:58:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder wrote:\n\n>> These details seem like good details for the commit message, so the\n>> next puzzled person looking at the code can see what behavior is\n>> deliberate and what are the incidental side-effects.\n>\n> All of it? I wasn't sure what part should go in the commit message.\n\nYeah. My rule when in doubt has been to just include everything that\nwould remain meaningful over time that I could be putting in a cover\nletter for the patch. The hard part is to try to be concise in doing\nso.\n\n>> The \"git fast-export a..$(git rev-parse HEAD^{commit})\" case sounds\n>> worth a test.\n>\n> A test_must_fail?\n\nYep.\n\n>>> +#define REF_HANDLED (ALL_REV_FLAGS + 1)\n>>\n>> Could TMP_MARK be used for this?\n>\n> I don't know its usage, is it?\n\nSince handle_tags_and_duplicates() happens after the main revision\ntraversal, it would be safe. But it's probably not good style. Any\nlater revwalk would be confused by or clobber that flag.\n\nMy actual worry was that if there are too many rev flags some day,\nthis REF_HANDLED could wrap around to 0. Now I see that custom\nper-command flags are not so rare --- it is just this idiom for\nallocating them by adding 1 to the all-ones bitmask that is unusual.\n\nThe most common idiom is to simply start with 1u<<16:\n\n\t#define REF_HANDLED (1u<<16)\n\nunpack-objects uses 1u<<20 instead. blame starts with 1u<<12. reflog\nstarts with 1u<<10. A part of me wishes the command-specific flags\nwere allocated in revision.h like the standard ones so one could write\n\n\t#define REF_HANDLED REVFLAGSUSR1\n\nby analogy with SIGUSR1, or that there were some other mechanism for\navoiding collisions.\n\n>>> +             dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);\n>>> +\n>>> +             if (!prefixcmp(full_name, \"refs/tags/\") &&\n>>\n>> What happens if dwim_ref fails, perhaps because a ref was deleted in\n>> the meantime?\n>\n> That would be bad. I assumed that we have a lock on the refs, should I\n> add back the die check that's done by the other dwim_ref caller?\n\nSure, there's a lock. It doesn't stop a non-git process-gone-mad like\n/bin/rm from deleting a file under .git/refs. :)\n\ndie()-ing on error sounds sane.\n\n[...]\n>> If tag_of_filtered_mode == ABORT, we are going to die() soon, right?\n>\n> I don't know to be honest, perhaps we would have already died by now?\n\nIt's the handle_tag() call, later in handle_tags_and_duplicates().\n\n[...]\n>> When does the !get_object_mark() case come up?\n>\n> Eh, it has something to do with it being a replacement (rather than\n> the same), maybe? This is mostly just taken from Dscho's original\n> patch.\n\nAh, this is similar to the mysterious case from patch 2/3.\n\nProbably this is the \"git fast-export a..a\" case, where 'a' was not\ndumped because UNINTERESTING but we still want to reset refs/tags/a to\npoint to it. But won't handle_tag() write\n\n\ttags refs/heads/a\n\tfrom :0\n\t[tagger, etc]\n\nwhen we get to it?\n\nSide question: should the\n\n\tfor (i = extra_refs->nr - 1; i >= 0; i--) {\n\nloop should be earlier in the function and set REF_HANDLED where\nappropriate, to avoid resets for these objects, too?\n\n[...]\n>> Just curious: is the REF_HANDLED handling actually needed?  What\n>> would happen if fast-export included the redundant resets?\n>\n> That would just be sloppy :). I don't think anything particularly bad\n> would happen.\n\nI suppose this is needed to avoid pointless changes in output which\ncould break git's or other projects' test suites without good reason.\nMakes sense.\n\nThanks for the clarifications.\nJonathan\n"},{"id":"179015","messageId":"20111107093249.GD30641@elie.hsd1.il.comcast.net","threadId":"28857","inReplyTo":"CAGdFq_hmF8xDA8PdDUPygSSAVsvrA=BRVKp+eCVRggHxLZzBsQ@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-07T09:32:49Z","receivedAt":"2011-11-07T09:32:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sun, Nov 6, 2011 at 05:31, Jonathan Nieder wrote:\n\n>>> as they use marks\n>>> files to store which commits have already been seen. The call graph\n>>> is something as follows:\n[...]\n>>> $ # run `git branch foo` and push it to remote\n>>> $ git fast-export --{im,ex}port-marks=marksfile foo\n>>>\n>>> When fast-export imports the marksfile and sees that all commits in\n>>> foo are marked as UNINTERESTING\n>>\n>> Hmm, I didn't know about this behavior.  Would it be possible to add\n>> a test for it, too?\n>\n> What behavior are you referring to here? What kind of test would you want added?\n\nI meant I hadn't remembered that marks result in commits being marked\nas UNINTERESTING (even though the manpage warned me), and that it's\npossible a priori that fast-export could be broken when you run\n\n\tgit fast-export --import-marks=marksfile master\n\neven without breaking\n\n\tgit fast-export master..master\n\nBut don't worry about it --- I can try it as a follow-on when this\nseries next visits the list if that doesn't sound like fun. :)\n\nFWIW, with the clarifications to the commit message Junio made, I'm\nhappy with this patch.\n"},{"id":"180158","messageId":"201111301756.32305.trast@student.ethz.ch","threadId":"28857","inReplyTo":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-11-30T16:56:32Z","receivedAt":"2011-11-30T16:56:32Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Sverre Rabbelier wrote:\n> When a revision is specified on the commandline we explicitly output\n> a 'reset' command for it if it was not handled already. This allows\n> for example the remote-helper protocol to use fast-export to create\n> branches that point to a commit that has already been exported.\n> \n> Initial-patch-by: Johannes Schindelin \n> Signed-off-by: Sverre Rabbelier \n\nMy apologies if this is redundant, I'm not up to speed on progress\nhere. But a crash in t9350.19 caught my eye:\n\n checking known breakage: \n (\n cd limit-by-paths &&\n git fast-export master~2..master~1 > output &&\n test_cmp output expected\n )\n\n ==23766== Invalid read of size 1\n ==23766== at 0x4FD21E: prefixcmp (strbuf.c:9)\n ==23766== by 0x42B936: handle_tags_and_duplicates (fast-export.c:563)\n ==23766== by 0x42C274: cmd_fast_export (fast-export.c:732)\n ==23766== by 0x4051F1: run_builtin (git.c:308)\n ==23766== by 0x40538B: handle_internal_command (git.c:466)\n ==23766== by 0x4054A5: run_argv (git.c:512)\n ==23766== by 0x40562C: main (git.c:585)\n ==23766== Address 0x0 is not stack'd, malloc'd or (recently) free'd\n ==23766== \n {\n \n Memcheck:Addr1\n fun:prefixcmp\n fun:handle_tags_and_duplicates\n fun:cmd_fast_export\n fun:run_builtin\n fun:handle_internal_command\n fun:run_argv\n fun:main\n }\n ==23766== \n ==23766== Process terminating with default action of signal 11 (SIGSEGV)\n ==23766== Access not within mapped region at address 0x0\n ==23766== at 0x4FD21E: prefixcmp (strbuf.c:9)\n ==23766== by 0x42B936: handle_tags_and_duplicates (fast-export.c:563)\n ==23766== by 0x42C274: cmd_fast_export (fast-export.c:732)\n ==23766== by 0x4051F1: run_builtin (git.c:308)\n ==23766== by 0x40538B: handle_internal_command (git.c:466)\n ==23766== by 0x4054A5: run_argv (git.c:512)\n ==23766== by 0x40562C: main (git.c:585)\n\nThe crash is hidden by the fact that the test is test_expect_failure.\nIt bisects to this commit. Perhaps we should distinguish between\ntest_expect_failure and test_expect_crash?...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"201814","messageId":"CAMP44s1hdZb_7Lv8SEe+MsfC_q-nXsnjJobABFq6eFR_er4TaA@mail.gmail.com","threadId":"28857","inReplyTo":"1320535407-4933-2-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-24T17:52:28Z","receivedAt":"2012-10-24T17:52:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nJoined late to the party :)\n\nOn Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier wrote:\n> This happens only when the corresponding commits are not exported in\n> the current fast-export run. This can happen either when the relevant\n> commit is already marked, or when the commit is explicitly marked\n> as UNINTERESTING with a negative ref by another argument.\n>\n> This breaks fast-export based remote helpers, as they use marks\n> files to store which commits have already been seen. The call graph\n> is something as follows:\n>\n> $ # push master to remote repo\n> $ git fast-export --{im,ex}port-marks=marksfile master\n> $ # make a commit on master and push it to remote\n> $ git fast-export --{im,ex}port-marks=marksfile master\n> $ # run `git branch foo` and push it to remote\n> $ git fast-export --{im,ex}port-marks=marksfile foo\n\nThat is correctly, but try this:\n$ git fast-export --{im,ex}port-marks=marksfile foo foo\n\nNow foo is updated.\n\n> When fast-export imports the marksfile and sees that all commits in\n> foo are marked as UNINTERESTING (they have already been exported\n> while pushing master), it exits without doing anything. However,\n> what we want is for it to reset 'foo' to the already-exported commit.\n>\n> Either way demonstrates the problem, and since this is the most\n> succint way to demonstrate the problem it is implemented by passing\n> master..master on the commandline.\n>\n> Signed-off-by: Sverre Rabbelier \n> ---\n> t/t9350-fast-export.sh | 11 +++++++++++\n> 1 files changed, 11 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 950d0ff..74914dc 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -440,4 +440,15 @@ test_expect_success 'fast-export quotes pathnames' '\n> )\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> + git fast-export master..master > actual &&\n> + test_cmp expected actual\n> +'\n> +\n> test_done\n\nThis test is completely wrong.\n\n1) Where are the marks file?\n2) master..master shouldn't export anything\n3) Why do you expect a SHA-1? It could be a mark.\n\nI decided to write my own this way:\n\n---\ncat > expected << EOF\nreset refs/heads/master\nfrom ##mark##\n\nEOF\n\ntest_expect_failure 'refs are updated even if no commits need to be exported' '\n\tcp tmp-marks /tmp\n\tgit fast-export --import-marks=tmp-marks \\\n\t\t--export-marks=tmp-marks master | true &&\n\tgit fast-export --import-marks=tmp-marks \\\n\t\t--export-marks=tmp-marks master > actual &&\n\tmark=$(grep $(git rev-parse master) tmp-marks | cut -f 1 -d \" \")\n\tsed -i -e \"s/##mark##/$mark/\" expected &&\n\ttest_cmp expected actual\n'\n---\n\nYes, it's true this fails, but change to 'master master', and then it works.\n\nThis can be easily fixed by this patch:\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 12220ad..3b4c2d6 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -523,10 +523,13 @@ static void get_tags_and_duplicates(struct\nobject_array *pending,\n typename(e->item->type));\n continue;\n }\n- if (commit->util)\n- /* more than one name for the same object */\n+ /*\n+ * This ref will not be updated through a commit, lets make\n+ * sure it gets properly updated eventually.\n+ */\n+ if (commit->util || commit->object.flags & SHOWN)\n string_list_append(extra_refs,\nfull_name)->util = commit;\n- else\n+ if (!commit->util)\n commit->util = full_name;\n }\n }\n\nNow if you specify a ref it will get updated regardless. However, this\npoints to another bug:\n\n% git fast-export --{im,ex}port-marks=/tmp/marks master ^foo foo.foo\n\nThe foo ref will be reset _twice_ because all pending refs after the\nfirst one get reset no matter how they were specified.\n\nThat is already the case, my patch will cause this to generate the same output:\n\n% git fast-export --{im,ex}port-marks=/tmp/marks ^foo foo.foo\n\nWhich is still not got, but not catastrophic by any means.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201815","messageId":"CAMP44s2hX=y+tH4ANJp_Jj3OD4zaNccroVOd+51NhvFz=xZd7A@mail.gmail.com","threadId":"28857","inReplyTo":"1320535407-4933-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 3/3] fast-export: output reset command for commandline revs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-24T18:02:40Z","receivedAt":"2012-10-24T18:02:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier wrote:\n> When a revision is specified on the commandline we explicitly output\n> a 'reset' command for it if it was not handled already. This allows\n> for example the remote-helper protocol to use fast-export to create\n> branches that point to a commit that has already been exported.\n\nThis simpler patch does the same, doesn't it?\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 12220ad..3b4c2d6 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -523,10 +523,13 @@ static void get_tags_and_duplicates(struct\nobject_array *pending,\n typename(e->item->type));\n continue;\n }\n- if (commit->util)\n- /* more than one name for the same object */\n+ /*\n+ * This ref will not be updated through a commit, lets make\n+ * sure it gets properly updated eventually.\n+ */\n+ if (commit->util || commit->object.flags & SHOWN)\n string_list_append(extra_refs,\nfull_name)->util = commit;\n- else\n+ if (!commit->util)\n commit->util = full_name;\n }\n }\n\n> Initial-patch-by: Johannes Schindelin \n> Signed-off-by: Sverre Rabbelier \n> ---\n>\n> Most of the hard work for this patch was done by Dscho. The rest of\n> it was basically me applying the technique used by jch in c3502fa\n> (25-08-2011 do not include sibling history in --ancestry-path).\n>\n> The if statement dealing with tag_of_filtered_mode is not as\n> elegant as either me or Dscho would have liked, but we couldn't\n> find a better way to determine if a ref is a tag at this point\n> in the code.\n\nWhich is needed why?\n\nRight now if I do:\n% git fast-export --{im,ex}port-marks=/tmp/marks foo1 tag-to-foo1\n\nWhere tag-to-foo1 is a tag that that points to foo1, I get a reset for that.\n\n> Additionally, the elem->whence != REV_CMD_RIGHT case should really\n> check if REV_CMD_RIGHT_REF, but as this is not provided by the\n> ref_info structure this is left as is. A result of this is that\n> incorrect input will result in incorrect output, rather than an\n> error message. That is: `git fast-export a..` will\n> incorrectly generate a `reset ` statement in the fast-export\n> stream.\n\nI don't see the point of this.\n\nBesides, you can check the return value of dwim_ref, if it's not 1,\nthen you shouldn't generate a reset.\n\n> The dwim_ref bit is a double work (it has already been done by the\n> caller of this function), but I decided it would be more work to\n> pass this information along than to recompute it for the few\n> commandline refs that were relevant.\n\nIt's already stored in commit->util, you don't need to do that.\n\nAs I said, I think the patch above does the trick, and it even has the\nadvantage of not having the above a.. issues.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201816","messageId":"20121024180807.GA3338@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s1hdZb_7Lv8SEe+MsfC_q-nXsnjJobABFq6eFR_er4TaA@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-24T18:08:08Z","receivedAt":"2012-10-24T18:08:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Felipe,\n\nFelipe Contreras wrote:\n\n> This test is completely wrong.\n>\n> 1) Where are the marks file?\n> 2) master..master shouldn't export anything\n\nWhy shouldn't master..master export anything? It means \"update the\nmaster ref; we already have all commits up to and including master^0\".\n\nThe underlying problem is that fast-export takes rev-list arguments as\nparameters, which is unfortunately only an approximation to what is\nreally intended. Ideally it would separately take a list of refs to\nimport and rev-list arguments representing the commits we already\nhave.\n\nHoping that clarifies,\nJonathan\n"},{"id":"201819","messageId":"CAMP44s2RspCrRXZbRTsVwezyU9X=+8RF=_9Q+3zX75LBJkdoPA@mail.gmail.com","threadId":"28857","inReplyTo":"20121024180807.GA3338@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-24T19:09:05Z","receivedAt":"2012-10-24T19:09:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 24, 2012 at 8:08 PM, Jonathan Nieder wrote:\n> Hi Felipe,\n>\n> Felipe Contreras wrote:\n>\n>> This test is completely wrong.\n>>\n>> 1) Where are the marks file?\n>> 2) master..master shouldn't export anything\n>\n> Why shouldn't master..master export anything? It means \"update the\n> master ref; we already have all commits up to and including master^0\".\n\nDoes it mean that? I don't think so, but let's assume that's the case.\n\nWe don't have all those commits; without the marks we have nothing. Or\nwhat exactly do you mean by 'we'?\n\nGo to your git.git repository, and run this:\n\n% git git init /tmp/git\n% git fast-export master^..master | git --git-dir=/tmp/git/.git fast-import\n\nWhat do you expect? I expect a single commit, and that's what we get,\nnow do the same with 'master..master', what do you expect?\n\nHow about 'git fast-export ^master'? Do you expect to get anything\nthere? Or what about '^master master'?\n\nWithout marks these idioms don't make any sense. Now lets assume that\nmarks were meant to be there.\n\nIf 'master..master' is supposed to update master, then what is\n'master' supposed to do?\n\n% git fast-export --{im,ex}port-marks=/tmp/marks master..master\n\nvs.\n\n% git fast-export --{im,ex}port-marks=/tmp/marks master\n\nEither way, my patch will make 'master..master' throw a reset (if the\nmarks are present, I haven't tried without them), I don't think it\nshould, but that's a different story, and a different patch fix.\n\n> The underlying problem is that fast-export takes rev-list arguments as\n> parameters, which is unfortunately only an approximation to what is\n> really intended. Ideally it would separately take a list of refs to\n> import and rev-list arguments representing the commits we already\n> have.\n\nThe commits we already have (exported before) are stored in the marks.\nMaybe we can store the refs there as well, but that would not change\nthe semantics of refspecs, nor the fact that we need the marks.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201821","messageId":"20121024191149.GA3120@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s2RspCrRXZbRTsVwezyU9X=+8RF=_9Q+3zX75LBJkdoPA@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-24T19:11:49Z","receivedAt":"2012-10-24T19:11:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Does it mean that? I don't think so, but let's assume that's the case.\n>\n> We don't have all those commits; without the marks we have nothing. Or\n> what exactly do you mean by 'we'?\n\nNot everyone uses marks.\n\nCiao,\nJonathan\n"},{"id":"201828","messageId":"alpine.DEB.1.00.1210242333550.5980@bonsai2","threadId":"28857","inReplyTo":"CAMP44s1hdZb_7Lv8SEe+MsfC_q-nXsnjJobABFq6eFR_er4TaA@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-10-24T21:41:39Z","receivedAt":"2012-10-24T21:41:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 24 Oct 2012, Felipe Contreras wrote:\n\n> 2) master..master shouldn't export anything\n\nThe underlying issue -- as explained in the thread -- is when you want to\nupdate master to a commit that another ref already points to. In that case\nno commits need to exported, but the ref needs to be updated nevertheless.\n\nWe just wrote the test in the most convenient way, no need to complicate\nthings more than necessary.\n\nHth,\nJohannes\n"},{"id":"201839","messageId":"CAMP44s2kjv9fHbruXv7NyVm9m+FjFnYDryuPZQ-RQXN9Nj6MAw@mail.gmail.com","threadId":"28857","inReplyTo":"20121024191149.GA3120@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T04:19:31Z","receivedAt":"2012-10-25T04:19:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 24, 2012 at 9:11 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> Does it mean that? I don't think so, but let's assume that's the case.\n>>\n>> We don't have all those commits; without the marks we have nothing. Or\n>> what exactly do you mean by 'we'?\n>\n> Not everyone uses marks.\n\nWhen you don't have marks you have to export *everything* that you are\ninterested. If you want all the history from the root to master, then\nthat's what you will get (and you specify 'master'), if you want only\nthe commit pointed to master and nothing else that's what you will get\n(with 'master^..master'), but when you do 'master..master', you get\nnothing, because that's what you asked for.\n\nAgain, if you don't have marks, I don't see what you expect to be\nexported with 'master..master', even with marks, I don't see what you\nexpect.\n\n-- \nFelipe Contreras\n"},{"id":"201840","messageId":"20121025042731.GA11243@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s2kjv9fHbruXv7NyVm9m+FjFnYDryuPZQ-RQXN9Nj6MAw@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T04:27:31Z","receivedAt":"2012-10-25T04:27:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Again, if you don't have marks, I don't see what you expect to be\n> exported with 'master..master', even with marks, I don't see what you\n> expect.\n\nAnd that's fine. Unless you were trying to do some work and this lack\nof understanding got in the way.\n\nIn that case, with a calmer and more humble approach you might find\npeople willing to help you. Maybe they will learn something from you,\ntoo.\n\nCiao,\nJonathan\n"},{"id":"201844","messageId":"CAMP44s16mbFgS__NfXAexAS53PgwANK0-cU7wjeu5PYi=aJwEA@mail.gmail.com","threadId":"28857","inReplyTo":"alpine.DEB.1.00.1210242333550.5980@bonsai2","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T05:13:07Z","receivedAt":"2012-10-25T05:13:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Oct 24, 2012 at 11:41 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Wed, 24 Oct 2012, Felipe Contreras wrote:\n>\n>> 2) master..master shouldn't export anything\n>\n> The underlying issue -- as explained in the thread -- is when you want to\n> update master to a commit that another ref already points to. In that case\n> no commits need to exported, but the ref needs to be updated nevertheless.\n>\n> We just wrote the test in the most convenient way, no need to complicate\n> things more than necessary.\n\nThat test cannot work, and it shouldn't work.\n\nYou say you want to 'update master to a commit that another ref\nalready points to'. What other ref? If you want to update master, this\nis what you do:\n\n% git fast-export master\n\nWhat do you expect 'git fast-export master..master' to export? This?\n\n---\nreset refs/heads/master\nfrom $(git rev-parse master)\n\n---\n\nWhat is a remote helper supposed to do with a SHA-1? Nothing, a git\nSHA-1 is useless to say, a mercurial remote helper. To make sense of\nit you would need to access the git repository and get the commit\nobject, and that's defeating the purpose of a fast exporter.\n\nNo, that's not what you want.\n\nBut at this point there's only one ref in the picture, you said\n'update master to a commit that another ref already points to', but\nthere's only one ref, where is the other ref?\n\nMaybe your test should do this:\n\n% git fast-export foo master\n\nBut wait, that actually works, except that the output will be nothing\nclose what you expected before, we would get all the commits and files\nthat constitute 'foo', which is actually useful, and what we expect\nfrom fast-export, and in addition, master will be updated to the right\nref.\n\nNo, the problem is not only 'update master to a commit that another\nref already points to', but that this happens in two different\ncommands, and that can only be done with marks, just like the test I\nproposed.\n\nThe original test doesn't expose the problem we are trying to solve,\nand it shouldn't work anyway.\n\nMoreover, what we eventually want to do is support the transport\nhelpers, so how about you run this:\n\n---\n#!/bin/sh\n\ncat > git-remote-foo <<-\\EOF\n#!/bin/sh\n\nread l\necho $l 1>&2\necho export\necho refspec refs/heads/*:refs/foo/origin/*\ntest -e /tmp/marks-git && echo *import-marks /tmp/marks-git\necho *export-marks /tmp/marks-git\necho\n\nread l\necho $l 1>&2\necho ? refs/heads/master\necho\n\nread l\necho $l 1>&2\n\nwhile read l; do\n\techo $l 1>&2\n\ttest \"$l\" == 'done' && exit\ndone\nEOF\n\nchmod +x git-remote-foo\n\nexport PATH=$PWD:$PATH\n\nrm -f /tmp/marks-git\n\n(\ngit init test\ncd test\necho Test >> Test\ngit add --all\ngit commit -m 'Initial commit'\ngit branch foo\necho \"== master ==\"\ngit push foo::test master\necho \"== foo ==\"\ngit push foo::test foo\n)\n---\n\nI get this output with my patch:\n\n---\n[master (root-commit) b159eff] Initial commit\n 1 file changed, 1 insertion(+)\n create mode 100644 Test\n== master ==\ncapabilities\nlist\nexport\nfeature done\nblob\nmark :1\ndata 5\nTest\n\nreset refs/heads/master\ncommit refs/heads/master\nmark :2\nauthor Felipe Contreras 1351140987 +0200\ncommitter Felipe Contreras 1351140987 +0200\ndata 15\nInitial commit\nM 100644 :1 Test\n\ndone\n== foo ==\ncapabilities\nlist\nexport\nfeature done\nreset refs/heads/foo\nfrom :2\n\ndone\n---\n\nHey, did you see that? 'foo' is updated, both 'master' and 'foo' point\nto the same object.\n\nWhat is the problem?\n\n-- \nFelipe Contreras\n"},{"id":"201845","messageId":"CAMP44s1Pe8Ef6-GRbmSs7rY7gWyaPCN+jWGysyttZp3drSDoZg@mail.gmail.com","threadId":"28857","inReplyTo":"20121025042731.GA11243@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T05:18:37Z","receivedAt":"2012-10-25T05:18:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 6:27 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> Again, if you don't have marks, I don't see what you expect to be\n>> exported with 'master..master', even with marks, I don't see what you\n>> expect.\n>\n> And that's fine. Unless you were trying to do some work and this lack\n> of understanding got in the way.\n\nWhat is fine? What lack of understanding?\n\nYou still haven't said what you expect the output to be.\n\nConsider this repo:\n\n---\ngit init test\ncd test\necho one >> file\ngit add --all\ngit commit -m 'one'\necho two >> file\ngit commit -m 'one'\n---\n\nWhat *exactly* should the output of 'git fast-export master..master'\nbe? I say nothing, what do you say?\n\n> In that case, with a calmer and more humble approach you might find\n> people willing to help you. Maybe they will learn something from you,\n> too.\n\nI don't need help, I am helping you, I was asked to take a look at\nthis patch series. If you don't want my help, then by all means, keep\nthis series rotting, it has being doing so for the past year without\nanybody complaining.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201846","messageId":"20121025052823.GB11243@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s1Pe8Ef6-GRbmSs7rY7gWyaPCN+jWGysyttZp3drSDoZg@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T05:28:23Z","receivedAt":"2012-10-25T05:28:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> I don't need help, I am helping you, I was asked to take a look at\n> this patch series. If you don't want my help, then by all means, keep\n> this series rotting, it has being doing so for the past year without\n> anybody complaining.\n\nAh, so _that_ (namely getting Sverre's remote helper to work) is the\nwork you were trying to do. Thanks for explaining.\n\nIf I understand correctly, it is possible to get Sverre's remote\nhelper to work without affecting this particular testcase. From that\npoint of view I think you were on the right track.\n\nThe testcase is imho correct and does not need changing. So yes, I\ndon't want your help changing it. I don't suspect you will be using\n\"git fast-export $(git rev-parse master)..master\". It is safe and\ngood to add additional testcases documenting the syntax that you do\nuse, as an independent topic.\n\nThanks,\nJonathan\n"},{"id":"201847","messageId":"CAGdFq_gg3gPvCADje9ibz8xHgPOLF+=79EqksVzG2JeTOfHocw@mail.gmail.com","threadId":"28857","inReplyTo":"20121025052823.GB11243@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-25T05:39:25Z","receivedAt":"2012-10-25T05:39:25Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder wrote:\n> The testcase is imho correct and does not need changing. So yes, I\n> don't want your help changing it. I don't suspect you will be using\n> \"git fast-export $(git rev-parse master)..master\". It is safe and\n> good to add additional testcases documenting the syntax that you do\n> use, as an independent topic.\n\nTo re-iterate Dscho's point, the reason for this testcase is that if\nyou do this:\n$ git checkout master\n$ git branch next\n$ git push hg://example.com master\n$ git push hg://example.com next\n\nWith the current design, next will not be present on the remote. This\nis caused by the fact that git looks at \"fast-export ^master next\",\nsees that it's empty, and decides not to export anything. This patch\nseries solves that, by having \"fast-export ^master next\" emit a \"from\n:42\\nreset next\" (or something like that, assuming :42 is where master\nis currently at).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"201848","messageId":"CAMP44s3Xwt5+J_yGte_HC3hG+MhMkWnJQ7mtuB_Y+sOLB1b1+A@mail.gmail.com","threadId":"28857","inReplyTo":"20121025052823.GB11243@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T05:40:37Z","receivedAt":"2012-10-25T05:40:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 7:28 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> I don't need help, I am helping you, I was asked to take a look at\n>> this patch series. If you don't want my help, then by all means, keep\n>> this series rotting, it has being doing so for the past year without\n>> anybody complaining.\n>\n> Ah, so _that_ (namely getting Sverre's remote helper to work) is the\n> work you were trying to do. Thanks for explaining.\n\nNo, that's not what I'm doing. I haven't even seen a remote-hg branch\nfrom either Sverre, or Johannes. IIRC the msysgit wiki mentions that\nthere were some patches not quite accepted in upstream that prevented\nthe remote-hg from getting upstream. But I don't know which patches\nare those, I don't know why they are needed, and I haven't even been\nable to run this stuff.\n\nI was told this might be an issue for all remote helpers, and it seems\nto be the case (albeit a small issue IMO).\n\n> If I understand correctly, it is possible to get Sverre's remote\n> helper to work without affecting this particular testcase. From that\n> point of view I think you were on the right track.\n\nThat makes sense. So are there any other patches?\n\n> The testcase is imho correct and does not need changing. So yes, I\n> don't want your help changing it. I don't suspect you will be using\n> \"git fast-export $(git rev-parse master)..master\". It is safe and\n> good to add additional testcases documenting the syntax that you do\n> use, as an independent topic.\n\nAll right, so I run this and get this:\n\n% git fast-export master..master\nreset refs/heads/master\nfrom 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0\n\nAs an user of fast-export, what do I do with that now?\n\n-- \nFelipe Contreras\n"},{"id":"201849","messageId":"CAMP44s3kBxzJbyoxPqWbRMWmpX9sNPGjdRy_KrTeRoVmGC-+Hg@mail.gmail.com","threadId":"28857","inReplyTo":"CAGdFq_gg3gPvCADje9ibz8xHgPOLF+=79EqksVzG2JeTOfHocw@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T05:50:15Z","receivedAt":"2012-10-25T05:50:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 7:39 AM, Sverre Rabbelier wrote:\n> On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder wrote:\n>> The testcase is imho correct and does not need changing. So yes, I\n>> don't want your help changing it. I don't suspect you will be using\n>> \"git fast-export $(git rev-parse master)..master\". It is safe and\n>> good to add additional testcases documenting the syntax that you do\n>> use, as an independent topic.\n>\n> To re-iterate Dscho's point, the reason for this testcase is that if\n> you do this:\n> $ git checkout master\n> $ git branch next\n> $ git push hg://example.com master\n> $ git push hg://example.com next\n>\n> With the current design, next will not be present on the remote. This\n> is caused by the fact that git looks at \"fast-export ^master next\",\n> sees that it's empty, and decides not to export anything. This patch\n> series solves that, by having \"fast-export ^master next\" emit a \"from\n> :42\\nreset next\" (or something like that, assuming :42 is where master\n> is currently at).\n\nOnly if the remote helper is using marks, and this particular patch is\nadding a test-case without any use of marks at all.\n\nIOW; this test is testing something completely different, which\nhappens to fix the original issue, but this is not the only way to\nfix, and in IMO certainly not the best.\n\nAs I showed in my script above:\n\n$ git checkout master\n$ git branch next\n$ git push hg://example.com master\n$ git push hg://example.com next\n\nThis works just fine. Go ahead, apply my patch, and run it, the second\nbranch gets updated.\n\nIt will fail this test, but that's because the test is not testing\nwhat it should: that *when using marks* the second branch exported is\nignored.\n\nThis test does that:\n\n---\ncat > expected << EOF\nreset refs/heads/master\nfrom ##mark##\n\nEOF\n\ntest_expect_failure 'refs are updated even if no commits need to be exported' '\n cp tmp-marks /tmp\n git fast-export --import-marks=tmp-marks \\\n --export-marks=tmp-marks master | true &&\n git fast-export --import-marks=tmp-marks \\\n --export-marks=tmp-marks master > actual &&\n mark=$(grep $(git rev-parse master) tmp-marks | cut -f 1 -d \" \")\n sed -i -e \"s/##mark##/$mark/\" expected &&\n test_cmp expected actual\n'\n---\n\n-- \nFelipe Contreras\n"},{"id":"201850","messageId":"20121025055343.GA13729@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s3Xwt5+J_yGte_HC3hG+MhMkWnJQ7mtuB_Y+sOLB1b1+A@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T05:53:43Z","receivedAt":"2012-10-25T05:53:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> All right, so I run this and get this:\n>\n> % git fast-export master..master\n> reset refs/heads/master\n> from 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0\n>\n> As an user of fast-export, what do I do with that now?\n\nYou passed \"master..\" on the command line, indicating that your\nrepository already has commit 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0.\nNow you can update the \"master\" branch to point to that commit,\nas the fast-export output indicates.\n\nJonathan\n"},{"id":"201852","messageId":"CAGdFq_jfiX9apPyq6pba4S4iCQLGLmDvSrLaujSB5rO0i+fzfg@mail.gmail.com","threadId":"28857","inReplyTo":"CAMP44s3kBxzJbyoxPqWbRMWmpX9sNPGjdRy_KrTeRoVmGC-+Hg@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-25T06:07:17Z","receivedAt":"2012-10-25T06:07:17Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras\n wrote:\n> This works just fine. Go ahead, apply my patch, and run it, the second\n> branch gets updated.\n\nYes, but as you said:\n\n> That is already the case, my patch will cause this to generate the same output:\n> % git fast-export --{im,ex}port-marks=/tmp/marks ^foo foo.foo\n> Which is still not got, but not catastrophic by any means.\n\nWhich is exactly the reason we (Dscho and I during our little\nhackathon) went with the approach we did. We considered the approach\nyou took (if I still had the repository I might even find something\nvery like your patch in my reflog), but dismissed it for that reason.\nBy teaching fast-export to properly re-export interesting refs, this\nexporting of negated refs does not happen. Additionally, you say it is\nnot catastrophic, but it _is_, if you run: 'git fast-export ^master\nfoo', you do not expect master to suddenly show up on the remote side.\n\nI agree that your test more accurately describes what we're testing\n(and in fact, it should probably go in the tests for remote helpers).\nHowever, this test points out a shortcoming of fast-export that\nprevents us from implementing a cleaner solution to the 'fast-export\npush an existing ref' problem.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"201853","messageId":"CAMP44s1cRg_we5nXeRG1WcWz7YUOBrauJigeNna1YETcno9p=A@mail.gmail.com","threadId":"28857","inReplyTo":"CAGdFq_jfiX9apPyq6pba4S4iCQLGLmDvSrLaujSB5rO0i+fzfg@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T06:19:36Z","receivedAt":"2012-10-25T06:19:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 8:07 AM, Sverre Rabbelier wrote:\n> On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras\n> wrote:\n>> This works just fine. Go ahead, apply my patch, and run it, the second\n>> branch gets updated.\n>\n> Yes, but as you said:\n>\n>> That is already the case, my patch will cause this to generate the same output:\n>> % git fast-export --{im,ex}port-marks=/tmp/marks ^foo foo.foo\n>> Which is still not got, but not catastrophic by any means.\n>\n> Which is exactly the reason we (Dscho and I during our little\n> hackathon) went with the approach we did. We considered the approach\n> you took (if I still had the repository I might even find something\n> very like your patch in my reflog), but dismissed it for that reason.\n> By teaching fast-export to properly re-export interesting refs, this\n> exporting of negated refs does not happen.\n\nOh really? This is with your patches:\n\n% git fast-export --{im,ex}port-marks=/tmp/marks foo1 ^foo2 foo3..foo3\nreset refs/heads/foo1\nfrom :21\n\nreset refs/heads/foo3\nfrom :21\n\nreset refs/heads/foo3\nfrom :21\n\nreset refs/heads/foo2\nfrom :21\n\nThis is with mine:\n\n% ./git fast-export --{im,ex}port-marks=/tmp/marks foo1 ^foo2 foo3..foo3\nreset refs/heads/foo3\nfrom :21\n\nreset refs/heads/foo2\nfrom :21\n\nreset refs/heads/foo1\nfrom :21\n\nNow tell me again. What is the benefit of your approach?\n\n> Additionally, you say it is\n> not catastrophic, but it _is_, if you run: 'git fast-export ^master\n> foo', you do not expect master to suddenly show up on the remote side.\n\nIf 'git fast-export ^master foo' is catastrophic, so is 'git\nfast-export foo ^master', and that already exports master *today*.\n\n> I agree that your test more accurately describes what we're testing\n> (and in fact, it should probably go in the tests for remote helpers).\n> However, this test points out a shortcoming of fast-export that\n> prevents us from implementing a cleaner solution to the 'fast-export\n> push an existing ref' problem.\n\nWhich is something few users will notice. What they surely notice is\nthat there's no remote-hg they can readily use. Nobody expects all\nsoftware to be perfect or have all the features from day 1. Something\nthat just fetches a hg repo is already better than the current\nsituation: *nothing*.\n\nAnd BTW, in mercurial a commit can be only on one branch anyway, so\nyou can't have 'foo' and 'master' both pointing to the same\ncommit/revision. Sure bookmarks is another story, but again, I don't\nthink people would prefer remote-hg to stay out because bookmarks\ndon't work _perfectly_.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201854","messageId":"CAMP44s2bNZLiyinu3wgmw4gaRM9XUvA857-8fOGebhKYFmDesw@mail.gmail.com","threadId":"28857","inReplyTo":"20121025055343.GA13729@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T06:39:08Z","receivedAt":"2012-10-25T06:39:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 7:53 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> All right, so I run this and get this:\n>>\n>> % git fast-export master..master\n>> reset refs/heads/master\n>> from 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0\n>>\n>> As an user of fast-export, what do I do with that now?\n>\n> You passed \"master..\" on the command line, indicating that your\n> repository already has commit 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0.\n\nNo I didn't.\n\nMaybe I'm not interested in all the old history and I just want to\ncreate a repository from that point forward. For example 'git\nfast-export v1.5.0..master. I don't want no references to objects I\ndon't have, there's no way I can do anything sensible with that SHA-1.\n\n% git fast-export master..master | git --git-dir=/tmp/git/.git fast-import\nfatal: Not a valid commit: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0\nfast-import: dumping crash report to /tmp/git/.git/fast_import_crash_32498\n\nDoes it make sense to you that the output of fast-export doesn't work\nwith fast-import?\n\n> Now you can update the \"master\" branch to point to that commit,\n> as the fast-export output indicates.\n\nI don't have that commit, I don't even know what 8c7a786 means.\n\nShow me a single remote helper that manually stores SHA-1's and I\nmight believe you, but I doubt that, marks are too convenient. Or show\nme a script. I doubt there will be any, because otherwise somebody\nwould have pushed for this patch, and there doesn't seem to be too\nmany people.\n\nBut fine, lets assume it's a valid use-case and people need this... it\nstill has absolutely nothing to do with the original intent of the\npatch series. The series is in fact doing two things:\n\n1) Use SHA-1's when a mark can't be found\n2) Update refs that have been already visited (through marks)\n\nThese two things are orthogonal to each other, we should have two\ntests, and in fact, two separate patch series. One will be useful for\nremote helpers, the other one will be useful to nobody IMO, but that's\nsomething that can be discussed there, and I particularly don't care.\n\nMy test and my patch are good for 2), and so far I haven't seen\nanybody saying otherwise.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201857","messageId":"CAGdFq_hgYPF5eeCB9hSsjVfUyEhkBNJAtzoNuNqs5N6V-+w9Hg@mail.gmail.com","threadId":"28857","inReplyTo":"CAMP44s1cRg_we5nXeRG1WcWz7YUOBrauJigeNna1YETcno9p=A@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-25T07:06:53Z","receivedAt":"2012-10-25T07:06:53Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Oct 24, 2012 at 11:19 PM, Felipe Contreras\n wrote:\n> Oh really? This is with your patches:\n>\n> % git fast-export --{im,ex}port-marks=/tmp/marks foo1 ^foo2 foo3..foo3\n> reset refs/heads/foo1\n> from :21\n>\n> reset refs/heads/foo3\n> from :21\n>\n> reset refs/heads/foo3\n> from :21\n>\n> reset refs/heads/foo2\n> from :21\n\nThat's weird, we have this bit:\n\n+\t\tif (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n+\t\t\tcontinue;\n\nIf I understand correctly that should cause it to only output revs\n(e.g. 'foo1') and the rhs side of a have..want spec.\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"201858","messageId":"20121025071815.GA15790@elie.Belkin","threadId":"28857","inReplyTo":"CAMP44s2bNZLiyinu3wgmw4gaRM9XUvA857-8fOGebhKYFmDesw@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T07:18:15Z","receivedAt":"2012-10-25T07:18:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Show me a single remote helper that manually stores SHA-1's and I\n> might believe you, but I doubt that, marks are too convenient.\n\nOh dear lord. Why are you arguing? Explain how coming to a consensus\non this will help accomplish something useful, and then I can explain\nmy point of view. In the meantime, this seems like a waste of time.\n\nLet's agree to disagree.\n\nRegards,\nJonathan\n"},{"id":"201859","messageId":"20121025073454.GB15790@elie.Belkin","threadId":"28857","inReplyTo":"CAGdFq_hgYPF5eeCB9hSsjVfUyEhkBNJAtzoNuNqs5N6V-+w9Hg@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T07:34:55Z","receivedAt":"2012-10-25T07:34:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> That's weird, we have this bit:\n>\n> +\t\tif (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)\n> +\t\t\tcontinue;\n>\n> If I understand correctly that should cause it to only output revs\n> (e.g. 'foo1') and the rhs side of a have..want spec.\n\nIf I remember right, '^foo1' is (whence == REV_CMD_REV) with (flags ==\nUNINTERESTING). That's why sequencer.c checks for unadorned revs like\nthis:\n\n\tif (opts->revs->cmdline.nr == 1 &&\n\t opts->revs->cmdline.rev->whence == REV_CMD_REV &&\n\t opts->revs->no_walk &&\n\t !opts->revs->cmdline.rev->flags) {\n\nMaybe\n\n\tif (elem->flags & UNINTERESTING)\n\t\tcontinue;\n\tif (elem->whence == REV_CMD_PARENTS_ONLY)\t/* foo^@ */\n\t\tcontinue;\n\nwould work well here? That would handle bizarre cases like \"--not\nnext..master\" (and ordinary cases like \"master...next\") better, by\nfocusing on the semantics instead of syntax.\n"},{"id":"201860","messageId":"CAGdFq_j5sWsHwJY-rWP-XJ6cMF6uwSq=9beFe9ZuZyixBa1fVA@mail.gmail.com","threadId":"28857","inReplyTo":"20121025073454.GB15790@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-25T07:43:03Z","receivedAt":"2012-10-25T07:43:03Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Thu, Oct 25, 2012 at 12:34 AM, Jonathan Nieder wrote:\n> If I remember right, '^foo1' is (whence == REV_CMD_REV) with (flags ==\n> UNINTERESTING). That's why sequencer.c checks for unadorned revs like\n> this:\n>\n> if (opts->revs->cmdline.nr == 1 &&\n> opts->revs->cmdline.rev->whence == REV_CMD_REV &&\n> opts->revs->no_walk &&\n> !opts->revs->cmdline.rev->flags) {\n>\n> Maybe\n>\n> if (elem->flags & UNINTERESTING)\n> continue;\n> if (elem->whence == REV_CMD_PARENTS_ONLY) /* foo^@ */\n> continue;\n>\n> would work well here? That would handle bizarre cases like \"--not\n> next..master\" (and ordinary cases like \"master...next\") better, by\n> focusing on the semantics instead of syntax.\n\nI know there was a reason why using UNINTERESTING didn't work\n(otherwise we could've used that to start with, instead of needing\nJunio's whence solution). I think all refs ended up being marked as\nUNINTERESTING or somesuch.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"201862","messageId":"20121025074829.GD15790@elie.Belkin","threadId":"28857","inReplyTo":"CAGdFq_j5sWsHwJY-rWP-XJ6cMF6uwSq=9beFe9ZuZyixBa1fVA@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-10-25T07:48:29Z","receivedAt":"2012-10-25T07:48:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> I know there was a reason why using UNINTERESTING didn't work\n> (otherwise we could've used that to start with, instead of needing\n> Junio's whence solution). I think all refs ended up being marked as\n> UNINTERESTING or somesuch.\n\nTrue. Is it be possible to check UNINTERESTING in revs->cmdline\nbefore the walk?\n"},{"id":"201863","messageId":"CAGdFq_ifDYKTXt_cKkz5ZTBPgZKB7HFEbWereNnhSmS3qbhbsA@mail.gmail.com","threadId":"28857","inReplyTo":"20121025074829.GD15790@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-10-25T07:50:58Z","receivedAt":"2012-10-25T07:50:58Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder wrote:\n> Sverre Rabbelier wrote:\n>\n>> I know there was a reason why using UNINTERESTING didn't work\n>> (otherwise we could've used that to start with, instead of needing\n>> Junio's whence solution). I think all refs ended up being marked as\n>> UNINTERESTING or somesuch.\n>\n> True. Is it be possible to check UNINTERESTING in revs->cmdline\n> before the walk?\n\nThat might work, maybe Dscho remembers why we did not go with that approach.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"201901","messageId":"CAMP44s0guY7GUhDVuuehGwvyv4ZFWPkxmnjMxhV257cN7uDtgg@mail.gmail.com","threadId":"28857","inReplyTo":"CAGdFq_ifDYKTXt_cKkz5ZTBPgZKB7HFEbWereNnhSmS3qbhbsA@mail.gmail.com","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T13:33:03Z","receivedAt":"2012-10-25T13:33:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 9:50 AM, Sverre Rabbelier wrote:\n> On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder wrote:\n>> Sverre Rabbelier wrote:\n>>\n>>> I know there was a reason why using UNINTERESTING didn't work\n>>> (otherwise we could've used that to start with, instead of needing\n>>> Junio's whence solution). I think all refs ended up being marked as\n>>> UNINTERESTING or somesuch.\n>>\n>> True. Is it be possible to check UNINTERESTING in revs->cmdline\n>> before the walk?\n\nIt is possible to check in revs->pending, but '^foo master' will mark\nthem both as UNINTERESTING, and 'master..master' as well, which again,\nis what we actually want, because that's how it works in the rest of\ngit.\n\n> That might work, maybe Dscho remembers why we did not go with that approach.\n\nBecause you want 'master..master' to output something, but that's\nwrong; it's changing the semantics of commitishes, and you don't need\nthat to solve this problem.\n\n-- \nFelipe Contreras\n"},{"id":"201910","messageId":"CAMP44s0+=t_yVXkGEa=Zm_ugBOxaMnG+ApyriSGn3DqTV-5nEA@mail.gmail.com","threadId":"28857","inReplyTo":"20121025071815.GA15790@elie.Belkin","subject":"Re: [PATCH 1/3] t9350: point out that refs are not updated correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-25T16:43:14Z","receivedAt":"2012-10-25T16:43:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 25, 2012 at 9:18 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> Show me a single remote helper that manually stores SHA-1's and I\n>> might believe you, but I doubt that, marks are too convenient.\n>\n> Oh dear lord. Why are you arguing? Explain how coming to a consensus\n> on this will help accomplish something useful, and then I can explain\n> my point of view. In the meantime, this seems like a waste of time.\n\nWe don't need to come to a consensus because there is no problem.\nNobody has requested this feature, and nobody has faced any problem\nwith this. If you have no evidence of the contrary, that's what I'll\nbelieve.\n\nI agree it's a waste of time, so let's not talk about the :0 -> SHA-1\nfeature, or the master..master feature in this thread; they are\northogonal.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"368030","messageId":"nycvar.QRO.7.76.6.1901292041210.41@tvgsbejvaqbjf.bet","threadId":"28857","inReplyTo":"CAGdFq_i=f+ZD7pdN0D-hFBeq6TejXtt15Rb07UDViv1=nnXkmg@mail.gmail.com","subject":"Re: [PATCH 2/3] fast-export: do not refer to non-existing marks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-29T19:41:40Z","receivedAt":"2019-01-29T19:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sverre,\n\nOn Sun, 6 Nov 2011, Sverre Rabbelier wrote:\n\n> On Sun, Nov 6, 2011 at 05:45, Jonathan Nieder wrote:\n> >> Extract a handle_reset function that deals with this, which can then\n> >> be re-used in a later commit.\n> >\n> > So, does this patch drop the confusing behavior and add one that is\n> > more intuitive for remote helpers?  It's not clear from this\n> > description what sort of deal the patch makes and whether it is a good\n> > or bad one.\n> \n> Ah, yes. Perhaps something like:\n> \n> \"Extract a reset_ref function that deals with this situation by\n> printing the commit sha1 when no mark has been written yet.\"\n> \n> > Ah --- the functional change is to use a sha1 when there is no mark\n> > corresponding to the object.\n> >\n> > Why is this codepath being run at all when b is excluded by the\n> > revision range (a..a a = ^a a a)?  Is this the same bug tested\n> > for in patch 1/3 or something separate?\n> \n> I must admit that I don't recall how exactly we stumbled on this case.\n> It might even make sense to instead die when we run into this corner\n> case, but I'm not convinced that there's no valid use case for this\n> (which we would block by die-ing).\n\nI know, it has been a while since we hacked on this in your tiny room in\nthe Netherlands, and it has been almost as long since this here mail\nthread stalled, when the consensus back then seemed that this patch is not\neven necessary.\n\nYou might find it satisfying that this change, in a slightly different\nform, made it to `master` recently, more precisely in\nhttps://github.com/git/git/commit/530ca19c02b1fa1d13195d24fc76c2926ceecdc2\n\nSo: closure, at long last.\n\nCiao,\nDscho"}]}