Bug#848721: apt: please make the moo reproducible

[Chris Lamb]

David Kalnischkies wrote:

> Good day mere mortal

I must say I very much enjoyed receiving this code review…  *grin*

> > Patch attached.
> 
> … but your sacrifice is not enough.

Hah! Okay, updated patch attached:

> On a nitpick level the cow feels kinda tricked by a method named
> GetTimeNow […] GetSecondsSinceEpoch() might be better.

Renamed.

> The cow suggests passing time forward as parameter.

I did initially try this but it makes the code/patch rather ugly in
that one needs to pass the time_t value to DooMooX as well as to
getMooLine and printMooLine…  Yuck.

(Your good point about generating errors has been addressed by simply
calling exit(2) on error; if the value is bad, let's just bail out.)

> > +   if (!source_date_epoch) {
> 
> The cow dislikes the '!' as it is easy to miss and prefers the far
> noisier "== nullptr" here.

Agreed.

> > +   const unsigned long long epoch = strtoull(source_date_epoch, &endptr, 10);
> 
> The speaker wonders how reproducible such a call is if it is effected by
> LANG – or is that just "bad environment setup"?

(It is not.)

> And while we are at it a minor nitpick: We prefer the const-modifier to be set
> after the type.

Updated throughout.

> > +   if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
> > +         || (errno != 0 && epoch == 0)) {
> > +      _error->Error("SOURCE_DATE_EPOCH: strtoull: %s\n", strerror(errno));
> 
> Using _error->Errno here would be more consistent.

So, I've made some bigger changes around here:

a) Use std::stringstream to parse the input. This is C++ after all
   and means we don't need a cast at the end of the method, the code
   is much shorter and far more readable. (eg. ss.eof() instead of
   checking for a null pointer, etc.)

b) Call exit(2) on failure as described above.

c) Use _error->Fatal as its now a fatal error due to "b".


Regards,

-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      lamby at debian.org / chris-lamb.co.uk
       `-
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-the-moo-reproducible-Closes-848721.patch
Type: text/x-patch
Size: 2913 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/reproducible-bugs/attachments/20161231/1998eaa2/attachment.bin>