Re: [PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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.