[PATCH 00/11] Stop relying on SHA1 fallback for `the_hash_algo`

Patrick Steinhardt

author

2024-04-19 09:51 UTC #0 lore raw

cover Stop relying on SHA1 fallback for `the_hash_algo`

Hi,

when starting up, Git will initialize the_repository by calling initialize_the_repository(). Part of this is also that we set the hash algo of the_repository to SHA1, which implicitly sets the_hash_algo because it is a macro expanding to the_repository->hash_algo.

Usually, we eventually set up the correct hash algorithm here once we have properly set up the_repository via setup_git_directory(). But in some commands we actually don't require a Git repository, and thus we will leave the SHA1 hash algorithm in place.

This has led to some subtle bugs when the context really asks for a SHA256 repository, which this patch series corrects for most of the part. Some commands need further work, like for example git-diff(1), where the user might want to have the ability to pick a hash function when run outside of a repository.

Ultimately, the last patch then drops the setup of the fallback hash algorithm completely. This will cause the_hash_algo to be a NULL pointer unless explicitly configured, and thus we now start to crash when it gets accessed without doing so beforehand. This is a rather risky thing to do, but it does catch bugs where we might otherwise accidentally do the wrong thing. And even though I think it is the right thing to do even conceptually, I'd be okay to drop it if folks think that the risk is not worth it.

Patrick

Patrick Steinhardt (11):
path: harden validation of HEAD with non-standard hashes
parse-options-cb: only abbreviate hashes when hash algo is known
attr: don't recompute default attribute source
attr: fix BUG() when parsing attrs outside of repo
remote-curl: fix parsing of detached SHA256 heads builtin/rev-parse: allow shortening to more than 40 hex characters builtin/blame: don't access potentially unitialized the_hash_algo builtin/bundle: abort "verify" early when there is no repository builtin/diff: explicitly set hash algo when there is no repo builtin/shortlog: don't set up revisions without repo
repository: stop setting SHA1 as the default object hash

 attr.c                     | 31 ++++++++++++++++++++++---------
 builtin/blame.c            |  5 ++---
 builtin/bundle.c           |  5 +++++
 builtin/diff.c             |  9 +++++++++
 builtin/rev-parse.c        |  5 ++---
 builtin/shortlog.c         |  2 +-
 parse-options-cb.c         |  3 ++-
 path.c                     |  2 +-
 remote-curl.c              | 19 ++++++++++++++++++-
 repository.c               |  2 --
 t/t0003-attributes.sh      | 15 +++++++++++++++
 t/t0040-parse-options.sh   | 17 +++++++++++++++++
 t/t1500-rev-parse.sh       |  6 ++++++
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 14 files changed, 115 insertions(+), 21 deletions(-)

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #1 lore raw

PATCH 1/11 path: harden validation of HEAD with non-standard hashes

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.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repostiory format.

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.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -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;

Signature · 1 line

2.44.GIT

brian m. carlson

2024-04-19 19:03 UTC #2 lore raw

Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

Quoted 4 lines · Patrick Steinhardt

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.

The patch looks fine and is well explained, but you have a typo ("repositroy").

Signature · 2 lines

brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

Patrick Steinhardt

author

2024-04-22 04:56 UTC #3 lore raw

Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

Quoted 8 lines · brian m. carlson

Quoted 4 lines · Patrick Steinhardt
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.
The patch looks fine and is well explained, but you have a typo ("repositroy").

Thanks, fixed! I'll refrain from sending out another version just to address this typo though.

Patrick

Junio C Hamano

2024-04-22 16:15 UTC #4 lore raw

Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

In reply to Patrick Steinhardt

Quoted 11 lines · Patrick Steinhardt

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Thanks.

Quoted 18 lines

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -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;

Patrick Steinhardt

author

2024-04-23 04:50 UTC #5 lore raw

Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

Quoted 22 lines · Junio C Hamano

Quoted 11 lines · Patrick Steinhardt
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.
Quoted 8 lines
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.

Quoted 7 lines

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.

Quoted 9 lines

Quoted 4 lines
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

Quoted 20 lines

Thanks.

Quoted 18 lines

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -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;

Junio C Hamano

2024-04-23 16:54 UTC #6 lore raw

Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

Quoted 10 lines · Patrick Steinhardt

Quoted 7 lines
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.

I was wondering if I was missing a future plans to call it from other code paths that are triggered after the set-up was done. If that is not the case, we should do so. It matters more for the future callers than the current ones. They _all_ have to be aware of the deliberate looseness of the check here.

Thanks.

Patrick Steinhardt

author

2024-04-19 09:51 UTC #7 lore raw

PATCH 2/11 parse-options-cb: only abbreviate hashes when hash algo is known

In reply to Patrick Steinhardt

The OPT__ABBREV() option can be used to add an option that abbreviates object IDs. When given an length longer than the_hash_algo->hexsz, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have the_hash_algo initialized properly as the hash algortihm can only be set up after we have set up the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use OPT__ABBREV() also have RUN_SETUP set and thus cannot work without a repository. Consequently, both the_repository and the_hash_algo would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

parse-options-cb.c +2 -1

index bdc7fae497..d99d688d3c 100644
@@ -7,6 +7,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "setup.h"
 #include "string-list.h"
 #include "strvec.h"
 #include "oid-array.h"

@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > the_hash_algo->hexsz)
+		else if (startup_info->have_repository && v > the_hash_algo->hexsz)
 			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;

t/t0040-parse-options.sh +17 -0

index 8bb2a8b453..45a773642f 100755
@@ -176,6 +176,23 @@ test_expect_success 'long options' '
 	test_cmp expect output
 '
 
+test_expect_success 'abbreviate to something longer than SHA1 length' '
+	cat >expect <<-EOF &&
+	boolean: 0
+	integer: 0
+	magnitude: 0
+	timestamp: 0
+	string: (not set)
+	abbrev: 100
+	verbose: -1
+	quiet: 0
+	dry run: no
+	file: (not set)
+	EOF
+	test-tool parse-options --abbrev=100 >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'missing required value' '
 	cat >expect <<-\EOF &&
 	error: switch `s'\'' requires a value

Signature · 1 line

2.44.GIT

Justin Tobler

2024-04-23 00:31 UTC #8 lore raw

Re: [PATCH 02/11] parse-options-cb: only abbreviate hashes when hash algo is known

Quoted 2 lines · Patrick Steinhardt

The OPT__ABBREV() option can be used to add an option that abbreviates object IDs. When given an length longer than the_hash_algo->hexsz,

s/an length/a length/

Quoted 4 lines

then it will instead set the length to that maximum length.
It may not always be guaranteed that we have the_hash_algo initialized properly as the hash algortihm can only be set up after we have set up

s/algortihm/algorithm/

Quoted 8 lines

the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.
In practice it's not a problem as all commands that use OPT__ABBREV() also have RUN_SETUP set and thus cannot work without a repository. Consequently, both the_repository and the_hash_algo would be properly set up.

Quoted 4 lines

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.
Signed-off-by Patrick Steinhardt <[email protected]>

...

A couple of typos I noticed.

-Justin

Patrick Steinhardt

author

2024-04-19 09:51 UTC #9 lore raw

PATCH 3/11 attr: don't recompute default attribute source

In reply to Patrick Steinhardt

The default_attr_source() function lazily computes the attr source supposedly once, only. This is done via a static variable attr_source that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This has approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable has_attr_source to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the ignore_bad_attr_tree to not be static anymore as the code will only be executed once anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

attr.c +16 -9

index 679e42258c..9d911aeb31 100644
@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,
 }
 
 static const char *default_attr_source_tree_object_name;
-static int ignore_bad_attr_tree;
 
 void set_git_attr_source(const char *tree_object_name)
 {
 	default_attr_source_tree_object_name = xstrdup(tree_object_name);
 }
 
-static void compute_default_attr_source(struct object_id *attr_source)
+static int compute_default_attr_source(struct object_id *attr_source)
 {
+	int ignore_bad_attr_tree = 0;
+
 	if (!default_attr_source_tree_object_name)
 		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);

@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)
 		ignore_bad_attr_tree = 1;
 	}
 
-	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
-		return;
+	if (!default_attr_source_tree_object_name)
+		return 0;
 
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
-				 attr_source) && !ignore_bad_attr_tree)
-		die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+				 attr_source)) {
+		if (!ignore_bad_attr_tree)
+			die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+		return 0;
+	}
+
+	return 1;
 }
 
 static struct object_id *default_attr_source(void)
 {
 	static struct object_id attr_source;
+	static int has_attr_source = -1;
 
-	if (is_null_oid(&attr_source))
-		compute_default_attr_source(&attr_source);
-	if (is_null_oid(&attr_source))
+	if (has_attr_source < 0)
+		has_attr_source = compute_default_attr_source(&attr_source);
+	if (!has_attr_source)
 		return NULL;
 	return &attr_source;
 }

Signature · 1 line

2.44.GIT

Justin Tobler

2024-04-23 00:33 UTC #10 lore raw

Re: [PATCH 03/11] attr: don't recompute default attribute source

Quoted 7 lines · Patrick Steinhardt

The default_attr_source() function lazily computes the attr source supposedly once, only. This is done via a static variable attr_source that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.
This has approach is flawed though: the variable will never be set to

Remove "has"

-Justin

Quoted 13 lines

anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.
Improve this by introducing a separate variable has_attr_source to track whether we already computed the attr source and, if so, whether we have an attr source or not.
This also allows us to convert the ignore_bad_attr_tree to not be static anymore as the code will only be executed once anyway.
Signed-off-by Patrick Steinhardt <[email protected]>

...

Patrick Steinhardt

author

2024-04-19 09:51 UTC #11 lore raw

PATCH 4/11 attr: fix BUG() when parsing attrs outside of repo

In reply to Patrick Steinhardt

If either the --attr-source option or the GIT_ATTR_SOURCE envvar are set, then compute_default_attr_source() will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using git grep --no-index. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

attr.c +6 -0

index 9d911aeb31..4bd09bcb4b 100644
@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)
 	if (!default_attr_source_tree_object_name)
 		return 0;
 
+	if (!startup_info->have_repository) {
+		if (!ignore_bad_attr_tree)
+			die(_("cannot use --attr-source or GIT_ATTR_SOURCE without repo"));
+		return 0;
+	}
+
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
 				 attr_source)) {

t/t0003-attributes.sh +15 -0

index 774b52c298..3efdec54dd 100755
@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre
 	)
 '
 
+test_expect_success 'diff without repository with attr source' '
+	mkdir -p "$TRASH_DIRECTORY/outside/nongit" &&
+	(
+		cd "$TRASH_DIRECTORY/outside/nongit" &&
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		touch file &&
+		cat >expect <<-EOF &&
+		fatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo
+		EOF
+		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #12 lore raw

PATCH 5/11 remote-curl: fix parsing of detached SHA256 heads

In reply to Patrick Steinhardt

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via http_fetch_ref(). This function will either parse the file as an object ID in case it is exactly the_hash_algo->hexsz long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update the_hash_algo to the discovered remote object hash. Consequently, the_hash_algo will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up the_hash_algo via repo_set_hash_algo(). While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for the_hash_algo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

remote-curl.c +18 -1

index 0b6d7815fd..004b707fdf 100644
@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 	return list;
 }
 
+/*
+ * Try to detect the hash algorithm used by the remote repository when using
+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash
+ * directly have to derive it from the advertised ref lengths.
+ */
 static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)
 {
 	const char *p = memchr(heads->buf, '\t', heads->len);
 	int algo;
+
+	/*
+	 * In case the remote has no refs we have no way to reliably determine
+	 * the object hash used by that repository. In that case we simply fall
+	 * back to SHA1, which may or may not be correct.
+	 */
 	if (!p)
-		return the_hash_algo;
+		return &hash_algos[GIT_HASH_SHA1];
 
 	algo = hash_algo_by_length((p - heads->buf) / 2);
 	if (algo == GIT_HASH_UNKNOWN)

@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)
 		    "is this a git repository?",
 		    transport_anonymize_url(url.buf));
 
+	/*
+	 * Set the repository's hash algo to whatever we have just detected.
+	 * This ensures that we can correctly parse the remote references.
+	 */
+	repo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));
+
 	data = heads->buf;
 	start = NULL;
 	mid = data;

t/t5550-http-fetch-dumb.sh +15 -0

index 4c3b32785d..5f16cbc58d 100755
@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '
 	test_cmp expect actual
 '
 
+
+test_expect_success 'list detached HEAD from outside any repository' '
+	git clone --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" \
+		update-ref --no-deref HEAD refs/heads/main &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" update-server-info &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	nongit git ls-remote "$HTTPD_URL/dumb/repo-detached.git" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #13 lore raw

PATCH 6/11 builtin/rev-parse: allow shortening to more than 40 hex characters

In reply to Patrick Steinhardt

The --short= option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to the_hash_algo->hexsz before we have even set up the repo. Consequently, the_hash_algo will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing the_hash_algo only after we have set up the repo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

builtin/rev-parse.c +2 -3

index 624182e507..d7b87c605e 100644
@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	const char *name = NULL;
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
-	const int hexsz = the_hash_algo->hexsz;
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;

@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				abbrev = strtoul(arg, NULL, 10);
 				if (abbrev < MINIMUM_ABBREV)
 					abbrev = MINIMUM_ABBREV;
-				else if (hexsz <= abbrev)
-					abbrev = hexsz;
+				else if ((int)the_hash_algo->hexsz <= abbrev)
+					abbrev = the_hash_algo->hexsz;
 				continue;
 			}
 			if (!strcmp(arg, "--sq")) {

t/t1500-rev-parse.sh +6 -0

index a669e592f1..30c31918fd 100755
@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '
 	test_cmp expect actual
 '
 
+test_expect_success '--short= truncates to the actual hash length' '
+	git rev-parse HEAD >expect &&
+	git rev-parse --short=100 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #14 lore raw

PATCH 7/11 builtin/blame: don't access potentially unitialized `the_hash_algo`

In reply to Patrick Steinhardt

We access the_hash_algo in git-blame(1) before we have executed parse_options_start(), which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify RUN_SETUP and thus require a repository.

There is one exception though, namely when passing -h to print the help. Here we will access the_hash_algo even if there is no repo. This works fine right now because the_hash_algo gets sets up to point to the SHA1 algorithm via initialize_repository(). But we're about to stop doing this, and thus the code would lead to a NULL pointer exception.

Prepare the code for this and only access the_hash_algo after we are sure that there is a proper repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

builtin/blame.c +2 -3

index 9aa74680a3..e325825936 100644
@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct range_set ranges;
 	unsigned int range_i;
 	long anchor;
-	const int hexsz = the_hash_algo->hexsz;
 	long num_lines = 0;
 	const char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;
 	const char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;

@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	} else if (show_progress < 0)
 		show_progress = isatty(2);
 
-	if (0 < abbrev && abbrev < hexsz)
+	if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
 	else if (!abbrev)
-		abbrev = hexsz;
+		abbrev = the_hash_algo->hexsz;
 
 	if (revs_file && read_ancestry(revs_file))
 		die_errno("reading graft file '%s' failed", revs_file);

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #15 lore raw

PATCH 8/11 builtin/bundle: abort "verify" early when there is no repository

In reply to Patrick Steinhardt

Verifying a bundle requires us to have a repository. This is encoded in verify_bundle(), which will return an error if there is no repository. We call open_bundle() before we call verify_bundle() though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because open_bundle() already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing the_hash_algo when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

builtin/bundle.c +5 -0

index 3ad11dc5d0..d5d41a8f67 100644
@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (!startup_info->have_repository) {
+		ret = error(_("need a repository to verify a bundle"));
+		goto cleanup;
+	}
+
 	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-19 09:51 UTC #16 lore raw

PATCH 9/11 builtin/diff: explicitly set hash algo when there is no repo

In reply to Patrick Steinhardt

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

```
diff --git a/a b/b
index 7898192..6178079 100644
--- a/a
+++ b/b
@@ -1 +1 @@
-a
+b
```

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that the_repository only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for the_repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

builtin/diff.c +9 -0

index 6e196e0c7d..58ec7e5da2 100644
@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
+	/*
+	 * When operating outside of a Git repository we need to have a hash
+	 * algorithm at hand so that we can generate the blob hashes. We
+	 * default to SHA1 here, but may eventually want to change this to be
+	 * configurable via a command line option.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	prefix = precompose_argv_prefix(argc, argv, prefix);

Signature · 1 line

2.44.GIT

Junio C Hamano

2024-04-22 18:41 UTC #17 lore raw

Re: [PATCH 09/11] builtin/diff: explicitly set hash algo when there is no repo

Quoted 13 lines · Patrick Steinhardt

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:
```
b renamed from a +1 -1
index 7898192..6178079 100644
@@ -1 +1 @@
-a
+b

