| Message ID | patch-3.4-8747afecdcd-20211206T165221Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usage API: Add and use die_message() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change code the "error: " output when we exit with 128 due to gc.log > errors to use a "fatal: " prefix instead. This adds a sibling function > to the die_errno() added in a preceding commit. > > Since it returns 128 instead of -1 (like die_message()) we'll need to > adjust report_last_gc_error(). > > Let's adjust it while we're at it to not conflate the "should skip" > and "exit with this non-zero code" conditions, as the caller is no > longer hardcoding "128", but relying on die_errno() to return a > nen-zero exit() status. > > Since we're touching this code let's also use "return ret" in place of > an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in > "git.c" might want to do. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/gc.c | 21 ++++++++++++--------- > git-compat-util.h | 1 + > usage.c | 12 ++++++++++++ > 3 files changed, 25 insertions(+), 9 deletions(-) The structure of the series makes sense here. 1 and 2 were structured to add new set of API functions in 1 and to make use of them in 2 (which is also OK if done right; as pointed out, a few changes need to be moved from 2 to 1 to make 1 complete). This makes further addition and adds its uses at the same time. > diff --git a/builtin/gc.c b/builtin/gc.c > index bcef6a4c8da..7b451dfeefc 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) > * gc should not proceed due to an error in the last run. Prints a > * message and returns -1 if an error occurred while reading gc.log > */ > -static int report_last_gc_error(void) > +static int report_last_gc_error(int *skip) > { > struct strbuf sb = STRBUF_INIT; > int ret = 0; > ssize_t len; > struct stat st; > char *gc_log_path = git_pathdup("gc.log"); > + *skip = 0; > > if (stat(gc_log_path, &st)) { Insert a new statement after the blank that separates the declaration from the statements. > if (errno == ENOENT) > goto done; > > - ret = error_errno(_("cannot stat '%s'"), gc_log_path); > + ret = die_message_errno(_("cannot stat '%s'"), gc_log_path); > goto done; > } > > @@ -493,7 +494,7 @@ static int report_last_gc_error(void) > > len = strbuf_read_file(&sb, gc_log_path, 0); > if (len < 0) > - ret = error_errno(_("cannot read '%s'"), gc_log_path); > + ret = die_message_errno(_("cannot read '%s'"), gc_log_path); > else if (len > 0) { > /* > * A previous gc failed. Report the error, and don't > @@ -507,7 +508,7 @@ static int report_last_gc_error(void) > "until the file is removed.\n\n" > "%s"), > gc_log_path, sb.buf); > - ret = 1; > + *skip = 1; > } > strbuf_release(&sb); > done: Nobody called die() in here, so we always returned to the caller. It feels a bit strange that we need, in addition to the existing 'ret' that becomes the return value, an extra out parameter *skip. Let's see why the caller needs both. > @@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix) > fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n")); > } > if (detach_auto) { > - int ret = report_last_gc_error(); > - if (ret < 0) > - /* an I/O error occurred, already reported */ > - exit(128); > - if (ret == 1) > + int skip; > + int ret = report_last_gc_error(&skip); > + > + if (skip) > /* Last gc --auto failed. Skip this one. */ > return 0; > + if (ret) > + /* an I/O error occurred, already reported */ > + return ret; We used to use the fact that - negative return was from error_errno() to cuase exit(128) to emulate die. - return with 1 is from the "previous gc failed" block and declare we can safely return 0 (success). - return with 0 is sucess and nothing needed to be, and was, done. So, I still do not see the point of this change to add an extra error channel. I do not find this argument > ... as the caller is no > longer hardcoding "128", but relying on die_errno() to return a > nen-zero exit() status. particularly a convincing one. We are not obliged to assign the value returned from die_message_errno() to 'ret' to begin with. report_last_gc_error() can document what its return values mean and the caller can react to it. Indeed, if we assign -1 to 'ret' where we used to call error_errno(), the caller needs no changes at all, no? Something like this as a replacement for this part? builtin/gc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git c/builtin/gc.c w/builtin/gc.c index f7270b26ea..2b54465608 100644 --- c/builtin/gc.c +++ w/builtin/gc.c @@ -477,47 +477,49 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) /* * Returns 0 if there was no previous error and gc can proceed, 1 if * gc should not proceed due to an error in the last run. Prints a * message and returns -1 if an error occurred while reading gc.log */ static int report_last_gc_error(void) { struct strbuf sb = STRBUF_INIT; - int ret = 0; + int ret = -1; /* a pessimist */ ssize_t len; struct stat st; char *gc_log_path = git_pathdup("gc.log"); if (stat(gc_log_path, &st)) { if (errno == ENOENT) goto done; - ret = error_errno(_("cannot stat '%s'"), gc_log_path); + die_message_errno(_("cannot stat '%s'"), gc_log_path); goto done; } - if (st.st_mtime < gc_log_expire_time) + if (st.st_mtime < gc_log_expire_time) { + ret = 0; goto done; + } len = strbuf_read_file(&sb, gc_log_path, 0); if (len < 0) - ret = error_errno(_("cannot read '%s'"), gc_log_path); + die_message_errno(_("cannot read '%s'"), gc_log_path); else if (len > 0) { /* * A previous gc failed. Report the error, and don't * bother with an automatic gc run since it is likely * to fail in the same way. */ warning(_("The last gc run reported the following. " "Please correct the root cause\n" "and remove %s\n" "Automatic cleanup will not be performed " "until the file is removed.\n\n" "%s"), gc_log_path, sb.buf); - ret = 1; + ret = 0; } strbuf_release(&sb); done: free(gc_log_path); return ret; }
diff --git a/builtin/gc.c b/builtin/gc.c index bcef6a4c8da..7b451dfeefc 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) * gc should not proceed due to an error in the last run. Prints a * message and returns -1 if an error occurred while reading gc.log */ -static int report_last_gc_error(void) +static int report_last_gc_error(int *skip) { struct strbuf sb = STRBUF_INIT; int ret = 0; ssize_t len; struct stat st; char *gc_log_path = git_pathdup("gc.log"); + *skip = 0; if (stat(gc_log_path, &st)) { if (errno == ENOENT) goto done; - ret = error_errno(_("cannot stat '%s'"), gc_log_path); + ret = die_message_errno(_("cannot stat '%s'"), gc_log_path); goto done; } @@ -493,7 +494,7 @@ static int report_last_gc_error(void) len = strbuf_read_file(&sb, gc_log_path, 0); if (len < 0) - ret = error_errno(_("cannot read '%s'"), gc_log_path); + ret = die_message_errno(_("cannot read '%s'"), gc_log_path); else if (len > 0) { /* * A previous gc failed. Report the error, and don't @@ -507,7 +508,7 @@ static int report_last_gc_error(void) "until the file is removed.\n\n" "%s"), gc_log_path, sb.buf); - ret = 1; + *skip = 1; } strbuf_release(&sb); done: @@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix) fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n")); } if (detach_auto) { - int ret = report_last_gc_error(); - if (ret < 0) - /* an I/O error occurred, already reported */ - exit(128); - if (ret == 1) + int skip; + int ret = report_last_gc_error(&skip); + + if (skip) /* Last gc --auto failed. Skip this one. */ return 0; + if (ret) + /* an I/O error occurred, already reported */ + return ret; if (lock_repo_for_gc(force, &pid)) return 0; diff --git a/git-compat-util.h b/git-compat-util.h index d5e495529c8..c6c6f7d6b51 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -480,6 +480,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)) NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2))); NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); int die_message(const char *err, ...) __attribute__((format (printf, 1, 2))); +int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); int error(const char *err, ...) __attribute__((format (printf, 1, 2))); int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); void warning(const char *err, ...) __attribute__((format (printf, 1, 2))); diff --git a/usage.c b/usage.c index 76399ba8409..3d09e8eea48 100644 --- a/usage.c +++ b/usage.c @@ -233,6 +233,18 @@ int die_message(const char *err, ...) return 128; } +#undef die_message_errno +int die_message_errno(const char *fmt, ...) +{ + char buf[1024]; + va_list params; + + va_start(params, fmt); + die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params); + va_end(params); + return 128; +} + #undef error_errno int error_errno(const char *fmt, ...) {
Change code the "error: " output when we exit with 128 due to gc.log errors to use a "fatal: " prefix instead. This adds a sibling function to the die_errno() added in a preceding commit. Since it returns 128 instead of -1 (like die_message()) we'll need to adjust report_last_gc_error(). Let's adjust it while we're at it to not conflate the "should skip" and "exit with this non-zero code" conditions, as the caller is no longer hardcoding "128", but relying on die_errno() to return a nen-zero exit() status. Since we're touching this code let's also use "return ret" in place of an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in "git.c" might want to do. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/gc.c | 21 ++++++++++++--------- git-compat-util.h | 1 + usage.c | 12 ++++++++++++ 3 files changed, 25 insertions(+), 9 deletions(-)