Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock
Michael Haggerty writes:
> This whole series LGTM; however, I suggest that this patch be split up.
> See below.
>
>> Signed-off-by: Stefan Beller
>> Signed-off-by: Junio C Hamano
>> ---
>> 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
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
> Signed-off-by: Junio C Hamano
> ---
> 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
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 3/3] refs.c: remove lock_fd from struct ref_lock
Stefan Beller 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
> Signed-off-by: Junio C Hamano
> ---
> 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
