| Message ID | 1fc060041db11b3df881cb2c7bd60630dc011a15.1691211879.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ee8c5ee08cee9aee6732ab315e94bc88631c7431 |
| Headers | show |
| Series | Trailer readability cleanups | expand |
"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(). > @@ -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)".
Glen Choo <chooglen@google.com> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: >> @@ -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? I don't see why we should preserve the if-statement and associated strlen() call if we can just get rid of it. > [...] 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)". The main idea behind this patch is to make find_patch_start() return the correct answer. Currently it does not in all cases (whether --no-divider is provided), and so the caller has to calculate the start of the patch with strlen manually. By moving the --no-divider flag into this function, we force all callers to consider this important option. For additional context we recently had to fix a bug where we weren't passing in this flag to the interpret-trailers builtin. See be3d654343 (commit: pass --no-divider to interpret-trailers, 2023-06-17). There we acknowledged that some callers forgot to pass in --no-divider to interpret-trailers (such as the caller that was fixed up in that commit). I mention the above example because although it's not the exact same thing as here, I think the scenario of "sometimes callers can forget about --no-divider" is an important one to prevent wherever possible. That's why I like this patch (in addition to the reasons cited in the commit message).
Linus Arver <linusa@google.com> writes: > I don't see why we should preserve the if-statement and associated > strlen() call if we can just get rid of it. Here are some reasons: - Without compiler optimizations, it is faster. - Subjectively, I find the early return easier to understand. I don't think I need to nitpick over such a tiny issue though, so I'm okay either way.
diff --git a/trailer.c b/trailer.c index 89246a0d395..3b9ce199636 100644 --- a/trailer.c +++ b/trailer.c @@ -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; } @@ -1109,11 +1109,7 @@ void trailer_info_get(struct trailer_info *info, const char *str, ensure_configured(); - if (opts->no_divider) - patch_start = strlen(str); - else - patch_start = find_patch_start(str); - + patch_start = find_patch_start(str, opts->no_divider); trailer_end = find_trailer_end(str, patch_start); trailer_start = find_trailer_start(str, trailer_end);