On Fri, Jan 3, 2025 at 12:46 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Fri, Jan 03, 2025 at 11:28:47AM -0800, Wink Saville wrote:
>
> > `git diff-tree --cc` SEGFAUTs after adding trace_printf to diff_tree_combined.
>
> Hmm, is it really a bug in Git if you had to add new code which contains
> the bug? :)
>
> > @@ -1595,8 +1597,16 @@ void diff_tree_combined(const struct object_id *oid,
> > }
> >
> > /* find out number of surviving paths */
> > - for (num_paths = 0, p = paths; p; p = p->next)
> > + trace_printf("Wink diff_tree_combined: find number of surviving paths num_parent=%d\n", num_parent);
> > + for (num_paths = 0, p = paths; p; p = p->next) {
> > + trace_printf("Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);
> > + for (i = 0; i < num_parent; i++) {
> > + trace_printf("Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\n",
> > + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);
> > + }
>
> The parent "path" strbufs are only initialized in intersect_paths() if
> combined_all_paths is set, and if there was an actual path change (a
> copy or rename).
>
> So you'd probably need something like this:
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 455bc19087..1e58809c4e 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1601,8 +1601,11 @@ void diff_tree_combined(const struct object_id *oid,
> for (num_paths = 0, p = paths; p; p = p->next) {
> trace_printf("Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);
> for (i = 0; i < num_parent; i++) {
> + const char *path = rev->combine_all_paths &&
> + filename_changed(p->parent[i].status) ?
> + p->parent[i].path.buf : NULL;
> trace_printf("Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\n",
> - i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);
> + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), path, path);
> }
> num_paths++;
> }
>
> -Peff
TYVM!
That worked but changed the name and fixed a typo in `combined_all_paths`:
```
wink@3900x 25-01-03T23:06:08.344Z:~/data/prgs/forks/git
(wink-segfault-with-minimal-changes)
$ git diff
diff --git a/combine-diff.c b/combine-diff.c
index 455bc19087..70394c3350 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1601,8 +1601,9 @@ void diff_tree_combined(const struct object_id *oid,
for (num_paths = 0, p = paths; p; p = p->next) {
trace_printf("Wink diff_tree_combined: num_paths=%d
&p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode,
oid_to_hex(&p->oid), p->path);
for (i = 0; i < num_parent; i++) {
+ const char *parent_path =
rev->combined_all_paths && filename_changed(p->parent[i].status) ?
p->parent[i].path.buf : NULL;
trace_printf("Wink diff_tree_combined:
&p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents
path.buf=%s\n",
- i, &p->parent[i],
p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),
p->parent[i].path.buf, p->parent[i].path.buf);
+ i, &p->parent[i],
p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),
parent_path, parent_path);
}
num_paths++;
}
```
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.
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?
-- wink
