On Sat, Apr 04, 2020 at 08:07:43AM -0400, Denton Liu wrote:
> > for_each_string_list_item(...) {
> > ret = for_each_fullref_in(...);
>
> This loop is missing a bit of important context:
>
> if (ret)
> break;
>
Yes, I omitted it because it's not relevant to whether ret is
initialized or not (i.e., if we enter the loop then it always is.
But I think it is to the argument you make below.
> > Your patch silences it, but is it doing the right thing? It sets "ret =
> > 0", but we haven't actually iterated anything. Should it be an error
> > instead?
>
> I understood the semantics of for_each_fullref_in_pattern() (in the
> non-early return case) to be "for each item, iterate and compute a
> value; if that value is non-zero return it. If not found, return zero".
> The missing context above is important since, without it, we lose the
> semantics.
>
> If I'm understanding the above correctly then, studying this function in
> a vacuum, it would make sense to assign a default value of 0 since if
> the for operates on an empty list, the function should consider the item
> vacuously not found and we should return 0 as a result.
I think the break on "ret" is better understood as "if we saw an error,
return it; otherwise keep going".
If we were given zero patterns, is that a noop success, or is it an
error? This should never happen because we cover that case earlier, so
it would be a bug in find_longest_prefixes. Perhaps:
diff --git a/ref-filter.c b/ref-filter.c
index b1812cb69a..b358567663 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1964,6 +1964,8 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
}
find_longest_prefixes(&prefixes, filter->name_patterns);
+ if (!prefixes.nr)
+ BUG("no common ref prefix?");
for_each_string_list_item(prefix, &prefixes) {
ret = for_each_fullref_in(prefix->string, cb, cb_data, broken);
is worth doing to document that assumption. Ideally that would then let
the compiler know that we'll always enter the loop, but it doesn't seem
to be quite the clever.
So I dunno. I think it probably doesn't matter between "0" and "-1",
because this case really shouldn't be reachable.
> This was the type of analysis I applied to the other changes. I'll admit
> that I studied the other functions in a vacuum as well since these
> seemed to be superficial warnings (since they aren't triggered with
> -O{0,2}) which indicated to me that the code was correct except for some
> "cosmetic" errors. I dunno, perhaps this is my inexperience showing
> through.
Yeah, the code is correct in this case, and I don't think the
uninitialized case can be reached. But the subtle linkage here is that
patterns[0] being non-NULL means that find_longest_prefixes() will never
return a zero-length list, which means we'll always enter that loop at
least once.
> P.S. Do we want to quash the -O3 warnings as well?
They're probably worth looking at. I've periodically swept through and
fixed them, as recently as last September (try "git log --grep=-O3").
But new ones seem to have cropped up. I'm not sure if they were
introduced in the code or if the compiler got smarter (or dumber).
Just skimming the output, I see:
In function ‘ll_binary_merge’,
inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10,
inlined from ‘ll_union_merge’ at ll-merge.c:151:9:
ll-merge.c:74:4: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
74 | warning("Cannot merge binary files: %s (%s vs. %s)",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
75 | path, name1, name2);
| ~~~~~~~~~~~~~~~~~~~
This one seems likely to be accurate (and just uncovered by more
aggressive inlining). The union_merge function passes in NULL filenames,
and probably could trigger this warning on binary input.
trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.0’:
trace2/tr2_dst.c:296:10: error: ‘fd’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
296 | dst->fd = fd;
| ~~~~~~~~^~~~
trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here
229 | int fd;
| ^~
This one looks like a false positive. The tr2 code does something
unusual for our code base: on error it returns errno. And I think the
compiler is not aware that errno would not be zero after a syscall
failure. It might be worth changing the return value to match our usual
pattern (return -1, and the caller can look in errno), which would
appease the compiler as a side effect.
revision.c: In function ‘do_add_index_objects_to_pending’:
revision.c:322:22: error: array subscript [1, 2147483647] is outside array bounds of ‘char[1]’ [-Werror=array-bounds]
322 | if (0 < len && name[len] && buf.len)
| ~~~~^~~~~
This one is confusing. I imagine the char[1] is the empty string we
frequently pass to add_pending_object() and friends. And the "len" here
comes from interpret_branch_name(name, ...). So somehow the compiler
doesn't quite know that the length we'll return is going to be less than
or equal to the string length we pass in.
I don't know how to fix that aside from this, which isn't great (btw, it
looks like a lot of this code could stand to switch to using ssize_t
instead of int; there are probably weird errors if you managed to
somehow feed a 2GB branch name).
diff --git a/revision.c b/revision.c
index 8136929e23..9bc398bec1 100644
--- a/revision.c
+++ b/revision.c
@@ -317,9 +317,10 @@ static void add_pending_object_with_path(struct rev_info *revs,
revs->no_walk = 0;
if (revs->reflog_info && obj->type == OBJ_COMMIT) {
struct strbuf buf = STRBUF_INIT;
- int len = interpret_branch_name(name, 0, &buf, 0);
+ size_t namelen = strlen(name);
+ int len = interpret_branch_name(name, namelen, &buf, 0);
- if (0 < len && name[len] && buf.len)
+ if (len == namelen && buf.len)
strbuf_addstr(&buf, name + len);
add_reflog_for_walk(revs->reflog_info,
(struct commit *)obj,
-Peff
