Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety
From: Jonathan Nieder <hidden>
Date: 2020-04-30 03:11:43
Hi, Taylor Blau wrote:
In previous patches, the functions 'commit_shallow_file' and 'rollback_shallow_file' were introduced to reset the shallowness validity checks on a repository after potentially modifying '.git/shallow'. These functions can be made safer by wrapping the 'struct lockfile *' in a new type, 'shallow_lock', so that they cannot be called with a raw lock (and potentially misused by other code that happens to possess a lockfile, but has nothing to do with shallowness). This patch introduces that type as a thin wrapper around 'struct lockfile', and updates the two aforementioned functions and their callers to use it. Suggested-by: Junio C Hamano <redacted> Signed-off-by: Taylor Blau <redacted> --- builtin/receive-pack.c | 2 +- fetch-pack.c | 2 +- shallow.c | 22 +++++++++++----------- shallow.h | 16 +++++++++++++--- 4 files changed, 26 insertions(+), 16 deletions(-)
Yay! Thanks for indulging the suggestion. [...]
quoted hunk ↗ jump to hunk
--- a/shallow.h +++ b/shallow.h@@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over int register_shallow(struct repository *r, const struct object_id *oid); int unregister_shallow(const struct object_id *oid); int is_repository_shallow(struct repository *r); + +/* + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict + * which locks can be used with '{commit,rollback}_shallow_file()'. + */ +struct shallow_lock { + struct lock_file lk; +}; +#define SHALLOW_LOCK_INIT { LOCK_INIT }
I think I disagree with Eric here: it's useful to have a comment here to describe the purpose of the struct (i.e., the "why" as opposed to the "what"). I wonder if we can go further, though --- when using a shallow_lock, how should I think of it as a caller? In some sense, the use of 'struct lock_file' is an implementation detail, so we could say: /* * Lock for updating the $GIT_DIR/shallow file. * * Use `commit_shallow_file()` to commit an update, or * `rollback_shallow_file()` to roll it back. In either case, * any in-memory cached information about which commits are * shallow will be appropriately invalidated so that future * operations reflect the new state. */ What do you think? [...]
quoted hunk ↗ jump to hunk
--- a/shallow.c +++ b/shallow.c
[...]
quoted hunk ↗ jump to hunk
@@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra) return ""; } -void setup_alternate_shallow(struct lock_file *shallow_lock, +void setup_alternate_shallow(struct shallow_lock *shallow_lock, const char **alternate_shallow_file, const struct oid_array *extra) { struct strbuf sb = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(shallow_lock, + fd = hold_lock_file_for_update(&shallow_lock->lk, git_path_shallow(the_repository), LOCK_DIE_ON_ERROR);
This is peeking into the underlying lock_file, so I should ask myself whether it's hinting at some missing function in the shallow_lock API. My feeling is "no": setup_alternate_shallow is itself that function. :) [...]
quoted hunk ↗ jump to hunk
@@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd) */ void prune_shallow(unsigned options) { - struct lock_file shallow_lock = LOCK_INIT; + struct shallow_lock shallow_lock = SHALLOW_LOCK_INIT; struct strbuf sb = STRBUF_INIT; unsigned flags = SEEN_ONLY; int fd;@@ -428,14 +428,14 @@ void prune_shallow(unsigned options) strbuf_release(&sb); return; } - fd = hold_lock_file_for_update(&shallow_lock, + fd = hold_lock_file_for_update(&shallow_lock.lk, git_path_shallow(the_repository), LOCK_DIE_ON_ERROR); check_shallow_file_for_update(the_repository); if (write_shallow_commits_1(&sb, 0, NULL, flags)) { if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", - get_lock_file_path(&shallow_lock)); + get_lock_file_path(&shallow_lock.lk)); commit_shallow_file(the_repository, &shallow_lock);
There's no obvious helper to extract here either, so this looks good to me. With whatever tweaks based on Eric's and Jonathan's reviews seem appropriate, Reviewed-by: Jonathan Nieder <redacted> Thanks.