Re: [PATCH] Make find_commit_subject() more robust

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:

> Hi Junio,
>
> On Mon, 20 Jun 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>> 
>> > Just like the pretty printing machinery, we should simply ignore empty
>> > lines at the beginning of the commit messages.
>> >
>> > This discrepancy was noticed when an early version of the rebase--helper
>> > produced commit objects with more than one empty line between the header
>> > and the commit message.
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
>> > ---
>> > Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v1
>> >
>> > 	Aaaaand another patch from the rebase--helper front. I guess I'll
>> > 	call it a day with this one.
>> 
>> Makes sense.  This has a trivial textual conflict with cleanup
>> patches in flight, I think, but that is not a big problem.
>
> I will gladly resend rebased to `next`, if you wish.

No, I'd prefer a patch that applies to 'master' for a new feature;
there is no need to deliberately get taken hostage by other topics.

>> It does hint that we might want to find a library function that can
>> replace a hand-rolled while loop we are adding here, though ;-)
>
> Heh. I cannot help you with that ;-)

The reason it hints such a thing is because the line nearby that
does this:

  		for (eol = p; *eol && *eol != '\n'; eol++)
  			; /* do nothing */

gets rewritten to

		eol = strchrnul(p, '\n');

i.e. "give me the pointer to the first byte that is '\n', or EOS".

Your patch introduces a similar loop with similar (but different)
purpose:

		while (*p == '\n')
			p++;

which would have been helped if there were a helper with an
opposite function, i.e.

		p = strcchrnul(p, '\n');

i.e. "give me the pointer to the first byte that is not '\n', or EOS".

But there is no such thing.  Although p += strcspn(p, "\n") is a
possibility, that somehow feels a bit odd.  And that is why I did
not hint any existing function and said "might want to find".

HOWEVER.

Stepping back a bit, I think what we actually want is

		p = skip_blank_lines(p);

that skips any and all blank lines, including an empty line that
consists of all whitespace.

For example

	(
		# grab the header lines
		git cat-file commit HEAD | sed -e '/^$/q'
                # throw in random blank lines
		echo
                echo " "
                echo "  "
                echo "   "
                echo
                echo "Title line"
	) | git hash-object -w -t commit --stdin

would mint a commit object that has many blank lines in the front,
some have whitespace and are not empty.  If you give it to

	git show -s | cat -e
        git show -s --oneline | cat -e

I think you would see what I mean.
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]