Re: [PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 10, 2025, 05:30 UTC
Message-ID: <aOiZ_v3bO35oVWf-@pks.im>
In-Reply-To: <20251009234957.1789543-1-okhuomonajayi54@gmail.com>

On Fri, Oct 10, 2025 at 12:49:57AM +0100, Okhuomon Ajayi wrote:

> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
> ---

For a change like this it is important to explain what the problem is, why it is a problem and how your change improves the code for the better. All of this needs to be patr of the commit message so that the reader can understand what you're actually doing.

Also, if this fixes a real issue, is it possible to demonstrate the issue and the fix with a test?

Show 24 quoted lines

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 606c6aea82..695e1a0ae1 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src, struct strbuf *result, int strict)
>  		return -1;
>  
>  	num = strtoul(src + 1, &endp, 10);
> -	/*
> -	 * NEEDSWORK: check for brokenness other than num > 1400, such as
> -	 *            (num % 100) >= 60, or ((num % 100) % 15) != 0 ?
> -	 */
> -	if (errno || endp == src + 1 || *endp || /* did not parse */
> -	    (strict && (1400 < num))             /* parsed a broken timezone */
> -	   )
> +	
> +
> +        unsigned int hours = num / 100;
> +        unsigned int minutes = num % 100;
> +
> +	if (errno || endp == src + 1 || *endp || 
> +	    (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){
>  		return -1;
> +	}

Despite the formatting issues I also think that this here is becoming hard to read. It may make sense to split this up into multiple conditions.

Thanks!

Patrick