Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c

[Alex Riesen <raa.lkml@gmail.com>]

Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
> >
> > Otherwise the function sometimes fail to resolve obviously correct refnames,
> > because the string data pointed to by "ref" argument were reused.
> >
> But your patch instead rewrites the computation of str2 by bypassing the
> call to "another_function_that_uses_get_pathname()" and duplicating its
> logic, which I do not think is a viable approach in the longer term.

There is not enough logic to bypass there. It's just a dumb sprintf!
I actually saved a useless call (two, if automatic inlining
optimization doesn't work).

> > diff --git a/sha1_name.c b/sha1_name.c
> > index 41b6809..b5b53bf 100644
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -242,6 +242,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
> >  {
> >  	const char **p, *r;
> >  	int refs_found = 0;
> > +	char fullref[PATH_MAX];
> >  
> >  	*ref = NULL;
> >  	for (p = ref_rev_parse_rules; *p; p++) {
> > @@ -249,7 +250,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
> >  		unsigned char *this_result;
> >  
> >  		this_result = refs_found ? sha1_from_ref : sha1;
> > -		r = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);
> > +		snprintf(fullref, sizeof(fullref), *p, len, str);
> > +		r = resolve_ref(fullref, this_result, 1, NULL);
> >  		if (r) {
> >  			if (!refs_found++)
> >  				*ref = xstrdup(r);
> 
> I suspect that I am grossly misleading the code, but I wonder why this xstrdup()
> is not protecting us from the reusing of "the string data pointed to by
> "ref" argument".  Are you fixing the overwriting of the string pointed to
> by "str" argument instead?

Yes, I mistyped. xstrdup's called after the breakage happened.

> What specific call chain has this breakage you found?

That's up to this function from builtin_rev_parse and resolve_ref
we're about to call.