Re: ALSA official git repository

Re: ALSA official git repository

[Linus Torvalds]

On Fri, 27 May 2005, Jaroslav Kysela wrote:
> 
> 	I created new git tree for the ALSA project at:
> 
> rsync://rsync.kernel.org/pub/scm/linux/kernel/git/perex/alsa.git

Your scripts(?) to generate these things are a bit strange, since they
leave an extra empty line in the commit message, which confuses at least
gitweb (ie just look at

   http://www.kernel.org/git/?p=linux/kernel/git/perex/alsa.git;a=summary

and note how the summary thing looks empty).

Now, arguably gitweb should ignore whitespace at the beginning, but 
equally arguably your commits shouldn't have them either...

		Linus

Re: ALSA official git repository

[Sean]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Fri, May 27, 2005 12:13 pm, Linus Torvalds said:
> On Fri, 27 May 2005, Jaroslav Kysela wrote:
>>
>> 	I created new git tree for the ALSA project at:
>>
>> rsync://rsync.kernel.org/pub/scm/linux/kernel/git/perex/alsa.git
>
> Your scripts(?) to generate these things are a bit strange, since they
> leave an extra empty line in the commit message, which confuses at least
> gitweb (ie just look at
>
>    http://www.kernel.org/git/?p=linux/kernel/git/perex/alsa.git;a=summary
>
> and note how the summary thing looks empty).
>
> Now, arguably gitweb should ignore whitespace at the beginning, but
> equally arguably your commits shouldn't have them either...
>

Perhaps git should enforce this?  Patch attached.


Remove leading empty lines from commit messages.

Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>


[-- Attachment #2: trim_leading_commit_ws.patch --]
[-- Type: text/plain, Size: 1133 bytes --]

--- raw/commit-tree.c	2005-05-26 23:38:30.000000000 -0400
+++ argp2/commit-tree.c	2005-05-27 12:46:54.000000000 -0400
@@ -90,6 +90,18 @@
 	free(buf);
 }
 
+static int whitespace(const char *msg) 
+{
+	while (*msg) 
+		switch (*msg) {
+		case ' ': case '\t': case '\n': case '\r':
+			msg++; break;
+		default:
+			return 0;
+		}
+	return 1;
+}
+
 /*
  * Having more than two parents is not strange at all, and this is
  * how multi-way merges are represented.
@@ -112,7 +124,7 @@
 	char comment[1000];
 	struct passwd *pw;
 	char *buffer;
-	unsigned int size;
+	unsigned int size, csize;
 
 	if (argc < 2 || get_sha1_hex(argv[1], tree_sha1) < 0)
 		usage(commit_tree_usage);
@@ -174,8 +186,10 @@
 	add_buffer(&buffer, &size, "committer %s <%s> %s\n\n", commitgecos, commitemail, realdate);
 
 	/* And add the comment */
+	csize = size;
 	while (fgets(comment, sizeof(comment), stdin) != NULL)
-		add_buffer(&buffer, &size, "%s", comment);
+		if (size > csize || ! whitespace(comment))
+			add_buffer(&buffer, &size, "%s", comment);
 
 	write_sha1_file(buffer, size, "commit", commit_sha1);
 	printf("%s\n", sha1_to_hex(commit_sha1));

Re: ALSA official git repository

[Linus Torvalds]

On Fri, 27 May 2005, Sean wrote:
> >
> > Now, arguably gitweb should ignore whitespace at the beginning, but
> > equally arguably your commits shouldn't have them either...
> 
> Perhaps git should enforce this?  Patch attached.
> 
> Remove leading empty lines from commit messages.
> 
> Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>

I'm not sure.

The thing is, right now git allows binary commit messages if somebody
really wants to. Now, a lot of the _tools_ end up only printing up to the
first '\0' or something, but in general, maybe somebody actually wants to
embed his own strange stuff in there (eg use encryption but still use
standard git tools).

Which makes me worry. So I _do_ do whitespace cleanup in my "apply email 
patches" scripts, but I'm not sure whether the core should care about the 
data that people feed it, even for commit messages.

Opinions?

		Linus

Re: ALSA official git repository

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Fri, 27 May 2005, Sean wrote:
>> >
>> > Now, arguably gitweb should ignore whitespace at the beginning, but
>> > equally arguably your commits shouldn't have them either...
>> 
>> Perhaps git should enforce this?  Patch attached.
>> 
>> Remove leading empty lines from commit messages.
>> 
>> Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>

LT> I'm not sure.
LT> Opinions?

