git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 07/11] Add a function to determine unique prefixes for a list of strings

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Aug 24, 2019, 12:38 UTC
Message-ID
<20190824123811.GL20404@szeder.dev>
In-Reply-To
<3000d7d08dfb64511b4ebf9d05617897dd7252f7.1563289115.git.gitgitgadget@gmail.com>
On Tue, Jul 16, 2019 at 07:58:42AM -0700, Slavica Djukic via GitGitGadget wrote:
Show 24 quoted lines
> In the `git add -i` command, we show unique prefixes of the commands and
> files, to give an indication what prefix would select them.
> 
> Naturally, the C implementation looks a lot different than the Perl
> implementation: in Perl, a trie is much easier implemented, while we
> already have a pretty neat hashmap implementation in C that we use for
> the purpose of storing (not necessarily unique) prefixes.
> 
> The idea: for each item that we add, we generate prefixes starting with
> the first letter, then the first two letters, then three, etc, until we
> find a prefix that is unique (or until the prefix length would be
> longer than we want). If we encounter a previously-unique prefix on the
> way, we adjust that item's prefix to make it unique again (or we mark it
> as having no unique prefix if we failed to find one). These partial
> prefixes are stored in a hash map (for quick lookup times).
> 
> To make sure that this function works as expected, we add a test using a
> special-purpose test helper that was added for that purpose.
> 
> Note: We expect the list of prefix items to be passed in as a list of
> pointers rather than as regular list to avoid having to copy information
> (the actual items will most likely contain more information than just
> the name and the length of the unique prefix, but passing in `struct
> prefix_item *` would not allow for that).
Show 115 quoted lines
> diff --git a/prefix-map.c b/prefix-map.c
> new file mode 100644
> index 0000000000..747ddb4ebc
> --- /dev/null
> +++ b/prefix-map.c
> @@ -0,0 +1,109 @@
> +#include "cache.h"
> +#include "prefix-map.h"
> +
> +static int map_cmp(const void *unused_cmp_data,
> +		   const void *entry,
> +		   const void *entry_or_key,
> +		   const void *unused_keydata)
> +{
> +	const struct prefix_map_entry *a = entry;
> +	const struct prefix_map_entry *b = entry_or_key;
> +
> +	return a->prefix_length != b->prefix_length ||
> +		strncmp(a->name, b->name, a->prefix_length);
> +}
> +
> +static void add_prefix_entry(struct hashmap *map, const char *name,
> +			     size_t prefix_length, struct prefix_item *item)
> +{
> +	struct prefix_map_entry *result = xmalloc(sizeof(*result));
> +	result->name = name;
> +	result->prefix_length = prefix_length;
> +	result->item = item;
> +	hashmap_entry_init(result, memhash(name, prefix_length));
> +	hashmap_add(map, result);
> +}
> +
> +static void init_prefix_map(struct prefix_map *prefix_map,
> +			    int min_prefix_length, int max_prefix_length)
> +{
> +	hashmap_init(&prefix_map->map, map_cmp, NULL, 0);
> +	prefix_map->min_length = min_prefix_length;
> +	prefix_map->max_length = max_prefix_length;
> +}
> +
> +static void add_prefix_item(struct prefix_map *prefix_map,
> +			    struct prefix_item *item)
> +{
> +	struct prefix_map_entry e = { { NULL } }, *e2;
> +	int j;
> +
> +	e.item = item;
> +	e.name = item->name;
> +
> +	for (j = prefix_map->min_length;
> +	     j <= prefix_map->max_length && e.name[j]; j++) {
> +		/* Avoid breaking UTF-8 multi-byte sequences */
> +		if (!isascii(e.name[j]))
> +			break;
> +
> +		e.prefix_length = j;
> +		hashmap_entry_init(&e, memhash(e.name, j));
> +		e2 = hashmap_get(&prefix_map->map, &e, NULL);
> +		if (!e2) {
> +			/* prefix is unique at this stage */
> +			item->prefix_length = j;
> +			add_prefix_entry(&prefix_map->map, e.name, j, item);
> +			break;
> +		}
> +
> +		if (!e2->item)
> +			continue; /* non-unique prefix */
> +
> +		if (j != e2->item->prefix_length || memcmp(e.name, e2->name, j))
> +			BUG("unexpected prefix length: %d != %d (%s != %s)",
> +			    j, (int)e2->item->prefix_length, e.name, e2->name);
> +
> +		/* skip common prefix */
> +		for (; j < prefix_map->max_length && e.name[j]; j++) {
> +			if (e.item->name[j] != e2->item->name[j])
> +				break;
> +			add_prefix_entry(&prefix_map->map, e.name, j + 1,
> +					 NULL);
> +		}
> +
> +		/* e2 no longer refers to a unique prefix */
> +		if (j < prefix_map->max_length && e2->name[j]) {
> +			/* found a new unique prefix for e2's item */
> +			e2->item->prefix_length = j + 1;
> +			add_prefix_entry(&prefix_map->map, e2->name, j + 1,
> +					 e2->item);
> +		}
> +		else
> +			e2->item->prefix_length = 0;
> +		e2->item = NULL;
> +
> +		if (j < prefix_map->max_length && e.name[j]) {
> +			/* found a unique prefix for the item */
> +			e.item->prefix_length = j + 1;
> +			add_prefix_entry(&prefix_map->map, e.name, j + 1,
> +					 e.item);
> +		} else
> +			/* item has no (short enough) unique prefix */
> +			e.item->prefix_length = 0;
> +
> +		break;
> +	}
> +}
> +
> +void find_unique_prefixes(struct prefix_item **list, size_t nr,
> +			  int min_length, int max_length)
> +{
> +	int i;
> +	struct prefix_map prefix_map;
> +
> +	init_prefix_map(&prefix_map, min_length, max_length);
> +	for (i = 0; i < nr; i++)
> +		add_prefix_item(&prefix_map, list[i]);
> +	hashmap_free(&prefix_map.map, 1);
> +}

