[PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[andy]

From: Andy Lester <andy@petdance.com>

Added const to some function parameters.
---
 builtin-send-pack.c |  190 ++-------------------------------------------------
 remote.c            |    4 +-
 remote.h            |    2 +-
 send-pack.h         |    2 +-
 transport.c         |   18 +++---
 transport.h         |    5 ++
 6 files changed, 23 insertions(+), 198 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d5a1c48..5c33f9d 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -5,6 +5,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "send-pack.h"
+#include "transport.h"
 
 static const char send_pack_usage[] =
 "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -29,7 +30,7 @@ static int feed_object(const unsigned char *sha1, int fd, int negative)
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
-static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extra, struct send_pack_args *args)
+static int pack_objects(int fd, const struct ref *refs, const struct extra_have_objects *extra, const struct send_pack_args *args)
 {
 	/*
 	 * The child becomes pack-objects --revs; we feed
@@ -146,157 +147,7 @@ static int receive_status(int in, struct ref *refs)
 	return ret;
 }
 
-static void update_tracking_ref(struct remote *remote, struct ref *ref)
-{
-	struct refspec rs;
-
-	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
-		return;
-
-	rs.src = ref->name;
-	rs.dst = NULL;
-
-	if (!remote_find_tracking(remote, &rs)) {
-		if (args.verbose)
-			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (ref->deletion) {
-			delete_ref(rs.dst, NULL, 0);
-		} else
-			update_ref("update by push", rs.dst,
-					ref->new_sha1, NULL, 0, 0);
-		free(rs.dst);
-	}
-}
-
-#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
-{
-	fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
-	if (from)
-		fprintf(stderr, "%s -> %s", prettify_ref(from), prettify_ref(to));
-	else
-		fputs(prettify_ref(to), stderr);
-	if (msg) {
-		fputs(" (", stderr);
-		fputs(msg, stderr);
-		fputc(')', stderr);
-	}
-	fputc('\n', stderr);
-}
-
-static const char *status_abbrev(unsigned char sha1[20])
-{
-	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
-}
-
-static void print_ok_ref_status(struct ref *ref)
-{
-	if (ref->deletion)
-		print_ref_status('-', "[deleted]", ref, NULL, NULL);
-	else if (is_null_sha1(ref->old_sha1))
-		print_ref_status('*',
-			(!prefixcmp(ref->name, "refs/tags/") ? "[new tag]" :
-			  "[new branch]"),
-			ref, ref->peer_ref, NULL);
-	else {
-		char quickref[84];
-		char type;
-		const char *msg;
-
-		strcpy(quickref, status_abbrev(ref->old_sha1));
-		if (ref->nonfastforward) {
-			strcat(quickref, "...");
-			type = '+';
-			msg = "forced update";
-		} else {
-			strcat(quickref, "..");
-			type = ' ';
-			msg = NULL;
-		}
-		strcat(quickref, status_abbrev(ref->new_sha1));
-
-		print_ref_status(type, quickref, ref, ref->peer_ref, msg);
-	}
-}
-
-static int print_one_push_status(struct ref *ref, const char *dest, int count)
-{
-	if (!count)
-		fprintf(stderr, "To %s\n", dest);
-
-	switch(ref->status) {
-	case REF_STATUS_NONE:
-		print_ref_status('X', "[no match]", ref, NULL, NULL);
-		break;
-	case REF_STATUS_REJECT_NODELETE:
-		print_ref_status('!', "[rejected]", ref, NULL,
-				"remote does not support deleting refs");
-		break;
-	case REF_STATUS_UPTODATE:
-		print_ref_status('=', "[up to date]", ref,
-				ref->peer_ref, NULL);
-		break;
-	case REF_STATUS_REJECT_NONFASTFORWARD:
-		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-				"non-fast forward");
-		break;
-	case REF_STATUS_REMOTE_REJECT:
-		print_ref_status('!', "[remote rejected]", ref,
-				ref->deletion ? NULL : ref->peer_ref,
-				ref->remote_status);
-		break;
-	case REF_STATUS_EXPECTING_REPORT:
-		print_ref_status('!', "[remote failure]", ref,
-				ref->deletion ? NULL : ref->peer_ref,
-				"remote failed to report status");
-		break;
-	case REF_STATUS_OK:
-		print_ok_ref_status(ref);
-		break;
-	}
-
-	return 1;
-}
-
-static void print_push_status(const char *dest, struct ref *refs)
-{
-	struct ref *ref;
-	int n = 0;
-
-	if (args.verbose) {
-		for (ref = refs; ref; ref = ref->next)
-			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n);
-	}
-
-	for (ref = refs; ref; ref = ref->next)
-		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n);
-
-	for (ref = refs; ref; ref = ref->next) {
-		if (ref->status != REF_STATUS_NONE &&
-		    ref->status != REF_STATUS_UPTODATE &&
-		    ref->status != REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n);
-	}
-}
-
-static int refs_pushed(struct ref *ref)
-{
-	for (; ref; ref = ref->next) {
-		switch(ref->status) {
-		case REF_STATUS_NONE:
-		case REF_STATUS_UPTODATE:
-			break;
-		default:
-			return 1;
-		}
-	}
-	return 0;
-}
-
-int send_pack(struct send_pack_args *args,
+int send_pack(const struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
 	      struct extra_have_objects *extra_have)
@@ -426,37 +277,6 @@ int send_pack(struct send_pack_args *args,
 	return 0;
 }
 
-static void verify_remote_names(int nr_heads, const char **heads)
-{
-	int i;
-
-	for (i = 0; i < nr_heads; i++) {
-		const char *local = heads[i];
-		const char *remote = strrchr(heads[i], ':');
-
-		if (*local == '+')
-			local++;
-
-		/* A matching refspec is okay.  */
-		if (remote == local && remote[1] == '\0')
-			continue;
-
-		remote = remote ? (remote + 1) : local;
-		switch (check_ref_format(remote)) {
-		case 0: /* ok */
-		case CHECK_REF_FORMAT_ONELEVEL:
-			/* ok but a single level -- that is fine for
-			 * a match pattern.
-			 */
-		case CHECK_REF_FORMAT_WILDCARD:
-			/* ok but ends with a pattern-match character */
-			continue;
-		}
-		die("remote part of refspec is not a valid name in %s",
-		    heads[i]);
-	}
-}
-
 int cmd_send_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, nr_refspecs = 0;
