From: Jonathan Nieder <jrnieder@gmail.com>
To: Ronnie Sahlberg <sahlberg@google.com>
Cc: git@vger.kernel.org, Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs
Date: Thu, 29 May 2014 11:17:32 -0700 [thread overview]
Message-ID: <20140529181732.GF12314@google.com> (raw)
In-Reply-To: <1401379676-9307-2-git-send-email-sahlberg@google.com>
Hi,
Ronnie Sahlberg wrote:
> Update repack_without_refs to take an err argument and update it if there
> is a failure. Pass the err variable from ref_transaction_commit to this
> function so that callers can print a meaningful error message if _commit
> fails due to a problem in repack_without_refs.
>
> Add a new function unable_to_lock_message that takes a strbuf argument and
> fills in the reason for the failure.
>
> In commit_packed_refs, make sure that we propagate any errno that
> commit_lock_file might have set back to our caller.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
> cache.h | 2 ++
> lockfile.c | 21 ++++++++++++---------
> refs.c | 25 +++++++++++++++++++------
> 3 files changed, 33 insertions(+), 15 deletions(-)
I don't want to sound like a broken record, but this still has the
same issues I described before at
http://thread.gmane.org/gmane.comp.version-control.git/250197/focus=250309.
The commit message or documentation or notes after the three dashes
above could explain what I missed when making my suggestions.
Otherwise I get no reality check as a reviewer, other reviewers get
less insight into what's happening in the patch, people in the future
looking into the patch don't understand its design as well, etc.
As a general rule, that is a good practice anyway --- even when a
reviewer was confused, what they got confused about can be an
indication of where to make the code or design documentation (commit
message) more clear, and when reposting a patch it can be a good
opportunity to explain how the patch evolved.
What would be wrong with the line of API documentation and the TODO
comment for a known bug I suggested? If they are a bad idea, can you
explain that so I can learn from it? Or if they were confusing, would
you like a patch that explains what I mean?
Jonathan
next prev parent reply other threads:[~2014-05-29 18:17 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-29 16:07 [PATCH v12 00/44] Use ref transactions for all ref updates Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs Ronnie Sahlberg
2014-05-29 18:17 ` Jonathan Nieder [this message]
2014-06-03 20:55 ` Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 08/41] refs.c: add an err argument to delete_ref_loose Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 24/41] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 25/41] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 26/41] walker.c: use ref transaction for ref updates Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 31/41] refs.c: make prune_ref use a transaction to delete the ref Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 32/41] refs.c: make delete_ref use a transaction Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 38/41] refs.c: pass a skip list to name_conflict_fn Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 40/41] fetch.c: change s_update_ref to use a ref transaction Ronnie Sahlberg
2014-05-29 16:07 ` [PATCH v12 41/41] refs.c: make write_ref_sha1 static Ronnie Sahlberg
2014-05-29 16:13 ` [PATCH v12 00/44] Use ref transactions for all ref updates Ronnie Sahlberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140529181732.GF12314@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
--cc=sahlberg@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.