Re: [PATCH v8 32/41] environment: add set_index_file()

From: Christian Couder <christian.couder@gmail.com>
Date: Jul 29, 2016, 14:21 UTC
Message-ID: <CAP8UFD1SSjSGV+1e+zP4s=p+ohgSz6mct3EhPXqbde_y48ST7g@mail.gmail.com>
In-Reply-To: <CACsJy8Bi00yBb6UPm7_x8BDnwQKCXXpOPbswKtayRofmSOstcg@mail.gmail.com>

On Wed, Jul 27, 2016 at 5:14 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 22 quoted lines

> On Tue, Jul 26, 2016 at 9:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> Introduce set_index_file() to be able to temporarily change the index file.
>>>
>>> It should be used like this:
>>>
>>>     /* Save current index file */
>>>     old_index_file = get_index_file();
>>>     set_index_file((char *)tmp_index_file);
>>>
>>>     /* Do stuff that will use tmp_index_file as the index file */
>>>     ...
>>>
>>>     /* When finished reset the index file */
>>>     set_index_file(old_index_file);
>>
>> WHY is glaringly missing.
>
> It's used in a4aaa23 (builtin/am: use apply api in run_apply() -
> 2016-06-27) as a straight conversion of "GIT_INDEX_FILE=xxx git apply"
> .

Yeah, I would add something like the following to clarify the WHY:

"It is intended to be used by builtins commands to temporarily change the index file used by libified code.

This is useful when libified code uses the global index, but a builtin command wants another index file to be used instead."

>> Doesn't calling this have a global effect that is flowned upon when
>> used by a library-ish function?  Who is the expected caller in this
>> series that wants to "libify" a part of the system?

The expected caller is a builtin, like "builtin/am.c".

> I agree we should avoid this. There's a bunch of cache_name_pos() (and
> even read_cache()) in the libified apply.c, those will need to be
> converted  to take struct index_state* directly (read_index_from or
> index_name_pos).

There is a lot of other "libified" code that is calling these functions:

$ git grep -l cache_name_pos -- '*.c' | grep -v builtin apply.c diff.c dir.c merge-recursive.c pathspec.c rerere.c sha1_name.c submodule.c wt-status.c

$ git grep -l read_cache -- '*.c' | grep -v builtin | egrep -v '^t/' apply.c check-racy.c diff.c dir.c merge-recursive.c merge.c read-cache.c rerere.c revision.c sequencer.c sha1_name.c submodule.c

and some of that code is perhaps directly and indirectly called by the libified apply code.

Also I am not sure that read_cache() and cache_name_pos() are the only functions that should be changed.

So it looks like it is a very big and different project to make the current libified code be explicit about which index it is using. And by the way perhaps this other project should just remove the "the_index" global altogether.

> But at least read-cache.c has supported multiple
> indexes for a long time.

This is great, but it is unfortunately not the only lib file involved.