On 09-Jun-2021, at 09:54, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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 */
OK.
> 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.
I will keep this in mind.
>> +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.
This solution is definitely an improvement over what I was doing.
That said, I like Danh's suggestion[1] more, because it eliminates the
need for parsing tokens entirely.
The fundamental thing that piece of code was meant to do is:
"If this line ends with '(fetch)', print the line, but without the '(fetch)'"
Parsing tokens only to put them back together through fprintf() may
not be necessary for this usage, so using strchr() with
strip_suffix_mem() should do the trick.
[1] https://lore.kernel.org/git/YL9jTFAoEBP+mDA2@xxxxxxxx/
Thanks for the comments :^)
