Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

[Martin Ågren]

On 6 October 2017 at 03:39, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index 65f4fe837..1c917eba9 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state 
>> *istate, struct lock_file *l
>>   int ret = do_write_index(istate, lock->tempfile, 0);
>>   if (ret)
>>   return ret;
>> - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
>> -(COMMIT_LOCK | CLOSE_LOCK));
>>   if (flags & COMMIT_LOCK)
>>   return commit_locked_index(lock);
>> - else if (flags & CLOSE_LOCK)
>> - return close_lock_file_gently(lock);
>> - else
>> - return ret;
>> + /*
>> +  * The lockfile already happens to have
>> +  * been closed, but let's be specific.
>> +  */
>> + return close_lock_file_gently(lock);
>
> "already happens to have been" is quite a mouthful, and is not quite
> truthful, as we do not foresee ever wanting to change that (because
> of that stat(2) issue you mentioned).  It might be better to declare
> that do_write_index() closes the lockfile after successfully writing
> the data out to it.  I dunno if that reasoning is strong enough to
> remove this (extra) close, though.
>
> When any of the ce_write() calls in do_write_index() fails, the
> function returns -1 without hitting the close/stat (obviously).
> Somebody very high in the callchain (e.g. write_locked_index())
> would clean it up by calling rollback_lock_file() eventually, so
> that would not be a problem ;-)

When I wrote this, I was too stuck in the "it gets closed accidentally"
world view. It would indeed be cleaner to specify that the close happens
in `do_write_index()`. As you say, because of the stat-ing, we simply
have to close.

It's still an implementation detail that closing the temporary file is
the same as closing the lock. We might want to refactor to hand over the
lock instead of its tempfile. Except the other caller has no suitable
lock, only a temporary file. I guess that caller could use a lock
instead, but it feels like the wrong solution to the wrong problem.

I'm sure that something could be done here to improve the cleanliness.
For this series, I think I'll document better that `do_write_index()`
closes the temporary file on success, that this might mean that it
actually closes a *lock*file, but that the latter should not be relied
upon. I'll get to this later today.

Thanks.

Martin

Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

[Junio C Hamano]

Martin Ågren  writes:

> diff --git a/read-cache.c b/read-cache.c
> index 65f4fe837..1c917eba9 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state 
> *istate, struct lock_file *l
>   int ret = do_write_index(istate, lock->tempfile, 0);
>   if (ret)
>   return ret;
> - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
> -(COMMIT_LOCK | CLOSE_LOCK));
>   if (flags & COMMIT_LOCK)
>   return commit_locked_index(lock);
> - else if (flags & CLOSE_LOCK)
> - return close_lock_file_gently(lock);
> - else
> - return ret;
> + /*
> +  * The lockfile already happens to have
> +  * been closed, but let's be specific.
> +  */
> + return close_lock_file_gently(lock);

"already happens to have been" is quite a mouthful, and is not quite
truthful, as we do not foresee ever wanting to change that (because
of that stat(2) issue you mentioned).  It might be better to declare
that do_write_index() closes the lockfile after successfully writing
the data out to it.  I dunno if that reasoning is strong enough to
remove this (extra) close, though.

When any of the ce_write() calls in do_write_index() fails, the
function returns -1 without hitting the close/stat (obviously).
Somebody very high in the callchain (e.g. write_locked_index())
would clean it up by calling rollback_lock_file() eventually, so
that would not be a problem ;-)

[PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

[Martin Ågren]

`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.
At most one is allowed. But it is also possible to use no flag, i.e.,
`0`. But when `write_locked_index()` calls `do_write_index()`, the
temporary file, a.k.a. the lockfile, will be closed. So passing `0` is
effectively the same as `CLOSE_LOCK`, which seems like a bug.

We might feel tempted to restructure the code in order to close the file
later, or conditionally. It also feels a bit unfortunate that we simply
"happen" to close the lock by way of an implementation detail of
lockfiles. But note that we need to close the temporary file before
`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close
index.lock in do_write_index, 2017-04-26).

Drop `CLOSE_LOCK` and make it explicit that `write_locked_index()`
always closes the lock. Whether it is also committed is governed by the
remaining flag, `COMMIT_LOCK`.

This means we neither have nor suggest that we have a mode to write the
index and leave the file open. Whatever extra contents we might
eventually want to write, we should probably write it from within
`write_locked_index()` itself anyway.

Signed-off-by: Martin Ågren 
---
v2: Drop `CLOSE_LOCK` altogether instead of requiring precisely one of
the flags to be set.

 builtin/commit.c | 10 +-
 cache.h  |  5 ++---
 read-cache.c | 11 +--
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0f8ddb686..32dc2101f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 
refresh_cache_or_die(refresh_flags);
 
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to create temporary index"));
 
old_index_env = getenv(INDEX_ENVIRONMENT);
@@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
if (reopen_lock_file(_lock) < 0)
die(_("unable to write index file"));
-   if (write_locked_index(_index, _lock, 
CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to update temporary index"));
} else
warning(_("Failed to update main cache tree"));
@@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_files_to_cache(also ? prefix : NULL, , 0);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write new_index file"));
commit_style = COMMIT_NORMAL;
ret = get_lock_file_path(_lock);
@@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_remove_files();
refresh_cache(REFRESH_QUIET);
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write new_index file"));
 
hold_lock_file_for_update(_lock,
@@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_remove_files();
refresh_cache(REFRESH_QUIET);
 
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write temporary index file"));
 
discard_cache();
diff --git a/cache.h b/cache.h
index e9d9556e3..21a6856c5 100644
--- a/cache.h
+++ b/cache.h
@@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
-#define CLOSE_LOCK (1 << 1)
 
 /*
- * Write the index while holding an already-taken lock. The flags may
- * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.
+ * Write the index while holding an already-taken lock. Close the lock,
+ * and if `COMMIT_LOCK` is given, commit it.
  *
  * Unless a split index is in use, write the index into the lockfile.
  *
diff --git a/read-cache.c b/read-cache.c
index 65f4fe837..1c917eba9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state 
*istate, struct lock_file *l
int ret = do_write_index(istate, lock->tempfile, 0);
if (ret)
return ret;
-   assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
-