Subject: Re: [PATCH v4] Handle atexit list internaly for unthreaded builds

From: Junio C Hamano <[email-masked]>
Date: Mon, 13 Oct 2014 13:00:16 -0700

Etienne Buira <[email-masked]> writes:

> Wrap atexit()s calls on unthreaded builds to handle callback list
> internally.
>
> This is needed because on unthreaded builds, asyncs inherits parent's
> atexit() list, that gets run as soon as the async exit()s (and again at
> the end of the parent process). That led to remove temporary and lock
> files too early.

... that does not explain what you did to builtin/clone.c at all.
Care to enlighten us, please?

>
> Fixes test 5537 (temporary shallow file vanished before unpack-objects
> could open it)
>
> V4: fix bogus preprocessor directives

Please do not write this "V4:" line in the log message.  People who
read "git log" output and find this description would not know
anything about the previous faulty ones.  Putting it _after_ the
three-dash line below will help the reviewers a lot.

>
> Thanks-to: Duy Nguyen <[email-masked]>
> Thanks-to: Andreas Schwab <[email-masked]>

Usually we spell these "Helped-by: " instead.

> Signed-off-by: Etienne Buira <[email-masked]>
> ---

Thanks.

>  builtin/clone.c   |  5 -----
>  git-compat-util.h |  5 +++++
>  run-command.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>  shallow.c         |  7 ++-----
>  4 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index bbd169c..e122f33 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -390,7 +390,6 @@ static void clone_local(const char *src_repo, const char *dest_repo)
>  
>  static const char *junk_work_tree;
>  static const char *junk_git_dir;
> -static pid_t junk_pid;
>  static enum {
>  	JUNK_LEAVE_NONE,
>  	JUNK_LEAVE_REPO,
> @@ -417,8 +416,6 @@ static void remove_junk(void)
>  		break;
>  	}
>  
> -	if (getpid() != junk_pid)
> -		return;
>  	if (junk_git_dir) {
>  		strbuf_addstr(&sb, junk_git_dir);
>  		remove_dir_recursively(&sb, 0);
> @@ -758,8 +755,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	struct refspec *refspec;
>  	const char *fetch_pattern;
>  
> -	junk_pid = getpid();
> -
>  	packet_trace_identity("clone");
>  	argc = parse_options(argc, argv, prefix, builtin_clone_options,
>  			     builtin_clone_usage, 0);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f587749..6dd63dd 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -577,6 +577,11 @@ int inet_pton(int af, const char *src, void *dst);
>  const char *inet_ntop(int af, const void *src, char *dst, size_t size);
>  #endif
>  
> +#ifdef NO_PTHREADS
> +#define atexit git_atexit
> +extern int git_atexit(void (*handler)(void));
> +#endif
> +
>  extern void release_pack_memory(size_t);
>  
>  typedef void (*try_to_free_t)(size_t);
> diff --git a/run-command.c b/run-command.c
> index 35a3ebf..0f9a9b0 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -624,6 +624,45 @@ static int async_die_is_recursing(void)
>  	return ret != NULL;
>  }
>  
> +#else
> +
> +static struct {
> +	void (**handlers)(void);
> +	size_t nr;
> +	size_t alloc;
> +} git_atexit_hdlrs;
> +
> +static int git_atexit_installed;
> +
> +static void git_atexit_dispatch()
> +{
> +	size_t i;
> +
> +	for (i=git_atexit_hdlrs.nr ; i ; i--)
> +		git_atexit_hdlrs.handlers[i-1]();
> +}
> +
> +static void git_atexit_clear()
> +{
> +	free(git_atexit_hdlrs.handlers);
> +	memset(&git_atexit_hdlrs, 0, sizeof(git_atexit_hdlrs));
> +	git_atexit_installed = 0;
> +}
> +
> +#undef atexit
> +int git_atexit(void (*handler)(void))
> +{
> +	ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
> +	git_atexit_hdlrs.handlers[git_atexit_hdlrs.nr++] = handler;
> +	if (!git_atexit_installed) {
> +		if (atexit(&git_atexit_dispatch))
> +			return -1;
> +		git_atexit_installed = 1;
> +	}
> +	return 0;
> +}
> +#define atexit git_atexit
> +
>  #endif
>  
>  int start_async(struct async *async)
> @@ -682,6 +721,7 @@ int start_async(struct async *async)
>  			close(fdin[1]);
>  		if (need_out)
>  			close(fdout[0]);
> +		git_atexit_clear();
>  		exit(!!async->proc(proc_in, proc_out, async->data));
>  	}
>  
> diff --git a/shallow.c b/shallow.c
> index de07709..f067811 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -226,7 +226,6 @@ static void remove_temporary_shallow_on_signal(int signo)
>  
>  const char *setup_temporary_shallow(const struct sha1_array *extra)
>  {
> -	static int installed_handler;
>  	struct strbuf sb = STRBUF_INIT;
>  	int fd;
>  
> @@ -237,10 +236,8 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
>  		strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
>  		fd = xmkstemp(temporary_shallow.buf);
>  
> -		if (!installed_handler) {
> -			atexit(remove_temporary_shallow);
> -			sigchain_push_common(remove_temporary_shallow_on_signal);
> -		}
> +		atexit(remove_temporary_shallow);
> +		sigchain_push_common(remove_temporary_shallow_on_signal);
>  
>  		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
>  			die_errno("failed to write to %s",