Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

Hi,

On Wed, 8 Mar 2017, Junio C Hamano wrote:

> Brandon Williams <bmwill@google.com> 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