Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)

From: René Scharfe <l.s.r@web.de>
Date: Oct 8, 2023, 17:34 UTC
Message-ID: <4014e490-c6c1-453d-b0ed-645220e3e614@web.de>
In-Reply-To: <xmqqh6n24zf1.fsf@gitster.g>

Am 07.10.23 um 10:20 schrieb Junio C Hamano:

Show 9 quoted lines

> * rs/parse-options-value-int (2023-09-18) 2 commits
>  - parse-options: use and require int pointer for OPT_CMDMODE
>  - parse-options: add int value pointer to struct option
>
>  A bit of type safety for the "value" pointer used in the
>  parse-options API.
>
>  What's the status of this thing?
>  source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>

tl;dr: Feel free to drop this topic if it's in the way.

I think the added _int variables are defensible, but the mode_int member added to struct resume_mode in builtin/am.c is too ugly. Extending OPT_CMDMODE to support arguments would allow getting rid of the struct altogether and is probably a good idea anyway. It's surprisingly hard to get right, though -- just discovered that the interference detection is only working half of the time in the current code, and we can't have that, can we?

   # a OPT_CMDMODE option
   $ t/helper/test-tool parse-options --mode1 | grep integer
   integer: 1

   # something else setting the same variable
   $ % t/helper/test-tool parse-options --set23 | grep integer
   integer: 23

   # combined use is flagged if the OPT_CMDMODE option comes last
   $ t/helper/test-tool parse-options --set23 --mode1
   error: option `mode1' : incompatible with something else

   # ... but not the other way around
   $ t/helper/test-tool parse-options --mode1 --set23 | grep integer
   integer: 23

Anyway, I'll get there eventually and present a nicely shaven yak -- or give up and focus on the original topic.

https://lore.kernel.org/git/6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de/ detects and handles the same type mismatch by adding a level of abstraction (getters and setters). That way is more general and adds more overhead compared to the _int variant. A fair comparison requires argument support in OPT_CMDMODE, though, I think.

I don't mind removing this topic from seen for now; it's not ready, yet.

René