Quoted 1 line

```

This will break "git am"; it is customary to indent such sample patch block in a log message.

Quoted 37 lines

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that the_repository only ever has a hash function when it was properly initialized via a repo's configuration.
To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for the_repository.
Signed-off-by Patrick Steinhardt <[email protected]>
---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)
builtin/diff.c +9 -0
index 6e196e0c7d..58ec7e5da2 100644
@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
+	/*
+	 * When operating outside of a Git repository we need to have a hash
+	 * algorithm at hand so that we can generate the blob hashes. We
+	 * default to SHA1 here, but may eventually want to change this to be
+	 * configurable via a command line option.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	prefix = precompose_argv_prefix(argc, argv, prefix);

Patrick Steinhardt

author

2024-04-19 09:51 UTC #18 lore raw

PATCH 10/11 builtin/shortlog: don't set up revisions without repo

In reply to Patrick Steinhardt

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Reghardless of that we still end up calling setup_revisions(). While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call setup_revisions() when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/shortlog.c +1 -1

index 3c7cd2d6ef..d4daf31e22 100644
@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		usage_with_options(shortlog_usage, options);
 	}
 
-	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
+	if (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);
 	}

Signature · 1 line

2.44.GIT

Justin Tobler

2024-04-23 00:36 UTC #19 lore raw

Re: [PATCH 10/11] builtin/shortlog: don't set up revisions without repo

Quoted 6 lines · Patrick Steinhardt

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.
Reghardless of that we still end up calling setup_revisions(). While

s/Reghardless/Regardless/

Quoted 8 lines

that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.
Refactor the code to only call setup_revisions() when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.
Signed-off-by Patrick Steinhardt <[email protected]>

...

Patrick Steinhardt

author

2024-04-19 09:52 UTC #20 lore raw

PATCH 11/11 repository: stop setting SHA1 as the default object hash

In reply to Patrick Steinhardt

During the startup of Git, we call initialize_the_repository() to set up the_repository as well as the_index. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that the_hash_algo is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via setup_git_directory(), which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access the_hash_algo when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that the_hash_algo will map to a NULL pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

repository.c +0 -2

index e15b416944..b65b1a8c8b 100644
@@ -35,8 +35,6 @@ void initialize_the_repository(void)
 	the_repo.parsed_objects = parsed_object_pool_new();
 
 	index_state_init(&the_index, the_repository);
-
-	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,

Signature · 1 line

2.44.GIT

brian m. carlson

2024-04-19 19:13 UTC #21 lore raw

In reply to Patrick Steinhardt

Quoted 18 lines · Patrick Steinhardt

Hi,
when starting up, Git will initialize the_repository by calling initialize_the_repository(). Part of this is also that we set the hash algo of the_repository to SHA1, which implicitly sets the_hash_algo because it is a macro expanding to the_repository->hash_algo.
Usually, we eventually set up the correct hash algorithm here once we have properly set up the_repository via setup_git_directory(). But in some commands we actually don't require a Git repository, and thus we will leave the SHA1 hash algorithm in place.
This has led to some subtle bugs when the context really asks for a SHA256 repository, which this patch series corrects for most of the part. Some commands need further work, like for example git-diff(1), where the user might want to have the ability to pick a hash function when run outside of a repository.

Quoted 8 lines

Ultimately, the last patch then drops the setup of the fallback hash algorithm completely. This will cause the_hash_algo to be a NULL pointer unless explicitly configured, and thus we now start to crash when it gets accessed without doing so beforehand. This is a rather risky thing to do, but it does catch bugs where we might otherwise accidentally do the wrong thing. And even though I think it is the right thing to do even conceptually, I'd be okay to drop it if folks think that the risk is not worth it.

I've taken a look, and other than the minor typo I noted, this seems fine. I'm in favour of getting rid of the SHA-1 default, even though my gut tells me we might find a bug or two along the way where things aren't initialized properly. I still think that'll be okay and it's worth doing, since it'll help us prepare for the case in the future where we want to switch the default and also for libification, where we won't want to make those kinds of assumptions.

Signature · 2 lines

brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

Junio C Hamano

2024-04-19 19:16 UTC #22 lore raw

Quoted 7 lines · brian m. carlson

I've taken a look, and other than the minor typo I noted, this seems fine. I'm in favour of getting rid of the SHA-1 default, even though my gut tells me we might find a bug or two along the way where things aren't initialized properly. I still think that'll be okay and it's worth doing, since it'll help us prepare for the case in the future where we want to switch the default and also for libification, where we won't want to make those kinds of assumptions.

Yup, thanks for reviewing (and thanks Patrick for writing the series).

Patrick Steinhardt

author

2024-04-22 04:56 UTC #23 lore raw

In reply to brian m. carlson

Quoted 28 lines · brian m. carlson

Quoted 18 lines · Patrick Steinhardt
Hi,
when starting up, Git will initialize the_repository by calling initialize_the_repository(). Part of this is also that we set the hash algo of the_repository to SHA1, which implicitly sets the_hash_algo because it is a macro expanding to the_repository->hash_algo.
Usually, we eventually set up the correct hash algorithm here once we have properly set up the_repository via setup_git_directory(). But in some commands we actually don't require a Git repository, and thus we will leave the SHA1 hash algorithm in place.
This has led to some subtle bugs when the context really asks for a SHA256 repository, which this patch series corrects for most of the part. Some commands need further work, like for example git-diff(1), where the user might want to have the ability to pick a hash function when run outside of a repository.
Quoted 8 lines
Ultimately, the last patch then drops the setup of the fallback hash algorithm completely. This will cause the_hash_algo to be a NULL pointer unless explicitly configured, and thus we now start to crash when it gets accessed without doing so beforehand. This is a rather risky thing to do, but it does catch bugs where we might otherwise accidentally do the wrong thing. And even though I think it is the right thing to do even conceptually, I'd be okay to drop it if folks think that the risk is not worth it.

Quoted 7 lines

I've taken a look, and other than the minor typo I noted, this seems fine. I'm in favour of getting rid of the SHA-1 default, even though my gut tells me we might find a bug or two along the way where things aren't initialized properly. I still think that'll be okay and it's worth doing, since it'll help us prepare for the case in the future where we want to switch the default and also for libification, where we won't want to make those kinds of assumptions.

Yeah, I would expect there to be a few more bugs that I just didn't spot. But I think if we do this early in the next release cycle it would give us plenty of time to detect any such issues. And in the worst case we would still be able to revert the last patch of this series. So all in all it hopefully shouldn't be all that bad and prepares us for the future.

Thanks for your review!

Patrick

Patrick Steinhardt

author

2024-04-23 05:07 UTC #24 lore raw

v2 cover Stop relying on SHA1 fallback for `the_hash_algo`

In reply to Patrick Steinhardt

Hi,

this is the second version of my patch series that causes us to stop relying on the SHA1 default hash.

Changes compared to v1:

- Various typo fixes in commit messages.

- Added another patch that moves validate_headref() into "setup.c" to clarify that it is only used during repository discovery.

- Indented a diff in a commit message so that git-am(1) is happy.

Thanks!

Patrick

Patrick Steinhardt (12):
path: harden validation of HEAD with non-standard hashes
path: move validate_headref() to its only user
parse-options-cb: only abbreviate hashes when hash algo is known
attr: don't recompute default attribute source
attr: fix BUG() when parsing attrs outside of repo
remote-curl: fix parsing of detached SHA256 heads builtin/rev-parse: allow shortening to more than 40 hex characters builtin/blame: don't access potentially unitialized the_hash_algo builtin/bundle: abort "verify" early when there is no repository builtin/diff: explicitly set hash algo when there is no repo builtin/shortlog: don't set up revisions without repo
repository: stop setting SHA1 as the default object hash

 attr.c                     | 31 +++++++++++++++-------
 builtin/blame.c            |  5 ++--
 builtin/bundle.c           |  5 ++++
 builtin/diff.c             |  9 +++++++
 builtin/rev-parse.c        |  5 ++--
 builtin/shortlog.c         |  2 +-
 parse-options-cb.c         |  3 ++-
 path.c                     | 53 --------------------------------------
 path.h                     |  1 -
 remote-curl.c              | 19 +++++++++++++-
 repository.c               |  2 --
 setup.c                    | 53 ++++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh      | 15 +++++++++++
 t/t0040-parse-options.sh   | 17 ++++++++++++
 t/t1500-rev-parse.sh       |  6 +++++
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++
 16 files changed, 167 insertions(+), 74 deletions(-)

Range-diff against v1:
1: aa4d6f508b ! 1: a986b464d3 path: harden validation of HEAD with non-standard hashes
@@ Commit message
current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood
- when trying to set up the repostiory format.
+ when trying to set up the repository format.

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.
+ repository.

Signed-off-by Patrick Steinhardt <[email protected]>

 -:  ---------- >  2:  a347c7e6ca path: move `validate_headref()` to its only user
 2:  5daaaed2b9 !  3:  c0a15b2fa6 parse-options-cb: only abbreviate hashes when hash algo is known

@@ Commit message
parse-options-cb: only abbreviate hashes when hash algo is known

The OPT__ABBREV() option can be used to add an option that abbreviates
- object IDs. When given an length longer than the_hash_algo->hexsz,
- then it will instead set the length to that maximum length.
+ object IDs. When given a length longer than the_hash_algo->hexsz, then
+ it will instead set the length to that maximum length.

It may not always be guaranteed that we have the_hash_algo initialized
- properly as the hash algortihm can only be set up after we have set up
+ properly as the hash algorithm can only be set up after we have set up
the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

3: ae91a27ffc ! 4: 1b5f904eed attr: don't recompute default attribute source
@@ Commit message
variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

- This has approach is flawed though: the variable will never be set to
+ This approach is flawed though: the variable will never be set to
anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try

 4:  53c8e1cd7c =  5:  26909daca4 attr: fix BUG() when parsing attrs outside of repo
 5:  32a429fb60 =  6:  0b99184f50 remote-curl: fix parsing of detached SHA256 heads
 6:  9cb7baa50c =  7:  ccfda3c2d2 builtin/rev-parse: allow shortening to more than 40 hex characters
 7:  e189a4ad15 =  8:  1813e7eb5c builtin/blame: don't access potentially unitialized `the_hash_algo`
 8:  bc4bda3508 =  9:  31182a1fc6 builtin/bundle: abort "verify" early when there is no repository
 9:  39e56dab62 ! 10:  78e19d0a1b builtin/diff: explicitly set hash algo when there is no repo

@@ Commit message
hashing the files that we are diffing so that we can print the "index" line:

- ```
- diff --git a/a b/b
- index 7898192..6178079 100644
- --- a/a
- +++ b/b
- @@ -1 +1 @@
- -a
- +b
- ```
+ ```
+ diff --git a/a b/b
+ index 7898192..6178079 100644
+ --- a/a
+ +++ b/b
+ @@ -1 +1 @@
+ -a
+ +b
+ ```

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine.
10: 508e28ed1e ! 11: 51bcddbc31 builtin/shortlog: don't set up revisions without repo
@@ Commit message
repository in that context, it is thus unsupported to pass any revisions as arguments.

- Reghardless of that we still end up calling setup_revisions(). While
+ Regardless of that we still end up calling setup_revisions(). While
that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

11: f86a6ff3ba = 12: e8126371e1 repository: stop setting SHA1 as the default object hash

base-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #25 lore raw

PATCH v2 1/12 path: harden validation of HEAD with non-standard hashes

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.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

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 repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -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;

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #26 lore raw

PATCH v2 2/12 path: move `validate_headref()` to its only user

In reply to Patrick Steinhardt

While validate_headref() is only called from is_git_directory() in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

path.c +0 ~~-53~~

index cc02165530..bd6e25245d 100644
@@ -5,7 +5,6 @@
 #include "abspath.h"
 #include "environment.h"
 #include "gettext.h"
-#include "hex.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "string-list.h"

@@ -647,58 +646,6 @@ void strbuf_git_common_path(struct strbuf *sb,
 	va_end(args);
 }
 
-int validate_headref(const char *path)
-{
-	struct stat st;
-	char buffer[256];
-	const char *refname;
-	struct object_id oid;
-	int fd;
-	ssize_t len;
-
-	if (lstat(path, &st) < 0)
-		return -1;
-
-	/* Make sure it is a "refs/.." symlink */
-	if (S_ISLNK(st.st_mode)) {
-		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
-			return 0;
-		return -1;
-	}
-
-	/*
-	 * Anything else, just open it and try to see if it is a symbolic ref.
-	 */
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		return -1;
-	len = read_in_full(fd, buffer, sizeof(buffer)-1);
-	close(fd);
-
-	if (len < 0)
-		return -1;
-	buffer[len] = '\0';
-
-	/*
-	 * Is it a symbolic ref?
-	 */
-	if (skip_prefix(buffer, "ref:", &refname)) {
-		while (isspace(*refname))
-			refname++;
-		if (starts_with(refname, "refs/"))
-			return 0;
-	}
-
-	/*
-	 * Is this a detached HEAD?
-	 */
-	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
-		return 0;
-
-	return -1;
-}
-
 static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;

path.h +0 -1

index ea96487b29..c3bc8617bd 100644
@@ -173,7 +173,6 @@ const char *git_path_fetch_head(struct repository *r);
 const char *git_path_shallow(struct repository *r);
 
 int ends_with_path_components(const char *path, const char *components);
-int validate_headref(const char *ref);
 
 int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);

setup.c +53 -0

65 lines not on this page · raw message

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #27 lore raw

PATCH v2 3/12 parse-options-cb: only abbreviate hashes when hash algo is known

In reply to Patrick Steinhardt

The OPT__ABBREV() option can be used to add an option that abbreviates object IDs. When given a length longer than the_hash_algo->hexsz, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have the_hash_algo initialized properly as the hash algorithm can only be set up after we have set up the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use OPT__ABBREV() also have RUN_SETUP set and thus cannot work without a repository. Consequently, both the_repository and the_hash_algo would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

parse-options-cb.c +2 -1

index bdc7fae497..d99d688d3c 100644
@@ -7,6 +7,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "setup.h"
 #include "string-list.h"
 #include "strvec.h"
 #include "oid-array.h"

@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > the_hash_algo->hexsz)
+		else if (startup_info->have_repository && v > the_hash_algo->hexsz)
 			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;

t/t0040-parse-options.sh +17 -0

index 8bb2a8b453..45a773642f 100755
@@ -176,6 +176,23 @@ test_expect_success 'long options' '
 	test_cmp expect output
 '
 
+test_expect_success 'abbreviate to something longer than SHA1 length' '
+	cat >expect <<-EOF &&
+	boolean: 0
+	integer: 0
+	magnitude: 0
+	timestamp: 0
+	string: (not set)
+	abbrev: 100
+	verbose: -1
+	quiet: 0
+	dry run: no
+	file: (not set)
+	EOF
+	test-tool parse-options --abbrev=100 >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'missing required value' '
 	cat >expect <<-\EOF &&
 	error: switch `s'\'' requires a value

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #28 lore raw

