Re: [PATCH v6 5/6] bisect--helper: reimplement `bisect_run` shell
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 9, 2021, 07:51 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2109090922310.55@tvgsbejvaqbjf.bet>
- In-Reply-To
- <xmqqlf48b5io.fsf@gitster.g>
Hi Junio,
On Tue, 7 Sep 2021, Junio C Hamano wrote:
Show 24 quoted lines
> "Miriam R." <mirucam@gmail.com> writes: > > >> However, I still don't like that we play such a `dup2()` game. I gave it a > >> quick try to avoid it (see the diff below, which corresponds to the commit > >> I pushed up as `git-bisect-work-part4-v7` to > >> https://github.com/dscho/git), which still could benefit from a bit of > >> polishing (maybe we should rethink the object model and extend/rename > >> `bisect_terms` to `bisect_state` and accumulate more fields, such as > >> `out_fd`. > >> > >> Obviously this will need to be cleaned up, and while I would _love_ to see > >> this make it into your next iteration, ultimately it is up to you, Miriam, > >> to decide whether you want to build on my diff (quite possibly making the > >> entire object model of the bisect part of Git's code more elegant and more > >> maintainable), and up to you, Junio, to decide whether you would be > >> willing to accept the patch series without this refactoring. > > If the code paths involved are shallow and narrow enough that not > too many existing callers need to start passing FILE *stdout down > (from the looks of your illustration patch, it does not seem to be > too bad), I do not mind a series that is a bit longer than the > current 6-patch series that has a preliminary enhancement step that > allows callers to pass their own "FILE *" for output destination > before the main part of the topic.
My impression, from the diff that I sent, is that this is too deep and wide, and indeed needs a follow-up patch series as indicated by Miriam. My preference would be (as I hinted at) to accumulate relevant data (such as the terms and, yes, the `FILE *`) into a `struct bisect_state` and pass that around. Sort of a light-weight object-oriented design, similar to how we do things in `builtin/am.c` with `struct am_state`.
Thanks, Dscho