Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-27 Thread Ronnie Sahlberg 
On Thu, May 22, 2014 at 4:08 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 This patch series can also be found at
 https://github.com/rsahlberg/git/tree/ref-transactions

 Continuing with the review of 65a1cb7b (2014-05-22 12:08):

  11/40 change ref_transaction_update() to do error checking and return status
  The there will be in the future sounds ominous.  Do you have an
  example in mind?  E.g., I suppose it would be nice if _update could
  notice D/F conflicts or connection to a database server closing early,
  but it's not clear to me whether the kind of errors you're talking
  about are that or something else.


Updated the message.
Next series moves both locking as well as checking for name conflicts
to _update.

  With or without such a clarification,
  Reviewed-by: Jonathan Nieder jrnie...@gmail.com

  12/40 change ref_transaction_create to do error checking and return status
  What does On failure the err buffer will be updated mean?  Will
  it clear err and replace it with a message, append to err, or
  something else?  Does the message explain the context or is the
  caller responsible for adding to it?  Does the message end with a
  newline or is the caller responsible for adding one when printing it
  out?

I have updated the documentation.
Message is appended to the string buffer. Caller is required to
strbuf_reset before calling the transaction if caller wants only most
recent error instead of all errors appended one by one.



  For cases like this where lots of functions have a similar API,
  API comments start to become potentially repetitive.  It might be
  better to explain conventions at the top of the file or in
  Documentation/technical/api-refs.txt and say See the top of the
  file for error handling conventions or Returns non-zero and
  appends a message to err on error.  See
  Documentation/technical/api-refs.txt for more details on error
  handling.

Done.


  13/40 ref_transaction_delete to check for error and return status
  Each successive commit dropped something from its subject. :)
  (First the (), then the verb.)

Done.


  Same comments as before about an example being useful for the
  log message and the API documentation on error handling being a
  bit vague.

  14/40 make ref_transaction_begin take an err argument
  I found the failed to connect to mysql example instructive while
  doing reviews.  Perhaps it would be worth mentioning in the commit
  message.

  Reviewed-by: Jonathan Nieder jrnie...@gmail.com

  15/40 add transaction.status and track OPEN/CLOSED/ERROR
  It says an ERRORed transaction cannot be committed and can be rolled
  back by calling _free.  Can a CLOSED transaction be committed or
  _freed?

  What does faild mean in the documentation comments?  (Maybe
  non-OPEN?)

  In the previous version of this patch passing a non-OPEN transaction
  would die(BUG: ...) to diagnose the caller's mistake.  Now I'm
  confused about the API: it seems you're allowed to pass a non-OPEN
  transaction but it doesn't append a message to 'err' in that case.
  Is this meant as a way to save the caller some typing, like
  fwrite/fclose do?  (I've found people often make mistakes with the
  fwrite API fwiw but can understand the appeal of it.)

  Maybe with more context I'd like this.  As is, it feels like a step
  in the wrong direction.

  16/40 tag: use ref transactions when doing updates
  Reviewed-by: Jonathan Nieder jrnie...@gmail.com

  17/40 replace: use ref transactions when doing updates
  Reviewed-by: Jonathan Nieder jrnie...@gmail.com

  18/40 commit: use ref transactions for updates
  Reviewed-by: Jonathan Nieder jrnie...@gmail.com

  19/40 sequencer: use ref transactions for all ref updates
  This would be a lot simpler if the ref_transaction_commit should not
  free the transaction patch came before it (yes, sorry, killing the
  fun).  I can push the result of a rebase doing that somewhere if you
  like.

Beeing done.


  20/40 fast-import: change update_branch to use ref transactions
  Likewise.
--
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

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-22 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 This version completes the work to convert all ref updates to use 
 transactions.

Finally got through this.  It had thorny bits but generally goes in a
very good direction.  Thanks for a pleasant read.

Feel free to send another iteration if you'd like review for the newer
code.  I expect except for the part about renames that most of what's
left is just nits so it should go faster.

Thanks,
Jonathan
--
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

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-22 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 This patch series can also be found at
 https://github.com/rsahlberg/git/tree/ref-transactions

