From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] refs.c: use a stringlist for repack_without_refs
Date: Tue, 18 Nov 2014 15:06:37 -0800 [thread overview]
Message-ID: <xmqqlhn8t8g2.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416350636-12934-1-git-send-email-sbeller@google.com> (Stefan Beller's message of "Tue, 18 Nov 2014 14:43:56 -0800")
Stefan Beller <sbeller@google•com> writes:
> This patch was heavily inspired by a part of the ref-transactions-rename
> series[1], but people tend to dislike large series and this part is
> relatively easy to take out and unrelated, so I'll send it as a single
> patch.
>
> This patch doesn't intend any functional changes. It is just a refactoring,
> which replaces a char** array by a stringlist in the function
> repack_without_refs.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> Idea-by: Ronnie Sahlberg <sahlberg@google•com>
> Signed-off-by: Stefan Beller <sbeller@google•com>
> ---
> builtin/remote.c | 22 +++++++---------------
> refs.c | 41 ++++++++++++++++++++---------------------
> refs.h | 3 +--
> 3 files changed, 28 insertions(+), 38 deletions(-)
In one codepath we were already using a string_list delete_refs_list
anyway, so it makes sense to reuse that by movingan existing call to
string_list_insert() a bit higher, instead of maintaining another
array of pointers delete_refs[] to strings.
OK, it simplifies the code by reducing the line count, which is a
plus ;-)
Sounds good.
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 7f28f92..dca4ebf 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
> static int remove_branches(struct string_list *branches)
> {
> struct strbuf err = STRBUF_INIT;
> - 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;
> - if (repack_without_refs(branch_names, branches->nr, &err))
> + if (repack_without_refs(branches, &err))
> result |= error("%s", err.buf);
> strbuf_release(&err);
> - free(branch_names);
>
> for (i = 0; i < branches->nr; i++) {
> struct string_list_item *item = branches->items + i;
> @@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
> int result = 0, i;
> struct ref_states states;
> struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
> - const char **delete_refs;
> const char *dangling_msg = dry_run
> ? _(" %s will become dangling!")
> : _(" %s has become dangling!");
> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
> memset(&states, 0, sizeof(states));
> get_remote_ref_states(remote, &states, GET_REF_STATES);
>
> + for (i = 0; i < states.stale.nr; i++)
> + string_list_insert(&delete_refs_list,
> + states.stale.items[i].util);
> +
> +
> if (states.stale.nr) {
> printf_ln(_("Pruning %s"), remote);
> printf_ln(_("URL: %s"),
> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
> ? 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) {
> struct strbuf err = STRBUF_INIT;
> - if (repack_without_refs(delete_refs, states.stale.nr,
> - &err))
> + if (repack_without_refs(&delete_refs_list, &err))
> result |= error("%s", err.buf);
> strbuf_release(&err);
> }
> - free(delete_refs);
> }
>
> for (i = 0; i < states.stale.nr; i++) {
> const char *refname = states.stale.items[i].util;
>
> - string_list_insert(&delete_refs_list, refname);
> -
> if (!dry_run)
> result |= delete_ref(refname, NULL, 0);
>
> diff --git a/refs.c b/refs.c
> index 5ff457e..2333a9b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
> return 0;
> }
>
> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
> +int repack_without_refs(struct string_list *without, 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, ret, removed = 0;
> + int count, ret, removed = 0;
>
> assert(err);
>
> - /* Look for a packed ref */
> - for (i = 0; i < n; i++)
> - if (get_packed_ref(refnames[i]))
> - break;
> + count = 0;
> + for_each_string_list_item(ref_to_delete, without)
> + if (get_packed_ref(ref_to_delete->string))
> + count++;
>
> - /* Avoid locking if we have nothing to do */
> - if (i == n)
> - return 0; /* no refname exists in packed refs */
> + /* No refname exists in packed refs */
> + if (!count)
> + return 0;
>
> if (lock_packed_refs(0)) {
> unable_to_lock_message(git_path("packed-refs"), errno, err);
> @@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
> packed = get_packed_refs(&ref_cache);
>
> /* Remove refnames from the cache */
> - for (i = 0; i < n; i++)
> - if (remove_entry(packed, refnames[i]) != -1)
> + for_each_string_list_item(ref_to_delete, without)
> + if (remove_entry(packed, ref_to_delete->string) != -1)
> removed = 1;
> if (!removed) {
> /*
> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
> int ref_transaction_commit(struct ref_transaction *transaction,
> struct strbuf *err)
> {
> - int ret = 0, delnum = 0, i;
> - const char **delnames;
> + int ret = 0, i;
> int n = transaction->nr;
> struct ref_update **updates = transaction->updates;
> + struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> + struct string_list_item *ref_to_delete;
>
> assert(err);
>
> @@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
> return 0;
> }
>
> - /* Allocate work space */
> - delnames = xmalloc(sizeof(*delnames) * n);
> -
> /* Copy, sort, and reject duplicate refs */
> qsort(updates, n, sizeof(*updates), ref_update_compare);
> if (ref_update_reject_duplicates(updates, n, err)) {
> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
> }
>
> if (!(update->flags & REF_ISPRUNING))
> - delnames[delnum++] = update->lock->ref_name;
> + string_list_insert(&refs_to_delete,
> + update->lock->ref_name);
> }
> }
>
> - if (repack_without_refs(delnames, delnum, err)) {
> + if (repack_without_refs(&refs_to_delete, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
> goto cleanup;
> }
> - for (i = 0; i < delnum; i++)
> - unlink_or_warn(git_path("logs/%s", delnames[i]));
> + for_each_string_list_item(ref_to_delete, &refs_to_delete)
> + unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
> clear_loose_ref_cache(&ref_cache);
>
> cleanup:
> @@ -3833,7 +3832,7 @@ cleanup:
> for (i = 0; i < n; i++)
> if (updates[i]->lock)
> unlock_ref(updates[i]->lock);
> - free(delnames);
> + string_list_clear(&refs_to_delete, 0);
> return ret;
> }
>
> diff --git a/refs.h b/refs.h
> index 2bc3556..0416e5f 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
> */
> int pack_refs(unsigned int flags);
>
> -extern int repack_without_refs(const char **refnames, int n,
> - struct strbuf *err);
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);
>
> extern int ref_exists(const char *);
next prev parent reply other threads:[~2014-11-18 23:06 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-18 22:43 [PATCH] refs.c: use a stringlist for repack_without_refs Stefan Beller
2014-11-18 23:06 ` Junio C Hamano [this message]
2014-11-18 23:39 ` Junio C Hamano
2014-11-18 23:45 ` Jonathan Nieder
2014-11-19 0:28 ` Stefan Beller
2014-11-19 1:08 ` Stefan Beller
2014-11-19 18:00 ` Junio C Hamano
2014-11-19 18:50 ` Stefan Beller
2014-11-19 20:44 ` Jonathan Nieder
2014-11-19 21:54 ` Stefan Beller
2014-11-19 21:59 ` [PATCH v4] " Stefan Beller
2014-11-20 2:15 ` Jonathan Nieder
2014-11-20 16:47 ` Junio C Hamano
2014-11-20 18:04 ` [PATCH v5 1/1] " Stefan Beller
2014-11-20 18:10 ` [PATCH] refs.c: repack_without_refs may be called without error string buffer Stefan Beller
2014-11-20 18:15 ` Ronnie Sahlberg
2014-11-20 18:35 ` Jonathan Nieder
2014-11-20 18:36 ` Ronnie Sahlberg
2014-11-20 18:56 ` Stefan Beller
2014-11-20 18:29 ` [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs Jonathan Nieder
2014-11-20 18:37 ` Jonathan Nieder
2014-11-20 19:01 ` Junio C Hamano
2014-11-20 19:05 ` Stefan Beller
2014-11-20 20:07 ` [PATCH v6] refs.c: use a string_list " Stefan Beller
2014-11-20 20:36 ` Jonathan Nieder
2014-11-21 14:09 ` [PATCH 0/6] repack_without_refs(): convert to string_list Michael Haggerty
2014-11-21 14:09 ` [PATCH 1/6] prune_remote(): exit early if there are no stale references Michael Haggerty
2014-11-22 21:07 ` Jonathan Nieder
2014-11-21 14:09 ` [PATCH 2/6] prune_remote(): initialize both delete_refs lists in a single loop Michael Haggerty
2014-11-21 14:09 ` [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse Michael Haggerty
2014-11-21 16:44 ` Junio C Hamano
2014-11-25 7:21 ` Michael Haggerty
2014-11-25 8:04 ` Michael Haggerty
2014-11-22 21:08 ` Jonathan Nieder
2014-11-21 14:09 ` [PATCH 4/6] repack_without_refs(): make the refnames argument a string_list Michael Haggerty
2014-11-22 21:17 ` Jonathan Nieder
2014-11-25 7:42 ` Michael Haggerty
2014-11-21 14:09 ` [PATCH 5/6] prune_remote(): rename local variable Michael Haggerty
2014-11-22 21:18 ` Jonathan Nieder
2014-11-21 14:09 ` [PATCH 6/6] prune_remote(): iterate using for_each_string_list_item() Michael Haggerty
2014-11-22 21:19 ` Jonathan Nieder
2014-11-21 14:25 ` [PATCH 0/6] repack_without_refs(): convert to string_list Michael Haggerty
2014-11-21 18:00 ` Junio C Hamano
2014-11-21 19:57 ` Stefan Beller
2014-11-25 0:28 ` Our cumbersome mailing list workflow (was: Re: [PATCH 0/6] repack_without_refs(): convert to string_list) Michael Haggerty
2014-11-27 17:46 ` Our cumbersome mailing list workflow Torsten Bögershausen
2014-11-27 18:24 ` Matthieu Moy
2014-11-28 12:09 ` Philip Oakley
2014-11-27 22:53 ` Eric Wong
2014-11-28 15:34 ` Michael Haggerty
2014-11-28 16:24 ` brian m. carlson
2014-12-01 2:46 ` Junio C Hamano
2014-12-03 2:20 ` Stefan Beller
2014-12-03 3:53 ` Jonathan Nieder
2014-12-03 17:18 ` Junio C Hamano
2014-12-03 17:28 ` Torsten Bögershausen
2014-11-28 14:31 ` Michael Haggerty
2014-11-28 15:42 ` Marc Branchaud
2014-11-28 21:39 ` Damien Robert
2014-12-03 23:57 ` Philip Oakley
2014-12-04 2:03 ` Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlhn8t8g2.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox