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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 26, 2013, 19:09 UTC
Message-ID: <20131126190902.GB4212@google.com>
In-Reply-To: <529488D5.80605@gmail.com>

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