Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions
On 07/08/2015 07:11 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: I think your v7 of this patch goes too far, by turning a failure to write to the reflog into a failure of the whole transaction. The problem is that this failure comes too late, in the commit phase of the transaction. Aborting at this late stage can leave some references changed and others rolled back, violating the promise of atomicity. Yeah, that sounds problematic. The old code reported a failure to write the reflog to stderr but didn't fail the transaction. I think that behavior is more appropriate. The reflog is of lower importance than the references themselves. Junio, do you agree? That is actually a loaded question. Do I agree that the current (i.e. before this change) behaviour is more appropriate given the current choice of representation of refs and reflogs on the filesystem, treating a failure to update reflog as lower importance event and accept it as a limitation that it cannot abort the whole transaction atomically? Compared to leaving the repository in a half-updated state where some refs and their logs are updated already, other remaining proposed updates are ignored, and the transaction claims to have failed even though some things have already changed and we cannot rollback, I would say that is a better compromise to treat reflog update as a lower importance. Do I agree that reflog writing should stay to be best-effort in the longer term? Not really. If we are moving the ref API in the direction where we can plug a backend that is different from the traditional filesystem based one for storing refs, I think the backend should also be responsible for storing logs for the refs it stores, and if the backend wants to promise atomicity, then we should be able to fail the whole transaction when updates to refs could proceed but updates to the log of one of these updated refs cannot. So I do not agree to cast in stone the reflog is of lower importance as a general rule. Junio, You make a good distinction. I was describing a compromise that we have to make now due to the limitations of the current ref/reflog backend. But I agree 100% that a future storage backend that can do correct rollback of refs *and* reflogs can fail a transaction if the reflog updates cannot be committed. Thanks, 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 v6 1/7] refs.c: add err arguments to reflog functions
On 07/08/2015 12:41 AM, David Turner wrote: On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote: On 06/29/2015 10:17 PM, David Turner wrote: [...] @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock, head_sha1, head_flag); if (head_ref (head_flag REF_ISSYMREF) !strcmp(head_ref, lock-ref_name)) - log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg); + log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg, + err); Here you don't check for errors from log_ref_write(). So it might or might not have written something to err. If it has, is that error handled correctly? That was the case before this patch too. But I guess I might as well check. This isn't about fixing a pre-existing bug. Your patch introduced a regression. Before this patch, commit_ref_update() didn't really need to know whether log_ref_write() failed, because it didn't take any further remedial action anyway. log_ref_write() would have already written an error message to stderr, and that was all the error handling that was needed. But now, log_ref_write() *doesn't* write its error message to stderr; it only stores it to `err`. So now it is the caller's responsibility to check whether there was an error, and if so write `err` to stderr. Otherwise the error message will get lost entirely, I think. I think your v7 of this patch goes too far, by turning a failure to write to the reflog into a failure of the whole transaction. The problem is that this failure comes too late, in the commit phase of the transaction. Aborting at this late stage can leave some references changed and others rolled back, violating the promise of atomicity. The old code reported a failure to write the reflog to stderr but didn't fail the transaction. I think that behavior is more appropriate. The reflog is of lower importance than the references themselves. Junio, do you agree? If so, it might be that adding err arguments to the reflog functions does not bring any benefits. 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 v6 1/7] refs.c: add err arguments to reflog functions
Michael Haggerty mhag...@alum.mit.edu writes: I think your v7 of this patch goes too far, by turning a failure to write to the reflog into a failure of the whole transaction. The problem is that this failure comes too late, in the commit phase of the transaction. Aborting at this late stage can leave some references changed and others rolled back, violating the promise of atomicity. Yeah, that sounds problematic. The old code reported a failure to write the reflog to stderr but didn't fail the transaction. I think that behavior is more appropriate. The reflog is of lower importance than the references themselves. Junio, do you agree? That is actually a loaded question. Do I agree that the current (i.e. before this change) behaviour is more appropriate given the current choice of representation of refs and reflogs on the filesystem, treating a failure to update reflog as lower importance event and accept it as a limitation that it cannot abort the whole transaction atomically? Compared to leaving the repository in a half-updated state where some refs and their logs are updated already, other remaining proposed updates are ignored, and the transaction claims to have failed even though some things have already changed and we cannot rollback, I would say that is a better compromise to treat reflog update as a lower importance. Do I agree that reflog writing should stay to be best-effort in the longer term? Not really. If we are moving the ref API in the direction where we can plug a backend that is different from the traditional filesystem based one for storing refs, I think the backend should also be responsible for storing logs for the refs it stores, and if the backend wants to promise atomicity, then we should be able to fail the whole transaction when updates to refs could proceed but updates to the log of one of these updated refs cannot. So I do not agree to cast in stone the reflog is of lower importance as a general rule. -- 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 v6 1/7] refs.c: add err arguments to reflog functions
On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote: On 06/29/2015 10:17 PM, David Turner wrote: Add an err argument to log_ref_setup that can explain the reason for a failure. This then eliminates the need to manage errno through this function since we can just add strerror(errno) to the err string when meaningful. No callers relied on errno from this function for anything else than the error message. Also add err arguments to private functions write_ref_to_lockfile, log_ref_write_1, commit_ref_update. This again eliminates the need to manage errno in these functions. Update of a patch by Ronnie Sahlberg. First a general comment: we have a convention, not yet very well adhered to, that error messages should start with lower-case letters. I know that in many cases you are just carrying along pre-existing error messages that are capitalized. But at a minimum, please avoid adding new error messages that are capitalized. And if you are feeling ambitious, feel free to lower-case some existing ones :-) I'll save lowercasing messages for other patches, but I'll try to take care with new messages. ... Above, the call to close(logfd) could clobber errno. It would be better to exchange the calls. Done, thanks. Since you are passing err into log_ref_write(), I assume that it would already have an error message written to it by the time you enter into this block. Yet in the block you append more text to it. It seems to me that you either want to clear err and replace it with your own message, or create a new message that looks like Cannot update the ref '%s': %s where the second %s is replaced with the error from log_ref_write(). Done, thanks. @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock, head_sha1, head_flag); if (head_ref (head_flag REF_ISSYMREF) !strcmp(head_ref, lock-ref_name)) - log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg); + log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg, + err); Here you don't check for errors from log_ref_write(). So it might or might not have written something to err. If it has, is that error handled correctly? That was the case before this patch too. But I guess I might as well check. Previously, the error generated here was cannot update the ref '%s'. But now that you are letting write_ref_to_lockfile() fill err, the error message will be something from that function. This might be OK or it might not. If you think it is, it would be worth a word or two of justification in the commit message. Will adjust commit message. -- 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 v6 1/7] refs.c: add err arguments to reflog functions
On 06/29/2015 10:17 PM, David Turner wrote:
Add an err argument to log_ref_setup that can explain the reason
for a failure. This then eliminates the need to manage errno through
this function since we can just add strerror(errno) to the err string
when meaningful. No callers relied on errno from this function for
anything else than the error message.
Also add err arguments to private functions write_ref_to_lockfile,
log_ref_write_1, commit_ref_update. This again eliminates the need to
manage errno in these functions.
Update of a patch by Ronnie Sahlberg.
First a general comment: we have a convention, not yet very well adhered
to, that error messages should start with lower-case letters. I know
that in many cases you are just carrying along pre-existing error
messages that are capitalized. But at a minimum, please avoid adding new
error messages that are capitalized. And if you are feeling ambitious,
feel free to lower-case some existing ones :-)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: David Turner dtur...@twopensource.com
---
builtin/checkout.c | 8 ++--
refs.c | 111
-
refs.h | 4 +-
3 files changed, 66 insertions(+), 57 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
[...]
diff --git a/refs.c b/refs.c
index fb568d7..c97ca02 100644
--- a/refs.c
+++ b/refs.c
[...]
@@ -3216,26 +3217,25 @@ static int log_ref_write_1(const char *refname, const
unsigned char *old_sha1,
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
git_committer_info(0), msg);
if (result) {
- int save_errno = errno;
close(logfd);
- error(Unable to append to %s, log_file);
- errno = save_errno;
+ strbuf_addf(err, Unable to append to %s. %s, log_file,
+ strerror(errno));
return -1;
Above, the call to close(logfd) could clobber errno. It would be better
to exchange the calls.
}
if (close(logfd)) {
- int save_errno = errno;
- error(Unable to append to %s, log_file);
- errno = save_errno;
+ strbuf_addf(err, Unable to append to %s. %s, log_file,
+ strerror(errno));
return -1;
}
return 0;
}
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg)
+ const unsigned char *new_sha1, const char *msg,
+ struct strbuf *err)
{
struct strbuf sb = STRBUF_INIT;
- int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, sb);
+ int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, sb, err);
strbuf_release(sb);
return ret;
}
[...]
@@ -3288,12 +3290,15 @@ static int write_ref_to_lockfile(struct ref_lock
*lock,
* necessary, using the specified lockmsg (which can be NULL).
*/
static int commit_ref_update(struct ref_lock *lock,
- const unsigned char *sha1, const char *logmsg)
+ const unsigned char *sha1, const char *logmsg,
+ struct strbuf *err)
{
clear_loose_ref_cache(ref_cache);
- if (log_ref_write(lock-ref_name, lock-old_oid.hash, sha1, logmsg) 0
||
+ if (log_ref_write(lock-ref_name, lock-old_oid.hash, sha1, logmsg,
err) 0 ||
(strcmp(lock-ref_name, lock-orig_ref_name)
- log_ref_write(lock-orig_ref_name, lock-old_oid.hash, sha1,
logmsg) 0)) {
+ log_ref_write(lock-orig_ref_name, lock-old_oid.hash, sha1,
logmsg, err) 0)) {
+ strbuf_addf(err, Cannot update the ref '%s'.,
+ lock-ref_name);
unlock_ref(lock);
return -1;
}
Since you are passing err into log_ref_write(), I assume that it would
already have an error message written to it by the time you enter into
this block. Yet in the block you append more text to it.
It seems to me that you either want to clear err and replace it with
your own message, or create a new message that looks like
Cannot update the ref '%s': %s
where the second %s is replaced with the error from log_ref_write().
@@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
head_sha1, head_flag);
if (head_ref (head_flag REF_ISSYMREF)
!strcmp(head_ref, lock-ref_name))
- log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg);
+ log_ref_write(HEAD, lock-old_oid.hash, sha1, logmsg,
+ err);
Here you don't check for errors from log_ref_write(). So it might or
might not have written something to err. If it has, is that error
handled
