* [PATCH] transport: Catch non positive --depth option value @ 2013-11-13 16:06 "Andrés G. Aragoneses" 2013-11-16 2:58 ` Duy Nguyen 2013-11-18 16:51 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-13 16:06 UTC (permalink / raw) To: git Instead of simply ignoring the value passed to --depth option when it is zero or negative, now it is caught and reported. This will let people know that they were using the option incorrectly (as depth<0 should be simply invalid, and under the hood depth==0 didn't mean 'no depth' or 'no history' but 'full depth' instead). Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> --- transport.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/transport.c b/transport.c index 7202b77..edd63eb 100644 --- a/transport.c +++ b/transport.c @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, opts->depth = strtol(value, &end, 0); if (*end) die("transport: invalid depth option '%s'", value); + if (opts->depth < 1) + die("transport: invalid depth option '%s' (non positive)", value); } return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] transport: Catch non positive --depth option value 2013-11-13 16:06 [PATCH] transport: Catch non positive --depth option value "Andrés G. Aragoneses" @ 2013-11-16 2:58 ` Duy Nguyen 2013-11-18 16:51 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Duy Nguyen @ 2013-11-16 2:58 UTC (permalink / raw) To: Andrés G. Aragoneses, Git Mailing List On Wed, Nov 13, 2013 at 11:06 PM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote: > Instead of simply ignoring the value passed to --depth > option when it is zero or negative, now it is caught > and reported. > > This will let people know that they were using the > option incorrectly (as depth<0 should be simply invalid, > and under the hood depth==0 didn't mean 'no depth' or > 'no history' but 'full depth' instead). 'full depth' may be confusing (is it --unshallow?). Other than that the patch looks fine. > > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > --- > transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/transport.c b/transport.c > index 7202b77..edd63eb 100644 > --- a/transport.c > +++ b/transport.c > @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options > *opts, > opts->depth = strtol(value, &end, 0); > if (*end) > die("transport: invalid depth option '%s'", > value); > + if (opts->depth < 1) > + die("transport: invalid depth option '%s' > (non positive)", value); > } > return 0; > } > -- > 1.8.1.2 -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] transport: Catch non positive --depth option value 2013-11-13 16:06 [PATCH] transport: Catch non positive --depth option value "Andrés G. Aragoneses" 2013-11-16 2:58 ` Duy Nguyen @ 2013-11-18 16:51 ` Junio C Hamano 2013-11-18 22:45 ` [PATCHv2] " "Andrés G. Aragoneses" 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2013-11-18 16:51 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: git "Andrés G. Aragoneses" <knocte@gmail.com> writes: > Instead of simply ignoring the value passed to --depth > option when it is zero or negative, now it is caught > and reported. > > This will let people know that they were using the > option incorrectly (as depth<0 should be simply invalid, > and under the hood depth==0 didn't mean 'no depth' or > 'no history' but 'full depth' instead). My initial knee-jerk reaction was: doesn't this change break existing use to unplug a shallow repository and bring it to a repository with an unshallow one to disallow depth=0, though? I somehow thought that the code supports unshallowing with --depth=0 even though since 4dcb167f (fetch: add --unshallow for turning shallow repo into complete one, 2013-01-11), the officially supported way to tell Git to unshallow is with that option. But apparently that is not the case; I do not think depth==0 meant 'full depth' (i.e. "git fetch --depth=0" did not unshallow); it was simply ignored in fetch_pack.c::find_common() and friends. So I think it should be a safe change to disallow non-positive depth like this patch does, but the proposed commit log message may need polishing. Thanks. > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > --- > transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/transport.c b/transport.c > index 7202b77..edd63eb 100644 > --- a/transport.c > +++ b/transport.c > @@ -483,6 +483,8 @@ static int set_git_option(struct > git_transport_options *opts, > opts->depth = strtol(value, &end, 0); > if (*end) > die("transport: invalid depth option '%s'", value); > + if (opts->depth < 1) > + die("transport: invalid depth option '%s' (non positive)", value); > } > return 0; > } ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv2] transport: Catch non positive --depth option value 2013-11-18 16:51 ` Junio C Hamano @ 2013-11-18 22:45 ` "Andrés G. Aragoneses" 2013-11-19 17:15 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-18 22:45 UTC (permalink / raw) To: git; +Cc: gitster, pclouds Instead of simply ignoring the value passed to --depth option when it is zero or negative, now it is caught and reported. This will let people know that they were using the option incorrectly (as depth<0 should be simply invalid, and under the hood depth==0 didn't have any effect). Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> Reviewed-by: Duy Nguyen <pclouds@gmail.com> Reviewed-by: Junio C Hamano <gitster@pobox.com> --- transport.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/transport.c b/transport.c index 7202b77..edd63eb 100644 --- a/transport.c +++ b/transport.c @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, opts->depth = strtol(value, &end, 0); if (*end) die("transport: invalid depth option '%s'", value); + if (opts->depth < 1) + die("transport: invalid depth option '%s' (non positive)", value); } return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv2] transport: Catch non positive --depth option value 2013-11-18 22:45 ` [PATCHv2] " "Andrés G. Aragoneses" @ 2013-11-19 17:15 ` Junio C Hamano 2013-11-21 15:27 ` [PATCHv3] " "Andrés G. Aragoneses" 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2013-11-19 17:15 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: git, pclouds "Andrés G. Aragoneses" <knocte@gmail.com> writes: > Instead of simply ignoring the value passed to --depth > option when it is zero or negative, now it is caught > and reported. > > This will let people know that they were using the > option incorrectly (as depth<0 should be simply invalid, > and under the hood depth==0 didn't have any effect). > > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > Reviewed-by: Duy Nguyen <pclouds@gmail.com> > Reviewed-by: Junio C Hamano <gitster@pobox.com> > --- > transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/transport.c b/transport.c > index 7202b77..edd63eb 100644 > --- a/transport.c > +++ b/transport.c > @@ -483,6 +483,8 @@ static int set_git_option(struct > git_transport_options *opts, > opts->depth = strtol(value, &end, 0); > if (*end) > die("transport: invalid depth option '%s'", value); > + if (opts->depth < 1) > + die("transport: invalid depth option '%s' (non > positive)", value); "transport: depth option '%s' must be positive", perhaps? > } > return 0; > } Linewrapped and whitespace damaged. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv3] transport: Catch non positive --depth option value 2013-11-19 17:15 ` Junio C Hamano @ 2013-11-21 15:27 ` "Andrés G. Aragoneses" 2013-11-21 17:34 ` Junio C Hamano 2013-11-21 20:18 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-21 15:27 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Junio C Hamano >From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> Date: Wed, 13 Nov 2013 16:55:08 +0100 Subject: [PATCH] transport: Catch non positive --depth option value Instead of simply ignoring the value passed to --depth option when it is zero or negative, now it is caught and reported. This will let people know that they were using the option incorrectly (as depth<0 should be simply invalid, and under the hood depth==0 didn't have any effect). Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> Reviewed-by: Duy Nguyen <pclouds@gmail.com> Reviewed-by: Junio C Hamano <gitster@pobox.com> --- transport.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/transport.c b/transport.c index 7202b77..edd63eb 100644 --- a/transport.c +++ b/transport.c @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, opts->depth = strtol(value, &end, 0); if (*end) die("transport: invalid depth option '%s'", value); + if (opts->depth < 1) + die("transport: invalid depth option '%s' (must be positive)", value); } return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-21 15:27 ` [PATCHv3] " "Andrés G. Aragoneses" @ 2013-11-21 17:34 ` Junio C Hamano 2013-11-21 20:18 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2013-11-21 17:34 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: git, Duy Nguyen thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-21 15:27 ` [PATCHv3] " "Andrés G. Aragoneses" 2013-11-21 17:34 ` Junio C Hamano @ 2013-11-21 20:18 ` Junio C Hamano 2013-11-22 1:18 ` Duy Nguyen 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2013-11-21 20:18 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: git, Duy Nguyen "Andrés G. Aragoneses" <knocte@gmail.com> writes: > From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> > Date: Wed, 13 Nov 2013 16:55:08 +0100 > Subject: [PATCH] transport: Catch non positive --depth option value > > Instead of simply ignoring the value passed to --depth > option when it is zero or negative, now it is caught > and reported. > > This will let people know that they were using the > option incorrectly (as depth<0 should be simply invalid, > and under the hood depth==0 didn't have any effect). > > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > Reviewed-by: Duy Nguyen <pclouds@gmail.com> > Reviewed-by: Junio C Hamano <gitster@pobox.com> I didn't exactly "review" this. Have you run the tests with this patch? It seems that it breaks quite a lot of them, including t5500, t5503, t5510, among others. > --- > transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/transport.c b/transport.c > index 7202b77..edd63eb 100644 > --- a/transport.c > +++ b/transport.c > @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, > opts->depth = strtol(value, &end, 0); > if (*end) > die("transport: invalid depth option '%s'", value); > + if (opts->depth < 1) > + die("transport: invalid depth option '%s' (must be positive)", value); > } > return 0; > } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-21 20:18 ` Junio C Hamano @ 2013-11-22 1:18 ` Duy Nguyen 2013-11-25 23:34 ` "Andrés G. Aragoneses" 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2013-11-22 1:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Andrés G. Aragoneses, Git Mailing List On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: > "Andrés G. Aragoneses" <knocte@gmail.com> writes: > >> From 99e387151594572dc136bf1fae45593ee710e817 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> >> Date: Wed, 13 Nov 2013 16:55:08 +0100 >> Subject: [PATCH] transport: Catch non positive --depth option value >> >> Instead of simply ignoring the value passed to --depth >> option when it is zero or negative, now it is caught >> and reported. >> >> This will let people know that they were using the >> option incorrectly (as depth<0 should be simply invalid, >> and under the hood depth==0 didn't have any effect). >> >> Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> >> Reviewed-by: Duy Nguyen <pclouds@gmail.com> >> Reviewed-by: Junio C Hamano <gitster@pobox.com> > > I didn't exactly "review" this. > > Have you run the tests with this patch? It seems that it breaks > quite a lot of them, including t5500, t5503, t5510, among others. I guess it's caused by builtin/fetch.c:backfill_tags(). And the call could be replaced with transport_set_option(transport, TRANS_OPT_DEPTH, NULL); > >> --- >> transport.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/transport.c b/transport.c >> index 7202b77..edd63eb 100644 >> --- a/transport.c >> +++ b/transport.c >> @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, >> opts->depth = strtol(value, &end, 0); >> if (*end) >> die("transport: invalid depth option '%s'", value); >> + if (opts->depth < 1) >> + die("transport: invalid depth option '%s' (must be positive)", value); >> } >> return 0; >> } > -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-22 1:18 ` Duy Nguyen @ 2013-11-25 23:34 ` "Andrés G. Aragoneses" 2013-11-26 3:06 ` Duy Nguyen 0 siblings, 1 reply; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-25 23:34 UTC (permalink / raw) To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List On 22/11/13 02:18, Duy Nguyen wrote: > On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Have you run the tests with this patch? It seems that it breaks >> quite a lot of them, including t5500, t5503, t5510, among others. > > I guess it's caused by builtin/fetch.c:backfill_tags(). And the call > could be replaced with > > transport_set_option(transport, TRANS_OPT_DEPTH, NULL); > Not sure what you mean, https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't call backfill_tags. What do you mean? Thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-25 23:34 ` "Andrés G. Aragoneses" @ 2013-11-26 3:06 ` Duy Nguyen 2013-11-26 10:43 ` "Andrés G. Aragoneses" 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2013-11-26 3:06 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: Junio C Hamano, Git Mailing List On Tue, Nov 26, 2013 at 6:34 AM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote: > On 22/11/13 02:18, Duy Nguyen wrote: >> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >>> Have you run the tests with this patch? It seems that it breaks >>> quite a lot of them, including t5500, t5503, t5510, among others. >> >> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call >> could be replaced with >> >> transport_set_option(transport, TRANS_OPT_DEPTH, NULL); >> > > Not sure what you mean, > https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't > call backfill_tags. What do you mean? I wrote "I guess" ;-) I did not check what t5550 does. > > Thanks > -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-26 3:06 ` Duy Nguyen @ 2013-11-26 10:43 ` "Andrés G. Aragoneses" 2013-11-26 11:09 ` Duy Nguyen 0 siblings, 1 reply; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-26 10:43 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano On 26/11/13 04:06, Duy Nguyen wrote: > On Tue, Nov 26, 2013 at 6:34 AM, "Andrés G. Aragoneses" > <knocte@gmail.com> wrote: >> On 22/11/13 02:18, Duy Nguyen wrote: >>> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >>>> Have you run the tests with this patch? It seems that it breaks >>>> quite a lot of them, including t5500, t5503, t5510, among others. >>> >>> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call >>> could be replaced with >>> >>> transport_set_option(transport, TRANS_OPT_DEPTH, NULL); >>> >> >> Not sure what you mean, >> https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't >> call backfill_tags. What do you mean? That was a typo, I meant https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh > I wrote "I guess" ;-) I did not check what t5550 does. Any hint on where to start looking? It doesn't look like any test is using a non-positive depth, so I'm really confused. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] transport: Catch non positive --depth option value 2013-11-26 10:43 ` "Andrés G. Aragoneses" @ 2013-11-26 11:09 ` Duy Nguyen 2013-11-26 11:41 ` [PATCHv4] " "Andrés G. Aragoneses" 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2013-11-26 11:09 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: Git Mailing List, Junio C Hamano On Tue, Nov 26, 2013 at 5:43 PM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote: > On 26/11/13 04:06, Duy Nguyen wrote: >> On Tue, Nov 26, 2013 at 6:34 AM, "Andrés G. Aragoneses" >> <knocte@gmail.com> wrote: >>> On 22/11/13 02:18, Duy Nguyen wrote: >>>> On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >>>>> Have you run the tests with this patch? It seems that it breaks >>>>> quite a lot of them, including t5500, t5503, t5510, among others. >>>> >>>> I guess it's caused by builtin/fetch.c:backfill_tags(). And the call >>>> could be replaced with >>>> >>>> transport_set_option(transport, TRANS_OPT_DEPTH, NULL); >>>> >>> >>> Not sure what you mean, >>> https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't >>> call backfill_tags. What do you mean? > > That was a typo, I meant > https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh > > >> I wrote "I guess" ;-) I did not check what t5550 does. > > Any hint on where to start looking? It doesn't look like any test is > using a non-positive depth, so I'm really confused. Replace die() with "*(char*)0 = 0;" and run t5500, I got a core dump. Running gdb shows this (gdb) bt #0 0x000000000053d98b in set_git_option (opts=0xb63d00, name=0x575767 "depth", value=0x575c91 "0") at transport.c:487 #1 0x000000000053f163 in transport_set_option (transport=0xb63f00, name=0x575767 "depth", value=0x575c91 "0") at transport.c:1000 #2 0x0000000000437b68 in backfill_tags (transport=0xb63f00, ref_map=0xb64d60) at builtin/fetch.c:773 #3 0x0000000000437f91 in do_fetch (transport=0xb63f00, refs=0xb643c0, ref_count=1) at builtin/fetch.c:869 #4 0x00000000004386d4 in fetch_one (remote=0xb63c20, argc=1, argv=0x7fff32f63588) at builtin/fetch.c:1037 #5 0x0000000000438a1d in cmd_fetch (argc=2, argv=0x7fff32f63580, prefix=0x0) at builtin/fetch.c:1115 #6 0x000000000040590f in run_builtin (p=0x7d59a8, argc=4, argv=0x7fff32f63580) at git.c:314 #7 0x0000000000405aa2 in handle_internal_command (argc=4, argv=0x7fff32f63580) at git.c:478 #8 0x0000000000405bbc in run_argv (argcp=0x7fff32f6346c, argv=0x7fff32f63470) at git.c:524 #9 0x0000000000405d61 in main (argc=4, av=0x7fff32f63578) at git.c:607 My guess seems right. -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv4] transport: Catch non positive --depth option value 2013-11-26 11:09 ` Duy Nguyen @ 2013-11-26 11:41 ` "Andrés G. Aragoneses" 2013-11-26 19:09 ` Jonathan Nieder 2013-11-26 22:19 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: "Andrés G. Aragoneses" @ 2013-11-26 11:41 UTC (permalink / raw) To: Git Mailing List, Junio C Hamano, Duy Nguyen >From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> Date: Tue, 26 Nov 2013 12:38:19 +0100 Subject: [PATCH] transport: Catch non positive --depth option value Instead of simply ignoring the value passed to --depth option when it is zero or negative, now it is caught and reported. This will let people know that they were using the option incorrectly (as depth<0 should be simply invalid, and under the hood depth==0 didn't have any effect). (The change in fetch.c is needed to avoid the tests failing because of this new restriction.) Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> Reviewed-by: Duy Nguyen <pclouds@gmail.com> --- builtin/fetch.c | 2 +- transport.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index bd7a101..88c04d7 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map) } transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL); - transport_set_option(transport, TRANS_OPT_DEPTH, "0"); + transport_set_option(transport, TRANS_OPT_DEPTH, NULL); fetch_refs(transport, ref_map); if (gsecondary) { diff --git a/transport.c b/transport.c index 7202b77..5b42ccb 100644 --- a/transport.c +++ b/transport.c @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, opts->depth = strtol(value, &end, 0); if (*end) die("transport: invalid depth option '%s'", value); + if (opts->depth < 1) + die("transport: invalid depth option '%s' (must be positive)", value); } return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv4] transport: Catch non positive --depth option value 2013-11-26 11:41 ` [PATCHv4] " "Andrés G. Aragoneses" @ 2013-11-26 19:09 ` Jonathan Nieder 2013-11-26 22:19 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Jonathan Nieder @ 2013-11-26 19:09 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: Git Mailing List, Junio C Hamano, Duy Nguyen Hi, Thanks for tackling this. This review will be kind of nitpicky, as a way to save time when reviewing future patches. Andrés G. Aragoneses wrote: > From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> > Date: Tue, 26 Nov 2013 12:38:19 +0100 > Subject: [PATCH] transport: Catch non positive --depth option value These lines are redundant next to the mail header, so they can and should be omitted to avoid some noise. > Instead of simply ignoring the value passed to --depth > option when it is zero or negative, now it is caught > and reported. Nit: commit messages usually give a command to the codebase, like this: When the value passed to --depth is zero or negative, instead of treating it as infinite depth, catch and report the mistake. > This will let people know that they were using the > option incorrectly (as depth<0 should be simply invalid, > and under the hood depth==0 didn't have any effect). Ok. Do we know that no one was using --depth=0 this way deliberately? > (The change in fetch.c is needed to avoid the tests > failing because of this new restriction.) Based on the surrounding thread I see that you're talking about the test script t5500 here. Which test failed? How does it use "git fetch"? Does the change just fix the test but keep in broken in production, or does it fix "git fetch" in production, too? > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > Reviewed-by: Duy Nguyen <pclouds@gmail.com> > --- > builtin/fetch.c | 2 +- > transport.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) It would be nice to have a brief test to demonstrate the fix and make sure we don't break it in the future. "grep fetch.*--depth t/*.sh" tells me t5500 would be a good place to put it. For example, something like test_expect_success 'fetch catches invalid --depth values' ' ( cd shallow && test_must_fail git fetch --depth=0 && test_must_fail git fetch --depth=-2 && test_must_fail git fetch --depth= && test_must_fail git fetch --depth=nonsense ) ' What do you think? Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv4] transport: Catch non positive --depth option value 2013-11-26 11:41 ` [PATCHv4] " "Andrés G. Aragoneses" 2013-11-26 19:09 ` Jonathan Nieder @ 2013-11-26 22:19 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2013-11-26 22:19 UTC (permalink / raw) To: Andrés G. Aragoneses; +Cc: Git Mailing List, Duy Nguyen "Andrés G. Aragoneses" <knocte@gmail.com> writes: > From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <knocte@gmail.com> > Date: Tue, 26 Nov 2013 12:38:19 +0100 > Subject: [PATCH] transport: Catch non positive --depth option value Please do not leave these four lines in your e-mail message, unless there is a good reason to do so (e.g. when you are forwarding a patch authored by somebody else, you may want a "From:" that names the real author at the beginning, but that does not apply in this case where you are sending your own). The first line is merely a marker to say the file is a format-patch output, and the header lines are there for those who use "git send-email" to mail the messages out, and/or for those who want to cut & paste some of them (not copy & paste) to their MUA header input widgets. > Instead of simply ignoring the value passed to --depth option when > it is zero or negative, now it is caught and reported. > > This will let people know that they were using the option > incorrectly (as depth<0 should be simply invalid, and under the > hood depth==0 didn't have any effect). > > (The change in fetch.c is needed to avoid the tests failing > because of this new restriction.) Good, but it is not just tests but without that change real operations break. In other words, it is an integral part of the patch, not a workaround for a broken test. Thanks. Will queue with a bit of tweak. > Signed-off-by: Andres G. Aragoneses <knocte@gmail.com> > Reviewed-by: Duy Nguyen <pclouds@gmail.com> > --- > builtin/fetch.c | 2 +- > transport.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index bd7a101..88c04d7 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map) > } > > transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL); > - transport_set_option(transport, TRANS_OPT_DEPTH, "0"); > + transport_set_option(transport, TRANS_OPT_DEPTH, NULL); > fetch_refs(transport, ref_map); > > if (gsecondary) { > diff --git a/transport.c b/transport.c > index 7202b77..5b42ccb 100644 > --- a/transport.c > +++ b/transport.c > @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options *opts, > opts->depth = strtol(value, &end, 0); > if (*end) > die("transport: invalid depth option '%s'", value); > + if (opts->depth < 1) > + die("transport: invalid depth option '%s' (must be positive)", value); > } > return 0; > } ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-11-26 22:19 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-13 16:06 [PATCH] transport: Catch non positive --depth option value "Andrés G. Aragoneses" 2013-11-16 2:58 ` Duy Nguyen 2013-11-18 16:51 ` Junio C Hamano 2013-11-18 22:45 ` [PATCHv2] " "Andrés G. Aragoneses" 2013-11-19 17:15 ` Junio C Hamano 2013-11-21 15:27 ` [PATCHv3] " "Andrés G. Aragoneses" 2013-11-21 17:34 ` Junio C Hamano 2013-11-21 20:18 ` Junio C Hamano 2013-11-22 1:18 ` Duy Nguyen 2013-11-25 23:34 ` "Andrés G. Aragoneses" 2013-11-26 3:06 ` Duy Nguyen 2013-11-26 10:43 ` "Andrés G. Aragoneses" 2013-11-26 11:09 ` Duy Nguyen 2013-11-26 11:41 ` [PATCHv4] " "Andrés G. Aragoneses" 2013-11-26 19:09 ` Jonathan Nieder 2013-11-26 22:19 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).