Thoughts on 65a1cb7b (2014-05-22 12:08):

 01/40 remove ref_transaction_rollback
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 02/40 constify the sha arguments for ref_transaction_create|delete|update
 still Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 03/40 allow passing NULL to ref_transaction_free
 The third paragraph (This allows us to write code like) is still
 confusing.  Why not drop that paragraph?

 04/40 --- oh, lunch time.  Will pick up when I get back.

Thanks,
Jonathan
--
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

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-22 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 This patch series can also be found at
 https://github.com/rsahlberg/git/tree/ref-transactions

Thoughts on 65a1cb7b (2014-05-22 12:08):

 04/40 add a strbuf argument to ref_transaction_commit for error logging

 Ideally this would come after the functions it calls so the comment
 If error is non-NULL we will add an error string to it to explain
 why the transaction failed would be true already.  But rearranging
 the series for that would be more fuss than it's worth IMHO.

 The sanity check

int ref_transaction_commit(...)
{
int ret = ref_transaction_commit_real(...);
if (ret  err  !err-len)
die(BUG: ref_transaction_commit forgot to write an 
error message);
return ret;
}

 shows no instances of forgotten error messages in cases exercised by
 tests, at least.  And it's a step in the right direction.

 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 05/40 add an err argument to repack_without_refs
 unable_to_lock_strbuf could be called unable_to_lock_message (which
 would make its behavior more obvious imho).

 This makes errno meaningful when commit_packed_refs returns an error.
 Probably its API documentation should say so to make it obvious to
 people modifying it in the future to preserve that property or audit
 callers.

 Via the new call to unable_to_lock_..., repack_without_refs cares
 about errno after a failed call to lock_packed_refs.  lock_packed_refs
 can only fail in hold_lock_file_for_update.  hold_lock_file_for_update
 is a thin wrapper around lockfile.c::lock_file.  lock_file can error
 out because

strlen(path) = max_path_len
adjust_shared_perm failed [calls error(), clobbers errno]
open failed

 So lock_file needs a similar kind of fix, and it's probably worth
 updating API documentation for these calls to make it clear that
 their errno is used (though that's not a new problem since
 repack_without_refs already called unable_to_lock_error).  Could be
 a separate, earlier patch since it's fixing an existing bug.

 06/40 make ref_update_reject_duplicates take a strbuf argument for errors
 still Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 07/40 add an err argument to delete_ref_loose
 The new unlink_or_err has an odd contract when the err argument is passed.
 On error:

  * if errno != ENOENT, it will append a message to err and return -1 (good)
  * if errno == ENOENT, it will not append a message to err but will
still return -1.

 Perhaps it should return 0 when errno == ENOENT.  After all, in that
 case the file does not exist any more, which is all we wanted.  And it
 would save the caller from having to inspect errno.

 On failure we seem to add our own message to err, too, so the resulting
 message would be something like

fatal: unable to unlink .git/refs/heads/master: \
Permission deniedfailed to delete loose ref 
'.git/refs/heads/master.lock'

 The second strbuf_addf is probably not needed.

 08/40 make update_ref_write update a strbuf on failure
 still Reviewed-by: Jonathan Nieder jrnie...@gmail.com
 
 09/40 log transaction error from the update_ref
 No actual functional change intended, right?  I'd say something like
 update-ref: use err argument to get error from ref_transaction_commit
 or something similar to make it clearer that this is just about
 changing APIs.  Or if there's an intended functional change, then the
 commit message could say something about that.

 10/40 remove the onerr argument to ref_transaction_commit
 still Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
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

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-22 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 This patch series can also be found at
 https://github.com/rsahlberg/git/tree/ref-transactions

