git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] object-file: use real paths when adding alternates

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 22, 2022, 00:56 UTC
Message-ID
<221122.868rk3bxbb.gmgdl@evledraar.gmail.com>
In-Reply-To
<pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com>
On Mon, Nov 21 2022, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <chooglen@google.com>
Aside from some small nits this looks good to me.
Show 25 quoted lines
>  object-file.c     | 12 ++++++------
>  t/t7700-repack.sh | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 957790098fa..ef2b762234d 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  {
>  	struct object_directory *ent;
>  	struct strbuf pathbuf = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
>  	khiter_t pos;
>  
>  	if (!is_absolute_path(entry->buf) && relative_base) {
> @@ -516,12 +517,14 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  	}
>  	strbuf_addbuf(&pathbuf, entry);
>  
> -	if (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {
> +	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
>  		error(_("unable to normalize alternate object path: %s"),
> -		      pathbuf.buf);
> +			pathbuf.buf);
This is a mis-indentation, it was OK in the pre-image, not now.
>  		strbuf_release(&pathbuf);

Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the &tmp you pass in here) and then "does a "goto error_out".

It then *resets* the strbuf, but doesn't release it, assuming that you're going to pass it in again. So in that case we'd leak here, no?

I.e. a NULL return value from strbuf_realpath() doesn't mean that it didn't allocate in the scratch area passed to it, so we need to strbuf_release(&tmp) here too.

Perhaps this on top is simpler (but also see below)?:
	
	diff --git a/object-file.c b/object-file.c
	index ef2b762234d..d5d502504bb 100644
	--- a/object-file.c
	+++ b/object-file.c
	@@ -510,6 +510,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	struct strbuf pathbuf = STRBUF_INIT;
	 	struct strbuf tmp = STRBUF_INIT;
	 	khiter_t pos;
	+	int ret = -1;
	 
	 	if (!is_absolute_path(entry->buf) && relative_base) {
	 		strbuf_realpath(&pathbuf, relative_base, 1);
	@@ -520,11 +521,9 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
	 		error(_("unable to normalize alternate object path: %s"),
	 			pathbuf.buf);
	-		strbuf_release(&pathbuf);
	-		return -1;
	+		goto error;
	 	}
	 	strbuf_swap(&pathbuf, &tmp);
	-	strbuf_release(&tmp);
	 
	 	/*
	 	 * The trailing slash after the directory name is given by
	@@ -533,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
	 		strbuf_setlen(&pathbuf, pathbuf.len - 1);
	 
	-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
	-		strbuf_release(&pathbuf);
	-		return -1;
	-	}
	+	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos))
	+		goto error;
	 
	 	CALLOC_ARRAY(ent, 1);
	 	/* pathbuf.buf is already in r->objects->odb_by_path */
	@@ -552,7 +549,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
	 	/* recursively add alternates */
	 	read_info_alternates(r, ent->path, depth + 1);
	 
	-	return 0;
	+	ret = 0;
	+error:
	+	strbuf_release(&tmp);
	+	strbuf_release(&pathbuf);
	+	return ret;
	 }
	 
	 static const char *parse_alt_odb_entry(const char *string,
Show 32 quoted lines
>  		return -1;
>  	}
> +	strbuf_swap(&pathbuf, &tmp);
> +	strbuf_release(&tmp);
>  
>  	/*
>  	 * The trailing slash after the directory name is given by
> @@ -596,10 +599,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
>  		return;
>  	}
>  
> -	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
> -	if (strbuf_normalize_path(&objdirbuf) < 0)
> -		die(_("unable to normalize object directory: %s"),
> -		    objdirbuf.buf);
> +	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);
>  
>  	while (*alt) {
>  		alt = parse_alt_odb_entry(alt, sep, &entry);
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 5be483bf887..ce1954d0977 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
>  	test_has_duplicate_object false
>  '
>  
> +test_expect_success '--local keeps packs when alternate is objectdir ' '
> +	git init alt_symlink &&
> +	(
> +		cd alt_symlink &&
> +		git init &&
The tests pass without this re-"git init", left over from development?
Show 13 quoted lines
> +		echo content >file4 &&
> +		git add file4 &&
> +		git commit -m commit_file4 &&
> +		git repack -a &&
> +		ls .git/objects/pack/*.pack >../expect &&
> +		ln -s objects .git/alt_objects &&
> +		echo "$(pwd)/.git/alt_objects" >.git/objects/info/alternates &&
> +		git repack -a -d -l &&
> +		ls .git/objects/pack/*.pack >../actual
> +	) &&
> +	test_cmp expect actual
> +'
> +
I think this is better squashed in:
	
	diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
	index ce1954d0977..79eef5b4aa7 100755
	--- a/t/t7700-repack.sh
	+++ b/t/t7700-repack.sh
	@@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' '
	 '
	 
	 test_expect_success '--local keeps packs when alternate is objectdir ' '
	-	git init alt_symlink &&
	+	test_when_finished "rm -rf repo" &&
	+	git init repo &&
	+	test_commit -C repo A &&
	 	(
	-		cd alt_symlink &&
	-		git init &&
	-		echo content >file4 &&
	-		git add file4 &&
	-		git commit -m commit_file4 &&
	+		cd repo &&
	 		git repack -a &&
	 		ls .git/objects/pack/*.pack >../expect &&
	 		ln -s objects .git/alt_objects &&
Because:
 * If it's not a setup for a later test let's call it "repo" and clean
   it up at the end.
 * The "file4" you're creating doesn't go with the existing pattern, the
   file{1..3} are created in the top-level .git, here you're making a
   file4 in another repo.
   This just calls it "A.t", and makes it with test_commit, since all
   you need is a dummy commit.
 * I think we typically use "find .. -type f", not "ls", see
   e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think
   aside from that test there's some other "let's compare the packed
   before & after" in the test suite, but I can't remember offhand...
Previous: Glen Choo via GitGitGadgetNext: Jeff King
Message 9 of 17 in “object-file: use real paths when adding alternates”
  1. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 17, 2022
  2. Jeff KingNov 17, 2022
  3. Ævar Arnfjörð BjarmasonNov 17, 2022
  4. Jeff KingNov 17, 2022
  5. Taylor BlauNov 17, 2022
  6. Glen ChooNov 18, 2022
  7. Taylor BlauNov 17, 2022
  8. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 21, 2022
  9. Ævar Arnfjörð BjarmasonNov 22, 2022
  10. Jeff KingNov 22, 2022
  11. Glen ChooNov 24, 2022
  12. Jeff KingNov 24, 2022
  13. Glen ChooNov 24, 2022
  14. Jeff KingNov 22, 2022
  15. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 24, 2022
  16. Jeff KingNov 24, 2022
  17. Junio C HamanoNov 25, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.