Re: [WIP/RFC PATCH 1/2] Introduce GIT_INDEX_PREFIX

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

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> GIT_INDEX_PREFIX is used to limit write access to a specific directory.
> Only "important" information is protected by index prefix (those will
> be used to create tree objects)
>
> When GIT_INDEX_PREFIX is set, any attempt to modify the index (refresh
> it is okay though) will bail out. read-tree and merge, however, can
> write to full index. For merge, no conflict is allowed outside index
> prefix.

This is kind of hard to judge as part of "narrow checkout" series, because
it is not clear how this will actually _help_ narrow checkout.

In other words, as a standalone "protect parts outside a single
subdirectory" it can be reviewed and judged, but it is unclear how it
would help narrow checkout if you excempted only a _single_ subdirectory.
E.g. you might want to limit yourself to arch/x86 _and_ include/asm-x86.

I sympathize that this series will be hard to get right as the places _you
identify_ in the code as "touching index entries" are the only places that
you _could_ write test scripts for --- if you missed one codepath it would
be easy for you to forget writing the test for that codepath.

Having said that...

> @@ -71,6 +73,9 @@ static void setup_git_env(void)
>  	git_graft_file = getenv(GRAFT_ENVIRONMENT);
>  	if (!git_graft_file)
>  		git_graft_file = xstrdup(git_path("info/grafts"));
> +	index_prefix = getenv(INDEX_PREFIX_ENVIRONMENT);
> +	if (index_prefix && (!*index_prefix || index_prefix[strlen(index_prefix)-1] != '/'))
> +		die("GIT_INDEX_PREFIX must end with a slash");

Not nice (aka "why 'must'?").

> diff --git a/read-cache.c b/read-cache.c
> index ac9a8e7..4f8d44b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -23,6 +23,11 @@
>  
>  struct index_state the_index;
>  
> +static int outside_index_prefix(const struct index_state *istate, const char *ce_name)
> +{
> +	return istate == &the_index && get_index_prefix() && prefixcmp(ce_name, get_index_prefix());
> +}

The first check above needs to be justified.

If you say "outside of this path are off-limits", why do you allow a
temporary index that is used during a partial commit and other
index_states excempt from that rule?

> @@ -793,21 +804,35 @@ static int check_file_directory_conflict(struct index_state *istate,
>  	return retval + has_dir_name(istate, ce, pos, ok_to_replace);
>  }
>  
> +static int ce_compare(const struct cache_entry *ce1, const struct cache_entry *ce2)
> +{
> +	return ce1->ce_mode == ce2->ce_mode &&
> +		((ce1->ce_flags ^ ce2->ce_flags) & ~(CE_HASHED | CE_UPDATE)) == 0 &&
> +		!memcmp(ce1->sha1, ce2->sha1, 20) &&
> +		!strcmp(ce1->name, ce2->name);
> +}

This needs a bit of commenting to explain why HASHED and UPDATE are
specifically ignored, so that people who later add their own bit to the
flag can easily tell if their new bit should also be ignored.

> +	cache1 = the_index.cache;
> +	cache2 = index->cache;
> +	start = 0;
> +	end1 = the_index.cache_nr ? the_index.cache_nr - 1 : 0;
> +	end2 = index->cache_nr ? index->cache_nr - 1 : 0;
> +	while (start < end1 && start < end2 &&
> +		ce_compare(cache1[start], cache2[start]))
> +		start ++;
> +
> +	while (end1 > start && end2 > start &&
> +		ce_compare(cache1[end1], cache2[end2])) {
> +		end1 --;
> +		end2 --;
> +	}

Style.  Drop excess SP before post-increment and -decrement.

> +	/*
> +	 * everything in start..end1 and start..end2 must
> +	 * be prefixed by get_index_prefix()
> +	 */
> +	if (start < end1 &&
> +		(prefixcmp(cache1[start]->name, index_prefix) ||
> +		prefixcmp(cache1[end1]->name, index_prefix)))
> +		return 1;

It is conventional to return negative or 0 for a function that checks "is
this kosher".  Returning 0/1 risks getting misunderstood that this returns
ok for 1 and bad for 0.