Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx> 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/20180219212130.4217-1-ungureanupaulsebastian@xxxxxxxxx/ > > Best regards, > Paul Ungureanu > > https://public-inbox.org/git/20180219212130.4217-1-ungureanupaulsebastian@xxxxxxxxx/ 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.
