"Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> This patch will make unit testing a bit more pleasant in this area in
> the future when we adopt a unit testing framework, because we would not
> have to test multiple functions to check how finding the start of a
> patch part works (we would only need to test find_patch_start).
Unit tests typically only test external-facing interfaces, not
implementatino details, so without seeing the unit tests or library
boundary, it's hard to tell whether find_patch_start() is something we
want to unit test or not. I would have assumed it's not, given that it's
tiny and only has a single caller, so I'm hesitant to say that we should
definitely handle no_divider inside find_patch_start().
> @@ -812,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len)
> * Return the position of the start of the patch or the length of str if there
> * is no patch in the message.
> */
> -static size_t find_patch_start(const char *str)
> +static size_t find_patch_start(const char *str, int no_divider)
> {
> const char *s;
>
> for (s = str; *s; s = next_line(s)) {
> const char *v;
>
> - if (skip_prefix(s, "---", &v) && isspace(*v))
> + if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v))
> return s - str;
> }
Assuming we wanted to make this unit-testable anyway, could we just move
the strlen() call into this function? Performance aside (I wouldn't be
surprised if a smart enough compiler could optimize away the noops), I
don't find this easier to understand. Now the reader needs to read the
code to see "if no_divider is given, noop until the end of the string,
at which point str will point to the end, and s - str will give us the
length of str", as opposed to "there are no dividers, so just return
strlen(str)".
