Hi,

Thomas Gummerer wrote:

The get_oid_hex_from_objpath takes care of creating a oid from a
pathname.  It does this by memcpy'ing the first two bytes of the path to
the "hex" string, then skipping the '/', and then copying the rest of the
path to the "hex" string.  Currently it fails to increase the pointer to
the hex string, so the second memcpy invocation just mashes over what
was copied in the first one, and leaves the last two bytes in the string
uninitialized.

Wow.  The fix is obviously correct.

This breaks valgrind in t5540, although the test passes without
valgrind:

[...]

Signed-off-by: Thomas Gummerer <redacted>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Would it be straightforward to add a correctness test for this?  It
seems like this code path didn't work at all and no one noticed.

This is the code path in http-push.c which says

 /*
  * NEEDSWORK: remote_ls() ignores info/refs on the remote side.  But it
  * should _only_ heed the information from that file, instead of trying to
  * determine the refs from the remote file system (badly: it does not even
  * know about packed-refs).
  */
 static void remote_ls(const char *path, int flags,

I think the problem is that when it fails, we end up thinking that
there are *fewer* objects than are actually present remotely so the
only ill effect is pushing too much.  So this should be observable in
server logs (i.e. it is testable) but it's not a catastrophic failure
which means it's harder to test than it would be otherwise.

Moreover, this is in the webdav-based "dumb http" push code path,
which I do not trust much at all.  I wonder if we could retire it
completely (or at least provide an option to turn it off).

Thanks,
Jonathan