On Wed, Jul 9, 2025 at 2:03 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Elijah Newren <newren@xxxxxxxxx> writes: > > Really minor nitpick, but it might be clearer to pre-increment space > > here than to increment it above. > > FWIW, I find what Christian wrote easier to follow. We find a " " > in the buffer, point it with a pointer and NUL-terminate the > substring. We know we want to further process bytes that follow, so > the pointer is post-incremented after the NUL-termination. The next > user of that pointer relies on the fact that the previous user > concluded its use with that post-increment. > > If 'space' variable were named more genericly, like '*cp', it would > have been perfect. Perhaps only the first half of the code was > written first, and it looked for a space, so the name was chosen, > but then later ... > > >> + > >> + /* Remove any trailing newline from format */ > >> + space = strchr(sig->sig_format, '\n'); > > ... it is used to point at a LF X-<, at which time the author could > have renamed it to keep readers' sanity ;-) Yeah, right, I couldn't think of a good name to rename it, so I postponed changing the name, and then forgot about it. > >> + if (space) > >> + *space = '\0'; > > I also wonder what should (not "does", as I can see that the code > does not do anything) happen if we do not find the LF we were > looking for. Is the caller of this function so loosely written that > it may or may not guarantee that the data it calls this function > with is properly terminated? This function is passed a pointer to 'command_buf.buf' after "gpgsig " was skipped from it. 'read_next_command()' uses 'strbuf_getline_lf()' to put each line in 'command_buf.buf'. And yeah 'strbuf_getline_lf()' should remove a LF from the end of 'command_buf.buf' if any. So we don't need the code to remove any trailing LF. I have removed it in my current version. This means that we can keep using "space" as the variable name, as it is now only related to the space between the arguments. > > Otherwise, I very much appreciate the work to create a testcase with > > both signature types on a single commit. > > Yup, thanks, both of you. It seems that we are getting closer to > the finish line? Yeah, I plan to send a v6 soon, maybe later today with the changes discussed here and in my reply to Elijah. Thanks.
