[PATCH 0/5] streaming.c: refactor for smaller + easier to understand code

[PATCH 0/5] streaming.c: refactor for smaller + easier to understand code

[Ævar Arnfjörð Bjarmason]

This is a prep series for my yet-to-be-sent re-roll of [1],
aka. ab/fsck-unexpected-type.

Jeff King had a comment in [2] saying it was odd to have an "oi2"
variable, that was because there was on an "oi" name already defined
via the new-gone open_method_decl(loose) macro. As we'll see we could
do without the initial "oi" and a few other types of indirection in
this interface.

Junio: Could you eject ab/fsck-unexpected-type while this is being
considered/cooked? I didn't really see how to address Jeff's feedback
about that variable name in a way that wouldn't just make something
like this refactoring part of an even bigger series. I think ejecting
the functional changes & trying to get this in first is the least
worst approach at this point. Eventually I'll submit a re-roll of
ab/fsck-unexpected-type either based on this, or master (if it's
landed already).

1. https://lore.kernel.org/git/cover-0.6-00000000000-20210413T093734Z-avarab@gmail.com/#t
2. https://lore.kernel.org/git/YILZHiuUyj0mt958@coredump.intra.peff.net/

Ævar Arnfjörð Bjarmason (5):
  streaming.c: avoid forward declarations
  streaming.c: remove enum/function/vtbl indirection
  streaming.c: remove {open,close,read}_method_decl() macros
  streaming.c: stop passing around "object_info *" to open()
  streaming.c: move {open,close,read} from vtable to "struct
    git_istream"

 streaming.c | 268 ++++++++++++++++++++++------------------------------
 1 file changed, 115 insertions(+), 153 deletions(-)

-- 
2.31.1.838.g7ac6e98bb53

[PATCH 1/5] streaming.c: avoid forward declarations

[Ævar Arnfjörð Bjarmason]

Change code added in 46bf043807c (streaming: a new API to read from
the object store, 2011-05-11) to avoid forward declarations of the
functions it uses. We can instead move this code to the bottom of the
file, and thus avoid the open_method_decl() calls.

Aside from the addition of the "static helpers[...]" comment being
added here, and the removal of the forward declarations this is a
move-only change.

The style of the added "static helpers[...]"  comment isn't in line
with our usual coding style, but is consistent with several other
comments used in this file, so let's use that style consistently here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 streaming.c | 171 +++++++++++++++++++++++++---------------------------
 1 file changed, 83 insertions(+), 88 deletions(-)

