Re: [PATCH 1/2] t1401: generalize reference locking

From: Jeff King <peff@peff.net>
Date: Jan 12, 2024, 07:01 UTC
Message-ID: <20240112070142.GD618729@coredump.intra.peff.net>
In-Reply-To: <ZZ_MPK2huH2j6CGd@tanuki>

On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:

Show 16 quoted lines

> > (note that you get a different error message if the refs are packed,
> > since there we can notice the d/f conflict manually).
> 
> If all we care for is the exit code then this would work for the
> reftable backend, too:
> 
> ```
> $ git init --ref-format=reftable repo
> Initialized empty Git repository in /tmp/repo/.git/
> $ cd repo/
> $ git commit --allow-empty --message message
> [main (root-commit) c2512d3] x
> $ git symbolic-ref refs/heads refs/heads/foo
> $ echo $?
> 1
> ```

Yep, exactly. That should work for both and cover what the test was originally trying to do.

> A bit unfortunate that there is no proper error message in that case,
> but that is a different topic.

Yeah, I would call that an outright bug. It does not have to be part of this patch, but is worth fixing (and testing). I suspect it's not going to be the only place with this problem. Most of the files-backend ref code is very happy to spew to stdout using error(), but the reftable code, having been written from a more lib-conscious perspective, probably doesn't.

The obvious quick fix is to sprinkle more error() into the reftable code. But in the longer term, I think the right direction is that the ref code should accept an error strbuf or similar mechanism to propagate human-readable error test to the caller.

-Peff