Porcelains and gitweb should play with each other nicely, but
the core should _not_ care by default.

An extra option ("--text", perhaps) to git-commit-tree is
acceptable to me, and it may be even a good thing to have.  It
would make life a bit easiear for Porcelain writers if nothing
else.  If that is to happen, I would say we could do more than
just leading blank line removal.  We can also remove trailing
blanks before each LF, tabify indented log message contents, and
remove empty lines before EOF.

Re: ALSA official git repository

[Jaroslav Kysela]

On Fri, 27 May 2005, Linus Torvalds wrote:

> On Fri, 27 May 2005, Jaroslav Kysela wrote:
> > 
> > 	I created new git tree for the ALSA project at:
> > 
> > rsync://rsync.kernel.org/pub/scm/linux/kernel/git/perex/alsa.git
> 
> Your scripts(?) to generate these things are a bit strange, since they
> leave an extra empty line in the commit message, which confuses at least
> gitweb (ie just look at
> 
>    http://www.kernel.org/git/?p=linux/kernel/git/perex/alsa.git;a=summary
> 
> and note how the summary thing looks empty).

Okay, sorry for this small bug. I'll recreate the ALSA git tree with
proper comments again. Also, the author is not correct (should be taken
from the first Signed-off-by:).

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: ALSA official git repository

[Linus Torvalds]

On Fri, 27 May 2005, Jaroslav Kysela wrote:
> 
> Okay, sorry for this small bug. I'll recreate the ALSA git tree with
> proper comments again. Also, the author is not correct (should be taken
> from the first Signed-off-by:).

Hmm.. That's not always true in general, since Sign-off does allow to sign
off on other peoples patches (see the "(b)" clause in DCO), but maybe in
the ALSA tree it is.

Are you coming from a CVS tree or what? It's clearly not my patch 
applicator thing, since that one removes spaces, I'm pretty sure.

		Linus

Re: ALSA official git repository

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Fri, 27 May 2005, Jaroslav Kysela wrote:
> > 
> > Okay, sorry for this small bug. I'll recreate the ALSA git tree with
> > proper comments again. Also, the author is not correct (should be taken
> > from the first Signed-off-by:).
> 
> Hmm.. That's not always true in general, since Sign-off does allow to sign
> off on other peoples patches (see the "(b)" clause in DCO), but maybe in
> the ALSA tree it is.

Yes, I'll occasionally do patches which were written by "A" as:

From: A
...
Signed-off-by: B

And that comes through email as:


...
From: <akpm@osdl.org>
...
From: A
...
Signed-off-by: B


which means that the algorithm for identifying the author is "the final
From:".

I guess the bug here is the use of From: to identify the primary author,
because transporting the patch via email adds ambiguity.

Maybe we should introduce "^Author:"?

Re: ALSA official git repository

[Junio C Hamano]

>>>>> "AM" == Andrew Morton <akpm@osdl.org> writes:

AM> I guess the bug here is the use of From: to identify the primary author,
AM> because transporting the patch via email adds ambiguity.

AM> Maybe we should introduce "^Author:"?

While we are at it, we probably would want "^Author-Date:" as
well.

Re: ALSA official git repository

[Jesper Juhl]

On Fri, 27 May 2005, Andrew Morton wrote:

> Linus Torvalds <torvalds@osdl.org> wrote:
> >
> > 
> > 
> > On Fri, 27 May 2005, Jaroslav Kysela wrote:
> > > 
> > > Okay, sorry for this small bug. I'll recreate the ALSA git tree with
> > > proper comments again. Also, the author is not correct (should be taken
> > > from the first Signed-off-by:).
> > 
> > Hmm.. That's not always true in general, since Sign-off does allow to sign
> > off on other peoples patches (see the "(b)" clause in DCO), but maybe in
> > the ALSA tree it is.
> 
> Yes, I'll occasionally do patches which were written by "A" as:
> 
> From: A
> ...
> Signed-off-by: B
> 
> And that comes through email as:
> 
> 
> ...
> From: <akpm@osdl.org>
> ...
> From: A
> ...
> Signed-off-by: B
> 
> 
> which means that the algorithm for identifying the author is "the final
> From:".
> 
> I guess the bug here is the use of From: to identify the primary author,
> because transporting the patch via email adds ambiguity.
> 
> Maybe we should introduce "^Author:"?
> 

