{"thread":{"id":"64289","subject":"[PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","startedAt":"2025-10-09T23:50:42Z","lastAt":"2025-10-10T17:17:07Z","messageCount":5,"participants":["Okhuomon Ajayi","Kristoffer Haugsbakk","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528435","messageId":"20251009234957.1789543-1-okhuomonajayi54@gmail.com","threadId":"64289","inReplyTo":null,"subject":"[PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-09T23:49:57Z","receivedAt":"2025-10-09T23:50:42Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Signed-off-by: Okhuomon Ajayi \n---\n builtin/fast-import.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 606c6aea82..695e1a0ae1 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n \t\treturn -1;\n \n \tnum = strtoul(src + 1, &endp, 10);\n-\t/*\n-\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n-\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n-\t */\n-\tif (errno || endp == src + 1 || *endp || /* did not parse */\n-\t (strict && (1400 < num)) /* parsed a broken timezone */\n-\t )\n+\t\n+\n+ unsigned int hours = num / 100;\n+ unsigned int minutes = num % 100;\n+\n+\tif (errno || endp == src + 1 || *endp || \n+\t (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){\n \t\treturn -1;\n+\t}\n \n \tstrbuf_addstr(result, orig_src);\n \treturn 0;\n-- \n2.43.0\n\n"},{"id":"528437","messageId":"3fa266f2-2376-4497-9d36-966fbdb4ca0e@app.fastmail.com","threadId":"64289","inReplyTo":"20251009234957.1789543-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-10-09T23:57:18Z","receivedAt":"2025-10-09T23:57:40Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Oct 10, 2025, at 01:49, Okhuomon Ajayi wrote:\n> Signed-off-by: Okhuomon Ajayi \n> ---\n> builtin/fast-import.c | 15 ++++++++-------\n> 1 file changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 606c6aea82..695e1a0ae1 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src,\n> struct strbuf *result, int strict)\n> \t\treturn -1;\n>\n> \tnum = strtoul(src + 1, &endp, 10);\n> -\t/*\n> -\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n> -\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n> -\t */\n> -\tif (errno || endp == src + 1 || *endp || /* did not parse */\n> -\t (strict && (1400 < num)) /* parsed a broken timezone */\n> -\t )\n> +\n> +\n\nThese two new blank lines should probably not be here.\n\n> + unsigned int hours = num / 100;\n> + unsigned int minutes = num % 100;\n> +\n\nYou’re mixing spaces-for-indentation right here with tabs (next). The\nproject uses tabs for indentation.\n\nTry to run `./ci/check-whitespace.sh @^`\n\n> +\tif (errno || endp == src + 1 || *endp ||\n> +\t (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){\n> \t\treturn -1;\n> +\t}\n>\n> \tstrbuf_addstr(result, orig_src);\n> \treturn 0;\n> --\n> 2.43.0\n"},{"id":"528457","messageId":"aOiZ_v3bO35oVWf-@pks.im","threadId":"64289","inReplyTo":"20251009234957.1789543-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T05:30:38Z","receivedAt":"2025-10-10T05:30:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:49:57AM +0100, Okhuomon Ajayi wrote:\n> Signed-off-by: Okhuomon Ajayi \n> ---\n\nFor a change like this it is important to explain what the problem is,\nwhy it is a problem and how your change improves the code for the\nbetter. All of this needs to be patr of the commit message so that the\nreader can understand what you're actually doing.\n\nAlso, if this fixes a real issue, is it possible to demonstrate the\nissue and the fix with a test?\n\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 606c6aea82..695e1a0ae1 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n> \t\treturn -1;\n> \n> \tnum = strtoul(src + 1, &endp, 10);\n> -\t/*\n> -\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n> -\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n> -\t */\n> -\tif (errno || endp == src + 1 || *endp || /* did not parse */\n> -\t (strict && (1400 < num)) /* parsed a broken timezone */\n> -\t )\n> +\t\n> +\n> + unsigned int hours = num / 100;\n> + unsigned int minutes = num % 100;\n> +\n> +\tif (errno || endp == src + 1 || *endp || \n> +\t (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){\n> \t\treturn -1;\n> +\t}\n\nDespite the formatting issues I also think that this here is becoming\nhard to read. It may make sense to split this up into multiple\nconditions.\n\nThanks!\n\nPatrick\n"},{"id":"528515","messageId":"xmqq1pnabw1p.fsf@gitster.g","threadId":"64289","inReplyTo":"aOiZ_v3bO35oVWf-@pks.im","subject":"Re: [PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-10T15:45:06Z","receivedAt":"2025-10-10T15:45:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Oct 10, 2025 at 12:49:57AM +0100, Okhuomon Ajayi wrote:\n>> Signed-off-by: Okhuomon Ajayi \n>> ---\n>\n> For a change like this it is important to explain what the problem is,\n> why it is a problem and how your change improves the code for the\n> better. All of this needs to be patr of the commit message so that the\n> reader can understand what you're actually doing.\n>\n> Also, if this fixes a real issue, is it possible to demonstrate the\n> issue and the fix with a test?\n>\n>> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n>> index 606c6aea82..695e1a0ae1 100644\n>> --- a/builtin/fast-import.c\n>> +++ b/builtin/fast-import.c\n>> @@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n>> \t\treturn -1;\n>> \n>> \tnum = strtoul(src + 1, &endp, 10);\n>> -\t/*\n>> -\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n>> -\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n>> -\t */\n>> -\tif (errno || endp == src + 1 || *endp || /* did not parse */\n>> -\t (strict && (1400 < num)) /* parsed a broken timezone */\n>> -\t )\n>> +\t\n>> +\n>> + unsigned int hours = num / 100;\n>> + unsigned int minutes = num % 100;\n>> +\n>> +\tif (errno || endp == src + 1 || *endp || \n>> +\t (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){\n>> \t\treturn -1;\n>> +\t}\n>\n> Despite the formatting issues I also think that this here is becoming\n> hard to read. It may make sense to split this up into multiple\n> conditions.\n>\n> Thanks!\n>\n> Patrick\n\nThanks for a good suggestion.\n\nThere is another thing we should be aware of about these NEEDSWORK\ncomments. Often, the task a NEEDSWORK comment suggests includes and\nstarts from assessing if the task indeed is worth doing. We should\nread a NEEDSWORK comment like above one as its author mumbling to\nthemselves: this feels lacking, and we may want to do more here,\nlike X and Y and Z. Maybe not.\n\nDo we need to check even more precisely here? What's the point of\ndoing so, and doing so here at this point in the control flow? Are\nthere better approaches than incrementally adding more of similar\nkinds of checks?\n\nWithout being able to answer these questions oneself, one shouldn't\nbe blindly following what a NEEDSWORK comment like this floats as\n\"ideas to do more\".\n\nThe current code may turn out to be good enough. Removing the\nNEEDSWORK comment with a solid answer to the question it poses in\nthe proposed log message would be a commit worth making in such a\ncase.\n\nThanks.\n"},{"id":"528527","messageId":"CAFpMFfCfLZgUDnZBWg7kmGN84Y7gTxOm6SYe7F2__t=hHhBZow@mail.gmail.com","threadId":"64289","inReplyTo":"xmqq1pnabw1p.fsf@gitster.g","subject":"Re: [PATCH] [Outreachy] patch-ids: fix NEEDSWORK timezone parsing in fast-import.c","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-10T17:16:55Z","receivedAt":"2025-10-10T17:17:07Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Thanks for the detailed explanation, Junio, and also thanks Patrick\nand Kristoffer for the earlier feedback.\nThat makes sense\n I’ll review whether the additional timezone checks are actually\nnecessary and if they improve correctness in real cases. I’ll also\nupdate the commit message to better describe the motivation, and fix\nthe formatting issues mentioned.\nOnce I’ve clarified the behavior and possibly added a test case, I’ll\nsend a v2 patch.\nThanks again for the guidance!\n\nOn Fri, Oct 10, 2025 at 4:45 PM Junio C Hamano wrote:\n>\n> Patrick Steinhardt writes:\n>\n> > On Fri, Oct 10, 2025 at 12:49:57AM +0100, Okhuomon Ajayi wrote:\n> >> Signed-off-by: Okhuomon Ajayi \n> >> ---\n> >\n> > For a change like this it is important to explain what the problem is,\n> > why it is a problem and how your change improves the code for the\n> > better. All of this needs to be patr of the commit message so that the\n> > reader can understand what you're actually doing.\n> >\n> > Also, if this fixes a real issue, is it possible to demonstrate the\n> > issue and the fix with a test?\n> >\n> >> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> >> index 606c6aea82..695e1a0ae1 100644\n> >> --- a/builtin/fast-import.c\n> >> +++ b/builtin/fast-import.c\n> >> @@ -1959,14 +1959,15 @@ static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n> >> return -1;\n> >>\n> >> num = strtoul(src + 1, &endp, 10);\n> >> - /*\n> >> - * NEEDSWORK: check for brokenness other than num > 1400, such as\n> >> - * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n> >> - */\n> >> - if (errno || endp == src + 1 || *endp || /* did not parse */\n> >> - (strict && (1400 < num)) /* parsed a broken timezone */\n> >> - )\n> >> +\n> >> +\n> >> + unsigned int hours = num / 100;\n> >> + unsigned int minutes = num % 100;\n> >> +\n> >> + if (errno || endp == src + 1 || *endp ||\n> >> + (strict && (num > 1400 || minutes >=60 || minutes % 15 != 0))){\n> >> return -1;\n> >> + }\n> >\n> > Despite the formatting issues I also think that this here is becoming\n> > hard to read. It may make sense to split this up into multiple\n> > conditions.\n> >\n> > Thanks!\n> >\n> > Patrick\n>\n> Thanks for a good suggestion.\n>\n> There is another thing we should be aware of about these NEEDSWORK\n> comments. Often, the task a NEEDSWORK comment suggests includes and\n> starts from assessing if the task indeed is worth doing. We should\n> read a NEEDSWORK comment like above one as its author mumbling to\n> themselves: this feels lacking, and we may want to do more here,\n> like X and Y and Z. Maybe not.\n>\n> Do we need to check even more precisely here? What's the point of\n> doing so, and doing so here at this point in the control flow? Are\n> there better approaches than incrementally adding more of similar\n> kinds of checks?\n>\n> Without being able to answer these questions oneself, one shouldn't\n> be blindly following what a NEEDSWORK comment like this floats as\n> \"ideas to do more\".\n>\n> The current code may turn out to be good enough. Removing the\n> NEEDSWORK comment with a solid answer to the question it poses in\n> the proposed log message would be a commit worth making in such a\n> case.\n>\n> Thanks.\n"}]}