Hi, On Wed, 8 Mar 2017, Junio C Hamano wrote: > Brandon Williams <bmwill@xxxxxxxxxx> writes: > > >> > diff --git a/abspath.c b/abspath.c > >> > index 2f0c26e0e2c..b02e068aa34 100644 > >> > --- a/abspath.c > >> > +++ b/abspath.c > >> > @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path) > >> > return strbuf_realpath(&realpath, path, 0); > >> > } > >> > > >> > -char *real_pathdup(const char *path) > >> > +char *real_pathdup(const char *path, int die_on_error) > >> > >> Adding a gentle variant (with the current implementation) and making > >> real_pathdup() die on error would be nicer, as it doesn't require > >> callers to pass magic flag values. Most cases use the dying variant, > >> so such a patch would have to touch less places: > > > > I agree with Junio and Rene that a gentle version would make the api > > slightly nicer (and more consistant with some of the other api's we > > have in git). > > > > This is exactly what I should have done back when I originally made > > the change. Sorry for missing this! > > While I agree that the shape of the code Rene gave us here is what we > would have liked to have in the original, it is a bit too late for that. > > As I already mentioned, as a regression fix patch, I find what Dscho > posted more sensible, because it makes it obvious that all existing > callsites were looked at while constructing the patch and more > importantly, it forces somebody to look at all the new callers of the > function that were added by the topics in flight, by changing the > func-signature and forcing compilation failure. While I would have agreed earlier that René's patch looks less intrusive, I have to point out that there would not have been any possible regression if the original patch had introduced the die_on_error parameter. It would have made the contract *obvious*. The nicer API made the contract unobvious, and that was the reason that the bug could hide. Ciao, Johannes