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 <gitster@xxxxxxxxx>
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> ---
> 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.
[...]
> --- 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
[...]
> @@ -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. :)
[...]
> @@ -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@xxxxxxxxx>
Thanks.
