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

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

On Thu, Jun 5, 2008 at 11:35 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Thu, 5 Jun 2008, Nguyen Thai Ngoc Duy wrote:
>
>> 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.
>
> When they operate purely on the index?  Why not?

They don't. But if they only operate on a subdirectory only, they must
operate only on that subdirectory in index (exceptions: git-merge and
git-read-tree)

>
> I guess that Junio's point was that the semantics are not at all clear.

On the second thought, I think this series is not a requirement for
narrow checkout. But it does help to
 -  detect any index violation (from unsupported commands)
 - draw an inititial path to index manipulation, needed for narrow
checkout (cache_refresh_ent() will be enhanced not to stat() outside
subtree though)

>
>> > 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 :)
>
> The thing is: if the concept is sound, chances are that it is _easy_ to
> support that, should someone need it.

It should be easy except check_index_prefix() for multiple index prefixes.

>
>> >> @@ -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.
>
> Not maybe.  Definitely.  That was what the comment was about.

OK. Definitetly :)

>
>> >> 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.
>
> unpack_trees() is what drives merges.  Does that mean that the only
> opportunity to end up with conflicts is _not_ prevented?

I am not sure I understand this. In my opinion, due to unpack_trees()
behavior, it is free to write to temporary indexes. I don't know if
any channce conflicts will be generated without unpack_trees(). But
then if an index is to be written to .git/index, it must be checked
against .git/index plus index prefix. Any opportunity that ends up
with conflicts does not matter, as long as it will not be written to
.git/index or conflicts are visible in narrow checkout. Otherwise it
must be checked. What I care is that .git/index is controlled, so that
people won't be able to change anything outside index prefix by
accident.

> Ciao,
> Dscho
>
>



-- 
Duy