ns/tmp-objdir: add support for temporary writable databases

---
 builtin/prune.c        | 20 ++++++++++++---
 builtin/receive-pack.c |  2 +-
 environment.c          |  5 ++++
 object-file.c          | 44 +++++++++++++++++++++++++++++++--
 object-store.h         | 19 +++++++++++++++
 object.c               |  2 +-
 tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
 tmp-objdir.h           | 29 +++++++++++++++++++---
 8 files changed, 162 insertions(+), 14 deletions(-)

builtin/prune.c +16 -4

index 485c9a3c56f..c2bcdc07db4 100644
@@ -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)) {
+		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;
 }

builtin/receive-pack.c +1 -1

index 49b846d9605..8815e24cde5 100644
@@ -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);

environment.c +5 -0

index 9da7f3c1a19..342400fcaad 100644
@@ -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 (tmp_objdir)
+		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
 	free(path);
 }

object-file.c +42 -2

56 lines not on this page · raw message

object-store.h +19 -0

index 952efb6a4be..cb173e69392 100644
@@ -27,6 +27,11 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This object store is ephemeral, so there is no need to fsync.
+	 */
+	int will_destroy;
+
 	/*
 	 * Path to the alternative object store. If this is a relative path,
 	 * it is relative to the current working directory.

@@ -58,6 +63,17 @@ void add_to_alternates_file(const char *dir);
  */
 void add_to_alternates_memory(const char *dir);
 
+/*
+ * Replace the current writable object directory with the specified temporary
+ * object directory; returns the former primary object directory.
+ */
+struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy);
+
+/*
+ * Restore a previous ODB replaced by set_temporary_main_odb.
+ */
+void restore_primary_odb(struct object_directory *restore_odb, const char *old_path);
+
 /*
  * Populate and return the loose object cache array corresponding to the
  * given object ID.

@@ -68,6 +84,9 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 /* Empty the loose object cache for the specified object directory. */
 void odb_clear_loose_cache(struct object_directory *odb);
 
+/* Clear and free the specified object directory */
+void free_object_directory(struct object_directory *odb);
+
 struct packed_git {
 	struct hashmap_entry packmap_ent;
 	struct packed_git *next;

object.c +1 -1

index 23a24e678a8..048f96a260e 100644
@@ -513,7 +513,7 @@ struct raw_object_store *raw_object_store_new(void)
 	return o;
 }
 
-static void free_object_directory(struct object_directory *odb)
+void free_object_directory(struct object_directory *odb)
 {
 	free(odb->path);
 	odb_clear_loose_cache(odb);

tmp-objdir.c +52 -3

102 lines not on this page · raw message

tmp-objdir.h +26 -3

index b1e45b4c75d..a3145051f25 100644
@@ -10,7 +10,7 @@
  *
  * Example:
  *
- *	struct tmp_objdir *t = tmp_objdir_create();
+ *	struct tmp_objdir *t = tmp_objdir_create("incoming");
  *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
  *	    !tmp_objdir_migrate(t))
  *		printf("success!\n");

@@ -22,9 +22,10 @@
 struct tmp_objdir;
 
 /*
- * Create a new temporary object directory; returns NULL on failure.
+ * Create a new temporary object directory with the specified prefix;
+ * returns NULL on failure.
  */
-struct tmp_objdir *tmp_objdir_create(void);
+struct tmp_objdir *tmp_objdir_create(const char *prefix);
 
 /*
  * Return a list of environment strings, suitable for use with

@@ -51,4 +52,26 @@ int tmp_objdir_destroy(struct tmp_objdir *);
  */
 void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
 
+/*
+ * Replaces the main object store in the current process with the temporary
+ * object directory and makes the former main object store an alternate.
+ * If will_destroy is nonzero, the object directory may not be migrated.
+ */
+void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);
+
+/*
+ * If the primary object database was replaced by a temporary object directory,
+ * restore it to its original value while keeping the directory contents around.
+ * Returns NULL if the primary object database was not replaced.
+ */
+struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);
+
+/*
+ * Reapplies the former primary temporary object database, after protentially
+ * changing its relative path.
+ */
+void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,
+		const char *new_cwd);
+
+
 #endif /* TMP_OBJDIR_H */

Signature · 1 line

gitgitgadget

Neeraj Singh via GitGitGadget

author

2021-12-04 02:41 UTC #2 lore raw

PATCH 2/2 tmp-objdir: disable ref updates when replacing the primary odb

From: Neeraj Singh <[email protected]>

When creating a subprocess with a temporary ODB, we set the GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not to update refs, since the tmp-objdir may go away.

Introduce a similar mechanism for in-process temporary ODBs when we call tmp_objdir_replace_primary_odb. Now both mechanisms set the disable_ref_updates flag on the odb, which is queried by the ref_transaction_prepare function.

Note: This change adds an assumption that the state of the_repository is relevant for any ref transaction that might be initiated. Unwinding this assumption should be straightforward by saving the relevant repository to query in the transaction or the ref_store.

Peff's test case was invoking ref updates via the cachetextconv setting. That particular code silently does nothing when a ref update is forbidden. See the call to notes_cache_put in fill_textconv where errors are ignored.

