Re: [PATCH v2 2/3] remote: repack packed-refs once when deleting multiple refs
Junio C Hamano writes: >> -if (delete_ref(refname, sha1, 0)) >> +if (delete_ref(refname, NULL, 0)) >> result |= error(_("Could not remove branch %s"), >> refname); By the way, how does this series interact with what Ronnie and Michael are working on with their ref-transaction series? -- 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 v2 2/3] remote: repack packed-refs once when deleting multiple refs
Jens Lindström writes: > When 'git remote rm' or 'git remote prune' were used in a repository > with many refs, and needed to delete many remote-tracking refs, a lot > of time was spent deleting those refs since for each deleted ref, > repack_without_refs() was called to rewrite packed-refs without just > that deleted ref. > > To avoid this, call repack_without_refs() first to repack without all > the refs that will be deleted, before calling delete_ref() to delete > each one completely. The call to repack_without_ref() in delete_ref() > then becomes a no-op, since packed-refs already won't contain any of > the deleted refs. > > Signed-off-by: Jens Lindström > --- > Note: remove_branches() no longer checks that the remote-tracking > branches it deletes point at the right object before deleting them > by passing the expected SHA-1 to delete_ref(). This was a required > change since all packed refs have been deleted already by the time > we call delete_ref(), which causes delete_ref() to fail if given an > expected SHA-1 to check. 'remote prune' already behaved this way. > > builtin/remote.c | 20 ++-- > refs.c | 2 +- > refs.h | 2 ++ > 3 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index 84802cd..d33abe6 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -749,15 +749,23 @@ static int mv(int argc, const char **argv) > > static int remove_branches(struct string_list *branches) > { > + const char **branch_names; > int i, result = 0; > + > + branch_names = xmalloc(branches->nr * sizeof(*branch_names)); > + for (i = 0; i < branches->nr; i++) > + branch_names[i] = branches->items[i].string; > + result |= repack_without_refs(branch_names, branches->nr); > + free(branch_names); Hmph. I wonder if you can refactor/enhance the interface to the repack_without_refs() function before this step in the series, so that the function lets the caller to know if each ref was actually removed or it was not in packed-refs from the beginning, and also I wonder if such a refactoring helps. My gut feeling is that it would not help that much this particular series, but it would probably be a good thing to do in the longer run. So probably it is better to do without such a change as a part of this series. > for (i = 0; i < branches->nr; i++) { > struct string_list_item *item = branches->items + i; > const char *refname = item->string; > - unsigned char *sha1 = item->util; Here, you can check if the refname still exists as a ref; if it no longer exists, it would mean that the only copy of the ref was in the packed-refs file and you have already deleted it, and you can refrain from calling delete_ref() altogether, e.g. if (!ref_exists(refname)) continue; /* already removed the sole copy from packed-ref */ and then still retain the safetly against racing somebody else who created or updated the ref you wanted to remove here by passing the object name to delete_ref(). > - if (delete_ref(refname, sha1, 0)) > + if (delete_ref(refname, NULL, 0)) > result |= error(_("Could not remove branch %s"), > refname); > } > + > return result; > } -- 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 v2 2/3] remote: repack packed-refs once when deleting multiple refs
When 'git remote rm' or 'git remote prune' were used in a repository with many refs, and needed to delete many remote-tracking refs, a lot of time was spent deleting those refs since for each deleted ref, repack_without_refs() was called to rewrite packed-refs without just that deleted ref. To avoid this, call repack_without_refs() first to repack without all the refs that will be deleted, before calling delete_ref() to delete each one completely. The call to repack_without_ref() in delete_ref() then becomes a no-op, since packed-refs already won't contain any of the deleted refs. Signed-off-by: Jens Lindström --- Note: remove_branches() no longer checks that the remote-tracking branches it deletes point at the right object before deleting them by passing the expected SHA-1 to delete_ref(). This was a required change since all packed refs have been deleted already by the time we call delete_ref(), which causes delete_ref() to fail if given an expected SHA-1 to check. 'remote prune' already behaved this way. builtin/remote.c | 20 ++-- refs.c | 2 +- refs.h | 2 ++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index 84802cd..d33abe6 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -749,15 +749,23 @@ static int mv(int argc, const char **argv) static int remove_branches(struct string_list *branches) { + const char **branch_names; int i, result = 0; + + branch_names = xmalloc(branches->nr * sizeof(*branch_names)); + for (i = 0; i < branches->nr; i++) + branch_names[i] = branches->items[i].string; + result |= repack_without_refs(branch_names, branches->nr); + free(branch_names); + for (i = 0; i < branches->nr; i++) { struct string_list_item *item = branches->items + i; const char *refname = item->string; - unsigned char *sha1 = item->util; - if (delete_ref(refname, sha1, 0)) + if (delete_ref(refname, NULL, 0)) result |= error(_("Could not remove branch %s"), refname); } + return result; } @@ -1305,6 +1313,7 @@ static int prune_remote(const char *remote, int dry_run) { int result = 0, i; struct ref_states states; + const char **delete_refs; const char *dangling_msg = dry_run ? _(" %s will become dangling!") : _(" %s has become dangling!"); @@ -1318,6 +1327,13 @@ static int prune_remote(const char *remote, int dry_run) states.remote->url_nr ? states.remote->url[0] : _("(no URL)")); + + delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs)); + for (i = 0; i < states.stale.nr; i++) + delete_refs[i] = states.stale.items[i].util; + if (!dry_run) + result |= repack_without_refs(delete_refs, states.stale.nr); + free(delete_refs); } for (i = 0; i < states.stale.nr; i++) { diff --git a/refs.c b/refs.c index 28d5eca..262c1c2 100644 --- a/refs.c +++ b/refs.c @@ -2431,7 +2431,7 @@ 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) +int repack_without_refs(const char **refnames, int n) { struct ref_dir *packed; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; diff --git a/refs.h b/refs.h index 87a1a79..f287c7a 100644 --- a/refs.h +++ b/refs.h @@ -132,6 +132,8 @@ extern void rollback_packed_refs(void); */ int pack_refs(unsigned int flags); +extern int repack_without_refs(const char **refnames, int n); + extern int ref_exists(const char *); /* -- 1.9.1 -- 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