threads / patch / 47912

v2ref-filter: Make "--contains <id>" less chatty if <id> is invalid

Subject: [GSoC][PATCH v2] ref-filter: Make "--contains <id>" less chatty if <id> is invalid

## tl;dr

4 messages between Feb 23, 2018 and Feb 24, 2018. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Paul-Sebastian Ungureanu· Feb 23, 2018, 16:25 UTC · lore

Hello, I have made the changes after review. This is the updated patch based on what was discussed last time [1].

In this patch, I have fixed the same issue that was also seen in "git branch" and "git for-reach-ref". I have also removed the dead code that was left and updated the patches accordingly.

[1] https://public-inbox.org/git/20180219212130.4217-1-ungureanupaulsebastian@gmail.com/

Best regards, Paul Ungureanu

https://public-inbox.org/git/20180219212130.4217-1-ungureanupaulsebastian@gmail.com/
---

Some git commands which use --contains <id> print the whole help text if <id> is invalid. It should only show the error message instead.

This patch applies to "git tag", "git branch", "git for-each-ref".

This bug was a side effect of looking up the commit in option parser callback. When a error occurs in the option parser, the full usage is shown. To fix this bug, the part related to looking up the commit was moved outside of the option parser to the ref-filter module.

Basically, the option parser only parses strings that represent commits and the ref-filter performs the commit look-up. If an error occurs during the option parsing, then it must be an invalid argument and the user should be informed of usage, but if a error occurs during ref-filtering, then it is a problem with the argument.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
 builtin/branch.c       | 12 +++----
 builtin/for-each-ref.c |  4 +--
 builtin/tag.c          | 16 ++++-----
 parse-options.h        |  3 +-
 ref-filter.c           | 23 +++++++++++++
 ref-filter.h           |  3 ++
 t/tcontains.sh         | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 136 insertions(+), 17 deletions(-)
 create mode 100755 t/tcontains.sh
Show changes to 7 files +136 −17

builtin/branch.c, builtin/for-each-ref.c, builtin/tag.c, parse-options.h, ref-filter.c, ref-filter.h, t/tcontains.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 8dcc2ed05..43442c12e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -596,10 +596,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT__COLOR(&branch_use_color, N_("use colored output")),
 		OPT_SET_INT('r', "remotes",     &filter.kind, N_("act on remote-tracking branches"),
 			FILTER_REFS_REMOTES),
-		OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
-		OPT_NO_CONTAINS(&filter.no_commit, N_("print only branches that don't contain the commit")),
-		OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
-		OPT_WITHOUT(&filter.no_commit, N_("print only branches that don't contain the commit")),
+		OPT_CONTAINS(&filter.with_commit_strs, N_("print only branches that contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only branches that don't contain the commit")),
+		OPT_WITH(&filter.with_commit_strs, N_("print only branches that contain the commit")),
+		OPT_WITHOUT(&filter.no_commit_strs, N_("print only branches that don't contain the commit")),
 		OPT__ABBREV(&filter.abbrev),
 
 		OPT_GROUP(N_("Specific git-branch actions:")),
@@ -657,8 +657,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-	    filter.no_commit)
+	if (filter.with_commit_strs.nr || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
+	    filter.no_commit_strs.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!copy + !!new_upstream +
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e931be9ce..deb9a779a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -44,8 +44,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 			     parse_opt_object_name),
 		OPT_MERGED(&filter, N_("print only refs that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only refs that are not merged")),
-		OPT_CONTAINS(&filter.with_commit, N_("print only refs which contain the commit")),
-		OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
+		OPT_CONTAINS(&filter.with_commit_strs, N_("print only refs which contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only refs which don't contain the commit")),
 		OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_END(),
 	};
