On Mon, Apr 22, 2024 at 09:15:41AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > The `validate_headref()` function takes a path to a supposed "HEAD" file > > and checks whether its format is something that we understand. It is > > used as part of our repository discovery to check whether a specific > > directory is a Git directory or not. > > > > Part of the validation is a check for a detached HEAD that contains a > > plain object ID. To do this validation we use `get_oid_hex()`, which > > relies on `the_hash_algo`. At this point in time the hash algo cannot > > yet be initialized though because we didn't yet read the Git config. > > Consequently, it will always be the SHA1 hash algorithm. > > > > In practice this works alright because `get_oid_hex()` only ends up > > checking whether the prefix of the buffer is a valid object ID. And > > because SHA1 is shorter than SHA256, the function will successfully > > parse SHA256 object IDs, as well. > > > > It is somewhat fragile though and not really the intent to only check > > for SHA1. With this in mind, harden the code to use `get_oid_hex_any()` > > to check whether the "HEAD" file parses as any known hash. > > All makes sense, and given the above, I strongly suspect that we > would want to make the validate_headref() function a file-scope > static in setup.c to make sure it is only called/callable from the > repository discovery codepath. Especially that if somebody calls > this function again after we find out that the repository uses > SHA-256, we would want to let the caller know that the detached HEAD > records SHA-1 and we are in an inconsistent state. Fair enough, we can definitely do so. It only has a single callsite anyway. > > It follows that we could just leave the current code intact, as in > > practice the code change doesn't have any user visible impact. But it > > also prepares us for `the_hash_algo` being unset when there is no > > repositroy. > > Or perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when > the_hash_algo has not been determined, and use !get_oid_hex() after > we have determined which algorithm we are using? It may be what you > did in a later step in the series, so let me read on. I didn't, and given that it's only used from `is_git_directory()` it would probably not make much sense, either. I'll rather add another patch on top that moves the function around to clarify that it is only expected to be called from "setup.c". Patrick > Thanks. > > > Signed-off-by: Patrick Steinhardt <ps@xxxxxx> > > --- > > path.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/path.c b/path.c > > index 67229edb9c..cc02165530 100644 > > --- a/path.c > > +++ b/path.c > > @@ -693,7 +693,7 @@ int validate_headref(const char *path) > > /* > > * Is this a detached HEAD? > > */ > > - if (!get_oid_hex(buffer, &oid)) > > + if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN) > > return 0; > > > > return -1;
Attachment:
signature.asc
Description: PGP signature
