From: Paolo Bonzini <paolo.bonzini@lu.unisi.ch>
Date: Mon, 05 Mar 2007 17:22:14 GMT
Subject: Re: [PATCH 1/3] git-branch: add --track and --no-track options
Message-ID: <45EC51C6.5080505@lu.unisi.ch>
In-Reply-To: <Pine.LNX.4.63.0703051812030.22628@wbgn013.biozentrum.uni-wuerzburg.de>


> Yes, you also check real_ref instead of checking if dwim_ref() returned 0. 
> I feel a little bit uneasy about that, since there is no guarantee that 
> these values are left untouched, whereas the return value is guaranteed to 
> behave as expected.

There is.  The man page says "Scanning stops when an input character does not match such a format character."  Scanning includes not processing %n elements, either.

Regarding dwim_ref, dwim_ref says:

int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
{
        const char **p, *r;
        int refs_found = 0;

        *ref = NULL;

and since this "*ref" is not used anyway in the rest of the routine, I figured out that it's part of the interface to set "*ref" to NULL if no ref is found.  Of course it could help (hint hint) if extern functions had a comment stating the interface.

I see however that I also have a "real_ref = NULL" that is actually pointless; I can take it away of course.

> I also feel a little uneasy about having to parse a format in order to 
> parse a string, when you know what that string should look like. For 
> example, you could make the code even more compact by asking 
> "(p = strstr(value, "/*:refs/remotes/"))".

Go down this way and you will say that printf is useless too.

Paolo