| Message ID | a096d122-52a3-700a-3a14-30a81b099cd8@theori.io (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 246959346f3407cb047c3d46ed9c44da84bd0b29 |
| Headers | show |
| Series | [v3] diff: make diff_free_filespec_data accept NULL | expand |
Hi Jinoh, On Fri, 6 Nov 2020, Jinoh Kang wrote: > Commit 3aef54e8b8 ("diff: munmap() file contents before running external > diff") introduced calls to diff_free_filespec_data in > run_external_diff, which may pass NULL pointers. Sorry for the breakage! Maybe the commit message could talk a bit about the circumstances when this happens? Of course, I (and every other reader...) could analyze your patch to guess what it is that triggers the bug, but it's really a good use of the commit message to describe it in plain English. > > Fix this and prevent any such bugs in the future by making > `diff_free_filespec_data(NULL)` a no-op. > > Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff") I am unaware of any other commit having a `Fixes:` trailer in the commit message. In any case, I would have expected `Fixes:` to mention a ticket or a bug report, not the commit that fixed _a separate_ issue (but unfortunately introduced this regression). > Signed-off-by: Jinoh Kang <luke1337@theori.io> > --- > diff.c | 3 +++ > t/t7800-difftool.sh | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/diff.c b/diff.c > index d24f47df99..ace4a1d387 100644 > --- a/diff.c > +++ b/diff.c > @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s) > > void diff_free_filespec_data(struct diff_filespec *s) > { > + if (!s) > + return; > + > diff_free_filespec_blob(s); > FREE_AND_NULL(s->cnt_data); We can write this in a more canonical way without wasting all that many lines: if (s) { diff_free_filespec_blob(s); FREE_AND_NULL(s->cnt_data); } > } > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index 524f30f7dc..e9391abb54 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' ' > git difftool --dir-diff --extcmd ls > ' > > +test_expect_success 'difftool --cached with unmerged files' ' > + test_when_finished git reset --hard && > + echo base >file && > + git add file && > + git commit -m base && This does not advance the committer date. Let's just use the helper function we invented to make this much easier: test_commit base This has also the advantage of already tagging the outcome. > + git checkout -B conflict-a && > + git checkout -B conflict-b && > + git checkout conflict-a && > + echo conflict-a >>file && > + git add file && > + git commit -m conflict-a && > + git checkout conflict-b && > + echo conflict-b >>file && > + git add file && > + git commit -m conflict-b && > + git checkout master && > + git merge conflict-a && > + test_must_fail git merge conflict-b && > + : >expect && > + git difftool --cached --no-prompt >actual && > + test_cmp expect actual Shouldn't this use the `test_must_be_empty` function instead? How about writing the test case this way: test_expect_success 'difftool --cached with unmerged files' ' test_when_finished git reset --hard && test_commit conflicting && test_commit conflict-a a conflicting.t && git reset --hard conflicting && test_commit conflict-b b conflicting.t && test_must_fail git merge conflict-a && git difftool --cached --no-prompt >out && test_must_be_empty out ' Ciao, Dscho > +' > + > test_expect_success 'outside worktree' ' > echo 1 >1 && > echo 2 >2 && > -- > 2.26.2 > >
On 11/10/20 12:08 PM, Johannes Schindelin wrote: > Hi Jinoh, > > On Fri, 6 Nov 2020, Jinoh Kang wrote: > >> Commit 3aef54e8b8 ("diff: munmap() file contents before running external >> diff") introduced calls to diff_free_filespec_data in >> run_external_diff, which may pass NULL pointers. > > Sorry for the breakage! No worries. Those functions were indeed confusing... > > Maybe the commit message could talk a bit about the circumstances when > this happens? Of course, I (and every other reader...) could analyze your > patch to guess what it is that triggers the bug, but it's really a good > use of the commit message to describe it in plain English. Agreed. The v1 PATCH, which was off-list, did explain that NULL filespecs are used to indicate unmerged files (i.e. with unresolved conflicts). > >> >> Fix this and prevent any such bugs in the future by making >> `diff_free_filespec_data(NULL)` a no-op. >> >> Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff") > > I am unaware of any other commit having a `Fixes:` trailer in the commit > message. In any case, I would have expected `Fixes:` to mention a ticket > or a bug report, not the commit that fixed _a separate_ issue (but > unfortunately introduced this regression). Sorry for this one; somehow I didn't notice that git and linux use different conventions.
On 11/10/20 1:16 PM, Jinoh Kang wrote: > On 11/10/20 12:08 PM, Johannes Schindelin wrote: >> I am unaware of any other commit having a `Fixes:` trailer in the commit >> message. In any case, I would have expected `Fixes:` to mention a ticket >> or a bug report, not the commit that fixed _a separate_ issue (but >> unfortunately introduced this regression). > > Sorry for this one; somehow I didn't notice that git and linux use > different conventions. > After leaving it out, I realized there are actually five commits using "Fixes: <commit>": $ git log --oneline --grep='Fixes: [0-9a-f]\{8,\} (".*")' master e2bfa50ac3 pack-write/docs: update regarding pack naming 7328482253 repack: disable bitmaps-by-default if .keep files exist ba3a08ca0e fsck: fix leak when traversing trees 5cae73d5d2 http: release strbuf on disabled alternates c22f620205 xread: retry after poll on EAGAIN/EWOULDBLOCK Since it's not that common, I guess it's fine to leave it as is?
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> void diff_free_filespec_data(struct diff_filespec *s) >> { >> + if (!s) >> + return; >> + >> diff_free_filespec_blob(s); >> FREE_AND_NULL(s->cnt_data); > > We can write this in a more canonical way without wasting all that many > lines: > > if (s) { > diff_free_filespec_blob(s); > FREE_AND_NULL(s->cnt_data); > } On only this part. Please do not use this one, as what was posted is better. By making it clear that we do not do anything when given NULL upfront, it lets readers concentrate on the main part of the function---"now we are done with NULL, what happens on a real case?" And when readers are doing so, one extra indentation level is just an extra noise that does not help. In this particular case, it is important more from coding discipline than anything else---for a small enough function like this one whose main part fits on less than fourth of a screen, extra indentation does not matter, but all code tends to grow and it starts to matter if/when we need to clean more things in the function. Everything else said in the review was excellent and very helpful. Thanks.
diff --git a/diff.c b/diff.c index d24f47df99..ace4a1d387 100644 --- a/diff.c +++ b/diff.c @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s) void diff_free_filespec_data(struct diff_filespec *s) { + if (!s) + return; + diff_free_filespec_blob(s); FREE_AND_NULL(s->cnt_data); } diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 524f30f7dc..e9391abb54 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' ' git difftool --dir-diff --extcmd ls ' +test_expect_success 'difftool --cached with unmerged files' ' + test_when_finished git reset --hard && + echo base >file && + git add file && + git commit -m base && + git checkout -B conflict-a && + git checkout -B conflict-b && + git checkout conflict-a && + echo conflict-a >>file && + git add file && + git commit -m conflict-a && + git checkout conflict-b && + echo conflict-b >>file && + git add file && + git commit -m conflict-b && + git checkout master && + git merge conflict-a && + test_must_fail git merge conflict-b && + : >expect && + git difftool --cached --no-prompt >actual && + test_cmp expect actual +' + test_expect_success 'outside worktree' ' echo 1 >1 && echo 2 >2 &&
Commit 3aef54e8b8 ("diff: munmap() file contents before running external diff") introduced calls to diff_free_filespec_data in run_external_diff, which may pass NULL pointers. Fix this and prevent any such bugs in the future by making `diff_free_filespec_data(NULL)` a no-op. Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff") Signed-off-by: Jinoh Kang <luke1337@theori.io> --- diff.c | 3 +++ t/t7800-difftool.sh | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+)