On Fri, Jul 9, 2021 at 12:03 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > > The reason I suggested re-purposing `add_opts.keep_locked` is to avoid > > polluting that structure with members having overlapping meaning, thus > > reducing the confusion-factor for clients of that structure (assuming > > that a tri-state `keep_locked` is indeed less confusing). That doesn't > > preclude adding a new variable or two local to the `add()` function to > > facilitate keeping `add_opts` pure. For instance, it might be as > > simple as the below patch. > > True. It is less trivial to construct the command line option > parser so that --reason=<why> and --lock can be given in any order > (e.g. they no longer can be a simple OPT_BOOL and OPT_STRING that > can be given independently but needs some postprocessing like your > patch did), but it is not rocket science and keeping add_opts struct > leaner will reduce the risk of runtime confusion, I would think, but > at the same time, the room for runtime confusion would probably be > minor to begin with---so I am fine, if the coder cannot cleanly > write the option parser to do so, with the code as posted. Although the leaner approach seems "simpler" and more obvious to me, it is subjective, and I can see how other people might find a tri-state add_opts::keep_locked confusing. So, I'm fine with it either way and don't consider it a blocker at all.
