On Fri, Jan 03, 2025 at 03:34:58PM -0800, Wink Saville wrote:
> But having to protect yourself is unobvious and especially if it isn't necessary
> when using the `fetch_paths_generic`.
>
> In addition, from strbuf.h `buf` is never NULL:
>
> "
> * strbufs have some invariants that are very important to keep in mind:
> *
> * - The `buf` member is never NULL, so it can be used in any usual C
> * string operations safely. strbufs _have_ to be initialized either by
> * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
> *
> "
>
> So I'd say this could be considered a bug in git at least in how
> combine_diff_path
> is being managed. I assume you agree that neither find_paths_generic or
> find_paths_multitree are adhering to at least that strbuf invariant and I wonder
> if the other strbuf invariants are being upheld.
The strbuf invariant can only be held on strbufs which have been
initialized, and this one has not. I don't think it's wrong to have
variables which not (yet) been initialized. It can make for a fragile
interface, though, if uninitialized struct members are exposed widely.
I'm not sure how wide this case is. It's mostly an internal combine-diff
data structure, though it looks like it gets exposed to other code in a
few spots (though nobody outside of combine-diff.c currently looks at
the parent paths at all).
So I wouldn't call it a bug, as the internals of Git are not part of the
public interface and there is no user-visible behavior problem without
patching. But I doubt anybody would object to a patch making the API
less fragile if it can be done cheaply and easily. And strbufs are
designed to be cheap to initialize. So something like (completely
untested):
diff --git a/combine-diff.c b/combine-diff.c
index 641bc92dbd..452b5f5beb 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -67,9 +67,9 @@ static struct combine_diff_path *intersect_paths(
p->parent[n].mode = q->queue[i]->one->mode;
p->parent[n].status = q->queue[i]->status;
+ strbuf_init(&p->parent[n].path, 0);
if (combined_all_paths &&
filename_changed(p->parent[n].status)) {
- strbuf_init(&p->parent[n].path, 0);
strbuf_addstr(&p->parent[n].path,
q->queue[i]->one->path);
}
@@ -92,9 +92,7 @@ static struct combine_diff_path *intersect_paths(
/* p->path not in q->queue[]; drop it */
*tail = p->next;
for (j = 0; j < num_parent; j++)
- if (combined_all_paths &&
- filename_changed(p->parent[j].status))
- strbuf_release(&p->parent[j].path);
+ strbuf_release(&p->parent[j].path);
free(p);
continue;
}
@@ -1645,9 +1643,7 @@ void diff_tree_combined(const struct object_id *oid,
struct combine_diff_path *tmp = paths;
paths = paths->next;
for (i = 0; i < num_parent; i++)
- if (rev->combined_all_paths &&
- filename_changed(tmp->parent[i].status))
- strbuf_release(&tmp->parent[i].path);
+ strbuf_release(&tmp->parent[i].path);
free(tmp);
}
might help the uninitialized-pointer issue. OTOH it is not really
solving the more fundamental problem, which is that p->parent[i].path is
only sometimes useful (we do not fill it in if it would just be the same
as p->path, so the patch only changes it from uninitialized memory into
an empty strbuf).
And that is probably not something we want to change, as allocating
duplicates of each path may be expensive. Probably we'd be better to
encapsulate it in a function which falls back to p->path automatically.
But then, AFAICT there are only two sites (both inside combine-diff.c)
which look at it, so it would mostly be hypothetical future-proofing. I
dunno.
> So, should this bug be "closed" and a new one "created"?
>
> Actually, using the mailing list to identify bugs and initially discuss
> them, seems fine. But is there a place where there is a list of current bugs and
> their state?
No, there's no bug tracker for the project[1]. Discussion may lead to a
patch or not, which may be applied or not, but there is no formal
classification of "open" or "fixed" or "won't fix".
-Peff
[1] Some folks seem to be using:
https://git.issues.gerritcodereview.com/issues?q=status:open
but I don't know how active it is. I never use it and had to dig out
the link to https://crbug.com, which now redirects there, from a
message from 2017. There's some recent activity, but I wouldn't
count on opening something there to get wide attention.
