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 <sahlb...@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

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?

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

Reply via email to