Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
- Date: Thu, 9 Mar 2017 12:24:11 +0100 (CET)
- From: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
- Subject: Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
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.