Re: [PATCH v4] connect: in ref advertisement, shallows are last

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 22, 2017, 22:16 UTC
Message-ID: <20170922151635.9c058ce5a06c67ed9610fc6e@google.com>
In-Reply-To: <20170922210104.GB24036@google.com>

On Fri, 22 Sep 2017 14:01:04 -0700 Brandon Williams <bmwill@google.com> wrote:

Show 7 quoted lines

> > +static void process_capabilities(int len)
> > +{
> > +	int nul_location = strlen(packet_buffer);
> 
> It may make more sense to not rely on accessing a global buffer here
> directly and instead pass in the buff you're working on, much like your
> are doing with len.

I wanted to preserve the existing code's behavior of using the global buffer, and it didn't make sense for me to alias it (like the existing code does).

I pass len in because I need to read beyond NUL.

> I'm not the biggest fan of dynamically allocating this and then using it
> to compare.  Maybe we can check to make sure that the oid matches the
> null_oid and that the name matches the "capabilities^{}" string?  That
> way you can avoid the allocation?

The dynamic allocation happens only once per process, since it is static. To check the oid matches null_oid, I would have to parse it first, and that seemed unnecessary.

Ideally I would just check again "000...000 capabilities^{}", but writing it in source code would be error-prone, I think.

Show 6 quoted lines

> > +static int process_ref(struct ref ***list, unsigned int flags,
> > +		       struct oid_array *extra_have)
> 
> So from comparing this to the current code it doesn't look like there is
> a check in 'process_ref' that ensures that a 'capabilities^{}' line
> doesn't show up after a normal ref, or am I missing something?

Ah...yes, you're right. I'll fix this by adding a check in process_ref().

This is getting more complicated than I thought, so I'll wait a while for other comments before sending out an updated patch.