[PATCH 0/1] Be careful about left-over files from git add --edit runs

[PATCH 0/1] Be careful about left-over files from git add --edit runs

[Johannes Schindelin via GitGitGadget]

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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index f65c172299..53c18ea429 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -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

Re: [PATCH 0/1] Be careful about left-over files from git add --edit runs

[Junio C Hamano]

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

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

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

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

[Junio C Hamano]

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

Re: [PATCH 0/1] Be careful about left-over files from git add --edit runs

[Johannes Schindelin]

Hi Junio,

On Tue, 15 Jan 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> 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.

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

Ciao,
Dscho

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