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@xxxxxxxxx> > 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@xxxxxxxxx> > Reviewed-by: Duy Nguyen <pclouds@xxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html