On Fri, Jan 3, 2025 at 7:32 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Fri, Jan 03, 2025 at 06:55:18PM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@xxxxxxxx> writes:
> >
> > > ... 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.
> >
> > Nicely said. I reached the same conclusion after looking at the
> > existing code, even though I have to admit that I am not a huge fan
> > of the more recent part of combine-diff.c and its data structures.
>
> I poked at this a little bit more, so here are a few tidbits:
>
> - the patch I showed earlier is not sufficient! There are lots of
> other spots that create combine_diff_path structs but don't bother
> to put anything in the parent paths at all. It works now because
> they also don't set a status that triggers filename_changed(). But
> what I showed earlier was wrong, because it was assuming in the
> cleanup functions that the strbufs were always initialized.
>
> - there's really no need for a strbuf at all here. It is always
> uninitialized/empty, or contains a direct copy of a path string. So
> a raw pointer with xstrdup() is plenty. And then we can use NULL to
> mean "it was not set".
>
> Which would Just Work for all those other spots if they bothered to
> zero the memory they allocated, but they don't. So we have to update
> them to set it to NULL anyway. That patch is below.
>
> - it is not at all clear to me that we need to be allocating at all.
> We always copy a string from the diff_queue. Do our
> combine_diff_path structs persist beyond then? I'm not sure. It is
> probably asking for trouble to just point to them directly without
> copying, as it creates a dependency (that even if it is not needed
> now, is a trap for somebody later). But it would drop some
> allocation/cleanup code, and we could just have p->parent[i].path
> fall back to p->path naturally.
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 641bc92dbd..0d9d344c4e 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -66,13 +66,9 @@ static struct combine_diff_path *intersect_paths(
> oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);
> p->parent[n].mode = q->queue[i]->one->mode;
> p->parent[n].status = q->queue[i]->status;
> -
> - 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);
> - }
> + p->parent[n].path = combined_all_paths &&
> + filename_changed(p->parent[n].status) ?
> + xstrdup(q->queue[i]->one->path) : NULL;
> *tail = p;
> tail = &p->next;
> }
> @@ -92,9 +88,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);
> + free(p->parent[j].path);
> free(p);
> continue;
> }
> @@ -108,10 +102,9 @@ static struct combine_diff_path *intersect_paths(
> oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);
> p->parent[n].mode = q->queue[i]->one->mode;
> p->parent[n].status = q->queue[i]->status;
> - if (combined_all_paths &&
> - filename_changed(p->parent[n].status))
> - strbuf_addstr(&p->parent[n].path,
> - q->queue[i]->one->path);
> + p->parent[n].path = combined_all_paths &&
> + filename_changed(p->parent[n].status) ?
> + xstrdup(q->queue[i]->one->path) : NULL;
>
> tail = &p->next;
> i++;
> @@ -996,8 +989,9 @@ static void show_combined_header(struct combine_diff_path *elem,
>
> if (rev->combined_all_paths) {
> for (i = 0; i < num_parent; i++) {
> - char *path = filename_changed(elem->parent[i].status)
> - ? elem->parent[i].path.buf : elem->path;
> + const char *path = elem->parent[i].path ?
> + elem->parent[i].path :
> + elem->path;
> if (elem->parent[i].status == DIFF_STATUS_ADDED)
> dump_quoted_path("--- ", "", "/dev/null",
> line_prefix, c_meta, c_reset);
> @@ -1278,12 +1272,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
>
> for (i = 0; i < num_parent; i++)
> if (rev->combined_all_paths) {
> - if (filename_changed(p->parent[i].status))
> - write_name_quoted(p->parent[i].path.buf, stdout,
> - inter_name_termination);
> - else
> - write_name_quoted(p->path, stdout,
> - inter_name_termination);
> + const char *path = p->parent[i].path ?
> + p->parent[i].path :
> + p->path;
> + write_name_quoted(path, stdout, inter_name_termination);
> }
> write_name_quoted(p->path, stdout, line_termination);
> }
> @@ -1645,9 +1637,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);
> + free(tmp->parent[i].path);
> free(tmp);
> }
>
> diff --git a/diff-lib.c b/diff-lib.c
> index c6d3bc4d37..88a5aed736 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -417,9 +417,11 @@ static int show_modified(struct rev_info *revs,
> memset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));
> p->parent[0].status = DIFF_STATUS_MODIFIED;
> p->parent[0].mode = new_entry->ce_mode;
> + p->parent[0].path = NULL;
> oidcpy(&p->parent[0].oid, &new_entry->oid);
> p->parent[1].status = DIFF_STATUS_MODIFIED;
> p->parent[1].mode = old_entry->ce_mode;
> + p->parent[1].path = NULL;
> oidcpy(&p->parent[1].oid, &old_entry->oid);
> show_combined_diff(p, 2, revs);
> free(p);
> diff --git a/diff.h b/diff.h
> index 6e6007c17b..3157faeabb 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -480,7 +480,7 @@ struct combine_diff_path {
> char status;
> unsigned int mode;
> struct object_id oid;
> - struct strbuf path;
> + char *path;
> } parent[FLEX_ARRAY];
> };
> #define combine_diff_path_size(n, l) \
> diff --git a/tree-diff.c b/tree-diff.c
> index d9237ffd9b..57af377c2b 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -272,6 +272,7 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,
> }
>
> p->parent[i].mode = mode_i;
> + p->parent[i].path = NULL;
> oidcpy(&p->parent[i].oid, oid_i);
> }
The above LGTM and hopefully it can be accepted.
With that change I can revert my trace_printfs of combine_diff_path
back to something simple:
```
$ git diff HEAD^
diff --git a/combine-diff.c b/combine-diff.c
index 5e0b7919bc..4764383f20 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1593,8 +1593,8 @@ 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++) {
- 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);
+ trace_printf("Wink diff_tree_combined:
&p->parent[%d]=%p status=%c mode=%x oid=%s path=%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);
}
num_paths++;
}
```
And the output doesn't SEGFAULT :)
```
$ GIT_TRACE=1 ./git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a
10:06:11.716284 git.c:476 trace: built-in: git diff-tree
--cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a
6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a
10:06:11.718102 combine-diff.c:1592 Wink diff_tree_combined: find
number of surviving paths num_parent=2
10:06:11.718108 combine-diff.c:1594 Wink diff_tree_combined:
num_paths=0 &p=0x643ac70f7ef0 mode=81a4,
oid=0f41b2fd4a6b679a1cfcaa9a584c382068146212 path=refs.c
10:06:11.718112 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[0]=0x643ac70f7f28 status=M mode=81a4
oid=7dd5e9fa3323111f06303674b213ae24ed2d04b6 path=(null)
10:06:11.718116 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[1]=0x643ac70f7f60 status=M mode=81a4
oid=c55583986940d8ef1e1c839364c03cd92d4f7114 path=(null)
10:06:11.718120 combine-diff.c:1594 Wink diff_tree_combined:
num_paths=1 &p=0x643ac70f7fb0 mode=81a4,
oid=a0cdd99250e8286b55808b697b0a94afac5d8319 path=refs.h
10:06:11.718123 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[0]=0x643ac70f7fe8 status=M mode=81a4
oid=09be47afbee51e99f4ae49588cd65596ccfcb07e path=(null)
10:06:11.718126 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[1]=0x643ac70f8020 status=M mode=81a4
oid=b0dfc65ed2e59c4b66967840339f81e7746a96d3 path=(null)
10:06:11.718129 combine-diff.c:1594 Wink diff_tree_combined:
num_paths=2 &p=0x643ac70f8900 mode=81a4,
oid=5cfb8b7ca8678e171b8e8a7ad6daf1af74a81b59 path=refs/files-backend.c
10:06:11.718132 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[0]=0x643ac70f8938 status=M mode=81a4
oid=467fe347fa7e7d82ed7a2836e43ea749bb90ad7d path=(null)
10:06:11.718135 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[1]=0x643ac70f8970 status=M mode=81a4
oid=8953d1c6d37b13b0db701888b3db92fd87a68aaa path=(null)
10:06:11.718138 combine-diff.c:1594 Wink diff_tree_combined:
num_paths=3 &p=0x643ac70f89d0 mode=81a4,
oid=16550862d3ebe3b357c52254088b143c7ba000d6 path=refs/refs-internal.h
10:06:11.718142 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[0]=0x643ac70f8a08 status=M mode=81a4
oid=66e66e0fc1e812ebebd1d4b0119899c84bf1c0ae path=(null)
10:06:11.718162 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[1]=0x643ac70f8a40 status=M mode=81a4
oid=79b287c5ec5c7d8f759869cf93cda405640186dc path=(null)
10:06:11.718181 combine-diff.c:1594 Wink diff_tree_combined:
num_paths=4 &p=0x643ac70f8aa0 mode=81a4,
oid=00d95a9a2f42ce74c5cb4a42175b0953287851a6
path=refs/reftable-backend.c
10:06:11.718184 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[0]=0x643ac70f8ad8 status=M mode=81a4
oid=8a2a5b847c3d86332e319da69bfb5c8a56a10e86 path=(null)
10:06:11.718188 combine-diff.c:1596 Wink diff_tree_combined:
&p->parent[1]=0x643ac70f8b10 status=M mode=81a4
oid=bec5962debea7b62572d08f6fa8fd38ab4cd8af6 path=(null)
10:06:11.718192 combine-diff.c:1601 Wink diff_tree_combined: found
5 surviving paths
diff --cc refs/files-backend.c
index 467fe347fa,8953d1c6d3..5cfb8b7ca8
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@@ -2533,9 -2539,15 +2543,15 @@@ static int check_old_oid(struct ref_upd
oid_to_hex(oid),
oid_to_hex(&update->old_oid));
- return -1;
+ return ret;
}
+ struct files_transaction_backend_data {
+ struct ref_transaction *packed_transaction;
+ int packed_refs_locked;
+ struct strmap ref_locks;
+ };
+
/*
* Prepare for carrying out update:
* - Lock the reference referred to by update.
```
