Improve use of lockfile API

drafnel · gitster · commit 4ed7cd3ab07f · 2008-01-16T15:35:35.000-08:00
Remove remaining double close(2)'s.  i.e. close() before
commit_locked_index() or commit_lock_file().

Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/builtin-add.c b/builtin-add.c
@@ -259,7 +259,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
  finish:
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
-		    close(newfd) || commit_locked_index(&lock_file))
+		    commit_locked_index(&lock_file))
 			die("Unable to write new index file");
 	}
 
diff --git a/builtin-apply.c b/builtin-apply.c
@@ -2921,7 +2921,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 
 	if (update_index) {
 		if (write_cache(newfd, active_cache, active_nr) ||
-		    close(newfd) || commit_locked_index(&lock_file))
+		    commit_locked_index(&lock_file))
 			die("Unable to write new index file");
 	}
 
diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
@@ -246,8 +246,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		 * want to update cache.
 		 */
 		if (state.refresh_cache) {
-			close(newfd); newfd = -1;
 			rollback_lock_file(&lock_file);
+			newfd = -1;
 		}
 		state.refresh_cache = 0;
 	}
@@ -297,7 +297,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 
 	if (0 <= newfd &&
 	    (write_cache(newfd, active_cache, active_nr) ||
-	     close(newfd) || commit_locked_index(&lock_file)))
+	     commit_locked_index(&lock_file)))
 		die("Unable to write new index file");
 	return 0;
 }
diff --git a/builtin-commit.c b/builtin-commit.c
@@ -237,7 +237,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(0, also ? prefix : NULL, pathspec);
 		refresh_cache(REFRESH_QUIET);
-		if (write_cache(fd, active_cache, active_nr))
+		if (write_cache(fd, active_cache, active_nr) ||
+		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
 		commit_style = COMMIT_NORMAL;
 		return index_lock.filename;
@@ -298,7 +299,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	fd = hold_locked_index(&index_lock, 1);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
-	if (write_cache(fd, active_cache, active_nr))
+	if (write_cache(fd, active_cache, active_nr) ||
+	    close_lock_file(&index_lock))
 		die("unable to write new_index file");
 
 	fd = hold_lock_file_for_update(&false_lock,
@@ -308,7 +310,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 
-	if (write_cache(fd, active_cache, active_nr))
+	if (write_cache(fd, active_cache, active_nr) ||
+	    close_lock_file(&false_lock))
 		die("unable to write temporary index file");
 	return false_lock.filename;
 }
diff --git a/builtin-diff.c b/builtin-diff.c
@@ -190,7 +190,7 @@ static void refresh_index_quietly(void)
 	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
 
 	if (active_cache_changed &&
-	    !write_cache(fd, active_cache, active_nr) && !close(fd))
+	    !write_cache(fd, active_cache, active_nr))
 		commit_locked_index(lock_file);
 
 	rollback_lock_file(lock_file);
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
@@ -783,7 +783,6 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 			unlink(shallow);
 			rollback_lock_file(&lock);
 		} else {
-			close(fd);
 			commit_lock_file(&lock);
 		}
 	}
diff --git a/builtin-mv.c b/builtin-mv.c
@@ -264,7 +264,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		if (active_cache_changed) {
 			if (write_cache(newfd, active_cache, active_nr) ||
-			    close(newfd) ||
 			    commit_locked_index(&lock_file))
 				die("Unable to write new index file");
 		}
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
@@ -108,6 +108,12 @@ static int pack_refs(unsigned int flags)
 		die("failed to write ref-pack file");
 	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
 		die("failed to write ref-pack file (%s)", strerror(errno));
+	/*
+	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
+	 * assign -1 to the lock file descriptor so that commit_lock_file()
+	 * won't try to close() it.
+	 */
+	packed.fd = -1;
 	if (commit_lock_file(&packed) < 0)
 		die("unable to overwrite old ref-pack file (%s)", strerror(errno));
 	if (cbdata.flags & PACK_REFS_PRUNE)
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
@@ -283,7 +283,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	}
 
 	if (write_cache(newfd, active_cache, active_nr) ||
-	    close(newfd) || commit_locked_index(&lock_file))
+	    commit_locked_index(&lock_file))
 		die("unable to write new index file");
 	return 0;
 }
