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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2014, 19:26 UTC
Message-ID: <xmqqd2d6v2yf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53C510A9.4010208@gmail.com>

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

Show 6 quoted lines

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

Show 14 quoted lines

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

Show 19 quoted lines

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