transport: Catch non positive --depth option value
PATCH 1/1 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 mean 'no depth' or 'no history' but 'full depth' instead).
--- transport.c | 2 ++ 1 file changed, 2 insertions(+)
transport.c +2 -0
@@ -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 ; }
Signature · 1 line
1.8.1.2
Quoted 8 lines · Andrés G. Aragoneses
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.
Quoted 24 lines
Signed-off-by Andres G. Aragoneses <[email protected]>--- transport.c | 2 ++ 1 file changed, 2 insertions(+)transport.c +2
-0@@ -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
Signature · 1 line
Duy
Quoted 8 lines · Andrés G. Aragoneses
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.
Quoted 19 lines
Signed-off-by Andres G. Aragoneses <[email protected]>--- transport.c | 2 ++ 1 file changed, 2 insertions(+)transport.c +2
-0@@ -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 ;}
PATCH v2 1/1 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).
--- transport.c | 2 ++ 1 file changed, 2 insertions(+)
transport.c +2 -0
@@ -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 ; }
Signature · 1 line
1.8.1.2
Quoted 20 lines · Andrés G. Aragoneses
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 <[email protected]>Reviewed-by Duy Nguyen <[email protected]>Reviewed-by Junio C Hamano <[email protected]>--- transport.c | 2 ++ 1 file changed, 2 insertions(+)transport.c +0
-0
transport.c static int set_git_option(struct · line 489
Quoted 7 lines
transport.c +2
-0@@ -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?
Quoted 3 lines
transport.c +0
-0}return 0; }
Linewrapped and whitespace damaged.
PATCH v3 1/1 transport: Catch non positive --depth option value
Quoted 1 line
From
99e387151594572dc136bf1fae45593ee710e817Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
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).
--- transport.c | 2 ++ 1 file changed, 2 insertions(+)
transport.c +2 -0
@@ -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 ; }
Signature · 1 line
1.8.1.2
Quoted 9 lines · Andrés G. Aragoneses
From
99e387151594572dc136bf1fae45593ee710e817Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
Date: Wed, 13 Nov 2013 16:55:08 +0100
Subject: [PATCH] transport: Catch non positive --depth option valueInstead of simply ignoring the value passed to --depth option when it is zero or negative, now it is caught and reported.
Quoted 7 lines
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 <[email protected]>Reviewed-by Duy Nguyen <[email protected]>Reviewed-by Junio C Hamano <[email protected]>
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.
Quoted 17 lines
--- transport.c | 2 ++ 1 file changed, 2 insertions(+)transport.c +2
-0@@ -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 ; }
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Andrés G. Aragoneses
From
99e387151594572dc136bf1fae45593ee710e817Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
Date: Wed, 13 Nov 2013 16:55:08 +0100
Subject: [PATCH] transport: Catch non positive --depth option valueInstead 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).
Quoted 8 lines
Quoted 3 lines
Signed-off-by Andres G. Aragoneses <[email protected]>Reviewed-by Duy Nguyen <[email protected]>Reviewed-by Junio C Hamano <[email protected]>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);
Quoted 19 lines
Quoted 18 lines
--- transport.c | 2 ++ 1 file changed, 2 insertions(+)transport.c +2
-0@@ -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 ; }
Signature · 1 line
Duy
Quoted 9 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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
Quoted 8 lines · Andrés G. Aragoneses
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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
Quoted 6 lines
Quoted 2 lines
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 callbackfill_tags. What do you mean?
I wrote "I guess" ;-) I did not check what t5550 does.
Quoted 3 lines
Thanks
Signature · 1 line
Duy
Quoted 10 lines · Duy Nguyen
Quoted 8 lines · Andrés G. Aragoneses
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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 callbackfill_tags. What do you mean?
That was a typo, I meant
https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh
Quoted 1 line
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.
Quoted 18 lines · Andrés G. Aragoneses
Quoted 10 lines · Duy Nguyen
Quoted 8 lines · Andrés G. Aragoneses
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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 callbackfill_tags. What do you mean?
Quoted 8 lines
That was a typo, I meant
https://github.com/git/git/blob/master/t/t5500-fetch-pack.shQuoted 1 line
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.
Signature · 1 line
Duy
PATCH v4 1/1 transport: Catch non positive --depth option value
Quoted 1 line
From
4f3b24379090b7b69046903fba494f3191577b20Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
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.)
--- builtin/fetch.c | 2 +- transport.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
builtin/fetch.c +1 -1
@@ -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) {
transport.c +2 -0
@@ -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 ; }
Signature · 1 line
1.8.1.2
Hi,
Thanks for tackling this. This review will be kind of nitpicky, as a way to save time when reviewing future patches.
Quoted 4 lines · Andrés G. Aragoneses
From
4f3b24379090b7b69046903fba494f3191577b20Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
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.
Quoted 3 lines
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.
Quoted 3 lines
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?
Quoted 2 lines
(The change in
fetch.cis 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?
Quoted 6 lines
Signed-off-by Andres G. Aragoneses <[email protected]>Reviewed-by Duy Nguyen <[email protected]>--- 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
Quoted 4 lines · Andrés G. Aragoneses
From
4f3b24379090b7b69046903fba494f3191577b20Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email protected]>
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.
Quoted 9 lines
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.cis 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.
Quoted 33 lines
Signed-off-by Andres G. Aragoneses <[email protected]>Reviewed-by Duy Nguyen <[email protected]>--- builtin/fetch.c | 2 +- transport.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)builtin/fetch.c +1
-1@@ -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) {transport.c +2
-0@@ -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 ; }
thanks.