Thanks. I have done all the additions of save_errno you suggested. On Thu, May 29, 2014 at 11:17 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > 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@xxxxxxxxxx> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html