Re: [PATCH v10 33/40] environment: add set_index_file()

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 10, 2016, 16:52 UTC
Message-ID: <CAP8UFD2ZAdUjQnO-4qnum2_AK84SfBN2_yO=py+Jj+pkV8pk-w@mail.gmail.com>
In-Reply-To: <xmqq60raewod.fsf@gitster.mtv.corp.google.com>

On Tue, Aug 9, 2016 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 43 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> Now if someone really needs to use this new function, 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);
>>
>> It is intended to be used by builtins commands, in fact only `git am`,
>> 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.
>
> That is OK, but I do not think NO_THE_INDEX_COMPATIBILITY_MACROS has
> much to do with this hack.  Even if you stop using the_index and
> have the caller pass its own temporary index_state, that structure
> does *not* know which file to read the (temporary) index from, or
> which file to write the (temporary) index to.  In fact, apply.c
> already does this in build_fake_ancestor():
>
>     static int build_fake_ancestor(struct patch *list, const char *filename)
>     {
>             ...
>             hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
>             res = write_locked_index(&result, &lock, COMMIT_LOCK);
>             ...
>     }
>
> As you can see, this function works with a non-standard/default
> index file _without_ having to use non-default index_state.  What
> the set_index_file() hack allows you to do is to use interface that
> does *NOT* pass "filename" like the caller does to this function.
>
> Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
> comments (there are two) pure red-herring?

Yeah, true.

So do you want me to refactor the code to use hold_lock_file_for_update() instead of hold_locked_index() and to avoid the set_index_file() hack?

Or would the set_index_file() hack be ok with a commit message like the following:

--- Introduce set_index_file() to be able to temporarily change the index file.

Yeah, this is a short cut and this new function should not be used by other code.

It adds a small technical debt, that could perhaps be avoided with a refactoring and by using hold_lock_file_for_update() instead of hold_locked_index() to pass the filename where the index should be written.

Now if someone really needs to use this new function, 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);

It is intended to be used by builtins commands, in fact only `git am`, 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. ---

And with comments like this on top of set_index_file() definition and declaration:

/*
 * Hack to temporarily change the index.
 * Yeah, the libification of 'apply' took a short-circuit by adding
 * this technical debt.
 * Please set the filename using for example hold_lock_file_for_update(),
 * instead of this function.
 * If you really need to use this function, please save the current
 * index file using get_index_file() before changing the index
 * file. And when finished, reset it to the saved value.
 */