[PATCH 09/11] read-cache: require flags for `write_locked_index()`
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Oct 1, 2017, 14:56 UTC
- Message-ID
- <d912e33a1395ff25c1496715d0a537858daa885a.1506862824.git.martin.agren@gmail.com>
- In-Reply-To
- <cover.1506862824.git.martin.agren@gmail.com>
`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).
So let's punt on the restructuring. Instead, require that one of the flags is set. Adjust documentation and the assert we already have for checking that we don't have too many flags. Add a macro `HAS_SINGLE_BIT` (inspired by `HAS_MULTI_BITS`) to simplify this check and similar checks in the future.
When (if) we need "write the index and leave the lockfile open", we can revisit this.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- cache.h | 4 ++-- git-compat-util.h | 7 ++++++- read-cache.c | 12 ++++++------ 3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h index 4605e8228..32aa8afdf 100644 --- a/cache.h +++ b/cache.h @@ -607,8 +607,8 @@ extern int read_index_unmerged(struct index_state *); #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. The flags must + * contain precisely one of `COMMIT_LOCK` and `CLOSE_LOCK`. * * Unless a split index is in use, write the index into the lock-file, * then commit or close it, as indicated by `flags`. diff --git a/git-compat-util.h b/git-compat-util.h index cedad4d58..fa8a5a95c 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -111,7 +111,12 @@ #endif #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (bitsizeof(x) - (bits)))) -#define HAS_MULTI_BITS(i) ((i) & ((i) - 1)) /* checks if an integer has more than 1 bit set */ + +/* Checks if an integer has more than 1 bit set. */ +#define HAS_MULTI_BITS(i) ((i) & ((i) - 1)) + +/* Checks if an integer has precisely 1 bit set. */ +#define HAS_SINGLE_BIT(i) ((i) && !HAS_MULTI_BITS(i)) #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d)) diff --git a/read-cache.c b/read-cache.c index 65f4fe837..1ec2e8304 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2343,14 +2343,14 @@ 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)); + assert(HAS_SINGLE_BIT(flags & (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; + /* + * Otherwise it's CLOSE_LOCK. The lockfile already happens + * to have been closed, but let's be specific. + */ + return close_lock_file_gently(lock); } static int write_split_index(struct index_state *istate,
-- 2.14.1.727.g9ddaf86