diff --git a/builtin-rerere.c b/builtin-rerere.c
@@ -61,9 +61,9 @@ static int write_rr(struct path_list *rr, int out_fd)
 		    write_in_full(out_fd, path, length) != length)
 			die("unable to write rerere record");
 	}
-	if (close(out_fd) != 0)
+	if (commit_lock_file(&write_lock) != 0)
 		die("unable to write rerere record");
-	return commit_lock_file(&write_lock);
+	return 0;
 }
 
 static int handle_file(const char *path,
diff --git a/builtin-reset.c b/builtin-reset.c
@@ -108,7 +108,6 @@ static int update_index_refresh(int fd, struct lock_file *index_lock)
 		return error("Could not read index");
 	result = refresh_cache(0) ? 1 : 0;
 	if (write_cache(fd, active_cache, active_nr) ||
-			close(fd) ||
 			commit_locked_index(index_lock))
 		return error ("Could not refresh index");
 	return result;
diff --git a/builtin-revert.c b/builtin-revert.c
@@ -371,13 +371,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 					i++;
 			}
 		}
-		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
+		if (commit_lock_file(&msg_file) < 0)
 			die ("Error wrapping up %s", defmsg);
 		fprintf(stderr, "Automatic %s failed.%s\n",
 			me, help_msg(commit->object.sha1));
 		exit(1);
 	}
-	if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
+	if (commit_lock_file(&msg_file) < 0)
 		die ("Error wrapping up %s", defmsg);
 	fprintf(stderr, "Finished one %s.\n", me);
 
diff --git a/builtin-rm.c b/builtin-rm.c
@@ -250,7 +250,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
-		    close(newfd) || commit_locked_index(&lock_file))
+		    commit_locked_index(&lock_file))
 			die("Unable to write new index file");
 	}
 
diff --git a/builtin-update-index.c b/builtin-update-index.c
@@ -738,7 +738,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			    get_index_file(), strerror(lock_error));
 		}
 		if (write_cache(newfd, active_cache, active_nr) ||
-		    close(newfd) || commit_locked_index(lock_file))
+		    commit_locked_index(lock_file))
 			die("Unable to write new index file");
 	}
 
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
@@ -35,11 +35,9 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 				      missing_ok, 0) < 0)
 			die("git-write-tree: error building trees");
 		if (0 <= newfd) {
-			if (!write_cache(newfd, active_cache, active_nr)
-					&& !close(newfd)) {
-				commit_lock_file(lock_file);
+			if (!write_cache(newfd, active_cache, active_nr) &&
+			    !commit_lock_file(lock_file))
 				newfd = -1;
-			}
 		}
 		/* Not being able to write is fine -- we are only interested
 		 * in updating the cache-tree part, and if the next caller
@@ -60,8 +58,7 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 		hashcpy(sha1, active_cache_tree->sha1);
 
 	if (0 <= newfd)
-		close(newfd);
-	rollback_lock_file(lock_file);
+		rollback_lock_file(lock_file);
 
 	return 0;
 }
diff --git a/bundle.c b/bundle.c
@@ -317,6 +317,14 @@ int create_bundle(struct bundle_header *header, const char *path,
 	rls.git_cmd = 1;
 	if (start_command(&rls))
 		return error("Could not spawn pack-objects");
+
+	/*
+	 * start_command closed bundle_fd if it was > 1
+	 * so set the lock fd to -1 so commit_lock_file()
+	 * won't fail trying to close it.
+	 */
+	lock.fd = -1;
+
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object *object = revs.pending.objects[i].item;
 		if (object->flags & UNINTERESTING)
