Re: [PATCH 04/16] list-files: add tag to each entry, filter duplicate tags

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> All entry strings start with two-letter tag and a space. If all
> entries have the same tags, tags are not displayed.
>
> The outcome before and after this patch is the same. But it will be
> useful in future when there are more than one type of entry.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/list-files.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/list-files.c b/builtin/list-files.c
> index c444a53..18af65c 100644
> --- a/builtin/list-files.c
> +++ b/builtin/list-files.c
> @@ -18,12 +18,16 @@ struct option ls_options[] = {
>  	OPT_END()
>  };
>  
> -static void add_one(struct string_list *result, const char *name)
> +static void add_one(struct string_list *result, const char *name,
> +		    const char *tag)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	struct string_list_item *item;
>  
>  	quote_path_relative(name, prefix_length ? prefix : NULL, &sb);
> +	strbuf_insert(&sb, 0, "   ", 3);
> +	sb.buf[0] = tag[0];
> +	sb.buf[1] = tag[1];
>  	item = string_list_append(result, strbuf_detach(&sb, NULL));
>  	item->util = (char *)name;
>  }
> @@ -42,7 +46,38 @@ static void populate_cached_entries(struct string_list *result,
>  				    S_ISGITLINK(ce->ce_mode)))
>  			continue;
>  
> -		add_one(result, ce->name);
> +		add_one(result, ce->name, "  ");
> +	}
> +}
> +
> +static void cleanup_tags(struct string_list *result)
> +{
> +	int i, same_1 = 1, same_2 = 1, pos, len;
> +
> +	for (i = 1; i < result->nr && (same_1 || same_2); i++) {
> +		const char *s0 = result->items[i - 1].string;
> +		const char *s1 = result->items[i].string;
> +
> +		same_1 = same_1 && s0[0] == s1[0];
> +		same_2 = same_2 && s0[1] == s1[1];
> +	}
> +
> +	if (same_1 && same_2) {
> +		pos = 0;
> +		len = 3;
> +	} else if (same_1) {
> +		pos = 0;
> +		len = 1;
> +	} else if (same_2) {
> +		pos = 1;
> +		len = 1;
> +	} else
> +		return;
> +
> +	for (i = 0; i < result->nr; i++) {
> +		char *s = result->items[i].string;
> +		int length = strlen(s);
> +		memmove(s + pos, s + pos + len, length - len + 1);
>  	}
>  }

Hmm, I wonder if a different implementation strategy would produce a
code that is better for longer term maintenance and readability.
For example, how much pain would be involved if we later find that
we would want three "tag" letters per entry and wanted to add
support for that third tag by modifying this code?

Instead of half-formatted result in item->string and then inspect
and update the already formatted string at the textual level, why
not invert the keys and values of the table and arrange things this
way instead:

 * the "table" is expressed as a string-list, as this series does;

 * the keys to the table, item->string, is the original pathname;

 * the values in the table, item->util, is a pointer to a structure
   that allows implementation of list-files a more meaningful access
   to the information (as opposed to "the first column of formatted
   text output means X, the second column means Y"), perhaps like

    struct list_item {
	enum {
            LS_IS_FILE, LS_IS_DIRECTORY, LS_IS_SYMLINK,	LS_IS_SUBMODULE
	} kind;
	unsigned changed_from_index:1,
                 changed_from_HEAD:1;
	struct cache_time mtime;
    };

 * Internal processing is done to the value found in item->util, and
   the textual output is created by formatting what is in the
   *((struct list_item *)item->util) at the output phase.

A hypothetical "we need more tags" case would then involve adding a
new field (could be a bitfield "breaks_build:1") to the list_item
structure with a reasonable default, keeping the existing codepath
that does not care about the new field intact and updating only the
output phase, which would be a lot less painful, no?

> @@ -93,6 +128,7 @@ int cmd_list_files(int argc, const char **argv, const char *cmd_prefix)
>  		      &pathspec, NULL, NULL);
>  
>  	populate_cached_entries(&result, &the_index);
> +	cleanup_tags(&result);
>  	display(&result);
>  	string_list_clear(&result, 0);
>  	return 0;