{"thread":{"id":"35332","subject":"[PATCH] transport: Catch non positive --depth option value","startedAt":"2013-11-13T16:06:24Z","lastAt":"2013-11-26T22:19:03Z","messageCount":16,"participants":["Andrés G. Aragoneses","Duy Nguyen","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"230565","messageId":"5283A380.9030308@gmail.com","threadId":"35332","inReplyTo":null,"subject":"[PATCH] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-13T16:06:24Z","receivedAt":"2013-11-13T16:06:24Z","isPatch":true,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":"Instead of simply ignoring the value passed to --depth\noption when it is zero or negative, now it is caught\nand reported.\n\nThis will let people know that they were using the\noption incorrectly (as depth<0 should be simply invalid,\nand under the hood depth==0 didn't mean 'no depth' or\n'no history' but 'full depth' instead).\n\nSigned-off-by: Andres G. Aragoneses \n---\n transport.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport.c b/transport.c\nindex 7202b77..edd63eb 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -483,6 +483,8 @@ static int set_git_option(struct \ngit_transport_options *opts,\n \t\t\topts->depth = strtol(value, &end, 0);\n \t\t\tif (*end)\n \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n+\t\t\tif (opts->depth < 1)\n+\t\t\t\tdie(\"transport: invalid depth option '%s' (non positive)\", value);\n \t\t}\n \t\treturn 0;\n \t}\n-- \n1.8.1.2\n"},{"id":"230692","messageId":"CACsJy8BQr_9YEZq=ydz44w=9d8d5XpBDTLeXNnui7AVrMuDd3A@mail.gmail.com","threadId":"35332","inReplyTo":"5283A380.9030308@gmail.com","subject":"Re: [PATCH] transport: Catch non positive --depth option value","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-11-16T02:58:58Z","receivedAt":"2013-11-16T02:58:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 13, 2013 at 11:06 PM, \"Andrés G. Aragoneses\"\n wrote:\n> Instead of simply ignoring the value passed to --depth\n> option when it is zero or negative, now it is caught\n> and reported.\n>\n> This will let people know that they were using the\n> option incorrectly (as depth<0 should be simply invalid,\n> and under the hood depth==0 didn't mean 'no depth' or\n> 'no history' but 'full depth' instead).\n\n'full depth' may be confusing (is it --unshallow?). Other than that\nthe patch looks fine.\n\n>\n> Signed-off-by: Andres G. Aragoneses \n> ---\n> transport.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/transport.c b/transport.c\n> index 7202b77..edd63eb 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options\n> *opts,\n> opts->depth = strtol(value, &end, 0);\n> if (*end)\n> die(\"transport: invalid depth option '%s'\",\n> value);\n> + if (opts->depth < 1)\n> + die(\"transport: invalid depth option '%s'\n> (non positive)\", value);\n> }\n> return 0;\n> }\n> --\n> 1.8.1.2\n\n\n\n-- \nDuy\n"},{"id":"230758","messageId":"xmqqzjp1bqm3.fsf@gitster.dls.corp.google.com","threadId":"35332","inReplyTo":"5283A380.9030308@gmail.com","subject":"Re: [PATCH] transport: Catch non positive --depth option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-18T16:51:16Z","receivedAt":"2013-11-18T16:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrés G. Aragoneses\" writes:\n\n> Instead of simply ignoring the value passed to --depth\n> option when it is zero or negative, now it is caught\n> and reported.\n>\n> This will let people know that they were using the\n> option incorrectly (as depth<0 should be simply invalid,\n> and under the hood depth==0 didn't mean 'no depth' or\n> 'no history' but 'full depth' instead).\n\nMy initial knee-jerk reaction was: doesn't this change break\nexisting use to unplug a shallow repository and bring it to a\nrepository with an unshallow one to disallow depth=0, though?\n\nI somehow thought that the code supports unshallowing with --depth=0\neven though since 4dcb167f (fetch: add --unshallow for turning\nshallow repo into complete one, 2013-01-11), the officially\nsupported way to tell Git to unshallow is with that option.\n\nBut apparently that is not the case; I do not think depth==0 meant\n'full depth' (i.e. \"git fetch --depth=0\" did not unshallow); it was\nsimply ignored in fetch_pack.c::find_common() and friends.\n\nSo I think it should be a safe change to disallow non-positive depth\nlike this patch does, but the proposed commit log message may need\npolishing.\n\nThanks.\n\n> Signed-off-by: Andres G. Aragoneses \n> ---\n> transport.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/transport.c b/transport.c\n> index 7202b77..edd63eb 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -483,6 +483,8 @@ static int set_git_option(struct\n> git_transport_options *opts,\n> \t\t\topts->depth = strtol(value, &end, 0);\n> \t\t\tif (*end)\n> \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n> +\t\t\tif (opts->depth < 1)\n> +\t\t\t\tdie(\"transport: invalid depth option '%s' (non positive)\", value);\n> \t\t}\n> \t\treturn 0;\n> \t}\n"},{"id":"230787","messageId":"528A9877.4060802@gmail.com","threadId":"35332","inReplyTo":"xmqqzjp1bqm3.fsf@gitster.dls.corp.google.com","subject":"[PATCHv2] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-18T22:45:11Z","receivedAt":"2013-11-18T22:45:11Z","isPatch":false,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":"Instead of simply ignoring the value passed to --depth\noption when it is zero or negative, now it is caught\nand reported.\n\nThis will let people know that they were using the\noption incorrectly (as depth<0 should be simply invalid,\nand under the hood depth==0 didn't have any effect).\n\nSigned-off-by: Andres G. Aragoneses \nReviewed-by: Duy Nguyen \nReviewed-by: Junio C Hamano \n---\n transport.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport.c b/transport.c\nindex 7202b77..edd63eb 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -483,6 +483,8 @@ static int set_git_option(struct \ngit_transport_options *opts,\n opts->depth = strtol(value, &end, 0);\n if (*end)\n die(\"transport: invalid depth option '%s'\", value);\n+ if (opts->depth < 1)\n+ die(\"transport: invalid depth option '%s' (non \npositive)\", value);\n }\n return 0;\n }\n-- \n1.8.1.2\n"},{"id":"230795","messageId":"xmqq61ro9utf.fsf@gitster.dls.corp.google.com","threadId":"35332","inReplyTo":"528A9877.4060802@gmail.com","subject":"Re: [PATCHv2] transport: Catch non positive --depth option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-19T17:15:39Z","receivedAt":"2013-11-19T17:15:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrés G. Aragoneses\" writes:\n\n> Instead of simply ignoring the value passed to --depth\n> option when it is zero or negative, now it is caught\n> and reported.\n>\n> This will let people know that they were using the\n> option incorrectly (as depth<0 should be simply invalid,\n> and under the hood depth==0 didn't have any effect).\n>\n> Signed-off-by: Andres G. Aragoneses \n> Reviewed-by: Duy Nguyen \n> Reviewed-by: Junio C Hamano \n> ---\n> transport.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/transport.c b/transport.c\n> index 7202b77..edd63eb 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -483,6 +483,8 @@ static int set_git_option(struct\n> git_transport_options *opts,\n> opts->depth = strtol(value, &end, 0);\n> if (*end)\n> die(\"transport: invalid depth option '%s'\", value);\n> + if (opts->depth < 1)\n> + die(\"transport: invalid depth option '%s' (non\n> positive)\", value);\n\n\"transport: depth option '%s' must be positive\", perhaps?\n\n> }\n> return 0;\n> }\n\nLinewrapped and whitespace damaged.\n"},{"id":"230890","messageId":"528E2660.6020107@gmail.com","threadId":"35332","inReplyTo":"xmqq61ro9utf.fsf@gitster.dls.corp.google.com","subject":"[PATCHv3] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-21T15:27:28Z","receivedAt":"2013-11-21T15:27:28Z","isPatch":false,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":">From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \nDate: Wed, 13 Nov 2013 16:55:08 +0100\nSubject: [PATCH] transport: Catch non positive --depth option value\n\nInstead of simply ignoring the value passed to --depth\noption when it is zero or negative, now it is caught\nand reported.\n\nThis will let people know that they were using the\noption incorrectly (as depth<0 should be simply invalid,\nand under the hood depth==0 didn't have any effect).\n\nSigned-off-by: Andres G. Aragoneses \nReviewed-by: Duy Nguyen \nReviewed-by: Junio C Hamano \n---\n transport.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport.c b/transport.c\nindex 7202b77..edd63eb 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts,\n \t\t\topts->depth = strtol(value, &end, 0);\n \t\t\tif (*end)\n \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n+\t\t\tif (opts->depth < 1)\n+\t\t\t\tdie(\"transport: invalid depth option '%s' (must be positive)\", value);\n \t\t}\n \t\treturn 0;\n \t}\n-- \n1.8.1.2\n"},{"id":"230893","messageId":"xmqqzjox4q1i.fsf@gitster.dls.corp.google.com","threadId":"35332","inReplyTo":"528E2660.6020107@gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T17:34:33Z","receivedAt":"2013-11-21T17:34:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"thanks.\n"},{"id":"230900","messageId":"xmqq1u294ih3.fsf@gitster.dls.corp.google.com","threadId":"35332","inReplyTo":"528E2660.6020107@gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T20:18:00Z","receivedAt":"2013-11-21T20:18:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrés G. Aragoneses\" writes:\n\n> From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \n> Date: Wed, 13 Nov 2013 16:55:08 +0100\n> Subject: [PATCH] transport: Catch non positive --depth option value\n>\n> Instead of simply ignoring the value passed to --depth\n> option when it is zero or negative, now it is caught\n> and reported.\n>\n> This will let people know that they were using the\n> option incorrectly (as depth<0 should be simply invalid,\n> and under the hood depth==0 didn't have any effect).\n>\n> Signed-off-by: Andres G. Aragoneses \n> Reviewed-by: Duy Nguyen \n> Reviewed-by: Junio C Hamano \n\nI didn't exactly \"review\" this.\n\nHave you run the tests with this patch? It seems that it breaks\nquite a lot of them, including t5500, t5503, t5510, among others.\n\n> ---\n> transport.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/transport.c b/transport.c\n> index 7202b77..edd63eb 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts,\n> \t\t\topts->depth = strtol(value, &end, 0);\n> \t\t\tif (*end)\n> \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n> +\t\t\tif (opts->depth < 1)\n> +\t\t\t\tdie(\"transport: invalid depth option '%s' (must be positive)\", value);\n> \t\t}\n> \t\treturn 0;\n> \t}\n"},{"id":"230929","messageId":"CACsJy8B0qBmBkx0n2B=ivUqZTgVz-ZLhTQ_nVJ4AV0njnZksfw@mail.gmail.com","threadId":"35332","inReplyTo":"xmqq1u294ih3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-11-22T01:18:44Z","receivedAt":"2013-11-22T01:18:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano wrote:\n> \"Andrés G. Aragoneses\" writes:\n>\n>> From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001\n>> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \n>> Date: Wed, 13 Nov 2013 16:55:08 +0100\n>> Subject: [PATCH] transport: Catch non positive --depth option value\n>>\n>> Instead of simply ignoring the value passed to --depth\n>> option when it is zero or negative, now it is caught\n>> and reported.\n>>\n>> This will let people know that they were using the\n>> option incorrectly (as depth<0 should be simply invalid,\n>> and under the hood depth==0 didn't have any effect).\n>>\n>> Signed-off-by: Andres G. Aragoneses \n>> Reviewed-by: Duy Nguyen \n>> Reviewed-by: Junio C Hamano \n>\n> I didn't exactly \"review\" this.\n>\n> Have you run the tests with this patch? It seems that it breaks\n> quite a lot of them, including t5500, t5503, t5510, among others.\n\nI guess it's caused by builtin/fetch.c:backfill_tags(). And the call\ncould be replaced with\n\ntransport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n\n>\n>> ---\n>> transport.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/transport.c b/transport.c\n>> index 7202b77..edd63eb 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts,\n>> opts->depth = strtol(value, &end, 0);\n>> if (*end)\n>> die(\"transport: invalid depth option '%s'\", value);\n>> + if (opts->depth < 1)\n>> + die(\"transport: invalid depth option '%s' (must be positive)\", value);\n>> }\n>> return 0;\n>> }\n>\n\n\n\n-- \nDuy\n"},{"id":"231117","messageId":"5293DE93.3020008@gmail.com","threadId":"35332","inReplyTo":"CACsJy8B0qBmBkx0n2B=ivUqZTgVz-ZLhTQ_nVJ4AV0njnZksfw@mail.gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-25T23:34:43Z","receivedAt":"2013-11-25T23:34:43Z","isPatch":false,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":"On 22/11/13 02:18, Duy Nguyen wrote:\n> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano wrote:\n>> Have you run the tests with this patch? It seems that it breaks\n>> quite a lot of them, including t5500, t5503, t5510, among others.\n> \n> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call\n> could be replaced with\n> \n> transport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n> \n\nNot sure what you mean,\nhttps://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't\ncall backfill_tags. What do you mean?\n\nThanks\n"},{"id":"231121","messageId":"CACsJy8BV74W63Sak-j_9RMjp_5Bo8HMd3Xc93GTtSn4yWStfEA@mail.gmail.com","threadId":"35332","inReplyTo":"5293DE93.3020008@gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-11-26T03:06:47Z","receivedAt":"2013-11-26T03:06:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 26, 2013 at 6:34 AM, \"Andrés G. Aragoneses\"\n wrote:\n> On 22/11/13 02:18, Duy Nguyen wrote:\n>> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano wrote:\n>>> Have you run the tests with this patch? It seems that it breaks\n>>> quite a lot of them, including t5500, t5503, t5510, among others.\n>>\n>> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call\n>> could be replaced with\n>>\n>> transport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n>>\n>\n> Not sure what you mean,\n> https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't\n> call backfill_tags. What do you mean?\n\nI wrote \"I guess\" ;-) I did not check what t5550 does.\n\n>\n> Thanks\n>\n\n\n\n-- \nDuy\n"},{"id":"231130","messageId":"52947B42.4080105@gmail.com","threadId":"35332","inReplyTo":"CACsJy8BV74W63Sak-j_9RMjp_5Bo8HMd3Xc93GTtSn4yWStfEA@mail.gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-26T10:43:14Z","receivedAt":"2013-11-26T10:43:14Z","isPatch":false,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":"On 26/11/13 04:06, Duy Nguyen wrote:\n> On Tue, Nov 26, 2013 at 6:34 AM, \"Andrés G. Aragoneses\"\n> wrote:\n>> On 22/11/13 02:18, Duy Nguyen wrote:\n>>> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano wrote:\n>>>> Have you run the tests with this patch? It seems that it breaks\n>>>> quite a lot of them, including t5500, t5503, t5510, among others.\n>>>\n>>> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call\n>>> could be replaced with\n>>>\n>>> transport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n>>>\n>>\n>> Not sure what you mean,\n>> https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't\n>> call backfill_tags. What do you mean?\n\nThat was a typo, I meant\nhttps://github.com/git/git/blob/master/t/t5500-fetch-pack.sh\n\n\n> I wrote \"I guess\" ;-) I did not check what t5550 does.\n\nAny hint on where to start looking? It doesn't look like any test is\nusing a non-positive depth, so I'm really confused.\n"},{"id":"231131","messageId":"CACsJy8Dfibu96VchD=p_05deLm-46mfXZzcYQg+0BqaN2=To=A@mail.gmail.com","threadId":"35332","inReplyTo":"52947B42.4080105@gmail.com","subject":"Re: [PATCHv3] transport: Catch non positive --depth option value","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-11-26T11:09:41Z","receivedAt":"2013-11-26T11:09:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 26, 2013 at 5:43 PM, \"Andrés G. Aragoneses\"\n wrote:\n> On 26/11/13 04:06, Duy Nguyen wrote:\n>> On Tue, Nov 26, 2013 at 6:34 AM, \"Andrés G. Aragoneses\"\n>> wrote:\n>>> On 22/11/13 02:18, Duy Nguyen wrote:\n>>>> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano wrote:\n>>>>> Have you run the tests with this patch? It seems that it breaks\n>>>>> quite a lot of them, including t5500, t5503, t5510, among others.\n>>>>\n>>>> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call\n>>>> could be replaced with\n>>>>\n>>>> transport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n>>>>\n>>>\n>>> Not sure what you mean,\n>>> https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't\n>>> call backfill_tags. What do you mean?\n>\n> That was a typo, I meant\n> https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh\n>\n>\n>> I wrote \"I guess\" ;-) I did not check what t5550 does.\n>\n> Any hint on where to start looking? It doesn't look like any test is\n> using a non-positive depth, so I'm really confused.\n\nReplace die() with \"*(char*)0 = 0;\" and run t5500, I got a core dump.\nRunning gdb shows this\n\n(gdb) bt\n#0 0x000000000053d98b in set_git_option (opts=0xb63d00, name=0x575767\n\"depth\", value=0x575c91 \"0\") at transport.c:487\n#1 0x000000000053f163 in transport_set_option (transport=0xb63f00,\nname=0x575767 \"depth\", value=0x575c91 \"0\") at transport.c:1000\n#2 0x0000000000437b68 in backfill_tags (transport=0xb63f00,\nref_map=0xb64d60) at builtin/fetch.c:773\n#3 0x0000000000437f91 in do_fetch (transport=0xb63f00, refs=0xb643c0,\nref_count=1) at builtin/fetch.c:869\n#4 0x00000000004386d4 in fetch_one (remote=0xb63c20, argc=1,\nargv=0x7fff32f63588) at builtin/fetch.c:1037\n#5 0x0000000000438a1d in cmd_fetch (argc=2, argv=0x7fff32f63580,\nprefix=0x0) at builtin/fetch.c:1115\n#6 0x000000000040590f in run_builtin (p=0x7d59a8, argc=4,\nargv=0x7fff32f63580) at git.c:314\n#7 0x0000000000405aa2 in handle_internal_command (argc=4,\nargv=0x7fff32f63580) at git.c:478\n#8 0x0000000000405bbc in run_argv (argcp=0x7fff32f6346c,\nargv=0x7fff32f63470) at git.c:524\n#9 0x0000000000405d61 in main (argc=4, av=0x7fff32f63578) at git.c:607\n\nMy guess seems right.\n-- \nDuy\n"},{"id":"231132","messageId":"529488D5.80605@gmail.com","threadId":"35332","inReplyTo":"CACsJy8Dfibu96VchD=p_05deLm-46mfXZzcYQg+0BqaN2=To=A@mail.gmail.com","subject":"[PATCHv4] transport: Catch non positive --depth option value","fromName":"Andrés G. Aragoneses","fromEmail":"knocte@gmail.com","sentAt":"2013-11-26T11:41:09Z","receivedAt":"2013-11-26T11:41:09Z","isPatch":false,"sender":{"key":"knocte@gmail.com","avatar":"https://gravatar.com/avatar/020c5605dcd7456d87a5f29a2af53e7bdf135fe9ccda514b9f34ab3a7e46db27?d=mp&s=160"},"body":">From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \nDate: Tue, 26 Nov 2013 12:38:19 +0100\nSubject: [PATCH] transport: Catch non positive --depth option value\n\nInstead of simply ignoring the value passed to --depth\noption when it is zero or negative, now it is caught\nand reported.\n\nThis will let people know that they were using the\noption incorrectly (as depth<0 should be simply invalid,\nand under the hood depth==0 didn't have any effect).\n\n(The change in fetch.c is needed to avoid the tests\nfailing because of this new restriction.)\n\nSigned-off-by: Andres G. Aragoneses \nReviewed-by: Duy Nguyen \n---\n builtin/fetch.c | 2 +-\n transport.c | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bd7a101..88c04d7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \t}\n \n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n-\ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n+\ttransport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n \tfetch_refs(transport, ref_map);\n \n \tif (gsecondary) {\ndiff --git a/transport.c b/transport.c\nindex 7202b77..5b42ccb 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts,\n \t\t\topts->depth = strtol(value, &end, 0);\n \t\t\tif (*end)\n \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n+\t\t\tif (opts->depth < 1)\n+\t\t\t\tdie(\"transport: invalid depth option '%s' (must be positive)\", value);\n \t\t}\n \t\treturn 0;\n \t}\n-- \n1.8.1.2\n"},{"id":"231141","messageId":"20131126190902.GB4212@google.com","threadId":"35332","inReplyTo":"529488D5.80605@gmail.com","subject":"Re: [PATCHv4] transport: Catch non positive --depth option value","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-26T19:09:02Z","receivedAt":"2013-11-26T19:09:02Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThanks for tackling this. This review will be kind of nitpicky, as a\nway to save time when reviewing future patches.\n\nAndrés G. Aragoneses wrote:\n\n> From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \n> Date: Tue, 26 Nov 2013 12:38:19 +0100\n> Subject: [PATCH] transport: Catch non positive --depth option value\n\nThese lines are redundant next to the mail header, so they can and\nshould be omitted to avoid some noise.\n\n> Instead of simply ignoring the value passed to --depth\n> option when it is zero or negative, now it is caught\n> and reported.\n\nNit: commit messages usually give a command to the codebase, like\nthis:\n\n\tWhen the value passed to --depth is zero or negative, instead of\n\ttreating it as infinite depth, catch and report the mistake.\n\n> This will let people know that they were using the\n> option incorrectly (as depth<0 should be simply invalid,\n> and under the hood depth==0 didn't have any effect).\n\nOk. Do we know that no one was using --depth=0 this way deliberately?\n\n> (The change in fetch.c is needed to avoid the tests\n> failing because of this new restriction.)\n\nBased on the surrounding thread I see that you're talking about the\ntest script t5500 here. Which test failed? How does it use \"git\nfetch\"? Does the change just fix the test but keep in broken in\nproduction, or does it fix \"git fetch\" in production, too?\n\n> Signed-off-by: Andres G. Aragoneses \n> Reviewed-by: Duy Nguyen \n> ---\n> builtin/fetch.c | 2 +-\n> transport.c | 2 ++\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n\nIt would be nice to have a brief test to demonstrate the fix and make\nsure we don't break it in the future. \"grep fetch.*--depth t/*.sh\"\ntells me t5500 would be a good place to put it. For example,\nsomething like\n\n\ttest_expect_success 'fetch catches invalid --depth values' '\n\t\t(\n\t\t\tcd shallow &&\n\t\t\ttest_must_fail git fetch --depth=0 &&\n\t\t\ttest_must_fail git fetch --depth=-2 &&\n\t\t\ttest_must_fail git fetch --depth= &&\n\t\t\ttest_must_fail git fetch --depth=nonsense\n\t\t)\n\t'\n\nWhat do you think?\nJonathan\n"},{"id":"231149","messageId":"xmqqfvqivmaw.fsf@gitster.dls.corp.google.com","threadId":"35332","inReplyTo":"529488D5.80605@gmail.com","subject":"Re: [PATCHv4] transport: Catch non positive --depth option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-26T22:19:03Z","receivedAt":"2013-11-26T22:19:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrés G. Aragoneses\" writes:\n\n> From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= \n> Date: Tue, 26 Nov 2013 12:38:19 +0100\n> Subject: [PATCH] transport: Catch non positive --depth option value\n\nPlease do not leave these four lines in your e-mail message, unless\nthere is a good reason to do so (e.g. when you are forwarding a\npatch authored by somebody else, you may want a \"From:\" that names\nthe real author at the beginning, but that does not apply in this\ncase where you are sending your own).\n\nThe first line is merely a marker to say the file is a format-patch\noutput, and the header lines are there for those who use \"git\nsend-email\" to mail the messages out, and/or for those who want to\ncut & paste some of them (not copy & paste) to their MUA header\ninput widgets.\n\n> Instead of simply ignoring the value passed to --depth option when\n> it is zero or negative, now it is caught and reported.\n>\n> This will let people know that they were using the option\n> incorrectly (as depth<0 should be simply invalid, and under the\n> hood depth==0 didn't have any effect).\n>\n> (The change in fetch.c is needed to avoid the tests failing\n> because of this new restriction.)\n\nGood, but it is not just tests but without that change real\noperations break. In other words, it is an integral part of the\npatch, not a workaround for a broken test.\n\nThanks. Will queue with a bit of tweak.\n\n> Signed-off-by: Andres G. Aragoneses \n> Reviewed-by: Duy Nguyen \n> ---\n> builtin/fetch.c | 2 +-\n> transport.c | 2 ++\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index bd7a101..88c04d7 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n> \t}\n> \n> \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n> -\ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n> +\ttransport_set_option(transport, TRANS_OPT_DEPTH, NULL);\n> \tfetch_refs(transport, ref_map);\n> \n> \tif (gsecondary) {\n> diff --git a/transport.c b/transport.c\n> index 7202b77..5b42ccb 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts,\n> \t\t\topts->depth = strtol(value, &end, 0);\n> \t\t\tif (*end)\n> \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n> +\t\t\tif (opts->depth < 1)\n> +\t\t\t\tdie(\"transport: invalid depth option '%s' (must be positive)\", value);\n> \t\t}\n> \t\treturn 0;\n> \t}\n"}]}