Re: [PATCH v2 20/24] fast-import: permit reading multiple marks files

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

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> In the future, we'll want to read marks files for submodules as well.
> Refactor the existing code to make it possible to read multiple marks
> files, each into their own marks set.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  fast-import.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)

This conversion looks a bit strange.  We are working off of 'marks'
that is file scope global, and the topmost caller passes the pointer
to that single instance down the callchain to this function (among
others) in "s", and it all looked no-op to my eye, but this actually
updates the global 'marks'.

> diff --git a/fast-import.c b/fast-import.c
> index b8b65a801c..b9ecd89699 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -493,25 +493,24 @@ static char *pool_strdup(const char *s)
>  	return r;
>  }
>  
> -static void insert_mark(uintmax_t idnum, struct object_entry *oe)
> +static void insert_mark(struct mark_set *s, uintmax_t idnum, struct object_entry *oe)
>  {
> -	struct mark_set *s = marks;

This is no longer needed as the caller gave the 'marks' to us in
's'.  Up to this line, this step is a no-op patch. 

>  	while ((idnum >> s->shift) >= 1024) {
>  		s = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));

Here we overwrite 's', and we lost the pointer the caller gave us!!!

>  		s->shift = marks->shift + 10;

And we still work off of global marks here and

>  		s->data.sets[0] = marks;
>  		marks = s;

try to swap it here.  But if our caller is also using 's' that was
passed from its caller, this assignment would not be seen by our
caller.  We'd need to pass a pointer to caller's 's' to this
function and update it around here.

>  	}
>  	while (s->shift) {
>  		uintmax_t i = idnum >> s->shift;
>  		idnum -= i << s->shift;
>  		if (!s->data.sets[i]) {
>  			s->data.sets[i] = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
>  			s->data.sets[i]->shift = s->shift - 10;
>  		}
>  		s = s->data.sets[i];
>  	}
>  	if (!s->data.marked[idnum])
>  		marks_set_count++;

I am not sure if this global also needs a separate counter and I
didn't bother to check.

>  	s->data.marked[idnum] = oe;
>  }

With this patch t9300 still seems to pass.

I do not know if this is the fix for Billes Tibor's "fast-import
oom" issue <c53bb69b-682d-3b47-4ed0-5f4559e69e37@gmx.com> but the
change to this function stood out.

Thanks.


 fast-import.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 0dfa14dc8c..f39b7890ac 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -534,13 +534,15 @@ static char *pool_strdup(const char *s)
 	return r;
 }
 
-static void insert_mark(struct mark_set *s, uintmax_t idnum, struct object_entry *oe)
+static void insert_mark(struct mark_set **sp, uintmax_t idnum, struct object_entry *oe)
 {
+	struct mark_set *s = *sp;
+
 	while ((idnum >> s->shift) >= 1024) {
 		s = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
-		s->shift = marks->shift + 10;
-		s->data.sets[0] = marks;
-		marks = s;
+		s->shift = (*sp)->shift + 10;
+		s->data.sets[0] = (*sp);
+		(*sp) = s;
 	}
 	while (s->shift) {
 		uintmax_t i = idnum >> s->shift;
@@ -958,7 +960,7 @@ static int store_object(
 
 	e = insert_object(&oid);
 	if (mark)
-		insert_mark(marks, mark, e);
+		insert_mark(&marks, mark, e);
 	if (e->idx.offset) {
 		duplicate_count_by_type[type]++;
 		return 1;
@@ -1156,7 +1158,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
 	e = insert_object(&oid);
 
 	if (mark)
-		insert_mark(marks, mark, e);
+		insert_mark(&marks, mark, e);
 
 	if (e->idx.offset) {
 		duplicate_count_by_type[OBJ_BLOB]++;
@@ -1745,12 +1747,12 @@ static void insert_object_entry(struct mark_set *s, struct object_id *oid, uintm
 		e->pack_id = MAX_PACK_ID;
 		e->idx.offset = 1; /* just not zero! */
 	}
-	insert_mark(s, mark, e);
+	insert_mark(&s, mark, e);
 }
 
 static void insert_oid_entry(struct mark_set *s, struct object_id *oid, uintmax_t mark)
 {
-	insert_mark(s, mark, xmemdupz(oid, sizeof(*oid)));
+	insert_mark(&s, mark, xmemdupz(oid, sizeof(*oid)));
 }
 
 static void read_mark_file(struct mark_set *s, FILE *f, mark_set_inserter_t inserter)
@@ -3242,7 +3244,7 @@ static void parse_alias(void)
 		die(_("Expected 'to' command, got %s"), command_buf.buf);
 	e = find_object(&b.oid);
 	assert(e);
-	insert_mark(marks, next_mark, e);
+	insert_mark(&marks, next_mark, e);
 }
 
 static char* make_fast_import_path(const char *path)