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

From: Junio C Hamano <[email-masked]>
Date: Tue, 26 Nov 2013 14:19:03 -0800

"Andrés G. Aragoneses" <[email-masked]> writes:

> From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= <[email-masked]>
> 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 <[email-masked]>
> Reviewed-by: Duy Nguyen <[email-masked]>
> ---
>  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;
>  	}