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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2018, 20:58 UTC
Message-ID: <xmqqwoz3s9s2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180223162557.31477-1-ungureanupaulsebastian@gmail.com>

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.