Just a bit of random comments, leaving the full review to mentors.
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..c9cb535312 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
> return !!ret;
> }
>
> +struct add_data {
> + const char *prefix;
> + const char *branch;
> + const char *reference_path;
> + const char *sm_path;
> + const char *sm_name;
> + const char *repo;
> + const char *realrepo;
> + int depth;
> + unsigned int force: 1;
> + unsigned int quiet: 1;
> + unsigned int progress: 1;
> + unsigned int dissociate: 1;
> +};
> +#define ADD_DATA_INIT { .depth = -1 }
> +
> +static char *parse_token(char **begin, const char *end, int *tok_len)
> +{
> + char *tok_start, *pos = *begin;
Make it a habit to have a blank line between the initial block
of declarations and the first statement.
> + while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
> + pos++;
> + tok_start = *begin;
> + *tok_len = pos - *begin;
> + *begin = pos + 1;
> + return tok_start;
> +}
> +static char *get_next_line(char *const begin, const char *const end)
> +{
> + char *pos = begin;
> + while (pos != end && *pos++ != '\n');
Write an empty loop on two lines, like this:
while (... condition ...)
; /* keep scanning */
If there is a NUL byte between begin and end, this keeps going and
the resulting string will contain one. Is that a problem?
> + return pos;
> +}
In general, this project is mature enough that we should question
ourselves if there is already a suitable line parser we can reuse
when tempted to write another one.
> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
> +{
> + struct child_process cp_remote = CHILD_PROCESS_INIT;
> + struct strbuf sb_remote_out = STRBUF_INIT;
> +
> + cp_remote.git_cmd = 1;
> + strvec_pushf(&cp_remote.env_array,
> + "GIT_DIR=%s", git_dir_path);
> + strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
> + strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
> + char *line;
> + char *begin = sb_remote_out.buf;
> + char *end = sb_remote_out.buf + sb_remote_out.len;
> + while (begin != end && (line = get_next_line(begin, end))) {
OK, so this tries to parse output from "git remote -v", so NUL will
not be an issue at all. We will get a string that is NUL terminated
and has zero or more lines, terminated with LFs.
If that is the case, I think it is far easier to read without
a custom get-next-line wrapper, e.g.
for (this_line = begin;
*this_line;
this_line = next_line) {
next_line = strchrnul(this_line, '\n');
... process bytes between this_line..next_line ...
}
> + int namelen = 0, urllen = 0, taillen = 0;
> + char *name = parse_token(&begin, line, &namelen);
Similarly, consider if strcspn() is useful in implementing
parse_token(). See how existing code uses the standard system
function with
$ git grep strcspn \*.c
> + char *url = parse_token(&begin, line, &urllen);
> + char *tail = parse_token(&begin, line, &taillen);
> + if (!memcmp(tail, "(fetch)", 7))
At this point do we know there are enough number of bytes after
tail[0] to allow us to do this comparison safely? Otherwise,
if (starts_with(tail, "(fetch)")
may be preferrable.
