Re: [PATCH v2 0/3] worktree: teach add to accept --reason with --lock
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 10, 2021, 07:15 UTC
- Message-ID
- <CAPig+cQyOgQb1zHA49kOT4+2SmTxUjZpfQ-f1ZxV1AwigCz01A@mail.gmail.com>
- In-Reply-To
- <xmqqv95j79ut.fsf@gitster.g>
On Fri, Jul 9, 2021 at 12:03 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> 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.