git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Mar 9, 2017, 11:24 UTC
Message-ID
<alpine.DEB.2.20.1703091221440.3767@virtualbox>
In-Reply-To
<xmqq4lz331wb.fsf@gitster.mtv.corp.google.com>
Hi,
On Wed, 8 Mar 2017, Junio C Hamano wrote:
Show 34 quoted lines
> 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

Previous: Junio C HamanoNext: René Scharfe
Message 6 of 10 in “Fix crashes due to real_pathdup() potentially returning NULL”
  1. 0/2 Fix crashes due to real_pathdup() potentially returning NULLJohannes Schindelin, Mar 8, 2017
  2. 2/2 Fix callsites of real_pathdup() that wanted it to die on errorJohannes Schindelin, Mar 8, 2017
  3. René ScharfeMar 8, 2017
  4. Brandon WilliamsMar 8, 2017
  5. Junio C HamanoMar 8, 2017
  6. Johannes SchindelinMar 9, 2017
  7. René ScharfeMar 9, 2017
  8. 1/2 Demonstrate NULL pointer access with invalid GIT_WORK_TREEJohannes Schindelin, Mar 8, 2017
  9. Jeff KingMar 8, 2017
  10. Johannes SchindelinMar 9, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.