{"thread":{"id":"32184","subject":"[PATCH] fast-export: Allow pruned-references in mark file","startedAt":"2012-11-24T09:47:12Z","lastAt":"2013-04-06T17:33:23Z","messageCount":13,"participants":["Antoine Pelisse","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"203760","messageId":"1353750432-17373-1-git-send-email-apelisse@gmail.com","threadId":"32184","inReplyTo":null,"subject":"[PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-24T09:47:12Z","receivedAt":"2012-11-24T09:47:12Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"fast-export can fail because of some pruned-reference when importing a\nmark file.\n\nThe problem happens in the following scenario:\n\n $ git fast-export --export-marks=MARKS master\n (rewrite master)\n $ git prune\n $ git fast-export --import-marks=MARKS master\n\nThis might fail if some references have been removed by prune\nbecause some marks will refer to non-existing commits.\n\nLet's warn when we have a mark for a commit we don't know.\nAlso, increment the last_idnum before, so we don't override\nthe mark.\n\nSigned-off-by: Antoine Pelisse \n---\n builtin/fast-export.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 12220ad..141b245 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -607,16 +607,19 @@ static void import_marks(char *input_file)\n \t\t\t|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n \t\t\tdie(\"corrupt mark line: %s\", line);\n \n+\t\tif (last_idnum < mark)\n+\t\t\tlast_idnum = mark;\n+\n \t\tobject = parse_object(sha1);\n-\t\tif (!object)\n-\t\t\tdie (\"Could not read blob %s\", sha1_to_hex(sha1));\n+\t\tif (!object) {\n+\t\t\twarning(\"Could not read blob %s\", sha1_to_hex(sha1));\n+\t\t\tcontinue;\n+\t\t}\n \n \t\tif (object->flags & SHOWN)\n \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n \n \t\tmark_object(object, mark);\n-\t\tif (last_idnum < mark)\n-\t\t\tlast_idnum = mark;\n \n \t\tobject->flags |= SHOWN;\n \t}\n-- \n1.7.9.5\n"},{"id":"203846","messageId":"7vd2z1xb6c.fsf@alter.siamese.dyndns.org","threadId":"32184","inReplyTo":"1353750432-17373-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T04:03:55Z","receivedAt":"2012-11-26T04:03:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> fast-export can fail because of some pruned-reference when importing a\n> mark file.\n>\n> The problem happens in the following scenario:\n>\n> $ git fast-export --export-marks=MARKS master\n> (rewrite master)\n> $ git prune\n> $ git fast-export --import-marks=MARKS master\n>\n> This might fail if some references have been removed by prune\n> because some marks will refer to non-existing commits.\n>\n> Let's warn when we have a mark for a commit we don't know.\n> Also, increment the last_idnum before, so we don't override\n> the mark.\n\nIs this a safe and sane thing to do, and if so why? Could you\ndescribe that in the log message here?\n\n> Signed-off-by: Antoine Pelisse \n> ---\n> builtin/fast-export.c | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 12220ad..141b245 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -607,16 +607,19 @@ static void import_marks(char *input_file)\n> \t\t\t|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n> \t\t\tdie(\"corrupt mark line: %s\", line);\n> \n> +\t\tif (last_idnum < mark)\n> +\t\t\tlast_idnum = mark;\n> +\n> \t\tobject = parse_object(sha1);\n> -\t\tif (!object)\n> -\t\t\tdie (\"Could not read blob %s\", sha1_to_hex(sha1));\n> +\t\tif (!object) {\n> +\t\t\twarning(\"Could not read blob %s\", sha1_to_hex(sha1));\n> +\t\t\tcontinue;\n> +\t\t}\n> \n> \t\tif (object->flags & SHOWN)\n> \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n> \n> \t\tmark_object(object, mark);\n> -\t\tif (last_idnum < mark)\n> -\t\t\tlast_idnum = mark;\n> \n> \t\tobject->flags |= SHOWN;\n> \t}\n"},{"id":"203869","messageId":"CAMP44s0iSkqcOW0YsD=Jm_=x1tuoRbFQ+EbVvkROa_yY2-WFcA@mail.gmail.com","threadId":"32184","inReplyTo":"7vd2z1xb6c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-26T11:37:07Z","receivedAt":"2012-11-26T11:37:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano wrote:\n> Antoine Pelisse writes:\n>\n>> fast-export can fail because of some pruned-reference when importing a\n>> mark file.\n>>\n>> The problem happens in the following scenario:\n>>\n>> $ git fast-export --export-marks=MARKS master\n>> (rewrite master)\n>> $ git prune\n>> $ git fast-export --import-marks=MARKS master\n>>\n>> This might fail if some references have been removed by prune\n>> because some marks will refer to non-existing commits.\n>>\n>> Let's warn when we have a mark for a commit we don't know.\n>> Also, increment the last_idnum before, so we don't override\n>> the mark.\n>\n> Is this a safe and sane thing to do, and if so why? Could you\n> describe that in the log message here?\n\nWhy would fast-export try to export something that was pruned? Doesn't\nthat mean it wasn't reachable?\n\nEssentially, if 'git rev-list $foo' can't possibly export this pruned\nobject, why would 'git fast-export $foo' would?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203878","messageId":"CALWbr2yZpAT=eSahGcGKw5weoz1MjTzbb16pdQndKDFcn_3VJg@mail.gmail.com","threadId":"32184","inReplyTo":"CAMP44s0iSkqcOW0YsD=Jm_=x1tuoRbFQ+EbVvkROa_yY2-WFcA@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-26T13:23:58Z","receivedAt":"2012-11-26T13:23:58Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras\n wrote:\n> On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano wrote:\n>> Is this a safe and sane thing to do, and if so why? Could you\n>> describe that in the log message here?\n> Why would fast-export try to export something that was pruned? Doesn't\n> that mean it wasn't reachable?\n\nHello Junio,\nHello Felipe,\n\nActually the issue happened while using Felipe's branch with his\ngit-remote-hg. Everything was going fine until I (or did it run\nautomatically, I dont remember) ran git gc that pruned unreachable\nobjects. Of course some of the branch I had pushed to the hg remote\nhad been changed (most likely rebased). References no longer exists\nin the repository (cleaned by gc), but the reference still exists in\nmark file, as it was exported earlier. Thus the failure when git\nfast-export reads the mark file.\n\nThen, is it safe ?\nUpdating the last_idnum as I do in the patch doesn't work because\nif the reference is the last, the number is going to be overwriten\nin the next run.\nFrom git point of view, I guess it is fine. The file is fully read at\nthe beginning of fast-export and fully written at the end.\nThe issue is more for git-remote-hg that keeps track of\nmatches between git marks and hg commits. The marks are going to\nchange and be overriden. It will most likely need to read the mark\nfile to see if a ref has changed, and update it's dictionary.\n\nOne of the solution I'm thinking of, is to update the mark file\nwith marks of newly exported objects instead of recreating it,\nand let obsolete references in the file. But of course that is\nnot acceptable.\n\nCheers,\nAntoine\n"},{"id":"203885","messageId":"CAMP44s3Xo2ko6X1-SO3hLiTYHA3+i912jTGOQCUihixxcbEuRQ@mail.gmail.com","threadId":"32184","inReplyTo":"CALWbr2yZpAT=eSahGcGKw5weoz1MjTzbb16pdQndKDFcn_3VJg@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-26T14:04:21Z","receivedAt":"2012-11-26T14:04:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 26, 2012 at 2:23 PM, Antoine Pelisse wrote:\n> On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras\n> wrote:\n>> On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano wrote:\n>>> Is this a safe and sane thing to do, and if so why? Could you\n>>> describe that in the log message here?\n>> Why would fast-export try to export something that was pruned? Doesn't\n>> that mean it wasn't reachable?\n>\n> Hello Junio,\n> Hello Felipe,\n>\n> Actually the issue happened while using Felipe's branch with his\n> git-remote-hg. Everything was going fine until I (or did it run\n> automatically, I dont remember) ran git gc that pruned unreachable\n> objects. Of course some of the branch I had pushed to the hg remote\n> had been changed (most likely rebased). References no longer exists\n> in the repository (cleaned by gc), but the reference still exists in\n> mark file, as it was exported earlier. Thus the failure when git\n> fast-export reads the mark file.\n\nAh, I see, so these objects are _before_ fast-export tries to do\nanything, it's just importing the marks without any knowledge if these\nobjects are going to be used in the export or not.\n\nIf that's the case, I don't think it should throw a warning even just skip them.\n\nThen, in the actual export if some of these objects are referenced the\nexport would fail anyway (but they won't).\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203886","messageId":"CALWbr2ympYDTpJ4wSSc8ThYKtE5gvcf1OM-ztj5cry_TDsJr9w@mail.gmail.com","threadId":"32184","inReplyTo":"CAMP44s3Xo2ko6X1-SO3hLiTYHA3+i912jTGOQCUihixxcbEuRQ@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-26T14:14:15Z","receivedAt":"2012-11-26T14:14:15Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> If that's the case, I don't think it should throw a warning even just skip them.\n\nRemoving the warning seems fine to me.\n\n> Then, in the actual export if some of these objects are referenced the\n> export would fail anyway (but they won't).\n\nOf course it will fail to export anything that requires the missing object.\nAs they are unreachable, it will be hard to provide a ref that needs\nit anyway.\n\nOn the other hand, I'm afraid that your file\n'.git/hg//marks-hg' needs consistent references to mark.\nIf a mark is removed, and then replaced by another object, can it\nbreak somehow git-remote-hg ? If not, I can provide a simpler patch.\nIf it does, it will be more complicated.\n\nCheers,\nAntoine\n"},{"id":"203894","messageId":"7vhaoctg6i.fsf@alter.siamese.dyndns.org","threadId":"32184","inReplyTo":"CALWbr2yZpAT=eSahGcGKw5weoz1MjTzbb16pdQndKDFcn_3VJg@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T17:41:41Z","receivedAt":"2012-11-26T17:41:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras\n> wrote:\n>> On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano wrote:\n>>> Is this a safe and sane thing to do, and if so why? Could you\n>>> describe that in the log message here?\n>> Why would fast-export try to export something that was pruned? Doesn't\n>> that mean it wasn't reachable?\n>\n> Hello Junio,\n> Hello Felipe,\n>\n> Actually the issue happened while using Felipe's branch with his\n> git-remote-hg. Everything was going fine until I (or did it run\n> automatically, I dont remember) ran git gc that pruned unreachable\n> objects. Of course some of the branch I had pushed to the hg remote\n> had been changed (most likely rebased). References no longer exists\n> in the repository (cleaned by gc), but the reference still exists in\n> mark file, as it was exported earlier. Thus the failure when git\n> fast-export reads the mark file.\n\nYou described that part very well in your proposed log message and I\ngot it just fine.\n\n> Then, is it safe ?\n> Updating the last_idnum as I do in the patch doesn't work because\n> if the reference is the last, the number is going to be overwriten\n> in the next run.\n> From git point of view, I guess it is fine. The file is fully read at\n> the beginning of fast-export and fully written at the end.\n\nI am not sure I follow the above, but anyway, I think the patch does\nis safe because (1) future \"fast-export\" will not refer to these\npruned objects in its output (we have decided that these pruned\nobjects are not used anywhere in the history so nobody will refer to\nthem) and (2) we still need to increment the id number so that later\nobjects in the marks file get assigned the same id number as they\nwere assigned originally (otherwise we will not name these objects\nconsistently when we later talk about them).\n\nAnd I wanted to see that kind of reasoning behind the patch in the\nproposed log message, because other people will need to refer to it\nwhen they read \"git log\" output to understand the change.\n\nThanks.\n"},{"id":"203912","messageId":"CALWbr2x4aia4DcdnmfEEBsZwCYasTEp2Jc0jwJgvsUqWSDaWTQ@mail.gmail.com","threadId":"32184","inReplyTo":"7vhaoctg6i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-26T20:04:13Z","receivedAt":"2012-11-26T20:04:13Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> I am not sure I follow the above, but anyway, I think the patch does\n> is safe because (1) future \"fast-export\" will not refer to these\n> pruned objects in its output (we have decided that these pruned\n> objects are not used anywhere in the history so nobody will refer to\n> them) and (2) we still need to increment the id number so that later\n> objects in the marks file get assigned the same id number as they\n> were assigned originally (otherwise we will not name these objects\n> consistently when we later talk about them).\n\nI fully agree on (1), not so much on (2) though.\n\nI have the following behavior using my patch and running that script\nthat doesn't look correct.\n\necho \"Working scenario\"\ngit init test &&\n(cd test &&\ngit commit --allow-empty -m \"Commit mark :1\" &&\ngit commit --allow-empty -m \"Commit mark :2\" &&\ngit fast-export --export-marks=MARKS master > /dev/null &&\ncat MARKS &&\ngit reset HEAD~1 &&\nsleep 1 &&\ngit reflog expire --all --expire=now &&\ngit prune --expire=now &&\ngit commit --allow-empty -m \"Commit mark :3\" &&\ngit fast-export --import-marks=MARKS \\\n --export-marks=MARKS master > /dev/null &&\ncat MARKS) &&\nrm -rf test\n\necho \"Non-working scenario\"\ngit init test &&\n(cd test &&\ngit commit --allow-empty -m \"Commit mark :1\" &&\ngit commit --allow-empty -m \"Commit mark :2\" &&\ngit fast-export --export-marks=MARKS master > /dev/null &&\ncat MARKS &&\ngit reset HEAD~1 &&\nsleep 1 &&\ngit reflog expire --all --expire=now &&\ngit prune --expire=now &&\ngit fast-export --import-marks=MARKS \\\n --export-marks=MARKS master > /dev/null &&\ngit commit --allow-empty -m \"Commit mark :3\" &&\ngit fast-export --import-marks=MARKS \\\n --export-marks=MARKS master > /dev/null &&\ncat MARKS) &&\nrm -rf test\n\noutputs something like this:\nWorking scenario\nInitialized empty Git repository in /home/antoine/test/.git/\n[master (root-commit) 6cf350d] Commit mark :1\n[master 8f97f85] Commit mark :2\n:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966\n:2 8f97f85e1e7badf6a3daf411cf8d1133b00d522e\n[master 21cadfd] Commit mark :3\nwarning: Could not read blob 8f97f85e1e7badf6a3daf411cf8d1133b00d522e\n:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966\n:3 21cadfd87d90c05ce8770c968e5ed3d072ead4ae\nNon-working scenario\nInitialized empty Git repository in /home/antoine/test/.git/\n[master (root-commit) 5b5f7ec] Commit mark :1\n[master b224390] Commit mark :2\n:2 b224390daee199644495c15503882eb84df07df5\n:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678\nwarning: Could not read blob b224390daee199644495c15503882eb84df07df5\n[master 181a774] Commit mark :3\n:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678\n:2 181a7744c6d3428edb01a1adc9df247e9620be5f\n\nBoth \"commit mark :2\" and \"commit mark :3\" end up being marked :2.\nAny tool like git-remote-hg that is using a mapping from mark <-> hg changeset\ncould then fail.\n"},{"id":"203919","messageId":"CAMP44s1iLUXQTd2cKbfG196eirPeqRyWHe-Rooi6BcV9H+SeDQ@mail.gmail.com","threadId":"32184","inReplyTo":"CALWbr2x4aia4DcdnmfEEBsZwCYasTEp2Jc0jwJgvsUqWSDaWTQ@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-26T20:39:20Z","receivedAt":"2012-11-26T20:39:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 26, 2012 at 9:04 PM, Antoine Pelisse wrote:\n>> I am not sure I follow the above, but anyway, I think the patch does\n>> is safe because (1) future \"fast-export\" will not refer to these\n>> pruned objects in its output (we have decided that these pruned\n>> objects are not used anywhere in the history so nobody will refer to\n>> them) and (2) we still need to increment the id number so that later\n>> objects in the marks file get assigned the same id number as they\n>> were assigned originally (otherwise we will not name these objects\n>> consistently when we later talk about them).\n>\n> I fully agree on (1), not so much on (2) though.\n>\n> I have the following behavior using my patch and running that script\n> that doesn't look correct.\n>\n> echo \"Working scenario\"\n> git init test &&\n> (cd test &&\n> git commit --allow-empty -m \"Commit mark :1\" &&\n> git commit --allow-empty -m \"Commit mark :2\" &&\n> git fast-export --export-marks=MARKS master > /dev/null &&\n> cat MARKS &&\n> git reset HEAD~1 &&\n> sleep 1 &&\n> git reflog expire --all --expire=now &&\n> git prune --expire=now &&\n> git commit --allow-empty -m \"Commit mark :3\" &&\n> git fast-export --import-marks=MARKS \\\n> --export-marks=MARKS master > /dev/null &&\n> cat MARKS) &&\n> rm -rf test\n>\n> echo \"Non-working scenario\"\n> git init test &&\n> (cd test &&\n> git commit --allow-empty -m \"Commit mark :1\" &&\n> git commit --allow-empty -m \"Commit mark :2\" &&\n> git fast-export --export-marks=MARKS master > /dev/null &&\n> cat MARKS &&\n> git reset HEAD~1 &&\n> sleep 1 &&\n> git reflog expire --all --expire=now &&\n> git prune --expire=now &&\n> git fast-export --import-marks=MARKS \\\n> --export-marks=MARKS master > /dev/null &&\n> git commit --allow-empty -m \"Commit mark :3\" &&\n> git fast-export --import-marks=MARKS \\\n> --export-marks=MARKS master > /dev/null &&\n> cat MARKS) &&\n> rm -rf test\n>\n> outputs something like this:\n> Working scenario\n> Initialized empty Git repository in /home/antoine/test/.git/\n> [master (root-commit) 6cf350d] Commit mark :1\n> [master 8f97f85] Commit mark :2\n> :1 6cf350d7ecb3dc6573b00f839a6a51625ed28966\n> :2 8f97f85e1e7badf6a3daf411cf8d1133b00d522e\n> [master 21cadfd] Commit mark :3\n> warning: Could not read blob 8f97f85e1e7badf6a3daf411cf8d1133b00d522e\n> :1 6cf350d7ecb3dc6573b00f839a6a51625ed28966\n> :3 21cadfd87d90c05ce8770c968e5ed3d072ead4ae\n> Non-working scenario\n> Initialized empty Git repository in /home/antoine/test/.git/\n> [master (root-commit) 5b5f7ec] Commit mark :1\n> [master b224390] Commit mark :2\n> :2 b224390daee199644495c15503882eb84df07df5\n> :1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678\n> warning: Could not read blob b224390daee199644495c15503882eb84df07df5\n> [master 181a774] Commit mark :3\n> :1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678\n> :2 181a7744c6d3428edb01a1adc9df247e9620be5f\n>\n> Both \"commit mark :2\" and \"commit mark :3\" end up being marked :2.\n> Any tool like git-remote-hg that is using a mapping from mark <-> hg changeset\n> could then fail.\n\nI don't understand. \"commit mark :2\" 'git fast-export' would never\npoint to that object again, the new commit would override that mark:\n\ncommit refs/heads/master\nmark :2\n...\ncommit mark :3\n\nThen 'git remote-hg' should override that mark as well.\n\nBut it doesn't matter, because that would be the case only for the\nlast object, as soon as you find another valid object, that object's\nmark will be considered the last one.\n\nAnd what Junio said is consistent with what you want: last_idnum\nshould be updated even if the object is not valid.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203920","messageId":"7vobikqelo.fsf@alter.siamese.dyndns.org","threadId":"32184","inReplyTo":"CALWbr2x4aia4DcdnmfEEBsZwCYasTEp2Jc0jwJgvsUqWSDaWTQ@mail.gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T20:44:03Z","receivedAt":"2012-11-26T20:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n>> I am not sure I follow the above, but anyway, I think the patch does\n>> is safe because (1) future \"fast-export\" will not refer to these\n>> pruned objects in its output (we have decided that these pruned\n>> objects are not used anywhere in the history so nobody will refer to\n>> them) and (2) we still need to increment the id number so that later\n>> objects in the marks file get assigned the same id number as they\n>> were assigned originally (otherwise we will not name these objects\n>> consistently when we later talk about them).\n>\n> I fully agree on (1), not so much on (2) though.\n> ...\n> Both \"commit mark :2\" and \"commit mark :3\" end up being marked :2.\n> Any tool like git-remote-hg that is using a mapping from mark <-> hg changeset\n> could then fail.\n\nYeah, I think I agree that you would need to make sure that the\nother side does not use the revision marked with :2, once you retire\nthe object you originally marked with :2 by pruning. Shouldn't the\nsecond export show :1 and :3 but not :2? It feels like a bug in the\nexporter to me that the mark number is reused in such a case.\n"},{"id":"204371","messageId":"CALWbr2yfBoMRSiRwUB04gjcPSypfMw5u+q2nGWw+e0GDTHzqUw@mail.gmail.com","threadId":"32184","inReplyTo":"7vobikqelo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-01T10:10:04Z","receivedAt":"2012-12-01T10:10:04Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> Yeah, I think I agree that you would need to make sure that the\n> other side does not use the revision marked with :2, once you retire\n> the object you originally marked with :2 by pruning. Shouldn't the\n> second export show :1 and :3 but not :2? It feels like a bug in the\n> exporter to me that the mark number is reused in such a case.\n\nIt depends what you call a bug.\n\nIf the last item from the list is pruned, and no new objects\nare exported, you will lose both reference and count to mark :2.\nIn this situation, incrementing last_idnum was pointless.\n\nAssuming that we can't do anything about that, marks should be\nconsidered mutable (and I don't think there is any way it\nshouldn't). Then incrementing last_idnum is always useless.\n\nNow, if marks can change, I don't understand why we use them at all.\n(or don't provide the possibility to not use them at least).\n\nIn the \"hg <-> git\" case, it seems like an unecessary step:\n\nhg revs <-> git marks <-> git sha1\n\nPotentially forces the remote-helper to re-read the \"marks <-> sha1\"\neverytime.\n\nAlso in the remote-helper, the \"list\" command requires sha1 for each\nheads, while \"import/export\" can't work with sha1 but only marks, which\nseems inconsistent.\n\nMy last point is about \"git-remote-hg\" and still mutable revs.\nIt seems like Felipe is using revs() rather than node() or hex() to\nrefer to mercurial changeset while those revs are also mutable, and\nthere exists immutable references: hex.\n\nTo sum up, the whole idea is, why would we use unsafe mutable marks\nwhen we can use safer immutable references ?\n\nCheers,\nAntoine\n"},{"id":"213346","messageId":"1365267871-2904-1-git-send-email-apelisse@gmail.com","threadId":"32184","inReplyTo":"CALWbr2x4aia4DcdnmfEEBsZwCYasTEp2Jc0jwJgvsUqWSDaWTQ@mail.gmail.com","subject":"[PATCH] fast-export: Allow pruned-references in mark file","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-04-06T17:04:31Z","receivedAt":"2013-04-06T17:04:31Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"fast-export can fail because of some pruned-reference when importing a\nmark file.\n\nThe problem happens in the following scenario:\n\n $ git fast-export --export-marks=MARKS master\n (rewrite master)\n $ git prune\n $ git fast-export --import-marks=MARKS master\n\nThis might fail if some references have been removed by prune\nbecause some marks will refer to no longer existing commits.\ngit-fast-export will not need these objects anyway as they were no\nlonger reachable.\n\nWe still need to update last_numid so we don't change the mapping\nbetween marks and objects for remote-helpers.\nUnfortunately, the mark file should not be rewritten without lost marks\nif no new objects has been exported, as we could lose track of the last\nlast_numid.\n\nSigned-off-by: Antoine Pelisse \n---\n Documentation/git-fast-export.txt | 2 ++\n builtin/fast-export.c | 11 +++++++----\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt\nindex d6487e1..feab7a3 100644\n--- a/Documentation/git-fast-export.txt\n+++ b/Documentation/git-fast-export.txt\n@@ -66,6 +66,8 @@ produced incorrect results if you gave these options.\n \tincremental runs. As is only opened and truncated\n \tat completion, the same path can also be safely given to\n \t\\--import-marks.\n+\tThe file will not be written if no new object has been\n+\tmarked/exported.\n \n --import-marks=::\n \tBefore processing any input, load the marks specified in\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d380155..f44b76c 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -618,9 +618,12 @@ static void import_marks(char *input_file)\n \t\t\t|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n \t\t\tdie(\"corrupt mark line: %s\", line);\n \n+\t\tif (last_idnum < mark)\n+\t\t\tlast_idnum = mark;\n+\n \t\tobject = parse_object(sha1);\n \t\tif (!object)\n-\t\t\tdie (\"Could not read blob %s\", sha1_to_hex(sha1));\n+\t\t\tcontinue;\n \n \t\tif (object->flags & SHOWN)\n \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n@@ -630,8 +633,6 @@ static void import_marks(char *input_file)\n \t\t\tcontinue;\n \n \t\tmark_object(object, mark);\n-\t\tif (last_idnum < mark)\n-\t\t\tlast_idnum = mark;\n \n \t\tobject->flags |= SHOWN;\n \t}\n@@ -645,6 +646,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tstruct string_list extra_refs = STRING_LIST_INIT_NODUP;\n \tstruct commit *commit;\n \tchar *export_filename = NULL, *import_filename = NULL;\n+\tuint32_t lastimportid;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"progress\", &progress,\n \t\t\t N_(\"show progress after objects\")),\n@@ -688,6 +690,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \tif (import_filename)\n \t\timport_marks(import_filename);\n+\tlastimportid = last_idnum;\n \n \tif (import_filename && revs.prune_data.nr)\n \t\tfull_tree = 1;\n@@ -710,7 +713,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \thandle_tags_and_duplicates(&extra_refs);\n \n-\tif (export_filename)\n+\tif (export_filename && lastimportid != last_idnum)\n \t\texport_marks(export_filename);\n \n \tif (use_done_feature)\n-- \n1.7.9.5\n"},{"id":"213364","messageId":"CAMP44s1n0ALFeOvYevT919iCUt_qRP-nUZn9VqNNnqLG-Xaa1Q@mail.gmail.com","threadId":"32184","inReplyTo":"1365267871-2904-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] fast-export: Allow pruned-references in mark file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-06T17:33:23Z","receivedAt":"2013-04-06T17:33:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 6, 2013 at 11:04 AM, Antoine Pelisse wrote:\n> fast-export can fail because of some pruned-reference when importing a\n> mark file.\n>\n> The problem happens in the following scenario:\n>\n> $ git fast-export --export-marks=MARKS master\n> (rewrite master)\n> $ git prune\n> $ git fast-export --import-marks=MARKS master\n>\n> This might fail if some references have been removed by prune\n> because some marks will refer to no longer existing commits.\n> git-fast-export will not need these objects anyway as they were no\n> longer reachable.\n>\n> We still need to update last_numid so we don't change the mapping\n> between marks and objects for remote-helpers.\n> Unfortunately, the mark file should not be rewritten without lost marks\n> if no new objects has been exported, as we could lose track of the last\n> last_numid.\n\nMakes sense to me.\n\nReviewed-by: Felipe Contreras \n\n-- \nFelipe Contreras\n"}]}