| Message ID | c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shallow: extract a header file | expand |
On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau <me@ttaylorr.com> wrote: > [...] > This patch introduces that type as a thin wrapper around 'struct > lockfile', and updates the two aforementioned functions and their > callers to use it. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > diff --git a/shallow.h b/shallow.h > @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over > +/* > + * 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; > +}; The documentation comment for 'shallow_lock' may help newcomers to C but probably doesn't add much value for seasoned programmers. If this is the sort of idiom we want to introduce (or exists already in this codebase) -- declaring a specific C type to avoid accidental use of an unrelated lock -- then it's probably better documented in CodingGuidelines rather than repeating it at each point in the code which employs the idiom.
On Wed, Apr 29, 2020 at 07:03:48PM -0400, Eric Sunshine wrote: > On Wed, Apr 29, 2020 at 6:39 PM Taylor Blau <me@ttaylorr.com> wrote: > > [...] > > This patch introduces that type as a thin wrapper around 'struct > > lockfile', and updates the two aforementioned functions and their > > callers to use it. > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > diff --git a/shallow.h b/shallow.h > > @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over > > +/* > > + * 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; > > +}; > > The documentation comment for 'shallow_lock' may help newcomers to C > but probably doesn't add much value for seasoned programmers. If this > is the sort of idiom we want to introduce (or exists already in this > codebase) -- declaring a specific C type to avoid accidental use of an > unrelated lock -- then it's probably better documented in > CodingGuidelines rather than repeating it at each point in the code > which employs the idiom. Fair enough; I definitely weighed the usefulness of this comment a little bit when I was writing it. I figured that I didn't want to commit to updating CodingGuidelines in this series, but that I didn't want to leave the explanation only in the commit message. I figure that it's probably fine to document it in the commit message, and leave it at that for now. Thanks for this suggestion (and the earlier ones, too). I have applied them locally, and I'll sit on them for a day or two before sending out v2. Thanks again. Thanks, Taylor
> @@ -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); > } else { > unlink(git_path_shallow(the_repository)); I was hoping that the inner lock ("lk") wouldn't need to be exposed, so that it wouldn't be possible to inadvertently commit the lock without going through commit_shallow_file(), but this patch is still a step towards that. > diff --git a/shallow.h b/shallow.h > index 08e1bc4fd0..d12096fbc4 100644 > --- 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; > +}; As far as I know, we don't use many abbreviations in struct members - maybe s/lk/lock/. I couldn't find any other issues with all 5 patches in this patch series, but I would like to be able to apply the patches to be sure.
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@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. [...] > --- 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@gmail.com> Thanks.
On Wed, Apr 29, 2020 at 11:11 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > Taylor Blau wrote: > > +/* > > + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict > > + * which locks can be used with '{commit,rollback}_shallow_file()'. > > + */ > > 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'm not, in general, opposed to the structure being documented; it's just that the comment, as presented, doesn't seem to add value. > 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? This comment makes more sense and wouldn't have led to me questioning its usefulness. Thanks.
On Thu, Apr 30, 2020 at 01:32:34AM -0400, Eric Sunshine wrote: > On Wed, Apr 29, 2020 at 11:11 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > Taylor Blau wrote: > > > +/* > > > + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict > > > + * which locks can be used with '{commit,rollback}_shallow_file()'. > > > + */ > > > > 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'm not, in general, opposed to the structure being documented; it's > just that the comment, as presented, doesn't seem to add value. > > > 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? > > This comment makes more sense and wouldn't have led to me questioning > its usefulness. Thanks. Me either, thanks for the suggestion. Thanks, Taylor
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 652661fa99..10fd2c52ec 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -855,7 +855,7 @@ static void refuse_unconfigured_deny_delete_current(void) static int command_singleton_iterator(void *cb_data, struct object_id *oid); static int update_shallow_ref(struct command *cmd, struct shallow_info *si) { - struct lock_file shallow_lock = LOCK_INIT; + struct shallow_lock shallow_lock = SHALLOW_LOCK_INIT; struct oid_array extra = OID_ARRAY_INIT; struct check_connected_options opt = CHECK_CONNECTED_INIT; uint32_t mask = 1 << (cmd->index % 32); diff --git a/fetch-pack.c b/fetch-pack.c index 401d028e41..27d2d08cc0 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -35,7 +35,7 @@ static int fetch_fsck_objects = -1; static int transfer_fsck_objects = -1; static int agent_supported; static int server_supports_filtering; -static struct lock_file shallow_lock; +static struct shallow_lock shallow_lock; static const char *alternate_shallow_file; static struct strbuf fsck_msg_types = STRBUF_INIT; diff --git a/shallow.c b/shallow.c index 76e00893fe..1acf8ce95b 100644 --- a/shallow.c +++ b/shallow.c @@ -92,16 +92,16 @@ static void reset_repository_shallow(struct repository *r) stat_validity_clear(r->parsed_objects->shallow_stat); } -int commit_shallow_file(struct repository *r, struct lock_file *lk) +int commit_shallow_file(struct repository *r, struct shallow_lock *lk) { - int res = commit_lock_file(lk); + int res = commit_lock_file(&lk->lk); reset_repository_shallow(r); return res; } -void rollback_shallow_file(struct repository *r, struct lock_file *lk) +void rollback_shallow_file(struct repository *r, struct shallow_lock *lk) { - rollback_lock_file(lk); + rollback_lock_file(&lk->lk); reset_repository_shallow(r); } @@ -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); check_shallow_file_for_update(the_repository); if (write_shallow_commits(&sb, 0, extra)) { if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", - get_lock_file_path(shallow_lock)); - *alternate_shallow_file = get_lock_file_path(shallow_lock); + get_lock_file_path(&shallow_lock->lk)); + *alternate_shallow_file = get_lock_file_path(&shallow_lock->lk); } else /* * is_repository_shallow() sees empty string as "no @@ -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); } else { unlink(git_path_shallow(the_repository)); diff --git a/shallow.h b/shallow.h index 08e1bc4fd0..d12096fbc4 100644 --- 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 } + /* * {commit,rollback}_shallow_file commits or performs a rollback to the * '.git/shallow' file, respectively, and resets stat-validity checks. */ -int commit_shallow_file(struct repository *r, struct lock_file *lk); -void rollback_shallow_file(struct repository *r, struct lock_file *lk); +int commit_shallow_file(struct repository *r, struct shallow_lock *lk); +void rollback_shallow_file(struct repository *r, struct shallow_lock *lk); struct commit_list *get_shallow_commits(struct object_array *heads, int depth, int shallow_flag, int not_shallow_flag); @@ -24,7 +34,7 @@ struct commit_list *get_shallow_commits_by_rev_list( int write_shallow_commits(struct strbuf *out, int use_pack_protocol, const struct oid_array *extra); -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);
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(-)