[PATCH v3 00/22] fsck: API improvements

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

Now that jt/transfer-fsck-across-packs has been merged to master
here's a re-roll of v1[1]+v2[2] of this series. v2 was slimmed-down +
had a trivial typo fix, so I've done the range-diff against v1.

This makes the recent fetch-pack work use the fsck_msg_id API to
distinguish messages, and has other various cleanups and improvements
to make the fsck API easier to use in the future.

There's a an easy merge conflict here with other in-flight changes to
fsck. I figured it was better to send this now than wait for those to
land.

1. https://lore.kernel.org/git/20210217194246.25342-1-avarab@gmail.com/
2. https://lore.kernel.org/git/20210218105840.11989-1-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (22):
  fsck.h: update FSCK_OPTIONS_* for object_name
  fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}
  fsck.h: reduce duplication between FSCK_OPTIONS_{DEFAULT,STRICT}
  fsck.h: add a FSCK_OPTIONS_COMMON_ERROR_FUNC macro
  fsck.h: indent arguments to of fsck_set_msg_type
  fsck.h: use "enum object_type" instead of "int"
  fsck.c: rename variables in fsck_set_msg_type() for less confusion
  fsck.c: move definition of msg_id into append_msg_id()
  fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
  fsck.c: refactor fsck_msg_type() to limit scope of "int msg_type"
  fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
  fsck.h: re-order and re-assign "enum fsck_msg_type"
  fsck.c: call parse_msg_type() early in fsck_set_msg_type()
  fsck.c: undefine temporary STR macro after use
  fsck.c: give "FOREACH_MSG_ID" a more specific name
  fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
  fsck.c: pass along the fsck_msg_id in the fsck_error callback
  fsck.c: add an fsck_set_msg_type() API that takes enums
  fsck.c: move gitmodules_{found,done} into fsck_options
  fetch-pack: don't needlessly copy fsck_options
  fetch-pack: use file-scope static struct for fsck_options
  fetch-pack: use new fsck API to printing dangling submodules

 Makefile                 |   1 +
 builtin/fsck.c           |   7 +-
 builtin/index-pack.c     |  30 ++-----
 builtin/mktag.c          |   7 +-
 builtin/unpack-objects.c |   3 +-
 fetch-pack.c             |   6 +-
 fsck-cb.c                |  16 ++++
 fsck.c                   | 175 ++++++++++++---------------------------
 fsck.h                   | 132 ++++++++++++++++++++++++++---
 9 files changed, 211 insertions(+), 166 deletions(-)
 create mode 100644 fsck-cb.c

