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

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

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

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