Merge branch 'mh/lockfile-stdio'

* mh/lockfile-stdio:
  commit_packed_refs(): reimplement using fdopen_lock_file()
  dump_marks(): reimplement using fdopen_lock_file()
  fdopen_lock_file(): access a lockfile using stdio

Author: gitster

Documentation/technical/api-lockfile.txt

@@ -42,9 +42,13 @@ The caller:
   of the final destination (e.g. `$GIT_DIR/index`) to
   `hold_lock_file_for_update` or `hold_lock_file_for_append`.
 
-* Writes new content for the destination file by writing to the file
-  descriptor returned by those functions (also available via
-  `lock->fd`).
+* Writes new content for the destination file by either:
+
+  * writing to the file descriptor returned by the `hold_lock_file_*`
+    functions (also available via `lock->fd`).
+
+  * calling `fdopen_lock_file` to get a `FILE` pointer for the open
+    file and writing to the file using stdio.
 
 When finished writing, the caller can:
 
@@ -70,10 +74,10 @@ any uncommitted changes.
 
 If you need to close the file descriptor you obtained from a
 `hold_lock_file_*` function yourself, do so by calling
-`close_lock_file`. You should never call `close(2)` yourself!
-Otherwise the `struct lock_file` structure would still think that the
-file descriptor needs to be closed, and a commit or rollback would
-result in duplicate calls to `close(2)`. Worse yet, if you `close(2)`
+`close_lock_file`. You should never call `close(2)` or `fclose(3)`
+yourself! Otherwise the `struct lock_file` structure would still think
+that the file descriptor needs to be closed, and a commit or rollback
+would result in duplicate calls to `close(2)`. Worse yet, if you close
 and then later open another file descriptor for a completely different
 purpose, then a commit or rollback might close that unrelated file
 descriptor.
@@ -143,6 +147,13 @@ hold_lock_file_for_append::
 	the existing contents of the file (if any) to the lockfile and
 	position its write pointer at the end of the file.
 
+fdopen_lock_file::
+
+	Associate a stdio stream with the lockfile. Return NULL
+	(*without* rolling back the lockfile) on error. The stream is
+	closed automatically when `close_lock_file` is called or when
+	the file is committed or rolled back.
+
 get_locked_file_path::
 
 	Return the path of the file that is locked by the specified
@@ -179,10 +190,11 @@ close_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized with an
 	earlier call to `hold_lock_file_for_update` or
-	`hold_lock_file_for_append`, and close the file descriptor.
-	Return 0 upon success. On failure to `close(2)`, return a
-	negative value and roll back the lock file. Usually
-	`commit_lock_file`, `commit_lock_file_to`, or
+	`hold_lock_file_for_append`. Close the file descriptor (and
+	the file pointer if it has been opened using
+	`fdopen_lock_file`). Return 0 upon success. On failure to
+	`close(2)`, return a negative value and roll back the lock
+	file. Usually `commit_lock_file`, `commit_lock_file_to`, or
 	`rollback_lock_file` should eventually be called if
 	`close_lock_file` succeeds.
 

fast-import.c

