Re: [PATCH 1/8] ls-files: add --json to dump the index

[Derrick Stolee <stolee@gmail.com>]

On 6/19/2019 5:58 AM, Nguyễn Thái Ngọc Duy wrote:
> So far we don't have a command to basically dump the index file out,
> with all its glory details. Checking some info, for example, stat
> time, usually involves either writing new code or firing up "xxd" and
> decoding values by yourself.
> 
> This --json is supposed to help that. It dumps the index in a human
> readable format but also easy to be processed with tools. And it will
> print almost enough info to reconstruct the index later.

In an earlier message, I stated that I like the idea of this feature.
I know that you wanted to get that feedback before working too hard on
the patch series, so now that interest is declared, please add some tests
that verify this output looks as you expect.

> In this patch we only dump the main part, not extensions. But at the
> end of the series, the entire index is dumped. The end result could be
> very verbose even on a small repository such as git.git.

I would expect this commit to include a complete "output matches expected"
test, and the later patches can update the index to include these
extensions then verify that their sections appear in the output using
a grep.

> +--json::
> +	Dump the entire index content in JSON format. This is for
> +	debugging purposes and the JSON structure may change from time
> +	to time.
> +

"...purposes and the JSON structure may change from time to time" may be better
as "...purposes. The JSON structure is subject to change."

> +		OPT_BOOL(0, "json", &show_json,
> +			N_("dump index content in json format")),

We should probably use "JSON" here in the help text.

> -	show_files(the_repository, &dir);
> -
> -	if (show_resolve_undo)
> -		show_ru_info(the_repository->index);
> +	if (!show_json) {
> +		show_files(the_repository, &dir);
> +
> +		if (show_resolve_undo)
> +			show_ru_info(the_repository->index);
> +	} else {
> +		struct json_writer jw = JSON_WRITER_INIT;
> +
> +		discard_index(the_repository->index);
> +		the_repository->index->jw = &jw;
> +		if (repo_read_index(the_repository) < 0)
> +			die("index file corrupt");
> +		puts(jw.json.buf);
> +		the_repository->index->jw = NULL;
> +		jw_release(&jw);
> +	}
>  
>  	if (ps_matched) {
>  		int bad;

I see this 'ps_matched' condition at the end, which is related to
the '--error-unmatch' option. I added "--error-unmatch foo" to my
command and got the appropriate error message:

  error: pathspec 'foo' did not match any file(s) known to git
  Did you forget to 'git add'?

This was sent to stderr while the JSON was in stdout, so this should
be appropriate to allow both options. Just pointing it out to make
sure this is intended.

> +void jw_object_stat_data(struct json_writer *jw, const char *name,
> +			 const struct stat_data *sd)
> +{
> +	jw_object_inline_begin_object(jw, name);
> +	jw_object_intmax(jw, "st_ctime.sec", sd->sd_ctime.sec);
> +	jw_object_intmax(jw, "st_ctime.nsec", sd->sd_ctime.nsec);
> +	jw_object_intmax(jw, "st_mtime.sec", sd->sd_mtime.sec);
> +	jw_object_intmax(jw, "st_mtime.nsec", sd->sd_mtime.nsec);
> +	jw_object_intmax(jw, "st_dev", sd->sd_dev);
> +	jw_object_intmax(jw, "st_ino", sd->sd_ino);
> +	jw_object_intmax(jw, "st_uid", sd->sd_uid);
> +	jw_object_intmax(jw, "st_gid", sd->sd_gid);
> +	jw_object_intmax(jw, "st_size", sd->sd_size);
> +	jw_end(jw);
> +}

If these are all part of the same object, are the "st_" prefixes
necessary for every member?

> +	/*
> +	 * again redundant info, just so you don't have to decode
> +	 * flags values manually
> +	 */
> +	if (ce->ce_flags & CE_VALID)
> +		jw_object_true(jw, "assume-unchanged");
> +	if (ce->ce_flags & CE_INTENT_TO_ADD)
> +		jw_object_true(jw, "intent-to-add");
> +	if (ce->ce_flags & CE_SKIP_WORKTREE)
> +		jw_object_true(jw, "skip-worktree");
> +	if (ce_stage(ce))
> +		jw_object_intmax(jw, "stage", ce_stage(ce));

I'm really glad these flags are getting expanded! Much easier to
understand what's going on this way.

> +	if (istate->jw) {
> +		jw_object_begin(istate->jw, jw_pretty);
> +		jw_object_intmax(istate->jw, "version", istate->version);
> +		jw_object_string(istate->jw, "oid", oid_to_hex(&istate->oid));
> +		jw_object_intmax(istate->jw, "st_mtime.sec", istate->timestamp.sec);
> +		jw_object_intmax(istate->jw, "st_mtime.nsec", istate->timestamp.nsec);

Here, the "st_" prefixes are not on every member, but would it
be confusing if they were not there? Also, including a "." in
a member name may be troublesome for JSON, as that typically
means we are accessing a member of an object. Perhaps use _sec
and _nsec here and in the earlier stat_data block.

Thanks,
-Stolee