Re: [PATCH 07/13] prune_remote(): use delete_refs()
On Tue, Jun 09, 2015 at 12:50:13PM +0200, Michael Haggerty wrote: > The new code (in delete_refs()) allows delete_ref() to emit its error, > but then follows it up with > > error(_("could not remove reference %s"), refname) > > The "could not remove reference" error originally came from a similar > message in remove_branches() (from builtin/remote.c). > > I *think* this is an improvement, because the error from delete_ref() > (which usually comes from ref_transaction_commit()) can be pretty > low-level, like > > Cannot lock ref '%s': unable to resolve reference %s: %s > > where the last "%s" is the original strerror. > > I would be happy to change the behavior if somebody has a concrete wish. > At the same time I don't think we need to sweat the details too much, > because these errors should only ever be seen in the case of a broken > repository or a race between two processes; i.e., only in pretty rare > and anomalous situations. Thanks for the explanation. I agree it probably doesn't matter much either way. -Peff -- 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 07/13] prune_remote(): use delete_refs()
On 06/08/2015 07:12 PM, Jeff King wrote: > On Mon, Jun 08, 2015 at 09:57:04AM -0700, Stefan Beller wrote: > >> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty >> wrote: >>> This will result in errors being emitted for references that can't be >>> deleted, but that is a good thing. >> >> This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!". > > I think the argument is "we failed to do that the user asked for, but > never reported the reason why". > > But I don't see how that is the case. We already complain if > repack_without_refs fail, and AFAICT the original call to delete_ref() > would emit an error, as well. The old and new code report errors that come from repack_without_refs() the same way. The difference is how they report errors within delete_ref(). The old code only allowed delete_ref() to emit its error. The new code (in delete_refs()) allows delete_ref() to emit its error, but then follows it up with error(_("could not remove reference %s"), refname) The "could not remove reference" error originally came from a similar message in remove_branches() (from builtin/remote.c). I *think* this is an improvement, because the error from delete_ref() (which usually comes from ref_transaction_commit()) can be pretty low-level, like Cannot lock ref '%s': unable to resolve reference %s: %s where the last "%s" is the original strerror. I would be happy to change the behavior if somebody has a concrete wish. At the same time I don't think we need to sweat the details too much, because these errors should only ever be seen in the case of a broken repository or a race between two processes; i.e., only in pretty rare and anomalous situations. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 07/13] prune_remote(): use delete_refs()
On Mon, Jun 08, 2015 at 09:57:04AM -0700, Stefan Beller wrote: > On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty wrote: > > This will result in errors being emitted for references that can't be > > deleted, but that is a good thing. > > This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!". I think the argument is "we failed to do that the user asked for, but never reported the reason why". But I don't see how that is the case. We already complain if repack_without_refs fail, and AFAICT the original call to delete_ref() would emit an error, as well. -Peff -- 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 07/13] prune_remote(): use delete_refs()
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty wrote: > This will result in errors being emitted for references that can't be > deleted, but that is a good thing. This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!". > > Signed-off-by: Michael Haggerty > --- > builtin/remote.c | 11 ++- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index c8dc724..cc3c741 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int > dry_run) > string_list_append(&refs_to_prune, item->util); > string_list_sort(&refs_to_prune); > > - if (!dry_run) { > - struct strbuf err = STRBUF_INIT; > - if (repack_without_refs(&refs_to_prune, &err)) > - result |= error("%s", err.buf); > - strbuf_release(&err); > - } > + if (!dry_run) > + result |= delete_refs(&refs_to_prune); > > for_each_string_list_item(item, &states.stale) { > const char *refname = item->util; > > - if (!dry_run) > - result |= delete_ref(refname, NULL, 0); > - > if (dry_run) > printf_ln(_(" * [would prune] %s"), >abbrev_ref(refname, "refs/remotes/")); > -- > 2.1.4 > -- 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 07/13] prune_remote(): use delete_refs()
This will result in errors being emitted for references that can't be deleted, but that is a good thing. Signed-off-by: Michael Haggerty --- builtin/remote.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index c8dc724..cc3c741 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int dry_run) string_list_append(&refs_to_prune, item->util); string_list_sort(&refs_to_prune); - if (!dry_run) { - struct strbuf err = STRBUF_INIT; - if (repack_without_refs(&refs_to_prune, &err)) - result |= error("%s", err.buf); - strbuf_release(&err); - } + if (!dry_run) + result |= delete_refs(&refs_to_prune); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; - if (!dry_run) - result |= delete_ref(refname, NULL, 0); - if (dry_run) printf_ln(_(" * [would prune] %s"), abbrev_ref(refname, "refs/remotes/")); -- 2.1.4 -- 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