| Message ID | 20240921150855.31574-1-RuffaloLavoisier@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | am: fix condition check on fseek | expand |
On Sat, Sep 21, 2024, at 17:08, Ruffalo Lavoisier wrote: > if fseek() is success, return value is 0 > > Signed-off-by: Ruffalo Lavoisier <RuffaloLavoisier@gmail.com> > --- > builtin/am.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/am.c b/builtin/am.c > index d8875ad402..a7727fd4ea 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -589,7 +589,7 @@ static int is_mail(FILE *fp) > regex_t regex; > int ret = 1; > > - if (fseek(fp, 0L, SEEK_SET)) > + if (!fseek(fp, 0L, SEEK_SET)) > die_errno(_("fseek failed")); > > if (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED)) > -- > 2.46.1 I don’t get this change? The function returns false on success. true if it fails (not zero). You want the program to die if it returns non-zero. It’s hard to wrap my head around… “false must mean “no errors” ” If the original code has a bug then I don’t see how git-am(1) could work considering it presumably always checks ‘is_mail’.
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes: > On Sat, Sep 21, 2024, at 17:08, Ruffalo Lavoisier wrote: >> if fseek() is success, return value is 0 >> >> Signed-off-by: Ruffalo Lavoisier <RuffaloLavoisier@gmail.com> >> --- >> builtin/am.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/builtin/am.c b/builtin/am.c >> index d8875ad402..a7727fd4ea 100644 >> --- a/builtin/am.c >> +++ b/builtin/am.c >> @@ -589,7 +589,7 @@ static int is_mail(FILE *fp) >> regex_t regex; >> int ret = 1; >> >> - if (fseek(fp, 0L, SEEK_SET)) >> + if (!fseek(fp, 0L, SEEK_SET)) >> die_errno(_("fseek failed")); >> >> if (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED)) >> -- >> 2.46.1 > > I don’t get this change? The function returns false on success. true if > it fails (not zero). You want the program to die if it returns non-zero. > > It’s hard to wrap my head around… “false must mean “no errors” ” > > If the original code has a bug then I don’t see how git-am(1) could work > considering it presumably always checks ‘is_mail’. Yeah, the proposed log message states a correct fact, but it is unclear how that justifies the change in the patch part of the message.
diff --git a/builtin/am.c b/builtin/am.c index d8875ad402..a7727fd4ea 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -589,7 +589,7 @@ static int is_mail(FILE *fp) regex_t regex; int ret = 1; - if (fseek(fp, 0L, SEEK_SET)) + if (!fseek(fp, 0L, SEEK_SET)) die_errno(_("fseek failed")); if (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED))
if fseek() is success, return value is 0 Signed-off-by: Ruffalo Lavoisier <RuffaloLavoisier@gmail.com> --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)