* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.