From: Junio C Hamano <gitster@pobox.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: "Michael Haggerty" <mhagger@alum.mit.edu>,
git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v5] gc: save log from daemonized gc --auto and print it next time
Date: Mon, 21 Sep 2015 09:43:15 -0700 [thread overview]
Message-ID: <xmqqtwqnbu58.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1442639603-25568-1-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Sat, 19 Sep 2015 12:13:23 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> While commit 9f673f9 (gc: config option for running --auto in
> background - 2014-02-08) helps reduce some complaints about 'gc
> --auto' hogging the terminal, it creates another set of problems.
>
> The latest in this set is, as the result of daemonizing, stderr is
> closed and all warnings are lost. This warning at the end of cmd_gc()
> is particularly important because it tells the user how to avoid "gc
> --auto" running repeatedly. Because stderr is closed, the user does
> not know, naturally they complain about 'gc --auto' wasting CPU.
>
> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto
> will not run and gc.log printed out until the user removes gc.log.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> This is the lock-based version. I'm sending an alternative that does
> not need atexit() with comparison between the two.
I think Michael deserves to be CC'ed.
I am wondering if we need an audit of what our signal handlers do in
the same light as what led to 507d7804 (pager: don't use unsafe
functions in signal handlers, 2015-09-04). lockfile API already
does removal in remove_lock_files_on_signal(), which may or may not
need further fixing. And that is why I really hate to see us adding
custom signal handler if we can avoid it.
What you are gaining by having a custom signal handler here is
twofold, I think:
* You have a chance to remove an empty log message upon signal, but
if you follow Michael's idea of equating an empty and missing log
file, that would not be a huge plus either way.
* You can keep a non-empty log message that may be incomplete when
you are killed yourself. If you reused the lockfile API and let
its signal handler clean it up, you obviously will lose that, but
you do not even know if that incomplete non-empty log message is
something you want to base the "stop auto-gc until an existing
problem is resolved" decision on. Your subprocess may have left
that message only because you are getting killed, no?
Neither looks like a compelling enough reason to have it to me.
Wouldn't the logic be vastly simplified and be still correct, if you
let the lockfile API do its usual "unless you explicitly commit,
lockfiles are removed upon signal or exit"?
> builtin/gc.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index bcc75d9..4e738fa 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -43,6 +43,7 @@ static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
> static struct argv_array rerere = ARGV_ARRAY_INIT;
>
> static char *pidfile;
> +static struct lock_file log_lock;
>
> static void remove_pidfile(void)
> {
> @@ -57,6 +58,28 @@ static void remove_pidfile_on_signal(int signo)
> raise(signo);
> }
>
> +static void process_log_file(void)
> +{
> + struct stat st;
> + if (!fstat(log_lock.fd, &st) && st.st_size)
> + commit_lock_file(&log_lock);
> + else
> + rollback_lock_file(&log_lock);
> +}
> +
> +static void process_log_file_at_exit(void)
> +{
> + fflush(stderr);
> + process_log_file();
> +}
> +
> +static void process_log_file_on_signal(int signo)
> +{
> + process_log_file();
> + sigchain_pop(signo);
> + raise(signo);
> +}
> +
> static void git_config_date_string(const char *key, const char **output)
> {
> if (git_config_get_string_const(key, output))
> @@ -253,6 +276,24 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> return NULL;
> }
>
> +static int report_last_gc_error(void)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
> +
> + ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
> + if (ret > 0)
> + return error(_("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"),
> + git_path("gc.log"), sb.buf);
> + strbuf_release(&sb);
> + return 0;
> +}
> +
> static int gc_before_repack(void)
> {
> if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> @@ -274,6 +315,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> int force = 0;
> const char *name;
> pid_t pid;
> + int daemonized = 0;
>
> struct option builtin_gc_options[] = {
> OPT__QUIET(&quiet, N_("suppress progress reporting")),
> @@ -330,13 +372,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
> }
> if (detach_auto) {
> + if (report_last_gc_error())
> + return -1;
> +
> if (gc_before_repack())
> return -1;
> /*
> * failure to daemonize is ok, we'll continue
> * in foreground
> */
> - daemonize();
> + daemonized = !daemonize();
> }
> } else
> add_repack_all_option();
> @@ -349,6 +394,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> name, (uintmax_t)pid);
> }
>
> + if (daemonized) {
> + hold_lock_file_for_update(&log_lock,
> + git_path("gc.log"),
> + LOCK_DIE_ON_ERROR);
> + dup2(log_lock.fd, 2);
> + sigchain_push_common(process_log_file_on_signal);
> + atexit(process_log_file_at_exit);
> + }
> +
> if (gc_before_repack())
> return -1;
next prev parent reply other threads:[~2015-09-21 16:43 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-08 12:28 Since gc.autodetach=1 you can end up with auto-gc on every command with no user notification Ævar Arnfjörð Bjarmason
2015-07-08 12:47 ` Duy Nguyen
2015-08-22 2:12 ` [PATCH v3] gc: save log from daemonized gc --auto and print it next time Nguyễn Thái Ngọc Duy
2015-08-25 17:49 ` Junio C Hamano
2015-08-31 10:17 ` Duy Nguyen
2015-09-13 1:36 ` [PATCH v4] " Nguyễn Thái Ngọc Duy
2015-09-14 17:24 ` Junio C Hamano
2015-09-14 17:37 ` Junio C Hamano
2015-09-16 9:28 ` Michael Haggerty
2015-09-16 16:00 ` Junio C Hamano
2015-09-17 9:40 ` Michael Haggerty
2015-09-17 13:08 ` Duy Nguyen
2015-09-17 14:48 ` Junio C Hamano
2015-09-19 5:13 ` [PATCH v5] " Nguyễn Thái Ngọc Duy
2015-09-21 16:43 ` Junio C Hamano [this message]
2015-09-19 5:14 ` [Alt. PATCH " Nguyễn Thái Ngọc Duy
2015-09-21 16:19 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqtwqnbu58.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).