PATCH v2 4/12 attr: don't recompute default attribute source

In reply to Patrick Steinhardt

The default_attr_source() function lazily computes the attr source supposedly once, only. This is done via a static variable attr_source that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable has_attr_source to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the ignore_bad_attr_tree to not be static anymore as the code will only be executed once anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

attr.c +16 -9

index 679e42258c..9d911aeb31 100644
@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,
 }
 
 static const char *default_attr_source_tree_object_name;
-static int ignore_bad_attr_tree;
 
 void set_git_attr_source(const char *tree_object_name)
 {
 	default_attr_source_tree_object_name = xstrdup(tree_object_name);
 }
 
-static void compute_default_attr_source(struct object_id *attr_source)
+static int compute_default_attr_source(struct object_id *attr_source)
 {
+	int ignore_bad_attr_tree = 0;
+
 	if (!default_attr_source_tree_object_name)
 		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);

@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)
 		ignore_bad_attr_tree = 1;
 	}
 
-	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
-		return;
+	if (!default_attr_source_tree_object_name)
+		return 0;
 
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
-				 attr_source) && !ignore_bad_attr_tree)
-		die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+				 attr_source)) {
+		if (!ignore_bad_attr_tree)
+			die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+		return 0;
+	}
+
+	return 1;
 }
 
 static struct object_id *default_attr_source(void)
 {
 	static struct object_id attr_source;
+	static int has_attr_source = -1;
 
-	if (is_null_oid(&attr_source))
-		compute_default_attr_source(&attr_source);
-	if (is_null_oid(&attr_source))
+	if (has_attr_source < 0)
+		has_attr_source = compute_default_attr_source(&attr_source);
+	if (!has_attr_source)
 		return NULL;
 	return &attr_source;
 }

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #29 lore raw

PATCH v2 5/12 attr: fix BUG() when parsing attrs outside of repo

In reply to Patrick Steinhardt

If either the --attr-source option or the GIT_ATTR_SOURCE envvar are set, then compute_default_attr_source() will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using git grep --no-index. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

