[PATCH 001/103] virJSONValueObjectAddVArgs: Add 'k' convertor for formatting non-negative integers

[Peter Krempa]

On Fri, Oct 08, 2021 at 13:20:54 +0200, Ján Tomko wrote:
> On a Friday in 2021, Kristina Hanicova wrote:
> > On Thu, Oct 7, 2021 at 6:43 PM Ján Tomko <jtomko at redhat.com> wrote:
> > 
> > > On a Thursday in 2021, Peter Krempa wrote:
> > > >In many cases we use a signed value, but use the sign to note that it
> > > >was not assigned. For converting to JSON objects it will be handy to
> > > >have possibility to do this automatically.
> > > >
> > > >Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> > > >---
> > > > src/util/virjson.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > >diff --git a/src/util/virjson.c b/src/util/virjson.c
> > > >index 9adcea4fff..70ea71b505 100644
> > > >--- a/src/util/virjson.c
> > > >+++ b/src/util/virjson.c
> > > >@@ -115,6 +115,7 @@ virJSONValueGetType(const virJSONValue *value)
> > > >  *
> > > >  * i: signed integer value
> > > >  * j: signed integer value, error if negative
> > > >+ * k: signed integer value, omitted if negative
> > > >  * z: signed integer value, omitted if zero
> > > >  * y: signed integer value, omitted if zero, error if negative
> > > >  *
> > > >@@ -189,6 +190,7 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
> > > >
> > > >         case 'z':
> > > >         case 'y':
> > > >+        case 'k':
> > > >         case 'j':
> > > >         case 'i': {
> > > >             int val = va_arg(args, int);
> > > >@@ -200,7 +202,10 @@ virJSONValueObjectAddVArgs(virJSONValue *obj,
> > > >                 return -1;
> > > >             }
> > > >
> > > >-            if (!val && (type == 'z' || type == 'y'))
> > > >+            if (val == 0 && (type == 'z' || type == 'y'))
> > > >+                continue;
> > > >+
> > > 
> > > Please split out this cosmetic style change.
> > > 
> > 
> > Please don't.
> > 
> 
> Why not?
> 
> It is unrelated to the goal of introducing the 'k' parser
> and having it here distracts from that goal.
> 
> Writing a new commit might cost a few seconds of the author's time,
> but it will save time for the readers of such commit. And you can
> expect a commit to be read more times (even by the author a few years
> into the future, after they long forgot about writing it), while it is
> only written once.
> 
> There might be some projects (I think linux kernel does this) that do
> not accept commits with purely  whitespace/style changes, but we do allow
> them in libvirt and I'm much happier to review them, since they make the
> actual functional changes more obvious. (And they make the review of a 100-patch
> series possible).
> 
> I like this blog post danpb wrote on the topic:
> https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/

I'd argue that it might be relevant to have it in the same commit as it
makes it more obvious that 'val' is a number and not a flag, especially
once I'm adding aonther check, but at the same time we already have this
inconsistency few lines above, so I'll remove the cosmetic change for
now.