Subject: Re: [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs

From: Jonathan Nieder <[email-masked]>
Date: Thu, 29 May 2014 11:17:32 -0700

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 <[email-masked]>
> ---
>  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