threads / patch / 35332

patchtransport: Catch non positive --depth option value

Subject: [PATCH] transport: Catch non positive --depth option value

## tl;dr

16 messages between Nov 13, 2013 and Nov 26, 2013. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Andrés G. Aragoneses· Nov 13, 2013, 16:06 UTC · lore

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(+)
Show changes to transport.c +2 −0
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 Nguyen· Nov 16, 2013, 02:58 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCH] transport: Catch non positive --depth option value

On Wed, Nov 13, 2013 at 11:06 PM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote:

Show 8 quoted 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 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.

Show 24 quoted lines
>
> 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
Junio C Hamano· Nov 18, 2013, 16:51 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCH] transport: Catch non positive --depth option value

"Andrés G. Aragoneses" <knocte@gmail.com> writes:
Show 8 quoted 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 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.
Show 19 quoted lines
> 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;
>  	}
Andrés G. Aragoneses· Nov 18, 2013, 22:45 UTC · re: Junio C Hamano · lore

[PATCHv2] 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(+)
Show changes to transport.c +2 −0
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
Junio C Hamano· Nov 19, 2013, 17:15 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv2] transport: Catch non positive --depth option value

"Andrés G. Aragoneses" <knocte@gmail.com> writes:
Show 27 quoted 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).
>
> 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.
Andrés G. Aragoneses· Nov 21, 2013, 15:27 UTC · re: Junio C Hamano · lore

[PATCHv3] transport: Catch non positive --depth option value

>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(+)
Show changes to transport.c +2 −0
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
Junio C Hamano· Nov 21, 2013, 20:18 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

"Andrés G. Aragoneses" <knocte@gmail.com> writes:
Show 16 quoted lines
> 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.

Show 17 quoted lines
> ---
>  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 Nguyen· Nov 22, 2013, 01:18 UTC · re: Junio C Hamano · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> "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);
Show 19 quoted lines
>
>> ---
>>  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
Andrés G. Aragoneses· Nov 25, 2013, 23:34 UTC · re: Duy Nguyen · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

On 22/11/13 02:18, Duy Nguyen wrote:
Show 9 quoted lines
> 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
Duy Nguyen· Nov 26, 2013, 03:06 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

On Tue, Nov 26, 2013 at 6:34 AM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote:

Show 14 quoted lines
> 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
Andrés G. Aragoneses· Nov 26, 2013, 10:43 UTC · re: Duy Nguyen · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

On 26/11/13 04:06, Duy Nguyen wrote:
Show 16 quoted lines
> 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.

Duy Nguyen· Nov 26, 2013, 11:09 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv3] transport: Catch non positive --depth option value

On Tue, Nov 26, 2013 at 5:43 PM, "Andrés G. Aragoneses" <knocte@gmail.com> wrote:

Show 26 quoted lines
> 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
Andrés G. Aragoneses· Nov 26, 2013, 11:41 UTC · re: Duy Nguyen · lore

[PATCHv4] transport: Catch non positive --depth option value

>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(-)
Show changes to 2 files +3 −1

builtin/fetch.c, transport.c

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
Jonathan Nieder· Nov 26, 2013, 19:09 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv4] transport: Catch non positive --depth option value

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?

Show 6 quoted lines
> 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

Junio C Hamano· Nov 26, 2013, 22:19 UTC · re: Andrés G. Aragoneses · lore

Re: [PATCHv4] transport: Catch non positive --depth option value

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

Show 9 quoted 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.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.
Show 33 quoted lines
> 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;
>  	}

← back to recent threads