threads / patch / 43343

patchRe: [PATCH] remove "[PATCH]" prefix from shortlog output

Subject: Re: [PATCH] remove "[PATCH]" prefix from shortlog output

## tl;dr

7 messages between Dec 10, 2006 and Dec 11, 2006. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Nicolas Pitre· Dec 10, 2006, 00:58 UTC · lore

[PATCH] remove "[PATCH]" prefix from shortlog output

This doesn't convey much information in the shortlog context. And the perl version did strip it as well.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Show changes to builtin-shortlog.c +5 −0
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 7a2ddfe..6c4606b 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -146,6 +146,11 @@ static void insert_author_oneline(struct path_list *list,
 	while (onelinelen > 0 && isspace(oneline[onelinelen - 1]))
 		onelinelen--;
 
+	if (onelinelen > 8 && !strncasecmp(oneline, "[PATCH] ", 8)) {
+		oneline += 8;
+		onelinelen -= 8;
+	}
+
 	buffer = xmalloc(onelinelen + 1);
 	memcpy(buffer, oneline, onelinelen);
Johannes Schindelin· Dec 10, 2006, 22:40 UTC · re: Nicolas Pitre · lore
Hi,
On Sat, 9 Dec 2006, Nicolas Pitre wrote:
> This doesn't convey much information in the shortlog context.
> And the perl version did strip it as well.
From the current builtin-shortlog.c:128ff:
        if (!strncmp(oneline, "[PATCH", 6)) {
                char *eob = strchr(oneline, ']');
                if (eob) {
                        while (isspace(eob[1]) && eob[1] != '\n')
                                eob++;
                        if (eob - oneline < onelinelen) {
                                onelinelen -= eob - oneline;
                                oneline = eob;
                        }
                }
        }

It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically every prefix beginning with "[PATCH" and ending in "]". I do not remember if I really tested that code, but it should work.

Ciao, Dscho

Jakub Narebski· Dec 10, 2006, 23:10 UTC · re: Johannes Schindelin · lore
Johannes Schindelin wrote:
Show 23 quoted lines
> On Sat, 9 Dec 2006, Nicolas Pitre wrote:
> 
>> This doesn't convey much information in the shortlog context.
>> And the perl version did strip it as well.
> 
> From the current builtin-shortlog.c:128ff:
> 
>         if (!strncmp(oneline, "[PATCH", 6)) {
>                 char *eob = strchr(oneline, ']');
> 
>                 if (eob) {
>                         while (isspace(eob[1]) && eob[1] != '\n')
>                                 eob++;
>                         if (eob - oneline < onelinelen) {
>                                 onelinelen -= eob - oneline;
>                                 oneline = eob;
>                         }
>                 }
>         }
> 
> It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically 
> every prefix beginning with "[PATCH" and ending in "]". I do not remember 
> if I really tested that code, but it should work.

What happens if you have "[PATCH" without closing "]"? Does it work (and doesn't crash)? Does it strip anything?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Johannes Schindelin· Dec 10, 2006, 23:35 UTC · re: Jakub Narebski · lore
Hi,
On Mon, 11 Dec 2006, Jakub Narebski wrote:
Show 28 quoted lines
> Johannes Schindelin wrote:
> 
> > On Sat, 9 Dec 2006, Nicolas Pitre wrote:
> > 
> >> This doesn't convey much information in the shortlog context.
> >> And the perl version did strip it as well.
> > 
> > From the current builtin-shortlog.c:128ff:
> > 
> >         if (!strncmp(oneline, "[PATCH", 6)) {
> >                 char *eob = strchr(oneline, ']');
> > 
> >                 if (eob) {
> >                         while (isspace(eob[1]) && eob[1] != '\n')
> >                                 eob++;
> >                         if (eob - oneline < onelinelen) {
> >                                 onelinelen -= eob - oneline;
> >                                 oneline = eob;
> >                         }
> >                 }
> >         }
> > 
> > It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically 
> > every prefix beginning with "[PATCH" and ending in "]". I do not remember 
> > if I really tested that code, but it should work.
> 
> What happens if you have "[PATCH" without closing "]"? Does it work (and
> doesn't crash)? Does it strip anything?
Use the source, Luke!

If "[PATCH" is there, but "]" is not, then eob will be NULL, and nothing happens.

Well, actually it is not completely true: the code searches for "]" in the _complete_ commit message (which _is_ terminated by '\0'). But only if it is found, _and_ it is in the first line, the substring is trimmed away.

Hth, Dscho

Junio C Hamano· Dec 10, 2006, 23:32 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically 
> every prefix beginning with "[PATCH" and ending in "]". I do not remember 
> if I really tested that code, but it should work.

The problem is that you forgot that the lines are indented when acting as a filter.

How about this? It also contains the extra whitespace removal from the author name.

Show changes to builtin-shortlog.c +10 −3
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 7a2ddfe..3fc43dd 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -188,18 +188,25 @@ static void read_from_stdin(struct path_list *list)
 				bob = buffer + strlen(buffer);
 			else {
 				offset = 8;
-				if (isspace(bob[-1]))
+				while (buffer + offset < bob &&
+				       isspace(bob[-1]))
 					bob--;
 			}
 
 			while (fgets(buffer2, sizeof(buffer2), stdin) &&
 					buffer2[0] != '\n')
 				; /* chomp input */
-			if (fgets(buffer2, sizeof(buffer2), stdin))
+			if (fgets(buffer2, sizeof(buffer2), stdin)) {
+				int l2 = strlen(buffer2);
+				int i;
+				for (i = 0; i < l2; i++)
+					if (!isspace(buffer2[i]))
+						break;
 				insert_author_oneline(list,
 						buffer + offset,
 						bob - buffer - offset,
-						buffer2, strlen(buffer2));
+						buffer2 + i, l2 - i);
+			}
 		}
 	}
 }
Johannes Schindelin· Dec 10, 2006, 23:41 UTC · re: Junio C Hamano · lore
Hi,
On Sun, 10 Dec 2006, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically 
> > every prefix beginning with "[PATCH" and ending in "]". I do not remember 
> > if I really tested that code, but it should work.
> 
> The problem is that you forgot that the lines are indented when
> acting as a filter.
Of course!

And your patch looks correct to me. Ciao,

Nicolas Pitre· Dec 11, 2006, 03:17 UTC · re: Johannes Schindelin · lore
On Mon, 11 Dec 2006, Johannes Schindelin wrote:
Show 9 quoted lines
> Hi,
> 
> On Sun, 10 Dec 2006, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > It tries not only to strip "[PATCH]", but also "[PATCH 0/n]" and basically 
> > > every prefix beginning with "[PATCH" and ending in "]". I do not remember 
> > > if I really tested that code, but it should work.
Hmmm well, right.  And of course I failed to notice that code was there.

I think I was corrupted by the dark side and failed to properly "use the source" twice on that tool this week.

> > The problem is that you forgot that the lines are indented when
> > acting as a filter.
Which was indeed my use case.
> Of course!
> 
> And your patch looks correct to me.
Confirmed.

← back to recent threads