Re: [PATCH v3] sha1_file: do not add own object directory as alternate

[Junio C Hamano <gitster@pobox.com>]

Ephrim Khong <dr.khong@gmail.com> writes:

> @@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int
>  			return -1;
>  		}
>  	}
> -	if (!strcmp(ent->base, objdir)) {
> +	if (!strcmp_icase(ent->base, normalized_objdir)) {

Not a problem with your patch, but we should rethink the name of
this function when the code base is more quiet.  It always makes me
wonder if it is something similar to strcasecmp(), but in fact it is
not.  It is meant to be used *only* for pathnames; pathname_cmp() or
something that has "path" in its name would be appropriate, but it
is wrong to call it "str"-anything.

> @@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
>  	struct string_list entries = STRING_LIST_INIT_NODUP;
>  	char *alt_copy;
>  	int i;
> +	struct strbuf objdirbuf = STRBUF_INIT;
>
>  	if (depth > 5) {
>  		error("%s: ignoring alternate object stores, nesting too deep.",
> @@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
>  		return;
>  	}
>
> +	strbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));
> +	normalize_path_copy(objdirbuf.buf, objdirbuf.buf);

This is somewhat a strange usage of a strbuf.

 - it relies on that normalize_path_copy() only shrinks, never
   lengthens, which is not too bad;

 - if the operation ever shrinks, objdirbuf.len becomes
   meaningless.  The allocated length is objdirbuf.alloc, length
   of the string is strlen(objdirbuf.buf).

 - abspath.c::absolute_path() is still restricted to PATH_MAX, so
   you are not gaining much by using strbuf here.

But at least this patch is not making things any worse, so....

> @@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
>  			error("%s: ignoring relative alternate object store %s",
>  					relative_base, entry);
>  		} else {
> -			link_alt_odb_entry(entry, relative_base, depth);
> +			link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
>  		}
>  	}
>  	string_list_clear(&entries, 0);
>  	free(alt_copy);
> +	strbuf_release(&objdirbuf);
>  }
>
>  void read_info_alternates(const char * relative_base, int depth)
> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh
> new file mode 100755
> index 0000000..8341d46
> --- /dev/null
> +++ b/t/t7702-repack-cyclic-alternate.sh

Do we really have to waste a new test file only for this test?

Don't we have any test that already uses alternate that these two
new test pieces can be added to?

    $ git grep info/alternates t/

seems to show a few existing ones, including 1450 (fsck) and 7700
(repack) that look very relevant (I didn't check what the tests in
them are about, though).