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

Thanks.

I have done all the additions of save_errno you suggested.


On Thu, May 29, 2014 at 11:17 AM, Jonathan Nieder [off-list ref] wrote:

Hi,

Ronnie Sahlberg wrote:

quoted

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 <redacted>
---
 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