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