Re: [PATCH] fast-import: do not truncate exported marks file

From: Junio C Hamano <gitster@pobox.com>
Date: May 17, 2016, 22:22 UTC
Message-ID: <xmqq1t50uxy1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1463521223-14565-1-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Certain lines of the marks file might be corrupted (or the objects
> missing due to a garbage collection), but that's no reason to truncate
> the file and essentially destroy the rest of it.

Hmm, so the issue is:

 - we use die_nicely() that calls dump_marks() after writing a crash
   report as our die_routine().

 - when dump_marks() is called, and export_marks_file names an
   existing file, it tries to write marks in it.  If we let it go
   through, we would end up writing a new marks file based on an
   incomplete set of marks we have only half-read in the earlier
   step, which is bad, as the resulting one is incomplete, and the
   original one that this replaced may have been a good one.

Is that what this change addresses?

I am just wondering if a solution to preserve both files is more desirable.

This change looks a bit over-eager to discard the dump die_nicely() is trying to create in one scenario, and a bit less careful at the same time in another scenario.

 - Even if we are reading from somewhere, export_marks_file can
   point at a completely new file that is different from
   import_marks file, in which case, we are not really losing any
   information by freshly creating a new marks file, no?

 - Even if we did not read from any existing marks file, if we are
   given export_marks_file that names an existing file, wouldn't we
   want to avoid corrupting it with a dump from this aborted run?

In other words, I understand the intent of "import-marks-file-done", but I am not sure if that is so special a case.

If the patch were to tell dump_marks() if the caller is die_nicely() or not, and stop it from overwriting an existing file, whether we read from any import-marks file, I would agree with the change a lot more strongly. An obvious extension of that line of thought would be to export to an alternative marks file, perhaps to strbuf_addf("%s.crash", exports_marks_file), if exports_marks_file already exists if we are called while dying.