Re: [PATCH 1/2] update-ref: Allow creation of multiple transactions

From: Jeff King <peff@peff.net>
Date: Nov 6, 2020, 17:52 UTC
Message-ID: <20201106175241.GA182486@coredump.intra.peff.net>
In-Reply-To: <xmqqa6vvij37.fsf@gitster.c.googlers.com>

On Thu, Nov 05, 2020 at 01:34:20PM -0800, Junio C Hamano wrote:

Show 22 quoted lines

> > The tests all look quite reasonable to me. Touching .git/refs like this
> > is a bit gross (and something we may have to deal with if we introduce
> > reftables, etc). But it's pretty pervasive in this file, so matching
> > the existing style is the best option for now.
> 
> 
> Shouldn't "git show-ref --verify" be usable portably across ref backends
> to test if a well-formed ref was created (or was not created)?
> 
> On the ref-creation side, there are cases where we need to directly
> futz with the filesystem entity.  For example, "git update-ref"
> cannot be used to place a non-commit at "refs/heads/foo", so
> something like
> 
> 	git rev-parse HEAD^{tree} >.git/refs/heads/bad-branch
> 
> cannot be avoided (this is a tangent but we probably should add a
> way to force setting _any_ value to any ref, that may not even point
> at an existing object or an object of a wrong type, to help test
> scripts).
> 
> But I do not think this is such a case.

Yeah, I agree completely that we could be using rev-parse in this instance. But it's definitely not alone there:

  $ git grep -c test_path_is.*.git/refs t/t1400-update-ref.sh
  t/t1400-update-ref.sh:13

So it is a question of "do an ugly thing that fits in with neighbors" or "be inconsistent but set a good example". And I am OK with either. Of course, "improve the neighbors on top" would be better still. :)

-Peff

PS And yeah, I agree in the long run we may need some mechanism to
   override internal safeties in order to test broken cases with
   reftable. We have sometimes resorted to manually munging on-disk
   files in similar tests (e.g., for broken packs, etc), but it gets
   rather tricky.