[PATCH 0/1] Fix the order of consuming unpackLimit config settings.

[PATCH 0/1] Fix the order of consuming unpackLimit config settings.

[Taylor Santiago]

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.

Taylor Santiago (1):
  Fix the order of consuming unpackLimit config settings.

 fetch-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: f1ed9d7dc0e49dc1a044941d821c9d2342313c26
-- 
2.41.0.694.ge786442a9b-goog

[PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Taylor Santiago]

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(-)

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;
 }
 
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Jeff King]

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.

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Junio C Hamano]

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" '
-- 
2.42.0

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Junio C Hamano]

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" '

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Jeff King]

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

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Junio C Hamano]

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.

Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.

[Taylor Santiago]

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.