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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 12, 2024, 07:45 UTC
Message-ID: <ZaDuEufXOnwH7hT4@framework>
In-Reply-To: <20240112070142.GD618729@coredump.intra.peff.net>

On Fri, Jan 12, 2024 at 02:01:42AM -0500, Jeff King wrote:

Show 31 quoted lines

> On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
> 
> > > (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.

Yup, I've already fixed this bug in the reftable backend.

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

Agreed, I think it's good that the reftable library itself does not print error messages. In this particular case we are already able to provide a proper error message due to the error code that the library returns. But there are certainly going to be other cases where it might make sense to pass in an error strbuf.

Patrick