[PATCH] shortlog: take the first populated line of the description

Way back the perl version of shortlog would take the first populated line
of the commit body.  The builtin version mearly takes the first line.
This leads to empty shortlog entries when there is some viable text in
the commit.

Reinstate this behaviour igoring all lines with nothing but whitespace.

Signed-off-by: Andy Whitcroft <redacted>
---

	This seems to be an improvement, returning to the original
	behaviour.  I cannot think of any good reason not to take the first
	populated line for a shortlog.	The alternative less agressive
	compromise might be to skip only completly empty lines at the
	start, but I am not sure that adds any value.

	I seem to get a lot of these in converted SVN commits.

	Comments?

	-apw
---
 builtin-shortlog.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index af31aba..b22b0ed 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -70,11 +70,12 @@ static void insert_one_record(struct shortlog *log,
 	else
 		free(buffer);
 
+	/* Skip any leading whitespace, including any blank lines. */
+	while (*oneline && isspace(*oneline))
+		oneline++;
 	eol = strchr(oneline, '\n');
 	if (!eol)
 		eol = oneline + strlen(oneline);
-	while (*oneline && isspace(*oneline) && *oneline != '\n')
-		oneline++;
 	if (!prefixcmp(oneline, "[PATCH")) {
 		char *eob = strchr(oneline, ']');
 		if (eob && (!eol || eob < eol))

Hi,

On Wed, 5 Mar 2008, Andy Whitcroft wrote:

Why do you move the code around?  Makes it harder to read your patch.  
Besides, you now strip empty lines at the beginning of the commit 
messages, right?  Who produces such a thing?

Ciao,
Dscho

On Wed, Mar 05, 2008 at 03:48:00PM +0100, Johannes Schindelin wrote:

Hi,

On Wed, 5 Mar 2008, Andy Whitcroft wrote:

quoted

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index af31aba..b22b0ed 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -70,11 +70,12 @@ static void insert_one_record(struct shortlog *log,
 	else
 		free(buffer);
 
+	/* Skip any leading whitespace, including any blank lines. */
+	while (*oneline && isspace(*oneline))
+		oneline++;
 	eol = strchr(oneline, '\n');
 	if (!eol)
 		eol = oneline + strlen(oneline);
-	while (*oneline && isspace(*oneline) && *oneline != '\n')
-		oneline++;
 	if (!prefixcmp(oneline, "[PATCH")) {
 		char *eob = strchr(oneline, ']');
 		if (eob && (!eol || eob < eol))

Why do you move the code around?  Makes it harder to read your patch.  
Besides, you now strip empty lines at the beginning of the commit 
messages, right?  Who produces such a thing?

I've not moved the code as such.  I added a loop to drop the leading
whitespace.  That made the second loop redundant as its job is already
done, so I killed it.

The point of the patch is to strip the empty lines at the start of
the commit.  I am ending up with them in my repo mostly due to
imcompetant users of SVN I suspect.  The main driver is that I have
those and the original non-C version coped and the builtin does not.

Now if people think that its a stupid idea I might suggest it could be
optional?

-apw

Hi,

On Wed, 5 Mar 2008, Andy Whitcroft wrote:

On Wed, Mar 05, 2008 at 03:48:00PM +0100, Johannes Schindelin wrote:

quoted

On Wed, 5 Mar 2008, Andy Whitcroft wrote:

quoted

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index af31aba..b22b0ed 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -70,11 +70,12 @@ static void insert_one_record(struct shortlog *log,
 	else
 		free(buffer);
 
+	/* Skip any leading whitespace, including any blank lines. */
+	while (*oneline && isspace(*oneline))
+		oneline++;
 	eol = strchr(oneline, '\n');
 	if (!eol)
 		eol = oneline + strlen(oneline);
-	while (*oneline && isspace(*oneline) && *oneline != '\n')
-		oneline++;
 	if (!prefixcmp(oneline, "[PATCH")) {
 		char *eob = strchr(oneline, ']');
 		if (eob && (!eol || eob < eol))

Why do you move the code around?  Makes it harder to read your patch.  
Besides, you now strip empty lines at the beginning of the commit 
messages, right?  Who produces such a thing?

I've not moved the code as such.

Well, it sure looks like that.  Maybe you want to make it look like that 
even more?

The point of the patch is to strip the empty lines at the start of the 
commit.  I am ending up with them in my repo mostly due to imcompetant 
users of SVN I suspect.  The main driver is that I have those and the 
original non-C version coped and the builtin does not.

Now if people think that its a stupid idea I might suggest it could be 
optional?

No, I think with an explanation like this in the commit message, it is 
good as-is.

Ciao,
Dscho

On Wed, 5 Mar 2008, Andy Whitcroft wrote:

Way back the perl version of shortlog would take the first populated line
of the commit body.  The builtin version mearly takes the first line.
This leads to empty shortlog entries when there is some viable text in
the commit.

Reinstate this behaviour igoring all lines with nothing but whitespace.

Signed-off-by: Andy Whitcroft <redacted>
---

	This seems to be an improvement, returning to the original
	behaviour.  I cannot think of any good reason not to take the first
	populated line for a shortlog.	The alternative less agressive
	compromise might be to skip only completly empty lines at the
	start, but I am not sure that adds any value.

	I seem to get a lot of these in converted SVN commits.

	Comments?

Maybe it is the SVN conversion that needs fixing?


Nicolas

Nicolas Pitre [off-list ref] writes:

On Wed, 5 Mar 2008, Andy Whitcroft wrote:
...

quoted

	...  I cannot think of any good reason not to take the first
	populated line for a shortlog.	The alternative less agressive
	compromise might be to skip only completly empty lines at the
	start, but I am not sure that adds any value.

	I seem to get a lot of these in converted SVN commits.

	Comments?

Maybe it is the SVN conversion that needs fixing?

I thought about saying the same, but I am of two minds.

It is likely that you would want to clean-up when importing,
especially when you are planning to abandon the other system and
switch to git.  But you may want to have an import that is as
close to the original as possible, excess blank lines in the log
messages and all.

I think Andy's fix to make the output side take away
unnecessary blank lines is unconditionally good.

I've added these three lines at the end of the log message.

    This is often useful when dealing with commits imported from foreign SCMs
    that do not tidy up the log message of useless blank lines at the
    beginning.

On Wed, 5 Mar 2008, Junio C Hamano wrote:

Nicolas Pitre [off-list ref] writes:

quoted

On Wed, 5 Mar 2008, Andy Whitcroft wrote:
...

quoted

	...  I cannot think of any good reason not to take the first
	populated line for a shortlog.	The alternative less agressive
	compromise might be to skip only completly empty lines at the
	start, but I am not sure that adds any value.

	I seem to get a lot of these in converted SVN commits.

	Comments?

Maybe it is the SVN conversion that needs fixing?

I thought about saying the same, but I am of two minds.

It is likely that you would want to clean-up when importing,
especially when you are planning to abandon the other system and
switch to git.  But you may want to have an import that is as
close to the original as possible, excess blank lines in the log
messages and all.

I don't think excess blank lines at the _beginning_ of the commit log 
has any value worth preserving though.

I think Andy's fix to make the output side take away
unnecessary blank lines is unconditionally good.

It is not "bad" in itself.  However that feels like papering over 
another problem which IMHO has greater merits to be fixed.  We have 
given special semantics to the first line of a commit log in many other 
places now, so unless all those places are also made aware of 
substandard commit logs too, I think it would be more productive to make 
sure those logs are semi sensible upon entering Git in the first place 
instead.


Nicolas