On Sat, Jul 30, 2016 at 10:25 AM, Christian Couder
<christian.couder@xxxxxxxxx> 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.
>
> 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?
Can you clarify in the commit message why you introduce this technical
debt and justify why the reviewers should be fine with it?
("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?
I could imagine that this would lead to a lot of
passing around an index pointer in the am code and
in the lower level stuff, which would make the function signatures
ugly and heavy to use.
This specifically sets the index file, would the same be possible
for just resetting the &the_index pointer?
(Conceptually the same, but slightly more leightweight?)
>
> +/*
> + * 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?
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();
?
Thanks,
Stefan
--
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