| Message ID | 20190208212221.31670-1-max@max630.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pack-refs: fail on falsely sorted packed-refs | expand |
On Fri, Feb 8, 2019 at 4:22 PM Max Kirillov <max@max630.net> wrote: > If packed-refs is marked as sorted but not really sorted it causes > very hard to comprehend misbehavior of reference resolving - a reference > is reported as not found, though it is listed by commands which output > the references list. > > As the scope of the issue is not clear, make it visible by failing > pack-refs command - the one which would not suffer performance penalty > to verify the sortedness - when it encounters not really sorted existing > data. > > Signed-off-by: Max Kirillov <max@max630.net> > --- > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > @@ -1137,6 +1138,21 @@ static int write_with_updates(struct packed_ref_store *refs, > + if (iter) > + { > + if (prev_ref.len && strcmp(prev_ref.buf, iter->refname) > 0) > + { > + [...] > + strbuf_release(&prev_ref); > + goto error; > + } > + > + strbuf_init(&prev_ref, 0); > + strbuf_addstr(&prev_ref, iter->refname); > + } The call to strbuf_init() is leaking the allocated strbuf buffer each time through the loop. The typical way to re-use a strbuf, and the way you should do it here, is strbuf_reset().
On Mon, Feb 11, 2019 at 08:24:46PM +0100, Michael Haggerty wrote: > The change to `write_with_updates()` doesn't only affect `pack-refs`. > That function is also called when the `packed-refs` file has to be > rewritten when a packed reference is deleted. This is another thing > that you could test. Ok, I'll check ti and add to the tests. > But that also means that fairly common commands like `git branch -d` > could be slowed down by this change. I doubt that the slowdown is > prohibitive, but it would be great to see numbers to prove it. For > example, create a repository with a lot (say 10000) references, pack > them, then run `git branch -d` to delete one of them. Benchmark that > once with master and once with your modification and document the > difference. At my hardware, with 1M references, "branch -d" takes 0.31s of user time before change vs 0.38 after change. Should I mention it in the commit message? >> +test_expect_success 'off-order branch not found' ' >> + test_must_fail git show-ref --verify --quiet refs/heads/b00 >> +' > > I don't think that the above test makes sense. We don't *guarantee* > that an out-of-order reference won't be found. That is an > implementation detail that we are free to change. I think that it > would be OK to just omit this test. Thanks, will remove this one
On Fri, Feb 08, 2019 at 04:40:28PM -0500, Eric Sunshine wrote: > The call to strbuf_init() is leaking the allocated strbuf buffer each > time through the loop. The typical way to re-use a strbuf, and the way > you should do it here, is strbuf_reset(). Thank you! Will fix it
diff --git a/refs/packed-backend.c b/refs/packed-backend.c index c01c7f5901..c89a5eb899 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1088,6 +1088,7 @@ static int write_with_updates(struct packed_ref_store *refs, FILE *out; struct strbuf sb = STRBUF_INIT; char *packed_refs_path; + struct strbuf prev_ref = STRBUF_INIT; if (!is_lock_file_locked(&refs->lock)) BUG("write_with_updates() called while unlocked"); @@ -1137,6 +1138,21 @@ static int write_with_updates(struct packed_ref_store *refs, struct ref_update *update = NULL; int cmp; + if (iter) + { + if (prev_ref.len && strcmp(prev_ref.buf, iter->refname) > 0) + { + strbuf_addf(err, "broken sorting in packed-refs: '%s' > '%s'", + prev_ref.buf, + iter->refname); + strbuf_release(&prev_ref); + goto error; + } + + strbuf_init(&prev_ref, 0); + strbuf_addstr(&prev_ref, iter->refname); + } + if (i >= updates->nr) { cmp = -1; } else { @@ -1240,6 +1256,8 @@ static int write_with_updates(struct packed_ref_store *refs, } } + strbuf_release(&prev_ref); + if (ok != ITER_DONE) { strbuf_addstr(err, "unable to write packed-refs file: " "error iterating over old contents"); diff --git a/t/t3212-pack-refs-broken-sorting.sh b/t/t3212-pack-refs-broken-sorting.sh new file mode 100755 index 0000000000..a44785c8fc --- /dev/null +++ b/t/t3212-pack-refs-broken-sorting.sh @@ -0,0 +1,26 @@ +#!/bin/sh + +test_description='tests for the falsely sorted refs' +. ./test-lib.sh + +test_expect_success 'setup' ' + git commit --allow-empty -m commit && + for num in $(test_seq 10) + do + git branch b$(printf "%02d" $num) || return 1 + done && + git pack-refs --all && + head_object=$(git rev-parse HEAD) && + printf "$head_object refs/heads/b00\\n" >>.git/packed-refs && + git branch b11 +' + +test_expect_success 'off-order branch not found' ' + test_must_fail git show-ref --verify --quiet refs/heads/b00 +' + +test_expect_success 'subsequent pack-refs fails' ' + test_must_fail git pack-refs --all +' + +test_done
If packed-refs is marked as sorted but not really sorted it causes very hard to comprehend misbehavior of reference resolving - a reference is reported as not found, though it is listed by commands which output the references list. As the scope of the issue is not clear, make it visible by failing pack-refs command - the one which would not suffer performance penalty to verify the sortedness - when it encounters not really sorted existing data. Signed-off-by: Max Kirillov <max@max630.net> --- Fixed the notes refs/packed-backend.c | 18 ++++++++++++++++++ t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100755 t/t3212-pack-refs-broken-sorting.sh