Between the commit message, the in-code comment, the names of the new files, and implementation I was left somewhat confused about what this is about and how it works. TBH, I didn't even try to understand how all the above works, in particular the add_prefix_item() function.

However, I think it would be much-much simpler to first sort (a copy of?) the array of prefix item pointers based on their 'name' field, and then look for a unique prefix in each neighboring pair. Perhaps it would even be faster, because it doesn't have to allocate a bunch of hashmap items, though I don't think that it matters much in practice (i.e. I expect the number of items to be fairly small; presumably nobody will run interactive add after a mass refactoring modifying thousands of files).

Show 15 quoted lines
> diff --git a/prefix-map.h b/prefix-map.h
> new file mode 100644
> index 0000000000..ce3b8a4a32
> --- /dev/null
> +++ b/prefix-map.h
> @@ -0,0 +1,40 @@
> +#ifndef PREFIX_MAP_H
> +#define PREFIX_MAP_H
>
> +#include "hashmap.h"
> +
> +struct prefix_item {
> +	const char *name;
> +	size_t prefix_length;
> +};
This struct is part of find_unique_prefixes()'s signature, good.
Show 12 quoted lines
> +struct prefix_map_entry {
> +	struct hashmap_entry e;
> +	const char *name;
> +	size_t prefix_length;
> +	/* if item is NULL, the prefix is not unique */
> +	struct prefix_item *item;
> +};
> +
> +struct prefix_map {
> +	struct hashmap map;
> +	int min_length, max_length;
> +};

However, neither of these two structs nor the hashmap appear in the function's signature, but are all implementation details. Therefore, they should not be defined and included here in the header but in the .c source file. (But as mentioned above, I think this could be implemented much simpler without these data structures.)

Furthermore, this is not a map. A map, in general, is a container of key-value pairs that allows efficient insertion, removal and lookup. This so-called prefix_map does none of that, so it should not be called a map.

> +/*
> + * Find unique prefixes in a given list of strings.

... and stores the length of the unique prefixes in the 'prefix_length' field of the elements of the given array.

> + *
> + * Typically, the `struct prefix_item` information will be but a field in the
s/but //, perhaps?
Show 10 quoted lines
> + * actual item struct; For this reason, the `list` parameter is specified as a
> + * list of pointers to the items.
> + *
> + * The `min_length`/`max_length` parameters define what length the unique
> + * prefixes should have.
> + *
> + * If no unique prefix could be found for a given item, its `prefix_length`
> + * will be set to 0.
> + */
> +void find_unique_prefixes(struct prefix_item **list, size_t nr,
The first argument is not a list but an array.
> +			  int min_length, int max_length);

