git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Christian Couder <christian.couder@gmail.com>
Date
Aug 3, 2016, 06:41 UTC
Message-ID
<CAP8UFD1d0P_mZQcHOpq7Kki5jngKwBdyZ0n_dWzkmWzOOOxhRA@mail.gmail.com>
In-Reply-To
<CAGZ79kYzxCq6M49Zmf_4J7xTdXHMunvgPuFaLnV7fXtuTbEogQ@mail.gmail.com>
On Mon, Aug 1, 2016 at 7:24 PM, Stefan Beller <sbeller@google.com> wrote:
Show 6 quoted lines
> 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.

Show 9 quoted lines
>> 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.
Show 9 quoted lines
> ("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.

Show 6 quoted lines
>> +/*
>> + * 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.
Show 6 quoted lines
> 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.

Previous: Christian CouderNext: Christian Couder
Message 35 of 53 in “libify apply and use lib in am, part 2”
  1. 00/41 libify apply and use lib in am, part 2Christian Couder, Jul 30, 2016
  2. 01/41 apply: make some names more specificChristian Couder, Jul 30, 2016
  3. 02/41 apply: move 'struct apply_state' to apply.hChristian Couder, Jul 30, 2016
  4. 03/41 builtin/apply: make apply_patch() return -1 or -128 instead of die()ingChristian Couder, Jul 30, 2016
  5. Stefan BellerAug 1, 2016
  6. 04/41 builtin/apply: read_patch_file() return -1 instead of die()ingChristian Couder, Jul 30, 2016
  7. Stefan BellerAug 1, 2016
  8. 05/41 builtin/apply: make find_header() return -128 instead of die()ingChristian Couder, Jul 30, 2016
  9. 06/41 builtin/apply: make parse_chunk() return a negative integer on errorChristian Couder, Jul 30, 2016
  10. 07/41 builtin/apply: make parse_single_patch() return -1 on errorChristian Couder, Jul 30, 2016
  11. 20/41 builtin/apply: make remove_file() return -1 on errorChristian Couder, Jul 30, 2016
  12. 16/41 builtin/apply: make gitdiff_*() return 1 at end of headerChristian Couder, Jul 30, 2016
  13. 18/41 builtin/apply: change die_on_unsafe_path() to check_unsafe_path()Christian Couder, Jul 30, 2016
  14. 23/41 builtin/apply: make create_file() return -1 on errorChristian Couder, Jul 30, 2016
  15. 25/41 builtin/apply: make write_out_results() return -1 on errorChristian Couder, Jul 30, 2016
  16. 27/41 builtin/apply: make create_one_file() return -1 on errorChristian Couder, Jul 30, 2016
  17. 21/41 builtin/apply: make add_conflicted_stages_file() return -1 on errorChristian Couder, Jul 30, 2016
  18. 24/41 builtin/apply: make write_out_one_result() return -1 on errorChristian Couder, Jul 30, 2016
  19. 29/41 apply: rename and move opt constants to apply.hChristian Couder, Jul 30, 2016
  20. 36/41 apply: don't print on stdout in verbosity_silent modeChristian Couder, Jul 30, 2016
  21. 31/41 apply: make some parsing functions static againChristian Couder, Jul 30, 2016
  22. 22/41 builtin/apply: make add_index_file() return -1 on errorChristian Couder, Jul 30, 2016
  23. 37/41 usage: add set_warn_routine()Christian Couder, Jul 30, 2016
  24. 19/41 builtin/apply: make build_fake_ancestor() return -1 on errorChristian Couder, Jul 30, 2016
  25. 26/41 builtin/apply: make try_create_file() return -1 on errorChristian Couder, Jul 30, 2016
  26. 39/41 apply: change error_routine when silentChristian Couder, Jul 30, 2016
  27. Stefan BellerAug 1, 2016
  28. Christian CouderAug 8, 2016
  29. 17/41 builtin/apply: make gitdiff_*() return -1 on errorChristian Couder, Jul 30, 2016
  30. 33/41 environment: add set_index_file()Christian Couder, Jul 30, 2016
  31. Stefan BellerAug 1, 2016
  32. Junio C HamanoAug 1, 2016
  33. Junio C HamanoAug 1, 2016
  34. Christian CouderAug 3, 2016
  35. Christian CouderAug 3, 2016
  36. 28/41 builtin/apply: rename option parsing functionsChristian Couder, Jul 30, 2016
  37. 34/41 write_or_die: use warning() instead of fprintf(stderr, ...)Christian Couder, Jul 30, 2016
  38. 38/41 usage: add get_error_routine() and get_warn_routine()Christian Couder, Jul 30, 2016
  39. 35/41 apply: make it possible to silently applyChristian Couder, Jul 30, 2016
  40. Junio C HamanoAug 1, 2016
  41. 41/41 builtin/am: use apply api in run_apply()Christian Couder, Jul 30, 2016
  42. 32/41 apply: use error_errno() where possibleChristian Couder, Jul 30, 2016
  43. 40/41 apply: refactor `git apply` option parsingChristian Couder, Jul 30, 2016
  44. 15/41 builtin/apply: make parse_traditional_patch() return -1 on errorChristian Couder, Jul 30, 2016
  45. 14/41 builtin/apply: make apply_all_patches() return 128 or 1 on errorChristian Couder, Jul 30, 2016
  46. 10/41 builtin/apply: move init_apply_state() to apply.cChristian Couder, Jul 30, 2016
  47. 12/41 builtin/apply: make check_apply_state() return -1 instead of die()ingChristian Couder, Jul 30, 2016
  48. 13/41 builtin/apply: move check_apply_state() to apply.cChristian Couder, Jul 30, 2016
  49. 11/41 apply: make init_apply_state() return -1 instead of exit()ingChristian Couder, Jul 30, 2016
  50. 08/41 builtin/apply: make parse_whitespace_option() return -1 instead of die()ingChristian Couder, Jul 30, 2016
  51. 09/41 builtin/apply: make parse_ignorewhitespace_option() return -1 instead of die()ingChristian Couder, Jul 30, 2016
  52. Christian CouderJul 30, 2016
  53. Junio C HamanoAug 1, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.