Re: [PATCH 1/1] add --edit: truncate the patch file

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2019, 19:01 UTC
Message-ID: <xmqqa7k1sq24.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <52f74c5d356325660ca1b494e78fb99a3c6c3861.1547566970.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Of course, this file should not be left lying around. But at least in
> one case, there was a stale copy, larger than the current diff. So the
> result was a corrupt diff.

After creating the fd (now with O_TRUNC), we die in a few places.

 - Failure to open the file for writing.  This is OK and do not need
   any further clean-up.

 - Failure to write a patch into that file.  In this case, it might
   be worth unlinking the file.

 - Error exit from the editor.  In this case, the user may want to
   take a look and use the contents in it manually, so not removing
   the file, which is what we do right now, may be the right thing.

   On the other hand, such a patch file is easy to recreate with
   "git diff", so it may be worth unlinking the file.

 - Failure from stat() after editing the file.  In this case, it
   might be worth unlinking the file.

So I agree that "this file should not be left lying around" in most of the cases (except a half).

In any case, when we are about to use the file for a newly created patch at the beginning of this function, we know that the old content is no longer relevant, so the change to truncate before using it is absolutely the right thing to do, and makes it moot to ask "should we remove it after an error, or should we leave it to avoid information loss?".

Good ;-)