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

Re: [PATCH v2 05/12] checkout-index: simplify locking logic

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 6, 2017, 01:21 UTC
Message-ID
<xmqqpoa1ysu1.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<fe78be885bb4f5a4e021abdf80097cd5c497746f.1507228170.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 16 quoted lines
> `newfd` starts out negative. If we then take the lock, `newfd` will
> become non-negative. We later check for exactly that property before
> calling `write_locked_index()`. That is, we are simply using `newfd` as
> a boolean to keep track of whether we took the lock or not. (We always
> use `newfd` and `lock_file` together, so they really are mirroring each
> other.)
>
> Drop `newfd` and check with `is_lock_file_locked()` instead. While at
> it, move the `static struct lock_file` into `cmd_checkout_index()` and
> make it non-static. It is only used in this function, and after
> 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we
> can have lockfiles on the stack.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> v2: New patch.
Well reasoned and explained.  Thanks.
Show 44 quoted lines
>
>  builtin/checkout-index.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 39c8be05d..b0e78b819 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = {
>  	NULL
>  };
>  
> -static struct lock_file lock_file;
> -
>  static int option_parse_stage(const struct option *opt,
>  			      const char *arg, int unset)
>  {
> @@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt,
>  int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
> -	int newfd = -1;
> +	struct lock_file lock_file = LOCK_INIT;
>  	int all = 0;
>  	int read_from_stdin = 0;
>  	int prefix_length;
> @@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  	if (index_opt && !state.base_dir_len && !to_tempfile) {
>  		state.refresh_cache = 1;
>  		state.istate = &the_index;
> -		newfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
> +		hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
>  	}
>  
>  	/* Check out named files first */
> @@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  	if (all)
>  		checkout_all(prefix, prefix_length);
>  
> -	if (0 <= newfd &&
> +	if (is_lock_file_locked(&lock_file) &&
>  	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>  		die("Unable to write new index file");
>  	return 0;
Previous: Martin ÅgrenNext: Martin Ågren
Message 42 of 69 in “various lockfile-leaks and -fixes”
  1. 00/11 various lockfile-leaks and -fixesMartin Ågren, Oct 1, 2017
  2. 01/11 sha1_file: do not leak `lock_file`Martin Ågren, Oct 1, 2017
  3. Jeff KingOct 2, 2017
  4. Martin ÅgrenOct 2, 2017
  5. 02/11 treewide: prefer lockfiles on the stackMartin Ågren, Oct 1, 2017
  6. Junio C HamanoOct 2, 2017
  7. Martin ÅgrenOct 2, 2017
  8. Jeff KingOct 2, 2017
  9. 03/11 lockfile: fix documentation on `close_lock_file_gently()`Martin Ågren, Oct 1, 2017
  10. Jeff KingOct 2, 2017
  11. 04/11 tempfile: fix documentation on `delete_tempfile()`Martin Ågren, Oct 1, 2017
  12. Jeff KingOct 2, 2017
  13. 05/11 cache-tree: simplify locking logicMartin Ågren, Oct 1, 2017
  14. Junio C HamanoOct 2, 2017
  15. Jeff KingOct 2, 2017
  16. 06/11 apply: move lockfile into `apply_state`Martin Ågren, Oct 1, 2017
  17. Jeff KingOct 2, 2017
  18. 07/11 apply: remove `newfd` from `struct apply_state`Martin Ågren, Oct 1, 2017
  19. Jeff KingOct 2, 2017
  20. 08/11 cache.h: document `write_locked_index()`Martin Ågren, Oct 1, 2017
  21. 09/11 read-cache: require flags for `write_locked_index()`Martin Ågren, Oct 1, 2017
  22. Junio C HamanoOct 2, 2017
  23. Martin ÅgrenOct 2, 2017
  24. Martin ÅgrenOct 2, 2017
  25. Jeff KingOct 2, 2017
  26. 10/11 read-cache: don't leave dangling pointer in `do_write_index()`Martin Ågren, Oct 1, 2017
  27. Jeff KingOct 2, 2017
  28. Jeff KingOct 2, 2017
  29. 11/11 read-cache: roll back lock on error with `COMMIT_LOCK`Martin Ågren, Oct 1, 2017
  30. Junio C HamanoOct 2, 2017
  31. Junio C HamanoOct 2, 2017
  32. Jeff KingOct 2, 2017
  33. Junio C HamanoOct 2, 2017
  34. Martin ÅgrenOct 2, 2017
  35. Junio C HamanoOct 3, 2017
  36. 00/12 Re: various lockfile-leaks and -fixesMartin Ågren, Oct 5, 2017
  37. 01/12 sha1_file: do not leak `lock_file`Martin Ågren, Oct 5, 2017
  38. Junio C HamanoOct 6, 2017
  39. 02/12 treewide: prefer lockfiles on the stackMartin Ågren, Oct 5, 2017
  40. 03/12 lockfile: fix documentation on `close_lock_file_gently()`Martin Ågren, Oct 5, 2017
  41. 05/12 checkout-index: simplify locking logicMartin Ågren, Oct 5, 2017
  42. Junio C HamanoOct 6, 2017
  43. 06/12 cache-tree: simplify locking logicMartin Ågren, Oct 5, 2017
  44. 07/12 apply: move lockfile into `apply_state`Martin Ågren, Oct 5, 2017
  45. 08/12 apply: remove `newfd` from `struct apply_state`Martin Ågren, Oct 5, 2017
  46. 10/12 read-cache: drop explicit `CLOSE_LOCK`-flagMartin Ågren, Oct 5, 2017
  47. Junio C HamanoOct 6, 2017
  48. Martin ÅgrenOct 6, 2017
  49. 11/12 read-cache: leave lock in right state in `write_locked_index()`Martin Ågren, Oct 5, 2017
  50. Junio C HamanoOct 6, 2017
  51. Martin ÅgrenOct 6, 2017
  52. Junio C HamanoOct 6, 2017
  53. Martin ÅgrenOct 6, 2017
  54. 00/12 Re: various lockfile-leaks and -fixesMartin Ågren, Oct 6, 2017
  55. 01/12 sha1_file: do not leak `lock_file`Martin Ågren, Oct 6, 2017
  56. 02/12 treewide: prefer lockfiles on the stackMartin Ågren, Oct 6, 2017
  57. 04/12 tempfile: fix documentation on `delete_tempfile()`Martin Ågren, Oct 6, 2017
  58. 06/12 cache-tree: simplify locking logicMartin Ågren, Oct 6, 2017
  59. 05/12 checkout-index: simplify locking logicMartin Ågren, Oct 6, 2017
  60. 08/12 apply: remove `newfd` from `struct apply_state`Martin Ågren, Oct 6, 2017
  61. 09/12 cache.h: document `write_locked_index()`Martin Ågren, Oct 6, 2017
  62. 10/12 read-cache: drop explicit `CLOSE_LOCK`-flagMartin Ågren, Oct 6, 2017
  63. 12/12 read_cache: roll back lock in `update_index_if_able()`Martin Ågren, Oct 6, 2017
  64. 11/12 read-cache: leave lock in right state in `write_locked_index()`Martin Ågren, Oct 6, 2017
  65. 03/12 lockfile: fix documentation on `close_lock_file_gently()`Martin Ågren, Oct 6, 2017
  66. 07/12 apply: move lockfile into `apply_state`Martin Ågren, Oct 6, 2017
  67. 09/12 cache.h: document `write_locked_index()`Martin Ågren, Oct 5, 2017
  68. 12/12 read_cache: roll back lock in `update_index_if_able()`Martin Ågren, Oct 5, 2017
  69. 04/12 tempfile: fix documentation on `delete_tempfile()`Martin Ågren, Oct 5, 2017

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.