[Rpm-maint] [PATCH] Extend %changelog to support full timestamps

Igor Gnatenko ignatenko at redhat.com
Fri Oct 7 10:58:19 UTC 2016


On Fri, Oct 7, 2016 at 12:33 PM, Pavlina Varekova <varekpaf at seznam.cz> wrote:
> Od: Pavlina Varekova <varekpaf at seznam.cz>Od: Panu Matilainen
> <pmatilai at laiskiainen.org>
>
> On 10/06/2016 05:36 PM, Florian Festi wrote:
>
>> On 10/06/2016 02:33 PM, Florian Weimer wrote:
>>> On 10/06/2016 09:10 AM, Pavlina Varekova wrote:
>>>> Extend %changelog to support full timestamps
>>>>
>>>> The newly accepted date format is
>>>>
>>>> Mon Jan 6 09:02:22 CEST 2016
>>>>
>>>> (like output of "date" command). Original format "Mon Jun 6 2016" is
>>>> still
>>>> supported.
>>>
>>> I think it would be better to use ISO 8601 format (perhaps with space
>>> instead of 'T' as the date/time separator). The week day information is
>>> redundant and only causes consistency issues.
>>
>> I am really not a fan of the format but it is what's already implemented
>> in rpm - especially on output with the :date and :day formats (which
>> basically do date %a %b %d %Y and date %c). So this is the right way to
>> go. I agree that the days of week are somehow redundant and an
>> annoyance. But they ca be made optional later in another patch.
>>
>> Wrt the actual code: It's just a minor nit pick, but there are several
>> lines duplicated in the two if / else blocks. Parsing the year and
>> generating the final result can probably be done outside of the if else
>> and those saving a few lines.
>
> Yup, copy-paste code is bad even in small quantities, it'll bite back
> one day.
>
> Another minor nit:
>
> /*
> * This is the rpm multi-line comment style, for better
> * or worse.
> */
>
> Bonus points available by making the code more readable - cramming
> multiple statements and if-actions etc into a single line is not a good
> practise. The existing code doing something stupid is not a good reason
> to duplicate it in new code :)
>
> A whole other perspective on the parsing: is there an actual reason
> strptime() cannot be used? It's not like I've tried, but I'd think you
> could just call it with couple of different formats, or maybe even make
> the "extended" format configurable via a macro.
>
> - Panu -
>
>
> Hello,
> from my point it seems the format which I use could be there because the
> majority is ok with it. Or am I wrong?
> I will test whether strptime function can be used, - thanks for the tip. If
> it can be used I will rewrite the patch using it. If not I will incorporate
> the other Florian and Panu comments.
>
>
> Thanks a lot for really fast feedback.
>
>
> Pavlina
>
>
> I fixed the "CEST 2016 in author field" bug in [1], thanks for spotting it.
> I will add this to the pull request once I incorporate the strptime function
> (it's usage has one side-effect: it is locale sensitive but I assume
> changelog timestamp must be always in "C" locale not in e.g. fr_FR - it
> would be quite complex to assume any locale in input).
I suppose it should be always POSIX, yes.
>
>
> Pavlina
>
>
> [1]
> https://github.com/pavlinamv/rpm/commit/c95c6ff86df0e14bd85d0d746dd148e2457a52b2
>
>
> _______________________________________________
> Rpm-maint mailing list
> Rpm-maint at lists.rpm.org
> http://lists.rpm.org/mailman/listinfo/rpm-maint
>



-- 
-Igor Gnatenko


More information about the Rpm-maint mailing list