Re: [PATCH 04/16] list-files: add tag to each entry, filter duplicate tags
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 12, 2015, 21:48 UTC
- Message-ID
- <xmqqk2yl28fw.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1425896314-10941-5-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 72 quoted lines
> 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?
Show 8 quoted lines
> @@ -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;