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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2020, 18:35 UTC
Message-ID: <xmqqo8l3fibz.fsf@gitster.c.googlers.com>
In-Reply-To: <f3e4f7ccc36dc214201c1838acce4aff@sebres.de>

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

Show 5 quoted lines

> 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)