attr.c +6 -0

index 9d911aeb31..4bd09bcb4b 100644
@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)
 	if (!default_attr_source_tree_object_name)
 		return 0;
 
+	if (!startup_info->have_repository) {
+		if (!ignore_bad_attr_tree)
+			die(_("cannot use --attr-source or GIT_ATTR_SOURCE without repo"));
+		return 0;
+	}
+
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
 				 attr_source)) {

t/t0003-attributes.sh +15 -0

index 774b52c298..3efdec54dd 100755
@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre
 	)
 '
 
+test_expect_success 'diff without repository with attr source' '
+	mkdir -p "$TRASH_DIRECTORY/outside/nongit" &&
+	(
+		cd "$TRASH_DIRECTORY/outside/nongit" &&
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		touch file &&
+		cat >expect <<-EOF &&
+		fatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo
+		EOF
+		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:07 UTC #30 lore raw

PATCH v2 6/12 remote-curl: fix parsing of detached SHA256 heads

In reply to Patrick Steinhardt

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via http_fetch_ref(). This function will either parse the file as an object ID in case it is exactly the_hash_algo->hexsz long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update the_hash_algo to the discovered remote object hash. Consequently, the_hash_algo will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up the_hash_algo via repo_set_hash_algo(). While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for the_hash_algo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

remote-curl.c +18 -1

index 0b6d7815fd..004b707fdf 100644
@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 	return list;
 }
 
+/*
+ * Try to detect the hash algorithm used by the remote repository when using
+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash
+ * directly have to derive it from the advertised ref lengths.
+ */
 static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)
 {
 	const char *p = memchr(heads->buf, '\t', heads->len);
 	int algo;
+
+	/*
+	 * In case the remote has no refs we have no way to reliably determine
+	 * the object hash used by that repository. In that case we simply fall
+	 * back to SHA1, which may or may not be correct.
+	 */
 	if (!p)
-		return the_hash_algo;
+		return &hash_algos[GIT_HASH_SHA1];
 
 	algo = hash_algo_by_length((p - heads->buf) / 2);
 	if (algo == GIT_HASH_UNKNOWN)

@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)
 		    "is this a git repository?",
 		    transport_anonymize_url(url.buf));
 
+	/*
+	 * Set the repository's hash algo to whatever we have just detected.
+	 * This ensures that we can correctly parse the remote references.
+	 */
+	repo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));
+
 	data = heads->buf;
 	start = NULL;
 	mid = data;

t/t5550-http-fetch-dumb.sh +15 -0

index 4c3b32785d..5f16cbc58d 100755
@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '
 	test_cmp expect actual
 '
 
+
+test_expect_success 'list detached HEAD from outside any repository' '
+	git clone --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" \
+		update-ref --no-deref HEAD refs/heads/main &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" update-server-info &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	nongit git ls-remote "$HTTPD_URL/dumb/repo-detached.git" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #31 lore raw

PATCH v2 7/12 builtin/rev-parse: allow shortening to more than 40 hex characters

In reply to Patrick Steinhardt

The --short= option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to the_hash_algo->hexsz before we have even set up the repo. Consequently, the_hash_algo will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing the_hash_algo only after we have set up the repo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

builtin/rev-parse.c +2 -3

index 624182e507..d7b87c605e 100644
@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	const char *name = NULL;
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
-	const int hexsz = the_hash_algo->hexsz;
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;

@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				abbrev = strtoul(arg, NULL, 10);
 				if (abbrev < MINIMUM_ABBREV)
 					abbrev = MINIMUM_ABBREV;
-				else if (hexsz <= abbrev)
-					abbrev = hexsz;
+				else if ((int)the_hash_algo->hexsz <= abbrev)
+					abbrev = the_hash_algo->hexsz;
 				continue;
 			}
 			if (!strcmp(arg, "--sq")) {

t/t1500-rev-parse.sh +6 -0

index a669e592f1..30c31918fd 100755
@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '
 	test_cmp expect actual
 '
 
+test_expect_success '--short= truncates to the actual hash length' '
+	git rev-parse HEAD >expect &&
+	git rev-parse --short=100 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #32 lore raw

PATCH v2 8/12 builtin/blame: don't access potentially unitialized `the_hash_algo`

In reply to Patrick Steinhardt

We access the_hash_algo in git-blame(1) before we have executed parse_options_start(), which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify RUN_SETUP and thus require a repository.

There is one exception though, namely when passing -h to print the help. Here we will access the_hash_algo even if there is no repo. This works fine right now because the_hash_algo gets sets up to point to the SHA1 algorithm via initialize_repository(). But we're about to stop doing this, and thus the code would lead to a NULL pointer exception.

Prepare the code for this and only access the_hash_algo after we are sure that there is a proper repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

builtin/blame.c +2 -3

index 9aa74680a3..e325825936 100644
@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct range_set ranges;
 	unsigned int range_i;
 	long anchor;
-	const int hexsz = the_hash_algo->hexsz;
 	long num_lines = 0;
 	const char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;
 	const char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;

@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	} else if (show_progress < 0)
 		show_progress = isatty(2);
 
-	if (0 < abbrev && abbrev < hexsz)
+	if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
 	else if (!abbrev)
-		abbrev = hexsz;
+		abbrev = the_hash_algo->hexsz;
 
 	if (revs_file && read_ancestry(revs_file))
 		die_errno("reading graft file '%s' failed", revs_file);

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #33 lore raw

PATCH v2 9/12 builtin/bundle: abort "verify" early when there is no repository

In reply to Patrick Steinhardt

Verifying a bundle requires us to have a repository. This is encoded in verify_bundle(), which will return an error if there is no repository. We call open_bundle() before we call verify_bundle() though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because open_bundle() already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing the_hash_algo when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

builtin/bundle.c +5 -0

index 3ad11dc5d0..d5d41a8f67 100644
@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (!startup_info->have_repository) {
+		ret = error(_("need a repository to verify a bundle"));
+		goto cleanup;
+	}
+
 	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #34 lore raw

PATCH v2 10/12 builtin/diff: explicitly set hash algo when there is no repo

In reply to Patrick Steinhardt

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that the_repository only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for the_repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

builtin/diff.c +9 -0

index 6e196e0c7d..58ec7e5da2 100644
@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
+	/*
+	 * When operating outside of a Git repository we need to have a hash
+	 * algorithm at hand so that we can generate the blob hashes. We
+	 * default to SHA1 here, but may eventually want to change this to be
+	 * configurable via a command line option.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	prefix = precompose_argv_prefix(argc, argv, prefix);

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #35 lore raw

PATCH v2 11/12 builtin/shortlog: don't set up revisions without repo

In reply to Patrick Steinhardt

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling setup_revisions(). While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call setup_revisions() when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/shortlog.c +1 -1

index 3c7cd2d6ef..d4daf31e22 100644
@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		usage_with_options(shortlog_usage, options);
 	}
 
-	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
+	if (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);
 	}

Signature · 1 line

2.45.0-rc0

Patrick Steinhardt

author

2024-04-23 05:08 UTC #36 lore raw

PATCH v2 12/12 repository: stop setting SHA1 as the default object hash

In reply to Patrick Steinhardt

During the startup of Git, we call initialize_the_repository() to set up the_repository as well as the_index. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that the_hash_algo is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via setup_git_directory(), which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access the_hash_algo when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that the_hash_algo will map to a NULL pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

repository.c +0 -2

index e15b416944..b65b1a8c8b 100644
@@ -35,8 +35,6 @@ void initialize_the_repository(void)
 	the_repo.parsed_objects = parsed_object_pool_new();
 
 	index_state_init(&the_index, the_repository);
-
-	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,

Signature · 1 line

2.45.0-rc0

Junio C Hamano

2024-04-27 22:09 UTC #37 lore raw

In reply to Patrick Steinhardt

Patrick Steinhardt <[email protected]> writes:

[cc: added Josh as the commit-graph fuzzer was his creation].

Quoted 2 lines

this is the second version of my patch series that causes us to stop relying on the SHA1 default hash.

With this topic merged, 'seen' fails "fuzz smoke test"; I think this

    https://github.com/git/git/actions/runs/8807729398/job/24175445340

is the first merge of this topic into 'seen' where "fuzz smoke test" started failing.

With the merge of the topic from 'seen' reverted tentatively,

    https://github.com/git/git/actions/runs/8862811497/job/24336185541

the same test seems happy.

Patrick Steinhardt

author

2024-04-29 06:05 UTC #38 lore raw

Quoted 11 lines · Junio C Hamano

Patrick Steinhardt <[email protected]> writes:
[cc: added Josh as the commit-graph fuzzer was his creation].
Quoted 2 lines
this is the second version of my patch series that causes us to stop relying on the SHA1 default hash.
With this topic merged, 'seen' fails "fuzz smoke test"; I think this
    https://github.com/git/git/actions/runs/8807729398/job/24175445340

Quoted 8 lines

is the first merge of this topic into 'seen' where "fuzz smoke test" started failing.
With the merge of the topic from 'seen' reverted tentatively,
    https://github.com/git/git/actions/runs/8862811497/job/24336185541
the same test seems happy.

Indeed, thanks for the heads up! Another test gap that we have in the GitLab CI setup. I'll add a separate patch series to plug this gap and add the job to GitLab CI, too.

The fix for the failure is easy:

    diff --git a/oss-fuzz/fuzz-commit-graph.c b/oss-fuzz/fuzz-commit-graph.c
    index 2992079dd9..94ecbb9242 100644
    --- a/oss-fuzz/fuzz-commit-graph.c
    +++ b/oss-fuzz/fuzz-commit-graph.c
    @@ -18,6 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
         * touching the disk to keep the individual fuzz-test cases as fast as
         * possible.
         */
    +	repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
        the_repository->settings.commit_graph_generation_version = 2;
        the_repository->settings.commit_graph_read_changed_paths = 1;
        g = parse_commit_graph(&the_repository->settings, (void *)data, size);

I'll send a v3.

Thanks!

Patrick

Patrick Steinhardt

author

2024-04-29 06:34 UTC #39 lore raw

v3 cover Stop relying on SHA1 fallback for `the_hash_algo`

In reply to Patrick Steinhardt

Hi,

this is the third version of my patch series that stops relying on the SHA1 fallback configured for the_hash_algo.

There's only a single change compared to v2, which is a new patch that fixes a segfault in the commit-graph fuzzer.

Thanks!

Patrick

Patrick Steinhardt (13):
path: harden validation of HEAD with non-standard hashes
path: move validate_headref() to its only user
parse-options-cb: only abbreviate hashes when hash algo is known
attr: don't recompute default attribute source
attr: fix BUG() when parsing attrs outside of repo
remote-curl: fix parsing of detached SHA256 heads builtin/rev-parse: allow shortening to more than 40 hex characters builtin/blame: don't access potentially unitialized the_hash_algo builtin/bundle: abort "verify" early when there is no repository builtin/diff: explicitly set hash algo when there is no repo builtin/shortlog: don't set up revisions without repo oss-fuzz/commit-graph: set up hash algorithm
repository: stop setting SHA1 as the default object hash

 attr.c                       | 31 +++++++++++++++------
 builtin/blame.c              |  5 ++--
 builtin/bundle.c             |  5 ++++
 builtin/diff.c               |  9 ++++++
 builtin/rev-parse.c          |  5 ++--
 builtin/shortlog.c           |  2 +-
 oss-fuzz/fuzz-commit-graph.c |  1 +
 parse-options-cb.c           |  3 +-
 path.c                       | 53 ------------------------------------
 path.h                       |  1 -
 remote-curl.c                | 19 ++++++++++++-
 repository.c                 |  2 --
 setup.c                      | 53 ++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh        | 15 ++++++++++
 t/t0040-parse-options.sh     | 17 ++++++++++++
 t/t1500-rev-parse.sh         |  6 ++++
 t/t5550-http-fetch-dumb.sh   | 15 ++++++++++
 17 files changed, 168 insertions(+), 74 deletions(-)