That might be good.  I honestly don't know what would be the best 
solution, but what happens often at the moment is that patches get passed 
on as "From" whatever maintainer (or random resender) happened to pass it 
on to Andrew/Linus and that person then effectively gets labeled as the 
author of the patch in the changelogs/git/whatever. That's not perfect...

Author: might solve it.. worth a shot if you ask me.. 


-- 
Jesper Juhl

Re: ALSA official git repository

[Schneelocke]

On 27/05/05, Andrew Morton <akpm@osdl.org> wrote:
> Yes, I'll occasionally do patches which were written by "A" as:
> 
> From: A
> ...
> Signed-off-by: B
> 
> And that comes through email as:
> 
> ...
> From: <akpm@osdl.org>
> ...
> From: A
> ...
> Signed-off-by: B
> 
> which means that the algorithm for identifying the author is "the final
> From:".
> 
> I guess the bug here is the use of From: to identify the primary author,
> because transporting the patch via email adds ambiguity.
> 
> Maybe we should introduce "^Author:"?

How about "^Written-by:"? That seems to fit in much more nicely with
"Signed-off-by:".
 
-- 
schnee

Re: ALSA official git repository

[Linus Torvalds]

On Fri, 27 May 2005, Andrew Morton wrote:
>
> Yes, I'll occasionally do patches which were written by "A" as:
> 
> From: A
> ...
> Signed-off-by: B
> 
> And that comes through email as:
> 
> 
> ...
> From: <akpm@osdl.org>
> ...
> From: A
> ...
> Signed-off-by: B
> 
> 
> which means that the algorithm for identifying the author is "the final
> From:".

No, the algorithm is:
 - the email author, _or_ if there is one, the top "From:" in the body.

And the rule is that you never remove (or add to) an existing From:, since 
the author doesn't change from being passed around.

Put another way: authorship is very different from sign-off. The sign-off 
gets stacked, the authorship is constant, and thus the rules are 
different.

Also, authorship is more important than sign-off-ship, so authorship goes 
at the top, while sign-offs go at the bottom.

> I guess the bug here is the use of From: to identify the primary author,
> because transporting the patch via email adds ambiguity.

No it doesn't, the email "from" just ends up being the "default" if no 
explicit authorship is noted.

> Maybe we should introduce "^Author:"?

It would still have the same rules, so it wouldn't change anything but the 
tag, so I don't think there is any real advantage to it.

		Linus

Re: ALSA official git repository

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> > which means that the algorithm for identifying the author is "the final
> > From:".
> 
> No, the algorithm is:
>  - the email author, _or_ if there is one, the top "From:" in the body.

That all assumes that the tools are smart enough to separate the email
headers from the body :(

Re: ALSA official git repository

[Linus Torvalds]

On Fri, 27 May 2005, Andrew Morton wrote:
> 
> That all assumes that the tools are smart enough to separate the email
> headers from the body :(

Well, _that_ is trivial: the first empty line is the marker between header 
and body. 

This is a stupid awk program to do this:

	/^From: / { name=$0 }
	state==1 { print name; exit }
	/^$/ { state=1 }

Or something. 


		Linus

Re: ALSA official git repository

[Chris Wedgwood]

On Fri, May 27, 2005 at 03:46:25PM -0700, Andrew Morton wrote:

> That all assumes that the tools are smart enough to separate the email
> headers from the body :(

the first blank line separates these, sed can do that --- so is it
really a problem?

Re: ALSA official git repository

[Jaroslav Kysela]

On Fri, 27 May 2005, Jaroslav Kysela wrote:

> On Fri, 27 May 2005, Linus Torvalds wrote:
> 
> > On Fri, 27 May 2005, Jaroslav Kysela wrote:
> > > 
> > > 	I created new git tree for the ALSA project at:
> > > 
> > > rsync://rsync.kernel.org/pub/scm/linux/kernel/git/perex/alsa.git
> > 
> > Your scripts(?) to generate these things are a bit strange, since they
> > leave an extra empty line in the commit message, which confuses at least
> > gitweb (ie just look at
> > 
> >    http://www.kernel.org/git/?p=linux/kernel/git/perex/alsa.git;a=summary
> > 
> > and note how the summary thing looks empty).
> 
> Okay, sorry for this small bug. I'll recreate the ALSA git tree with
> proper comments again. Also, the author is not correct (should be taken
> from the first Signed-off-by:).

The ALSA git tree is updated with all fixes now. I had an old git version 
which inserted this extra line at top of comments.

Also, it seems that there's a delay between master.kernel.org and git web 
interface at www.kernel.org (the changes are not on web yet).

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs