Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:
> "Alex Riesen" <raa.lkml@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
