On Thu, 2014-11-20 at 10:47 -0500, Simo Sorce wrote: > On Thu, 20 Nov 2014 16:47:29 +0200 > Alexander Bokovoy <aboko...@redhat.com> wrote: > > > On Thu, 20 Nov 2014, Nathaniel McCallum wrote: > > >On Thu, 2014-11-20 at 09:12 -0500, Simo Sorce wrote: > > >> On Thu, 20 Nov 2014 12:36:45 +0200 > > >> Alexander Bokovoy <aboko...@redhat.com> wrote: > > >> > > >> > On Wed, 19 Nov 2014, Simo Sorce wrote: > > >> > >----- Original Message ----- > > >> > >> From: "Alexander Bokovoy" <aboko...@redhat.com> > > >> > >[...] > > >> > > > > >> > >> Regarding the patchset itself: > > >> > >> > > >> > >> Patch 0001: fix 'wuld' in the commit message. The rest is > > >> > >> fine. > > >> > > > > >> > >Fixed. > > >> > > > > >> > >> Patch 0002: > > >> > >> - ticket number is missing in the commit message > > >> > > > > >> > >Added. > > >> > > > > >> > >> - perhaps, an instruction how to regenerate asn1 code can be > > >> > >> made a Makefile target? We don't need to call it ourselves > > >> > >> but this would simplify things in future > > >> > > > > >> > >Added make regenerate target to asn1c makefile > > >> > > > > >> > >> - I'm little uncomfortable how ASN_DEBUG() output goes > > >> > >> explicitly to stderr but I guess this is something we > > >> > >> currently cannot override with DS-specific log printing, so > > >> > >> no big deal right now > > >> > > > > >> > >ASN_DEBUG() is currently disabled as EMIT_ASN_DEBUG is > > >> > >undefined, we can later provide a replacement ASN_DEBUG > > >> > >function to hook debugging, but given the same code is used in > > >> > >both DS plugins and ipa-getkeytab binary I did not want to > > >> > >assume anything, and how to wire it up (if we even want to) > > >> > >should probably be discussed at a later time. > > >> > > > > >> > >> - any specific need to get asn1/compile committed? We don't > > >> > >> commit it in the client code (ipa-client/compile). > > >> > > > > >> > >Added 'compile' to .gitignore in second patch > > >> > > > > >> > >> Patch 0003: OK > > >> > > > > >> > >Nothing changed here. > > >> > > > > >> > >I also remembered the patch naming policy :-) so new patch > > >> > >names/numbers are 514,515,516, third revision. > > >> > Thanks. The only complaint I have left is number of whitespace > > >> > errors that git says are in the 515th patch. > > >> > > >> Yeah the autogenerated code is not a pretty sight style-wise, do we > > >> want to run an automatic indenter on it ? > > >> I was hesitant to do so, but I wouldn't mind adding that, if we > > >> feel strongly about it. > > > > > >Let's please not try to correct autogenerated code. > > I'm not tied to this but Simo now thinks it is better to run indenter > > in the generator rule as this will give less problems in actual > > comparison noise that git diff would give. > > > > I'll make sure to talk back to asn1c author to see if we can improve > > its generators upstream. > > So given Nathaniel doesn't like to touch autogenerated code I'll leave > it as it is. > I am going to push with the only change being to remove > asn1/config.h.in~ with was added to the second commit by mistake.
LGTM _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