diff --git a/streaming.c b/streaming.c
index 800f07a52cc..b0d439e47a7 100644
--- a/streaming.c
+++ b/streaming.c
@@ -42,20 +42,6 @@ struct stream_vtbl {
 	ssize_t read_istream_ ##name \
 	(struct git_istream *st, char *buf, size_t sz)
 
-/* forward declaration */
-static open_method_decl(incore);
-static open_method_decl(loose);
-static open_method_decl(pack_non_delta);
-static struct git_istream *attach_stream_filter(struct git_istream *st,
-						struct stream_filter *filter);
-
-
-static open_istream_fn open_istream_tbl[] = {
-	open_istream_incore,
-	open_istream_loose,
-	open_istream_pack_non_delta,
-};
-
 #define FILTER_BUFFER (1024*16)
 
 struct filtered_istream {
@@ -97,80 +83,6 @@ struct git_istream {
 	} u;
 };
 
-int close_istream(struct git_istream *st)
-{
-	int r = st->vtbl->close(st);
-	free(st);
-	return r;
-}
-
-ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
-{
-	return st->vtbl->read(st, buf, sz);
-}
-
-static enum input_source istream_source(struct repository *r,
-					const struct object_id *oid,
-					enum object_type *type,
-					struct object_info *oi)
-{
-	unsigned long size;
-	int status;
-
-	oi->typep = type;
-	oi->sizep = &size;
-	status = oid_object_info_extended(r, oid, oi, 0);
-	if (status < 0)
-		return stream_error;
-
-	switch (oi->whence) {
-	case OI_LOOSE:
-		return loose;
-	case OI_PACKED:
-		if (!oi->u.packed.is_delta && big_file_threshold < size)
-			return pack_non_delta;
-		/* fallthru */
-	default:
-		return incore;
-	}
-}
-
-struct git_istream *open_istream(struct repository *r,
-				 const struct object_id *oid,
-				 enum object_type *type,
-				 unsigned long *size,
-				 struct stream_filter *filter)
-{
-	struct git_istream *st;
-	struct object_info oi = OBJECT_INFO_INIT;
-	const struct object_id *real = lookup_replace_object(r, oid);
-	enum input_source src = istream_source(r, real, type, &oi);
-
-	if (src < 0)
-		return NULL;
-
-	st = xmalloc(sizeof(*st));
-	if (open_istream_tbl[src](st, r, &oi, real, type)) {
-		if (open_istream_incore(st, r, &oi, real, type)) {
-			free(st);
-			return NULL;
-		}
-	}
-	if (filter) {
-		/* Add "&& !is_null_stream_filter(filter)" for performance */
-		struct git_istream *nst = attach_stream_filter(st, filter);
-		if (!nst) {
-			close_istream(st);
-			return NULL;
-		}
-		st = nst;
-	}
-
-	*size = st->size;
-	return st;
-}
-
-
 /*****************************************************************
  *
  * Common helpers
@@ -508,11 +420,94 @@ static open_method_decl(incore)
 	return st->u.incore.buf ? 0 : -1;
 }
 
+/*****************************************************************************
+ * static helpers variables and functions for users of streaming interface
+ *****************************************************************************/
+
+static open_istream_fn open_istream_tbl[] = {
+	open_istream_incore,
+	open_istream_loose,
+	open_istream_pack_non_delta,
+};
+
+static enum input_source istream_source(struct repository *r,
+					const struct object_id *oid,
+					enum object_type *type,
+					struct object_info *oi)
+{
+	unsigned long size;
+	int status;
+
+	oi->typep = type;
+	oi->sizep = &size;
+	status = oid_object_info_extended(r, oid, oi, 0);
+	if (status < 0)
+		return stream_error;
+
+	switch (oi->whence) {
+	case OI_LOOSE:
+		return loose;
+	case OI_PACKED:
+		if (!oi->u.packed.is_delta && big_file_threshold < size)
+			return pack_non_delta;
+		/* fallthru */
+	default:
+		return incore;
+	}
+}
+
 
 /****************************************************************
  * Users of streaming interface
  ****************************************************************/
 
+int close_istream(struct git_istream *st)
+{
+	int r = st->vtbl->close(st);
+	free(st);
+	return r;
+}
+
+ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
+{
+	return st->vtbl->read(st, buf, sz);
+}
+
+struct git_istream *open_istream(struct repository *r,
+				 const struct object_id *oid,
+				 enum object_type *type,
+				 unsigned long *size,
+				 struct stream_filter *filter)
+{
+	struct git_istream *st;
+	struct object_info oi = OBJECT_INFO_INIT;
+	const struct object_id *real = lookup_replace_object(r, oid);
+	enum input_source src = istream_source(r, real, type, &oi);
+
+	if (src < 0)
+		return NULL;
+
+	st = xmalloc(sizeof(*st));
+	if (open_istream_tbl[src](st, r, &oi, real, type)) {
+		if (open_istream_incore(st, r, &oi, real, type)) {
+			free(st);
+			return NULL;
+		}
+	}
+	if (filter) {
+		/* Add "&& !is_null_stream_filter(filter)" for performance */
+		struct git_istream *nst = attach_stream_filter(st, filter);
+		if (!nst) {
+			close_istream(st);
+			return NULL;
+		}
+		st = nst;
+	}
+
+	*size = st->size;
+	return st;
+}
+
 int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter *filter,
 		      int can_seek)
 {
-- 
2.31.1.838.g7ac6e98bb53

[PATCH 2/5] streaming.c: remove enum/function/vtbl indirection

[Ævar Arnfjörð Bjarmason]

Remove the indirection of discovering a function pointer to use via an
enum and virtual table. This refactors code added in
46bf043807c (streaming: a new API to read from the object store,
2011-05-11).

We can instead simply return an "open_istream_fn" for use from the
"istream_source()" selector function directly. This allows us to get
rid of the "incore", "loose" and "pack_non_delta" enum
variables. We'll return the functions instead.

The "stream_error" variable in that enum can likewise go in favor of
returning NULL, which is what the open_istream() was doing when it got
that value anyway.

We can thus remove the entire enum, and the "open_istream_tbl" virtual
table that (indirectly) referenced it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 streaming.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/streaming.c b/streaming.c
index b0d439e47a7..628369519b9 100644
--- a/streaming.c
+++ b/streaming.c
@@ -8,13 +8,6 @@
 #include "replace-object.h"
 #include "packfile.h"
 
-enum input_source {
-	stream_error = -1,
-	incore = 0,
-	loose = 1,
-	pack_non_delta = 2
-};
-
 typedef int (*open_istream_fn)(struct git_istream *,
 			       struct repository *,
 			       struct object_info *,
@@ -424,16 +417,10 @@ static open_method_decl(incore)
  * static helpers variables and functions for users of streaming interface
  *****************************************************************************/
 
-static open_istream_fn open_istream_tbl[] = {
-	open_istream_incore,
-	open_istream_loose,
-	open_istream_pack_non_delta,
-};
-
-static enum input_source istream_source(struct repository *r,
-					const struct object_id *oid,
-					enum object_type *type,
-					struct object_info *oi)
+static open_istream_fn istream_source(struct repository *r,
+				      const struct object_id *oid,
+				      enum object_type *type,
+				      struct object_info *oi)
 {
 	unsigned long size;
 	int status;
@@ -442,21 +429,20 @@ static enum input_source istream_source(struct repository *r,
 	oi->sizep = &size;
 	status = oid_object_info_extended(r, oid, oi, 0);
 	if (status < 0)
-		return stream_error;
+		return NULL;
 
 	switch (oi->whence) {
 	case OI_LOOSE:
-		return loose;
+		return open_istream_loose;
 	case OI_PACKED:
 		if (!oi->u.packed.is_delta && big_file_threshold < size)
-			return pack_non_delta;
+			return open_istream_pack_non_delta;
 		/* fallthru */
 	default:
-		return incore;
+		return open_istream_incore;
 	}
 }
 
-
 /****************************************************************
  * Users of streaming interface
  ****************************************************************/
@@ -482,13 +468,13 @@ struct git_istream *open_istream(struct repository *r,
 	struct git_istream *st;
 	struct object_info oi = OBJECT_INFO_INIT;
 	const struct object_id *real = lookup_replace_object(r, oid);
-	enum input_source src = istream_source(r, real, type, &oi);
+	open_istream_fn open_fn = istream_source(r, real, type, &oi);
 
-	if (src < 0)
+	if (!open_fn)
 		return NULL;
 
 	st = xmalloc(sizeof(*st));
-	if (open_istream_tbl[src](st, r, &oi, real, type)) {
+	if (open_fn(st, r, &oi, real, type)) {
 		if (open_istream_incore(st, r, &oi, real, type)) {
 			free(st);
 			return NULL;
-- 
2.31.1.838.g7ac6e98bb53

Re: [PATCH 2/5] streaming.c: remove enum/function/vtbl indirection

[Jeff King]

On Wed, May 05, 2021 at 02:33:29PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Remove the indirection of discovering a function pointer to use via an
> enum and virtual table. This refactors code added in
> 46bf043807c (streaming: a new API to read from the object store,
> 2011-05-11).
> 
> We can instead simply return an "open_istream_fn" for use from the
> "istream_source()" selector function directly. This allows us to get
> rid of the "incore", "loose" and "pack_non_delta" enum
> variables. We'll return the functions instead.
> 
> The "stream_error" variable in that enum can likewise go in favor of
> returning NULL, which is what the open_istream() was doing when it got
> that value anyway.
> 
> We can thus remove the entire enum, and the "open_istream_tbl" virtual
> table that (indirectly) referenced it.

Yeah, I think this is simpler. The value of the vtable was that we might
have added more functions to it, but we haven't done so over the course
of the last 10 years. And I have trouble imagining for what purpose we
would. So it seems like unnecessary complexity.

-Peff

Re: [PATCH 2/5] streaming.c: remove enum/function/vtbl indirection

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> We can thus remove the entire enum, and the "open_istream_tbl" virtual
>> table that (indirectly) referenced it.
>
> Yeah, I think this is simpler. The value of the vtable was that we might
> have added more functions to it, but we haven't done so over the course
> of the last 10 years. And I have trouble imagining for what purpose we
> would. So it seems like unnecessary complexity.

The code hasn't changed all that much since it was written, so
another equally plausible way forward is not to touch it at all ;-),
but I am fine with this simplification, too.

[PATCH 3/5] streaming.c: remove {open,close,read}_method_decl() macros

[Ævar Arnfjörð Bjarmason]

Remove the {open,close,read}_method_decl() macros added in
46bf043807c (streaming: a new API to read from the object store,
2011-05-11) in favor of inlining the definition of the arguments of
these functions.

Since we'll end up using them via the "{open,close,read}_istream_fn"
types we don't gain anything in the way of compiler checking by using
these macros, and as of preceding commits we no longer need to declare
these argument lists twice. So declaring them at a distance just
serves to make the code less readable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 streaming.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/streaming.c b/streaming.c
index 628369519b9..7e039df388b 100644
--- a/streaming.c
+++ b/streaming.c
@@ -21,20 +21,6 @@ struct stream_vtbl {
 	read_istream_fn read;
 };
 
-#define open_method_decl(name) \
-	int open_istream_ ##name \
-	(struct git_istream *st, struct repository *r, \
-	 struct object_info *oi, const struct object_id *oid, \
-	 enum object_type *type)
-
-#define close_method_decl(name) \
-	int close_istream_ ##name \
-	(struct git_istream *st)
-
-#define read_method_decl(name) \
-	ssize_t read_istream_ ##name \
-	(struct git_istream *st, char *buf, size_t sz)
-
 #define FILTER_BUFFER (1024*16)
 
 struct filtered_istream {
@@ -95,13 +81,14 @@ static void close_deflated_stream(struct git_istream *st)
  *
  *****************************************************************/
 
-static close_method_decl(filtered)
+static int close_istream_filtered(struct git_istream *st)
 {
 	free_stream_filter(st->u.filtered.filter);
 	return close_istream(st->u.filtered.upstream);
 }
 
-static read_method_decl(filtered)
+static ssize_t read_istream_filtered(struct git_istream *st, char *buf,
+				     size_t sz)
 {
 	struct filtered_istream *fs = &(st->u.filtered);
 	size_t filled = 0;
@@ -187,7 +174,7 @@ static struct git_istream *attach_stream_filter(struct git_istream *st,
  *
  *****************************************************************/
 
-static read_method_decl(loose)
+static ssize_t read_istream_loose(struct git_istream *st, char *buf, size_t sz)
 {
 	size_t total_read = 0;
 
@@ -232,7 +219,7 @@ static read_method_decl(loose)
 	return total_read;
 }
 
-static close_method_decl(loose)
+static int close_istream_loose(struct git_istream *st)
 {
 	close_deflated_stream(st);
 	munmap(st->u.loose.mapped, st->u.loose.mapsize);
@@ -244,7 +231,10 @@ static struct stream_vtbl loose_vtbl = {
 	read_istream_loose,
 };
 
-static open_method_decl(loose)
+static int open_istream_loose(struct git_istream *st, struct repository *r,
+			      struct object_info *oi,
+			      const struct object_id *oid,
+			      enum object_type *type)
 {
 	st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);
 	if (!st->u.loose.mapped)
@@ -275,7 +265,8 @@ static open_method_decl(loose)
  *
  *****************************************************************/
 
-static read_method_decl(pack_non_delta)
+static ssize_t read_istream_pack_non_delta(struct git_istream *st, char *buf,
+					   size_t sz)
 {
 	size_t total_read = 0;
 
@@ -333,7 +324,7 @@ static read_method_decl(pack_non_delta)
 	return total_read;
 }
 
-static close_method_decl(pack_non_delta)
+static int close_istream_pack_non_delta(struct git_istream *st)
 {
 	close_deflated_stream(st);
 	return 0;
@@ -344,7 +335,11 @@ static struct stream_vtbl pack_non_delta_vtbl = {
 	read_istream_pack_non_delta,
 };
 
-static open_method_decl(pack_non_delta)
+static int open_istream_pack_non_delta(struct git_istream *st,
+				       struct repository *r,
+				       struct object_info *oi,
+				       const struct object_id *oid,
+				       enum object_type *type)
 {
 	struct pack_window *window;
 	enum object_type in_pack_type;
@@ -379,13 +374,13 @@ static open_method_decl(pack_non_delta)
  *
  *****************************************************************/
 
-static close_method_decl(incore)
+static int close_istream_incore(struct git_istream *st)
 {
 	free(st->u.incore.buf);
 	return 0;
 }
 
-static read_method_decl(incore)
+static ssize_t read_istream_incore(struct git_istream *st, char *buf, size_t sz)
 {
 	size_t read_size = sz;
 	size_t remainder = st->size - st->u.incore.read_ptr;
@@ -404,7 +399,9 @@ static struct stream_vtbl incore_vtbl = {
 	read_istream_incore,
 };
 
-static open_method_decl(incore)
+static int open_istream_incore(struct git_istream *st, struct repository *r,
+			   struct object_info *oi, const struct object_id *oid,
+			   enum object_type *type)
 {
 	st->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);
 	st->u.incore.read_ptr = 0;
-- 
2.31.1.838.g7ac6e98bb53

Re: [PATCH 3/5] streaming.c: remove {open,close,read}_method_decl() macros

[Jeff King]

On Wed, May 05, 2021 at 02:33:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Remove the {open,close,read}_method_decl() macros added in
> 46bf043807c (streaming: a new API to read from the object store,
> 2011-05-11) in favor of inlining the definition of the arguments of
> these functions.
> 
> Since we'll end up using them via the "{open,close,read}_istream_fn"
> types we don't gain anything in the way of compiler checking by using
> these macros, and as of preceding commits we no longer need to declare
> these argument lists twice. So declaring them at a distance just
> serves to make the code less readable.

Heh. I have a very similar patch pending. In addition to readability, my
reasons there are:

  - you can't find the functions with ctags, etc, when they're hidden
    behind macros

  - you can't annotate the function interfaces to avoid
    -Wunused-parameter warnings. :)

So I am very much in favor of this (and patch 1 is nice here, too,
because it skips an extra time you'd have to repeat the interface in the
forward declaration).

-Peff

[PATCH 4/5] streaming.c: stop passing around "object_info *" to open()

[Ævar Arnfjörð Bjarmason]

Change the streaming interface to stop passing around the "struct
object_info" the open() functions.

As seen in 7ef2d9a2604 (streaming: read non-delta incrementally from a
pack, 2011-05-13) which introduced the "st->u.in_pack" assignments
being changed here only the open_istream_pack_non_delta() path need
these.

So let's instead do this when preparing the selected callback in the
istream_source() function. This might also allow the compiler to
reduce the lifetime of the "oi" variable, as we've moved it from
"git_istream()" to "istream_source()".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 streaming.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/streaming.c b/streaming.c
index 7e039df388b..f6059ee828e 100644
--- a/streaming.c
+++ b/streaming.c
@@ -10,7 +10,6 @@
 
 typedef int (*open_istream_fn)(struct git_istream *,
 			       struct repository *,
-			       struct object_info *,
 			       const struct object_id *,
 			       enum object_type *);
 typedef int (*close_istream_fn)(struct git_istream *);
@@ -232,7 +231,6 @@ static struct stream_vtbl loose_vtbl = {
 };
 
 static int open_istream_loose(struct git_istream *st, struct repository *r,
-			      struct object_info *oi,
 			      const struct object_id *oid,
 			      enum object_type *type)
 {
@@ -337,15 +335,12 @@ static struct stream_vtbl pack_non_delta_vtbl = {
 
 static int open_istream_pack_non_delta(struct git_istream *st,
 				       struct repository *r,
-				       struct object_info *oi,
 				       const struct object_id *oid,
 				       enum object_type *type)
 {
 	struct pack_window *window;
 	enum object_type in_pack_type;
 
-	st->u.in_pack.pack = oi->u.packed.pack;
-	st->u.in_pack.pos = oi->u.packed.offset;
 	window = NULL;
 
 	in_pack_type = unpack_object_header(st->u.in_pack.pack,
@@ -400,8 +395,7 @@ static struct stream_vtbl incore_vtbl = {
 };
 
 static int open_istream_incore(struct git_istream *st, struct repository *r,
-			   struct object_info *oi, const struct object_id *oid,
-			   enum object_type *type)
+			       const struct object_id *oid, enum object_type *type)
 {
 	st->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);
 	st->u.incore.read_ptr = 0;
@@ -414,26 +408,30 @@ static int open_istream_incore(struct git_istream *st, struct repository *r,
  * static helpers variables and functions for users of streaming interface
  *****************************************************************************/
 
-static open_istream_fn istream_source(struct repository *r,
+static open_istream_fn istream_source(struct git_istream *st,
+				      struct repository *r,
 				      const struct object_id *oid,
-				      enum object_type *type,
-				      struct object_info *oi)
+				      enum object_type *type)
 {
 	unsigned long size;
 	int status;
+	struct object_info oi = OBJECT_INFO_INIT;
 
-	oi->typep = type;
-	oi->sizep = &size;
-	status = oid_object_info_extended(r, oid, oi, 0);
+	oi.typep = type;
+	oi.sizep = &size;
+	status = oid_object_info_extended(r, oid, &oi, 0);
 	if (status < 0)
 		return NULL;
 
-	switch (oi->whence) {
+	switch (oi.whence) {
 	case OI_LOOSE:
 		return open_istream_loose;
 	case OI_PACKED:
-		if (!oi->u.packed.is_delta && big_file_threshold < size)
+		if (!oi.u.packed.is_delta && big_file_threshold < size) {
+			st->u.in_pack.pack = oi.u.packed.pack;
+			st->u.in_pack.pos = oi.u.packed.offset;
 			return open_istream_pack_non_delta;
+		}
 		/* fallthru */
 	default:
 		return open_istream_incore;
@@ -462,17 +460,17 @@ struct git_istream *open_istream(struct repository *r,
 				 unsigned long *size,
 				 struct stream_filter *filter)
 {
-	struct git_istream *st;
-	struct object_info oi = OBJECT_INFO_INIT;
+	struct git_istream *st = xmalloc(sizeof(*st));
 	const struct object_id *real = lookup_replace_object(r, oid);
-	open_istream_fn open_fn = istream_source(r, real, type, &oi);
+	open_istream_fn open_fn = istream_source(st, r, real, type);
 
-	if (!open_fn)
+	if (!open_fn) {
+		free(st);
 		return NULL;
+	}
 
-	st = xmalloc(sizeof(*st));
-	if (open_fn(st, r, &oi, real, type)) {
-		if (open_istream_incore(st, r, &oi, real, type)) {
+	if (open_fn(st, r, real, type)) {
+		if (open_istream_incore(st, r, real, type)) {
 			free(st);
 			return NULL;
 		}
-- 
2.31.1.838.g7ac6e98bb53

Re: [PATCH 4/5] streaming.c: stop passing around "object_info *" to open()

[Jeff King]

On Wed, May 05, 2021 at 02:33:31PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Change the streaming interface to stop passing around the "struct
> object_info" the open() functions.
> 
> As seen in 7ef2d9a2604 (streaming: read non-delta incrementally from a
> pack, 2011-05-13) which introduced the "st->u.in_pack" assignments
> being changed here only the open_istream_pack_non_delta() path need
> these.
> 
> So let's instead do this when preparing the selected callback in the
> istream_source() function. This might also allow the compiler to
> reduce the lifetime of the "oi" variable, as we've moved it from
> "git_istream()" to "istream_source()".

OK. This blurs the lines a bit between what the generic istream_source()
is responsible for, versus the type-specific helpers. I think that works
a bit against the original design, which was trying to be very abstract
and polymorphic. But that doesn't seem to have bought us much. Just
considering the whole set of streaming code as a module with helper
functions is a bit simpler and more usual for our codebase.

So I'm OK with it unless Junio (as the original author) has strong
objections.

-Peff

[PATCH 5/5] streaming.c: move {open,close,read} from vtable to "struct git_istream"

[Ævar Arnfjörð Bjarmason]

Move the definition of the structure around the open/close/read
functions introduced in 46bf043807c (streaming: a new API to read from
the object store, 2011-05-11) to instead populate "close" and "read"
members in the "struct git_istream".

This gets us rid of an extra pointer deference, and I think makes more
sense. The "close" and "read" functions are the primary interface to
the stream itself.

Let's also populate a "open" callback in the same struct. That's now
used by open_istream() after istream_source() decides what "open"
function should be used. This isn't needed to get rid of the
"stream_vtbl" variables, but makes sense for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 streaming.c | 72 +++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

diff --git a/streaming.c b/streaming.c
index f6059ee828e..5f480ad50c4 100644
--- a/streaming.c
+++ b/streaming.c
@@ -15,11 +15,6 @@ typedef int (*open_istream_fn)(struct git_istream *,
 typedef int (*close_istream_fn)(struct git_istream *);
 typedef ssize_t (*read_istream_fn)(struct git_istream *, char *, size_t);
 
-struct stream_vtbl {
-	close_istream_fn close;
-	read_istream_fn read;
-};
-
 #define FILTER_BUFFER (1024*16)
 
 struct filtered_istream {
@@ -33,7 +28,10 @@ struct filtered_istream {
 };
 
 struct git_istream {
-	const struct stream_vtbl *vtbl;
+	open_istream_fn open;
+	close_istream_fn close;
+	read_istream_fn read;
+
 	unsigned long size; /* inflated size of full object */
 	git_zstream z;
 	enum { z_unused, z_used, z_done, z_error } z_state;
@@ -146,18 +144,14 @@ static ssize_t read_istream_filtered(struct git_istream *st, char *buf,
 	return filled;
 }
 
-static struct stream_vtbl filtered_vtbl = {
-	close_istream_filtered,
-	read_istream_filtered,
-};
-
 static struct git_istream *attach_stream_filter(struct git_istream *st,
 						struct stream_filter *filter)
 {
 	struct git_istream *ifs = xmalloc(sizeof(*ifs));
 	struct filtered_istream *fs = &(ifs->u.filtered);
 
-	ifs->vtbl = &filtered_vtbl;
+	ifs->close = close_istream_filtered;
+	ifs->read = read_istream_filtered;
 	fs->upstream = st;
 	fs->filter = filter;
 	fs->i_end = fs->i_ptr = 0;
@@ -225,11 +219,6 @@ static int close_istream_loose(struct git_istream *st)
 	return 0;
 }
 
-static struct stream_vtbl loose_vtbl = {
-	close_istream_loose,
-	read_istream_loose,
-};
-
 static int open_istream_loose(struct git_istream *st, struct repository *r,
 			      const struct object_id *oid,
 			      enum object_type *type)
@@ -251,8 +240,9 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,
 	st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;
 	st->u.loose.hdr_avail = st->z.total_out;
 	st->z_state = z_used;
+	st->close = close_istream_loose;
+	st->read = read_istream_loose;
 
-	st->vtbl = &loose_vtbl;
 	return 0;
 }
 
@@ -328,11 +318,6 @@ static int close_istream_pack_non_delta(struct git_istream *st)
 	return 0;
 }
 
-static struct stream_vtbl pack_non_delta_vtbl = {
-	close_istream_pack_non_delta,
-	read_istream_pack_non_delta,
-};
-
 static int open_istream_pack_non_delta(struct git_istream *st,
 				       struct repository *r,
 				       const struct object_id *oid,
@@ -358,7 +343,9 @@ static int open_istream_pack_non_delta(struct git_istream *st,
 		break;
 	}
 	st->z_state = z_unused;
-	st->vtbl = &pack_non_delta_vtbl;
+	st->close = close_istream_pack_non_delta;
+	st->read = read_istream_pack_non_delta;
+
 	return 0;
 }
 
@@ -389,17 +376,13 @@ static ssize_t read_istream_incore(struct git_istream *st, char *buf, size_t sz)
 	return read_size;
 }
 
-static struct stream_vtbl incore_vtbl = {
-	close_istream_incore,
-	read_istream_incore,
-};
-
 static int open_istream_incore(struct git_istream *st, struct repository *r,
 			       const struct object_id *oid, enum object_type *type)
 {
 	st->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);
 	st->u.incore.read_ptr = 0;
-	st->vtbl = &incore_vtbl;
+	st->close = close_istream_incore;
+	st->read = read_istream_incore;
 
 	return st->u.incore.buf ? 0 : -1;
 }
@@ -408,10 +391,10 @@ static int open_istream_incore(struct git_istream *st, struct repository *r,
  * static helpers variables and functions for users of streaming interface
  *****************************************************************************/
 
-static open_istream_fn istream_source(struct git_istream *st,
-				      struct repository *r,
-				      const struct object_id *oid,
-				      enum object_type *type)
+static int istream_source(struct git_istream *st,
+			  struct repository *r,
+			  const struct object_id *oid,
+			  enum object_type *type)
 {
 	unsigned long size;
 	int status;
@@ -421,20 +404,23 @@ static open_istream_fn istream_source(struct git_istream *st,
 	oi.sizep = &size;
 	status = oid_object_info_extended(r, oid, &oi, 0);
 	if (status < 0)
-		return NULL;
+		return status;
 
 	switch (oi.whence) {
 	case OI_LOOSE:
-		return open_istream_loose;
+		st->open = open_istream_loose;
+		return 0;
 	case OI_PACKED:
 		if (!oi.u.packed.is_delta && big_file_threshold < size) {
 			st->u.in_pack.pack = oi.u.packed.pack;
 			st->u.in_pack.pos = oi.u.packed.offset;
-			return open_istream_pack_non_delta;
+			st->open = open_istream_pack_non_delta;
+			return 0;
 		}
 		/* fallthru */
 	default:
-		return open_istream_incore;
+		st->open = open_istream_incore;
+		return 0;
 	}
 }
 
@@ -444,14 +430,14 @@ static open_istream_fn istream_source(struct git_istream *st,
 
 int close_istream(struct git_istream *st)
 {
-	int r = st->vtbl->close(st);
+	int r = st->close(st);
 	free(st);
 	return r;
 }
 
 ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
 {
-	return st->vtbl->read(st, buf, sz);
+	return st->read(st, buf, sz);
 }
 
 struct git_istream *open_istream(struct repository *r,
@@ -462,14 +448,14 @@ struct git_istream *open_istream(struct repository *r,
 {
 	struct git_istream *st = xmalloc(sizeof(*st));
 	const struct object_id *real = lookup_replace_object(r, oid);
-	open_istream_fn open_fn = istream_source(st, r, real, type);
+	int ret = istream_source(st, r, real, type);
 
-	if (!open_fn) {
+	if (ret) {
 		free(st);
 		return NULL;
 	}
 
-	if (open_fn(st, r, real, type)) {
+	if (st->open(st, r, real, type)) {
 		if (open_istream_incore(st, r, real, type)) {
 			free(st);
 			return NULL;
-- 
2.31.1.838.g7ac6e98bb53

Re: [PATCH 5/5] streaming.c: move {open,close,read} from vtable to "struct git_istream"

[Jeff King]

On Wed, May 05, 2021 at 02:33:32PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Move the definition of the structure around the open/close/read
> functions introduced in 46bf043807c (streaming: a new API to read from
> the object store, 2011-05-11) to instead populate "close" and "read"
> members in the "struct git_istream".
> 
> This gets us rid of an extra pointer deference, and I think makes more
> sense. The "close" and "read" functions are the primary interface to
> the stream itself.
> 
> Let's also populate a "open" callback in the same struct. That's now
> used by open_istream() after istream_source() decides what "open"
> function should be used. This isn't needed to get rid of the
> "stream_vtbl" variables, but makes sense for consistency.

Yeah, I think if we are going to get rid of the open vtable in an earlier
patch, it makes sense to go all the way here and make things consistent.

If we had a bunch of these objects, having the layer of indirection to
the vtable would save us some per-object memory. But we really only ever
have one of these open at a time.

-Peff

Re: [PATCH 0/5] streaming.c: refactor for smaller + easier to understand code

[Jeff King]

On Wed, May 05, 2021 at 02:33:27PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This is a prep series for my yet-to-be-sent re-roll of [1],
> aka. ab/fsck-unexpected-type.
> 
> Jeff King had a comment in [2] saying it was odd to have an "oi2"
> variable, that was because there was on an "oi" name already defined
> via the new-gone open_method_decl(loose) macro. As we'll see we could
> do without the initial "oi" and a few other types of indirection in
> this interface.
> 
> Junio: Could you eject ab/fsck-unexpected-type while this is being
> considered/cooked? I didn't really see how to address Jeff's feedback
> about that variable name in a way that wouldn't just make something
> like this refactoring part of an even bigger series. I think ejecting
> the functional changes & trying to get this in first is the least
> worst approach at this point. Eventually I'll submit a re-roll of
> ab/fsck-unexpected-type either based on this, or master (if it's
> landed already).

I think my feedback was really just: if that patch didn't refactor
parse_loose_header() to get rid of the "simple" and extended versions,
then you wouldn't have to touch streaming.c at all. It could just
continue to use the simple version.

That said, I don't mind the cleanups here, especially getting rid of the
macros.

-Peff