Re: [PATCH] builtin-bundle create - use lock_file

Mark Levedahl [off-list ref] writes:

git bundle create would leave an invalid, partially written bundle if
an error occured during creation. Fix that using lock_file.

Signed-off-by: Mark Levedahl <redacted>
---
 struct lock_file is now static.
 *caller* closes the lock file before commiting / rolling back.

I think you broke the support to write a bundle out to the
standard output by giving "-" as the path.

Also, the whole point of the "NEEDSWORK" suggestion to use
lockfile was because you do not have to do lwrite_or_die() in
the first place ;-).  When you exit before committing lockfiles
you hold, atexit handler rolls back and unlink them for you.

The following might be a better replacement.

By the way, by using lockfile's "create temporary, generate into
it and then rename to final", we are allowing an overwrite of an
existing bundle, which we did not allow earlier.  Is this check
something we would want to preserve?

---

 builtin-bundle.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index f4b4f03..1b65006 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c

@@ -189,7 +189,9 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
+	static struct lock_file lock;
 	int bundle_fd = -1;
+	int bundle_to_stdout;
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(5 * sizeof(const char *));
 	int i, ref_count = 0;

@@ -198,14 +200,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	struct child_process rls;
 	FILE *rls_fout;
 
-	/*
-	 * NEEDSWORK: this should use something like lock-file
-	 * to create temporary that is cleaned up upon error.
-	 */
-	bundle_fd = (!strcmp(path, "-") ? 1 :
-			open(path, O_CREAT | O_EXCL | O_WRONLY, 0666));
-	if (bundle_fd < 0)
-		return error("Could not create '%s': %s", path, strerror(errno));
+	bundle_to_stdout = !strcmp(path, "-");
+	if (bundle_to_stdout)
+		bundle_fd = 1;
+	else
+		bundle_fd = hold_lock_file_for_update(&lock, path, 1);
 
 	/* write signature */
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));

@@ -341,6 +340,9 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	}
 	if (finish_command(&rls))
 		return error ("pack-objects died");
+	close(bundle_fd);
+	if (!bundle_to_stdout)
+		commit_lock_file(&lock);
 	return 0;
 }
 

Junio C Hamano wrote:

When you exit before committing lockfiles
you hold, atexit handler rolls back and unlink them for you.

The following might be a better replacement.

By the way, by using lockfile's "create temporary, generate into
it and then rename to final", we are allowing an overwrite of an
existing bundle, which we did not allow earlier.  Is this check
something we would want to preserve?
  

Ack - yours is much better. Thanks for the lesson. Also, the former 
behavior of not allowing overwrite of an existing file was (at least in 
my opinion) very un-git'ish so I am happy to see that go.

Mark