@@ -576,12 +396,12 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	ret |= finish_connect(conn);
 
-	print_push_status(dest, remote_refs);
+	print_push_status(dest, remote_refs, args.verbose);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
 		for (ref = remote_refs; ref; ref = ref->next)
-			update_tracking_ref(remote, ref);
+			update_tracking_ref(remote, ref, args.verbose);
 	}
 
 	if (!ret && !refs_pushed(remote_refs))
diff --git a/remote.c b/remote.c
index 91f7485..f7a5c49 100644
--- a/remote.c
+++ b/remote.c
@@ -769,9 +769,9 @@ static int match_name_with_pattern(const char *key, const char *name,
 	return ret;
 }
 
-int remote_find_tracking(struct remote *remote, struct refspec *refspec)
+int remote_find_tracking(const struct remote *remote, struct refspec *refspec)
 {
-	int find_src = refspec->src == NULL;
+	const int find_src = (refspec->src == NULL);
 	char *needle, **result;
 	int i;
 
diff --git a/remote.h b/remote.h
index 99706a8..d624e08 100644
--- a/remote.h
+++ b/remote.h
@@ -108,7 +108,7 @@ struct ref *get_remote_ref(const struct ref *remote_refs, const char *name);
 /*
  * For the given remote, reads the refspec's src and sets the other fields.
  */
-int remote_find_tracking(struct remote *remote, struct refspec *refspec);
+int remote_find_tracking(const struct remote *remote, struct refspec *refspec);
 
 struct branch {
 	const char *name;
diff --git a/send-pack.h b/send-pack.h
index 83d76c7..88a407e 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -9,7 +9,7 @@ struct send_pack_args {
 		dry_run:1;
 };
 
-int send_pack(struct send_pack_args *args,
+int send_pack(const struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs, struct extra_have_objects *extra_have);
 
diff --git a/transport.c b/transport.c
index 3dfb03c..d50160b 100644
--- a/transport.c
+++ b/transport.c
@@ -690,7 +690,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	return (refs ? 0 : -1);
 }
 
-static int refs_pushed(struct ref *ref)
+int refs_pushed(const struct ref *ref)
 {
 	for (; ref; ref = ref->next) {
 		switch(ref->status) {
@@ -704,7 +704,7 @@ static int refs_pushed(struct ref *ref)
 	return 0;
 }
 
-static void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
+void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose)
 {
 	struct refspec rs;
 
@@ -728,7 +728,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref, int verb
 
 #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+static void print_ref_status(char flag, const char *summary, const struct ref *to, const struct ref *from, const char *msg)
 {
 	fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
 	if (from)
@@ -743,12 +743,12 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 	fputc('\n', stderr);
 }
 
-static const char *status_abbrev(unsigned char sha1[20])
+static const char *status_abbrev(const unsigned char *sha1)
 {
 	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
 }
 
-static void print_ok_ref_status(struct ref *ref)
+static void print_ok_ref_status(const struct ref *ref)
 {
 	if (ref->deletion)
 		print_ref_status('-', "[deleted]", ref, NULL, NULL);
@@ -778,7 +778,7 @@ static void print_ok_ref_status(struct ref *ref)
 	}
 }
 
-static int print_one_push_status(struct ref *ref, const char *dest, int count)
+static int print_one_push_status(const struct ref *ref, const char *dest, int count)
 {
 	if (!count)
 		fprintf(stderr, "To %s\n", dest);
@@ -817,9 +817,9 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)
 	return 1;
 }
 
-static void print_push_status(const char *dest, struct ref *refs, int verbose)
+void print_push_status(const char *dest, const struct ref *refs, int verbose)
 {
-	struct ref *ref;
+	const struct ref *ref;
 	int n = 0;
 
 	if (verbose) {
@@ -840,7 +840,7 @@ static void print_push_status(const char *dest, struct ref *refs, int verbose)
 	}
 }
 
-static void verify_remote_names(int nr_heads, const char **heads)
+void verify_remote_names(int nr_heads, const char **heads)
 {
 	int i;
 
diff --git a/transport.h b/transport.h
index b1c2252..ea77c7c 100644
--- a/transport.h
+++ b/transport.h
@@ -75,4 +75,9 @@ int transport_fetch_refs(struct transport *transport, const struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
 int transport_disconnect(struct transport *transport);
 
+void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose);
+void print_push_status(const char *dest, const struct ref *refs, int verbose);
+int refs_pushed(const struct ref *ref);
+void verify_remote_names(int nr_heads, const char **heads);
+
 #endif
-- 
1.6.2.4

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Jeff King]

Great, I think this is something that needs to be done, and it is always
nice to see a diffstat like:

>  6 files changed, 23 insertions(+), 198 deletions(-)

that shows massive cleanup. But your patch is a little hard to review,
so let me try to constructively critique your commit message for a
moment.

First off, it really seems like there are two things happening here:
removing the functions mentioned in the subject, and:

> Added const to some function parameters.

I don't think those are related, so it makes sense to split them into
two patches in a series. This has a few advantages:

  - when reviewers read the patch, they know to which topic each change
    belongs (and yes, we can figure it out by reading the change
    carefully, but it is a lot easier when you start reading a diff to
    say "OK, I know approximately what this is going to do from the
    commit message" and then confirm that it does what you thought)

  - if one of the topics is controversial but the other is not, the
    non-controversial changes are not held hostage while the
    controversial ones are discussed or re-done

Moving on to the message itself, it is very top-heavy:

> Subject: Re: [PATCH] Removed redundant static functions such as
>	update_tracking_ref() and verify_remote_names() from
>	builtin-send-pack.c, and made the ones in transport.c not be static
>	so they can be used instead.

The first line of the message is really supposed to be a one-liner, like
the subject of an email, to give people a general sense of what is going
on. Then you can go into more detail in a follow-on paragraph. That
makes things like "gitk" and "git log --oneline" more useful.

And as a grammatical nit, in git itself we usually use the imperative
mood in commit mesages. So "remove" and "add" instead of "removed and
added".

As far as the details itself, usually you want to talk about _why_ to
make this change. In this case, removing redundant code is a pretty
obvious reason, but I am left wondering another why: why is this OK to
do? In other words, where did the duplication come from, why was it
duplicated instead of refactored in the first place (simple oversight,
or some assumption that was true then, etc), and why are things
different now (correcting an oversight, that assumption no longer holds,
etc). From our prior discussion, the code came from 64fcef2. But I'm not
sure if the duplicated code is completely identical. I.e., was it
tweaked when it was copied to transport.c? If not, then say so, because
that is a question every reviewer should have. If so, then why is it OK
for send-pack to start using the tweaked version?

I have some guesses about the answers to those from our prior
discussions. But part of making the patch would be looking into those
things. And keep in mind that Junio probably didn't read our prior
discussion, nor will somebody reading the commit message two years from
now.

So I think the commmit message you want would be something like:

  remove duplicate functions from builtin-send-pack.c

  These functions are helpers for handling tracking refs, printing
  output, etc. They were originally used only by send-pack, but commit
  64fcef2 copied them to transport.c so that they could be used by all
  transports.

  As the versions in transport.c and builtin-send-pack.c are identical
  [or whatever you find out when you investigate], there is no reason 
  for there to be two copies. Copying instead of moving in 64fcef2
  appears to have simply been an oversight [even better, get
  confirmation from Daniel on why he did it that way].

  This patch just removes the versions in builtin-send-pack.c, and
  makes the ones in transport.c available as library functions.

As for the patch itself, there are a few spots I noticed in my cursory
look:

> --- a/remote.c
> +++ b/remote.c
> @@ -769,9 +769,9 @@ static int match_name_with_pattern(const char *key, const char *name,
> [...]
> +int remote_find_tracking(const struct remote *remote, struct refspec *refspec)
>  {
> -	int find_src = refspec->src == NULL;
> +	const int find_src = (refspec->src == NULL);

I don't think we usually worry about const-ing local variables like
this, but instead just focus on const-ing parameters. The compiler can
generally already detect constness of find_src here, because it can see
all of the places it is used (whereas crossing a function boundary,
anything can happen to a non-const parameter).

> -static const char *status_abbrev(unsigned char sha1[20])
> +static const char *status_abbrev(const unsigned char *sha1)

Is there a good reason to drop this from an array to a pointer?

> +void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose);
> +void print_push_status(const char *dest, const struct ref *refs, int verbose);
> +int refs_pushed(const struct ref *ref);
> +void verify_remote_names(int nr_heads, const char **heads);

This might need to be given more descriptive names if they will have
global linkage.

I do wonder, though...if http and other transports are using these via
transport.c, then why is the git transport not doing the same thing? In
other words, should they actually be statics, and the calls ripped out
of send_pack()? Or send_pack() just moved into transport.c?

-Peff

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Andy Lester]

On Apr 24, 2009, at 4:04 PM, Jeff King wrote:

> in git itself we usually use the imperative
> mood in commit mesages.


Boy, you guys are hardcore. :-)

This was what I was looking for.  I think what I'll do is fold your  
message into Documentation/SubmittingPatches and submit that as a  
patch first.

Thanks,
xoxo,
Andy

--
Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Jeff King]

On Fri, Apr 24, 2009 at 04:13:14PM -0500, Andy Lester wrote:

> This was what I was looking for.  I think what I'll do is fold your  
> message into Documentation/SubmittingPatches and submit that as a patch 
> first.

That probably makes sense.

I keep thinking about writing a separate "how to write a good commit
message" document that would be more universal than just "here's how you
submit a patch to git". And some of what I wrote to you could probably
go in such a document. But I don't know if it makes sense to start a new
document just with what I said there; it might be a bit sparse (OTOH,
maybe people would then be encouraged to add their tips to it).

-Peff

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 24, 2009 at 04:13:14PM -0500, Andy Lester wrote:
>
>> This was what I was looking for.  I think what I'll do is fold your  
>> message into Documentation/SubmittingPatches and submit that as a patch 
>> first.
>
> That probably makes sense.
>
> I keep thinking about writing a separate "how to write a good commit
> message" document that would be more universal than just "here's how you
> submit a patch to git". And some of what I wrote to you could probably
> go in such a document. But I don't know if it makes sense to start a new
> document just with what I said there; it might be a bit sparse (OTOH,
> maybe people would then be encouraged to add their tips to it).

I'm sure our capable project secretary would come up with list of quotes
in the archive from Linus and I perhaps over the weekend in her copious
spare time ;-).

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Johannes Schindelin]

