From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id CC9071F9E0 for ; Thu, 23 Apr 2020 20:42:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726160AbgDWUl4 (ORCPT ); Thu, 23 Apr 2020 16:41:56 -0400 Received: from smtp.hosts.co.uk ([85.233.160.19]:46081 "EHLO smtp.hosts.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725884AbgDWUl4 (ORCPT ); Thu, 23 Apr 2020 16:41:56 -0400 Received: from [92.30.123.115] (helo=[192.168.1.39]) by smtp.hosts.co.uk with esmtpa (Exim) (envelope-from ) id 1jRifP-000BFe-4z; Thu, 23 Apr 2020 21:41:52 +0100 Subject: Re: [PATCH v3 1/2] date.c: skip fractional second part of ISO-8601 To: Junio C Hamano , Danh Doan Cc: git@vger.kernel.org, "Brian M . Carlson" References: <20200423011812.GA1930@danh.dev> From: Philip Oakley Message-ID: <1861c472-7756-d433-9185-d83c03d72b9b@iee.email> Date: Thu, 23 Apr 2020 21:41:49 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-GB Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 23/04/2020 20:28, Junio C Hamano wrote: > Danh Doan writes: > >>>> if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) { >>>> tm->tm_hour = num; >>>> tm->tm_min = num2; >>> And after all that is done, if (and others) are within a >>> reasonable range, we use that as HH:MM:SS. >>> >>> OK. If (or , or even for that matter) weren't >>> reasonable, is it still sensible to discard the fractional part? >>> The answer is not immediately obvious to me. >>> >>> To be safe, it might make sense to extract a helper function from >>> the next conditional, i.e. >>> >>> static int is_hms(long num1, long num2, long num3) >> I'll make it `is_time` on par with is_date check. > That is probably a lot more readable name than is_hms(). I didn't immediately recognise hms and ymd (below) as abbreviations. > > I do not worry too much if the name is not "on par with" it, though, > because is_date() does more than just "check", as you noticed below, > unlike the "is hour-minute-seconds are within reasonable range?" > check. > >> I'll look into it and check if int or long is suitable for parameter's >> type. >> >>> { >>> return (0 <= num1 && num1 < 25 && >>> 0 <= num2 && num2 < 60 && >>> 0 <= num3 && num3 <= 60); >> Does it worth to add an explicit comment that we intentionally ignore >> the old-and-defective 2nd leap seconds (i.e. second with value 61)? >> >> I saw in one of your previous email doubting if we should check for >> `num3 <= 61` or not. > I wrote that without checking anything, even what our own code does. > As the existing code seems to want to work only with a second part > that is 60 or less, not allowing a minute with 62 seconds, I think > sticking to that and saying "0 <= num3 && num3 <= 60" is good. > >>> } >>> >>> and use it in the new "else if" block like so? >>> >>> >>> } else if (*end == '.' && isdigit(end[1]) && >>> is_date(tm->tm_year, tm->tm_mon, tm->tm_mday, NULL, now, tm) && >> When running into this, the code patch for non-approxidate hasn't >> initialised value for now, yet. > We may want to separate the logic that relies on the value of 'now' > and 'now_tm->tm_year' out of is_date() to make it more easily > reusable. In this generic codepath, for example, we do not > necessarily want to say "we refuse to parse timestamp that is 10 > days or more into the future". > > The longer I stare at is_date(), the more I am inclined to say it is > a bit of impedance mismatch, and we instead should have the is_hms() > helper as I suggested in the message you are responding to, plus > something like: Would is_hhmmss() and is_yyyymmdd() be more obvious abbreviations for most readers? Now that I type them, they do feel that bit too long... , as naming is hard, maybe stick with the yms and hms, though I do keep wanting to type ytd for the former.. > > static int is_ymd(int year, int month, int day) > { > /* like tm_to_time_t(), we only work between 1970-2099 */ > static const int days_in_month[] = { > 31, 28, 31, 30, ..., 31 > }; > > return !(year < 1970 || 2100 <= year || > month <= 0 || 13 <= month || > day <= 0 || year / 4 + days_in_month[month-1] <= day); > } > > and use it here. I am not sure if we can reuse this inside > is_date(), but if we can do so that would be good (and if we cannot, > that is fine, too). > > Thanks. -- Philip