Range-diff against v2:

 1:  a986b464d3 =  1:  5134f35cda path: harden validation of HEAD with non-standard hashes
 2:  a347c7e6ca =  2:  589b6a99ef path: move `validate_headref()` to its only user
 3:  c0a15b2fa6 =  3:  9a63c445d2 parse-options-cb: only abbreviate hashes when hash algo is known
 4:  1b5f904eed =  4:  929bacbfce attr: don't recompute default attribute source
 5:  26909daca4 =  5:  8f20aec1ee attr: fix BUG() when parsing attrs outside of repo
 6:  0b99184f50 =  6:  53439067a1 remote-curl: fix parsing of detached SHA256 heads
 7:  ccfda3c2d2 =  7:  1f74960760 builtin/rev-parse: allow shortening to more than 40 hex characters
 8:  1813e7eb5c =  8:  2d985abca1 builtin/blame: don't access potentially unitialized `the_hash_algo`
 9:  31182a1fc6 =  9:  f3b23d28aa builtin/bundle: abort "verify" early when there is no repository
10:  78e19d0a1b = 10:  7577b6b96c builtin/diff: explicitly set hash algo when there is no repo
11:  51bcddbc31 = 11:  509c79d1d3 builtin/shortlog: don't set up revisions without repo
 -:  ---------- > 12:  660f976129 oss-fuzz/commit-graph: set up hash algorithm
12:  e8126371e1 = 13:  95909c2da5 repository: stop setting SHA1 as the default object hash

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #40 lore raw

PATCH v3 1/13 path: harden validation of HEAD with non-standard hashes

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.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

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 repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -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;

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #41 lore raw

PATCH v3 2/13 path: move `validate_headref()` to its only user

In reply to Patrick Steinhardt

While validate_headref() is only called from is_git_directory() in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

path.c +0 ~~-53~~

index cc02165530..bd6e25245d 100644
@@ -5,7 +5,6 @@
 #include "abspath.h"
 #include "environment.h"
 #include "gettext.h"
-#include "hex.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "string-list.h"

@@ -647,58 +646,6 @@ void strbuf_git_common_path(struct strbuf *sb,
 	va_end(args);
 }
 
-int validate_headref(const char *path)
-{
-	struct stat st;
-	char buffer[256];
-	const char *refname;
-	struct object_id oid;
-	int fd;
-	ssize_t len;
-
-	if (lstat(path, &st) < 0)
-		return -1;
-
-	/* Make sure it is a "refs/.." symlink */
-	if (S_ISLNK(st.st_mode)) {
-		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
-			return 0;
-		return -1;
-	}
-
-	/*
-	 * Anything else, just open it and try to see if it is a symbolic ref.
-	 */
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		return -1;
-	len = read_in_full(fd, buffer, sizeof(buffer)-1);
-	close(fd);
-
-	if (len < 0)
-		return -1;
-	buffer[len] = '\0';
-
-	/*
-	 * Is it a symbolic ref?
-	 */
-	if (skip_prefix(buffer, "ref:", &refname)) {
-		while (isspace(*refname))
-			refname++;
-		if (starts_with(refname, "refs/"))
-			return 0;
-	}
-
-	/*
-	 * Is this a detached HEAD?
-	 */
-	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
-		return 0;
-
-	return -1;
-}
-
 static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;

path.h +0 -1

index ea96487b29..c3bc8617bd 100644
@@ -173,7 +173,6 @@ const char *git_path_fetch_head(struct repository *r);
 const char *git_path_shallow(struct repository *r);
 
 int ends_with_path_components(const char *path, const char *components);
-int validate_headref(const char *ref);
 
 int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);

setup.c +53 -0

65 lines not on this page · raw message

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #42 lore raw

PATCH v3 3/13 parse-options-cb: only abbreviate hashes when hash algo is known

In reply to Patrick Steinhardt

The OPT__ABBREV() option can be used to add an option that abbreviates object IDs. When given a length longer than the_hash_algo->hexsz, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have the_hash_algo initialized properly as the hash algorithm can only be set up after we have set up the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use OPT__ABBREV() also have RUN_SETUP set and thus cannot work without a repository. Consequently, both the_repository and the_hash_algo would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

parse-options-cb.c +2 -1

index bdc7fae497..d99d688d3c 100644
@@ -7,6 +7,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "setup.h"
 #include "string-list.h"
 #include "strvec.h"
 #include "oid-array.h"

@@ -29,7 +30,7 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > the_hash_algo->hexsz)
+		else if (startup_info->have_repository && v > the_hash_algo->hexsz)
 			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;

t/t0040-parse-options.sh +17 -0

index 8bb2a8b453..45a773642f 100755
@@ -176,6 +176,23 @@ test_expect_success 'long options' '
 	test_cmp expect output
 '
 
