Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
On 04/24/2015 11:51 PM, Eric Sunshine wrote: > On Fri, Apr 24, 2015 at 7:35 AM, 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 > > s/kindof/kind of/ > >> sometimes had different values. >> >> Signed-off-by: Michael Haggerty Hehe thanks for looking over my scribblings. In this case, neither "kindof" nor "kind of" is in fact correct English. I used the slang word "kindof" (sometimes spelled "kinda") to mean "somewhat", I guess because "somewhat" sounded too formal for my slapdash opinion. But I suppose it's kindof thoughtless to use slang in commit messages :-), especially given that they are also meant for people for whom English is a second language (let alone sloppy American English). I suggest s/kindof/potentially/, at least until I have time to submit a patch to the English language to make "kindof" a reputable word :-) Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
On Fri, Apr 24, 2015 at 7:35 AM, 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 s/kindof/kind of/ > sometimes had different values. > > Signed-off-by: Michael Haggerty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
On Fri, Apr 24, 2015 at 11:15:09PM +0200, Michael Haggerty wrote: > > 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. Thanks for checking. If nobody is affected (and is not likely to be), I agree it's not worth worrying about. > 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. I don't think it's necessary. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
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 mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables
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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] ref_transaction_commit(): remove the local flags variables
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.
Signed-off-by: Michael Haggerty
---
refs.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index a55d541..782e777 100644
--- a/refs.c
+++ b/refs.c
@@ -3752,16 +3752,15 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- unsigned int flags = update->flags;
- if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
- flags |= REF_DELETING;
+ if ((update->flags & REF_HAVE_NEW) &&
is_null_sha1(update->new_sha1))
+ update->flags |= REF_DELETING;
update->lock = lock_ref_sha1_basic(
update->refname,
((update->flags & REF_HAVE_OLD) ?
update->old_sha1 : NULL),
NULL,
- flags,
+ update->flags,
&update->type);
if (!update->lock) {
ret = (errno == ENOTDIR)
@@ -3776,9 +3775,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Perform updates first so live commits remain referenced */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- int flags = update->flags;
- if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {
+ if ((update->flags & REF_HAVE_NEW) &&
!is_null_sha1(update->new_sha1)) {
int overwriting_symref = ((update->type & REF_ISSYMREF)
&&
(update->flags &
REF_NODEREF));
@@ -3810,15 +3808,14 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Perform deletes now that updates are safely completed */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- int flags = update->flags;
- if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {
+ if (update->flags & REF_DELETING) {
if (delete_ref_loose(update->lock, update->type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
- if (!(flags & REF_ISPRUNING))
+ if (!(update->flags & REF_ISPRUNING))
string_list_append(&refs_to_delete,
update->lock->ref_name);
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
