Hello Junio,
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
>> because the function is also used by `is_current_worktree_ref()` and
>> making it stricter to match only known pseudorefs might have unintended
>> consequences due to files like 'BISECT_START' which isn't a pseudoref
>> but sometimes contains object ID.
>
> Well described.
>
>> diff --git a/refs.c b/refs.c
>> index 20e8f1ff1f..4b6bfc66fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
>> return 1;
>> }
>>
>> +int is_pseudoref(struct ref_store *refs, const char *refname)
>> +{
>> + static const char *const irregular_pseudorefs[] = {
>> + "AUTO_MERGE",
>> + "BISECT_EXPECTED_REV",
>> + "NOTES_MERGE_PARTIAL",
>> + "NOTES_MERGE_REF",
>> + "MERGE_AUTOSTASH"
>
> Let's end an array's initializer with a trailing comma, to help
> future patches to add entries to this array without unnecessary
> patch noise.
Sure, will add!
>> + };
>> + size_t i;
>> +
>> + if (!is_pseudoref_syntax(refname))
>> + return 0;
>> +
>> + if (ends_with(refname, "_HEAD"))
>> + return refs_ref_exists(refs, refname);
>> +
>> + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
>> + if (!strcmp(refname, irregular_pseudorefs[i]))
>> + return refs_ref_exists(refs, refname);
>> +
>> + return 0;
>> +}
>
> The above uses refs_ref_exists() because we want these to
> successfully resolve for reading.
>
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> + if (!strcmp(refname, "HEAD"))
>> + return refs_ref_exists(refs, refname);
>
> Given that "git for-each-ref refs/remotes" does not show
> "refs/remotes/origin/HEAD" in the output when we do not have the
> remote-tracking branch that symref points at, we probably do want
> to omit "HEAD" from the output when the HEAD symref points at an
> unborn branch. If refs_ref_exists() says "no, it does not exist"
> in such a case, we are perfectly fine with this code.
>
> We do not have to worry about the unborn state for pseudorefs
> because they would never be symbolic. But that in turn makes me
> suspect that the check done with refs_ref_exists() in the
> is_pseudoref() helper is a bit too lenient by allowing it to be a
> symbolic ref. Shouldn't we be using a check based on
> read_ref_full(), like we did in another topic recently [*]?
>
>
> [Reference]
>
> * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>
Thanks, this makes sense and the link is helpful. I'll do something
similar, but since HEAD can be a symref, I'll drop the
`RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.
I'll wait a day or two, before sending in the new version with the
fixes. The current diff is
diff --git a/refs.c b/refs.c
index b5e63f133a..4a1fd30ef2 100644
--- a/refs.c
+++ b/refs.c
@@ -866,7 +866,7 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)
"BISECT_EXPECTED_REV",
"NOTES_MERGE_PARTIAL",
"NOTES_MERGE_REF",
- "MERGE_AUTOSTASH"
+ "MERGE_AUTOSTASH",
};
size_t i;
@@ -885,10 +885,23 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)
int is_headref(struct ref_store *refs, const char *refname)
{
- if (!strcmp(refname, "HEAD"))
- return refs_ref_exists(refs, refname);
+ struct object_id oid;
+ int flag;
- return 0;
+ if (strcmp(refname, "HEAD"))
+ return 0;
+
+ /*
+ * If HEAD doesn't exist, we don't have to die, but rather,
+ * we simply return 0.
+ */
+ if (read_ref_full("HEAD", RESOLVE_REF_READING, &oid, &flag))
+ return 0;
+
+ if (is_null_oid(&oid))
+ return 0;
+
+ return 1;
}
static int is_current_worktree_ref(const char *ref) {
- Karthik
Attachment:
signature.asc
Description: PGP signature