(https://github.com/git/git/pull/1076#discussion_r750645345)

---
 environment.c  | 4 ++++
 object-file.c  | 6 ++++++
 object-store.h | 9 ++++++++-
 refs.c         | 2 +-
 repository.c   | 2 ++
 repository.h   | 1 +
 6 files changed, 22 insertions(+), 2 deletions(-)

environment.c +4 -0

index 342400fcaad..2701dfeeec8 100644
@@ -169,6 +169,10 @@ void setup_git_env(const char *git_dir)
 	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
 	args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT);
 	args.alternate_db = getenv_safe(&to_free, ALTERNATE_DB_ENVIRONMENT);
+	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+		args.disable_ref_updates = 1;
+	}
+
 	repo_set_gitdir(the_repository, git_dir, &args);
 	strvec_clear(&to_free);

object-file.c +6 -0

index 0b6a61aeaff..659ef7623ff 100644
@@ -699,6 +699,12 @@ struct object_directory *set_temporary_primary_odb(const char *dir, int will_des
 	 */
 	new_odb = xcalloc(1, sizeof(*new_odb));
 	new_odb->path = xstrdup(dir);
+
+	/*
+	 * Disable ref updates while a temporary odb is active, since
+	 * the objects in the database may roll back.
+	 */
+	new_odb->disable_ref_updates = 1;
 	new_odb->will_destroy = will_destroy;
 	new_odb->next = the_repository->objects->odb;
 	the_repository->objects->odb = new_odb;

object-store.h +8 -1

index cb173e69392..9ae9262c340 100644
@@ -27,10 +27,17 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This is a temporary object store created by the tmp_objdir
+	 * facility. Disable ref updates since the objects in the store
+	 * might be discarded on rollback.
+	 */
+	unsigned int disable_ref_updates : 1;
+
 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */
-	int will_destroy;
+	unsigned int will_destroy : 1;
 
 	/*
 	 * Path to the alternative object store. If this is a relative path,

refs.c +1 -1

index d7cc0a23a3b..27ec7d1fc64 100644
@@ -2137,7 +2137,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
 		break;
 	}
 
-	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+	if (the_repository->objects->odb->disable_ref_updates) {
 		strbuf_addstr(err,
 			      _("ref updates forbidden inside quarantine environment"));
 		return -1;

repository.c +2 -0

index c5b90ba93ea..dce8e35ac20 100644
@@ -80,6 +80,8 @@ void repo_set_gitdir(struct repository *repo,
 	expand_base_dir(&repo->objects->odb->path, o->object_dir,
 			repo->commondir, "objects");
 
+	repo->objects->odb->disable_ref_updates = o->disable_ref_updates;
+
 	free(repo->objects->alternate_db);
 	repo->objects->alternate_db = xstrdup_or_null(o->alternate_db);
 	expand_base_dir(&repo->graft_file, o->graft_file,

repository.h +1 -0

index a057653981c..7c04e99ac5c 100644
@@ -158,6 +158,7 @@ struct set_gitdir_args {
 	const char *graft_file;
 	const char *index_file;
 	const char *alternate_db;
+	int disable_ref_updates;
 };
 
 void repo_set_gitdir(struct repository *repo, const char *root,

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-12-05 18:23 UTC #3 lore raw

Re: [PATCH 2/2] tmp-objdir: disable ref updates when replacing the primary odb

object-store.h struct object_directory { · line 40

Quoted 5 lines · Neeraj Singh via GitGitGadget

object-store.h +1 -1

 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */
-	int will_destroy;
+	unsigned int will_destroy : 1;

<CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com> ?

Neeraj Singh

2021-12-05 23:44 UTC #4 lore raw

Re: [PATCH 2/2] tmp-objdir: disable ref updates when replacing the primary odb

Quoted 11 lines · Junio C Hamano

object-store.h struct object_directory { · line 40
Quoted 5 lines · Neeraj Singh via GitGitGadget
object-store.h +1 -1
 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */
-	int will_destroy;
+	unsigned int will_destroy : 1;
<CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com> ?

Quoted 1 line

(https://github.com/git/git/pull/1076#discussion_r750645345)

Thanks for noticing this! I also lost one other change while splitting this out: we are referencing the_repository from the refs code, but as of 34224e14d we should be picking it up from the ref_store. I'll submit an updated series as soon as it passes CI.

Thanks,
Neeraj

Junio C Hamano

2021-12-05 23:56 UTC #5 lore raw

Re: [PATCH 2/2] tmp-objdir: disable ref updates when replacing the primary odb

Quoted 12 lines · Neeraj Singh

Quoted 11 lines · Junio C Hamano
object-store.h struct object_directory { · line 40
Quoted 5 lines · Neeraj Singh via GitGitGadget
object-store.h +1 -1
 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */
-	int will_destroy;
+	unsigned int will_destroy : 1;
<CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com> ?

Quoted 7 lines

Quoted 1 line
(https://github.com/git/git/pull/1076#discussion_r750645345)
Thanks for noticing this! I also lost one other change while splitting this out: we are referencing the_repository from the refs code, but as of 34224e14d we should be picking it up from the ref_store. I'll submit an updated series as soon as it passes CI.

No rush.

Reviewers and other project participants would appreciate you more if you took a deep breath, after seeing a CI success, and gave a final re-reading of the patches with a critical pair of eyes, before you send the updated series out.

Thanks.

Neeraj Singh

2021-12-06 03:10 UTC #6 lore raw

Re: [PATCH 2/2] tmp-objdir: disable ref updates when replacing the primary odb

Quoted 23 lines · Junio C Hamano

Quoted 12 lines · Neeraj Singh
Quoted 11 lines · Junio C Hamano
object-store.h struct object_directory { · line 40
Quoted 5 lines · Neeraj Singh via GitGitGadget
   /*
    * This object store is ephemeral, so there is no need to fsync.
    */
object-store.h +1 -1
-  int will_destroy;
+  unsigned int will_destroy : 1;
<CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com> ?
Quoted 7 lines
Quoted 1 line
(https://github.com/git/git/pull/1076#discussion_r750645345)
Thanks for noticing this! I also lost one other change while splitting this out: we are referencing the_repository from the refs code, but as of 34224e14d we should be picking it up from the ref_store. I'll submit an updated series as soon as it passes CI.

Quoted 8 lines

No rush.
Reviewers and other project participants would appreciate you more if you took a deep breath, after seeing a CI success, and gave a final re-reading of the patches with a critical pair of eyes, before you send the updated series out.
Thanks.

Fair enough. Of course I didn't see your email before I resubmitted. Thanks for the feedback.

Neeraj K. Singh via GitGitGadget

author

2021-12-06 00:36 UTC #7 lore raw

v2 cover ns/tmp-objdir: add support for temporary writable databases

V2 changes: I lost a couple changes in the shuffle while splitting these changes out

* Make the will-destroy boolean a single bit field of type unsigned int so that it doesn't change twice in this small patch series.
* Remove a the_repository reference in the disable ref updates code. Now the repository is taken from the ref_store.

New interface into the tmp-objdir API to help in-core use of the quarantine feature.

This patch series was formerly part of the ns/batched-fsync topic [1]. It's now split out into its own gitgitgadget PR and discussion thread since it is the base for en/remerge-diff as well.

The most recent feedback was in [2]. I removed printing from prune_subdir and simplified the strbuf handling in prune_tmp_file.

References: [1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/CABPp-BH6m4q_EoX77bqLcpCN1HRfJ_XayeCV2O0sRybX53rPrw@mail.gmail.com/

Neeraj Singh (2):
tmp-objdir: new API for creating temporary writable databases
tmp-objdir: disable ref updates when replacing the primary odb

 builtin/prune.c        | 20 ++++++++++++---
 builtin/receive-pack.c |  2 +-
 environment.c          |  9 +++++++
 object-file.c          | 50 ++++++++++++++++++++++++++++++++++++--
 object-store.h         | 26 ++++++++++++++++++++
 object.c               |  2 +-
 refs.c                 |  2 +-
 repository.c           |  2 ++
 repository.h           |  1 +
 tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
 tmp-objdir.h           | 29 +++++++++++++++++++---
 11 files changed, 183 insertions(+), 15 deletions(-)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1091%2Fneerajsi-msft%2Fns%2Ftmp-objdir-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1091/neerajsi-msft/ns/tmp-objdir-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1091

Range-diff vs v1:

1: 4ae4303595e ! 1: 36c00613d9a tmp-objdir: new API for creating temporary writable databases

     @@ object-store.h: struct object_directory {
      +	/*
      +	 * This object store is ephemeral, so there is no need to fsync.
      +	 */
     -+	int will_destroy;
     ++	unsigned int will_destroy : 1;
      +
       	/*
       	 * Path to the alternative object store. If this is a relative path,
 2:  d8ae001500c ! 2:  f667cbcc47d tmp-objdir: disable ref updates when replacing the primary odb
     @@ object-store.h: struct object_directory {
       	/*
       	 * This object store is ephemeral, so there is no need to fsync.
       	 */
     --	int will_destroy;
     -+	unsigned int will_destroy : 1;
     - 
     - 	/*
     - 	 * Path to the alternative object store. If this is a relative path,
      
       ## refs.c ##
      @@ refs.c: int ref_transaction_prepare(struct ref_transaction *transaction,
     @@ refs.c: int ref_transaction_prepare(struct ref_transaction *transaction,
       	}
       
      -	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
     -+	if (the_repository->objects->odb->disable_ref_updates) {
     ++	if (refs->repo->objects->odb->disable_ref_updates) {
       		strbuf_addstr(err,
       			      _("ref updates forbidden inside quarantine environment"));
       		return -1;

Signature · 1 line

gitgitgadget

Neeraj Singh via GitGitGadget

author

2021-12-06 00:36 UTC #8 lore raw

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

From: Neeraj Singh <[email protected]>

For the --remerge-diff usecase, add a new will_destroy flag in struct object_database to mark ephemeral object databases that do not require fsync durability.

Add 'git prune' support for removing temporary object databases, and make sure that they have a name starting with tmp_ and containing an operation-specific name.

Based-on-patch-by Elijah Newren <[email protected]>

---
 builtin/prune.c        | 20 ++++++++++++---
 builtin/receive-pack.c |  2 +-
 environment.c          |  5 ++++
 object-file.c          | 44 +++++++++++++++++++++++++++++++--
 object-store.h         | 19 +++++++++++++++
 object.c               |  2 +-
 tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
 tmp-objdir.h           | 29 +++++++++++++++++++---
 8 files changed, 162 insertions(+), 14 deletions(-)

builtin/prune.c +16 -4

index 485c9a3c56f..c2bcdc07db4 100644
@@ -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)) {
+		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;
 }

builtin/receive-pack.c +1 -1

index 49b846d9605..8815e24cde5 100644
@@ -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);

environment.c +5 -0

index 9da7f3c1a19..342400fcaad 100644
@@ -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 (tmp_objdir)
+		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
 	free(path);
 }

object-file.c +42 -2

56 lines not on this page · raw message

object-store.h +19 -0

index 952efb6a4be..82cf13f1054 100644
@@ -27,6 +27,11 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This object store is ephemeral, so there is no need to fsync.
+	 */
+	unsigned int will_destroy : 1;
+
 	/*
 	 * Path to the alternative object store. If this is a relative path,
 	 * it is relative to the current working directory.

@@ -58,6 +63,17 @@ void add_to_alternates_file(const char *dir);
  */
 void add_to_alternates_memory(const char *dir);
 
+/*
+ * Replace the current writable object directory with the specified temporary
+ * object directory; returns the former primary object directory.
+ */
+struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy);
+
+/*
+ * Restore a previous ODB replaced by set_temporary_main_odb.
+ */
+void restore_primary_odb(struct object_directory *restore_odb, const char *old_path);
+
 /*
  * Populate and return the loose object cache array corresponding to the
  * given object ID.

@@ -68,6 +84,9 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 /* Empty the loose object cache for the specified object directory. */
 void odb_clear_loose_cache(struct object_directory *odb);
 
+/* Clear and free the specified object directory */
+void free_object_directory(struct object_directory *odb);
+
 struct packed_git {
 	struct hashmap_entry packmap_ent;
 	struct packed_git *next;

object.c +1 -1

index 23a24e678a8..048f96a260e 100644
@@ -513,7 +513,7 @@ struct raw_object_store *raw_object_store_new(void)
 	return o;
 }
 
-static void free_object_directory(struct object_directory *odb)
+void free_object_directory(struct object_directory *odb)
 {
 	free(odb->path);
 	odb_clear_loose_cache(odb);

tmp-objdir.c +52 -3

102 lines not on this page · raw message

tmp-objdir.h +26 -3

index b1e45b4c75d..a3145051f25 100644
@@ -10,7 +10,7 @@
  *
  * Example:
  *
- *	struct tmp_objdir *t = tmp_objdir_create();
+ *	struct tmp_objdir *t = tmp_objdir_create("incoming");
  *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
  *	    !tmp_objdir_migrate(t))
  *		printf("success!\n");

@@ -22,9 +22,10 @@
 struct tmp_objdir;
 
 /*
- * Create a new temporary object directory; returns NULL on failure.
+ * Create a new temporary object directory with the specified prefix;
+ * returns NULL on failure.
  */
-struct tmp_objdir *tmp_objdir_create(void);
+struct tmp_objdir *tmp_objdir_create(const char *prefix);
 
 /*
  * Return a list of environment strings, suitable for use with

@@ -51,4 +52,26 @@ int tmp_objdir_destroy(struct tmp_objdir *);
  */
 void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
 
+/*
+ * Replaces the main object store in the current process with the temporary
+ * object directory and makes the former main object store an alternate.
+ * If will_destroy is nonzero, the object directory may not be migrated.
+ */
+void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);
+
+/*
+ * If the primary object database was replaced by a temporary object directory,
+ * restore it to its original value while keeping the directory contents around.
+ * Returns NULL if the primary object database was not replaced.
+ */
+struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);
+
+/*
+ * Reapplies the former primary temporary object database, after protentially
+ * changing its relative path.
+ */
+void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,
+		const char *new_cwd);
+
+
 #endif /* TMP_OBJDIR_H */

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-12-06 07:43 UTC #9 lore raw

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

builtin/prune.c static int prune_tmp_file(const char *fullpath) · line 29

Quoted 13 lines · Neeraj Singh via GitGitGadget

builtin/prune.c +1 -4

index 485c9a3c56f..c2bcdc07db4 100644
@@ -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.

Quoted 44 lines

builtin/prune.c +15 -0

+		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;
 }

builtin/receive-pack.c +1 -1

index 49b846d9605..8815e24cde5 100644
@@ -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);

environment.c +1 -0

index 9da7f3c1a19..342400fcaad 100644
@@ -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"

environment.c static void update_relative_gitdir(const char *name, · line 340

Quoted 9 lines

environment.c +2 -0

@@ -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()").

environment.c static void update_relative_gitdir(const char *name, · line 344

Quoted 4 lines

environment.c +2 -0

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

environment.c int hash_object_file(const struct git_hash_algo *algo, const void *buf, · line 1851

Quoted 9 lines

environment.c +3 -2

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

Quoted 12 lines

tmp-objdir.c +1 -0

index b8d880e3626..3d38eeab66b 100644
@@ -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"

tmp-objdir.c line 16

Quoted 5 lines

tmp-objdir.c +2 -0

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

Neeraj Singh

2021-12-06 08:53 UTC #10 lore raw

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

Quoted 17 lines · Junio C Hamano

environment.c static void update_relative_gitdir(const char *name, · line 340
Quoted 10 lines · Neeraj Singh via GitGitGadget
environment.c +2 -0
@@ -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()").

Will fix.

Quoted 10 lines

environment.c static void update_relative_gitdir(const char *name, · line 344
Quoted 4 lines
environment.c +2 -0
+	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.

The lack of this code was causing a failure, I believe in t2107-update-index-basic.sh: "--refresh triggers late setup_work_tree".

This problem came up after applying: https://lore.kernel.org/git/4a40fd4a29a468b9ce320bc7b22f19e5a526fad6.1637020263.git.gitgitgadget@gmail.com/

I thought it would be best to fix this in the tmp-objdir code so that callers could plug/unplug bulk checkin without any subtle surprises.

Quoted 21 lines

object-file.c int hash_object_file(const struct git_hash_algo *algo, const void *buf, · line 1851
Quoted 9 lines
object-file.c +3 -2
@@ -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.

Quoted 7 lines

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.

Yes, this code is there exactly for remerge-diff and anyone doing something similar in the future.

Quoted 15 lines

tmp-objdir.c +1 -0

index b8d880e3626..3d38eeab66b 100644
@@ -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;

Quoted 9 lines

tmp-objdir.c line 16
Quoted 2 lines
tmp-objdir.c +2 -0
+	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.

The principle I was trying to follow here is that the only flag in a structure might as well be a full integer, but when we have two or more it might be worth combining them into a single machine word. Given that these are not highly replicated structures, you're right that's it's not a big benefit.

I'll switch everything to an int and call it good.

Given that this patch series introduces functions with no users, are you going to hold off on putting this into 'next' until another next-worthy patch series is ready? I've already reworked the batch mode stuff on Github, but I'll need to do a lot more testing before sending it to the list.

Thanks,
Neeraj

Junio C Hamano

2021-12-06 17:39 UTC #11 lore raw

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

Quoted 11 lines · Neeraj Singh

Quoted 10 lines
environment.c static void update_relative_gitdir(const char *name, · line 344
Quoted 4 lines
environment.c +2 -0
+	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.

Quoted 7 lines

The lack of this code was causing a failure, I believe in t2107-update-index-basic.sh: "--refresh triggers late setup_work_tree".
This problem came up after applying: https://lore.kernel.org/git/4a40fd4a29a468b9ce320bc7b22f19e5a526fad6.1637020263.git.gitgitgadget@gmail.com/
I thought it would be best to fix this in the tmp-objdir code so that callers could plug/unplug bulk checkin without any subtle surprises.

OK, I think that is fine.

As a slightly-related tangent that is outside the topic, I think we should revisit "update-index", which is one of the oldest plumbing commands with its own quirks. I do not offhand see why it needs to sprinkle this many setup_work_tree() calls everywhere. Having an index to work on means we must have a working tree to update and/or refresh from. We should be able to get away with the NEED_WORK_TREE bit in the git.c::commands[] table for this command. If this were a more recent command, I may suspect that there were valid reasons like "in this particular mode, update-index must work inside a bare repository" to force us to take this unusual program structure, but because this is probably a lot older than NEED_WORK_TREE bit, I would not be surprised if the answer were "nobody noticed the ugliness so far".

Quoted 3 lines

Given that this patch series introduces functions with no users, are you going to hold off on putting this into 'next' until another next-worthy patch series is ready?

Even without any existing callers, as long as we see Reviewed-by: by Elijah, who we know will have to build on top of this series, I think this can and should go to 'next'.

Thanks.

Neeraj Singh via GitGitGadget

author

2021-12-06 00:36 UTC #12 lore raw

PATCH v2 2/2 tmp-objdir: disable ref updates when replacing the primary odb

From: Neeraj Singh <[email protected]>

When creating a subprocess with a temporary ODB, we set the GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not to update refs, since the tmp-objdir may go away.

---
 environment.c  | 4 ++++
 object-file.c  | 6 ++++++
 object-store.h | 7 +++++++
 refs.c         | 2 +-
 repository.c   | 2 ++
 repository.h   | 1 +
 6 files changed, 21 insertions(+), 1 deletion(-)

environment.c +4 -0

index 342400fcaad..2701dfeeec8 100644
@@ -169,6 +169,10 @@ void setup_git_env(const char *git_dir)
 	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
 	args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT);
 	args.alternate_db = getenv_safe(&to_free, ALTERNATE_DB_ENVIRONMENT);
+	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+		args.disable_ref_updates = 1;
+	}
+
 	repo_set_gitdir(the_repository, git_dir, &args);
 	strvec_clear(&to_free);

object-file.c +6 -0

index 0b6a61aeaff..659ef7623ff 100644
@@ -699,6 +699,12 @@ struct object_directory *set_temporary_primary_odb(const char *dir, int will_des
 	 */
 	new_odb = xcalloc(1, sizeof(*new_odb));
 	new_odb->path = xstrdup(dir);
+
+	/*
+	 * Disable ref updates while a temporary odb is active, since
+	 * the objects in the database may roll back.
+	 */
+	new_odb->disable_ref_updates = 1;
 	new_odb->will_destroy = will_destroy;
 	new_odb->next = the_repository->objects->odb;
 	the_repository->objects->odb = new_odb;

object-store.h +7 -0

index 82cf13f1054..9ae9262c340 100644
@@ -27,6 +27,13 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This is a temporary object store created by the tmp_objdir
+	 * facility. Disable ref updates since the objects in the store
+	 * might be discarded on rollback.
+	 */
+	unsigned int disable_ref_updates : 1;
+
 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */

refs.c +1 -1

index d7cc0a23a3b..ac744e85f5f 100644
@@ -2137,7 +2137,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
 		break;
 	}
 
-	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+	if (refs->repo->objects->odb->disable_ref_updates) {
 		strbuf_addstr(err,
 			      _("ref updates forbidden inside quarantine environment"));
 		return -1;

repository.c +2 -0

index c5b90ba93ea..dce8e35ac20 100644
@@ -80,6 +80,8 @@ void repo_set_gitdir(struct repository *repo,
 	expand_base_dir(&repo->objects->odb->path, o->object_dir,
 			repo->commondir, "objects");
 
+	repo->objects->odb->disable_ref_updates = o->disable_ref_updates;
+
 	free(repo->objects->alternate_db);
 	repo->objects->alternate_db = xstrdup_or_null(o->alternate_db);
 	expand_base_dir(&repo->graft_file, o->graft_file,

repository.h +1 -0

index a057653981c..7c04e99ac5c 100644
@@ -158,6 +158,7 @@ struct set_gitdir_args {
 	const char *graft_file;
 	const char *index_file;
 	const char *alternate_db;
+	int disable_ref_updates;
 };
 
 void repo_set_gitdir(struct repository *repo, const char *root,

Signature · 1 line

gitgitgadget

Neeraj Singh

2021-12-06 03:12 UTC #13 lore raw

Re: [PATCH v2 2/2] tmp-objdir: disable ref updates when replacing the primary odb

Quoted 9 lines · Neeraj Singh via GitGitGadget

From: Neeraj Singh <[email protected]>
Note: This change adds an assumption that the state of the_repository is relevant for any ref transaction that might be initiated. Unwinding this assumption should be straightforward by saving the relevant repository to query in the transaction or the ref_store.

This part of the commit description needs to be deleted, since it no longer applies. We pick up the repo from the ref_store now.

Neeraj K. Singh via GitGitGadget

author

2021-12-06 22:05 UTC #14 lore raw

v3 cover ns/tmp-objdir: add support for temporary writable databases

V3 (hopefully final):

* Fix the commit description for patch [2/2] to reflect the fact that disabling ref updates no longer depends on the_repository.
* Add a link to Jeff King's test case in patch [2/2]. The test relies on remerge-diff, so it can't be directly included here.
* Adjust line spacing in update_relative_gitdir (gitster)
* Switch struct object_directory to use full-width integers rather than flags (gitster)
* Fix typo s/protentially/potentially (neerajsi)

V2 changes: I lost a couple changes in the shuffle while splitting these changes out

New interface into the tmp-objdir API to help in-core use of the quarantine feature.

This patch series was formerly part of the ns/batched-fsync topic [1]. It's now split out into its own gitgitgadget PR and discussion thread since it is the base for en/remerge-diff as well.

The most recent feedback was in [2]. I removed printing from prune_subdir and simplified the strbuf handling in prune_tmp_file.

References: [1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/CABPp-BH6m4q_EoX77bqLcpCN1HRfJ_XayeCV2O0sRybX53rPrw@mail.gmail.com/

Neeraj Singh (2):
tmp-objdir: new API for creating temporary writable databases
tmp-objdir: disable ref updates when replacing the primary odb

 builtin/prune.c        | 20 ++++++++++++---
 builtin/receive-pack.c |  2 +-
 environment.c          |  9 +++++++
 object-file.c          | 50 ++++++++++++++++++++++++++++++++++++--
 object-store.h         | 26 ++++++++++++++++++++
 object.c               |  2 +-
 refs.c                 |  2 +-
 repository.c           |  2 ++
 repository.h           |  1 +
 tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
 tmp-objdir.h           | 29 +++++++++++++++++++---
 11 files changed, 183 insertions(+), 15 deletions(-)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1091%2Fneerajsi-msft%2Fns%2Ftmp-objdir-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1091/neerajsi-msft/ns/tmp-objdir-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1091

Range-diff vs v2:

1: 36c00613d9a ! 1: cccb3888070 tmp-objdir: new API for creating temporary writable databases

     @@ environment.c: static void update_relative_gitdir(const char *name,
       {
       	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 (tmp_objdir)
      +		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
     @@ object-store.h: struct object_directory {
      +	/*
      +	 * This object store is ephemeral, so there is no need to fsync.
      +	 */
     -+	unsigned int will_destroy : 1;
     ++	int will_destroy;
      +
       	/*
       	 * Path to the alternative object store. If this is a relative path,
     @@ tmp-objdir.h: int tmp_objdir_destroy(struct tmp_objdir *);
       void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
       
      +/*
     -+ * Replaces the main object store in the current process with the temporary
     ++ * Replaces the writable object store in the current process with the temporary
      + * object directory and makes the former main object store an alternate.
      + * If will_destroy is nonzero, the object directory may not be migrated.
      + */
     @@ tmp-objdir.h: int tmp_objdir_destroy(struct tmp_objdir *);
      +struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);
      +
      +/*
     -+ * Reapplies the former primary temporary object database, after protentially
     ++ * Reapplies the former primary temporary object database, after potentially
      + * changing its relative path.
      + */
      +void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,

2: f667cbcc47d ! 2: 4e44121c2d7 tmp-objdir: disable ref updates when replacing the primary odb
@@ Commit message
the disable_ref_updates flag on the odb, which is queried by
the ref_transaction_prepare function.

- Note: This change adds an assumption that the state of
- the_repository is relevant for any ref transaction that might
- be initiated. Unwinding this assumption should be straightforward
- by saving the relevant repository to query in the transaction or
- the ref_store.
-
- Peff's test case was invoking ref updates via the cachetextconv
+ Peff's test case [1] was invoking ref updates via the cachetextconv
setting. That particular code silently does nothing when a ref update is forbidden. See the call to notes_cache_put in fill_textconv where errors are ignored.

+ [1] https://lore.kernel.org/git/[email protected]/
+

@@ object-store.h: struct object_directory {
+ * facility. Disable ref updates since the objects in the store
+ * might be discarded on rollback.
+ */
-+ unsigned int disable_ref_updates : 1;
++ int disable_ref_updates;
+
/*
* This object store is ephemeral, so there is no need to fsync.

Signature · 1 line

gitgitgadget

Neeraj Singh via GitGitGadget

author

2021-12-06 22:05 UTC #15 lore raw

PATCH v3 1/2 tmp-objdir: new API for creating temporary writable databases

From: Neeraj Singh <[email protected]>

For the --remerge-diff usecase, add a new will_destroy flag in struct object_database to mark ephemeral object databases that do not require fsync durability.

Add 'git prune' support for removing temporary object databases, and make sure that they have a name starting with tmp_ and containing an operation-specific name.

Based-on-patch-by Elijah Newren <[email protected]>

---
 builtin/prune.c        | 20 ++++++++++++---
 builtin/receive-pack.c |  2 +-
 environment.c          |  5 ++++
 object-file.c          | 44 +++++++++++++++++++++++++++++++--
 object-store.h         | 19 +++++++++++++++
 object.c               |  2 +-
 tmp-objdir.c           | 55 +++++++++++++++++++++++++++++++++++++++---
 tmp-objdir.h           | 29 +++++++++++++++++++---
 8 files changed, 162 insertions(+), 14 deletions(-)

builtin/prune.c +16 -4

index 485c9a3c56f..c2bcdc07db4 100644
@@ -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)) {
+		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;
 }

builtin/receive-pack.c +1 -1

index 49b846d9605..8815e24cde5 100644
@@ -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);

environment.c +5 -0

index 9da7f3c1a19..fe51dfe24d4 100644
@@ -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 (tmp_objdir)
+		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
 	free(path);
 }

object-file.c +42 -2

56 lines not on this page · raw message

object-store.h +19 -0

index 952efb6a4be..cb173e69392 100644
@@ -27,6 +27,11 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This object store is ephemeral, so there is no need to fsync.
+	 */
+	int will_destroy;
+
 	/*
 	 * Path to the alternative object store. If this is a relative path,
 	 * it is relative to the current working directory.

@@ -58,6 +63,17 @@ void add_to_alternates_file(const char *dir);
  */
 void add_to_alternates_memory(const char *dir);
 
+/*
+ * Replace the current writable object directory with the specified temporary
+ * object directory; returns the former primary object directory.
+ */
+struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy);
+
+/*
+ * Restore a previous ODB replaced by set_temporary_main_odb.
+ */
+void restore_primary_odb(struct object_directory *restore_odb, const char *old_path);
+
 /*
  * Populate and return the loose object cache array corresponding to the
  * given object ID.

@@ -68,6 +84,9 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 /* Empty the loose object cache for the specified object directory. */
 void odb_clear_loose_cache(struct object_directory *odb);
 
+/* Clear and free the specified object directory */
+void free_object_directory(struct object_directory *odb);
+
 struct packed_git {
 	struct hashmap_entry packmap_ent;
 	struct packed_git *next;

object.c +1 -1

index 23a24e678a8..048f96a260e 100644
@@ -513,7 +513,7 @@ struct raw_object_store *raw_object_store_new(void)
 	return o;
 }
 
-static void free_object_directory(struct object_directory *odb)
+void free_object_directory(struct object_directory *odb)
 {
 	free(odb->path);
 	odb_clear_loose_cache(odb);

tmp-objdir.c +52 -3

102 lines not on this page · raw message

tmp-objdir.h +26 -3

index b1e45b4c75d..cda5ec76778 100644
@@ -10,7 +10,7 @@
  *
  * Example:
  *
- *	struct tmp_objdir *t = tmp_objdir_create();
+ *	struct tmp_objdir *t = tmp_objdir_create("incoming");
  *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
  *	    !tmp_objdir_migrate(t))
  *		printf("success!\n");

@@ -22,9 +22,10 @@
 struct tmp_objdir;
 
 /*
- * Create a new temporary object directory; returns NULL on failure.
+ * Create a new temporary object directory with the specified prefix;
+ * returns NULL on failure.
  */
-struct tmp_objdir *tmp_objdir_create(void);
+struct tmp_objdir *tmp_objdir_create(const char *prefix);
 
 /*
  * Return a list of environment strings, suitable for use with

@@ -51,4 +52,26 @@ int tmp_objdir_destroy(struct tmp_objdir *);
  */
 void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
 
+/*
+ * Replaces the writable object store in the current process with the temporary
+ * object directory and makes the former main object store an alternate.
+ * If will_destroy is nonzero, the object directory may not be migrated.
+ */
+void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);
+
+/*
+ * If the primary object database was replaced by a temporary object directory,
+ * restore it to its original value while keeping the directory contents around.
+ * Returns NULL if the primary object database was not replaced.
+ */
+struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);
+
+/*
+ * Reapplies the former primary temporary object database, after potentially
+ * changing its relative path.
+ */
+void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,
+		const char *new_cwd);
+
+
 #endif /* TMP_OBJDIR_H */

Signature · 1 line

gitgitgadget

Neeraj Singh via GitGitGadget

author

2021-12-06 22:05 UTC #16 lore raw

PATCH v3 2/2 tmp-objdir: disable ref updates when replacing the primary odb

[1] https://lore.kernel.org/git/[email protected]/

From: Neeraj Singh <[email protected]>

When creating a subprocess with a temporary ODB, we set the GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not to update refs, since the tmp-objdir may go away.

Peff's test case [1] was invoking ref updates via the cachetextconv setting. That particular code silently does nothing when a ref update is forbidden. See the call to notes_cache_put in fill_textconv where errors are ignored.

---
 environment.c  | 4 ++++
 object-file.c  | 6 ++++++
 object-store.h | 7 +++++++
 refs.c         | 2 +-
 repository.c   | 2 ++
 repository.h   | 1 +
 6 files changed, 21 insertions(+), 1 deletion(-)

environment.c +4 -0

index fe51dfe24d4..a8b64f5194f 100644
@@ -169,6 +169,10 @@ void setup_git_env(const char *git_dir)
 	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
 	args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT);
 	args.alternate_db = getenv_safe(&to_free, ALTERNATE_DB_ENVIRONMENT);
+	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+		args.disable_ref_updates = 1;
+	}
+
 	repo_set_gitdir(the_repository, git_dir, &args);
 	strvec_clear(&to_free);

object-file.c +6 -0

index 0b6a61aeaff..659ef7623ff 100644
@@ -699,6 +699,12 @@ struct object_directory *set_temporary_primary_odb(const char *dir, int will_des
 	 */
 	new_odb = xcalloc(1, sizeof(*new_odb));
 	new_odb->path = xstrdup(dir);
+
+	/*
+	 * Disable ref updates while a temporary odb is active, since
+	 * the objects in the database may roll back.
+	 */
+	new_odb->disable_ref_updates = 1;
 	new_odb->will_destroy = will_destroy;
 	new_odb->next = the_repository->objects->odb;
 	the_repository->objects->odb = new_odb;

object-store.h +7 -0

index cb173e69392..6f89482df03 100644
@@ -27,6 +27,13 @@ struct object_directory {
 	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
 	struct oidtree *loose_objects_cache;
 
+	/*
+	 * This is a temporary object store created by the tmp_objdir
+	 * facility. Disable ref updates since the objects in the store
+	 * might be discarded on rollback.
+	 */
+	int disable_ref_updates;
+
 	/*
 	 * This object store is ephemeral, so there is no need to fsync.
 	 */

refs.c +1 -1

index d7cc0a23a3b..ac744e85f5f 100644
@@ -2137,7 +2137,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
 		break;
 	}
 
-	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+	if (refs->repo->objects->odb->disable_ref_updates) {
 		strbuf_addstr(err,
 			      _("ref updates forbidden inside quarantine environment"));
 		return -1;

repository.c +2 -0

index c5b90ba93ea..dce8e35ac20 100644
@@ -80,6 +80,8 @@ void repo_set_gitdir(struct repository *repo,
 	expand_base_dir(&repo->objects->odb->path, o->object_dir,
 			repo->commondir, "objects");
 
+	repo->objects->odb->disable_ref_updates = o->disable_ref_updates;
+
 	free(repo->objects->alternate_db);
 	repo->objects->alternate_db = xstrdup_or_null(o->alternate_db);
 	expand_base_dir(&repo->graft_file, o->graft_file,

repository.h +1 -0

index a057653981c..7c04e99ac5c 100644
@@ -158,6 +158,7 @@ struct set_gitdir_args {
 	const char *graft_file;
 	const char *index_file;
 	const char *alternate_db;
+	int disable_ref_updates;
 };
 
 void repo_set_gitdir(struct repository *repo, const char *root,

Signature · 1 line

gitgitgadget

Elijah Newren

Reviewed-by on the v3 series

2021-12-08 16:41 UTC #17 lore raw