Re: [PATCH v9 33/41] environment: add set_index_file()

[Christian Couder <christian.couder@gmail.com>]

On Mon, Aug 1, 2016 at 7:24 PM, Stefan Beller <sbeller@google.com> wrote:
> On Sat, Jul 30, 2016 at 10:25 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>
> I have reviewed briefly all 41 patches and generally they look good to me.
> There were some nits, which should not stop us from proceeding, though.
> This one however is not one of the nits.

Ok, thanks for your review. I will take another look at improving the
comments related to this patch.

>> And yeah this is a short cut and this new function should not be used
>> by other code.
>
>> + * Yeah, the libification of 'apply' took a short-circuit by adding this
>> + * technical debt; please do not call this function in new codepaths.
>> + */
>
> How much effort would it take to not introduce this technical debt?
> Do you consider getting rid of it anytime in the future?

I don't know exactly how much effort it would take to reduce this
technical debt. I discussed that with Duy already:

https://public-inbox.org/git/CAP8UFD1SSjSGV%2B1e%2BzP4s%3Dp%2BohgSz6mct3EhPXqbde_y48ST7g%40mail.gmail.com/

and both Duy and Junio thought that it is ok to add this small
technical debt now.

My opinion is that most of this technical debt already exists as the
apply code and a lot of other "libified" code already call functions
like read_cache(), discard_cache() and cache_name_pos() instead of
functions like read_index_from(), discard_index() and
index_name_pos().

And it is not as simple as just changing these functions in apply.c,
as these functions can be called by other "libified" code that can
indirectly be called by apply code.

For example cache_name_pos() is used in dir.c and diff.c, and then
dir.h and diff.h are included in many other "libified" *.c files
(including "apply.c"). So it is very difficult to make sure that apply
code doesn't indirectly call any of the problematic functions.

And even if it was possible to make sure that now apply.c doesn't
indirectly call any of these functions, how could we make sure that
later a refactoring in other "libified" code will not change a
function that is indirectly called by apply.c to make it call another
function that indirectly calls a problematic function?

That's why I think it's a different project to remove calls to
problematic functions (like read_cache(), discard_cache(),
cache_name_pos() and so on) in all the libified code.

> Can you clarify in the commit message why you introduce this technical
> debt and justify why the reviewers should be fine with it?

Ok, I will add some of the above in the commit message.

> ("It is better than before, because ..." instead of "Yeah I know it's bad".
> Tell us why it is bad, and why it is not as bad as we thought?)
>
> In cache.h we have a NO_THE_INDEX_COMPATIBILITY_MACROS,
> and lots of
>   #define foo_bar(..) frob_bar(&the_index, (..))
>
> Could you operate on the raw functions that take pointers to &the_index
> and point these to a temporary index?

As explained above this is not enough.
If I did only that, it could even give a false sense of security.

>> +/*
>> + * Temporarily change the index file.
>> + * Please save the current index file using get_index_file() before changing
>> + * the index file. And when finished, reset it to the saved value.
>
> Should this warning go inside cache.h?

Ok, I will add one there too.

> Or should we make that implicit by providing a stack for the user
>
>     extern void push_new_index_file(const char *index);
>     extern int pop_index_file_redirection();
>
> ?

I don't think this is necessary. It could even encourage using these functions.

Thanks,
Christian.