Re: [PATCH] fsck: use starts_with() in fsck_commit()
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Nov 1, 2025, 14:59 UTC
- Message-ID
- <CAP8UFD0CqC2tgERkPHuoOPO2ON9NNw_C1R-6UpBXjpgYEM8yxQ@mail.gmail.com>
- In-Reply-To
- <q4heOh8stc94r_P5mX-tucCyqQ1JdGP2dJ9Dot3WTyDboRWsboWMAIoCQIXuyuRvNRmN5AGQYjhmAnjuxgOXNooUDjPAEEvKPc2k_DCDEtY=@proton.me>
Hi,
On Fri, Oct 31, 2025 at 11:01 PM keita <rudykeita@proton.me> wrote:
> From 30136adebaffb97edacae2c58c4ea491e39e3f5b Mon Sep 17 00:00:00 2001From: Songiso Cooper Lyambai <rudykeita@proton.me> > Date: Fri, 31 Oct 2025 23:45:23 +0200 > Subject: [PATCH] fsck: use starts_with() in fsck_commit()
If this is related to Outreachy, it would be better to put "[Outreachy]" at the start of the subject.
> Replace manual buffer checks with starts_with() for safety and clarity. > > This avoids buffer overreads and follows Git's idiomatic style used
It looks like the above sentence is not finished. Maybe s/ used/./ ?
> Signed-off-by: Songiso Cooper Lyambai <rudykeita@proton.me> > --- > fsck.c | 124 +++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 67 insertions(+), 57 deletions(-)
A lot of lines seem to be changed for a patch that wants to use starts_with() in fsck_commit(). Let's see below.
Show 12 quoted lines
> diff --git a/fsck.c b/fsck.c > index 341e100d24..7172c4ff1c 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -921,67 +921,77 @@ static int fsck_ident(const char **ident, > } > > static int fsck_commit(const struct object_id *oid, > - const char *buffer, unsigned long size, > - struct fsck_options *options) > + const char *buffer, unsigned long size, > + struct fsck_options *options)
If a patch does other things than it's main goal, it should be mentioned in the commit message that it's doing those other things "while at it".
Anyway when I look at fsck_commit() in "fsck.c" on 'master', it seems to me that the function is properly indented. So I suspect that your patch changes its indentation for no good reason.
Show 12 quoted lines
> {
> - struct object_id tree_oid, parent_oid;
> - unsigned author_count;
> - int err;
> - const char *buffer_begin = buffer;
> - const char *buffer_end = buffer + size;
> - const char *p;
> + struct object_id tree_oid, parent_oid;
> + unsigned author_count = 0;
> + int err = 0;
> + const char *buffer_end = buffer + size;
> + const char *p;Here also I suspect that the indentation changes are not necessary. They are also making it more difficult to spot actual changes like 'buffer_begin' being removed and 'author_count' and 'err' being initialized to 0.
Sorry I am stopping my review here as there seems to be too many indentation changes that are masking the actual changes.
Please make sure you send patches that don't change the indentation for no good reason.
Thanks.