threads / rfc / 42233

RFC patchpathspec: get non matching arguments without reporting.

Subject: [RFC PATCH] pathspec: get non matching arguments without reporting.

## tl;dr

3 messages between May 5, 2016 and May 5, 2016. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Stefan Beller· May 5, 2016, 22:59 UTC · lore

When giving more than just pathspec arguments (e.g. submodule labels), we need a way to check all non-matching arguments for the pathspec parsing if these are the submodule labels or typos.

This patch prepares for that use case by splitting up `report_path_error` into a new checking function `unmatched_pathspec_items` and a reporting callback.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
  So I imagine the unmatched_pathspec_items to be used later similar as
  in report_path_error, maybe just like this:
void submodule_list_fn(const struct pathspec *pathspec,
			  int pathspec_index,
			  void *data_cb)
{
	if (submodule_label_exist(pathspec->items[pathspec_index].original))
		// ok, record in data_cb
	else
		error(" '%s' is neither recognized as matching pathspec nor did it
			match any submodule label",
			pathspec->items[pathspec_index].original);
}
submodule_list(const char *ps_matched,
		 const struct pathspec *pathspec,
		 const char *prefix)
{
	struct string_list detected_labels;
	unmatched_pathspec_items(ps_matched, pathspec, prefix,
				submodule_list_fn, &detected_labels);
	
	foreach (submodule s) { 
		if matches_pathspec(s, pathspec) or label_match(s, detected_labels)
			// do a thing
		else
			continue
	}
}

What do you think of such a design? Is it worth carrying and polishing or would there be another way to do it which matches the pathspec mechanism better?

Thanks, Stefan

 dir.c | 36 ++++++++++++++++++++++++++----------
 dir.h |  8 ++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)
Show changes to 2 files +34 −10

dir.c, dir.h

diff --git a/dir.c b/dir.c
index a4a9d9f..bc8b199 100644
--- a/dir.c
+++ b/dir.c
@@ -394,14 +394,13 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
-int report_path_error(const char *ps_matched,
-		      const struct pathspec *pathspec,
-		      const char *prefix)
+void unmatched_pathspec_items(const char *ps_matched,
+			     const struct pathspec *pathspec,
+			     const char *prefix,
+			     unmatched_pathspec_items_fn fn,
+			     void *data_cb)
 {
-	/*
-	 * Make sure all pathspec matched; otherwise it is an error.
-	 */
-	int num, errors = 0;
+	int num;
 	for (num = 0; num < pathspec->nr; num++) {
 		int other, found_dup;
 
@@ -428,10 +427,27 @@ int report_path_error(const char *ps_matched,
 		if (found_dup)
 			continue;
 
-		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec->items[num].original);
-		errors++;
+		fn(pathspec, num, data_cb);
 	}
+}
+
+void report_path_error_fn(const struct pathspec *pathspec,
+			  int pathspec_index,
+			  void *data_cb)
+{
+	int *errors = data_cb;
+	error("pathspec '%s' did not match any file(s) known to git.",
+	      pathspec->items[pathspec_index].original);
+	(*errors)++;
+}
+
+int report_path_error(const char *ps_matched,
+		      const struct pathspec *pathspec,
+		      const char *prefix)
+{
+	int errors = 0;
+	unmatched_pathspec_items(ps_matched, pathspec, prefix,
+				report_path_error_fn, &errors);
 	return errors;
 }
 
diff --git a/dir.h b/dir.h
index cd46f30..ea222eb 100644
--- a/dir.h
+++ b/dir.h
@@ -211,6 +211,14 @@ extern char *common_prefix(const struct pathspec *pathspec);
 extern int match_pathspec(const struct pathspec *pathspec,
 			  const char *name, int namelen,
 			  int prefix, char *seen, int is_dir);
+typedef void (*unmatched_pathspec_items_fn)(const struct pathspec *pathspec,
+					    int pathspec_index,
+					    void *data_cb);
+void unmatched_pathspec_items(const char *ps_matched,
+			     const struct pathspec *pathspec,
+			     const char *prefix,
+			     unmatched_pathspec_items_fn fn,
+			     void *data_cb);
 extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);
 extern int within_depth(const char *name, int namelen, int depth, int max_depth);
 

-- 
2.8.0.1.g3af9c03
Junio C Hamano· May 5, 2016, 23:11 UTC · re: Stefan Beller · lore

Re: [RFC PATCH] pathspec: get non matching arguments without reporting.

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> When giving more than just pathspec arguments (e.g. submodule labels),
> we need a way to check all non-matching arguments for the pathspec parsing
> if these are the submodule labels or typos.
>
> This patch prepares for that use case by splitting up `report_path_error`
> into a new checking function `unmatched_pathspec_items` and a
> reporting callback.

I seem to recall that there is a longstanding plan to move the "seen[]" array that is separate and outside from the pathspec structure into the pathspec structure. Wouldn't that be a more sensible approach to solve the same thing?

Junio C Hamano· May 5, 2016, 23:22 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] pathspec: get non matching arguments without reporting.

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> When giving more than just pathspec arguments (e.g. submodule labels),
>> we need a way to check all non-matching arguments for the pathspec parsing
>> if these are the submodule labels or typos.
>>
>> This patch prepares for that use case by splitting up `report_path_error`
>> into a new checking function `unmatched_pathspec_items` and a
>> reporting callback.
>
> I seem to recall that there is a longstanding plan to move the
> "seen[]" array that is separate and outside from the pathspec
> structure into the pathspec structure.  Wouldn't that be a more
> sensible approach to solve the same thing?

Even with such a refactoring, the need to split the "preprocessing" (mainly "dedup") still remains if you want to silence this codepath, so I think this is OK from pathspec/dir.c point of view.

But I do not agree with the first paragraph at all. You'd be using separate syntax like '*label', ':name', etc. for the purpose of enumerating submodules, so instead of blindly passing argv[] to pathspec code, I expected you to do a preprocessing of argv[] upfront, sifting them into three bins (labels, names and paths), and giving only the last ones to the pathspec machinery.

And if you did that, report_path_error() would never have to worry about "ah, this thing does not exist in the working tree or in the index but that is natural because it is a submodule label" at all, no?

← back to recent threads