Continuing with the review of 65a1cb7b (2014-05-22 12:08):

 11/40 change ref_transaction_update() to do error checking and return status
 The there will be in the future sounds ominous.  Do you have an
 example in mind?  E.g., I suppose it would be nice if _update could
 notice D/F conflicts or connection to a database server closing early,
 but it's not clear to me whether the kind of errors you're talking
 about are that or something else.

 With or without such a clarification,
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 12/40 change ref_transaction_create to do error checking and return status
 What does On failure the err buffer will be updated mean?  Will
 it clear err and replace it with a message, append to err, or
 something else?  Does the message explain the context or is the
 caller responsible for adding to it?  Does the message end with a
 newline or is the caller responsible for adding one when printing it
 out?

 For cases like this where lots of functions have a similar API,
 API comments start to become potentially repetitive.  It might be
 better to explain conventions at the top of the file or in
 Documentation/technical/api-refs.txt and say See the top of the
 file for error handling conventions or Returns non-zero and
 appends a message to err on error.  See
 Documentation/technical/api-refs.txt for more details on error
 handling.

 13/40 ref_transaction_delete to check for error and return status
 Each successive commit dropped something from its subject. :)
 (First the (), then the verb.)

 Same comments as before about an example being useful for the
 log message and the API documentation on error handling being a
 bit vague.

 14/40 make ref_transaction_begin take an err argument
 I found the failed to connect to mysql example instructive while
 doing reviews.  Perhaps it would be worth mentioning in the commit
 message.

 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 15/40 add transaction.status and track OPEN/CLOSED/ERROR
 It says an ERRORed transaction cannot be committed and can be rolled
 back by calling _free.  Can a CLOSED transaction be committed or
 _freed?

 What does faild mean in the documentation comments?  (Maybe
 non-OPEN?)

 In the previous version of this patch passing a non-OPEN transaction
 would die(BUG: ...) to diagnose the caller's mistake.  Now I'm
 confused about the API: it seems you're allowed to pass a non-OPEN
 transaction but it doesn't append a message to 'err' in that case.
 Is this meant as a way to save the caller some typing, like
 fwrite/fclose do?  (I've found people often make mistakes with the
 fwrite API fwiw but can understand the appeal of it.)

 Maybe with more context I'd like this.  As is, it feels like a step
 in the wrong direction.

 16/40 tag: use ref transactions when doing updates
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 17/40 replace: use ref transactions when doing updates
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

 18/40 commit: use ref transactions for updates
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com
 
 19/40 sequencer: use ref transactions for all ref updates
 This would be a lot simpler if the ref_transaction_commit should not
 free the transaction patch came before it (yes, sorry, killing the
 fun).  I can push the result of a rebase doing that somewhere if you
 like.

 20/40 fast-import: change update_branch to use ref transactions
 Likewise.
--
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

[PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-15 Thread Ronnie Sahlberg 
This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions


This patch series is based on next and expands on the transaction API. It
converts all ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most of the ref updates to become
atomic when there are failures locking or writing to a ref.

This version completes the work to convert all ref updates to use transactions.
Now that all updates are through transactions I will start working on
cleaning up the reading of refs and to create an api for managing reflogs but
all that will go in a different patch series.

Version 8:
 - Updates after review by JN
 - Improve commit messages
 - Add a patch that adds an err argument to repack_without_refs
 - Add a patch that adds an err argument to delete_loose_ref
 - Better document that a _update/_delete/_create failure means the whole
   transaction has failed and needs rollback.

Version 7:
 - Updated commit messages per JNs review comments.
 - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
   exposed through refs.h

Version 6:
 - Convert all updates in refs.c to use transactions too.

Version 5:
 - Reword commit messages for having _create/_delete/_update returning
   success/failure. There are no conditions yet that return an error from
   these failures but there will be in the future. So we still check the
   return from these functions in the callers in preparation for this.
 - Don't leak memory by just passing a strbuf_detach() pointer to functions.
   Use obj.buf and explicitely strbuf_release the data afterwards.
 - Remove the function update_ref_lock.
 - Remove the function update_ref_write.
 - Track transaction status and die(BUG:) if we call _create/_delete/_update/
   _commit for a transaction that is not OPEN.

Version 4:
 - Rename patch series from Use ref transactions from most callers to
   Use ref transactions for all ref updates.
 - Convert all external ref writes to use transactions and make write_ref_sha1
   and lock_ref_sha1 static functions.
 - Change the ref commit and free handling so we no longer pass pointer to
   pointer to _commit. _commit no longer frees the transaction. The caller
   MUST call _free itself.
 - Change _commit to take a strbuf pointer instead of a char* for error
   reporting back to the caller.
 - Re-add the walker patch after fixing it.

Version 3:
 - Remove the walker patch for now. Walker needs more complex solution
   so defer it until the basics are done.
 - Remove the onerr argument to ref_transaction_commit(). All callers
   that need to die() on error now have to do this explicitely.
 - Pass an error string from ref_transaction_commit() back to the callers
   so that they can craft a nice error message upon failures.
 - Make ref_transaction_rollback() accept NULL as argument.
 - Change ref_transaction_commit() to take a pointer to pointer argument for
   the transaction and have it clear the callers pointer to NULL when
   invoked. This allows for much nicer handling of transaction rollback on
   failure.
Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head-object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.



Ronnie Sahlberg (44):
  refs.c: constify the sha arguments for
ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: add a strbuf argument to ref_transaction_commit for error
logging
  refs.c: add an err argument to repack_without_refs
  refs.c: make ref_update_reject_duplicates take a strbuf argument for
errors
  refs.c: add an err argument ro delete_loose_ref
  refs.c: make update_ref_write

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-15 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 This patch series is based on next and expands on the transaction API.

Thanks.  Will pick up in v8 where I left off with v6.

Applies with just one minor conflict on top of a merge of
mh/ref-transaction, rs/ref-update-check-errors-early, and
rs/reflog-exists.  Here's an interdiff against version 6 for those
following along.

 Version 8:
  - Updates after review by JN
  - Improve commit messages
  - Add a patch that adds an err argument to repack_without_refs
  - Add a patch that adds an err argument to delete_loose_ref
  - Better document that a _update/_delete/_create failure means the whole
transaction has failed and needs rollback.

 Version 7:
  - Updated commit messages per JNs review comments.
  - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
exposed through refs.h

diff --git a/builtin/commit.c b/builtin/commit.c
index 0f4e1fc..07ccc97 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   0, !!current_head, sb.buf) ||
ref_transaction_commit(transaction, err)) {
rollback_index_files();
-   die(_(HEAD: cannot update ref: %s), err.buf);
+   die(%s, err.buf);
}
ref_transaction_free(transaction);
 
