Re: [PATCH] fast-import: fix over-allocation of marks storage

[Junio C Hamano <gitster@pobox.com>]

"Dipl. Ing. Sergey Brester" <serg.brester@sebres.de> writes:

> May be this is a sign to introduce real issue tracker finally? :)
> No offence, but I was always wondering how a team is able to hold all
> the issue related stuff in form
> of a mailing list, without to experience such an "embarrassments".
> Especially on such large projects and communities.

I do not know if an issue-tracker would have helped, though.  The
issue was discovered and discussed there the day before:

  https://lore.kernel.org/git/xmqqimg5o5fq.fsf@gitster.c.googlers.com/

and then was discussed in other thread the next day.  Somehow the
discussion petered out without producing the final patch to be
applied.  For this particular case, what we need is a functioning
patch tracker *and* people who pay attention to patches in the "came
very close to conclusion but no final patch in the tree" state.  We
need people who can volunteer their eyeballs and attention to nudge,
prod and help patches to perfection, and that won't be me.

By the way, now I know why it looked familiar---the fix largely was
my code.  And the diff between Brian's from June and Peff's in this
thread is indeed quite small (shown below), which actually worries
me.  Was there something in the old attempt that was incomplete that
made us wait for the final finishing touches?  If so, is the current
round missing the same thing?  Or perhaps the test was what was
missing in the old attempt, in which case it's perfect (in the
attached diff, I excluded t/ directroy as the old fix didn't have
tests).

Thanks.

diff --git w/builtin/fast-import.c c/builtin/fast-import.c
index 71289b21e3..70d7d25eed 100644
--- w/builtin/fast-import.c
+++ c/builtin/fast-import.c
@@ -526,15 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)
 	return r;
 }
 
-static void insert_mark(struct mark_set **sp, uintmax_t idnum, struct object_entry *oe)
+static void insert_mark(struct mark_set **top, uintmax_t idnum, struct object_entry *oe)
 {
-	struct mark_set *s = *sp;
+	struct mark_set *s = *top;
 
 	while ((idnum >> s->shift) >= 1024) {
 		s = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
-		s->shift = (*sp)->shift + 10;
-		s->data.sets[0] = (*sp);
-		(*sp) = s;
+		s->shift = (*top)->shift + 10;
+		s->data.sets[0] = *top;
+		*top = s;
 	}
 	while (s->shift) {
 		uintmax_t i = idnum >> s->shift;
@@ -3323,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)
 	*f = '\0';
 	f++;
 	ms = xcalloc(1, sizeof(*ms));
-	string_list_insert(list, s)->util = ms;
 
 	fp = fopen(f, "r");
 	if (!fp)
 		die_errno("cannot read '%s'", f);
 	read_mark_file(&ms, fp, insert_oid_entry);
 	fclose(fp);
+
+	string_list_insert(list, s)->util = ms;
 }
 
 static int parse_one_option(const char *option)