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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> +		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.

> @@ -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.

> 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.