From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Thu, 11 Aug 2011 09:50:57 GMT
Subject: Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions
Message-ID: <CALkWK0mMVgnngU5JB4O+7crCjGCdRnVrbm4jjsgkFFUMeBS9_A@mail.gmail.com>
In-Reply-To: <20110810151527.GC31315@elie.gateway.2wire.net>

Hi Jonathan,

I fixed everything; just a few comments/ doubts:

Jonathan Nieder writes:
>>       } else
>> -             return NULL;
>> +             return -1;
>
> Unrelated to this patch: maybe
>
>        return error("unrecognized action in sequencer file: %s", start);
>
> to be easier to debug.

I'd initially refrained from doing this so that errors don't become
overtly verbose, but I suppose it's alright considering the fact that
we're going to make the instruction sheet editable sometime in the
future.  I tweaked the error strings a little so that we get something
like:

  error: Unrecognized action: part
  error: Could not parse line 3.
  fatal: Unusable instruction sheet: .git/sequencer

In essence, I didn't want to be redundant and mention the sequencer in
every line.  I like the above.

>>       q = strchr(p, ' ');
>>       if (!q)
>> -             return NULL;
>> +             return -1;
>
> So we reject "pick a87c8989"?  That's a shame.

Good point.  Fixing this will have to be in another patch, where I'll
advertise the fact that I'm changing the instruction sheet format.

>>       q++;
>>
>>       strlcpy(sha1_abbrev, p, q - p);
>
> memcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?

Good point.  I'm tempted to check (q - p < 40); is there a better way
to do this by not hardcoding "40" perhaps?

> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- Ram