threads / patch / 50230

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

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

## tl;dr

5 messages between Jan 15, 2019 and Jan 16, 2019. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Johannes Schindelin via GitGitGadget· Jan 15, 2019, 15:42 UTC · lore

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
Johannes Schindelin via GitGitGadget· Jan 15, 2019, 15:42 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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(-)
Show changes to builtin/add.c +1 −1
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
Junio C Hamano· Jan 15, 2019, 19:01 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

"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 ;-)
Junio C Hamano· Jan 15, 2019, 18:50 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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

Show 7 quoted lines
> 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.

Show 12 quoted lines
>
> 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
Johannes Schindelin· Jan 16, 2019, 13:47 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Tue, 15 Jan 2019, Junio C Hamano wrote:
Show 7 quoted lines
> "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

Show 14 quoted lines
> 
> >
> > 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
>

← back to recent threads