@@ -326,10 +334,8 @@ 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;
+
+	return bundle_to_stdout ? close(bundle_fd) : commit_lock_file(&lock);
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd)
diff --git a/config.c b/config.c
@@ -955,14 +955,12 @@ int git_config_set_multivar(const char* key, const char* value,
 		munmap(contents, contents_sz);
 	}
 
-	if (close(fd) || commit_lock_file(lock) < 0) {
+	if (commit_lock_file(lock) < 0) {
 		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
-	/* fd is closed, so don't try to close it below. */
-	fd = -1;
 	/*
 	 * lock is committed, so don't try to roll it back below.
 	 * NOTE: Since lockfile.c keeps a linked list of all created
@@ -973,8 +971,6 @@ int git_config_set_multivar(const char* key, const char* value,
 	ret = 0;
 
 out_free:
-	if (0 <= fd)
-		close(fd);
 	if (lock)
 		rollback_lock_file(lock);
 	free(config_filename);
@@ -1072,7 +1068,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 	}
 	fclose(config_file);
  unlock_and_out:
-	if (close(out_fd) || commit_lock_file(lock) < 0)
+	if (commit_lock_file(lock) < 0)
 			ret = error("Cannot commit config file!");
  out:
 	free(config_filename);
diff --git a/fast-import.c b/fast-import.c
@@ -1546,10 +1546,18 @@ static void dump_marks(void)
 	}
 
 	dump_marks_helper(f, 0, marks);
-	fclose(f);
-	if (commit_lock_file(&mark_lock))
+	if (ferror(f) || fclose(f))
 		failure |= error("Unable to write marks file %s: %s",
 			mark_file, strerror(errno));
+	/*
+	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
+	 * assign -1 to the lock file descriptor so that commit_lock_file()
+	 * won't try to close() it.
+	 */
+	mark_lock.fd = -1;
+	if (commit_lock_file(&mark_lock))
+		failure |= error("Unable to write commit file %s: %s",
+			mark_file, strerror(errno));
 }
 
 static int read_next_command(void)
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -1753,7 +1753,7 @@ int main(int argc, char *argv[])
 
 	if (active_cache_changed &&
 	    (write_cache(index_fd, active_cache, active_nr) ||
-	     close(index_fd) || commit_locked_index(lock)))
+	     commit_locked_index(lock)))
 			die ("unable to write %s", get_index_file());
 
 	return clean ? 0: 1;
diff --git a/refs.c b/refs.c
@@ -864,7 +864,6 @@ static int repack_without_ref(const char *refname)
 			die("too long a refname '%s'", list->name);
 		write_or_die(fd, line, len);
 	}
-	close(fd);
 	return commit_lock_file(&packlock);
 }
 
@@ -1021,12 +1020,9 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 
 void unlock_ref(struct ref_lock *lock)
 {
-	if (lock->lock_fd >= 0) {
-		close(lock->lock_fd);
-		/* Do not free lock->lk -- atexit() still looks at them */
-		if (lock->lk)
-			rollback_lock_file(lock->lk);
-	}
+	/* Do not free lock->lk -- atexit() still looks at them */
+	if (lock->lk)
+		rollback_lock_file(lock->lk);
 	free(lock->ref_name);
 	free(lock->orig_ref_name);
 	free(lock);
@@ -1151,7 +1147,7 @@ int write_ref_sha1(struct ref_lock *lock,
 	}
 	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lock_fd, &term, 1) != 1