diff --git a/builtin/replace.c b/builtin/replace.c
index 47c360c..952b589 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
ref_transaction_update(transaction, ref, repl, prev,
   0, !is_null_sha1(prev), NULL) ||
ref_transaction_commit(transaction, err))
-   die(_(%s: failed to replace ref: %s), ref, err.buf);
+   die(%s: failed to replace ref: %s, ref, err.buf);
 
ref_transaction_free(transaction);
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c5aff92..bd7e96f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
 
if (ref_transaction_create(transaction, refname, new_sha1,
   update_flags, msg))
-   die(failed transaction create for %s, refname);
+   die(cannot create ref '%s', refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 302a2b3..ed93b75 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
return 0;
 }
 
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ *  pruned.
+ */
+#define REF_ISPRUNING  0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ *  need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
+
 /*
  * Try to read one refname component from the front of refname.  Return
  * the length of the component found, or -1 if the component is not
@@ -2447,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf 
*err)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
-   int i, removed = 0;
+   int i, ret, removed = 0;
 
/* Look for a packed ref */
for (i = 0; i  n; i++)
@@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames, int 
n)
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
+   if (err)
+   strbuf_addf(err, cannot delete '%s' from packed refs,
+   refnames[i]);
return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
@@ -2490,20 +2502,28 @@ static int repack_without_refs(const char **refnames, 
int n)
}
 
