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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2017, 21:16 UTC
Message-ID: <xmqq4lz331wb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170308183840.GA130604@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 22 quoted lines

>> > 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.

It may be somewhat unfortunate that that somebody needs to be me, but that is what maintainers are for, so ... ;-)

Once the dust settles, let's do the clean-up along the lines of Rene's patch.

Thanks.