| Message ID | 5f787ddac2d80391feadb8cf6be379fc8e58652f.1742367347.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pretty: minor bugfixing, some refactorings | expand |
On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote: > First we look for "auto,", then we try "always,", then we fall back to Nit: we typically have the body carry enough context so that it makes sense even without reading the commit subject. > the default, which is to do exactly the same thing as we do for "auto,". > The amount of code duplication isn't huge, but still: reading this code > carefully requires spending at least *some* time on making sure the two > blocks of code are indeed identical. > > Rearrange the checks so that we end with the default case, > opportunistically consuming the "auto," which may or may not be there. > > In the "always," case, we don't actually *do* anything, so if we were > into golfing, we'd just write the whole thing as a single > > if (!skip_prefix(begin, "always,", &begin)) { > ... > } > > If we ever learn something new besides "always," and "auto," we'd need > to pull things apart again. Plus we still need somewhere to place the > comment. Let's focus on code de-duplication rather than golfing for now. > > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > pretty.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/pretty.c b/pretty.c > index a4e5fc5c50..6a4264dd01 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1076,13 +1076,11 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ > if (!end) > return 0; > > - if (skip_prefix(begin, "auto,", &begin)) { > - if (!want_color(c->pretty_ctx->color)) > - return end - placeholder + 1; > - } else if (skip_prefix(begin, "always,", &begin)) { > + if (skip_prefix(begin, "always,", &begin)) { > /* nothing to do; we do not respect want_color at all */ > } else { > /* the default is the same as "auto" */ > + skip_prefix(begin, "auto,", &begin); > if (!want_color(c->pretty_ctx->color)) > return end - placeholder + 1; > } Okay, this change should lead to the same results as before indeed. As you mention it does require us to be more careful if we ever were to introduce another option here. But I still think it's fine to simplify the code like this. Patrick
Hi Patrick, Thanks for reviewing! On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote: > > On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote: > > First we look for "auto,", then we try "always,", then we fall back to > > Nit: we typically have the body carry enough context so that it makes > sense even without reading the commit subject. Thanks. I'll add some context: "After spotting "%C", we first ..." Martin
On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote: > First we look for "auto,", then we try "always,", then we fall back to > the default, which is to do exactly the same thing as we do for "auto,". > The amount of code duplication isn't huge, but still: reading this code > carefully requires spending at least *some* time on making sure the two > blocks of code are indeed identical. > > Rearrange the checks so that we end with the default case, > opportunistically consuming the "auto," which may or may not be there. OK. The duplicated lines are not all that long, but I don't mind collapsing the cases, especially with the explanatory comment that's there. > In the "always," case, we don't actually *do* anything, so if we were > into golfing, we'd just write the whole thing as a single > > if (!skip_prefix(begin, "always,", &begin)) { > ... > } > > If we ever learn something new besides "always," and "auto," we'd need > to pull things apart again. Plus we still need somewhere to place the > comment. Let's focus on code de-duplication rather than golfing for now. Yeah, I think what you wrote in the patch is much better than trying to golf further. So looks good to me. -Peff
diff --git a/pretty.c b/pretty.c index a4e5fc5c50..6a4264dd01 100644 --- a/pretty.c +++ b/pretty.c @@ -1076,13 +1076,11 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ if (!end) return 0; - if (skip_prefix(begin, "auto,", &begin)) { - if (!want_color(c->pretty_ctx->color)) - return end - placeholder + 1; - } else if (skip_prefix(begin, "always,", &begin)) { + if (skip_prefix(begin, "always,", &begin)) { /* nothing to do; we do not respect want_color at all */ } else { /* the default is the same as "auto" */ + skip_prefix(begin, "auto,", &begin); if (!want_color(c->pretty_ctx->color)) return end - placeholder + 1; }
First we look for "auto,", then we try "always,", then we fall back to the default, which is to do exactly the same thing as we do for "auto,". The amount of code duplication isn't huge, but still: reading this code carefully requires spending at least *some* time on making sure the two blocks of code are indeed identical. Rearrange the checks so that we end with the default case, opportunistically consuming the "auto," which may or may not be there. In the "always," case, we don't actually *do* anything, so if we were into golfing, we'd just write the whole thing as a single if (!skip_prefix(begin, "always,", &begin)) { ... } If we ever learn something new besides "always," and "auto," we'd need to pull things apart again. Plus we still need somewhere to place the comment. Let's focus on code de-duplication rather than golfing for now. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- pretty.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)