Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C
- From
Pranit Bauva <pranit.bauva@gmail.com>
- Date
- Nov 16, 2016, 16:49 UTC
- Message-ID
- <CAFZEwPMrC=7wJtUO+G3dcESPOJisFsrZ0_F6oKE8eDVGfBEVpg@mail.gmail.com>
- In-Reply-To
- <xmqq37istoay.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Wed, Nov 16, 2016 at 3:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
>
> Stephan Beyer <s-beyer@gmx.net> writes:
>
> >> +int bisect_clean_state(void)
> >> +{
> >> + int result = 0;
> >> +
> >> + /* There may be some refs packed during bisection */
> >> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> >> + for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
> >> + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> >> + result = delete_refs(&refs_for_removal, REF_NODEREF);
> >> + refs_for_removal.strdup_strings = 1;
> >> + string_list_clear(&refs_for_removal, 0);
> >
> > Does it have advantages to populate a list (with duplicated strings),
> > hand it to delete_refs(), and clear the list (and strings), instead of
> > just doing a single delete_ref() (or whatever name the singular function
> > has) in the callback?
>
> Depending on ref backends, removing multiple refs may be a lot more
> efficient than calling a single ref removal for the same set of
> refs, and the comment upfront I think hints that the code was
> written in the way exactly with that in mind. Removing N refs from
> a packed refs file will involve a loop that runs N times, each
> iteration loading the file, locating an entry among possibly 100s of
> refs to remove, and then rewriting the file.
>
> Besides, it is bad taste to delete each individual item being
> iterated over in an interator in general, isn't it?
>Not just that, deleting a ref inside for_each*() is illegal because it builds some kind of index and that is spoiled if anything is deleted in between. Thus it gives a seg fault. See this[1]. I did the same mistake when making this patch and I was confused about that was happening but then Michael Haggerty pointed this out[2].
[1]: https://github.com/git/git/blob/v2.11.0-rc1/refs.h#L183-L191 [2]: http://public-inbox.org/git/574D122F.7080608@alum.mit.edu/
Regards, Pranit Bauva