Range-diff:
13:  8de91fac068 =  1:  9d809466bd1 fsck.h: update FSCK_OPTIONS_* for object_name
 -:  ----------- >  2:  33e8b6d6545 fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}
 -:  ----------- >  3:  c23f7ce9e4a fsck.h: reduce duplication between FSCK_OPTIONS_{DEFAULT,STRICT}
 -:  ----------- >  4:  5dde68df6c3 fsck.h: add a FSCK_OPTIONS_COMMON_ERROR_FUNC macro
 1:  88b347b74ed =  5:  7ae35a6e9d2 fsck.h: indent arguments to of fsck_set_msg_type
 2:  1a60d65d2ca !  6:  dfb5f754b37 fsck.h: use use "enum object_type" instead of "int"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    fsck.h: use use "enum object_type" instead of "int"
    +    fsck.h: use "enum object_type" instead of "int"
     
         Change the fsck_walk_func to use an "enum object_type" instead of an
         "int" type. The types are compatible, and ever since this was added in
 3:  24761f269b7 !  7:  fd58ec73c6b fsck.c: rename variables in fsck_set_msg_type() for less confusion
    @@ Commit message
         It was needlessly confusing that it took a "msg_type" argument, but
         then later declared another "msg_type" of a different type.
     
    -    Let's rename that to "tmp", and rename "id" to "msg_id" and "msg_id"
    -    to "msg_id_str" etc. This will make a follow-up change smaller.
    +    Let's rename that to "severity", and rename "id" to "msg_id" and
    +    "msg_id" to "msg_id_str" etc. This will make a follow-up change
    +    smaller.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ fsck.c: int is_valid_msg_type(const char *msg_id, const char *msg_type)
      		int i;
     -		int *msg_type;
     -		ALLOC_ARRAY(msg_type, FSCK_MSG_MAX);
    -+		int *tmp;
    -+		ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
    ++		int *severity;
    ++		ALLOC_ARRAY(severity, FSCK_MSG_MAX);
      		for (i = 0; i < FSCK_MSG_MAX; i++)
     -			msg_type[i] = fsck_msg_type(i, options);
     -		options->msg_type = msg_type;
    -+			tmp[i] = fsck_msg_type(i, options);
    -+		options->msg_type = tmp;
    ++			severity[i] = fsck_msg_type(i, options);
    ++		options->msg_type = severity;
      	}
      
     -	options->msg_type[id] = type;
 4:  fb4c66f9305 =  8:  48cb4d3bb70 fsck.c: move definition of msg_id into append_msg_id()
 5:  a129dbd9964 !  9:  2c80ad32038 fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
    @@ Commit message
         "msg_id". This change is relatively small, and is worth the churn for
         a later change where we have different id's in the "report" function.
     
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +
      ## fsck.c ##
     @@ fsck.c: void fsck_set_msg_types(struct fsck_options *options, const char *values)
      	free(to_free);
 -:  ----------- > 10:  92dfbdfb624 fsck.c: refactor fsck_msg_type() to limit scope of "int msg_type"
 6:  d9bee41072e ! 11:  c1c476af69b fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
    @@ Commit message
          - f27d05b1704 (fsck: allow upgrading fsck warnings to errors,
            2015-06-22)
     
    +    The reason these were defined in two different places is because we
    +    use FSCK_{IGNORE,INFO,FATAL} only in fsck.c, but FSCK_{ERROR,WARN} are
    +    used by external callbacks.
    +
    +    Untangling that would take some more work, since we expose the new
    +    "enum fsck_msg_type" to both. Similar to "enum object_type" it's not
    +    worth structuring the API in such a way that only those who need
    +    FSCK_{ERROR,WARN} pass around a different type.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/fsck.c ##
    @@ builtin/fsck.c: static int objerror(struct object *obj, const char *err)
      	switch (msg_type) {
      	case FSCK_WARN:
     
    + ## builtin/index-pack.c ##
    +@@ builtin/index-pack.c: static void show_pack_info(int stat_only)
    + static int print_dangling_gitmodules(struct fsck_options *o,
    + 				     const struct object_id *oid,
    + 				     enum object_type object_type,
    +-				     int msg_type, const char *message)
    ++				     enum fsck_msg_type msg_type,
    ++				     const char *message)
    + {
    + 	/*
    + 	 * NEEDSWORK: Plumb the MSG_ID (from fsck.c) here and use it
    +
      ## builtin/mktag.c ##
     @@ builtin/mktag.c: static int mktag_config(const char *var, const char *value, void *cb)
      static int mktag_fsck_error_func(struct fsck_options *o,
    @@ fsck.c: void list_config_fsck_msg_ids(struct string_list *list, const char *pref
     +static enum fsck_msg_type fsck_msg_type(enum fsck_msg_id msg_id,
      	struct fsck_options *options)
      {
    --	int msg_type;
    -+	enum fsck_msg_type msg_type;
    - 
      	assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
      
    + 	if (!options->msg_type) {
    +-		int msg_type = msg_id_info[msg_id].msg_type;
    ++		enum fsck_msg_type msg_type = msg_id_info[msg_id].msg_type;
    + 
    + 		if (options->strict && msg_type == FSCK_WARN)
    + 			msg_type = FSCK_ERROR;
     @@ fsck.c: static int fsck_msg_type(enum fsck_msg_id msg_id,
    - 	return msg_type;
    + 	return options->msg_type[msg_id];
      }
      
     -static int parse_msg_type(const char *str)
    @@ fsck.c: void fsck_set_msg_type(struct fsck_options *options,
      
      	if (!options->msg_type) {
      		int i;
    --		int *tmp;
    -+		enum fsck_msg_type *tmp;
    - 		ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
    +-		int *severity;
    ++		enum fsck_msg_type *severity;
    + 		ALLOC_ARRAY(severity, FSCK_MSG_MAX);
      		for (i = 0; i < FSCK_MSG_MAX; i++)
    - 			tmp[i] = fsck_msg_type(i, options);
    + 			severity[i] = fsck_msg_type(i, options);
     @@ fsck.c: static int report(struct fsck_options *options,
      {
      	va_list ap;
    @@ fsck.h
     -#define FSCK_ERROR 1
     -#define FSCK_WARN 2
     -#define FSCK_IGNORE 3
    --
     +enum fsck_msg_type {
    -+	FSCK_INFO = -2,
    ++	FSCK_INFO  = -2,
     +	FSCK_FATAL = -1,
     +	FSCK_ERROR = 1,
     +	FSCK_WARN,
     +	FSCK_IGNORE
     +};
    + 
      struct fsck_options;
      struct object;
    - 
     @@ fsck.h: typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,
      /* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */
      typedef int (*fsck_error)(struct fsck_options *o,
 -:  ----------- > 12:  d55587719a5 fsck.h: re-order and re-assign "enum fsck_msg_type"
 7:  423568026c3 = 13:  32828d1c78c fsck.c: call parse_msg_type() early in fsck_set_msg_type()
 8:  cb43e832738 = 14:  5c62066235c fsck.c: undefine temporary STR macro after use
 9:  2cd14cb4e2a = 15:  f8e50fbf7d3 fsck.c: give "FOREACH_MSG_ID" a more specific name
10:  1ada154ef23 ! 16:  cd74dee8769 fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
    @@ fsck.c
      ## fsck.h ##
     @@ fsck.h: enum fsck_msg_type {
      	FSCK_WARN,
    - 	FSCK_IGNORE
      };
    -+
    + 
     +#define FOREACH_FSCK_MSG_ID(FUNC) \
     +	/* fatal errors */ \
     +	FUNC(NUL_IN_HEADER, FATAL) \
11:  c4179445f22 ! 17:  234e287d081 fsck.c: pass along the fsck_msg_id in the fsck_error callback
    @@ builtin/fsck.c: static int objerror(struct object *obj, const char *err)
      	switch (msg_type) {
      	case FSCK_WARN:
     
    + ## builtin/index-pack.c ##
    +@@ builtin/index-pack.c: static int print_dangling_gitmodules(struct fsck_options *o,
    + 				     const struct object_id *oid,
    + 				     enum object_type object_type,
    + 				     enum fsck_msg_type msg_type,
    ++				     enum fsck_msg_id msg_id,
    + 				     const char *message)
    + {
    + 	/*
    +@@ builtin/index-pack.c: static int print_dangling_gitmodules(struct fsck_options *o,
    + 		printf("%s\n", oid_to_hex(oid));
    + 		return 0;
    + 	}
    +-	return fsck_error_function(o, oid, object_type, msg_type, message);
    ++	return fsck_error_function(o, oid, object_type, msg_type, msg_id, message);
    + }
    + 
    + int cmd_index_pack(int argc, const char **argv, const char *prefix)
    +
      ## builtin/mktag.c ##
     @@ builtin/mktag.c: static int mktag_fsck_error_func(struct fsck_options *o,
      				 const struct object_id *oid,
12:  c1fc724f0e8 ! 18:  8049dc07391 fsck.c: add an fsck_set_msg_type() API that takes enums
    @@ fsck.c: int is_valid_msg_type(const char *msg_id, const char *msg_type)
     +{
     +	if (!options->msg_type) {
     +		int i;
    -+		enum fsck_msg_type *tmp;
    -+		ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
    ++		enum fsck_msg_type *severity;
    ++		ALLOC_ARRAY(severity, FSCK_MSG_MAX);
     +		for (i = 0; i < FSCK_MSG_MAX; i++)
    -+			tmp[i] = fsck_msg_type(i, options);
    -+		options->msg_type = tmp;
    ++			severity[i] = fsck_msg_type(i, options);
    ++		options->msg_type = severity;
     +	}
     +
     +	options->msg_type[msg_id] = msg_type;
    @@ fsck.c: void fsck_set_msg_type(struct fsck_options *options,
      
     -	if (!options->msg_type) {
     -		int i;
    --		enum fsck_msg_type *tmp;
    --		ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
    +-		enum fsck_msg_type *severity;
    +-		ALLOC_ARRAY(severity, FSCK_MSG_MAX);
     -		for (i = 0; i < FSCK_MSG_MAX; i++)
    --			tmp[i] = fsck_msg_type(i, options);
    --		options->msg_type = tmp;
    +-			severity[i] = fsck_msg_type(i, options);
    +-		options->msg_type = severity;
     -	}
     -
     -	options->msg_type[msg_id] = msg_type;
14:  29ff97856ff ! 19:  4224a29d15c fsck.c: move gitmodules_{found,done} into fsck_options
    @@ Commit message
         fsck_options struct. It makes sense to keep all the context in the
         same place.
     
    +    This requires changing the recently added register_found_gitmodules()
    +    function added in 5476e1efde (fetch-pack: print and use dangling
    +    .gitmodules, 2021-02-22) to take fsck_options. That function will be
    +    removed in a subsequent commit, but as it'll require the new
    +    gitmodules_found attribute of "fsck_options" we need this intermediate
    +    step first.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    + ## fetch-pack.c ##
    +@@ fetch-pack.c: static void fsck_gitmodules_oids(struct oidset *gitmodules_oids)
    + 
    + 	oidset_iter_init(gitmodules_oids, &iter);
    + 	while ((oid = oidset_iter_next(&iter)))
    +-		register_found_gitmodules(oid);
    ++		register_found_gitmodules(&fo, oid);
    + 	if (fsck_finish(&fo))
    + 		die("fsck failed");
    + }
    +
      ## fsck.c ##
     @@
      #include "credential.h"
    @@ fsck.c: static int fsck_blob(const struct object_id *oid, const char *buf,
      
      	if (object_on_skiplist(options, oid))
      		return 0;
    +@@ fsck.c: int fsck_error_function(struct fsck_options *o,
    + 	return 1;
    + }
    + 
    +-void register_found_gitmodules(const struct object_id *oid)
    ++void register_found_gitmodules(struct fsck_options *options, const struct object_id *oid)
    + {
    +-	oidset_insert(&gitmodules_found, oid);
    ++	oidset_insert(&options->gitmodules_found, oid);
    + }
    + 
    + int fsck_finish(struct fsck_options *options)
     @@ fsck.c: int fsck_finish(struct fsck_options *options)
      	struct oidset_iter iter;
      	const struct object_id *oid;
    @@ fsck.h: struct fsck_options {
      	kh_oid_map_t *object_names;
      };
      
    --#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT, NULL }
    --#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT, NULL }
    -+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT, OIDSET_INIT, OIDSET_INIT, NULL }
    -+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT, OIDSET_INIT, OIDSET_INIT, NULL }
    +@@ fsck.h: struct fsck_options {
    + 	.walk = NULL, \
    + 	.msg_type = NULL, \
    + 	.skiplist = OIDSET_INIT, \
    ++	.gitmodules_found = OIDSET_INIT, \
    ++	.gitmodules_done = OIDSET_INIT, \
    + 	.object_names = NULL,
    + #define FSCK_OPTIONS_COMMON_ERROR_FUNC \
    + 	FSCK_OPTIONS_COMMON \
    +@@ fsck.h: int fsck_walk(struct object *obj, void *data, struct fsck_options *options);
    + int fsck_object(struct object *obj, void *data, unsigned long size,
    + 	struct fsck_options *options);
    + 
    +-void register_found_gitmodules(const struct object_id *oid);
    ++void register_found_gitmodules(struct fsck_options *options,
    ++			       const struct object_id *oid);
      
    - /* descend in all linked child objects
    -  * the return value is:
    + /*
    +  * fsck a tag, and pass info about it back to the caller. This is
 -:  ----------- > 20:  40b13468129 fetch-pack: don't needlessly copy fsck_options
 -:  ----------- > 21:  8e418abfbd7 fetch-pack: use file-scope static struct for fsck_options
 -:  ----------- > 22:  113de190f7d fetch-pack: use new fsck API to printing dangling submodules
-- 
2.31.0.rc0.126.g04f22c5b82