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

[Jonathan Tan <jonathantanmy@google.com>]

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

> > +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.

> > +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.