From: Michael Haggerty <mhagger@alum•mit.edu>
To: Jeff King <peff@peff•net>
Cc: Junio C Hamano <gitster@pobox•com>,
Stefan Beller <sbeller@google•com>,
git@vger•kernel.org
Subject: Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
Date: Fri, 24 Apr 2015 23:15:09 +0200 [thread overview]
Message-ID: <553AB25D.9090201@alum.mit.edu> (raw)
In-Reply-To: <20150424173005.GC30592@peff.net>
On 04/24/2015 07:30 PM, Jeff King wrote:
> On Fri, Apr 24, 2015 at 01:35:48PM +0200, Michael Haggerty wrote:
>
>> Instead, work directly with update->flags. This has the advantage that
>> the REF_DELETING bit, set in the first loop, can be read in the third
>> loop instead of having to compute the same expression again. Plus, it
>> was kindof confusing having both update->flags and flags, which
>> sometimes had different values.
>
> Hmm. I think this is losing the distinction of "flags the caller has
> passed in to us" versus "flags we are using locally only during the
> transaction_commit routine". If callers look at the flags in the
> REF_TRANSACTION_CLOSED state, do they care about seeing these new flags?
>
> My guess is probably not in practice, and "leaking" these flags is an
> acceptable tradeoff for keeping the transaction_commit function simpler.
> But I haven't looked that closely.
"struct ref_update" is opaque to callers outside of the refs module, and
ref_update::flags is not read anywhere outside of
ref_transaction_commit() (and its value is passed to
lock_ref_sha1_basic()). So I don't think we have to be shy about storing
our own internal information there.
In fact, REF_DELETING, REF_ISPRUNING, REF_HAVE_NEW, and REF_HAVE_OLD are
also private to the refs module.
I suppose we could mask out all the "private" bits in the flags
parameter passed by the caller, to make sure that the caller hasn't
accidentally set other bits. I think that would be more defensive than
our usual practice, but I don't mind doing it if people think it would
be prudent.
Michael
--
Michael Haggerty
mhagger@alum•mit.edu
next prev parent reply other threads:[~2015-04-24 21:15 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-24 11:35 [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit() Michael Haggerty
2015-04-24 11:35 ` [PATCH 1/5] write_ref_to_lockfile(): new function, extracted from write_ref_sha1() Michael Haggerty
2015-04-24 11:35 ` [PATCH 2/5] commit_ref_update(): " Michael Haggerty
2015-04-24 11:35 ` [PATCH 3/5] write_ref_sha1(): inline function at callers Michael Haggerty
2015-04-24 11:35 ` [PATCH 4/5] ref_transaction_commit(): remove the local flags variables Michael Haggerty
2015-04-24 17:30 ` Jeff King
2015-04-24 21:15 ` Michael Haggerty [this message]
2015-04-24 21:19 ` Jeff King
2015-04-24 21:51 ` Eric Sunshine
2015-04-24 22:22 ` Michael Haggerty
2015-04-24 11:35 ` [PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time Michael Haggerty
2015-04-25 6:08 ` Michael Haggerty
2015-04-25 6:57 ` Junio C Hamano
2015-04-25 19:21 ` Junio C Hamano
2015-04-28 4:36 ` Jeff King
2015-04-24 17:26 ` [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit() Jeff King
2015-04-24 19:13 ` Stefan Beller
2015-04-25 19:27 ` Junio C Hamano
2015-04-25 4:28 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=553AB25D.9090201@alum.mit.edu \
--to=mhagger@alum$(echo .)mit.edu \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox