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

Re: [PATCH v3 11/19] dir.c: use a single struct exclude_list per source of excludes

From
Adam Spiers <git@adamspiers.org>
Date
Jan 6, 2013, 22:53 UTC
Message-ID
<20130106225311.GB6552@pacific.linksys.moosehall>
In-Reply-To
<7v7gnqnjn7.fsf@alter.siamese.dyndns.org>
On Sun, Jan 06, 2013 at 12:25:48PM -0800, Junio C Hamano wrote:
Show 42 quoted lines
> Adam Spiers <git@adamspiers.org> writes:
> 
> > On Fri, Jan 04, 2013 at 01:03:59PM -0800, Junio C Hamano wrote:
> >> Adam Spiers <git@adamspiers.org> writes:
> >> 
> >> > diff --git a/builtin/clean.c b/builtin/clean.c
> >> > index 0c7b3d0..bd18b88 100644
> >> > --- a/builtin/clean.c
> >> > +++ b/builtin/clean.c
> >> > @@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> >> >  	if (!ignored)
> >> >  		setup_standard_excludes(&dir);
> >> >  
> >> > +	add_exclude_list(&dir, EXC_CMDL);
> >> >  	for (i = 0; i < exclude_list.nr; i++)
> >> >  		add_exclude(exclude_list.items[i].string, "", 0,
> >> > -			    &dir.exclude_list[EXC_CMDL]);
> >> > +			    &dir.exclude_list_groups[EXC_CMDL].ary[0]);
> >> 
> >> This looks somewhat ugly for two reasons.
> >> 
> >>  * The abstraction add_exclude() offers to its callers is just to
> >>    let them add one pattern to the list of patterns for the kind
> >>    (here, EXC_CMDL); why should they care about .ary[0] part?
> >
> > Because the caller has to decide which exclude list the new exclude
> > should be added to; that is how it has been for a long time, and I am
> > not proposing we change that.
> 
> Unless I was mistaken, I never objected to the EXC_CMDL, etc
> appearing in the text of the calling site of add_exclude().
> 
> The objection was about the .ary[0] bit.  From the point of view of
> a caller of the API, it:
> 
>     - calls add_exclude_list() to declare "I now start adding new
>       patterns that come from a new source of patterns"; then
> 
>     - calls add_exclude() repeatedly to add the patterns that come
>       from that source.
> 
> no?
Correct.
> Why does the latter has to keep repeating "Here is the new
> pattern for the EXC_CMDL group; it comes from the latest source I
> earlier declared, by the way", instead of just "Here is the new
> pattern for the EXC_CMDL group"?
Mainly because there is no guarantee that such a group exists.
unpack_trees() has:
	if (add_excludes_from_file_to_list(git_path("info/sparse-checkout"), "", 0, &el, 0) < 0)

so if you change the signature of add_exclude() to require an exclude_list_group, then there is no way to implement add_excludes_from_file_to_list().

Even if you could, you still haven't reduced the number of parameters add_exclude() requires, so I'm dubious of the benefits of this "simplification".

Show 18 quoted lines
> >>    Are
> >>    there cases any sane caller (not the implementation of the
> >>    exclude_list_group machinery e.g. add_excludes_from_... function)
> >>    may want to call it with .ary[1]?
> >
> > Effectively yes, although it is not written like ".ary[1]".  For
> > example prep_exclude() calls add_excludes_from_file_to_list() for each
> > new .gitignore file
> 
> That is part of the "implementation of the machinery".  If the API
> for the outside callers are to call add_exclude_list() to declare
> that patterns added by subsequent calls to add_exclude() are from
> one new source of the patterns (e.g. .gitignore file in a new
> directory level), and then call add_exclude() to add each pattern,
> then the callers to add_exclude() shouldn't have to care about the
> implementation detail that individual sources in exclude_list_group
> is implemented as an array in that sructure, and the latest ones
> should go to its ->array[0].

That's a valid point. However, the ary[0] part which assumes external knowledge of the internal implementation can trivially be avoided by squashing this patch onto the commit we are discussing:

