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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 18, 2016, 03:31 UTC
Message-ID: <CAMP44s1WDzRAFV8iNj_RYiiRwJdBcuUDVR-Ew5FF37qdpEO5Mg@mail.gmail.com>
In-Reply-To: <xmqq1t50uxy1.fsf@gitster.mtv.corp.google.com>

On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> 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?

Yes. As I said; the marks file gets truncated.

Show 11 quoted lines

> 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?

Right, we are not losing any information, but we are not gaining much either: it's a truncated version of the import marks.

>  - 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?

If we don't run from an existing marks file, this patch has no effect.

-- 
Felipe Contreras