Hi,

On Fri, 24 Apr 2009, Andy Lester wrote:

> On Apr 24, 2009, at 4:04 PM, Jeff King wrote:
> 
> >in git itself we usually use the imperative mood in commit mesages.
> 
> This was what I was looking for.  I think what I'll do is fold your 
> message into Documentation/SubmittingPatches and submit that as a patch 
> first.

I dunno.  The most important part of CodingGuidelines is this:

	As for more concrete guidelines, just imitate the existing code
	(this is a good guideline, no matter which project you are
	contributing to).

(And of course, this holds for the style of commit messages, too.)

Ciao,
Dscho

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Andy Lester]

On Apr 24, 2009, at 7:07 PM, Johannes Schindelin wrote:

> I dunno.  The most important part of CodingGuidelines is this:
>
> 	As for more concrete guidelines, just imitate the existing code
> 	(this is a good guideline, no matter which project you are
> 	contributing to).
>
> (And of course, this holds for the style of commit messages, too.)


Would you rather I not bother?  Far be it from me to try to force  
myself on any project.

--
Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Johannes Schindelin]

Hi,

On Fri, 24 Apr 2009, Andy Lester wrote:

> On Apr 24, 2009, at 7:07 PM, Johannes Schindelin wrote:
> 
> >I dunno.  The most important part of CodingGuidelines is this:
> >
> > As for more concrete guidelines, just imitate the existing code
> > (this is a good guideline, no matter which project you are
> > contributing to).
> >
> >(And of course, this holds for the style of commit messages, too.)
> 
> 
> Would you rather I not bother?  Far be it from me to try to force myself on
> any project.

