| Message ID | 20230817215325.2550998-2-taylorsantiago@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix the order of consuming unpackLimit config settings. | expand |
On Thu, Aug 17, 2023 at 02:53:25PM -0700, Taylor Santiago wrote: > The documentation for fetch.unpackLimit states that fetch.unpackLimit > should be treated as higher priority than transfer.unpackLimit, but the > intended order is currently inverted. Looks like this has been broken since it was introduced in e28714c527 (Consolidate {receive,fetch}.unpackLimit, 2007-01-24). Sometimes when documentation and code have diverged for so long, we prefer to change the documentation instead, to avoid disrupting users. But that would make these weirdly unlike most other "specific overrides general" config options. And the fact that the bug has existed for so long without anyone noticing implies to me that nobody really tries to mix and match them much. So I'm in favor of fixing them[1]. Doesn't receive-pack have the same bug, though? And we'd probably want to have tests for both. -Peff [1] Commit e28714c527 does mention deprecating the operation-specific ones. In my experience (and I did use transfer.unpackLimit quite a bit for server-side code at GitHub) there is no real need for have operation-specific ones. OTOH it is work to deprecate them, and they are not causing a big maintenance burden. So it is probably simplest to keep them.
Jeff King <peff@peff.net> writes: > So I'm in favor of fixing them[1]. Doesn't receive-pack have the same > bug, though? And we'd probably want to have tests for both. Thanks, both. The receive-pack side (changes to the code and additional test) should look like this squashable patch. builtin/receive-pack.c | 6 +++--- t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 1a31a58367..03ac7b01d2 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2524,10 +2524,10 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) if (cert_nonce_seed) push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL)); - if (0 <= transfer_unpack_limit) - unpack_limit = transfer_unpack_limit; - else if (0 <= receive_unpack_limit) + if (0 <= receive_unpack_limit) unpack_limit = receive_unpack_limit; + else if (0 <= transfer_unpack_limit) + unpack_limit = transfer_unpack_limit; switch (determine_protocol_version_server()) { case protocol_v2: diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh index eed3c9d81a..9fc9ba552f 100755 --- a/t/t5546-receive-limits.sh +++ b/t/t5546-receive-limits.sh @@ -9,10 +9,26 @@ TEST_PASSES_SANITIZE_LEAK=true # When the limit is 1, `git receive-pack` will call `git index-pack`. # When the limit is 10000, `git receive-pack` will call `git unpack-objects`. +validate_store_type () { + git -C dest count-objects -v >actual && + case "$store_type" in + index) + grep "^count: 0$" actual ;; + unpack) + grep "^packs: 0$" actual ;; + esac || { + echo "store_type is $store_type" + cat actual + false; + } +} + test_pack_input_limit () { - case "$1" in - index) unpack_limit=1 ;; - unpack) unpack_limit=10000 ;; + store_type=$1 + + case "$store_type" in + index) unpack_limit=1 other_limit=10000 ;; + unpack) unpack_limit=10000 other_limit=1 ;; esac test_expect_success 'prepare destination repository' ' @@ -43,6 +59,19 @@ test_pack_input_limit () { git --git-dir=dest config receive.maxInputSize 0 && git push dest HEAD ' + + test_expect_success 'prepare destination repository (once more)' ' + rm -fr dest && + git --bare init dest + ' + + test_expect_success 'receive trumps transfer' ' + git --git-dir=dest config receive.unpacklimit "$unpack_limit" && + git --git-dir=dest config transfer.unpacklimit "$other_limit" && + git push dest HEAD && + validate_store_type + ' + } test_expect_success "create known-size (1024 bytes) commit" '
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> So I'm in favor of fixing them[1]. Doesn't receive-pack have the same >> bug, though? And we'd probably want to have tests for both. > > Thanks, both. The receive-pack side (changes to the code and > additional test) should look like this squashable patch. > > > > builtin/receive-pack.c | 6 +++--- > t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++--- > 2 files changed, 35 insertions(+), 6 deletions(-) Yesterday I was a bit too busy (well it was a release day wasn't it?) and did not bother writing the tests for fetch/transfer, but it seems nobody took the bait to finish it so far, so here it is again. This time, what I am sending is not a squashable patch, but the whole thing as a single patch. ------- >8 ------------- >8 ------------- >8 ------- Subject: transfer.unpackLimit: fetch/receive.unpackLimit takes precedence The transfer.unpackLimit configuration variable is documented to be used only as a fallback value when the more operation-specific fetch.unpackLimit and receive.unpackLimit variables are not set, but the implementation had the precedence reversed. Apparently this was broken since the transfer.unpackLimit was introduced in e28714c5 (Consolidate {receive,fetch}.unpackLimit, 2007-01-24). Often when documentation and code have diverged for so long, we prefer to change the documentation instead, to avoid disrupting users. But doing so would make these weirdly unlike most other "specific overrides general" config options. And the fact that the bug has existed for so long without anyone noticing implies to me that nobody really tries to mix and match them much. Signed-off-by: Taylor Santiago <taylorsantiago@google.com> [jc: rewrote the log message, added tests, covered receive-pack as well] Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/receive-pack.c | 6 +++--- fetch-pack.c | 6 +++--- t/t5510-fetch.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++--- 4 files changed, 84 insertions(+), 9 deletions(-) diff --git c/builtin/receive-pack.c w/builtin/receive-pack.c index 1a31a58367..03ac7b01d2 100644 --- c/builtin/receive-pack.c +++ w/builtin/receive-pack.c @@ -2524,10 +2524,10 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) if (cert_nonce_seed) push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL)); - if (0 <= transfer_unpack_limit) - unpack_limit = transfer_unpack_limit; - else if (0 <= receive_unpack_limit) + if (0 <= receive_unpack_limit) unpack_limit = receive_unpack_limit; + else if (0 <= transfer_unpack_limit) + unpack_limit = transfer_unpack_limit; switch (determine_protocol_version_server()) { case protocol_v2: diff --git c/fetch-pack.c w/fetch-pack.c index 0f71054fba..8b300545d5 100644 --- c/fetch-pack.c +++ w/fetch-pack.c @@ -1911,10 +1911,10 @@ static void fetch_pack_setup(void) if (did_setup) return; fetch_pack_config(); - if (0 <= transfer_unpack_limit) - unpack_limit = transfer_unpack_limit; - else if (0 <= fetch_unpack_limit) + if (0 <= fetch_unpack_limit) unpack_limit = fetch_unpack_limit; + else if (0 <= transfer_unpack_limit) + unpack_limit = transfer_unpack_limit; did_setup = 1; } diff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh index 4f289063ce..19c36b57f4 100755 --- c/t/t5510-fetch.sh +++ w/t/t5510-fetch.sh @@ -1127,6 +1127,52 @@ do ' done +test_expect_success 'prepare source branch' ' + echo one >onebranch && + git checkout --orphan onebranch && + git rm --cached -r . && + git add onebranch && + git commit -m onebranch && + git rev-list --objects onebranch -- >actual && + # 3 objects should be created, at least ... + test 3 -le $(wc -l <actual) +' + +validate_store_type () { + git -C dest count-objects -v >actual && + case "$store_type" in + packed) + grep "^count: 0$" actual ;; + loose) + grep "^packs: 0$" actual ;; + esac || { + echo "store_type is $store_type" + cat actual + false + } +} + +test_unpack_limit () { + store_type=$1 + + case "$store_type" in + packed) fetch_limit=1 transfer_limit=10000 ;; + loose) fetch_limit=10000 transfer_limit=1 ;; + esac + + test_expect_success "fetch trumps transfer limit" ' + rm -fr dest && + git --bare init dest && + git -C dest config fetch.unpacklimit $fetch_limit && + git -C dest config transfer.unpacklimit $transfer_limit && + git -C dest fetch .. onebranch && + validate_store_type + ' +} + +test_unpack_limit packed +test_unpack_limit loose + setup_negotiation_tip () { SERVER="$1" URL="$2" diff --git c/t/t5546-receive-limits.sh w/t/t5546-receive-limits.sh index eed3c9d81a..9fc9ba552f 100755 --- c/t/t5546-receive-limits.sh +++ w/t/t5546-receive-limits.sh @@ -9,10 +9,26 @@ TEST_PASSES_SANITIZE_LEAK=true # When the limit is 1, `git receive-pack` will call `git index-pack`. # When the limit is 10000, `git receive-pack` will call `git unpack-objects`. +validate_store_type () { + git -C dest count-objects -v >actual && + case "$store_type" in + index) + grep "^count: 0$" actual ;; + unpack) + grep "^packs: 0$" actual ;; + esac || { + echo "store_type is $store_type" + cat actual + false; + } +} + test_pack_input_limit () { - case "$1" in - index) unpack_limit=1 ;; - unpack) unpack_limit=10000 ;; + store_type=$1 + + case "$store_type" in + index) unpack_limit=1 other_limit=10000 ;; + unpack) unpack_limit=10000 other_limit=1 ;; esac test_expect_success 'prepare destination repository' ' @@ -43,6 +59,19 @@ test_pack_input_limit () { git --git-dir=dest config receive.maxInputSize 0 && git push dest HEAD ' + + test_expect_success 'prepare destination repository (once more)' ' + rm -fr dest && + git --bare init dest + ' + + test_expect_success 'receive trumps transfer' ' + git --git-dir=dest config receive.unpacklimit "$unpack_limit" && + git --git-dir=dest config transfer.unpacklimit "$other_limit" && + git push dest HEAD && + validate_store_type + ' + } test_expect_success "create known-size (1024 bytes) commit" '
Taylor Santiago <taylorsantiago@google.com> writes: > Thank you! How would you like me to proceed? Should I submit the above as a > v2 of the earlier patch? There is nothing "above" as you seem to be top posting ;-) When somebody else helps by supplying an "squashable" patch, often people are expected to review it and then update their patch(es) using the given material to produce a v2. But as I said, the "squashable" one was only about the receive-pack side; even if you combined it with your original, tests for the fetch side were still missing, so it was not sufficient for a v2. As I didn't see your reply message (to which I am responding to) until now, mostly because it was dropped by the mailing list (perhaps it was an HTML e-mail from GMail or something???), I've further worked on the tests to cover the fetch side and sent out a full version (not a squashable, but just a replacement for the whole thing). It is archived and viewable at https://lore.kernel.org/git/xmqqpm3eh7f6.fsf@gitster.g Part of it is still your original patch, some material in its proposed commit log message was given by Peff, and the rest was written by me, so it carries names of three people. If the result looks acceptable to you, then saying "Yup, that looks good" would be the simplest answer to give to move things forward. Thanks.
Looks good to me. Thanks for the info on the patch process. I also am sending this mail in plain text mode so hopefully the mailing list doesn't drop it. On Tue, Aug 22, 2023 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote: > > Taylor Santiago <taylorsantiago@google.com> writes: > > > Thank you! How would you like me to proceed? Should I submit the above as a > > v2 of the earlier patch? > > There is nothing "above" as you seem to be top posting ;-) > > When somebody else helps by supplying an "squashable" patch, often > people are expected to review it and then update their patch(es) > using the given material to produce a v2. > > But as I said, the "squashable" one was only about the receive-pack > side; even if you combined it with your original, tests for the > fetch side were still missing, so it was not sufficient for a v2. > > As I didn't see your reply message (to which I am responding to) > until now, mostly because it was dropped by the mailing list > (perhaps it was an HTML e-mail from GMail or something???), I've > further worked on the tests to cover the fetch side and sent out a > full version (not a squashable, but just a replacement for the whole > thing). It is archived and viewable at > > https://lore.kernel.org/git/xmqqpm3eh7f6.fsf@gitster.g > > Part of it is still your original patch, some material in its > proposed commit log message was given by Peff, and the rest was > written by me, so it carries names of three people. > > If the result looks acceptable to you, then saying "Yup, that looks > good" would be the simplest answer to give to move things forward. > > Thanks.
On Tue, Aug 22, 2023 at 06:30:21PM -0700, Junio C Hamano wrote: > This time, what I am sending is not a squashable patch, but the > whole thing as a single patch. > > ------- >8 ------------- >8 ------------- >8 ------- > Subject: transfer.unpackLimit: fetch/receive.unpackLimit takes precedence Thanks, this looks fine to me. -Peff
diff --git a/fetch-pack.c b/fetch-pack.c index 65c1ff4bb4..26999e3b65 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1911,10 +1911,10 @@ static void fetch_pack_setup(void) if (did_setup) return; fetch_pack_config(); - if (0 <= transfer_unpack_limit) - unpack_limit = transfer_unpack_limit; - else if (0 <= fetch_unpack_limit) + if (0 <= fetch_unpack_limit) unpack_limit = fetch_unpack_limit; + else if (0 <= transfer_unpack_limit) + unpack_limit = transfer_unpack_limit; did_setup = 1; }
The documentation for fetch.unpackLimit states that fetch.unpackLimit should be treated as higher priority than transfer.unpackLimit, but the intended order is currently inverted. Signed-off-by: Taylor Santiago <taylorsantiago@google.com> --- fetch-pack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)