/* Write what remains */
-   return commit_packed_refs();
+   ret = commit_packed_refs();
+   if (ret  err)
+   strbuf_addf(err, unable to overwrite old ref-pack file);
+   return ret;
 }
 
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf 
*err)
 {
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
/* loose */
-   int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+   int res, i = strlen(lock-lk-filename) - 5; /* .lock */
 
lock-lk-filename[i] = 0;
-   err = unlink_or_warn(lock-lk-filename);
+   res = unlink_or_warn(lock-lk-filename);

Re: [PATCH v8 00/44] Use ref transactions for all ref updates

2014-05-15 Thread Junio C Hamano 
Jonathan Nieder jrnie...@gmail.com writes:

 Ronnie Sahlberg wrote:

 This patch series is based on next and expands on the transaction API.

 Thanks.  Will pick up in v8 where I left off with v6.

 Applies with just one minor conflict on top of a merge of
 mh/ref-transaction, rs/ref-update-check-errors-early, and
 rs/reflog-exists.  Here's an interdiff against version 6 for those
 following along.

The interdiffs look mostly sensible.  Thanks.

 Version 8:
  - Updates after review by JN
  - Improve commit messages
  - Add a patch that adds an err argument to repack_without_refs
  - Add a patch that adds an err argument to delete_loose_ref
  - Better document that a _update/_delete/_create failure means the whole
transaction has failed and needs rollback.

 Version 7:
  - Updated commit messages per JNs review comments.
  - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
exposed through refs.h

 diff --git a/builtin/commit.c b/builtin/commit.c
 index 0f4e1fc..07ccc97 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char 
 *prefix)
  0, !!current_head, sb.buf) ||
   ref_transaction_commit(transaction, err)) {
   rollback_index_files();
 - die(_(HEAD: cannot update ref: %s), err.buf);
 + die(%s, err.buf);
   }
   ref_transaction_free(transaction);
  
 diff --git a/builtin/replace.c b/builtin/replace.c
 index 47c360c..952b589 100644
 --- a/builtin/replace.c
 +++ b/builtin/replace.c
 @@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const 
 char *replace_ref,
   ref_transaction_update(transaction, ref, repl, prev,
  0, !is_null_sha1(prev), NULL) ||
   ref_transaction_commit(transaction, err))
 - die(_(%s: failed to replace ref: %s), ref, err.buf);
 + die(%s: failed to replace ref: %s, ref, err.buf);
  
   ref_transaction_free(transaction);
   return 0;
 diff --git a/builtin/update-ref.c b/builtin/update-ref.c
 index c5aff92..bd7e96f 100644
 --- a/builtin/update-ref.c
 +++ b/builtin/update-ref.c
 @@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, 
 const char *next)
  
   if (ref_transaction_create(transaction, refname, new_sha1,
  update_flags, msg))
 - die(failed transaction create for %s, refname);
 + die(cannot create ref '%s', refname);
  
   update_flags = 0;
   free(refname);
 diff --git a/refs.c b/refs.c
 index 302a2b3..ed93b75 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
   return 0;
  }
  
 +/** Used as a flag to ref_transaction_delete when a loose ref is beeing
 + *  pruned.
 + */
 +#define REF_ISPRUNING0x0100
 +/** Deletion of a ref that only exists as a packed ref in which case we do 
 not
 + *  need to lock the loose ref during the transaction.
 + */
 +#define REF_ISPACKONLY   0x0200
 +
  /*
   * Try to read one refname component from the front of refname.  Return
   * the length of the component found, or -1 if the component is not
 @@ -2447,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry 
 *entry, void *cb_data)
   return 0;
  }
  
 -static int repack_without_refs(const char **refnames, int n)
 +static int repack_without_refs(const char **refnames, int n, struct strbuf 
 *err)
  {
   struct ref_dir *packed;
   struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
   struct string_list_item *ref_to_delete;
 - int i, removed = 0;
 + int i, ret, removed = 0;
  
   /* Look for a packed ref */
   for (i = 0; i  n; i++)
 @@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames, 
 int n)
  
   if (lock_packed_refs(0)) {
   unable_to_lock_error(git_path(packed-refs), errno);
 + if (err)
 + strbuf_addf(err, cannot delete '%s' from packed refs,
 + refnames[i]);
   return error(cannot delete '%s' from packed refs, 
 refnames[i]);
   }
   packed = get_packed_refs(ref_cache);
 @@ -2490,20 +2502,28 @@ static int repack_without_refs(const char **refnames, 
 int n)
   }
  
   /* Write what remains */
 - return commit_packed_refs();
 + ret = commit_packed_refs();
 + if (ret  err)
 + strbuf_addf(err, unable to overwrite old ref-pack file);
 + return ret;
  }
  
 -static int delete_ref_loose(struct ref_lock *lock, int flag)
 +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf 
 *err)
  {
   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
   /* loose */
 - int err, i = strlen(lock-lk-filename) - 5; /* .lock */
 + int res, i = strlen(lock-lk-filename) - 5; /* .lock */