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

["Nguyen Thai Ngoc Duy" <pclouds@gmail.com>]

On Thu, Jun 5, 2008 at 1:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc 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.

When you do narrow checkout. You only have a subdirectory, so you
would not want some commands to accidentally change things outside
that subdirectory in index.

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

Well it could be extended to support multiple path separated by colon
later if someone needs it :)

> 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'?").

Simpler handling. Maybe it should append slash by itself if missing.

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

Because unpack_trees writes new index from scratch so it will always
violate that. That check is IMO enough for simple index manipulation
like add/remove an entry. For unpack_trees, I have
check_index_prefix() to match a temporary index with  current index
before it gets written to disk.

Yes I did miss partial commit. Did not know it used temporary index too.

> [the rest]

OK will do.

-- 
Duy