git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v6 33/39] Extract a function commit_lock_file_to()

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Sep 26, 2014, 10:08 UTC
Message-ID
<1411726119-31598-34-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1411726119-31598-1-git-send-email-mhagger@alum.mit.edu>

commit_locked_index(), when writing to an alternate index file, duplicates (poorly) the code in commit_lock_file(). And anyway, it shouldn't have to know so much about the internal workings of lockfile objects. So extract a new function commit_lock_file_to() that does the work common to the two functions, and call it from both commit_lock_file() and commit_locked_index().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 36 ++++++++++++++++------------
 cache.h                                  |  1 +
 lockfile.c                               | 40 +++++++++++++++++++++-----------
 read-cache.c                             | 13 +++--------
 4 files changed, 51 insertions(+), 39 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index d7882df..0b8db64 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -48,14 +48,14 @@ The caller:
 When finished writing, the caller can:
 
 * Close the file descriptor and rename the lockfile to its final
-  destination by calling `commit_lock_file`.
+  destination by calling `commit_lock_file` or `commit_lock_file_to`.
 
 * Close the file descriptor and remove the lockfile by calling
   `rollback_lock_file`.
 
 * Close the file descriptor without removing or renaming the lockfile
-  by calling `close_lock_file`, and later call `commit_lock_file` or
-  `rollback_lock_file`.
+  by calling `close_lock_file`, and later call `commit_lock_file`,
+  `commit_lock_file_to`, or `rollback_lock_file`.
 
 At this point, the `lock_file` object must not be freed or altered,
 because it is still registered in the `lock_file_list`. However, it
@@ -63,20 +63,19 @@ may be reused; just pass it to another call of
 `hold_lock_file_for_update` or `hold_lock_file_for_append`.
 
 If the program exits before you have called one of `commit_lock_file`,