size_t, perhaps? These are closely related to 'prefix_item.prefix_length', which is already (rightfully) size_t.

Show 42 quoted lines
> +
> +#endif
> diff --git a/t/helper/test-prefix-map.c b/t/helper/test-prefix-map.c
> new file mode 100644
> index 0000000000..3f1c90eaf0
> --- /dev/null
> +++ b/t/helper/test-prefix-map.c
> @@ -0,0 +1,58 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +#include "prefix-map.h"
> +
> +static size_t test_count, failed_count;
> +
> +static void check(int succeeded, const char *file, size_t line_no,
> +		  const char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	test_count++;
> +	if (succeeded)
> +		return;
> +
> +	va_start(ap, fmt);
> +	fprintf(stderr, "%s:%d: ", file, (int)line_no);
> +	vfprintf(stderr, fmt, ap);
> +	fputc('\n', stderr);
> +	va_end(ap);
> +
> +	failed_count++;
> +}
> +
> +#define EXPECT_SIZE_T_EQUALS(expect, actual, hint) \
> +	check(expect == actual, __FILE__, __LINE__, \
> +	      "size_t's do not match: %" \
> +	      PRIdMAX " != %" PRIdMAX " (%s) (%s)", \
> +	      (intmax_t)expect, (intmax_t)actual, #actual, hint)
> +
> +int cmd__prefix_map(int argc, const char **argv)
> +{
> +#define NR 5
> +	struct prefix_item items[NR] = {

You don't have to tell the compiler how many elements this array will contain, it will figure that out on its own.

Show 7 quoted lines
> +		{ "unique" },
> +		{ "hell" },
> +		{ "hello" },
> +		{ "wok" },
> +		{ "world" },
> +	};
> +	struct prefix_item *list[NR] = {
Likewise.
> +		items, items + 1, items + 2, items + 3, items + 4
> +	};
> +
> +	find_unique_prefixes(list, NR, 1, 3);

This could be find_unique_prefixes(list, ARRAY_SIZE(list), 1, 3), and then there is no need for that NR macro anymore.

Show 13 quoted lines
> +
> +#define EXPECT_PREFIX_LENGTH_EQUALS(expect, index) \
> +	EXPECT_SIZE_T_EQUALS(expect, list[index]->prefix_length, \
> +			     list[index]->name)
> +
> +	EXPECT_PREFIX_LENGTH_EQUALS(1, 0);
> +	EXPECT_PREFIX_LENGTH_EQUALS(0, 1);
> +	EXPECT_PREFIX_LENGTH_EQUALS(0, 2);
> +	EXPECT_PREFIX_LENGTH_EQUALS(3, 3);
> +	EXPECT_PREFIX_LENGTH_EQUALS(3, 4);
> +
> +	return !!failed_count;
> +}
Previous: Junio C HamanoNext: Johannes Schindelin
Message 51 of 124 in “git add -i: add a rudimentary version in C (supporting only status and help so far)”
  1. 00/11 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Apr 10, 2019
  2. 01/11 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Apr 10, 2019
  3. Jeff HostetlerApr 18, 2019
  4. Jeff KingApr 18, 2019
  5. Johannes SchindelinApr 30, 2019
  6. Jeff KingMay 1, 2019
  7. Johannes SchindelinMay 13, 2019
  8. 02/11 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Apr 10, 2019
  9. 04/11 built-in add -i: refresh the index before running `status`Johannes Schindelin via GitGitGadget, Apr 10, 2019
  10. 03/11 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Apr 10, 2019
  11. 05/11 built-in add -i: color the header in the `status` commandJohannes Schindelin via GitGitGadget, Apr 10, 2019
  12. 06/11 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Apr 10, 2019
  13. Jeff HostetlerApr 18, 2019
  14. Johannes SchindelinMay 13, 2019
  15. 09/11 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Apr 10, 2019
  16. 08/11 built-in add -i: show unique prefixes of the commandsSlavica Djukic via GitGitGadget, Apr 10, 2019
  17. 07/11 Add a function to determine unique prefixes for a list of stringsSlavica Djukic via GitGitGadget, Apr 10, 2019
  18. Jeff HostetlerApr 18, 2019
  19. Johannes SchindelinMay 13, 2019
  20. 10/11 built-in add -i: use color in the main loopSlavica Djukic via GitGitGadget, Apr 10, 2019
  21. 11/11 built-in add -i: implement the `help` commandJohannes Schindelin via GitGitGadget, Apr 10, 2019
  22. 00/11 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, May 13, 2019
  23. 02/11 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, May 13, 2019
  24. 04/11 built-in add -i: refresh the index before running `status`Johannes Schindelin via GitGitGadget, May 13, 2019
  25. 06/11 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, May 13, 2019
  26. 08/11 built-in add -i: show unique prefixes of the commandsSlavica Djukic via GitGitGadget, May 13, 2019
  27. 11/11 built-in add -i: implement the `help` commandJohannes Schindelin via GitGitGadget, May 13, 2019
  28. 10/11 built-in add -i: use color in the main loopSlavica Djukic via GitGitGadget, May 13, 2019
  29. 07/11 Add a function to determine unique prefixes for a list of stringsSlavica Djukic via GitGitGadget, May 13, 2019
  30. 05/11 built-in add -i: color the header in the `status` commandJohannes Schindelin via GitGitGadget, May 13, 2019
  31. 09/11 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, May 13, 2019
  32. 01/11 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, May 13, 2019
  33. 03/11 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, May 13, 2019
  34. 00/11 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Jul 16, 2019
  35. 01/11 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Jul 16, 2019
  36. Junio C HamanoJul 31, 2019
  37. Johannes SchindelinAug 26, 2019
  38. Junio C HamanoAug 27, 2019
  39. Johannes SchindelinAug 28, 2019
  40. Junio C HamanoAug 28, 2019
  41. 02/11 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Jul 16, 2019
  42. Junio C HamanoJul 31, 2019
  43. Johannes SchindelinAug 27, 2019
  44. 05/11 built-in add -i: color the header in the `status` commandJohannes Schindelin via GitGitGadget, Jul 16, 2019
  45. 04/11 built-in add -i: refresh the index before running `status`Johannes Schindelin via GitGitGadget, Jul 16, 2019
  46. 06/11 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Jul 16, 2019
  47. Junio C HamanoJul 31, 2019
  48. 08/11 built-in add -i: show unique prefixes of the commandsSlavica Djukic via GitGitGadget, Jul 16, 2019
  49. 07/11 Add a function to determine unique prefixes for a list of stringsSlavica Djukic via GitGitGadget, Jul 16, 2019
  50. Junio C HamanoJul 31, 2019
  51. SZEDER GáborAug 24, 2019
  52. Johannes SchindelinAug 27, 2019
  53. SZEDER GáborAug 28, 2019
  54. [PoC] A simpler find_unique_prefixes() implementationSZEDER Gábor, Aug 28, 2019
  55. Johannes SchindelinAug 30, 2019
  56. 09/11 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Jul 16, 2019
  57. 03/11 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Jul 16, 2019
  58. Junio C HamanoJul 31, 2019
  59. Johannes SchindelinAug 27, 2019
  60. 10/11 built-in add -i: use color in the main loopSlavica Djukic via GitGitGadget, Jul 16, 2019
  61. 11/11 built-in add -i: implement the `help` commandJohannes Schindelin via GitGitGadget, Jul 16, 2019
  62. Junio C HamanoAug 2, 2019
  63. Jeff KingAug 2, 2019
  64. Johannes SchindelinJul 16, 2019
  65. Junio C HamanoAug 2, 2019
  66. 00/11 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Aug 27, 2019
  67. 01/11 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Aug 27, 2019
  68. 08/11 built-in add -i: show unique prefixes of the commandsSlavica Djukic via GitGitGadget, Aug 27, 2019
  69. 03/11 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Aug 27, 2019
  70. 06/11 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Aug 27, 2019
  71. 04/11 built-in add -i: refresh the index before running `status`Johannes Schindelin via GitGitGadget, Aug 27, 2019
  72. 09/11 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Aug 27, 2019
  73. 11/11 built-in add -i: implement the `help` commandJohannes Schindelin via GitGitGadget, Aug 27, 2019
  74. 10/11 built-in add -i: use color in the main loopSlavica Djukic via GitGitGadget, Aug 27, 2019
  75. 02/11 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Aug 27, 2019
  76. 07/11 Add a function to determine unique prefixes for a list of stringsSlavica Djukic via GitGitGadget, Aug 27, 2019
  77. 05/11 built-in add -i: color the header in the `status` commandJohannes Schindelin via GitGitGadget, Aug 27, 2019
  78. 0/9 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Nov 4, 2019
  79. 1/9 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Nov 4, 2019
  80. Junio C HamanoNov 8, 2019
  81. Johannes SchindelinNov 9, 2019
  82. Junio C HamanoNov 10, 2019
  83. Johannes SchindelinNov 11, 2019
  84. Junio C HamanoNov 11, 2019
  85. Johannes SchindelinNov 12, 2019
  86. Junio C HamanoNov 13, 2019
  87. Johannes SchindelinNov 13, 2019
  88. Junio C HamanoNov 13, 2019
  89. 4/9 built-in add -i: color the header in the `status` commandSlavica Đukić via GitGitGadget, Nov 4, 2019
  90. 3/9 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Nov 4, 2019
  91. Junio C HamanoNov 8, 2019
  92. 2/9 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Nov 4, 2019
  93. Junio C HamanoNov 8, 2019
  94. 7/9 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Nov 4, 2019
  95. 5/9 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Nov 4, 2019
  96. Junio C HamanoNov 8, 2019
  97. Johannes SchindelinNov 9, 2019
  98. 8/9 built-in add -i: use color in the main loopSlavica Đukić via GitGitGadget, Nov 4, 2019
  99. 9/9 built-in add -i: implement the `help` commandSlavica Đukić via GitGitGadget, Nov 4, 2019
  100. 6/9 built-in add -i: show unique prefixes of the commandsJohannes Schindelin via GitGitGadget, Nov 4, 2019
  101. 0/9 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Nov 13, 2019
  102. 2/9 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Nov 13, 2019
  103. 1/9 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Nov 13, 2019
  104. Junio C HamanoNov 14, 2019
  105. Johannes SchindelinNov 14, 2019
  106. Junio C HamanoNov 15, 2019
  107. 3/9 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Nov 13, 2019
  108. 6/9 built-in add -i: show unique prefixes of the commandsJohannes Schindelin via GitGitGadget, Nov 13, 2019
  109. 7/9 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Nov 13, 2019
  110. 9/9 built-in add -i: implement the `help` commandSlavica Đukić via GitGitGadget, Nov 13, 2019
  111. 5/9 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Nov 13, 2019
  112. 8/9 built-in add -i: use color in the main loopSlavica Đukić via GitGitGadget, Nov 13, 2019
  113. 4/9 built-in add -i: color the header in the `status` commandSlavica Đukić via GitGitGadget, Nov 13, 2019
  114. Johannes SchindelinNov 13, 2019
  115. 0/9 git add -i: add a rudimentary version in C (supporting only status and help so far)Johannes Schindelin via GitGitGadget, Nov 15, 2019
  116. 1/9 Start to implement a built-in version of `git add --interactive`Johannes Schindelin via GitGitGadget, Nov 15, 2019
  117. 4/9 built-in add -i: color the header in the `status` commandSlavica Đukić via GitGitGadget, Nov 15, 2019
  118. 7/9 built-in add -i: support `?` (prompt help)Johannes Schindelin via GitGitGadget, Nov 15, 2019
  119. 5/9 built-in add -i: implement the main loopJohannes Schindelin via GitGitGadget, Nov 15, 2019
  120. 2/9 diff: export diffstat interfaceDaniel Ferreira via GitGitGadget, Nov 15, 2019
  121. 6/9 built-in add -i: show unique prefixes of the commandsJohannes Schindelin via GitGitGadget, Nov 15, 2019
  122. 3/9 built-in add -i: implement the `status` commandDaniel Ferreira via GitGitGadget, Nov 15, 2019
  123. 8/9 built-in add -i: use color in the main loopSlavica Đukić via GitGitGadget, Nov 15, 2019
  124. 9/9 built-in add -i: implement the `help` commandSlavica Đukić via GitGitGadget, Nov 15, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.