From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Fri, 18 Feb 2011 08:02:23 GMT
Subject: Re: [PATCHv2 2/2] git-patch-id: do not trip over "no newline" markers
Message-ID: <4D5E278F.2010509@drmicha.warpmail.net>
In-Reply-To: <20110218041609.GA12348@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 18.02.2011 05:16:
> On Thu, Feb 17, 2011 at 08:44:42AM +0100, Michael J Gruber wrote:
> 
>> --- a/builtin/patch-id.c
>> +++ b/builtin/patch-id.c
>> @@ -73,6 +73,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
>>  			p += 7;
>>  		else if (!memcmp(line, "From ", 5))
>>  			p += 5;
>> +		else if (!memcmp(line, "\\ ", 2) && strlen(line)>=12)
>> +			continue;
> 
> Wow, that's pretty obscure. I wonder if the test should be factored out
> into line_is_no_newline_at_end_of_file() (or surely there is some more
> sensible name), and used by both apply and patch-id. Along with a nice
> comment (which I see apply already has) describing what in the world the
> magic number 12 means.

So, where do you suggest it should go? As far as I can see, we have two
places reading these markers (the above builtins) and two places writing
them (diff.c, xdiff/xutils.c), at least for git-core. (git-gui's
diff.tcl has its own strict check etc.)

I would opt for putting a more detailed explanation in the commit
message (reasoning + ref. to apply.c) rather then trying to factor this.

After all, our code is largely undocumented as far as inline
documentation goes (it has frustrated me a few times), and the more
comprehensive documentation are the commit messages which git blame
points you to.

Michael