Re: [PATCH v6 3/3] submodule: port submodule subcommand 'status' from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2017, 01:08 UTC
Message-ID: <xmqqy3ouqruh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170929094453.4499-4-pc44800@gmail.com>

Prathamesh Chavan <pc44800@gmail.com> writes:

>  
>  #define CB_OPT_QUIET		(1<<0)
> +#define CB_OPT_CACHED		(1<<1)
> +#define CB_OPT_RECURSIVE	(1<<2)

Same comments on both naming and formatting.

Show 29 quoted lines

> @@ -245,6 +250,53 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)
>  	}
>  }
>  
> +static char *compute_rev_name(const char *sub_path, const char* object_id)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	const char ***d;
> +
> +	static const char *describe_bare[] = {
> +		NULL
> +	};
> +
> +	static const char *describe_tags[] = {
> +		"--tags", NULL
> +	};
> +
> +	static const char *describe_contains[] = {
> +		"--contains", NULL
> +	};
> +
> +	static const char *describe_all_always[] = {
> +		"--all", "--always", NULL
> +	};
> +
> +	static const char **describe_argv[] = {
> +		describe_bare, describe_tags, describe_contains,
> +		describe_all_always, NULL
> +	};

"make style" seems to suggest a lot more compact version to be used for the above, and I tend to agree with its diagnosis.

Show 9 quoted lines

> @@ -503,6 +555,160 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct status_cb {
> +	const char *prefix;
> +	unsigned int cb_flags;
> +};
> +#define STATUS_CB_INIT { NULL, 0 }

Same three comments as the previous "init_cb" patch apply.

Show 5 quoted lines

> +	argv_array_pushl(&diff_files_args, "diff-files",
> +			 "--ignore-submodules=dirty", "--quiet", "--",
> +			 path, NULL);
> +
> +	git_config(git_diff_basic_config, NULL);

Should this be called every time? The config file is not changing, no?

> +	init_revisions(&rev, prefix);
> +	rev.abbrev = 0;

This part looks OK.

> +	precompose_argv(diff_files_args.argc, diff_files_args.argv);

I do not think this is correct. We certainly did not get the path argument (i.e. args.argv) from the command line of macOS X box and the correction for UTF-8 canonicalization should not be necessary. Even if we did get path from the command line, I think the UTF-8 correction should have been done for us for any command (like "git submodule--helper") that uses parse-optoins API already.

Just dropping the line should be sufficient to correct this, I think.

The remainder of the patch looked more-or-less OK, but I'd revisit it later to make sure.

Thanks.