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: Wed, 19 Nov 2014 10:00:00 -0800 [thread overview]
Message-ID: <xmqq4mtvt6jj.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416359308-14831-1-git-send-email-sbeller@google.com> (Stefan Beller's message of "Tue, 18 Nov 2014 17:08:28 -0800")
Stefan Beller <sbeller@google•com> writes:
> 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.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code less pointers.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google•com>
> Signed-off-by: Stefan Beller <sbeller@google•com>
>
> -
Have three of them, not just one, here. (no need to resend to fix
only this). Or...
>
> 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.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> ---
... next time, write the comments here, where Git already gives you
three dashes.
Also mention what you updated and why relative to your earlier round
here, if not covered in the log message already.
For example, renaming of delete_refs_list (in v1) to delete_refs
(this version) is a sensible change because readers know it is a
list from its type being string_list already, but that change is new
relative to the codebase, so it could go to the log message ("Having
array delete_refs[] and string_list delete_refs_list is redundant;
drop the array and give the string_list variable the shorter name",
or something like that) if you wanted to.
> @@ -1316,8 +1311,8 @@ 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;
> + struct string_list delete_refs = STRING_LIST_INIT_NODUP;
> + struct string_list_item *ref;
> const char *dangling_msg = dry_run
> ? _(" %s will become dangling!")
> : _(" %s has become dangling!");
> @@ -1325,6 +1320,9 @@ 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_each_string_list_item(ref, &delete_refs)
> + string_list_append(&delete_refs, ref->string);
What are you trying to do here?
Initialise delete_refs to an empty string list, and then iterate
over its elements and append them into the same string list???
It looks like a "currently noop, waiting for somebody to throw an
item to the list before this code, at which time it turns into an
infinite memory eater".
Curious...
next prev parent reply other threads:[~2014-11-19 18:00 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
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 [this message]
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=xmqq4mtvt6jj.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