Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> resolve_ref() is taught to return a xstrdup'd buffer if alloc is true.
> All callers are updated to receive static buffer as before though.
Thanks for working on this. I have a slight fear that this may collide
with some topics in flight, but that is something we can worry about after
1.7.8 release is done.
I like the direction of the entire series, and agree with the reasoning
behind the choices of callsites to be converted in later part of the
series (they are clearly documented).
After this series, all the callsites use either 0 or 1 and never a
computed value for the extra argument. I think it would make it a lot
safer and easier for other people who later want to touch the codepath
that has resolve_ref(..., 0) calls in it, if we instead did this in the
following way:
(1) Rename resolve_ref() to resolve_ref_unsafe() everywhere to make it
stand out in the code, without changing anything else. This is the
moral equivalent of a half or your [01/10].
(2) Introduce resolve_ref() that returns a copy. This is equivalent to
the other half of your [01/10].
(3) Convert the ones that should use resolve_ref() to do so. This is
equivalent to the remainder of your series.
(4) Convert callsites of resolve_ref_unsafe() that xstrdup()'s the
returned value back to use resolve_ref() as needed.
(5) And finally document in in-code comments at each tricky callsites of
resolve_ref_unsafe() why the use of the unsafe variant is OK in that
context. This can be done as part of step 1. We obviously do not have
to document trivial ones (e.g. a variable in a narrow sub-function
scope receives the return value of resolve_ref_unsafe(), and the
value is immediately used without giving it a longer lifetime).
The above is primarily a naming issue, but names are important. It forces
developers to _think_ before using something named _unsafe_, and helps
them to be on the lookout when they insert new code between an existing
call to the unsafe one and use of its result.
There are other "unsafe" functions like git_path() whose callsites can be
updated with the same strategy, and we probably should do that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html