Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Apr 29, 2020, 23:51 UTC
- Message-ID
- <20200429235146.GA20588@syl.local>
- In-Reply-To
- <CAPig+cTvX1wT8kAha22uUQ5PDQ0mjeoDGQdA6VtMzfnAELpTaA@mail.gmail.com>
On Wed, Apr 29, 2020 at 07:03:48PM -0400, Eric Sunshine wrote:
Show 25 quoted lines
> 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