Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

Stefan Beller [off-list ref] writes:

On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

-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:

quoted

quoted

      lock->force_write = 1;
      hashcpy(lock->old_sha1, orig_sha1);

On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

-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:

quoted

quoted

quoted

      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.