Paul-Sebastian Ungureanu <[email protected]> writes:
> 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/[email protected]/
>
> Best regards,
> Paul Ungureanu
>
> https://public-inbox.org/git/[email protected]/
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.
> ---
>
> 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?
> 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.
> +{
> + 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.
> @@ -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.
