# [PATCH 0/1] Be careful about left-over files from git add --edit runs 5 messages from 2019-01-15 to 2019-01-16. Participants: Johannes Schindelin via GitGitGadget, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/50230 ## Johannes Schindelin via GitGitGadget, 2019-01-15 15:42 Subject: [PATCH 0/1] Be careful about left-over files from git add --edit runs Message-ID: URL: https://gitlist.dev/e/pull.107.git.gitgitgadget%40gmail.com ``` 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, 2019-01-15 15:42 Subject: [PATCH 1/1] add --edit: truncate the patch file Message-ID: <52f74c5d356325660ca1b494e78fb99a3c6c3861.1547566970.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/52f74c5d356325660ca1b494e78fb99a3c6c3861.1547566970.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin 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 --- 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 ``` ## Junio C Hamano, 2019-01-15 18:50 Subject: Re: [PATCH 0/1] Be careful about left-over files from git add --edit runs Message-ID: URL: https://gitlist.dev/e/xmqqef9dsqkh.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" 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 ``` ## Junio C Hamano, 2019-01-15 19:01 Subject: Re: [PATCH 1/1] add --edit: truncate the patch file Message-ID: URL: https://gitlist.dev/e/xmqqa7k1sq24.fsf%40gitster-ct.c.googlers.com In-Reply-To: <52f74c5d356325660ca1b494e78fb99a3c6c3861.1547566970.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" 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 ;-) ``` ## Johannes Schindelin, 2019-01-16 13:47 Subject: Re: [PATCH 0/1] Be careful about left-over files from git add --edit runs Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1901161447150.41%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Junio, On Tue, 15 Jan 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > 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 > ```