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

> 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