Re: [PATCH 4/5] trailer: teach find_patch_start about --no-divider

From: Glen Choo <chooglen@google.com>
Date: Aug 7, 2023, 23:28 UTC
Message-ID: <kl6ly1im8ma5.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <1fc060041db11b3df881cb2c7bd60630dc011a15.1691211879.git.gitgitgadget@gmail.com>

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> 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().

Show 16 quoted lines

> @@ -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)".