-`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler
-will close and remove the lockfile, essentially rolling back any
-uncommitted changes.
+`commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an
+`atexit(3)` handler will close and remove the lockfile, essentially
+rolling back 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 later call to
-`commit_lock_file` or `rollback_lock_file` or program exit would
+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)`
 and then later open another file descriptor for a completely different
-purpose, then a call to `commit_lock_file` or `rollback_lock_file`
-might close that unrelated file descriptor.
+purpose, then a commit or rollback might close that unrelated file
+descriptor.
 
 
 Error handling
@@ -99,9 +98,9 @@ unable_to_lock_die::
 
 	Emit an appropriate error message and `die()`.
 
-Similarly, `commit_lock_file` and `close_lock_file` return 0 on
-success. On failure they set `errno` appropriately, do their best to
-roll back the lockfile, and return -1.
+Similarly, `commit_lock_file`, `commit_lock_file_to`, and
+`close_lock_file` return 0 on success. On failure they set `errno`
+appropriately, do their best to roll back the lockfile, and return -1.
 
 
 Flags
@@ -155,6 +154,12 @@ commit_lock_file::
 	`commit_lock_file` for a `lock_file` object that is not
 	currently locked.
 
+commit_lock_file_to::
+
+	Like `commit_lock_file()`, except that it takes an explicit
+	`path` argument to which the lockfile should be renamed. The
+	`path` must be on the same filesystem as the lock file.
+
 rollback_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized with an
@@ -171,5 +176,6 @@ close_lock_file::
 	`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` or `rollback_lock_file` should eventually
-	be called if `close_lock_file` succeeds.
+	`commit_lock_file`, `commit_lock_file_to`, or
+	`rollback_lock_file` should eventually be called if
+	`close_lock_file` succeeds.
diff --git a/cache.h b/cache.h
index 433fae5..30883b3 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,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 int commit_lock_file_to(struct lock_file *, const char *path);
 extern int commit_lock_file(struct lock_file *);
 extern int reopen_lock_file(struct lock_file *);
 extern void update_index_if_able(struct index_state *, struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 56ad7e8..cf7f4d0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -43,9 +43,9 @@
  *   Same as the previous state, except that the lockfile is closed
  *   and fd is -1.
  *
- * - Unlocked (after commit_lock_file(), rollback_lock_file(), a
- *   failed attempt to lock, or a failed close_lock_file()).  In this
- *   state:
+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),
+ *   rollback_lock_file(), a failed attempt to lock, or a failed
+ *   close_lock_file()).  In this state:
  *   - active is unset
  *   - filename is empty (usually, though there are transitory
  *     states in which this condition doesn't hold). Client code should
@@ -284,23 +284,15 @@ int reopen_lock_file(struct lock_file *lk)
 	return lk->fd;
 }
 
-int commit_lock_file(struct lock_file *lk)
+int commit_lock_file_to(struct lock_file *lk, const char *path)
 {
-	static struct strbuf result_file = STRBUF_INIT;
-	int err;
-
 	if (!lk->active)
-		die("BUG: attempt to commit unlocked object");
+		die("BUG: attempt to commit unlocked object to \"%s\"", path);
 
 	if (close_lock_file(lk))
 		return -1;
 
-	/* remove ".lock": */
-	strbuf_add(&result_file, lk->filename.buf,
-		   lk->filename.len - LOCK_SUFFIX_LEN);
-	err = rename(lk->filename.buf, result_file.buf);
-	strbuf_reset(&result_file);
-	if (err) {
+	if (rename(lk->filename.buf, path)) {
 		int save_errno = errno;
 		rollback_lock_file(lk);
 		errno = save_errno;
@@ -312,6 +304,26 @@ int commit_lock_file(struct lock_file *lk)
 	return 0;
 }
 
+int commit_lock_file(struct lock_file *lk)
+{
+	static struct strbuf result_file = STRBUF_INIT;
+	int err;
+
+	if (!lk->active)
+		die("BUG: attempt to commit unlocked object");
+
+	if (lk->filename.len <= LOCK_SUFFIX_LEN ||
+	    strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))
+		die("BUG: lockfile filename corrupt");
+
+	/* remove ".lock": */
+	strbuf_add(&result_file, lk->filename.buf,
+		   lk->filename.len - LOCK_SUFFIX_LEN);
+	err = commit_lock_file_to(lk, result_file.buf);
+	strbuf_reset(&result_file);
+	return err;
+}
+
 int hold_locked_index(struct lock_file *lk, int die_on_error)
 {
 	return hold_lock_file_for_update(lk, get_index_file(),
diff --git a/read-cache.c b/read-cache.c
index 91bf876..e887e23 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2041,17 +2041,10 @@ void set_alternate_index_output(const char *name)
 
 static int commit_locked_index(struct lock_file *lk)
 {
-	if (alternate_index_output) {
-		if (close_lock_file(lk))
-			return -1;
-		if (rename(lk->filename.buf, alternate_index_output))
-			return -1;
-		lk->active = 0;
-		strbuf_reset(&lk->filename);
-		return 0;
-	} else {
+	if (alternate_index_output)
+		return commit_lock_file_to(lk, alternate_index_output);
+	else
 		return commit_lock_file(lk);
-	}
 }
 
 static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,

-- 
2.1.0
Previous: Michael HaggertyNext: Michael Haggerty
Message 46 of 55 in “Lockfile correctness and refactoring”
  1. 00/39 Lockfile correctness and refactoringMichael Haggerty, Sep 26, 2014
  2. 01/39 unable_to_lock_die(): rename function from unable_to_lock_index_die()Michael Haggerty, Sep 26, 2014
  3. 02/39 api-lockfile: revise and expand the documentationMichael Haggerty, Sep 26, 2014
  4. Junio C HamanoSep 26, 2014
  5. Michael HaggertySep 30, 2014
  6. Junio C HamanoSep 30, 2014
  7. Michael HaggertyOct 1, 2014
  8. Junio C HamanoSep 26, 2014
  9. Michael HaggertySep 30, 2014
  10. Jeff KingSep 30, 2014
  11. Junio C HamanoSep 30, 2014
  12. Michael HaggertyOct 1, 2014
  13. 03/39 close_lock_file(): exit (successfully) if file is already closedMichael Haggerty, Sep 26, 2014
  14. 04/39 rollback_lock_file(): do not clear filename redundantlyMichael Haggerty, Sep 26, 2014
  15. 05/39 rollback_lock_file(): exit early if lock is not activeMichael Haggerty, Sep 26, 2014
  16. 06/39 rollback_lock_file(): set fd to -1Michael Haggerty, Sep 26, 2014
  17. 07/39 lockfile: unlock file if lockfile permissions cannot be adjustedMichael Haggerty, Sep 26, 2014
  18. 08/39 hold_lock_file_for_append(): release lock on errorsMichael Haggerty, Sep 26, 2014
  19. 09/39 lock_file(): always initialize and register lock_file objectMichael Haggerty, Sep 26, 2014
  20. 10/39 lockfile.c: document the various states of lock_file objectsMichael Haggerty, Sep 26, 2014
  21. 11/39 cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LENMichael Haggerty, Sep 26, 2014
  22. 12/39 delete_ref_loose(): don't muck around in the lock_file's filenameMichael Haggerty, Sep 26, 2014
  23. 13/39 prepare_index(): declare return value to be (const char *)Michael Haggerty, Sep 26, 2014
  24. 14/39 write_packed_entry_fn(): convert cb_data into a (const int *)Michael Haggerty, Sep 26, 2014
  25. 15/39 lock_file(): exit early if lockfile cannot be openedMichael Haggerty, Sep 26, 2014
  26. 16/39 remove_lock_file(): call rollback_lock_file()Michael Haggerty, Sep 26, 2014
  27. 17/39 commit_lock_file(): inline temporary variableMichael Haggerty, Sep 26, 2014
  28. 18/39 commit_lock_file(): die() if called for unlocked lockfile objectMichael Haggerty, Sep 26, 2014
  29. 19/39 close_lock_file(): if close fails, roll backMichael Haggerty, Sep 26, 2014
  30. 20/39 commit_lock_file(): rollback lock file on failure to renameMichael Haggerty, Sep 26, 2014
  31. 21/39 api-lockfile: document edge casesMichael Haggerty, Sep 26, 2014
  32. 22/39 dump_marks(): remove a redundant call to rollback_lock_file()Michael Haggerty, Sep 26, 2014
  33. 23/39 git_config_set_multivar_in_file(): avoid call to rollback_lock_file()Michael Haggerty, Sep 26, 2014
  34. 24/39 lockfile: avoid transitory invalid statesMichael Haggerty, Sep 26, 2014
  35. 25/39 struct lock_file: declare some fields volatileMichael Haggerty, Sep 26, 2014
  36. 26/39 try_merge_strategy(): remove redundant lock_file allocationMichael Haggerty, Sep 26, 2014
  37. 27/39 try_merge_strategy(): use a statically-allocated lock_file objectMichael Haggerty, Sep 26, 2014
  38. Junio C HamanoSep 26, 2014
  39. Michael HaggertySep 30, 2014
  40. Junio C HamanoSep 30, 2014
  41. 28/39 commit_lock_file(): use a strbuf to manage temporary spaceMichael Haggerty, Sep 26, 2014
  42. 29/39 Change lock_file::filename into a strbufMichael Haggerty, Sep 26, 2014
  43. 30/39 resolve_symlink(): use a strbuf for internal scratch spaceMichael Haggerty, Sep 26, 2014
  44. 31/39 resolve_symlink(): take a strbuf parameterMichael Haggerty, Sep 26, 2014
  45. 32/39 trim_last_path_component(): replace last_path_elm()Michael Haggerty, Sep 26, 2014
  46. 33/39 Extract a function commit_lock_file_to()Michael Haggerty, Sep 26, 2014
  47. 34/39 Rename LOCK_NODEREF to LOCK_NO_DEREFMichael Haggerty, Sep 26, 2014
  48. 35/39 lockfile.c: rename static functionsMichael Haggerty, Sep 26, 2014
  49. 36/39 get_locked_file_path(): new functionMichael Haggerty, Sep 26, 2014
  50. 37/39 hold_lock_file_for_append(): restore errno before returningMichael Haggerty, Sep 26, 2014
  51. 38/39 Move read_index() definition to read-cache.cMichael Haggerty, Sep 26, 2014
  52. 39/39 lockfile.h: extract new header file for the functions in lockfile.cMichael Haggerty, Sep 26, 2014
  53. Junio C HamanoSep 26, 2014
  54. Michael HaggertySep 30, 2014
  55. Junio C HamanoSep 26, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.