Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

2015-04-15 Thread Junio C Hamano 
Michael Haggerty mhag...@alum.mit.edu writes:

 This whole series LGTM; however, I suggest that this patch be split up.
 See below.

 Signed-off-by: Stefan Beller sbel...@google.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  refs.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)
 
 diff --git a/refs.c b/refs.c
 index 14e52ca..4066752 100644
 --- a/refs.c
 +++ b/refs.c
 [...]
 @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
 *refname,
  goto error_return;
  }
  
 -lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
 -if (lock-lock_fd  0) {
 +if (hold_lock_file_for_update(lock-lk, ref_file, lflags)  0) {
 +last_errno = errno;
  if (errno == ENOENT  --attempts_remaining  0)
  /*
   * Maybe somebody just deleted one of the
 [...]

 Here you add the line last_errno = errno. It is a good change, but it
 is not part of removing ref_lock::lock_fd.

I think this patch came from an ancient codebase before 06839515
(lock_ref_sha1_basic: do not die on locking errors, 2014-11-19),
which added the last_errno = errno, and was not rebased to match
more recent codebase.

I am planning to apply these on top of v2.4.0-rc, so there will be
no new save to last_errno in the end.

Thanks.

--
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 3/3] refs.c: remove lock_fd from struct ref_lock

2015-04-15 Thread Michael Haggerty 
On 04/15/2015 12:25 AM, Stefan Beller wrote:
 The 'lock_fd' is the same as 'lk-fd'. No need to store it twice so remove
 it. You may argue this introduces more coupling as we need to know more
 about the internals of the lock file mechanism, but this will be solved in
 a later patch.
 
 No functional changes intended.

This whole series LGTM; however, I suggest that this patch be split up.
See below.

 Signed-off-by: Stefan Beller sbel...@google.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  refs.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)
 
 diff --git a/refs.c b/refs.c
 index 14e52ca..4066752 100644
 --- a/refs.c
 +++ b/refs.c
 [...]
 @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
 *refname,
   goto error_return;
   }
  
 - lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
 - if (lock-lock_fd  0) {
 + if (hold_lock_file_for_update(lock-lk, ref_file, lflags)  0) {
 + last_errno = errno;
   if (errno == ENOENT  --attempts_remaining  0)
   /*
* Maybe somebody just deleted one of the
 [...]

Here you add the line last_errno = errno. It is a good change, but it
is not part of removing ref_lock::lock_fd.

I suggest that you move this change to a separate commit. You might also
consider moving the new line to the else clause, because it's really
about preserving errno around the call to error() and preparing for
goto error_return.

With or without this split, this patch is

Reviewed-by: Michael Haggerty mhag...@alum.mit.edu

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

[PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

2015-04-14 Thread Stefan Beller 
The 'lock_fd' is the same as 'lk-fd'. No need to store it twice so remove
it. You may argue this introduces more coupling as we need to know more
about the internals of the lock file mechanism, but this will be solved in
a later patch.

No functional changes intended.

Signed-off-by: Stefan Beller sbel...@google.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 refs.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 14e52ca..4066752 100644
--- a/refs.c
+++ b/refs.c
@@ -11,7 +11,6 @@ struct ref_lock {
char *orig_ref_name;
struct lock_file *lk;
unsigned char old_sha1[20];
-   int lock_fd;
int force_write;
 };
 
@@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
int attempts_remaining = 3;
 
lock = xcalloc(1, sizeof(struct ref_lock));
-   lock-lock_fd = -1;
 
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
@@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
goto error_return;
}
 
-   lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
-   if (lock-lock_fd  0) {
+   if (hold_lock_file_for_update(lock-lk, ref_file, lflags)  0) {
+   last_errno = errno;
if (errno == ENOENT  --attempts_remaining  0)
/*
 * Maybe somebody just deleted one of the
@@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
 {
if (close_lock_file(lock-lk))
return -1;
-   lock-lock_fd = -1;
return 0;
 }
 
@@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
 {
if (commit_lock_file(lock-lk))
return -1;
-   lock-lock_fd = -1;
return 0;
 }
 
@@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = EINVAL;
return -1;
}
-   if (write_in_full(lock-lock_fd, sha1_to_hex(sha1), 40) != 40 ||
-   write_in_full(lock-lock_fd, term, 1) != 1 ||
+   if (write_in_full(lock-lk-fd, sha1_to_hex(sha1), 40) != 40 ||
+   write_in_full(lock-lk-fd, term, 1) != 1 ||
close_ref(lock)  0) {
int save_errno = errno;
error(Couldn't write %s, lock-lk-filename.buf);
@@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
status |= error(couldn't write %s: %s, log_file,
strerror(errno));
} else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
-   (write_in_full(lock-lock_fd,
+   (write_in_full(lock-lk-fd,
sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock-lock_fd, \n) != 1 ||
+write_str_in_full(lock-lk-fd, \n) != 1 ||
 close_ref(lock)  0)) {
status |= error(couldn't write %s,
lock-lk-filename.buf);
-- 
2.3.0.81.gc37f363

--
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 3/3] refs.c: remove lock_fd from struct ref_lock

2015-04-14 Thread Junio C Hamano 
Stefan Beller sbel...@google.com writes:

 The 'lock_fd' is the same as 'lk-fd'. No need to store it twice so remove
 it. You may argue this introduces more coupling as we need to know more
 about the internals of the lock file mechanism, but this will be solved in
 a later patch.

 No functional changes intended.

It is somewhat strange to hear in a later patch in [PATCH 3/3] of
a 3-patch series ;-), but I think this makes sense.  Whenever
we take a ref-lock, and we are going to actually write something
into the filesystem, we would go thru the lock_file API, so we can
depend on lk to have its own file descriptor field.





 Signed-off-by: Stefan Beller sbel...@google.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  refs.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

 diff --git a/refs.c b/refs.c
 index 14e52ca..4066752 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -11,7 +11,6 @@ struct ref_lock {
   char *orig_ref_name;
   struct lock_file *lk;
   unsigned char old_sha1[20];
 - int lock_fd;
   int force_write;
  };
  
 @@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
 *refname,
   int attempts_remaining = 3;
  
   lock = xcalloc(1, sizeof(struct ref_lock));
 - lock-lock_fd = -1;
  
   if (mustexist)
   resolve_flags |= RESOLVE_REF_READING;
 @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
 *refname,
   goto error_return;
   }
  
 - lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
 - if (lock-lock_fd  0) {
 + if (hold_lock_file_for_update(lock-lk, ref_file, lflags)  0) {
 + last_errno = errno;
   if (errno == ENOENT  --attempts_remaining  0)
   /*
* Maybe somebody just deleted one of the
 @@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
  {
   if (close_lock_file(lock-lk))
   return -1;
 - lock-lock_fd = -1;
   return 0;
  }
  
 @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
  {
   if (commit_lock_file(lock-lk))
   return -1;
 - lock-lock_fd = -1;
   return 0;
  }
  
 @@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,
   errno = EINVAL;
   return -1;
   }
 - if (write_in_full(lock-lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 - write_in_full(lock-lock_fd, term, 1) != 1 ||
 + if (write_in_full(lock-lk-fd, sha1_to_hex(sha1), 40) != 40 ||
 + write_in_full(lock-lk-fd, term, 1) != 1 ||
   close_ref(lock)  0) {
   int save_errno = errno;
   error(Couldn't write %s, lock-lk-filename.buf);
 @@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned 
 char *sha1,
   status |= error(couldn't write %s: %s, log_file,
   strerror(errno));
   } else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
 - (write_in_full(lock-lock_fd,
 + (write_in_full(lock-lk-fd,
   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
 -  write_str_in_full(lock-lock_fd, \n) != 1 ||
 +  write_str_in_full(lock-lk-fd, \n) != 1 ||
close_ref(lock)  0)) {
   status |= error(couldn't write %s,
   lock-lk-filename.buf);
--
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