{"thread":{"id":"42361","subject":"[PATCH] fast-import: do not truncate exported marks file","startedAt":"2016-05-17T21:40:23Z","lastAt":"2016-05-18T05:02:04Z","messageCount":6,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286837","messageId":"1463521223-14565-1-git-send-email-felipe.contreras@gmail.com","threadId":"42361","inReplyTo":null,"subject":"[PATCH] fast-import: do not truncate exported marks file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2016-05-17T21:40:23Z","receivedAt":"2016-05-17T21:40:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Certain lines of the marks file might be corrupted (or the objects\nmissing due to a garbage collection), but that's no reason to truncate\nthe file and essentially destroy the rest of it.\n\nIdeally missing objects should not cause a crash, we could just skip\nthem, but that's another patch.\n\nSigned-off-by: Felipe Contreras \n---\n fast-import.c | 7 +++++--\n t/t9300-fast-import.sh | 15 +++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 9fc7093..a975c34 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -329,6 +329,7 @@ static const char *export_marks_file;\n static const char *import_marks_file;\n static int import_marks_file_from_stream;\n static int import_marks_file_ignore_missing;\n+static int import_marks_file_done;\n static int relative_marks_paths;\n \n /* Our last blob */\n@@ -1802,7 +1803,7 @@ static void dump_marks(void)\n \tstatic struct lock_file mark_lock;\n \tFILE *f;\n \n-\tif (!export_marks_file)\n+\tif (!export_marks_file || (import_marks_file && !import_marks_file_done))\n \t\treturn;\n \n \tif (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {\n@@ -1835,7 +1836,7 @@ static void read_marks(void)\n \tif (f)\n \t\t;\n \telse if (import_marks_file_ignore_missing && errno == ENOENT)\n-\t\treturn; /* Marks file does not exist */\n+\t\tgoto done; /* Marks file does not exist */\n \telse\n \t\tdie_errno(\"cannot read '%s'\", import_marks_file);\n \twhile (fgets(line, sizeof(line), f)) {\n@@ -1865,6 +1866,8 @@ static void read_marks(void)\n \t\tinsert_mark(mark, e);\n \t}\n \tfclose(f);\n+done:\n+\timport_marks_file_done = 1;\n }\n \n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 25bb60b..4bca35c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2650,6 +2650,21 @@ test_expect_success 'R: ignore non-git options' '\n \tgit fast-import expect <<-EOF &&\n+\t:3 0000000000000000000000000000000000000000\n+\t:1 $blob\n+\t:2 $blob\n+\tEOF\n+\tcp expect io.marks &&\n+\ttest_must_fail git fast-import --import-marks=io.marks --export-marks=io.marks <<-\\EOF &&\n+\n+\tEOF\n+\ttest_cmp expect io.marks\n+'\n+\n ##\n ## R: very large blobs\n ##\n-- \n2.8.2.dirty\n"},{"id":"286840","messageId":"xmqq1t50uxy1.fsf@gitster.mtv.corp.google.com","threadId":"42361","inReplyTo":"1463521223-14565-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] fast-import: do not truncate exported marks file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T22:22:30Z","receivedAt":"2016-05-17T22:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Certain lines of the marks file might be corrupted (or the objects\n> missing due to a garbage collection), but that's no reason to truncate\n> the file and essentially destroy the rest of it.\n\nHmm, so the issue is:\n\n - we use die_nicely() that calls dump_marks() after writing a crash\n report as our die_routine().\n\n - when dump_marks() is called, and export_marks_file names an\n existing file, it tries to write marks in it. If we let it go\n through, we would end up writing a new marks file based on an\n incomplete set of marks we have only half-read in the earlier\n step, which is bad, as the resulting one is incomplete, and the\n original one that this replaced may have been a good one.\n\nIs that what this change addresses?\n\nI am just wondering if a solution to preserve both files is more\ndesirable.\n\nThis change looks a bit over-eager to discard the dump die_nicely()\nis trying to create in one scenario, and a bit less careful at the\nsame time in another scenario.\n\n - Even if we are reading from somewhere, export_marks_file can\n point at a completely new file that is different from\n import_marks file, in which case, we are not really losing any\n information by freshly creating a new marks file, no?\n\n - Even if we did not read from any existing marks file, if we are\n given export_marks_file that names an existing file, wouldn't we\n want to avoid corrupting it with a dump from this aborted run?\n\nIn other words, I understand the intent of \"import-marks-file-done\",\nbut I am not sure if that is so special a case.\n\nIf the patch were to tell dump_marks() if the caller is die_nicely()\nor not, and stop it from overwriting an existing file, whether we\nread from any import-marks file, I would agree with the change a lot\nmore strongly. An obvious extension of that line of thought would\nbe to export to an alternative marks file, perhaps to\nstrbuf_addf(\"%s.crash\", exports_marks_file), if exports_marks_file\nalready exists if we are called while dying.\n"},{"id":"286850","messageId":"CAMP44s1WDzRAFV8iNj_RYiiRwJdBcuUDVR-Ew5FF37qdpEO5Mg@mail.gmail.com","threadId":"42361","inReplyTo":"xmqq1t50uxy1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fast-import: do not truncate exported marks file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2016-05-18T03:31:10Z","receivedAt":"2016-05-18T03:31:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Certain lines of the marks file might be corrupted (or the objects\n>> missing due to a garbage collection), but that's no reason to truncate\n>> the file and essentially destroy the rest of it.\n>\n> Hmm, so the issue is:\n>\n> - we use die_nicely() that calls dump_marks() after writing a crash\n> report as our die_routine().\n>\n> - when dump_marks() is called, and export_marks_file names an\n> existing file, it tries to write marks in it. If we let it go\n> through, we would end up writing a new marks file based on an\n> incomplete set of marks we have only half-read in the earlier\n> step, which is bad, as the resulting one is incomplete, and the\n> original one that this replaced may have been a good one.\n>\n> Is that what this change addresses?\n\nYes. As I said; the marks file gets truncated.\n\n> I am just wondering if a solution to preserve both files is more\n> desirable.\n>\n> This change looks a bit over-eager to discard the dump die_nicely()\n> is trying to create in one scenario, and a bit less careful at the\n> same time in another scenario.\n>\n> - Even if we are reading from somewhere, export_marks_file can\n> point at a completely new file that is different from\n> import_marks file, in which case, we are not really losing any\n> information by freshly creating a new marks file, no?\n\nRight, we are not losing any information, but we are not gaining much\neither: it's a truncated version of the import marks.\n\n> - Even if we did not read from any existing marks file, if we are\n> given export_marks_file that names an existing file, wouldn't we\n> want to avoid corrupting it with a dump from this aborted run?\n\nIf we don't run from an existing marks file, this patch has no effect.\n\n-- \nFelipe Contreras\n"},{"id":"286852","messageId":"CAPc5daVJCfnEfA1sHrAGsXaA-80kFV4_4Hd0tLOMocE+qVV=-Q@mail.gmail.com","threadId":"42361","inReplyTo":"CAMP44s1WDzRAFV8iNj_RYiiRwJdBcuUDVR-Ew5FF37qdpEO5Mg@mail.gmail.com","subject":"Re: [PATCH] fast-import: do not truncate exported marks file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T03:59:47Z","receivedAt":"2016-05-18T03:59:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras\n wrote:\n> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>> - Even if we are reading from somewhere, export_marks_file can\n>> point at a completely new file that is different from\n>> import_marks file, in which case, we are not really losing any\n>> information by freshly creating a new marks file, no?\n>\n> Right, we are not losing any information, but we are not gaining much\n> either: it's a truncated version of the import marks.\n\nAh, if we finished reading from the marks file and then die elsewhere,\nthe new conditional that says \"don't clobber if we haven't finished reading\"\nwould not have any effect, right. We'll see the imported ones plus some\nof the new ones.\n\nAnd if we didn't read anything but have export defined, then we can overwrite\nthe existing marks file, but that is nothing new.\n\n>> - Even if we did not read from any existing marks file, if we are\n>> given export_marks_file that names an existing file, wouldn't we\n>> want to avoid corrupting it with a dump from this aborted run?\n>\n> If we don't run from an existing marks file, this patch has no effect.\n\nYes, that is exactly what I was wondering if we may want to improve\nwhile at it.\n"},{"id":"286853","messageId":"CAMP44s06+V8_PvYA=wsqZ625hrMPLKuZ6v5eOmLBO97kTqwxvw@mail.gmail.com","threadId":"42361","inReplyTo":"CAPc5daVJCfnEfA1sHrAGsXaA-80kFV4_4Hd0tLOMocE+qVV=-Q@mail.gmail.com","subject":"Re: [PATCH] fast-import: do not truncate exported marks file","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2016-05-18T04:03:41Z","receivedAt":"2016-05-18T04:03:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 17, 2016 at 10:59 PM, Junio C Hamano wrote:\n> On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras\n> wrote:\n>> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano wrote:\n\n>>> - Even if we did not read from any existing marks file, if we are\n>>> given export_marks_file that names an existing file, wouldn't we\n>>> want to avoid corrupting it with a dump from this aborted run?\n>>\n>> If we don't run from an existing marks file, this patch has no effect.\n>\n> Yes, that is exactly what I was wondering if we may want to improve\n> while at it.\n\nThis doesn't make much sense. Corrupted from where? This patch is\ntackling the issue where the imported marks file is \"corrupted\".\n\n-- \nFelipe Contreras\n"},{"id":"286855","messageId":"xmqqoa84t0vn.fsf@gitster.mtv.corp.google.com","threadId":"42361","inReplyTo":"CAMP44s06+V8_PvYA=wsqZ625hrMPLKuZ6v5eOmLBO97kTqwxvw@mail.gmail.com","subject":"Re: [PATCH] fast-import: do not truncate exported marks file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T05:02:04Z","receivedAt":"2016-05-18T05:02:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Tue, May 17, 2016 at 10:59 PM, Junio C Hamano wrote:\n>> On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras\n>> wrote:\n>>> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano wrote:\n>\n>>>> - Even if we did not read from any existing marks file, if we are\n>>>> given export_marks_file that names an existing file, wouldn't we\n>>>> want to avoid corrupting it with a dump from this aborted run?\n>>>\n>>> If we don't run from an existing marks file, this patch has no effect.\n>>\n>> Yes, that is exactly what I was wondering if we may want to improve\n>> while at it.\n>\n> This doesn't make much sense. Corrupted from where? This patch is\n> tackling the issue where the imported marks file is \"corrupted\".\n\ns/corrupted/overwritten by garbage/ is what I really meant, but in\nany case, I do not think there is any valid use case that relied on\nthe current behaviour that will be broken by this update. I do not\nthink a script that used separate import and export marks files (and\nused mv to rename the exported one to the import used by the next\nround, only after seeing fast-import successfully exits) would be\nharmed, as a non-zero status is still a non-exit status and it\nwouldn't have moved a corrupt export file to the next import, for\nexample. Other people and real users of fast-import might find\ncases I couldn't think of that this change negatively affects, but\nthat's expected for any change and that's why we cook any changes in\n'pu' and then 'next'.\n\nSo let's queue this as a strict improvement.\n\nThanks.\n"}]}