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

On Fri, 22 Sep 2017 14:01:04 -0700
Brandon Williams [off-list ref] wrote:

quoted

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

quoted

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