{"thread":{"id":"15321","subject":"[PATCH 1/1] fast-import: show a warning for non-existent files.","startedAt":"2008-09-01T13:20:14Z","lastAt":"2008-09-02T07:57:51Z","messageCount":17,"participants":["Felipe Contreras","Junio C Hamano","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"89417","messageId":"1220275214-6178-1-git-send-email-felipe.contreras@gmail.com","threadId":"15321","inReplyTo":null,"subject":"[PATCH 1/1] fast-import: show a warning for non-existent files.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T13:20:14Z","receivedAt":"2008-09-01T13:20:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This is useful in certain SCMs like monotone, where each 'merge revision' has\nthe changes of all the micro-branches merged. So it appears as duplicated commands.\n\nThe delete command was ignoring the issue completely. The rename/copy commands\nwhere throwing a fatal exception.\n---\n fast-import.c | 14 ++++++++++++--\n 1 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 7089e6f..3dd2ab6 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1945,6 +1945,7 @@ static void file_change_d(struct branch *b)\n \tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tconst char *endp;\n+\tstruct tree_entry leaf;\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)\n \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n \t\tp = uq.buf;\n \t}\n-\ttree_content_remove(&b->branch_tree, p, NULL);\n+\tmemset(&leaf, 0, sizeof(leaf));\n+\ttree_content_remove(&b->branch_tree, p, &leaf);\n+\tif (!leaf.versions[1].mode)\n+\t{\n+\t\twarning(\"Path %s not in branch\", p);\n+\t\treturn;\n+\t}\n }\n \n static void file_change_cr(struct branch *b, int rename)\n@@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)\n \telse\n \t\ttree_content_get(&b->branch_tree, s, &leaf);\n \tif (!leaf.versions[1].mode)\n-\t\tdie(\"Path %s not in branch\", s);\n+\t{\n+\t\twarning(\"Path %s not in branch\", s);\n+\t\treturn;\n+\t}\n \ttree_content_set(&b->branch_tree, d,\n \t\tleaf.versions[1].sha1,\n \t\tleaf.versions[1].mode,\n-- \n1.6.0.1\n"},{"id":"89431","messageId":"7vwshvvgap.fsf@gitster.siamese.dyndns.org","threadId":"15321","inReplyTo":"1220275214-6178-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/1] fast-import: show a warning for non-existent files.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-01T19:04:30Z","receivedAt":"2008-09-01T19:04:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> This is useful in certain SCMs like monotone, where each 'merge revision' has\n> the changes of all the micro-branches merged. So it appears as duplicated commands.\n\nThe patch appears to add warning to when you try to 'D'elete something\nthat should not exist in the revision, whose moral equivalents are\nimplemented in the codepath to deal with 'R'enaming and 'C'opying an\nnon-existent path.\n\nBut instead of making it die(), it merely warns, and even worse, you are\ndemoting an existing die() in Rename/Copy codepath to mere warning\nunconditionally. Why?\n\n\"This\" that begins your proposed commit log message needs to be clarified,\nbut I am guessing that you are defending your change to demote existing\nerror check to die on inconsistent input to a mere warning. I do not find\nit a particularly good defending argument. It sounds more like you are\npapering over bugs in _one_ broken converter that produces and feeds an\nincorrect input to fast-import, breaking a safety valve for everybody else.\n\n> The delete command was ignoring the issue completely. The rename/copy commands\n> where throwing a fatal exception.\n\nYes. I think this has to be two patch series, that:\n\n (1) Adds the equivalent \"you cannot delete what you do not have\" die() in\n the delete codepath; and\n\n (2) Adds an option that demotes *all* the \"don't touch (rename, modify,\n copy or delete) what you do not have\" die()s to warning().\n\nprovided if we can give a good rationale to the latter. Otherwise we\nshould just do (1) and forget about (2).\n"},{"id":"89434","messageId":"20080901192506.GE7482@spearce.org","threadId":"15321","inReplyTo":"1220275214-6178-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/1] fast-import: show a warning for non-existent files.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-01T19:25:06Z","receivedAt":"2008-09-01T19:25:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Felipe Contreras wrote:\n> This is useful in certain SCMs like monotone, where each 'merge revision' has\n> the changes of all the micro-branches merged. So it appears as duplicated commands.\n> \n> The delete command was ignoring the issue completely. The rename/copy commands\n> where throwing a fatal exception.\n\nSigned-off-by line? See Documentation/SubmittingPatches.\n\n> diff --git a/fast-import.c b/fast-import.c\n> index 7089e6f..3dd2ab6 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)\n> \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n> \t\tp = uq.buf;\n> \t}\n> -\ttree_content_remove(&b->branch_tree, p, NULL);\n> +\tmemset(&leaf, 0, sizeof(leaf));\n> +\ttree_content_remove(&b->branch_tree, p, &leaf);\n> +\tif (!leaf.versions[1].mode)\n> +\t{\n> +\t\twarning(\"Path %s not in branch\", p);\n> +\t\treturn;\n> +\t}\n> }\n> \n> static void file_change_cr(struct branch *b, int rename)\n\nThis is going to leak memory unless you add this before the\nif (..mode) condition:\n\n\tif (leaf->tree)\n\t\trelease_tree_content_recursive(e->tree);\n\nWe didn't worry about deleting a path that doesn't exist because\nthe importer clearly wants it gone. If it wants it gone and it is\nalready gone then it should be fine to ignore the delete command.\n\nBut as I point out below some import front-ends should be accurate\nenough that they should not send a 'D' command unless the path is\nalready in the tree. Thus this can be an error condition for some\ntypes of frontends, but can be ignored for others.\n\n> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)\n> \telse\n> \t\ttree_content_get(&b->branch_tree, s, &leaf);\n> \tif (!leaf.versions[1].mode)\n> -\t\tdie(\"Path %s not in branch\", s);\n> +\t{\n> +\t\twarning(\"Path %s not in branch\", s);\n> +\t\treturn;\n> +\t}\n> \ttree_content_set(&b->branch_tree, d,\n> \t\tleaf.versions[1].sha1,\n> \t\tleaf.versions[1].mode,\n\nNormally we consider invalid paths to be an error. I wonder if this\nshould still be an error, unless the front-end passes an option on\nthe command line. Then monotone based importers can make these\nwarnings, but other importers that don't have this problem can\nstill treat them what they are, which is a fatal error.\n\nDid you run the test suite (t/t9300-fast-import.sh) after your patch?\nI would have thought a few of the bad path errors should be caught\nthere.\n\n-- \nShawn.\n"},{"id":"89448","messageId":"94a0d4530809011458r25f73f49web2c3ae601a5544d@mail.gmail.com","threadId":"15321","inReplyTo":"7vwshvvgap.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/1] fast-import: show a warning for non-existent files.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T21:58:59Z","receivedAt":"2008-09-01T21:58:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Sep 1, 2008 at 10:04 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> This is useful in certain SCMs like monotone, where each 'merge revision' has\n>> the changes of all the micro-branches merged. So it appears as duplicated commands.\n>\n> The patch appears to add warning to when you try to 'D'elete something\n> that should not exist in the revision, whose moral equivalents are\n> implemented in the codepath to deal with 'R'enaming and 'C'opying an\n> non-existent path.\n>\n> But instead of making it die(), it merely warns, and even worse, you are\n> demoting an existing die() in Rename/Copy codepath to mere warning\n> unconditionally. Why?\n\nRight, I tried to do two things at once. I thought adding the code in\n'D'elete in a separate branch was too much separation for such a\ntrivial thing.\n\n> \"This\" that begins your proposed commit log message needs to be clarified,\n> but I am guessing that you are defending your change to demote existing\n> error check to die on inconsistent input to a mere warning. I do not find\n> it a particularly good defending argument. It sounds more like you are\n> papering over bugs in _one_ broken converter that produces and feeds an\n> incorrect input to fast-import, breaking a safety valve for everybody else.\n\nIn monotone you can have something like:\n\n A\n/ \\\nB D\n| |\nC E\n\\ /\n F\n\nIf you do a 'delete foo' in B, and a 'delete bar' in F, you will get\nthis stored in the merge revision (F):\n\nold_revision [C]\ndelete \"foo\"\ndelete \"bar\"\n\nold_revision [E]\ndelete \"bar\"\n\nAll the changes of the merged branches are stored again in the merge revision.\n\nI guess ideally my converter should check which changes appear on both\nbranches, and only apply those. Of course, only on revisions that are\nmerges.\nThat complicates the code quite a lot. I tried it and it didn't work\nquite well (I did something wrong).\n\nSo I tried to workaround this in fast-import, and it's simple, and it works.\n\n\n\n-- \nFelipe Contreras\n"},{"id":"89449","messageId":"94a0d4530809011501n651c42c8xebb5cc39a93aac4c@mail.gmail.com","threadId":"15321","inReplyTo":"20080901192506.GE7482@spearce.org","subject":"Re: [PATCH 1/1] fast-import: show a warning for non-existent files.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T22:01:52Z","receivedAt":"2008-09-01T22:01:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Sep 1, 2008 at 10:25 PM, Shawn O. Pearce wrote:\n> Felipe Contreras wrote:\n>> This is useful in certain SCMs like monotone, where each 'merge revision' has\n>> the changes of all the micro-branches merged. So it appears as duplicated commands.\n>>\n>> The delete command was ignoring the issue completely. The rename/copy commands\n>> where throwing a fatal exception.\n>\n> Signed-off-by line? See Documentation/SubmittingPatches.\n\nAll right, read.\n\n>> diff --git a/fast-import.c b/fast-import.c\n>> index 7089e6f..3dd2ab6 100644\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)\n>> die(\"Garbage after path in: %s\", command_buf.buf);\n>> p = uq.buf;\n>> }\n>> - tree_content_remove(&b->branch_tree, p, NULL);\n>> + memset(&leaf, 0, sizeof(leaf));\n>> + tree_content_remove(&b->branch_tree, p, &leaf);\n>> + if (!leaf.versions[1].mode)\n>> + {\n>> + warning(\"Path %s not in branch\", p);\n>> + return;\n>> + }\n>> }\n>>\n>> static void file_change_cr(struct branch *b, int rename)\n>\n> This is going to leak memory unless you add this before the\n> if (..mode) condition:\n>\n> if (leaf->tree)\n> release_tree_content_recursive(e->tree);\n\nHmm, ok.\n\n> We didn't worry about deleting a path that doesn't exist because\n> the importer clearly wants it gone. If it wants it gone and it is\n> already gone then it should be fine to ignore the delete command.\n>\n> But as I point out below some import front-ends should be accurate\n> enough that they should not send a 'D' command unless the path is\n> already in the tree. Thus this can be an error condition for some\n> types of frontends, but can be ignored for others.\n\nI'm sending the patch again with this behavior as an option.\n\n>> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)\n>> else\n>> tree_content_get(&b->branch_tree, s, &leaf);\n>> if (!leaf.versions[1].mode)\n>> - die(\"Path %s not in branch\", s);\n>> + {\n>> + warning(\"Path %s not in branch\", s);\n>> + return;\n>> + }\n>> tree_content_set(&b->branch_tree, d,\n>> leaf.versions[1].sha1,\n>> leaf.versions[1].mode,\n>\n> Normally we consider invalid paths to be an error. I wonder if this\n> should still be an error, unless the front-end passes an option on\n> the command line. Then monotone based importers can make these\n> warnings, but other importers that don't have this problem can\n> still treat them what they are, which is a fatal error.\n>\n> Did you run the test suite (t/t9300-fast-import.sh) after your patch?\n> I would have thought a few of the bad path errors should be caught\n> there.\n\nI didn't initially, now I just did and it doesn't seem to be checking\nfor such things.\n\nBest regards.\n\n-- \nFelipe Contreras\n"},{"id":"89450","messageId":"1220308173-20392-1-git-send-email-felipe.contreras@gmail.com","threadId":"15321","inReplyTo":"94a0d4530809011501n651c42c8xebb5cc39a93aac4c@mail.gmail.com","subject":"[PATCH] fast-import: add ignore non-existent files option.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T22:30:08Z","receivedAt":"2008-09-01T22:30:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This is useful for SCMs that don't have proper changesets in each\nrevision (monotone).\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/git-fast-import.txt | 4 ++++\n fast-import.c | 14 ++++++++++++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex c2f483a..42e10c1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -95,6 +95,10 @@ OPTIONS\n \tmemory used by fast-import during this run. Showing this output\n \tis currently the default, but can be disabled with \\--quiet.\n \n+--tolerant::\n+\tAvoid fatal exceptions when actions are executed in non-existent\n+\tfiles. For example removing a file that is not there.\n+\n \n Performance\n -----------\ndiff --git a/fast-import.c b/fast-import.c\nindex 7089e6f..01be3fa 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -345,6 +345,7 @@ static struct recent_command *rc_free;\n static unsigned int cmd_save = 100;\n static uintmax_t next_mark;\n static struct strbuf new_data = STRBUF_INIT;\n+static int tolerant;\n \n static void write_branch_report(FILE *rpt, struct branch *b)\n {\n@@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n \t\ttree_content_remove(&b->branch_tree, s, &leaf);\n \telse\n \t\ttree_content_get(&b->branch_tree, s, &leaf);\n-\tif (!leaf.versions[1].mode)\n-\t\tdie(\"Path %s not in branch\", s);\n+\tif (!leaf.versions[1].mode) {\n+\t\tif (tolerant) {\n+\t\t\tif (leaf.tree)\n+\t\t\t\trelease_tree_content_recursive(leaf.tree);\n+\t\t\twarning(\"Path %s not in branch\", s);\n+\t\t\treturn;\n+\t\t} else\n+\t\t\tdie(\"Path %s not in branch\", s);\n+\t}\n \ttree_content_set(&b->branch_tree, d,\n \t\tleaf.versions[1].sha1,\n \t\tleaf.versions[1].mode,\n@@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)\n \t\t\tshow_stats = 0;\n \t\telse if (!strcmp(a, \"--stats\"))\n \t\t\tshow_stats = 1;\n+\t\telse if (!strcmp(a, \"--tolerant\"))\n+\t\t\ttolerant = 1;\n \t\telse\n \t\t\tdie(\"unknown option %s\", a);\n \t}\n-- \n1.6.0.1\n"},{"id":"89451","messageId":"20080901223802.GK7482@spearce.org","threadId":"15321","inReplyTo":"1220308173-20392-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-01T22:38:03Z","receivedAt":"2008-09-01T22:38:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Felipe Contreras wrote:\n> This is useful for SCMs that don't have proper changesets in each\n> revision (monotone).\n \n> +--tolerant::\n> +\tAvoid fatal exceptions when actions are executed in non-existent\n> +\tfiles. For example removing a file that is not there.\n> +\n\nYea. But I'm not sure --tolerant is the best name. --ignore-errors\nor --treat-errors-as-warnings sounds better to me.\n \n> diff --git a/fast-import.c b/fast-import.c\n> index 7089e6f..01be3fa 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -345,6 +345,7 @@ static struct recent_command *rc_free;\n> static unsigned int cmd_save = 100;\n> static uintmax_t next_mark;\n> static struct strbuf new_data = STRBUF_INIT;\n> +static int tolerant;\n> \n> static void write_branch_report(FILE *rpt, struct branch *b)\n> {\n> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n> \t\ttree_content_remove(&b->branch_tree, s, &leaf);\n> \telse\n> \t\ttree_content_get(&b->branch_tree, s, &leaf);\n> -\tif (!leaf.versions[1].mode)\n> -\t\tdie(\"Path %s not in branch\", s);\n> +\tif (!leaf.versions[1].mode) {\n> +\t\tif (tolerant) {\n> +\t\t\tif (leaf.tree)\n> +\t\t\t\trelease_tree_content_recursive(leaf.tree);\n> +\t\t\twarning(\"Path %s not in branch\", s);\n> +\t\t\treturn;\n> +\t\t} else\n> +\t\t\tdie(\"Path %s not in branch\", s);\n> +\t}\n> \ttree_content_set(&b->branch_tree, d,\n> \t\tleaf.versions[1].sha1,\n> \t\tleaf.versions[1].mode,\n> @@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)\n> \t\t\tshow_stats = 0;\n> \t\telse if (!strcmp(a, \"--stats\"))\n> \t\t\tshow_stats = 1;\n> +\t\telse if (!strcmp(a, \"--tolerant\"))\n> +\t\t\ttolerant = 1;\n> \t\telse\n> \t\t\tdie(\"unknown option %s\", a);\n> \t}\n\n-- \nShawn.\n"},{"id":"89453","messageId":"94a0d4530809011552h59e476e0q9033f0b917288a8f@mail.gmail.com","threadId":"15321","inReplyTo":"20080901223802.GK7482@spearce.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T22:52:48Z","receivedAt":"2008-09-01T22:52:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 2, 2008 at 1:38 AM, Shawn O. Pearce wrote:\n> Felipe Contreras wrote:\n>> This is useful for SCMs that don't have proper changesets in each\n>> revision (monotone).\n>\n>> +--tolerant::\n>> + Avoid fatal exceptions when actions are executed in non-existent\n>> + files. For example removing a file that is not there.\n>> +\n>\n> Yea. But I'm not sure --tolerant is the best name. --ignore-errors\n> or --treat-errors-as-warnings sounds better to me.\n\nI initially named it --ignore-non-existent, but I thought the option\nwas too specific.\n\n--ignore-errors or --treat-errors-as-warnings imply all errors. It\nmight make sense to always fail at certain errors, like 'mark not\nfound'. I thought 'relaxed' or 'tolerant' would imply that only some\nerrors will be allowed, not all.\n\n>> diff --git a/fast-import.c b/fast-import.c\n>> index 7089e6f..01be3fa 100644\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -345,6 +345,7 @@ static struct recent_command *rc_free;\n>> static unsigned int cmd_save = 100;\n>> static uintmax_t next_mark;\n>> static struct strbuf new_data = STRBUF_INIT;\n>> +static int tolerant;\n>>\n>> static void write_branch_report(FILE *rpt, struct branch *b)\n>> {\n>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n>> tree_content_remove(&b->branch_tree, s, &leaf);\n>> else\n>> tree_content_get(&b->branch_tree, s, &leaf);\n>> - if (!leaf.versions[1].mode)\n>> - die(\"Path %s not in branch\", s);\n>> + if (!leaf.versions[1].mode) {\n>> + if (tolerant) {\n>> + if (leaf.tree)\n>> + release_tree_content_recursive(leaf.tree);\n>> + warning(\"Path %s not in branch\", s);\n>> + return;\n>> + } else\n>> + die(\"Path %s not in branch\", s);\n>> + }\n>> tree_content_set(&b->branch_tree, d,\n>> leaf.versions[1].sha1,\n>> leaf.versions[1].mode,\n>> @@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)\n>> show_stats = 0;\n>> else if (!strcmp(a, \"--stats\"))\n>> show_stats = 1;\n>> + else if (!strcmp(a, \"--tolerant\"))\n>> + tolerant = 1;\n>> else\n>> die(\"unknown option %s\", a);\n>> }\n>\n> --\n> Shawn.\n>\n\n\n\n-- \nFelipe Contreras\n"},{"id":"89456","messageId":"7v63pftqmk.fsf@gitster.siamese.dyndns.org","threadId":"15321","inReplyTo":"1220308173-20392-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-01T23:04:19Z","receivedAt":"2008-09-01T23:04:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> This is useful for SCMs that don't have proper changesets in each\n> revision (monotone).\n\nI am still not convinced this is a proper workaround for the issue. Why\nshouldn't the feeder of fast-import be able to do this?\n\n> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n> ...\n\nAlso what happened to the missing warning() for 'D'elete codepath?\n"},{"id":"89461","messageId":"94a0d4530809011625n772fdc58h3ce1c04e79fb116f@mail.gmail.com","threadId":"15321","inReplyTo":"7v63pftqmk.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-01T23:25:14Z","receivedAt":"2008-09-01T23:25:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> This is useful for SCMs that don't have proper changesets in each\n>> revision (monotone).\n>\n> I am still not convinced this is a proper workaround for the issue. Why\n> shouldn't the feeder of fast-import be able to do this?\n\nIf I could get a list of the files that changed on each revision from\nmonotone I would, but that's not possible (I've asked in their mailing\nlist). Apparently there's a way to feed the right data, but it's\ncomplicated.\n\nJust to give you a sense. Right now I'm simply parsing all the changes\nin the revision and converting them to fast-import's format. I don't\nneed to check if the revision has multiple parents, or not.\n\nNow, in order to feed the right data I would have to find the\nold_revision markers, use a hash table to store the file changes per\nrevision, after the parsing find out if there's more than one parent.\nIf there's more than one parent then find out the duplicate actions\nthat are in all the parents and store them in a new list of actions.\nIf there's only one parent then simply use the list of actions of that\nparent.\n\nI can spend some time trying to get that working, but a) this\nworkaround is much simpler, and b) I'm thinking on writing the final\nconverter in C. I really don't want to mess with all this complexity\nunless I absolutely must.\n\nIf you are interested in the current code: http://pastie.org/264209.\n\n>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n>> ...\n>\n> Also what happened to the missing warning() for 'D'elete codepath?\n\nI'm not interested in it.\n\nI added it because I thought it was missing. Since apparently it's\nmore important to don't alter the old behaviour I prefer to scratch my\nown itch and focus on what I need.\n\n-- \nFelipe Contreras\n"},{"id":"89473","messageId":"7vk5dvs3k8.fsf@gitster.siamese.dyndns.org","threadId":"15321","inReplyTo":"94a0d4530809011625n772fdc58h3ce1c04e79fb116f@mail.gmail.com","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T02:07:51Z","receivedAt":"2008-09-02T02:07:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Felipe Contreras\" writes:\n\n> On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>> This is useful for SCMs that don't have proper changesets in each\n>>> revision (monotone).\n>>\n>> I am still not convinced this is a proper workaround for the issue. Why\n>> shouldn't the feeder of fast-import be able to do this?\n>\n> If I could get a list of the files that changed on each revision from\n> monotone I would, but that's not possible (I've asked in their mailing\n> list). Apparently there's a way to feed the right data, but it's\n> complicated.\n\nOk, I did not realize that you are not keeping track of what the parents'\ntrees look like when processing a merge commit.\n\nBut it raises a bigger question. You earlier said:\n\n In monotone you can have something like:\n\n A\n / \\\n B D\n | |\n C E\n \\ /\n F\n\n If you do a 'delete foo' in B, and a 'delete bar' in F, you will get\n this stored in the merge revision (F):\n\n old_revision [C]\n delete \"foo\"\n delete \"bar\"\n\n old_revision [E]\n delete \"bar\"\n\n All the changes of the merged branches are stored again in the merge revision.\n\nNow, does it talk about C and E because they are immediate parents of B\nand D, or because they are immediate child of the common ancestor F? I am\nguessing it is the latter (what I mean is if it still talks aobut C and E\nif you had more intermediate commits between B and C). What the merge at\nA records is not relative to B/D but relative to immediate child of the\nfork point.\n\nIf that is the case, ignoring a delete that deletes already deleted path\nis fine; I think it is the least of your problem. The above description\nmakes me wonder what they say about modification. Do they talk about the\nsame modification that happened between B and C when talking about A? If\nthat is the case, it would be a far larger problem. You cannot just say\n\"ignore any modification recorded in a merge, because they have been\nalready done on the branches being merged (i.e. up to B and up to D)\", as\nA may have to be an evil merge.\n\nI have to wonder if you can \"mark\" the tree object of C, and when you\nprocess merge A, represent A's tree by starting from that marked tree,\napplying only the description to bring the state of \"old_revision [C]\"\nto that of A (delete \"foo\" and delete \"bar\" in your illustration), and\nrecording that tree with parents B and D.\n\n>>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n>>> ...\n>>\n>> Also what happened to the missing warning() for 'D'elete codepath?\n>\n> I'm not interested in it.\n\nIf I were asking for an unrelated \"feature\" when you are developing some\nother feature, it would have been different, but I do not think that is a\ngood answer in this particular case.\n\nYour --tolerant (or --ignore-errors) is about customizing strictness of\nthe error handling, and you know of a case where the error handling is not\nstrict enough in the existing code. In other words, your \"not interested\"\nis _not_ saying \"It is an unrelated feature that I am not interested in\";\nit is saying \"I am not interested in making my patch self consistent; a\nhalf-baked hack that happens to solve my case is good enough for me.\"\n"},{"id":"89476","messageId":"20080902043951.GB13248@spearce.org","threadId":"15321","inReplyTo":"94a0d4530809011552h59e476e0q9033f0b917288a8f@mail.gmail.com","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-02T04:39:51Z","receivedAt":"2008-09-02T04:39:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, Sep 2, 2008 at 1:38 AM, Shawn O. Pearce wrote:\n> > Felipe Contreras wrote:\n> >> This is useful for SCMs that don't have proper changesets in each\n> >> revision (monotone).\n> >\n> >> +--tolerant::\n> >> + Avoid fatal exceptions when actions are executed in non-existent\n> >> + files. For example removing a file that is not there.\n> >> +\n> >\n> > Yea. But I'm not sure --tolerant is the best name. --ignore-errors\n> > or --treat-errors-as-warnings sounds better to me.\n> \n> I initially named it --ignore-non-existent, but I thought the option\n> was too specific.\n> \n> --ignore-errors or --treat-errors-as-warnings imply all errors. It\n> might make sense to always fail at certain errors, like 'mark not\n> found'. I thought 'relaxed' or 'tolerant' would imply that only some\n> errors will be allowed, not all.\n\nOK, that argument makes sense. Then I wonder if more specific\nerror ignoring would be better:\n\n --ignore-error=already-deleted\n --ignore-error=already-deleted,missing-mark,missing-copy-source\n\nI'm not really fond of turning an existing error condition that\nexists to catch broken frontends into a generic tolerant flag.\nBut being able to selectively turn it off while leaving other\nerrors as errors isn't entirely unreasonable.\n\n-- \nShawn.\n"},{"id":"89477","messageId":"7vfxojrvvp.fsf@gitster.siamese.dyndns.org","threadId":"15321","inReplyTo":"20080902043951.GB13248@spearce.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T04:53:46Z","receivedAt":"2008-09-02T04:53:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> OK, that argument makes sense. Then I wonder if more specific\n> error ignoring would be better:\n>\n> --ignore-error=already-deleted\n> --ignore-error=already-deleted,missing-mark,missing-copy-source\n>\n> I'm not really fond of turning an existing error condition that\n> exists to catch broken frontends into a generic tolerant flag.\n> But being able to selectively turn it off while leaving other\n> errors as errors isn't entirely unreasonable.\n\nI think selective loosening of consistency check makes sense very much,\nbut I have been wondering if these should be command line options.\n\nThe only example we saw so far is about output from one exporter. Perhaps\nit should be given to fast-import as initial set of commands (\"#pragma\"!)\nthat describes the nature of the input file?\n"},{"id":"89478","messageId":"20080902053503.GD13248@spearce.org","threadId":"15321","inReplyTo":"7vfxojrvvp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-02T05:35:03Z","receivedAt":"2008-09-02T05:35:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > OK, that argument makes sense. Then I wonder if more specific\n> > error ignoring would be better:\n> >\n> > --ignore-error=already-deleted\n> > --ignore-error=already-deleted,missing-mark,missing-copy-source\n> >\n> > I'm not really fond of turning an existing error condition that\n> > exists to catch broken frontends into a generic tolerant flag.\n> > But being able to selectively turn it off while leaving other\n> > errors as errors isn't entirely unreasonable.\n> \n> I think selective loosening of consistency check makes sense very much,\n> but I have been wondering if these should be command line options.\n> \n> The only example we saw so far is about output from one exporter. Perhaps\n> it should be given to fast-import as initial set of commands (\"#pragma\"!)\n> that describes the nature of the input file?\n\nYea, I briefly considered that when I added the timestamp format\noption. I didn't bother because it was a single option and I figured\nmost frontends start git-fast-import directly. But with this being\nadded a \"format pragrma header thingy\" makes a lot of sense.\n\nSince comments are supported we could backdoor it with #pragma or\nsomething like that, so existing files can still be (mostly) parsed\nby an earlier git-fast-import. But I wonder if that is wise given\nthat some classes of errors would still fail on the older import,\nbut would work on a newer one. Might as well just make it a proper\ncommand that would cause an older importer to fail out of the gate.\n\n-- \nShawn.\n"},{"id":"89494","messageId":"7v8wubm22y.fsf@gitster.siamese.dyndns.org","threadId":"15321","inReplyTo":"20080902053503.GD13248@spearce.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T07:36:21Z","receivedAt":"2008-09-02T07:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Junio C Hamano wrote:\n> ...\n>> I think selective loosening of consistency check makes sense very much,\n>> but I have been wondering if these should be command line options.\n>> \n>> The only example we saw so far is about output from one exporter. Perhaps\n>> it should be given to fast-import as initial set of commands (\"#pragma\"!)\n>> that describes the nature of the input file?\n>\n> Yea, I briefly considered that when I added the timestamp format\n> option. I didn't bother because it was a single option and I figured\n> most frontends start git-fast-import directly. But with this being\n> added a \"format pragrma header thingy\" makes a lot of sense.\n\nOh, I did not mean to suggest hiding it as a comment to silently allow\nolder fast-import slurp such input and produce broken results.\n\nFor input that needs such loosened error checking, old fast-import won't\nproduce correct results _anyway_, so I would agree that making these\nthings into explicit commands to cause older fast-import to error out\nwould make a lot more sense.\n"},{"id":"89495","messageId":"94a0d4530809020048n3894131as351d72d2394ea1a7@mail.gmail.com","threadId":"15321","inReplyTo":"7v8wubm22y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-02T07:48:50Z","receivedAt":"2008-09-02T07:48:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 2, 2008 at 10:36 AM, Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n>\n>> Junio C Hamano wrote:\n>> ...\n>>> I think selective loosening of consistency check makes sense very much,\n>>> but I have been wondering if these should be command line options.\n>>>\n>>> The only example we saw so far is about output from one exporter. Perhaps\n>>> it should be given to fast-import as initial set of commands (\"#pragma\"!)\n>>> that describes the nature of the input file?\n>>\n>> Yea, I briefly considered that when I added the timestamp format\n>> option. I didn't bother because it was a single option and I figured\n>> most frontends start git-fast-import directly. But with this being\n>> added a \"format pragrma header thingy\" makes a lot of sense.\n>\n> Oh, I did not mean to suggest hiding it as a comment to silently allow\n> older fast-import slurp such input and produce broken results.\n>\n> For input that needs such loosened error checking, old fast-import won't\n> produce correct results _anyway_, so I would agree that making these\n> things into explicit commands to cause older fast-import to error out\n> would make a lot more sense.\n\nI like the idea. This way all the information to make it work is\nalready on the stream.\n\nIf the importer doesn't have a certain feature it can fail already at\ninitialization.\n\n-- \nFelipe Contreras\n"},{"id":"89496","messageId":"94a0d4530809020057w3bffd130xdfdcdfb622698d65@mail.gmail.com","threadId":"15321","inReplyTo":"7vk5dvs3k8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: add ignore non-existent files option.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-09-02T07:57:51Z","receivedAt":"2008-09-02T07:57:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Sep 2, 2008 at 5:07 AM, Junio C Hamano wrote:\n> \"Felipe Contreras\" writes:\n>\n>> On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>> This is useful for SCMs that don't have proper changesets in each\n>>>> revision (monotone).\n>>>\n>>> I am still not convinced this is a proper workaround for the issue. Why\n>>> shouldn't the feeder of fast-import be able to do this?\n>>\n>> If I could get a list of the files that changed on each revision from\n>> monotone I would, but that's not possible (I've asked in their mailing\n>> list). Apparently there's a way to feed the right data, but it's\n>> complicated.\n>\n> Ok, I did not realize that you are not keeping track of what the parents'\n> trees look like when processing a merge commit.\n>\n> But it raises a bigger question. You earlier said:\n>\n> In monotone you can have something like:\n>\n> A\n> / \\\n> B D\n> | |\n> C E\n> \\ /\n> F\n>\n> If you do a 'delete foo' in B, and a 'delete bar' in F, you will get\n> this stored in the merge revision (F):\n>\n> old_revision [C]\n> delete \"foo\"\n> delete \"bar\"\n>\n> old_revision [E]\n> delete \"bar\"\n>\n> All the changes of the merged branches are stored again in the merge revision.\n>\n> Now, does it talk about C and E because they are immediate parents of B\n> and D, or because they are immediate child of the common ancestor F? I am\n> guessing it is the latter (what I mean is if it still talks aobut C and E\n> if you had more intermediate commits between B and C). What the merge at\n> A records is not relative to B/D but relative to immediate child of the\n> fork point.\n\nEr, no it's the other way around. A is the original child, F is the\nmerge. I thought it was evident because of the alphabetical ordering.\n\n> If that is the case, ignoring a delete that deletes already deleted path\n> is fine; I think it is the least of your problem. The above description\n> makes me wonder what they say about modification. Do they talk about the\n> same modification that happened between B and C when talking about A? If\n> that is the case, it would be a far larger problem. You cannot just say\n> \"ignore any modification recorded in a merge, because they have been\n> already done on the branches being merged (i.e. up to B and up to D)\", as\n> A may have to be an evil merge.\n>\n> I have to wonder if you can \"mark\" the tree object of C, and when you\n> process merge A, represent A's tree by starting from that marked tree,\n> applying only the description to bring the state of \"old_revision [C]\"\n> to that of A (delete \"foo\" and delete \"bar\" in your illustration), and\n> recording that tree with parents B and D.\n\nI'm not sure I understand correctly, but anyway, how would you propose\nto \"mark\" the tree objects?\n\n>>>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)\n>>>> ...\n>>>\n>>> Also what happened to the missing warning() for 'D'elete codepath?\n>>\n>> I'm not interested in it.\n>\n> If I were asking for an unrelated \"feature\" when you are developing some\n> other feature, it would have been different, but I do not think that is a\n> good answer in this particular case.\n>\n> Your --tolerant (or --ignore-errors) is about customizing strictness of\n> the error handling, and you know of a case where the error handling is not\n> strict enough in the existing code. In other words, your \"not interested\"\n> is _not_ saying \"It is an unrelated feature that I am not interested in\";\n> it is saying \"I am not interested in making my patch self consistent; a\n> half-baked hack that happens to solve my case is good enough for me.\"\n\nHmm, ok.\n\n-- \nFelipe Contreras\n"}]}