Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Aug 11, 2011, 09:50 UTC
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:

Show 9 quoted lines

>>       } 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.

Show 6 quoted lines

>>       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.

Show 5 quoted lines

>>       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