Sorry, Andy, I forgot to add the

Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)

Still with me?  Good.  Nice to meet you.

Just for the record: responding to a patch is my strongest way of saying
that I appreciate your work.

The thing with SubmittingPatches is: I think it is already too long for 
people to quickly read and get stuff done.

But hey, I was wrong before, and I will be wrong again.  That's why I 
offered my opinion, and I _can_ be convinced of another opinion.

Ciao,
Dscho

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Sam Vilain]

Andy Lester wrote:
> > I dunno.  The most important part of CodingGuidelines is this:
> >     As for more concrete guidelines, just imitate the existing code
> >     (this is a good guideline, no matter which project you are
> >     contributing to).
> > (And of course, this holds for the style of commit messages, too.)
> Would you rather I not bother?  Far be it from me to try to force
> myself on any project.

Bother?  What bother?  Do you think we're kidding? :-)

Subject: [PATCH] SubmittingPatches: itemize and reflect upon well written changes

The SubmittingPatches file was trimmed down from a somewhat
overwhelming set of requirements from the Linux Kernel equivalent;
however perhaps a little of it can be returned without making the
text too long.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 <insert funny meta-circular joke here>

 Documentation/SubmittingPatches |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 8d818a2..76fc84d 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -6,9 +6,13 @@ Checklist (and a short version for the impatient):
 	- check for unnecessary whitespace with "git diff --check"
 	  before committing
 	- do not check in commented out code or unneeded files
