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@xxxxxxxxx> 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
