Re: [PATCH 4/6] update-ref: add support for 'symref-create' command
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- May 19, 2024, 14:01 UTC
- Message-ID
- <CAOLa=ZQzxax0GoCKMTNxdwy+0f+nU3HFC_ixp24RXgW0+jeXWQ@mail.gmail.com>
- In-Reply-To
- <ZkXpbLPGgdqy6YJk@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
Show 23 quoted lines
> On Tue, May 14, 2024 at 02:44:09PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>> diff --git a/refs.c b/refs.c
>> index c2c9889466..6b724343fe 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1302,15 +1302,16 @@ int ref_transaction_update(struct ref_transaction *transaction,
>> int ref_transaction_create(struct ref_transaction *transaction,
>> const char *refname,
>> const struct object_id *new_oid,
>> + const char *new_target,
>> unsigned int flags, const char *msg,
>> struct strbuf *err)
>> {
>> - if (!new_oid || is_null_oid(new_oid)) {
>> - strbuf_addf(err, "'%s' has a null OID", refname);
>> + if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
>> + strbuf_addf(err, "'%s' has a null OID or no new target", refname);
>> return 1;
>> }
>
> Shouldn't this be "'%s' has neither an OID nor a target"?
>Well it's actually all three. If new_oid is not set, or if new_oid is NULL or there is no new_target.
I think "'%s' has neither a valid OID nor a target" would be best.
> Also, we again miss `new_oid && new_target`. >
Will fix for all the commands!
Show 21 quoted lines
>> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh >> index a390cffc80..cc7e20431e 100755 >> --- a/t/t0600-reffiles-backend.sh >> +++ b/t/t0600-reffiles-backend.sh >> @@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' >> esac >> ' >> >> +test_expect_success SYMLINKS 'symref transaction supports symlinks' ' >> + test_when_finished "git symbolic-ref -d TESTSYMREFONE" && >> + git update-ref refs/heads/new @ && >> + test_config core.prefersymlinkrefs true && >> + cat >stdin <<-EOF && >> + start >> + symref-create TESTSYMREFONE refs/heads/new > > Let's future proof this and create the ref with a name that matches our > root ref restrictions, like "TEST_SYMREF_HEAD". We do plan to enforce > those soonish, so these tests would break. > > Patrick
Makes sense, will fix. Thanks.