Prathamesh Chavan <pc44800@xxxxxxxxx> 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.
> @@ -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.
> @@ -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.
> + 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.
