Hello,
Patrick Steinhardt <ps@xxxxxx> writes:
>> +
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> + if (!strcmp(refname, "HEAD"))
>> + return refs_ref_exists(refs, refname);
>> +
>> + return 0;
>> +}
>
> The same comment applies here, as well.
>
> I also worry a bit about the API we have. It becomes really hard to
> figure out which function to call now as the API surface seems to
> explode. We have:
>
> - is_pseudoref_syntax
> - is_pseudoref
> - is_headref
> - check_refname_format
> - refname_is_safe
>
I also found `is_head()` in 'reflog.c'.
> I wonder whether we can maybe consolidate the interface into one or
> maybe even two functions where the behaviour can be tweaked with a flag
> field.
>
You do bring up an interesting point about the APIs present and I agree
that it would be best to consolidate them to something much simpler and
nicer.
> Something like `refname_is_valid()` with a bunch of flags:
>
> - REFNAME_ACCEPT_HEAD to accept "HEAD"
> - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
> "_HEAD" or being one of the irregular pseudorefs.
> - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
> valid, but which would pass `refname_is_safe()`.
>
> Another alternative could be something like `classify_refname()` that
> accepts a refname and returns an enum saying what kind of ref something
> is.
>
> Given that this topic won't be included in Git v2.44 anymore, I think
> that opening this can of worms would be sensible now.
>
Over the two I do prefer something like the former method of the callee
using flags to state the requirements, this way the function only does
what is necessary between the listed operations.
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
>> I think it's quite confusing that `is_pseudoref()` not only checks
>> whether the refname may be a pseudoref, but also whether it actually
>> exists. Furthermore, why is a pseudoref only considered to exist in case
>> it's not a symbolic ref? That sounds overly restrictive to me.
>
> I am torn on this, but in favor of the proposed naming, primarily
> because is_pseudoref_syntax() was about "does this string look like
> the fullref a pseudoref would have?", and the reason why we wanted
> to have this new function was we wanted to ask "does this string
> name a valid pseudoref?"
>
> Q: Is CHERRY_PICK_HEAD a pseudoref?
> A: It would have been if it existed, but I see only
> $GIT_DIR/CHERRY_PICK_HEAD that is a symbolic link, and it cannot
> be a pseudoref.
>
> I can certainly see a broken out set of helper functions to check
>
> - Does this string make a good fullref for a pseudoref?
> - Does a pseudoref with his string as its fullref exist?
>
> independently. The first one would answer Yes and the second one
> would answer No in such a context.
>
This does work into the flags based mechanism that Patrick mentioned
too. The users of this new function can then only request information as
necessary.
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> + if (ends_with(refname, "_HEAD")) {
>> + refs_resolve_ref_unsafe(refs, refname,
>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> + &oid, NULL);
>> + return !is_null_oid(&oid);
>> + }
>
> FYI. I see
>
> .git/rebase-apply/patch:31: space before tab in indent.
> RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> .git/rebase-apply/patch:32: space before tab in indent.
> &oid, NULL);
> .git/rebase-apply/patch:33: space before tab in indent.
> return !is_null_oid(&oid);
>
> around here.
Thanks for notifying, Jeff mentioned the same and I thought I fixed it.
I'll have a look at why my editor did this and add some steps to avoid
this in the following patches.
Attachment:
signature.asc
Description: PGP signature
