Re: [PATCH] SortSupport for macaddr type

On Tue, Feb 28, 2017 at 08:58:24AM -0800, Brandur Leach wrote:
> Hi Julien,
>

Hello Brandur,

> Thanks for the expedient reply, even after I'd dropped the
> ball for so long :)

:)

> Cool! I just re-read my own comment a few days later and I
> think that it still mostly makes sense, but definitely open
> to other edits if anyone else has one.
>

Ok.

> > One last thing, I noticed that you added:
> >
> > +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);
> >
> > but the existing function is declared as
> >
> > static int32
> > macaddr_internal_cmp(macaddr *a1, macaddr *a2)
> >
> > I'd be in favor to declare both as int.
>
> Great catch! I have no idea how I missed that. I've done as
> you suggested and made them both "int", which seems
> consistent with SortSupport implementations elsewhere.
>

Great.

> > After this, I think this patch will be ready for committer.
>
> Excellent! I've attached a new (and hopefully final)
> version of the patch.
>
> Two final questions about the process if you'd be so kind:
>
> * Should I change the status on the Commitfest link [1] or
> do I leave that to you (or someone else like a committer)?
>

This is is done by the reviewer, after check of the last patch version. I just
changed the status to ready for committer!

> * I've been generating a new OID value with the
> `unused_oids` script, but pretty much every time I rebase
> I collide with someone else's addition and need to find a
> new one. Is it better for me to pick an OID in an exotic
> range for my final patch, or that a committer just finds
> a new one (if necessary) as they're putting it into
> master?

I think picking the value with unused_oids as you dd is the right thing to do.
As Robert said, if this oid is used in another patch in the meantime, updating
it at commit time is not a big deal. Moreover, this patch will require a
catversion bump, which is meant to be done by the committer.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org