Re: [GSoC][PATCH v2] ref-filter: Make "--contains <id>" less chatty if <id> is invalid
From: Junio C Hamano <hidden>
Date: 2018-02-23 20:58:45
Paul-Sebastian Ungureanu [off-list ref] 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@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.
--- 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?
quoted hunk
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.
quoted hunk
@@ -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.