[PATCH v6 1/7] refs.c: add err arguments to reflog functions
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.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: David Turner
---
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
index c018ab3..93f63d3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -624,16 +624,18 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
struct strbuf log_file = STRBUF_INIT;
int ret;
const char *ref_name;
+ struct strbuf err = STRBUF_INIT;
ref_name = mkpath("refs/heads/%s",
opts->new_orphan_branch);
temp = log_all_ref_updates;
log_all_ref_updates = 1;
- ret = log_ref_setup(ref_name, &log_file);
+ ret = log_ref_setup(ref_name, &log_file, &err);
log_all_ref_updates = temp;
strbuf_release(&log_file);
if (ret) {
- fprintf(stderr, _("Can not do reflog
for '%s'\n"),
- opts->new_orphan_branch);
+ fprintf(stderr, _("Can not do reflog
for '%s'. %s\n"),
+ opts->new_orphan_branch,
err.buf);
+ strbuf_release(&err);
return;
}
}
diff --git a/refs.c b/refs.c
index fb568d7..c97ca02 100644
--- a/refs.c
+++ b/refs.c
@@ -2975,9 +2975,11 @@ static int rename_ref_available(const char *oldname,
const char *newname)
return ret;
}
-static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char
*sha1);
+static int write_ref_to_lockfile(struct ref_lock *lock,
+const unsigned char *sha1, struct strbuf *err);
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);
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
@@ -3038,9 +3040,10 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
hashcpy(lock->old_oid.hash, orig_sha1);
- if (write_ref_to_lockfile(lock, orig_sha1) ||
- commit_ref_update(lock, orig_sha1, logmsg)) {
- error("unable to write current sha1 into %s", newrefname);
+ if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
+ commit_ref_update(lock, orig_sha1, logmsg, &err)) {
+ error("unable to write current sha1 into %s: %s", newrefname,
err.buf);
+ strbuf_release(&err);
goto rollback;
}
@@ -3056,9 +3059,11 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
flag = log_all_ref_updates;
log_all_ref_updates = 0;
- if (write_ref_to_lockfile(lock, orig_sha1) ||
- commit_ref_update(lock, orig_sha1, NULL))
- error("unable to write current sha1 into %s", oldrefname);
+ if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
+ commit_ref_update(lock, orig_sha1, NULL, &err)) {
+ error("unable to write current sha1 into %s: %s", oldrefname,
err.buf);
+ strbuf_release(&err);
+ }
log_all_ref_updates = flag;
rollbacklog:
@@ -3113,8 +3118,8 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
-/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
+/* This function will fill in *err and return -1 on failure */
+int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct
strbuf *err)
{
int logfd, oflags = O_APPEND | O_WRONLY;
char *logfile;
@@ -3129,9 +3134,8 @@ int log_ref_setup(const char *refname, struct strbuf
*sb_logfile)
starts_with(refname, "refs/notes/") ||
!strcmp(refname, "HEAD
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
> Signed-off-by: David Turner
> ---
> 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_wr
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 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 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 07/08/2015 07:11 PM, Junio C Hamano wrote: > Michael Haggerty 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