-	- provide a meaningful commit message
 	- the first line of the commit message should be a short
 	  description and should skip the full stop
+	- the body should provide a meaningful commit message, which:
+		- uses the imperative, present tense: "change",
+		  not "changed" or "changes".
+		- includes motivation for the change, and contrasts
+		  its implementation with previous behaviour
 	- if you want your work included in git.git, add a
 	  "Signed-off-by: Your Name <you@example.com>" line to the
 	  commit message (or just use the option "-s" when
@@ -62,6 +66,14 @@ Describe the technical detail of the change(s).
 
 If your description starts to get too long, that's a sign that you
 probably need to split up your commit to finer grained pieces.
+That being said, patches which plainly describe the things that
+help reviewers check the patch, and future maintainers understand
+the code, are the most beautiful patches.  Descriptions that summarise
+the point in the subject well, and describe the motivation for the
+change, the approach taken by the change, and if relevant how this
+differs substantially from the prior version, can be found on Usenet
+archives back into the late 80's.  Consider it like good Netiquette,
+but for code.
 
 Oh, another thing.  I am picky about whitespaces.  Make sure your
 changes do not trigger errors with the sample pre-commit hook shipped
-- 
1.6.2.234.g28eec

Re: [PATCH] Removed redundant static functions such as update_tracking_ref() and verify_remote_names() from builtin-send-pack.c, and made the ones in transport.c not be static so they can be used instead.

[Jeff King]

On Tue, Apr 28, 2009 at 02:38:47AM +1200, Sam Vilain wrote:

> Subject: [PATCH] SubmittingPatches: itemize and reflect upon well written changes
> 
> The SubmittingPatches file was trimmed down from a somewhat
> overwhelming set of requirements from the Linux Kernel equivalent;
> however perhaps a little of it can be returned without making the
> text too long.

This is an improvement, IMHO (and much less verbose than what I wrote to
Andy earlier).

-Peff