Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 30, 2020, 03:11 UTC
- Message-ID
- <20200430031138.GC115238@google.com>
- In-Reply-To
- <c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com>
Hi,
Taylor Blau wrote:
Show 22 quoted lines
> 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 <gitster@pobox.com> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > 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.
[...]
Show 15 quoted lines
> --- 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?
[...]
> --- a/shallow.c > +++ b/shallow.c
[...]
Show 16 quoted lines
> @@ -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. :)
[...]
Show 24 quoted lines
> @@ -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 <jrnieder@gmail.com>
Thanks.