Be careful about left-over files from git add --edit runs

J Wyman reported almost a year ago (and I fixed this issue in Git for Windows around that time) that the .git/ADD_EDIT.patch file might still lie around at the beginning of git add --edit from previous runs, and if the new patch is smaller than the old one, the resulting diff is obviously corrupt.

This is yet another Git for Windows patch finally making it to the Git mailing list.

Johannes Schindelin (1):
add --edit: truncate the patch file

 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

base-commit: ecbdaf0899161c067986e9d9d564586d4b045d62
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-107%2Fdscho%2Fadd-e-truncate-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-107/dscho/add-e-truncate-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/107

gitgitgadget

From: Johannes Schindelin <[email protected]>

If there is already a .git/ADD_EDIT.patch file, we fail to truncate it properly, which could result in very funny errors.

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.

Let's just truncate the file when we write it and not worry about it too much.

Reported by J Wyman.

---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index f65c172299..53c18ea429 100644
@@ -239,7 +239,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 	rev.diffopt.use_color = 0;
 	rev.diffopt.flags.ignore_dirty_submodules = 1;
-	out = open(file, O_CREAT | O_WRONLY, 0666);
+	out = open(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	if (out < 0)
 		die(_("Could not open '%s' for writing."), file);
 	rev.diffopt.file = xfdopen(out, "w");

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Good, finally ;-).

I didn't realize that "add -e" codepath was so old until today to check with "git blame" to see how old this bug was (it predates the transition from builtin-foo.c to builtin/foo.c); it seems that it was from day one of the feature.

"Johannes Schindelin via GitGitGadget" <[email protected]>

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 ;-)

Hi Junio,

Yes, I introduced that bug right from the start. On the good side: this is *not* an off-by-one bug!

Ciao,
Dscho