On Mon, Sep 20, 2021 at 02:39:12PM -0700, Carlo Marcelo Arenas Belón wrote:
> > Therefore (if I'm reading this correctly), it is absolutely correct
> > for add_rev_cmdline() to be duplicating that string to ensure that the
> > hexified OID value remains valid, and incorrect for this patch to be
> > removing the call to xstrdup().
>
> Indeed, but the values that are being strdup were never used anyway, so
> I suspect the original code might had just put it as a logical default.
We do look at them in a few cases, like "fast-export", but only if they
are not marked UNINTERESTING. And add_rev_cmdline_list(), the variant
that writes the hex values, only ever gets called with the UNINTERESTING
flag.
So I think you're right that these ones would never be seen. I did
wonder if we'd ever show them with "log --source" or similar, but that
pulls the name from object_array_entry, I think.
> -------- >8 --------
> Subject: [PATCH] revision: remove xstrdup() of name in add_rev_cmdline()
>
> a765499a08 (revision.c: treat A...B merge bases as if manually
> specified, 2013-05-13) adds calls to this function in a loop,
> abusing oid_to_hex (at that time called sha1_to_hex).
>
> df835d3a0c (add_rev_cmdline(): make a copy of the name argument,
> 2013-05-25) adds the strdup, introducing a leak.
>
> All names we will ever get should come from the commandline or be
> constant values, so it is safe not to xstrdup and clean them up.
This last paragraph is questionable, I think. We feed the argv from
setup_revisions() here, but that is not always coming from the actual
command line. Most cases seem to finish with the traversal before what
they've passed in goes out of scope, but not all. The call in
bisect_rev_setup() intentionally leaks the strvec (even though it
doesn't need to do so with the current code). The one in
cmd__fast_rebase() does clear its strvec after setup_revisions() but
before the actual traversal. I wouldn't be surprised if your patch
triggered memory problems there.
> @@ -1504,10 +1499,10 @@ static void add_rev_cmdline_list(struct rev_info *revs,
> int whence,
> unsigned flags)
> {
> + static const char *synthetic = ".synthetic";
I don't think there's any point in making this static. It's not an
array, but rather a pointer to a string literal. That string literal
will remain valid regardless (the standard does not guarantee we get the
_same_ string literal every time, but that doesn't matter for our
purposes. And in practice it will be the same one).
> @@ -1753,7 +1748,7 @@ struct add_alternate_refs_data {
> static void add_one_alternate_ref(const struct object_id *oid,
> void *vdata)
> {
> - const char *name = ".alternate";
> + static const char *name = ".alternate";
> struct add_alternate_refs_data *data = vdata;
> struct object *obj;
Ditto here.
> @@ -1940,7 +1935,7 @@ static int handle_dotdot_1(const char *arg, char *dotdot,
> struct object_context *a_oc,
> struct object_context *b_oc)
> {
> - const char *a_name, *b_name;
> + static const char *a_name, *b_name;
> struct object_id a_oid, b_oid;
> struct object *a_obj, *b_obj;
> unsigned int a_flags, b_flags;
And I don't see how this changes anything at all. Our pointers will live
on, but the memory they point to is not affected.
-Peff