+test_expect_success 'abbreviate to something longer than SHA1 length' '
+	cat >expect <<-EOF &&
+	boolean: 0
+	integer: 0
+	magnitude: 0
+	timestamp: 0
+	string: (not set)
+	abbrev: 100
+	verbose: -1
+	quiet: 0
+	dry run: no
+	file: (not set)
+	EOF
+	test-tool parse-options --abbrev=100 >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'missing required value' '
 	cat >expect <<-\EOF &&
 	error: switch `s'\'' requires a value

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #43 lore raw

PATCH v3 4/13 attr: don't recompute default attribute source

In reply to Patrick Steinhardt

The default_attr_source() function lazily computes the attr source supposedly once, only. This is done via a static variable attr_source that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable has_attr_source to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the ignore_bad_attr_tree to not be static anymore as the code will only be executed once anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

attr.c +16 -9

index 679e42258c..9d911aeb31 100644
@@ -1206,15 +1206,16 @@ static void collect_some_attrs(struct index_state *istate,
 }
 
 static const char *default_attr_source_tree_object_name;
-static int ignore_bad_attr_tree;
 
 void set_git_attr_source(const char *tree_object_name)
 {
 	default_attr_source_tree_object_name = xstrdup(tree_object_name);
 }
 
-static void compute_default_attr_source(struct object_id *attr_source)
+static int compute_default_attr_source(struct object_id *attr_source)
 {
+	int ignore_bad_attr_tree = 0;
+
 	if (!default_attr_source_tree_object_name)
 		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);

@@ -1230,22 +1231,28 @@ static void compute_default_attr_source(struct object_id *attr_source)
 		ignore_bad_attr_tree = 1;
 	}
 
-	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
-		return;
+	if (!default_attr_source_tree_object_name)
+		return 0;
 
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
-				 attr_source) && !ignore_bad_attr_tree)
-		die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+				 attr_source)) {
+		if (!ignore_bad_attr_tree)
+			die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+		return 0;
+	}
+
+	return 1;
 }
 
 static struct object_id *default_attr_source(void)
 {
 	static struct object_id attr_source;
+	static int has_attr_source = -1;
 
-	if (is_null_oid(&attr_source))
-		compute_default_attr_source(&attr_source);
-	if (is_null_oid(&attr_source))
+	if (has_attr_source < 0)
+		has_attr_source = compute_default_attr_source(&attr_source);
+	if (!has_attr_source)
 		return NULL;
 	return &attr_source;
 }

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #44 lore raw

PATCH v3 5/13 attr: fix BUG() when parsing attrs outside of repo

In reply to Patrick Steinhardt

If either the --attr-source option or the GIT_ATTR_SOURCE envvar are set, then compute_default_attr_source() will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using git grep --no-index. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

attr.c +6 -0

index 9d911aeb31..4bd09bcb4b 100644
@@ -1234,6 +1234,12 @@ static int compute_default_attr_source(struct object_id *attr_source)
 	if (!default_attr_source_tree_object_name)
 		return 0;
 
+	if (!startup_info->have_repository) {
+		if (!ignore_bad_attr_tree)
+			die(_("cannot use --attr-source or GIT_ATTR_SOURCE without repo"));
+		return 0;
+	}
+
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
 				 attr_source)) {

t/t0003-attributes.sh +15 -0

index 774b52c298..3efdec54dd 100755
@@ -428,6 +428,21 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre
 	)
 '
 
+test_expect_success 'diff without repository with attr source' '
+	mkdir -p "$TRASH_DIRECTORY/outside/nongit" &&
+	(
+		cd "$TRASH_DIRECTORY/outside/nongit" &&
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		touch file &&
+		cat >expect <<-EOF &&
+		fatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo
+		EOF
+		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #45 lore raw

PATCH v3 6/13 remote-curl: fix parsing of detached SHA256 heads

In reply to Patrick Steinhardt

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via http_fetch_ref(). This function will either parse the file as an object ID in case it is exactly the_hash_algo->hexsz long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update the_hash_algo to the discovered remote object hash. Consequently, the_hash_algo will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up the_hash_algo via repo_set_hash_algo(). While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for the_hash_algo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

remote-curl.c +18 -1

index 0b6d7815fd..004b707fdf 100644
@@ -266,12 +266,23 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 	return list;
 }
 
+/*
+ * Try to detect the hash algorithm used by the remote repository when using
+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash
+ * directly have to derive it from the advertised ref lengths.
+ */
 static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)
 {
 	const char *p = memchr(heads->buf, '\t', heads->len);
 	int algo;
+
+	/*
+	 * In case the remote has no refs we have no way to reliably determine
+	 * the object hash used by that repository. In that case we simply fall
+	 * back to SHA1, which may or may not be correct.
+	 */
 	if (!p)
-		return the_hash_algo;
+		return &hash_algos[GIT_HASH_SHA1];
 
 	algo = hash_algo_by_length((p - heads->buf) / 2);
 	if (algo == GIT_HASH_UNKNOWN)

@@ -295,6 +306,12 @@ static struct ref *parse_info_refs(struct discovery *heads)
 		    "is this a git repository?",
 		    transport_anonymize_url(url.buf));
 
+	/*
+	 * Set the repository's hash algo to whatever we have just detected.
+	 * This ensures that we can correctly parse the remote references.
+	 */
+	repo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));
+
 	data = heads->buf;
 	start = NULL;
 	mid = data;

t/t5550-http-fetch-dumb.sh +15 -0

index 4c3b32785d..5f16cbc58d 100755
@@ -55,6 +55,21 @@ test_expect_success 'list refs from outside any repository' '
 	test_cmp expect actual
 '
 
+
+test_expect_success 'list detached HEAD from outside any repository' '
+	git clone --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" \
+		update-ref --no-deref HEAD refs/heads/main &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" update-server-info &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	nongit git ls-remote "$HTTPD_URL/dumb/repo-detached.git" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #46 lore raw

PATCH v3 7/13 builtin/rev-parse: allow shortening to more than 40 hex characters

In reply to Patrick Steinhardt

The --short= option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to the_hash_algo->hexsz before we have even set up the repo. Consequently, the_hash_algo will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing the_hash_algo only after we have set up the repo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

builtin/rev-parse.c +2 -3

index 624182e507..d7b87c605e 100644
@@ -687,7 +687,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	const char *name = NULL;
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
-	const int hexsz = the_hash_algo->hexsz;
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;

@@ -863,8 +862,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				abbrev = strtoul(arg, NULL, 10);
 				if (abbrev < MINIMUM_ABBREV)
 					abbrev = MINIMUM_ABBREV;
-				else if (hexsz <= abbrev)
-					abbrev = hexsz;
+				else if ((int)the_hash_algo->hexsz <= abbrev)
+					abbrev = the_hash_algo->hexsz;
 				continue;
 			}
 			if (!strcmp(arg, "--sq")) {

t/t1500-rev-parse.sh +6 -0

index a669e592f1..30c31918fd 100755
@@ -304,4 +304,10 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '
 	test_cmp expect actual
 '
 
+test_expect_success '--short= truncates to the actual hash length' '
+	git rev-parse HEAD >expect &&
+	git rev-parse --short=100 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #47 lore raw

PATCH v3 8/13 builtin/blame: don't access potentially unitialized `the_hash_algo`

In reply to Patrick Steinhardt

We access the_hash_algo in git-blame(1) before we have executed parse_options_start(), which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify RUN_SETUP and thus require a repository.

There is one exception though, namely when passing -h to print the help. Here we will access the_hash_algo even if there is no repo. This works fine right now because the_hash_algo gets sets up to point to the SHA1 algorithm via initialize_repository(). But we're about to stop doing this, and thus the code would lead to a NULL pointer exception.

Prepare the code for this and only access the_hash_algo after we are sure that there is a proper repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

builtin/blame.c +2 -3

index 9aa74680a3..e325825936 100644
@@ -915,7 +915,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct range_set ranges;
 	unsigned int range_i;
 	long anchor;
-	const int hexsz = the_hash_algo->hexsz;
 	long num_lines = 0;
 	const char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;
 	const char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;

@@ -973,11 +972,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	} else if (show_progress < 0)
 		show_progress = isatty(2);
 
-	if (0 < abbrev && abbrev < hexsz)
+	if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
 	else if (!abbrev)
-		abbrev = hexsz;
+		abbrev = the_hash_algo->hexsz;
 
 	if (revs_file && read_ancestry(revs_file))
 		die_errno("reading graft file '%s' failed", revs_file);

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:34 UTC #48 lore raw

PATCH v3 9/13 builtin/bundle: abort "verify" early when there is no repository

In reply to Patrick Steinhardt

Verifying a bundle requires us to have a repository. This is encoded in verify_bundle(), which will return an error if there is no repository. We call open_bundle() before we call verify_bundle() though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because open_bundle() already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing the_hash_algo when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

builtin/bundle.c +5 -0

index 3ad11dc5d0..d5d41a8f67 100644
@@ -140,6 +140,11 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (!startup_info->have_repository) {
+		ret = error(_("need a repository to verify a bundle"));
+		goto cleanup;
+	}
+
 	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:35 UTC #49 lore raw

PATCH v3 10/13 builtin/diff: explicitly set hash algo when there is no repo

In reply to Patrick Steinhardt

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that the_repository only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for the_repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

builtin/diff.c +9 -0

index 6e196e0c7d..58ec7e5da2 100644
@@ -465,6 +465,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
+	/*
+	 * When operating outside of a Git repository we need to have a hash
+	 * algorithm at hand so that we can generate the blob hashes. We
+	 * default to SHA1 here, but may eventually want to change this to be
+	 * configurable via a command line option.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	prefix = precompose_argv_prefix(argc, argv, prefix);

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:35 UTC #50 lore raw

PATCH v3 11/13 builtin/shortlog: don't set up revisions without repo

In reply to Patrick Steinhardt

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling setup_revisions(). While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call setup_revisions() when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/shortlog.c +1 -1

index 3c7cd2d6ef..d4daf31e22 100644
@@ -435,7 +435,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		usage_with_options(shortlog_usage, options);
 	}
 
-	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
+	if (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);
 	}

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:35 UTC #51 lore raw

PATCH v3 12/13 oss-fuzz/commit-graph: set up hash algorithm

In reply to Patrick Steinhardt

Our fuzzing setups don't work in a proper repository, but only use the in-memory configured the_repository. Consequently, we never go through the full repository setup procedures and thus do not set up the hash algo used by the repository.

The commit-graph fuzzer does rely on a properly initialized hash algo though. Initialize it explicitly.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 oss-fuzz/fuzz-commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

oss-fuzz/fuzz-commit-graph.c +1 -0

index 2992079dd9..94ecbb9242 100644
@@ -18,6 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 	 * touching the disk to keep the individual fuzz-test cases as fast as
 	 * possible.
 	 */
+	repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
 	the_repository->settings.commit_graph_generation_version = 2;
 	the_repository->settings.commit_graph_read_changed_paths = 1;
 	g = parse_commit_graph(&the_repository->settings, (void *)data, size);

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-04-29 06:35 UTC #52 lore raw

PATCH v3 13/13 repository: stop setting SHA1 as the default object hash

In reply to Patrick Steinhardt

During the startup of Git, we call initialize_the_repository() to set up the_repository as well as the_index. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that the_hash_algo is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via setup_git_directory(), which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access the_hash_algo when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that the_hash_algo will map to a NULL pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 repository.c | 2 --
 1 file changed, 2 deletions(-)

repository.c +0 -2

index e15b416944..b65b1a8c8b 100644
@@ -35,8 +35,6 @@ void initialize_the_repository(void)
 	the_repo.parsed_objects = parsed_object_pool_new();
 
 	index_state_init(&the_index, the_repository);
-
-	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,

Signature · 1 line

2.45.0-rc1

Patrick Steinhardt

author

2024-05-07 04:52 UTC #53 lore raw

v4 cover Stop relying on SHA1 fallback for `the_hash_algo`

In reply to Patrick Steinhardt

Hi,

this is the third version of my patch series that stops relying on the SHA1 fallback configured for the_hash_algo. Instead, any callers that access the_hash_algo without it being initialized will now crash hard. This is designed to catch various subtle bugs going forward.

The only change compared to v3 of this patch series is a rebase on top of the following two topics:

- ps/the-index-is-no-more, which causes a conflict in the final patch.

- jc/no-default-attr-tree-in-bare, which causes a conflict in the fourth patch.

Interestingly, the range diff does not surface the second resolved conflict. This is because the context is actually the same, but when applying the patch in question you would get a conflict. To aid with this I decided to generate patches with --unified=4, which surfaces the previously-conflicting hunk in compute_default_attr_source().

Patrick

Patrick Steinhardt (13):
path: harden validation of HEAD with non-standard hashes
path: move validate_headref() to its only user
parse-options-cb: only abbreviate hashes when hash algo is known
attr: don't recompute default attribute source
attr: fix BUG() when parsing attrs outside of repo
remote-curl: fix parsing of detached SHA256 heads builtin/rev-parse: allow shortening to more than 40 hex characters builtin/blame: don't access potentially unitialized the_hash_algo builtin/bundle: abort "verify" early when there is no repository builtin/diff: explicitly set hash algo when there is no repo builtin/shortlog: don't set up revisions without repo oss-fuzz/commit-graph: set up hash algorithm
repository: stop setting SHA1 as the default object hash

 attr.c                       | 31 +++++++++++++++------
 builtin/blame.c              |  5 ++--
 builtin/bundle.c             |  5 ++++
 builtin/diff.c               |  9 ++++++
 builtin/rev-parse.c          |  5 ++--
 builtin/shortlog.c           |  2 +-
 oss-fuzz/fuzz-commit-graph.c |  1 +
 parse-options-cb.c           |  3 +-
 path.c                       | 53 ------------------------------------
 path.h                       |  1 -
 remote-curl.c                | 19 ++++++++++++-
 repository.c                 | 20 --------------
 setup.c                      | 53 ++++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh        | 15 ++++++++++
 t/t0040-parse-options.sh     | 17 ++++++++++++
 t/t1500-rev-parse.sh         |  6 ++++
 t/t5550-http-fetch-dumb.sh   | 15 ++++++++++
 17 files changed, 168 insertions(+), 92 deletions(-)

Range-diff against v3:

 1:  5134f35cda =  1:  5ee372c2d8 path: harden validation of HEAD with non-standard hashes
 2:  589b6a99ef =  2:  ece0ab94a8 path: move `validate_headref()` to its only user
 3:  9a63c445d2 =  3:  1a0859eaf1 parse-options-cb: only abbreviate hashes when hash algo is known
 4:  929bacbfce =  4:  1204a34216 attr: don't recompute default attribute source
 5:  8f20aec1ee =  5:  f098ce9690 attr: fix BUG() when parsing attrs outside of repo
 6:  53439067a1 =  6:  e8876052ad remote-curl: fix parsing of detached SHA256 heads
 7:  1f74960760 =  7:  6116030310 builtin/rev-parse: allow shortening to more than 40 hex characters
 8:  2d985abca1 =  8:  872ded113e builtin/blame: don't access potentially unitialized `the_hash_algo`
 9:  f3b23d28aa =  9:  5b4a21d2ce builtin/bundle: abort "verify" early when there is no repository
10:  7577b6b96c = 10:  e7254ae757 builtin/diff: explicitly set hash algo when there is no repo
11:  509c79d1d3 = 11:  c21b15ba17 builtin/shortlog: don't set up revisions without repo
12:  660f976129 = 12:  f37beb0e89 oss-fuzz/commit-graph: set up hash algorithm
13:  95909c2da5 ! 13:  950b08bc78 repository: stop setting SHA1 as the default object hash

@@ Commit message

Signed-off-by Patrick Steinhardt <[email protected]>

## repository.c ##
-@@ repository.c: void initialize_the_repository(void)
- the_repo.parsed_objects = parsed_object_pool_new();
-
- index_state_init(&the_index, the_repository);
+@@ repository.c: void initialize_repository(struct repository *repo)
+ repo->parsed_objects = parsed_object_pool_new();
+ ALLOC_ARRAY(repo->index, 1);
+ index_state_init(repo->index, repo);
-
-- repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
+- /*
+- * Unfortunately, we need to keep this hack around for the time being:
+- *
+- * - Not setting up the hash algorithm for the_repository leads to
+- * crashes because the_hash_algo is a macro that expands to
+- * the_repository->hash_algo. So if Git commands try to access
+- * the_hash_algo without a Git directory we crash.
+- *
+- * - Setting up the hash algorithm to be SHA1 by default breaks other
+- * commands when running with SHA256.
+- *
+- * This is another point in case why having global state is a bad idea.
+- * Eventually, we should remove this hack and stop setting the hash
+- * algorithm in this function altogether. Instead, it should only ever
+- * be set via our repository setup procedures. But that requires more
+- * work.
+- */
+- if (repo == the_repository)
+- repo_set_hash_algo(repo, GIT_HASH_SHA1);
}

static void expand_base_dir(char **out, const char *in,

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:52 UTC #54 lore raw

PATCH v4 1/13 path: harden validation of HEAD with non-standard hashes

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.

One might be hard pressed to tighten the check even further and fully validate the file contents, not only the prefix. In practice though that wouldn't make a lot of sense as it could be that the repository uses a hash function that produces longer hashes than SHA256, but which the current version of Git doesn't understand yet. We'd still want to detect the repository as proper Git repository in that case, and we will fail eventually with a proper error message that the hash isn't understood when trying to set up the repository format.

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 repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

path.c +1 -1

index 67229edb9c..cc02165530 100644
@@ -692,9 +692,9 @@ 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;
 }

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:52 UTC #55 lore raw

PATCH v4 2/13 path: move `validate_headref()` to its only user

In reply to Patrick Steinhardt

While validate_headref() is only called from is_git_directory() in "setup.c", it is currently implemented in "path.c". Move it over such that it becomes clear that it is only really used during setup in order to discover repositories.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 path.c  | 53 -----------------------------------------------------
 path.h  |  1 -
 setup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 54 deletions(-)

path.c +0 ~~-53~~

index cc02165530..bd6e25245d 100644
@@ -4,9 +4,8 @@
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "environment.h"
 #include "gettext.h"
-#include "hex.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "string-list.h"
 #include "dir.h"

@@ -646,60 +645,8 @@ void strbuf_git_common_path(struct strbuf *sb,
 	do_git_common_path(repo, sb, fmt, args);
 	va_end(args);
 }
 
-int validate_headref(const char *path)
-{
-	struct stat st;
-	char buffer[256];
-	const char *refname;
-	struct object_id oid;
-	int fd;
-	ssize_t len;
-
-	if (lstat(path, &st) < 0)
-		return -1;
-
-	/* Make sure it is a "refs/.." symlink */
-	if (S_ISLNK(st.st_mode)) {
-		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
-			return 0;
-		return -1;
-	}
-
-	/*
-	 * Anything else, just open it and try to see if it is a symbolic ref.
-	 */
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		return -1;
-	len = read_in_full(fd, buffer, sizeof(buffer)-1);
-	close(fd);
-
-	if (len < 0)
-		return -1;
-	buffer[len] = '\0';
-
-	/*
-	 * Is it a symbolic ref?
-	 */
-	if (skip_prefix(buffer, "ref:", &refname)) {
-		while (isspace(*refname))
-			refname++;
-		if (starts_with(refname, "refs/"))
-			return 0;
-	}
-
-	/*
-	 * Is this a detached HEAD?
-	 */
-	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
-		return 0;
-
-	return -1;
-}
-
 static struct passwd *getpw_str(const char *username, size_t len)
 {
 	struct passwd *pw;
 	char *username_z = xmemdupz(username, len);

path.h +0 -1

index ea96487b29..c3bc8617bd 100644
@@ -172,9 +172,8 @@ const char *git_path_merge_head(struct repository *r);
 const char *git_path_fetch_head(struct repository *r);
 const char *git_path_shallow(struct repository *r);
 
 int ends_with_path_components(const char *path, const char *components);
-int validate_headref(const char *ref);
 
 int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);

setup.c +53 -0

69 lines not on this page · raw message

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #56 lore raw

PATCH v4 3/13 parse-options-cb: only abbreviate hashes when hash algo is known

In reply to Patrick Steinhardt

The OPT__ABBREV() option can be used to add an option that abbreviates object IDs. When given a length longer than the_hash_algo->hexsz, then it will instead set the length to that maximum length.

It may not always be guaranteed that we have the_hash_algo initialized properly as the hash algorithm can only be set up after we have set up the_repository. In that case, the hash would always be truncated to the hex length of SHA1, which may not be what the user desires.

In practice it's not a problem as all commands that use OPT__ABBREV() also have RUN_SETUP set and thus cannot work without a repository. Consequently, both the_repository and the_hash_algo would be properly set up.

Regardless of that, harden the code to not truncate the length when we didn't set up a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 parse-options-cb.c       |  3 ++-
 t/t0040-parse-options.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

parse-options-cb.c +2 -1

index bdc7fae497..d99d688d3c 100644
@@ -6,8 +6,9 @@
 #include "date.h"
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "setup.h"
 #include "string-list.h"
 #include "strvec.h"
 #include "oid-array.h"

@@ -28,9 +29,9 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 			return error(_("option `%s' expects a numerical value"),
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > the_hash_algo->hexsz)
+		else if (startup_info->have_repository && v > the_hash_algo->hexsz)
 			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;
 	return 0;