diff --git a/builtin/tag.c b/builtin/tag.c
index 8885e21dd..6be7f53ae 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -396,10 +396,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
-		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
-		OPT_NO_CONTAINS(&filter.no_commit, N_("print only tags that don't contain the commit")),
-		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
-		OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
+		OPT_CONTAINS(&filter.with_commit_strs, N_("print only tags that contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only tags that don't contain the commit")),
+		OPT_WITH(&filter.with_commit_strs, N_("print only tags that contain the commit")),
+		OPT_WITHOUT(&filter.no_commit_strs, N_("print only tags that don't contain the commit")),
 		OPT_MERGED(&filter, N_("print only tags that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
@@ -435,8 +435,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!cmdmode) {
 		if (argc == 0)
 			cmdmode = 'l';
-		else if (filter.with_commit || filter.no_commit ||
-			 filter.points_at.nr || filter.merge_commit ||
+		else if (filter.points_at.nr || filter.merge_commit ||
+			 filter.with_commit_strs.nr || filter.no_commit_strs.nr ||
 			 filter.lines != -1)
 			cmdmode = 'l';
 	}
@@ -473,9 +473,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (filter.lines != -1)
 		die(_("-n option is only allowed in list mode"));
-	if (filter.with_commit)
+	if (filter.with_commit_strs.nr)
 		die(_("--contains option is only allowed in list mode"));
-	if (filter.no_commit)
+	if (filter.no_commit_strs.nr)
 		die(_("--no-contains option is only allowed in list mode"));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed in list mode"));
diff --git a/parse-options.h b/parse-options.h
index af711227a..4b4734f2e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -256,8 +256,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
 #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \
 	{ OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
 	  PARSE_OPT_LASTARG_DEFAULT | flag, \
-	  parse_opt_commits, (intptr_t) "HEAD" \
+	  parse_opt_string_list, (intptr_t) "HEAD" \
 	}
+
 #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
 #define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
 #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)
diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7..aa282a27f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2000,6 +2000,25 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 	free(to_clear);
 }
 
+int add_str_to_commit_list(struct string_list_item *item, void *commit_list)
+{
+	struct object_id oid;
+	struct commit *commit;
+
+	if (get_oid(item->string, &oid)) {
+		error(_("malformed object name %s"), item->string);
+		exit(1);
+	}
+	commit = lookup_commit_reference(&oid);
+	if (!commit) {
+		error(_("no such commit %s"), item->string);
+		exit(1);
+	}
+	commit_list_insert(commit, commit_list);
+
+	return 0;
+}
+
 /*
  * API for filtering a set of refs. Based on the type of refs the user
  * has requested, we iterate through those refs and apply filters
@@ -2012,6 +2031,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	int ret = 0;
 	unsigned int broken = 0;
 
+	/* Convert string representation and add to commit list. */
+	for_each_string_list(&filter->with_commit_strs, add_str_to_commit_list, &filter->with_commit);
+	for_each_string_list(&filter->no_commit_strs, add_str_to_commit_list, &filter->no_commit);
+
 	ref_cbdata.array = array;
 	ref_cbdata.filter = filter;
 
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..62f37760f 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -55,6 +55,9 @@ struct ref_filter {
 	struct commit_list *with_commit;
 	struct commit_list *no_commit;
 
+	struct string_list with_commit_strs;
+	struct string_list no_commit_strs;
+
 	enum {
 		REF_FILTER_MERGED_NONE = 0,
 		REF_FILTER_MERGED_INCLUDE,
diff --git a/t/tcontains.sh b/t/tcontains.sh
new file mode 100755
index 000000000..4856111ff
--- /dev/null
+++ b/t/tcontains.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='Test "contains" argument behavior'
+
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+	git init . &&
+	echo "this is a test" >file &&
+	git add -A &&
+	git commit -am "tag test" &&
+	git tag "v1.0" &&
+	git tag "v1.1"
+'
+
+test_expect_success 'tag --contains <existent_tag>' '
+	git tag --contains "v1.0" >actual &&
+	grep "v1.0" actual &&
+	grep "v1.1" actual
+'
+
+test_expect_success 'tag --contains <inexistent_tag>' '
+	test_must_fail git tag --contains "notag" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag --no-contains <existent_tag>' '
+	git tag --no-contains "v1.1" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'tag --no-contains <inexistent_tag>' '
+	test_must_fail git tag --no-contains "notag" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag usage error' '
+	test_must_fail git tag --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_expect_success 'branch --contains <existent_commit>' '
+	git branch --contains "master" >actual &&
+	test_i18ngrep "master" actual
+'
+
+test_expect_success 'branch --contains <inexistent_commit>' '
+	test_must_fail git branch --no-contains "nocommit" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch --no-contains <existent_commit>' '
+	git branch --no-contains "master" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'branch --no-contains <inexistent_commit>' '
+	test_must_fail git branch --no-contains "nocommit" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch usage error' '
+	test_must_fail git branch --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_expect_success 'for-each-ref --contains <existent_object>' '
+	git for-each-ref --contains "master" >actual &&
+	test_line_count = 3 actual
+'
+
+test_expect_success 'for-each-ref --contains <inexistent_object>' '
+	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref --no-contains <existent_object>' '
+	git for-each-ref --no-contains "master" >actual &&
+	test_line_count = 0 actual
+'
+
+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
+	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref usage error' '
+	test_must_fail git for-each-ref --noopt 2>actual &&
+	test_i18ngrep "usage" actual
+'
+
+test_done

-- 
2.16.1.195.g8f471d4ad.dirty
Junio C Hamano· Feb 23, 2018, 20:58 UTC · re: Paul-Sebastian Ungureanu · lore

Re: [GSoC][PATCH v2] ref-filter: Make "--contains <id>" less chatty if <id> is invalid

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
Show 14 quoted lines
> Hello,
> I have made the changes after review. This is the updated patch
> based on what was discussed last time [1].
>
> In this patch, I have fixed the same issue that was also seen
> in "git branch" and "git for-reach-ref". I have also removed the
> dead code that was left and updated the patches accordingly.
>
> [1] https://public-inbox.org/git/20180219212130.4217-1-ungureanupaulsebastian@gmail.com/
>
> Best regards,
> Paul Ungureanu
>
> https://public-inbox.org/git/20180219212130.4217-1-ungureanupaulsebastian@gmail.com/

You do not want all of the above, upto and including the "---" below, to appear in the log message of the resulting commit. One way to tell the reading end that you have such preamble in your message is to write "-- >8 --" instead of "---" there.

Show 20 quoted lines
> ---
>
> Some git commands which use --contains <id> print the whole
> help text if <id> is invalid. It should only show the error
> message instead.
>
> This patch applies to "git tag", "git branch", "git for-each-ref".
>
> This bug was a side effect of looking up the commit in option
> parser callback. When a error occurs in the option parser, the
> full usage is shown. To fix this bug, the part related to
> looking up the commit was moved outside of the option parser
> to the ref-filter module.
>
> Basically, the option parser only parses strings that represent
> commits and the ref-filter performs the commit look-up. If an
> error occurs during the option parsing, then it must be an invalid
> argument and the user should be informed of usage, but if a error
> occurs during ref-filtering, then it is a problem with the
> argument.

The same problem appears for "git branch --points-at <commit>", doesn't it?

Show 9 quoted lines
> diff --git a/ref-filter.c b/ref-filter.c
> index f9e25aea7..aa282a27f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2000,6 +2000,25 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
>  	free(to_clear);
>  }
>  
> +int add_str_to_commit_list(struct string_list_item *item, void *commit_list)

If this function can be static to this file (and I suspect it is), please make it so.

Show 14 quoted lines
> +{
> +	struct object_id oid;
> +	struct commit *commit;
> +
> +	if (get_oid(item->string, &oid)) {
> +		error(_("malformed object name %s"), item->string);
> +		exit(1);
> +	}
> +	commit = lookup_commit_reference(&oid);
> +	if (!commit) {
> +		error(_("no such commit %s"), item->string);
> +		exit(1);
> +	}
> +	commit_list_insert(commit, commit_list);

The original (i.e. before this patch) does commit_list_insert() in the order the commits are given on the command line. This version collects the command line arguments with string_list_append() that preserves the order, and feeds them to commit_list_insert() here, so the resulting commit_list will have the commits in the same order before or after this patch.

Which is good.
> +	return 0;
> +}

The code after this patch is a strict improvement (the current code do not do so either), so this is outside the scope of this patch, but we may want to give this function another "const char *" that is used to report which option we got a malformed object name for.

Show 8 quoted lines
> @@ -2012,6 +2031,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>  	int ret = 0;
>  	unsigned int broken = 0;
>  
> +	/* Convert string representation and add to commit list. */
> +	for_each_string_list(&filter->with_commit_strs, add_str_to_commit_list, &filter->with_commit);
> +	for_each_string_list(&filter->no_commit_strs, add_str_to_commit_list, &filter->no_commit);
> +

As it does not use item->util in the callback helper, this should use for_each_string_list_item() instead; then you can do

	for_each_string_list_item(item, &filter_no_commit_strs)
		collect_commit(&filter->no_commit, item->string);

which allows the other helper take a simple string, instead of requiring a string_list_item.

Junio C Hamano· Feb 23, 2018, 23:44 UTC · re: Junio C Hamano · lore

Re: [GSoC][PATCH v2] ref-filter: Make "--contains <id>" less chatty if <id> is invalid

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
>
>> Basically, the option parser only parses strings that represent
>> commits and the ref-filter performs the commit look-up. If an
>> error occurs during the option parsing, then it must be an invalid
>> argument and the user should be informed of usage, but if a error
>> occurs during ref-filtering, then it is a problem with the
>> argument.

After staring the code a bit longer, I started to dislike the approach taken by this patch quite a lot. Isn't the problem that we let parse-options machinery to show the usage help, which is designed to be shown when the user does not know what options the command supports, even when we recognised the option perfectly fine?

That is, if we added a mechanism for a callback function given to OPT_CALLBACK to tell the calling parse-options machinery "I recognise the option; but the value given to the option is wrong and that is why I am returning an error", and made the caller in the parse-options machinery to refrain from showing the usage help, would it solve the issue with minimum fuss and stop the execution at the very first error we detect?

Stepping back even further, I wonder if any error detected in a custom callback handler given to OPT_CALLBACK even wants to show the usage help. I offhand do not think of any situation--- the callback was called only because OPT_CALLBACK item in the options[] list matched what the user gave us, so at that point we know the user gave us one of the valid options. The error message from the callback may say "Hey I only take commit object name", or it could (theoretically) even be "Sorry I do not take any values", but in any case, I do not think there is a reason for a failure detected in the callback should lead to the usage help.

So perhaps "if we added a machanism...to tell..." part in the previous paragraph is not even needed, and the only thing we need to do is to make the caller in parse-options that calls a custom callback function given to OPT_CALLBACK to stop giving the usage help. Wouldn't that automatically fix the "branch --points-at garbage" issue that is not addressed by this patch, too?

Paul-Sebastian Ungureanu· Feb 24, 2018, 14:12 UTC · re: Junio C Hamano · lore

Re: [GSoC][PATCH v2] ref-filter: Make "--contains <id>" less chatty if <id> is invalid

Hello,

Your proposed solution makes a lot more sense. I have actually considered a solution similar to this (the third solution from [1]), but found it more complicated. I did not account for the fact that once a callback is called, the user is already aware of the available options and the user only supplied an invalid argument value.

I also have to make sure that all parsers (all callbacks and standard ones, for integer, filename, etc.) are already printing errors appropriately. Otherwise, some commands may fail and the user will not be aware of it because nothing will be shown (no usage is shown and no errors either).

I will be implementing this solution and come back with another patch.
Thank you for your review. I really appreciate it!

[1] https://public-inbox.org/git/20160118215433.GB24136@sigill.intra.pe ff.net/

Best regards, Paul Ungureanu

← back to recent threads