@@ -1794,20 +1794,18 @@ static void dump_marks_helper(FILE *f,
 static void dump_marks(void)
 {
 	static struct lock_file mark_lock;
-	int mark_fd;
 	FILE *f;
 
 	if (!export_marks_file)
 		return;
 
-	mark_fd = hold_lock_file_for_update(&mark_lock, export_marks_file, 0);
-	if (mark_fd < 0) {
+	if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {
 		failure |= error("Unable to write marks file %s: %s",
 			export_marks_file, strerror(errno));
 		return;
 	}
 
-	f = fdopen(mark_fd, "w");
+	f = fdopen_lock_file(&mark_lock, "w");
 	if (!f) {
 		int saved_errno = errno;
 		rollback_lock_file(&mark_lock);
@@ -1816,22 +1814,7 @@ static void dump_marks(void)
 		return;
 	}
 
-	/*
-	 * Since the lock file was fdopen()'ed, it should not be close()'ed.
-	 * Assign -1 to the lock file descriptor so that commit_lock_file()
-	 * won't try to close() it.
-	 */
-	mark_lock.fd = -1;
-
 	dump_marks_helper(f, 0, marks);
-	if (ferror(f) || fclose(f)) {
-		int saved_errno = errno;
-		rollback_lock_file(&mark_lock);
-		failure |= error("Unable to write marks file %s: %s",
-			export_marks_file, strerror(saved_errno));
-		return;
-	}
-
 	if (commit_lock_file(&mark_lock)) {
 		failure |= error("Unable to commit marks file %s: %s",
 			export_marks_file, strerror(errno));

lockfile.c

@@ -7,20 +7,29 @@
 
 static struct lock_file *volatile lock_file_list;
 
-static void remove_lock_files(void)
+static void remove_lock_files(int skip_fclose)
 {
 	pid_t me = getpid();
 
 	while (lock_file_list) {
-		if (lock_file_list->owner == me)
+		if (lock_file_list->owner == me) {
+			/* fclose() is not safe to call in a signal handler */
+			if (skip_fclose)
+				lock_file_list->fp = NULL;
 			rollback_lock_file(lock_file_list);
+		}
 		lock_file_list = lock_file_list->next;
 	}
 }
 
+static void remove_lock_files_on_exit(void)
+{
+	remove_lock_files(0);
+}
+
 static void remove_lock_files_on_signal(int signo)
 {
-	remove_lock_files();
+	remove_lock_files(1);
 	sigchain_pop(signo);
 	raise(signo);
 }
@@ -97,7 +106,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	if (!lock_file_list) {
 		/* One-time initialization */
 		sigchain_push_common(remove_lock_files_on_signal);
-		atexit(remove_lock_files);
+		atexit(remove_lock_files_on_exit);
 	}
 
 	if (lk->active)
@@ -106,6 +115,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	if (!lk->on_list) {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
+		lk->fp = NULL;
 		lk->active = 0;
 		lk->owner = 0;
 		strbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);
@@ -217,6 +227,17 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	return fd;
 }
 
+FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
+{
+	if (!lk->active)
+		die("BUG: fdopen_lock_file() called for unlocked object");
+	if (lk->fp)
+		die("BUG: fdopen_lock_file() called twice for file '%s'", lk->filename.buf);
+
+	lk->fp = fdopen(lk->fd, mode);
+	return lk->fp;
+}
+
 char *get_locked_file_path(struct lock_file *lk)
 {
 	if (!lk->active)
@@ -229,17 +250,32 @@ char *get_locked_file_path(struct lock_file *lk)
 int close_lock_file(struct lock_file *lk)
 {
 	int fd = lk->fd;
+	FILE *fp = lk->fp;
+	int err;
 
 	if (fd < 0)
 		return 0;
 
 	lk->fd = -1;
-	if (close(fd)) {
+	if (fp) {
+		lk->fp = NULL;
+
+		/*
+		 * Note: no short-circuiting here; we want to fclose()
+		 * in any case!
+		 */
+		err = ferror(fp) | fclose(fp);
+	} else {
+		err = close(fd);
+	}
+
+	if (err) {
 		int save_errno = errno;
 		rollback_lock_file(lk);
 		errno = save_errno;
 		return -1;
 	}
+
 	return 0;
 }
 

lockfile.h

@@ -34,6 +34,8 @@
  *   - active is set
  *   - filename holds the filename of the lockfile
  *   - fd holds a file descriptor open for writing to the lockfile
+ *   - fp holds a pointer to an open FILE object if and only if
+ *     fdopen_lock_file() has been called on the object
  *   - owner holds the PID of the process that locked the file
  *
  * - Locked, lockfile closed (after successful close_lock_file()).
@@ -56,6 +58,7 @@ struct lock_file {
 	struct lock_file *volatile next;
 	volatile sig_atomic_t active;
 	volatile int fd;
+	FILE *volatile fp;
 	volatile pid_t owner;
 	char on_list;
 	struct strbuf filename;
@@ -74,6 +77,7 @@ extern void unable_to_lock_message(const char *path, int err,
 extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
+extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
 extern char *get_locked_file_path(struct lock_file *);
 extern int commit_lock_file_to(struct lock_file *, const char *path);
 extern int commit_lock_file(struct lock_file *);

refs.c

@@ -2309,16 +2309,13 @@ int commit_packed_refs(void)
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
 
-	out = fdopen(packed_ref_cache->lock->fd, "w");
+	out = fdopen_lock_file(packed_ref_cache->lock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
 	fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
 	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
 				 0, write_packed_entry_fn, out);
-	if (fclose(out))
-		die_errno("write error");
-	packed_ref_cache->lock->fd = -1;
 
 	if (commit_lock_file(packed_ref_cache->lock)) {
 		save_errno = errno;