Re: [PATCH v2 1/2] tmp-objdir: new API for creating temporary writable databases

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2021, 07:43 UTC
Message-ID: <xmqq4k7mi3g4.fsf@gitster.g>
In-Reply-To: <36c00613d9a6ad4fc768e15b9ec23f9af520338a.1638750965.git.gitgitgadget@gmail.com>

"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

> diff --git a/builtin/prune.c b/builtin/prune.c
> index 485c9a3c56f..c2bcdc07db4 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -26,10 +26,22 @@ static int prune_tmp_file(const char *fullpath)
>  		return error("Could not stat '%s'", fullpath);
>  	if (st.st_mtime > expire)
>  		return 0;
> -	if (show_only || verbose)
> -		printf("Removing stale temporary file %s\n", fullpath);
> -	if (!show_only)
> -		unlink_or_warn(fullpath);
> +	if (S_ISDIR(st.st_mode)) {

Because the updated tmp_objdir_create() always uses "tmp_objdir-" as the common prefix (instead of "incoming-" that we used to use, prune_cruft() will call this function not just for temporary files for loose objects, but also for directories. So a new code to do an equivalent of "rm -fr" is added here. OK.

Show 53 quoted lines

> +		if (show_only || verbose)
> +			printf("Removing stale temporary directory %s\n", fullpath);
> +		if (!show_only) {
> +			struct strbuf remove_dir_buf = STRBUF_INIT;
> +
> +			strbuf_addstr(&remove_dir_buf, fullpath);
> +			remove_dir_recursively(&remove_dir_buf, 0);
> +			strbuf_release(&remove_dir_buf);
> +		}
> +	} else {
> +		if (show_only || verbose)
> +			printf("Removing stale temporary file %s\n", fullpath);
> +		if (!show_only)
> +			unlink_or_warn(fullpath);
> +	}
>  	return 0;
>  }
>  
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 49b846d9605..8815e24cde5 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2213,7 +2213,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>  		strvec_push(&child.args, alt_shallow_file);
>  	}
>  
> -	tmp_objdir = tmp_objdir_create();
> +	tmp_objdir = tmp_objdir_create("incoming");
>  	if (!tmp_objdir) {
>  		if (err_fd > 0)
>  			close(err_fd);
> diff --git a/environment.c b/environment.c
> index 9da7f3c1a19..342400fcaad 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -17,6 +17,7 @@
>  #include "commit.h"
>  #include "strvec.h"
>  #include "object-store.h"
> +#include "tmp-objdir.h"
>  #include "chdir-notify.h"
>  #include "shallow.h"
>  
> @@ -331,10 +332,14 @@ static void update_relative_gitdir(const char *name,
>  				   void *data)
>  {
>  	char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());
> +	struct tmp_objdir *tmp_objdir = tmp_objdir_unapply_primary_odb();
>  	trace_printf_key(&trace_setup_key,
>  			 "setup: move $GIT_DIR to '%s'",
>  			 path);
> +
>  	set_git_dir_1(path);

If a blank line needs to be added, have it between the variable declarations and the first statement (i.e. before the above call to "trace_printf_key()").

> +	if (tmp_objdir)
> +		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
>  	free(path);
>  }

This is called during set_git_dir(), which happens fairly early in the set-up sequence. I wonder if there is a real use case that creates a tmp-objdir that early in the process to require this unapply-reapply sequence.

Show 9 quoted lines

> @@ -1809,8 +1846,11 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,
>  /* Finalize a file on disk, and close it. */
>  static void close_loose_object(int fd)
>  {
> -	if (fsync_object_files)
> -		fsync_or_die(fd, "loose object file");
> +	if (!the_repository->objects->odb->will_destroy) {
> +		if (fsync_object_files)
> +			fsync_or_die(fd, "loose object file");

OK, so we omit fsync because these newly created loose objects may not survive and instead get discarded. Presumably when we migrate them to the real object store, we'll make sure they hit the disk platter in some other way?

	... goes and cheats by reading ahead ...

Ahh, ok, new objects created in a temporary object store that is marked with the will_destroy bit is not allowed to migrate to the real object store, so there is no point to fsync them.

set_temporary_primary_odb() and tmp_objdir_replace_primary_odb() can mark the temporary one to be throw-away, but unfortunately there is no caller in this step, so it is a bit hard to see when a throw-away object store is useful. I guess remerge-diff wants to do tentative merges that create new objects in a throw-away object directory, because it is logically a read-only operation.

Show 17 quoted lines

> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index b8d880e3626..3d38eeab66b 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -1,5 +1,6 @@
>  #include "cache.h"
>  #include "tmp-objdir.h"
> +#include "chdir-notify.h"
>  #include "dir.h"
>  #include "sigchain.h"
>  #include "string-list.h"
> @@ -11,6 +12,8 @@
>  struct tmp_objdir {
>  	struct strbuf path;
>  	struct strvec env;
> +	struct object_directory *prev_odb;
> +	int will_destroy;

The other one was a one-bit unsigned bitfield, but this is a full integer. I somehow think that the other one can and should be a full integer, too---it's not like there are tons of bits need to be stored in the structure or we will have tons of instances of the structure that storing many bits compactly matters.

Thanks.