From: Michael Montalbo Date: Fri, 29 May 2026 00:51:47 GMT Subject: Re: [PATCH v2 3/4] diff: add long-running diff process via diff..process Message-ID: In-Reply-To: On Mon, May 25, 2026 at 6:56 PM Junio C Hamano wrote: > > "Michael Montalbo via GitGitGadget" writes: > > > +struct diff_subprocess { > > + struct subprocess_entry subprocess; > > + unsigned int supported_capabilities; > > +}; > > + > > +static int subprocess_map_initialized; > > +static struct hashmap subprocess_map; > > Can we avoid introducing new global variables like these? Would > "struct userdiff_driver" or "struct diff_options" be a good place to > hang this hashmap, perhaps? > Will clean this up. > > +static int send_file_content(int fd, const char *buf, long size) > > +{ > > + int ret; > > + > > + if (size > 0) > > + ret = write_packetized_from_buf_no_flush(buf, size, fd); > > + else > > + ret = 0; > > Shouldn't "size == -24" be flagged as an invalid input? > Will fix and do a broader audit of input validation and bounds checking. > > + if (ret) > > + return ret; > > + return packet_flush_gently(fd); > > +} > > > +static int parse_hunk_line(const char *line, struct xdl_hunk *hunk) > > +{ > > +... > > +} > > This gives a silent error diagnosis, which is good for a lower level > helper. > > > +int diff_process_get_hunks(struct userdiff_driver *drv, > > + const char *path, > > + const char *old_buf, long old_size, > > + const char *new_buf, long new_size, > > + struct xdl_hunk **hunks_out, > > + size_t *nr_hunks_out) > > +{ > > + struct diff_subprocess *backend; > > + struct child_process *process; > > + int fd_in, fd_out; > > + struct strbuf status = STRBUF_INIT; > > + struct xdl_hunk *hunks = NULL; > > + struct xdl_hunk hunk; > > + size_t nr_hunks = 0, alloc_hunks = 0; > > + int len; > > + char *line; > > + > > + if (!drv || !drv->process) > > + return -1; > > A driver that does not define process is not an error; it is > perfectly normal in the current world order where nobody has such an > external process and even fi this patch lands, external processes > are optional. So here "return -1" does not mean an error, and > silent return is perfectly fine. > > > + backend = find_or_start_process(drv->process); > > + if (!backend) > > + return -1; > > This is probably an error; the user specified drv->process, we > either tried to find or start the process and failed. Isn't it an > event that deserves to be reported in an error message? > > > + if (!(backend->supported_capabilities & CAP_HUNKS)) > > + return -1; > > Backend started, but the "hunks" feature is not supported. Perhaps > in a year or two, this external process protocol may have become so > popular that it gained more capabilities, possibly making get_hunks > obsolete. We may be looking at such an external process that uses > other capabilities but not this one. This is not an error, so > silent return is perfectly fine. > > > + process = subprocess_get_child_process(&backend->subprocess); > > + fd_in = process->in; > > + fd_out = process->out; > > + > > + /* Send request */ > > + if (packet_write_fmt_gently(fd_in, "command=hunks\n") || > > + packet_write_fmt_gently(fd_in, "pathname=%s\n", path) || > > + packet_flush_gently(fd_in)) > > + goto error; > > + > > + /* Send old file content */ > > + if (send_file_content(fd_in, old_buf, old_size)) > > + goto error; > > + > > + /* Send new file content */ > > + if (send_file_content(fd_in, new_buf, new_size)) > > + goto error; > > + > > + /* Read hunks until flush packet */ > > + while ((len = packet_read_line_gently(fd_out, NULL, &line)) >= 0 && > > + line) { > > + if (parse_hunk_line(line, &hunk) < 0) > > + goto error; > > + ALLOC_GROW(hunks, nr_hunks + 1, alloc_hunks); > > + hunks[nr_hunks++] = hunk; > > + } > > + if (len < 0) > > + goto error; > > + > > + /* Read status */ > > + if (subprocess_read_status(fd_out, &status)) > > + goto error; > > + > > + if (strcmp(status.buf, "success")) { > > + if (!strcmp(status.buf, "abort")) > > + backend->supported_capabilities &= ~CAP_HUNKS; > > + goto error; > > + } > > + > > + *hunks_out = hunks; > > + *nr_hunks_out = nr_hunks; > > + strbuf_release(&status); > > + return 0; > > + > > +error: > > All exceptions that lead here look like events that should be > reported to the end-user. > Agreed on all points. I will restructure things so errors are flagged when appropriate (i.e., user specified a process but one was not found / couldn't start and exceptions) and non-errors are treated as they should be. > > + free(hunks); > > + strbuf_release(&status); > > + return -1; > > +} > > > +/* > > + * Query a diff process for hunks describing the changes > > + * between old_buf and new_buf. > > + * > > + * The backend is a long-running subprocess configured via > > + * diff..process. It receives file content via > > + * pkt-line and returns hunks with 1-based line numbers. > > + * > > + * On success, sets *hunks_out and *nr_hunks_out to a newly allocated > > + * array (caller must free) and returns 0. > > + * > > + * On failure, returns -1. The caller should fall back to the > > + * builtin diff algorithm. > > + */ > > I do not agree with this. If it is a failure, the user should fix > the external process (or disable). It shouldn't be hidden behind a > fallback. As I left comments, in this round of implementation, > there are conditions that returns -1 for soemthing that is not an > error (i.e., not configured, or process not supporting the > particular capability) *and* in those cases the caller should fall > back as if nothing happened. But some error cases, the caller > should't hide them. Will address in a follow-up. Thank you for the feedback!