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

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2016, 05:02 UTC
Message-ID: <xmqqoa84t0vn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAMP44s06+V8_PvYA=wsqZ625hrMPLKuZ6v5eOmLBO97kTqwxvw@mail.gmail.com>

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

Show 16 quoted lines

> On Tue, May 17, 2016 at 10:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>>>  - 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.
>>
>> Yes, that is exactly what I was wondering if we may want to improve
>> while at it.
>
> This doesn't make much sense. Corrupted from where? This patch is
> tackling the issue where the imported marks file is "corrupted".

s/corrupted/overwritten by garbage/ is what I really meant, but in any case, I do not think there is any valid use case that relied on the current behaviour that will be broken by this update. I do not think a script that used separate import and export marks files (and used mv to rename the exported one to the import used by the next round, only after seeing fast-import successfully exits) would be harmed, as a non-zero status is still a non-exit status and it wouldn't have moved a corrupt export file to the next import, for example. Other people and real users of fast-import might find cases I couldn't think of that this change negatively affects, but that's expected for any change and that's why we cook any changes in 'pu' and then 'next'.

So let's queue this as a strict improvement.

Thanks.