On Tue, Aug 9, 2016 at 12:13 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 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.
*/
?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
