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

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

Previous: Junio C HamanoNext: Miriam Rubio
Message 15 of 17 in “Finish converting git bisect to C part 4”
  1. 0/6 Finish converting git bisect to C part 4Miriam Rubio, Sep 2, 2021
  2. 1/6 t6030-bisect-porcelain: add tests to control bisect run exit casesMiriam Rubio, Sep 2, 2021
  3. Junio C HamanoSep 2, 2021
  4. 2/6 t6030-bisect-porcelain: add test for bisect visualizeMiriam Rubio, Sep 2, 2021
  5. Junio C HamanoSep 2, 2021
  6. 3/6 run-command: make `exists_in_PATH()` non-staticMiriam Rubio, Sep 2, 2021
  7. Junio C HamanoSep 2, 2021
  8. 4/6 bisect--helper: reimplement `bisect_visualize()`shell function in CMiriam Rubio, Sep 2, 2021
  9. Junio C HamanoSep 2, 2021
  10. 5/6 bisect--helper: reimplement `bisect_run` shellMiriam Rubio, Sep 2, 2021
  11. Junio C HamanoSep 2, 2021
  12. Johannes SchindelinSep 6, 2021
  13. Miriam R.Sep 6, 2021
  14. Junio C HamanoSep 7, 2021
  15. Johannes SchindelinSep 9, 2021
  16. 6/6 bisect--helper: retire `--bisect-next-check` subcommandMiriam Rubio, Sep 2, 2021
  17. Junio C HamanoSep 2, 2021

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.