[PATCH] builtin-bundle create - use lock_file semantics

[PATCH] builtin-bundle create - use lock_file semantics

[Mark Levedahl]

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 <mdl123@verizon.net>
---
 builtin-bundle.c |   51 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index f4b4f03..82e00f5 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -186,10 +186,20 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)
 	return list_refs(&header->references, argc, argv);
 }
 
+/* create_bundle uses lock_file, delete if write fails */
+static inline void lwrite_or_die(int fd, const void *buf, size_t count, struct lock_file *lock)
+{
+	if (write_in_full(fd, buf, count) != count) {
+		rollback_lock_file(lock);
+		die("Unable to write bundle");
+	}
+}
+
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
 	int bundle_fd = -1;
+	struct lock_file lock;
 	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,17 +208,9 @@ 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));
-
 	/* write signature */
-	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
+	bundle_fd = hold_lock_file_for_update(&lock, path, 1);
+	lwrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature), &lock);
 
 	/* init revs to list objects for pack-objects later */
 	save_commit_buffer = 0;
@@ -230,7 +232,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	while (fgets(buffer, sizeof(buffer), rls_fout)) {
 		unsigned char sha1[20];
 		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, strlen(buffer));
+			lwrite_or_die(bundle_fd, buffer, strlen(buffer), &lock);
 			if (!get_sha1_hex(buffer + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
@@ -242,13 +244,17 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		}
 	}
 	fclose(rls_fout);
-	if (finish_command(&rls))
+	if (finish_command(&rls)) {
+		rollback_lock_file(&lock);
 		return error("rev-list died");
+	}
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	if (argc > 1)
+	if (argc > 1) {
+		rollback_lock_file(&lock);
 		return error("unrecognized argument: %s'", argv[1]);
+	}
 
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
@@ -307,17 +313,19 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		}
 
 		ref_count++;
-		write_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);
-		write_or_die(bundle_fd, " ", 1);
-		write_or_die(bundle_fd, ref, strlen(ref));
-		write_or_die(bundle_fd, "\n", 1);
+		lwrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40, &lock);
+		lwrite_or_die(bundle_fd, " ", 1, &lock);
+		lwrite_or_die(bundle_fd, ref, strlen(ref), &lock);
+		lwrite_or_die(bundle_fd, "\n", 1, &lock);
 		free(ref);
 	}
-	if (!ref_count)
+	if (!ref_count) {
+		rollback_lock_file(&lock);
 		die ("Refusing to create empty bundle.");
+	}
 
 	/* end header */
-	write_or_die(bundle_fd, "\n", 1);
+	lwrite_or_die(bundle_fd, "\n", 1, &lock);
 
 	/* write pack */
 	argv_pack[0] = "pack-objects";
@@ -339,8 +347,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		write(rls.in, sha1_to_hex(object->sha1), 40);
 		write(rls.in, "\n", 1);
 	}
-	if (finish_command(&rls))
+	if (finish_command(&rls)) {
+		rollback_lock_file(&lock);
 		return error ("pack-objects died");
+	}
+	commit_lock_file(&lock);
 	return 0;
 }
 
-- 
1.5.3.rc4.78.g5acb3-dirty

[PATCH] builtin-bundle create - use lock_file

[Mark Levedahl]

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 <mdl123@verizon.net>
---
 struct lock_file is now static.
 *caller* closes the lock file before commiting / rolling back.

 builtin-bundle.c |   57 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index f4b4f03..e5a2859 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -186,10 +186,21 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)
 	return list_refs(&header->references, argc, argv);
 }

+/* create_bundle uses lock_file, delete if write fails */
+static inline void lwrite_or_die(int fd, const void *buf, size_t count, struct lock_file *lock)
+{
+	if (write_in_full(fd, buf, count) != count) {
+		close(fd);
+		rollback_lock_file(lock);
+		die("Unable to write bundle");
+	}
+}
+
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
 	int bundle_fd = -1;
+	static struct lock_file lock;
 	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,17 +209,9 @@ 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));
-
 	/* write signature */
-	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
+	bundle_fd = hold_lock_file_for_update(&lock, path, 1);
+	lwrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature), &lock);

 	/* init revs to list objects for pack-objects later */
 	save_commit_buffer = 0;
@@ -230,7 +233,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	while (fgets(buffer, sizeof(buffer), rls_fout)) {
 		unsigned char sha1[20];
 		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, strlen(buffer));
+			lwrite_or_die(bundle_fd, buffer, strlen(buffer), &lock);
 			if (!get_sha1_hex(buffer + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
@@ -242,13 +245,19 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		}
 	}
 	fclose(rls_fout);
-	if (finish_command(&rls))
+	if (finish_command(&rls)) {
+		close(bundle_fd);
+		rollback_lock_file(&lock);
 		return error("rev-list died");
+	}

 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	if (argc > 1)
+	if (argc > 1) {
+		close(bundle_fd);
+		rollback_lock_file(&lock);
 		return error("unrecognized argument: %s'", argv[1]);
+	}

 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
@@ -307,17 +316,20 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		}

 		ref_count++;
-		write_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);
-		write_or_die(bundle_fd, " ", 1);
-		write_or_die(bundle_fd, ref, strlen(ref));
-		write_or_die(bundle_fd, "\n", 1);
+		lwrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40, &lock);
+		lwrite_or_die(bundle_fd, " ", 1, &lock);
+		lwrite_or_die(bundle_fd, ref, strlen(ref), &lock);
+		lwrite_or_die(bundle_fd, "\n", 1, &lock);
 		free(ref);
 	}
-	if (!ref_count)
+	if (!ref_count) {
+		close(bundle_fd);
+		rollback_lock_file(&lock);
 		die ("Refusing to create empty bundle.");
+	}

 	/* end header */
-	write_or_die(bundle_fd, "\n", 1);
+	lwrite_or_die(bundle_fd, "\n", 1, &lock);

 	/* write pack */
 	argv_pack[0] = "pack-objects";
@@ -339,8 +351,13 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		write(rls.in, sha1_to_hex(object->sha1), 40);
 		write(rls.in, "\n", 1);
 	}
-	if (finish_command(&rls))
+	if (finish_command(&rls)) {
+		close(bundle_fd);
+		rollback_lock_file(&lock);
 		return error ("pack-objects died");
+	}
+	close(bundle_fd);
+	commit_lock_file(&lock);
 	return 0;
 }

--
1.5.3.rc4.79.gb5c7e-dirty

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

[Junio C Hamano]

Mark Levedahl <mdl123@verizon.net> 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 <mdl123@verizon.net>
> ---
>  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;
 }
 

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

[Mark Levedahl]

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