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

[Panu Matilainen]

On 10/06/2016 10:10 PM, Pavlina Varekova wrote:
> 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 haven't really formed an opinion one way or the other :) I dont find 
it horrible but then I wonder about the reasoning for this particular 
format. The input is far more likely to come from eg git than date 
command, so maybe being compatible with git default date format would be 
more useful. Just a thought - I really dunno.

A bigger problem is that the current code isn't quite right, in the new 
format the timezone and year "leak" into the author field. This %changelog:

* Thu Oct 06 22:09:09 CEST 2016 Panu Matilainen <pmatilai at laiskiainen.org>
- new

* Thu Oct 06 2016 Panu Matilainen <pmatilai at laiskiainen.org>
- old

...becomes this in the actual package:

rpm -qp --qf "[%{changelogtime} - %{changelogname}\n]" 
/home/pmatilai/rpmbuild/RPMS/x86_64/foo-1.0-1.fc24.x86_64.rpm
1475784549 - CEST 2016 Panu Matilainen <pmatilai at laiskiainen.org>
1475755200 - Panu Matilainen <pmatilai at laiskiainen.org>


> 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.

strptime() with a macro-configurable format would largely eliminate the 
whole problem of having to decide on a format. And of course get rid of 
the custom date parsing code, another benefit.

	- Panu -

>
> Thanks a lot for really fast feedback.
>
>
> Pavlina
>
>
>
>
>
> _______________________________________________
> Rpm-maint mailing list
> Rpm-maint at lists.rpm.org
> http://lists.rpm.org/mailman/listinfo/rpm-maint
>