diff --git a/builtin/clean.c b/builtin/clean.c
index dd89737..6e21ca6 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -45,6 +45,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	static const char **pathspec;
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
+	struct exclude_list *el;
 	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
@@ -97,10 +98,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (!ignored)
 		setup_standard_excludes(&dir);
 
-	add_exclude_list(&dir, EXC_CMDL);
+	el = add_exclude_list(&dir, EXC_CMDL);
 	for (i = 0; i < exclude_list.nr; i++)
-		add_exclude(exclude_list.items[i].string, "", 0,
-			    &dir.exclude_list_group[EXC_CMDL].el[0]);
+		add_exclude(exclude_list.items[i].string, "", 0, el);
 
 	pathspec = get_pathspec(prefix, argv);


and by adopting the same approach for ls-files.c:

 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 0ca9d8e..0406adc 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -420,10 +420,11 @@ static int option_parse_z(const struct option *opt,
 static int option_parse_exclude(const struct option *opt,
 				const char *arg, int unset)
 {
-	struct exclude_list_group *group = opt->value;
+	struct string_list *exclude_list = opt->value;
 
 	exc_given = 1;
-	add_exclude(arg, "", 0, &group->el[0]);
+	string_list_append(exclude_list, arg);
+	fprintf(stderr, "append %s\n", arg);
 
 	return 0;
 }
@@ -452,9 +453,11 @@ static int option_parse_exclude_standard(const struct option *opt,
 
 int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
-	int require_work_tree = 0, show_tag = 0;
+	int require_work_tree = 0, show_tag = 0, i;
 	const char *max_prefix;
 	struct dir_struct dir;
+	struct exclude_list *el;
+	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct option builtin_ls_files_options[] = {
 		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 			"paths are separated with NUL character",
@@ -489,7 +492,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOLEAN(0, "resolve-undo", &show_resolve_undo,
 			    "show resolve-undo information"),
 		{ OPTION_CALLBACK, 'x', "exclude",
-			&dir.exclude_list_group[EXC_CMDL], "pattern",
+			&exclude_list, "pattern",
 			"skip files matching pattern",
 			0, option_parse_exclude },
 		{ OPTION_CALLBACK, 'X', "exclude-from", &dir, "file",
@@ -524,9 +527,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	add_exclude_list(&dir, EXC_CMDL);
 	argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 			ls_files_usage, 0);
+	el = add_exclude_list(&dir, EXC_CMDL);
+	for (i = 0; i < exclude_list.nr; i++) {
+		fprintf(stderr, "adding exclude: %s\n", exclude_list.items[i].string);
+		add_exclude(exclude_list.items[i].string, "", 0, el);
+	}
 	if (show_tag || show_valid_bit) {
 		tag_cached = "H ";
 		tag_unmerged = "M ";

> The implementation of the machinery may find it more convenient if
> they can add one or more "sources" to an exclude_list_group before
> starting to add patterns to ->array[0] or ->array[1] or ->array[2],
> and a finer grained internal API that lets the caller pass an
> instance of "struct exclude_list" regardless of where in an
> exclude_list_group's ary[] that instance sits may be necessary to do
> so.
> 
> But that does not mean other existing callers has to be aware of
> such inner detail.  If the implementation of the machinery needs a
> helper function that adds an element to any struct exclude_list, not
> necessarily the one at the tip of an exclude_list_group, we can
> still do that by having the bulk of the logic in the internal, finer
> grained helper, say, add_pattern_to_exclude_list(), and keep the
> external API simpler by making it a thin wrapper around it, perhaps
> like:
> 
>    static void add_pattern_to_exclude_list(const char *pattern,
>    		    const char *base, int baselen,
>                     struct exclude_list *el);
> 
>    void add_exclude(const char *pattern,
>    		    const char *base, int baselen,
>                     struct exclude_list_group *group) {
> 	add_pattern_to_exclude_list(pattern, base, baselen, &group->ary[0]);

Presumably you mean

	add_pattern_to_exclude_list(pattern, base, baselen,
				    &group->ary[group->nr - 1]);

(although at your request, I already renamed 'ary' to 'el').

I have made a genuine attempt to implement your suggestion, but due to
the unpack_trees() case stated above, I don't see how it can be done.
Previous: Junio C HamanoNext: Adam Spiers
Message 25 of 55 in “new git check-ignore sub-command”
  1. 00/19 new git check-ignore sub-commandAdam Spiers, Dec 27, 2012
  2. 01/19 api-directory-listing.txt: update to match codeAdam Spiers, Dec 27, 2012
  3. 02/19 Improve documentation and comments regarding directory traversal APIAdam Spiers, Dec 27, 2012
  4. Junio C HamanoJan 1, 2013
  5. Adam SpiersJan 2, 2013
  6. Adam SpiersJan 6, 2013
  7. 03/19 dir.c: rename cryptic 'which' variable to more consistent nameAdam Spiers, Dec 27, 2012
  8. 04/19 dir.c: rename path_excluded() to is_path_excluded()Adam Spiers, Dec 27, 2012
  9. 05/19 dir.c: rename excluded_from_list() to is_excluded_from_list()Adam Spiers, Dec 27, 2012
  10. 06/19 dir.c: rename excluded() to is_excluded()Adam Spiers, Dec 27, 2012
  11. 07/19 dir.c: refactor is_excluded_from_list()Adam Spiers, Dec 27, 2012
  12. 08/19 dir.c: refactor is_excluded()Adam Spiers, Dec 27, 2012
  13. 09/19 dir.c: refactor is_path_excluded()Adam Spiers, Dec 27, 2012
  14. 10/19 dir.c: rename free_excludes() to clear_exclude_list()Adam Spiers, Dec 27, 2012
  15. 11/19 dir.c: use a single struct exclude_list per source of excludesAdam Spiers, Dec 27, 2012
  16. Junio C HamanoJan 4, 2013
  17. Junio C HamanoJan 5, 2013
  18. Adam SpiersJan 6, 2013
  19. api-allocation-growing.txt: encourage better variable namingAdam Spiers, Jan 6, 2013
  20. Junio C HamanoJan 6, 2013
  21. Adam SpiersJan 6, 2013
  22. Junio C HamanoJan 6, 2013
  23. Adam SpiersJan 6, 2013
  24. Junio C HamanoJan 6, 2013
  25. Adam SpiersJan 6, 2013
  26. Adam SpiersJan 6, 2013
  27. Junio C HamanoJan 6, 2013
  28. 12/19 dir.c: keep track of where patterns came fromAdam Spiers, Dec 27, 2012
  29. 13/19 dir.c: provide clear_directory() for reclaiming dir_struct memoryAdam Spiers, Dec 27, 2012
  30. 14/19 add.c: refactor treat_gitlinks()Adam Spiers, Dec 27, 2012
  31. 15/19 add.c: remove unused argument from validate_pathspec()Adam Spiers, Dec 27, 2012
  32. 16/19 pathspec.c: move reusable code from builtin/add.cAdam Spiers, Dec 27, 2012
  33. Junio C HamanoDec 28, 2012
  34. Adam SpiersDec 28, 2012
  35. Adam SpiersDec 29, 2012
  36. Junio C HamanoDec 28, 2012
  37. Adam SpiersDec 28, 2012
  38. 17/19 pathspec.c: extract new validate_path() for reuseAdam Spiers, Dec 27, 2012
  39. Junio C HamanoDec 28, 2012
  40. Adam SpiersDec 28, 2012
  41. 18/19 setup.c: document get_pathspec()Adam Spiers, Dec 27, 2012
  42. Junio C HamanoDec 28, 2012
  43. Adam SpiersDec 28, 2012
  44. Adam SpiersDec 29, 2012
  45. Junio C HamanoDec 29, 2012
  46. 19/19 Add git-check-ignore sub-commandAdam Spiers, Dec 27, 2012
  47. Junio C HamanoDec 28, 2012
  48. Adam SpiersDec 29, 2012
  49. Adam SpiersDec 29, 2012
  50. Michael LealDec 27, 2012
  51. Junio C HamanoDec 28, 2012
  52. Adam SpiersDec 28, 2012
  53. Antoine PelisseDec 28, 2012
  54. Junio C HamanoDec 28, 2012
  55. Junio C HamanoDec 28, 2012

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.