[PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
This makes write_ref_sha1 only write the the lock file, committing
needs to be done outside of that function. This will help us change
the ref_transaction_commit in a later patch.
Also instead of calling unlock_ref before each return in write_ref_sha1
we can call this after the call. This is a first step to split up
write_ref_sha1 into the write and commit phase which is done in a
later patch.
There is a call in each code path after write_ref_sha1 now. Even in
the last hunk in the error case, the 'goto cleanup;' will make sure
there is a call to unlock_ref.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
new in v3
refs.c | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/refs.c b/refs.c
index f1eefc7..1bfc84b 100644
--- a/refs.c
+++ b/refs.c
@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)
return 0;
}
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
+
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
error(unable to write current sha1 into %s, newrefname);
goto rollback;
}
+ unlock_ref(lock);
return 0;
@@ -2896,8 +2902,11 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
lock-force_write = 1;
flag = log_all_ref_updates;
log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
+ if (write_ref_sha1(lock, orig_sha1, NULL)
+ || commit_ref(lock, orig_sha1))
error(unable to write current sha1 into %s, oldrefname);
+
+ unlock_ref(lock);
log_all_ref_updates = flag;
rollbacklog:
@@ -3067,22 +3076,19 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = EINVAL;
return -1;
}
- if (!lock-force_write !hashcmp(lock-old_sha1, sha1)) {
- unlock_ref(lock);
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
return 0;
- }
+
o = parse_object(sha1);
if (!o) {
error(Trying to write ref %s with nonexistent object %s,
lock-ref_name, sha1_to_hex(sha1));
- unlock_ref(lock);
errno = EINVAL;
return -1;
}
if (o-type != OBJ_COMMIT is_branch(lock-ref_name)) {
error(Trying to write non-commit object %s to branch %s,
sha1_to_hex(sha1), lock-ref_name);
- unlock_ref(lock);
errno = EINVAL;
return -1;
}
@@ -3091,7 +3097,6 @@ static int write_ref_sha1(struct ref_lock *lock,
close_ref(lock) 0) {
int save_errno = errno;
error(Couldn't write %s, lock-lk-filename.buf);
- unlock_ref(lock);
errno = save_errno;
return -1;
}
@@ -3099,7 +3104,6 @@ static int write_ref_sha1(struct ref_lock *lock,
if (log_ref_write(lock-ref_name, lock-old_sha1, sha1, logmsg) 0 ||
(strcmp(lock-ref_name, lock-orig_ref_name)
log_ref_write(lock-orig_ref_name, lock-old_sha1, sha1, logmsg)
0)) {
- unlock_ref(lock);
return -1;
}
if (strcmp(lock-orig_ref_name, HEAD) != 0) {
@@ -3124,12 +3128,6 @@ static int write_ref_sha1(struct ref_lock *lock,
!strcmp(head_ref, lock-ref_name))
log_ref_write(HEAD, lock-old_sha1, sha1, logmsg);
}
- if (commit_ref(lock)) {
- error(Couldn't set %s, lock-ref_name);
- unlock_ref(lock);
- return -1;
- }
- unlock_ref(lock);
return 0;
}
@@ -3762,14 +3760,15 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (!is_null_sha1(update-new_sha1)) {
if (write_ref_sha1(update-lock, update-new_sha1,
- update-msg)) {
- update-lock = NULL; /* freed by write_ref_sha1
*/
+ update-msg)
+ || commit_ref(update-lock, update-new_sha1)) {
strbuf_addf(err, Cannot update the ref '%s'.,
update-refname);
ret =
Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange.
Which (not new) problem are you talking about here? Do you have
a reference?
These two lines in pre-context of the hunk:
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
So these 2 lines (specially the force_write=1 line) is just there to trigger
the valid early exit path as you sent in the other mail :
if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
return 0;
when we have the same sha1?
and you're saying that's a problem because hard to understand?
I am confused as well by now.
--
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: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller sbel...@google.com writes:
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange. When the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one). For some (unknown to me) reason, however, the
code pretends that lock-old_sha1 has the new SHA-1 already before
we start to do the write or commit.
And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.
Regardless of what this particular caller does, I am not sure if the
early-return codepath in commit_ref() is correct. From the callers'
point of view, it sometimes unlocks the ref (i.e. when a different
SHA-1 is written or force_write is set) and sometimes keeps the ref
locked (i.e. when early-return is taken). Shouldn't these two cases
behave identically? Or am I wrong to assume that the early return
using hashcmp(lock-old_sha1, sha1) is a mere optimization?
--
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: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange.
Which (not new) problem are you talking about here? Do you have
a reference?
Regardless of what this particular caller does, I am not sure if the
early-return codepath in commit_ref() is correct. From the callers'
point of view, it sometimes unlocks the ref (i.e. when a different
SHA-1 is written or force_write is set) and sometimes keeps the ref
locked (i.e. when early-return is taken). Shouldn't these two cases
behave identically? Or am I wrong to assume that the early return
using hashcmp(lock-old_sha1, sha1) is a mere optimization?
I assumed it was not just an optimization as the test suite fails if I
touch that line. I'll look into cleaning it up in a more obvious fashion.
--
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: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange.
Which (not new) problem are you talking about here? Do you have
a reference?
These two lines in pre-context of the hunk:
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
--
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: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock-lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange.
Which (not new) problem are you talking about here? Do you have
a reference?
These two lines in pre-context of the hunk:
lock-force_write = 1;
hashcpy(lock-old_sha1, orig_sha1);
So these 2 lines (specially the force_write=1 line) is just there to trigger
the valid early exit path as you sent in the other mail :
if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
return 0;
when we have the same sha1?
and you're saying that's a problem because hard to understand?
What is the point of that hashcmp() in the first place? My
understanding is that
(1) lock-old_sha1 is to hold the original SHA-1 in the ref we took
the lock on.
(2) when not forcing, and when the two SHA-1 are the same, we
bypass and return early because overwriting the ref with the
same value is no-op.
Now, in that codepath, when the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one). For some (unknown to me) reason, however, the
code pretends that lock-old_sha1 has the new SHA-1 already before
we start to do the write or commit.
And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.
Let me rephrase.
A natural way to write that caller, I think, would be more like
this:
... lock is given by the caller, probably with -old_sha1
... filled in; it is not likely to be orig_sha1, as it is
... either null (if locking to create a new ref) or some
... unrelated value read from the ref being overwritten by
... this rename_ref() operation.
write_ref_sha1(lock, orig_sha1, logmsg);
# which may do the don't write the same value optmization
# if we are renaming another ref that happens to have the
# same value, in which case it is OK. Otherwise this will
# overwrite without being forced.
... *IF* and only if there is some reason lock-old_sha1
... needs to match what is in the filesystem ref right now,
... then do
hashcpy(lock-old_sha1, orig_sha1);
... but probably there is no reason to do so.
The two lines hashcpy() and -force_write = 1 that appear before the
write_ref_sha1() do not conceptually make sense. The former does
not make sense because -old_sha1 is supposed to be the original
value that is already stored in the ref, to allow us optimize write
the same value case, and you are breaking that invariant by doing
hashcpy() before write_ref_sha1(). The lock-old_sha1 value does
not have anything to do with the (original) value of the ref we took
the lock on.
And -force_write = 1 becomes necessary only because the effect of
this nonsensical hashcpy() is to make the !hashcmp() used by the
short-cut logic trigger. Since the code needs to override that
logic, you need to say force before calling write_ref_sha1(). If
you didn't do the hashcpy(), you wouldn't have to say force, no?
--
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
