Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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;
}