-		|| close(lock->lock_fd) < 0) {
+		|| close_lock_file(lock->lk) < 0) {
 		error("Couldn't write %s", lock->lk->filename);
 		unlock_ref(lock);
 		return -1;

Original file line number	Diff line number	Diff line change
`@@ -259,7 +259,7 @@ int cmd_add(int argc, const char *argv, const char prefix)`
`259`	`259`	`finish:`
`260`	`260`	`if (active_cache_changed) {`
`261`	`261`	`if (write_cache(newfd, active_cache, active_nr) \|\|`
`262`		`- close(newfd) \|\| commit_locked_index(&lock_file))`
	`262`	`+ commit_locked_index(&lock_file))`
`263`	`263`	`die("Unable to write new index file");`
`264`	`264`	`}`
`265`	`265`
Original file line number	Diff line number	Diff line change
`@@ -2921,7 +2921,7 @@ int cmd_apply(int argc, const char *argv, const char unused_prefix)`
`2921`	`2921`
`2922`	`2922`	`if (update_index) {`
`2923`	`2923`	`if (write_cache(newfd, active_cache, active_nr) \|\|`
`2924`		`- close(newfd) \|\| commit_locked_index(&lock_file))`
	`2924`	`+ commit_locked_index(&lock_file))`
`2925`	`2925`	`die("Unable to write new index file");`
`2926`	`2926`	`}`
`2927`	`2927`
Original file line number	Diff line number	Diff line change
`@@ -246,8 +246,8 @@ int cmd_checkout_index(int argc, const char *argv, const char prefix)`
`246`	`246`	`* want to update cache.`
`247`	`247`	`*/`
`248`	`248`	`if (state.refresh_cache) {`
`249`		`- close(newfd); newfd = -1;`
`250`	`249`	`rollback_lock_file(&lock_file);`
	`250`	`+ newfd = -1;`
`251`	`251`	`}`
`252`	`252`	`state.refresh_cache = 0;`
`253`	`253`	`}`
`@@ -297,7 +297,7 @@ int cmd_checkout_index(int argc, const char *argv, const char prefix)`
`297`	`297`
`298`	`298`	`if (0 <= newfd &&`
`299`	`299`	`(write_cache(newfd, active_cache, active_nr) \|\|`
`300`		`- close(newfd) \|\| commit_locked_index(&lock_file)))`
	`300`	`+ commit_locked_index(&lock_file)))`
`301`	`301`	`die("Unable to write new index file");`
`302`	`302`	`return 0;`
`303`	`303`	`}`
Original file line number	Diff line number	Diff line change
`@@ -783,7 +783,6 @@ struct ref fetch_pack(struct fetch_pack_args my_args,`
`783`	`783`	`unlink(shallow);`
`784`	`784`	`rollback_lock_file(&lock);`
`785`	`785`	`} else {`
`786`		`- close(fd);`
`787`	`786`	`commit_lock_file(&lock);`
`788`	`787`	`}`
`789`	`788`	`}`
Original file line number	Diff line number	Diff line change
`@@ -264,7 +264,6 @@ int cmd_mv(int argc, const char *argv, const char prefix)`
`264`	`264`
`265`	`265`	`if (active_cache_changed) {`
`266`	`266`	`if (write_cache(newfd, active_cache, active_nr) \|\|`
`267`		`- close(newfd) \|\|`
`268`	`267`	`commit_locked_index(&lock_file))`
`269`	`268`	`die("Unable to write new index file");`
`270`	`269`	`}`
Original file line number	Diff line number	Diff line change
`@@ -283,7 +283,7 @@ int cmd_read_tree(int argc, const char *argv, const char unused_prefix)`
`283`	`283`	`}`
`284`	`284`
`285`	`285`	`if (write_cache(newfd, active_cache, active_nr) \|\|`
`286`		`- close(newfd) \|\| commit_locked_index(&lock_file))`
	`286`	`+ commit_locked_index(&lock_file))`
`287`	`287`	`die("unable to write new index file");`
`288`	`288`	`return 0;`
`289`	`289`	`}`
Original file line number	Diff line number	Diff line change
`@@ -61,9 +61,9 @@ static int write_rr(struct path_list *rr, int out_fd)`
`61`	`61`	`write_in_full(out_fd, path, length) != length)`
`62`	`62`	`die("unable to write rerere record");`
`63`	`63`	`}`
`64`		`- if (close(out_fd) != 0)`
	`64`	`+ if (commit_lock_file(&write_lock) != 0)`
`65`	`65`	`die("unable to write rerere record");`
`66`		`- return commit_lock_file(&write_lock);`
	`66`	`+ return 0;`
`67`	`67`	`}`
`68`	`68`
`69`	`69`	`static int handle_file(const char *path,`