t/t0040-parse-options.sh +17 -0

index 8bb2a8b453..45a773642f 100755
@@ -175,8 +175,25 @@ test_expect_success 'long options' '
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
+test_expect_success 'abbreviate to something longer than SHA1 length' '
+	cat >expect <<-EOF &&
+	boolean: 0
+	integer: 0
+	magnitude: 0
+	timestamp: 0
+	string: (not set)
+	abbrev: 100
+	verbose: -1
+	quiet: 0
+	dry run: no
+	file: (not set)
+	EOF
+	test-tool parse-options --abbrev=100 >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'missing required value' '
 	cat >expect <<-\EOF &&
 	error: switch `s'\'' requires a value
 	EOF

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #57 lore raw

PATCH v4 4/13 attr: don't recompute default attribute source

In reply to Patrick Steinhardt

The default_attr_source() function lazily computes the attr source supposedly once, only. This is done via a static variable attr_source that contains the resolved object ID of the attr source's tree. If the variable is the null object ID then we try to look up the attr source, otherwise we skip over it.

This approach is flawed though: the variable will never be set to anything else but the null object ID in case there is no attr source. Consequently, we re-compute the information on every call. And in the worst case, when we silently ignore bad trees, this will cause us to try and look up the treeish every single time.

Improve this by introducing a separate variable has_attr_source to track whether we already computed the attr source and, if so, whether we have an attr source or not.

This also allows us to convert the ignore_bad_attr_tree to not be static anymore as the code will only be executed once anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

attr.c +16 -9

index 6af7151088..a5b717e4ce 100644
@@ -1205,41 +1205,48 @@ static void collect_some_attrs(struct index_state *istate,
 	fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
 }
 
 static const char *default_attr_source_tree_object_name;
-static int ignore_bad_attr_tree;
 
 void set_git_attr_source(const char *tree_object_name)
 {
 	default_attr_source_tree_object_name = xstrdup(tree_object_name);
 }
 
-static void compute_default_attr_source(struct object_id *attr_source)
+static int compute_default_attr_source(struct object_id *attr_source)
 {
+	int ignore_bad_attr_tree = 0;
+
 	if (!default_attr_source_tree_object_name)
 		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
 
 	if (!default_attr_source_tree_object_name && git_attr_tree) {
 		default_attr_source_tree_object_name = git_attr_tree;
 		ignore_bad_attr_tree = 1;
 	}
 
-	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
-		return;
+	if (!default_attr_source_tree_object_name)
+		return 0;
 
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
-				 attr_source) && !ignore_bad_attr_tree)
-		die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+				 attr_source)) {
+		if (!ignore_bad_attr_tree)
+			die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+		return 0;
+	}
+
+	return 1;
 }
 
 static struct object_id *default_attr_source(void)
 {
 	static struct object_id attr_source;
+	static int has_attr_source = -1;
 
-	if (is_null_oid(&attr_source))
-		compute_default_attr_source(&attr_source);
-	if (is_null_oid(&attr_source))
+	if (has_attr_source < 0)
+		has_attr_source = compute_default_attr_source(&attr_source);
+	if (!has_attr_source)
 		return NULL;
 	return &attr_source;
 }

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #58 lore raw

PATCH v4 5/13 attr: fix BUG() when parsing attrs outside of repo

In reply to Patrick Steinhardt

If either the --attr-source option or the GIT_ATTR_SOURCE envvar are set, then compute_default_attr_source() will try to look up the value as a treeish. It is possible to hit that function while outside of a Git repository though, for example when using git grep --no-index. In that case, Git will hit a bug because we try to look up the main ref store outside of a repository.

Handle the case gracefully and detect when we try to look up an attr source without a repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 attr.c                |  6 ++++++
 t/t0003-attributes.sh | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

attr.c +6 -0

index a5b717e4ce..c89ab3478e 100644
@@ -1226,8 +1226,14 @@ static int compute_default_attr_source(struct object_id *attr_source)
 
 	if (!default_attr_source_tree_object_name)
 		return 0;
 
+	if (!startup_info->have_repository) {
+		if (!ignore_bad_attr_tree)
+			die(_("cannot use --attr-source or GIT_ATTR_SOURCE without repo"));
+		return 0;
+	}
+
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
 				 attr_source)) {
 		if (!ignore_bad_attr_tree)

t/t0003-attributes.sh +15 -0

index d755cc3c29..72fadca1e8 100755
@@ -433,8 +433,23 @@ test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tre
 		test_cmp expect actual
 	)
 '
 
+test_expect_success 'diff without repository with attr source' '
+	mkdir -p "$TRASH_DIRECTORY/outside/nongit" &&
+	(
+		cd "$TRASH_DIRECTORY/outside/nongit" &&
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		touch file &&
+		cat >expect <<-EOF &&
+		fatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo
+		EOF
+		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&
+		test_cmp expect err
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&
 		attr_check_source foo/bar/f f tag-1 &&

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #59 lore raw

PATCH v4 6/13 remote-curl: fix parsing of detached SHA256 heads

In reply to Patrick Steinhardt

The dumb HTTP transport tries to read the remote HEAD reference by downloading the "HEAD" file and then parsing it via http_fetch_ref(). This function will either parse the file as an object ID in case it is exactly the_hash_algo->hexsz long, or otherwise it will check whether the reference starts with "ref :" and parse it as a symbolic ref.

This is broken when parsing detached HEADs of a remote SHA256 repository because we never update the_hash_algo to the discovered remote object hash. Consequently, the_hash_algo will always be the fallback SHA1 hash algorithm, which will cause us to fail parsing HEAD altogteher when it contains a SHA256 object ID.

Fix this issue by setting up the_hash_algo via repo_set_hash_algo(). While at it, let's make the expected SHA1 fallback explicit in our code, which also addresses an upcoming issue where we are going to remove the SHA1 fallback for the_hash_algo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 remote-curl.c              | 19 ++++++++++++++++++-
 t/t5550-http-fetch-dumb.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

remote-curl.c +18 -1

index 0b6d7815fd..004b707fdf 100644
@@ -265,14 +265,25 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 
 	return list;
 }
 
+/*
+ * Try to detect the hash algorithm used by the remote repository when using
+ * the dumb HTTP transport. As dumb transports cannot tell us the object hash
+ * directly have to derive it from the advertised ref lengths.
+ */
 static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)
 {
 	const char *p = memchr(heads->buf, '\t', heads->len);
 	int algo;
+
+	/*
+	 * In case the remote has no refs we have no way to reliably determine
+	 * the object hash used by that repository. In that case we simply fall
+	 * back to SHA1, which may or may not be correct.
+	 */
 	if (!p)
-		return the_hash_algo;
+		return &hash_algos[GIT_HASH_SHA1];
 
 	algo = hash_algo_by_length((p - heads->buf) / 2);
 	if (algo == GIT_HASH_UNKNOWN)
 		return NULL;

@@ -294,8 +305,14 @@ static struct ref *parse_info_refs(struct discovery *heads)
 		die("%sinfo/refs not valid: could not determine hash algorithm; "
 		    "is this a git repository?",
 		    transport_anonymize_url(url.buf));
 
+	/*
+	 * Set the repository's hash algo to whatever we have just detected.
+	 * This ensures that we can correctly parse the remote references.
+	 */
+	repo_set_hash_algo(the_repository, hash_algo_by_ptr(options.hash_algo));
+
 	data = heads->buf;
 	start = NULL;
 	mid = data;
 	while (i < heads->len) {

t/t5550-http-fetch-dumb.sh +15 -0

index 4c3b32785d..5f16cbc58d 100755
@@ -54,8 +54,23 @@ test_expect_success 'list refs from outside any repository' '
 	nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
 	test_cmp expect actual
 '
 
+
+test_expect_success 'list detached HEAD from outside any repository' '
+	git clone --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		"$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" \
+		update-ref --no-deref HEAD refs/heads/main &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo-detached.git" update-server-info &&
+	cat >expect <<-EOF &&
+	$(git rev-parse main)	HEAD
+	$(git rev-parse main)	refs/heads/main
+	EOF
+	nongit git ls-remote "$HTTPD_URL/dumb/repo-detached.git" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
 	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git"

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #60 lore raw

PATCH v4 7/13 builtin/rev-parse: allow shortening to more than 40 hex characters

In reply to Patrick Steinhardt

The --short= option for git-rev-parse(1) allows the user to specify to how many characters object IDs should be shortened to. The option is broken though for SHA256 repositories because we set the maximum allowed hash size to the_hash_algo->hexsz before we have even set up the repo. Consequently, the_hash_algo will always be SHA1 and thus we truncate every hash after at most 40 characters.

Fix this by accessing the_hash_algo only after we have set up the repo.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/rev-parse.c  | 5 ++---
 t/t1500-rev-parse.sh | 6 ++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

builtin/rev-parse.c +2 -3

index af79538632..7d10ee33c4 100644
@@ -686,9 +686,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	unsigned int flags = 0;
 	const char *name = NULL;
 	struct object_context unused;
 	struct strbuf buf = STRBUF_INIT;
-	const int hexsz = the_hash_algo->hexsz;
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;
 
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))

@@ -862,10 +861,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					continue;
 				abbrev = strtoul(arg, NULL, 10);
 				if (abbrev < MINIMUM_ABBREV)
 					abbrev = MINIMUM_ABBREV;
-				else if (hexsz <= abbrev)
-					abbrev = hexsz;
+				else if ((int)the_hash_algo->hexsz <= abbrev)
+					abbrev = the_hash_algo->hexsz;
 				continue;
 			}
 			if (!strcmp(arg, "--sq")) {
 				output_sq = 1;

t/t1500-rev-parse.sh +6 -0

index a669e592f1..30c31918fd 100755
@@ -303,5 +303,11 @@ test_expect_success 'rev-parse --bisect includes bad, excludes good' '
 	git rev-parse --symbolic-full-name --bisect >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success '--short= truncates to the actual hash length' '
+	git rev-parse HEAD >expect &&
+	git rev-parse --short=100 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #61 lore raw

PATCH v4 8/13 builtin/blame: don't access potentially unitialized `the_hash_algo`

In reply to Patrick Steinhardt

We access the_hash_algo in git-blame(1) before we have executed parse_options_start(), which may not be properly set up in case we have no repository. This is fine for most of the part because all the call paths that lead to it (git-blame(1), git-annotate(1) as well as git-pick-axe(1)) specify RUN_SETUP and thus require a repository.

There is one exception though, namely when passing -h to print the help. Here we will access the_hash_algo even if there is no repo. This works fine right now because the_hash_algo gets sets up to point to the SHA1 algorithm via initialize_repository(). But we're about to stop doing this, and thus the code would lead to a NULL pointer exception.

Prepare the code for this and only access the_hash_algo after we are sure that there is a proper repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

builtin/blame.c +2 -3

index 9aa74680a3..e325825936 100644
@@ -914,9 +914,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	int cmd_is_annotate = !strcmp(argv[0], "annotate");
 	struct range_set ranges;
 	unsigned int range_i;
 	long anchor;
-	const int hexsz = the_hash_algo->hexsz;
 	long num_lines = 0;
 	const char *str_usage = cmd_is_annotate ? annotate_usage : blame_usage;
 	const char **opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;

@@ -972,13 +971,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		show_progress = 0;
 	} else if (show_progress < 0)
 		show_progress = isatty(2);
 
-	if (0 < abbrev && abbrev < hexsz)
+	if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
 	else if (!abbrev)
-		abbrev = hexsz;
+		abbrev = the_hash_algo->hexsz;
 
 	if (revs_file && read_ancestry(revs_file))
 		die_errno("reading graft file '%s' failed", revs_file);

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #62 lore raw

PATCH v4 9/13 builtin/bundle: abort "verify" early when there is no repository

In reply to Patrick Steinhardt

Verifying a bundle requires us to have a repository. This is encoded in verify_bundle(), which will return an error if there is no repository. We call open_bundle() before we call verify_bundle() though, which already performs some verifications even though we may ultimately abort due to a missing repository.

This is problematic because open_bundle() already reads the bundle header and verifies that it contains a properly formatted hash. When there is no repository we have no clue what hash function to expect though, so we always end up assuming SHA1 here, which may or may not be correct. Furthermore, we are about to stop initializing the_hash_algo when there is no repository, which will lead to segfaults.

Check early on whether we have a repository to fix this issue.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/bundle.c | 5 +++++
 1 file changed, 5 insertions(+)

builtin/bundle.c +5 -0

index 3ad11dc5d0..d5d41a8f67 100644
@@ -139,8 +139,13 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (!startup_info->have_repository) {
+		ret = error(_("need a repository to verify a bundle"));
+		goto cleanup;
+	}
+
 	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #63 lore raw

PATCH v4 10/13 builtin/diff: explicitly set hash algo when there is no repo

In reply to Patrick Steinhardt

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line:

    ```
    diff --git a/a b/b
    index 7898192..6178079 100644
    --- a/a
    +++ b/b
    @@ -1 +1 @@
    -a
    +b
    ```

We implicitly use SHA1 to calculate the hash here, which is because the_repository gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that the_repository only ever has a hash function when it was properly initialized via a repo's configuration.

To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for the_repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/diff.c | 9 +++++++++
 1 file changed, 9 insertions(+)

builtin/diff.c +9 -0

index efc37483b3..9b6cdabe15 100644
@@ -464,8 +464,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 				!path_inside_repo(prefix, argv[i + 1]))))
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
+	/*
+	 * When operating outside of a Git repository we need to have a hash
+	 * algorithm at hand so that we can generate the blob hashes. We
+	 * default to SHA1 here, but may eventually want to change this to be
+	 * configurable via a command line option.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	prefix = precompose_argv_prefix(argc, argv, prefix);

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #64 lore raw

PATCH v4 11/13 builtin/shortlog: don't set up revisions without repo

In reply to Patrick Steinhardt

It is possible to run git-shortlog(1) outside of a repository by passing it output from git-log(1) via standard input. Obviously, as there is no repository in that context, it is thus unsupported to pass any revisions as arguments.

Regardless of that we still end up calling setup_revisions(). While that works alright, it is somewhat strange. Furthermore, this is about to cause problems when we unset the default object hash.

Refactor the code to only call setup_revisions() when we have a repository. This is safe to do as we already verify that there are no arguments when running outside of a repository anyway.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/shortlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/shortlog.c +1 -1

index 3c7cd2d6ef..d4daf31e22 100644
@@ -434,9 +434,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		error(_("too many arguments given outside repository"));
 		usage_with_options(shortlog_usage, options);
 	}
 
-	if (setup_revisions(argc, argv, &rev, NULL) != 1) {
+	if (!nongit && setup_revisions(argc, argv, &rev, NULL) != 1) {
 		error(_("unrecognized argument: %s"), argv[1]);
 		usage_with_options(shortlog_usage, options);
 	}

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #65 lore raw

PATCH v4 12/13 oss-fuzz/commit-graph: set up hash algorithm

In reply to Patrick Steinhardt

Our fuzzing setups don't work in a proper repository, but only use the in-memory configured the_repository. Consequently, we never go through the full repository setup procedures and thus do not set up the hash algo used by the repository.

The commit-graph fuzzer does rely on a properly initialized hash algo though. Initialize it explicitly.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 oss-fuzz/fuzz-commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

oss-fuzz/fuzz-commit-graph.c +1 -0

index fe15e2c225..75e668a057 100644
@@ -18,8 +18,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 	 * normally be read from the repository's gitdir. We want to avoid
 	 * touching the disk to keep the individual fuzz-test cases as fast as
 	 * possible.
 	 */
+	repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
 	the_repository->settings.commit_graph_generation_version = 2;
 	the_repository->settings.commit_graph_read_changed_paths = 1;
 	g = parse_commit_graph(&the_repository->settings, (void *)data, size);
 	repo_clear(the_repository);

Signature · 1 line

2.45.0

Patrick Steinhardt

author

2024-05-07 04:53 UTC #66 lore raw

PATCH v4 13/13 repository: stop setting SHA1 as the default object hash

In reply to Patrick Steinhardt

During the startup of Git, we call initialize_the_repository() to set up the_repository as well as the_index. Part of this setup is also to set the default object hash of the repository to SHA1. This has the effect that the_hash_algo is getting initialized to SHA1, as well. This default hash algorithm eventually gets overridden by most Git commands via setup_git_directory(), which also detects the actual hash algorithm used by the repository.

There are some commands though that don't access a repository at all, or at a later point only, and thus retain the default hash function for some amount of time. As some of the the preceding commits demonstrate, this can lead to subtle issues when we access the_hash_algo when no repository has been set up.

Address this issue by dropping the set up of the default hash algorithm completely. The effect of this is that the_hash_algo will map to a NULL pointer and thus cause Git to crash when something tries to access the hash algorithm without it being properly initialized. It thus forces all Git commands to explicitly set up the hash algorithm in case there is no repository.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 repository.c | 20 --------------------
 1 file changed, 20 deletions(-)

repository.c +0 ~~-20~~

index 2118f563e3..15c10015b0 100644
@@ -25,28 +25,8 @@ void initialize_repository(struct repository *repo)
 	repo->remote_state = remote_state_new();
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
-
-	/*
-	 * Unfortunately, we need to keep this hack around for the time being:
-	 *
-	 *   - Not setting up the hash algorithm for `the_repository` leads to
-	 *     crashes because `the_hash_algo` is a macro that expands to
-	 *     `the_repository->hash_algo`. So if Git commands try to access
-	 *     `the_hash_algo` without a Git directory we crash.
-	 *
-	 *   - Setting up the hash algorithm to be SHA1 by default breaks other
-	 *     commands when running with SHA256.
-	 *
-	 * This is another point in case why having global state is a bad idea.
-	 * Eventually, we should remove this hack and stop setting the hash
-	 * algorithm in this function altogether. Instead, it should only ever
-	 * be set via our repository setup procedures. But that requires more
-	 * work.
-	 */
-	if (repo == the_repository)
-		repo_set_hash_algo(repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,
 			    const char *base_dir, const char *def_in)

Signature · 1 line

2.45.0