{"thread":{"id":"19031","subject":"[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.","startedAt":"2009-04-24T04:13:52Z","lastAt":"2009-04-29T04:09:46Z","messageCount":10,"participants":["andy@petdance.com","Jeff King","Andy Lester","Junio C Hamano","Johannes Schindelin","Sam Vilain"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"112154","messageId":"1240546432-26212-1-git-send-email-andy@petdance.com","threadId":"19031","inReplyTo":null,"subject":"[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.","fromName":"","fromEmail":"andy@petdance.com","sentAt":"2009-04-24T04:13:52Z","receivedAt":"2009-04-24T04:13:52Z","isPatch":true,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"From: Andy Lester <andy@petdance.com>\n\nAdded const to some function parameters.\n---\n builtin-send-pack.c |  190 ++-------------------------------------------------\n remote.c            |    4 +-\n remote.h            |    2 +-\n send-pack.h         |    2 +-\n transport.c         |   18 +++---\n transport.h         |    5 ++\n 6 files changed, 23 insertions(+), 198 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex d5a1c48..5c33f9d 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"remote.h\"\n #include \"send-pack.h\"\n+#include \"transport.h\"\n \n static const char send_pack_usage[] =\n \"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\\n\"\n@@ -29,7 +30,7 @@ static int feed_object(const unsigned char *sha1, int fd, int negative)\n /*\n  * Make a pack stream and spit it out into file descriptor fd\n  */\n-static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extra, struct send_pack_args *args)\n+static int pack_objects(int fd, const struct ref *refs, const struct extra_have_objects *extra, const struct send_pack_args *args)\n {\n \t/*\n \t * The child becomes pack-objects --revs; we feed\n@@ -146,157 +147,7 @@ static int receive_status(int in, struct ref *refs)\n \treturn ret;\n }\n \n-static void update_tracking_ref(struct remote *remote, struct ref *ref)\n-{\n-\tstruct refspec rs;\n-\n-\tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n-\t\treturn;\n-\n-\trs.src = ref->name;\n-\trs.dst = NULL;\n-\n-\tif (!remote_find_tracking(remote, &rs)) {\n-\t\tif (args.verbose)\n-\t\t\tfprintf(stderr, \"updating local tracking ref '%s'\\n\", rs.dst);\n-\t\tif (ref->deletion) {\n-\t\t\tdelete_ref(rs.dst, NULL, 0);\n-\t\t} else\n-\t\t\tupdate_ref(\"update by push\", rs.dst,\n-\t\t\t\t\tref->new_sha1, NULL, 0, 0);\n-\t\tfree(rs.dst);\n-\t}\n-}\n-\n-#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n-\n-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n-{\n-\tfprintf(stderr, \" %c %-*s \", flag, SUMMARY_WIDTH, summary);\n-\tif (from)\n-\t\tfprintf(stderr, \"%s -> %s\", prettify_ref(from), prettify_ref(to));\n-\telse\n-\t\tfputs(prettify_ref(to), stderr);\n-\tif (msg) {\n-\t\tfputs(\" (\", stderr);\n-\t\tfputs(msg, stderr);\n-\t\tfputc(')', stderr);\n-\t}\n-\tfputc('\\n', stderr);\n-}\n-\n-static const char *status_abbrev(unsigned char sha1[20])\n-{\n-\treturn find_unique_abbrev(sha1, DEFAULT_ABBREV);\n-}\n-\n-static void print_ok_ref_status(struct ref *ref)\n-{\n-\tif (ref->deletion)\n-\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n-\telse if (is_null_sha1(ref->old_sha1))\n-\t\tprint_ref_status('*',\n-\t\t\t(!prefixcmp(ref->name, \"refs/tags/\") ? \"[new tag]\" :\n-\t\t\t  \"[new branch]\"),\n-\t\t\tref, ref->peer_ref, NULL);\n-\telse {\n-\t\tchar quickref[84];\n-\t\tchar type;\n-\t\tconst char *msg;\n-\n-\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n-\t\tif (ref->nonfastforward) {\n-\t\t\tstrcat(quickref, \"...\");\n-\t\t\ttype = '+';\n-\t\t\tmsg = \"forced update\";\n-\t\t} else {\n-\t\t\tstrcat(quickref, \"..\");\n-\t\t\ttype = ' ';\n-\t\t\tmsg = NULL;\n-\t\t}\n-\t\tstrcat(quickref, status_abbrev(ref->new_sha1));\n-\n-\t\tprint_ref_status(type, quickref, ref, ref->peer_ref, msg);\n-\t}\n-}\n-\n-static int print_one_push_status(struct ref *ref, const char *dest, int count)\n-{\n-\tif (!count)\n-\t\tfprintf(stderr, \"To %s\\n\", dest);\n-\n-\tswitch(ref->status) {\n-\tcase REF_STATUS_NONE:\n-\t\tprint_ref_status('X', \"[no match]\", ref, NULL, NULL);\n-\t\tbreak;\n-\tcase REF_STATUS_REJECT_NODELETE:\n-\t\tprint_ref_status('!', \"[rejected]\", ref, NULL,\n-\t\t\t\t\"remote does not support deleting refs\");\n-\t\tbreak;\n-\tcase REF_STATUS_UPTODATE:\n-\t\tprint_ref_status('=', \"[up to date]\", ref,\n-\t\t\t\tref->peer_ref, NULL);\n-\t\tbreak;\n-\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n-\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\"non-fast forward\");\n-\t\tbreak;\n-\tcase REF_STATUS_REMOTE_REJECT:\n-\t\tprint_ref_status('!', \"[remote rejected]\", ref,\n-\t\t\t\tref->deletion ? NULL : ref->peer_ref,\n-\t\t\t\tref->remote_status);\n-\t\tbreak;\n-\tcase REF_STATUS_EXPECTING_REPORT:\n-\t\tprint_ref_status('!', \"[remote failure]\", ref,\n-\t\t\t\tref->deletion ? NULL : ref->peer_ref,\n-\t\t\t\t\"remote failed to report status\");\n-\t\tbreak;\n-\tcase REF_STATUS_OK:\n-\t\tprint_ok_ref_status(ref);\n-\t\tbreak;\n-\t}\n-\n-\treturn 1;\n-}\n-\n-static void print_push_status(const char *dest, struct ref *refs)\n-{\n-\tstruct ref *ref;\n-\tint n = 0;\n-\n-\tif (args.verbose) {\n-\t\tfor (ref = refs; ref; ref = ref->next)\n-\t\t\tif (ref->status == REF_STATUS_UPTODATE)\n-\t\t\t\tn += print_one_push_status(ref, dest, n);\n-\t}\n-\n-\tfor (ref = refs; ref; ref = ref->next)\n-\t\tif (ref->status == REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n);\n-\n-\tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (ref->status != REF_STATUS_NONE &&\n-\t\t    ref->status != REF_STATUS_UPTODATE &&\n-\t\t    ref->status != REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n);\n-\t}\n-}\n-\n-static int refs_pushed(struct ref *ref)\n-{\n-\tfor (; ref; ref = ref->next) {\n-\t\tswitch(ref->status) {\n-\t\tcase REF_STATUS_NONE:\n-\t\tcase REF_STATUS_UPTODATE:\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\treturn 0;\n-}\n-\n-int send_pack(struct send_pack_args *args,\n+int send_pack(const struct send_pack_args *args,\n \t      int fd[], struct child_process *conn,\n \t      struct ref *remote_refs,\n \t      struct extra_have_objects *extra_have)\n@@ -426,37 +277,6 @@ int send_pack(struct send_pack_args *args,\n \treturn 0;\n }\n \n-static void verify_remote_names(int nr_heads, const char **heads)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < nr_heads; i++) {\n-\t\tconst char *local = heads[i];\n-\t\tconst char *remote = strrchr(heads[i], ':');\n-\n-\t\tif (*local == '+')\n-\t\t\tlocal++;\n-\n-\t\t/* A matching refspec is okay.  */\n-\t\tif (remote == local && remote[1] == '\\0')\n-\t\t\tcontinue;\n-\n-\t\tremote = remote ? (remote + 1) : local;\n-\t\tswitch (check_ref_format(remote)) {\n-\t\tcase 0: /* ok */\n-\t\tcase CHECK_REF_FORMAT_ONELEVEL:\n-\t\t\t/* ok but a single level -- that is fine for\n-\t\t\t * a match pattern.\n-\t\t\t */\n-\t\tcase CHECK_REF_FORMAT_WILDCARD:\n-\t\t\t/* ok but ends with a pattern-match character */\n-\t\t\tcontinue;\n-\t\t}\n-\t\tdie(\"remote part of refspec is not a valid name in %s\",\n-\t\t    heads[i]);\n-\t}\n-}\n-\n int cmd_send_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, nr_refspecs = 0;\n@@ -576,12 +396,12 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \n \tret |= finish_connect(conn);\n \n-\tprint_push_status(dest, remote_refs);\n+\tprint_push_status(dest, remote_refs, args.verbose);\n \n \tif (!args.dry_run && remote) {\n \t\tstruct ref *ref;\n \t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\tupdate_tracking_ref(remote, ref);\n+\t\t\tupdate_tracking_ref(remote, ref, args.verbose);\n \t}\n \n \tif (!ret && !refs_pushed(remote_refs))\ndiff --git a/remote.c b/remote.c\nindex 91f7485..f7a5c49 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -769,9 +769,9 @@ static int match_name_with_pattern(const char *key, const char *name,\n \treturn ret;\n }\n \n-int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n+int remote_find_tracking(const struct remote *remote, struct refspec *refspec)\n {\n-\tint find_src = refspec->src == NULL;\n+\tconst int find_src = (refspec->src == NULL);\n \tchar *needle, **result;\n \tint i;\n \ndiff --git a/remote.h b/remote.h\nindex 99706a8..d624e08 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -108,7 +108,7 @@ struct ref *get_remote_ref(const struct ref *remote_refs, const char *name);\n /*\n  * For the given remote, reads the refspec's src and sets the other fields.\n  */\n-int remote_find_tracking(struct remote *remote, struct refspec *refspec);\n+int remote_find_tracking(const struct remote *remote, struct refspec *refspec);\n \n struct branch {\n \tconst char *name;\ndiff --git a/send-pack.h b/send-pack.h\nindex 83d76c7..88a407e 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -9,7 +9,7 @@ struct send_pack_args {\n \t\tdry_run:1;\n };\n \n-int send_pack(struct send_pack_args *args,\n+int send_pack(const struct send_pack_args *args,\n \t      int fd[], struct child_process *conn,\n \t      struct ref *remote_refs, struct extra_have_objects *extra_have);\n \ndiff --git a/transport.c b/transport.c\nindex 3dfb03c..d50160b 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -690,7 +690,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \treturn (refs ? 0 : -1);\n }\n \n-static int refs_pushed(struct ref *ref)\n+int refs_pushed(const struct ref *ref)\n {\n \tfor (; ref; ref = ref->next) {\n \t\tswitch(ref->status) {\n@@ -704,7 +704,7 @@ static int refs_pushed(struct ref *ref)\n \treturn 0;\n }\n \n-static void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)\n+void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose)\n {\n \tstruct refspec rs;\n \n@@ -728,7 +728,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref, int verb\n \n #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n \n-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n+static void print_ref_status(char flag, const char *summary, const struct ref *to, const struct ref *from, const char *msg)\n {\n \tfprintf(stderr, \" %c %-*s \", flag, SUMMARY_WIDTH, summary);\n \tif (from)\n@@ -743,12 +743,12 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str\n \tfputc('\\n', stderr);\n }\n \n-static const char *status_abbrev(unsigned char sha1[20])\n+static const char *status_abbrev(const unsigned char *sha1)\n {\n \treturn find_unique_abbrev(sha1, DEFAULT_ABBREV);\n }\n \n-static void print_ok_ref_status(struct ref *ref)\n+static void print_ok_ref_status(const struct ref *ref)\n {\n \tif (ref->deletion)\n \t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n@@ -778,7 +778,7 @@ static void print_ok_ref_status(struct ref *ref)\n \t}\n }\n \n-static int print_one_push_status(struct ref *ref, const char *dest, int count)\n+static int print_one_push_status(const struct ref *ref, const char *dest, int count)\n {\n \tif (!count)\n \t\tfprintf(stderr, \"To %s\\n\", dest);\n@@ -817,9 +817,9 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)\n \treturn 1;\n }\n \n-static void print_push_status(const char *dest, struct ref *refs, int verbose)\n+void print_push_status(const char *dest, const struct ref *refs, int verbose)\n {\n-\tstruct ref *ref;\n+\tconst struct ref *ref;\n \tint n = 0;\n \n \tif (verbose) {\n@@ -840,7 +840,7 @@ static void print_push_status(const char *dest, struct ref *refs, int verbose)\n \t}\n }\n \n-static void verify_remote_names(int nr_heads, const char **heads)\n+void verify_remote_names(int nr_heads, const char **heads)\n {\n \tint i;\n \ndiff --git a/transport.h b/transport.h\nindex b1c2252..ea77c7c 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -75,4 +75,9 @@ int transport_fetch_refs(struct transport *transport, const struct ref *refs);\n void transport_unlock_pack(struct transport *transport);\n int transport_disconnect(struct transport *transport);\n \n+void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose);\n+void print_push_status(const char *dest, const struct ref *refs, int verbose);\n+int refs_pushed(const struct ref *ref);\n+void verify_remote_names(int nr_heads, const char **heads);\n+\n #endif\n-- \n1.6.2.4\n"},{"id":"112234","messageId":"20090424210418.GC13561@coredump.intra.peff.net","threadId":"19031","inReplyTo":"1240546432-26212-1-git-send-email-andy@petdance.com","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.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-24T21:04:18Z","receivedAt":"2009-04-24T21:04:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Great, I think this is something that needs to be done, and it is always\nnice to see a diffstat like:\n\n>  6 files changed, 23 insertions(+), 198 deletions(-)\n\nthat shows massive cleanup. But your patch is a little hard to review,\nso let me try to constructively critique your commit message for a\nmoment.\n\nFirst off, it really seems like there are two things happening here:\nremoving the functions mentioned in the subject, and:\n\n> Added const to some function parameters.\n\nI don't think those are related, so it makes sense to split them into\ntwo patches in a series. This has a few advantages:\n\n  - when reviewers read the patch, they know to which topic each change\n    belongs (and yes, we can figure it out by reading the change\n    carefully, but it is a lot easier when you start reading a diff to\n    say \"OK, I know approximately what this is going to do from the\n    commit message\" and then confirm that it does what you thought)\n\n  - if one of the topics is controversial but the other is not, the\n    non-controversial changes are not held hostage while the\n    controversial ones are discussed or re-done\n\nMoving on to the message itself, it is very top-heavy:\n\n> Subject: Re: [PATCH] Removed redundant static functions such as\n>\tupdate_tracking_ref() and verify_remote_names() from\n>\tbuiltin-send-pack.c, and made the ones in transport.c not be static\n>\tso they can be used instead.\n\nThe first line of the message is really supposed to be a one-liner, like\nthe subject of an email, to give people a general sense of what is going\non. Then you can go into more detail in a follow-on paragraph. That\nmakes things like \"gitk\" and \"git log --oneline\" more useful.\n\nAnd as a grammatical nit, in git itself we usually use the imperative\nmood in commit mesages. So \"remove\" and \"add\" instead of \"removed and\nadded\".\n\nAs far as the details itself, usually you want to talk about _why_ to\nmake this change. In this case, removing redundant code is a pretty\nobvious reason, but I am left wondering another why: why is this OK to\ndo? In other words, where did the duplication come from, why was it\nduplicated instead of refactored in the first place (simple oversight,\nor some assumption that was true then, etc), and why are things\ndifferent now (correcting an oversight, that assumption no longer holds,\netc). From our prior discussion, the code came from 64fcef2. But I'm not\nsure if the duplicated code is completely identical. I.e., was it\ntweaked when it was copied to transport.c? If not, then say so, because\nthat is a question every reviewer should have. If so, then why is it OK\nfor send-pack to start using the tweaked version?\n\nI have some guesses about the answers to those from our prior\ndiscussions. But part of making the patch would be looking into those\nthings. And keep in mind that Junio probably didn't read our prior\ndiscussion, nor will somebody reading the commit message two years from\nnow.\n\nSo I think the commmit message you want would be something like:\n\n  remove duplicate functions from builtin-send-pack.c\n\n  These functions are helpers for handling tracking refs, printing\n  output, etc. They were originally used only by send-pack, but commit\n  64fcef2 copied them to transport.c so that they could be used by all\n  transports.\n\n  As the versions in transport.c and builtin-send-pack.c are identical\n  [or whatever you find out when you investigate], there is no reason \n  for there to be two copies. Copying instead of moving in 64fcef2\n  appears to have simply been an oversight [even better, get\n  confirmation from Daniel on why he did it that way].\n\n  This patch just removes the versions in builtin-send-pack.c, and\n  makes the ones in transport.c available as library functions.\n\nAs for the patch itself, there are a few spots I noticed in my cursory\nlook:\n\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -769,9 +769,9 @@ static int match_name_with_pattern(const char *key, const char *name,\n> [...]\n> +int remote_find_tracking(const struct remote *remote, struct refspec *refspec)\n>  {\n> -\tint find_src = refspec->src == NULL;\n> +\tconst int find_src = (refspec->src == NULL);\n\nI don't think we usually worry about const-ing local variables like\nthis, but instead just focus on const-ing parameters. The compiler can\ngenerally already detect constness of find_src here, because it can see\nall of the places it is used (whereas crossing a function boundary,\nanything can happen to a non-const parameter).\n\n> -static const char *status_abbrev(unsigned char sha1[20])\n> +static const char *status_abbrev(const unsigned char *sha1)\n\nIs there a good reason to drop this from an array to a pointer?\n\n> +void update_tracking_ref(const struct remote *remote, struct ref *ref, int verbose);\n> +void print_push_status(const char *dest, const struct ref *refs, int verbose);\n> +int refs_pushed(const struct ref *ref);\n> +void verify_remote_names(int nr_heads, const char **heads);\n\nThis might need to be given more descriptive names if they will have\nglobal linkage.\n\nI do wonder, though...if http and other transports are using these via\ntransport.c, then why is the git transport not doing the same thing? In\nother words, should they actually be statics, and the calls ripped out\nof send_pack()? Or send_pack() just moved into transport.c?\n\n-Peff\n"},{"id":"112236","messageId":"99B4BF12-01B9-4A68-B2E0-EF5DF2595FF0@petdance.com","threadId":"19031","inReplyTo":"20090424210418.GC13561@coredump.intra.peff.net","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.","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2009-04-24T21:13:14Z","receivedAt":"2009-04-24T21:13:14Z","isPatch":true,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"\nOn Apr 24, 2009, at 4:04 PM, Jeff King wrote:\n\n> in git itself we usually use the imperative\n> mood in commit mesages.\n\n\nBoy, you guys are hardcore. :-)\n\nThis was what I was looking for.  I think what I'll do is fold your  \nmessage into Documentation/SubmittingPatches and submit that as a  \npatch first.\n\nThanks,\nxoxo,\nAndy\n\n--\nAndy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance\n"},{"id":"112237","messageId":"20090424212313.GA14435@coredump.intra.peff.net","threadId":"19031","inReplyTo":"99B4BF12-01B9-4A68-B2E0-EF5DF2595FF0@petdance.com","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.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-24T21:23:14Z","receivedAt":"2009-04-24T21:23:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2009 at 04:13:14PM -0500, Andy Lester wrote:\n\n> This was what I was looking for.  I think what I'll do is fold your  \n> message into Documentation/SubmittingPatches and submit that as a patch \n> first.\n\nThat probably makes sense.\n\nI keep thinking about writing a separate \"how to write a good commit\nmessage\" document that would be more universal than just \"here's how you\nsubmit a patch to git\". And some of what I wrote to you could probably\ngo in such a document. But I don't know if it makes sense to start a new\ndocument just with what I said there; it might be a bit sparse (OTOH,\nmaybe people would then be encouraged to add their tips to it).\n\n-Peff\n"},{"id":"112252","messageId":"7vfxfxad3h.fsf@gitster.siamese.dyndns.org","threadId":"19031","inReplyTo":"20090424212313.GA14435@coredump.intra.peff.net","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.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-24T22:41:54Z","receivedAt":"2009-04-24T22:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 24, 2009 at 04:13:14PM -0500, Andy Lester wrote:\n>\n>> This was what I was looking for.  I think what I'll do is fold your  \n>> message into Documentation/SubmittingPatches and submit that as a patch \n>> first.\n>\n> That probably makes sense.\n>\n> I keep thinking about writing a separate \"how to write a good commit\n> message\" document that would be more universal than just \"here's how you\n> submit a patch to git\". And some of what I wrote to you could probably\n> go in such a document. But I don't know if it makes sense to start a new\n> document just with what I said there; it might be a bit sparse (OTOH,\n> maybe people would then be encouraged to add their tips to it).\n\nI'm sure our capable project secretary would come up with list of quotes\nin the archive from Linus and I perhaps over the weekend in her copious\nspare time ;-).\n"},{"id":"112272","messageId":"alpine.DEB.1.00.0904250206250.10279@pacific.mpi-cbg.de","threadId":"19031","inReplyTo":"99B4BF12-01B9-4A68-B2E0-EF5DF2595FF0@petdance.com","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.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-25T00:07:46Z","receivedAt":"2009-04-25T00:07:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 24 Apr 2009, Andy Lester wrote:\n\n> On Apr 24, 2009, at 4:04 PM, Jeff King wrote:\n> \n> >in git itself we usually use the imperative mood in commit mesages.\n> \n> This was what I was looking for.  I think what I'll do is fold your \n> message into Documentation/SubmittingPatches and submit that as a patch \n> first.\n\nI dunno.  The most important part of CodingGuidelines is this:\n\n\tAs for more concrete guidelines, just imitate the existing code\n\t(this is a good guideline, no matter which project you are\n\tcontributing to).\n\n(And of course, this holds for the style of commit messages, too.)\n\nCiao,\nDscho\n"},{"id":"112281","messageId":"4B2541E8-7A27-45D5-B77D-AE93C0430EA8@petdance.com","threadId":"19031","inReplyTo":"alpine.DEB.1.00.0904250206250.10279@pacific.mpi-cbg.de","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.","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2009-04-25T04:15:07Z","receivedAt":"2009-04-25T04:15:07Z","isPatch":true,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"\nOn Apr 24, 2009, at 7:07 PM, Johannes Schindelin wrote:\n\n> I dunno.  The most important part of CodingGuidelines is this:\n>\n> \tAs for more concrete guidelines, just imitate the existing code\n> \t(this is a good guideline, no matter which project you are\n> \tcontributing to).\n>\n> (And of course, this holds for the style of commit messages, too.)\n\n\nWould you rather I not bother?  Far be it from me to try to force  \nmyself on any project.\n\n--\nAndy Lester => andy@petdance.com => www.petdance.com => AIM:petdance\n"},{"id":"112286","messageId":"alpine.DEB.1.00.0904251115550.10279@pacific.mpi-cbg.de","threadId":"19031","inReplyTo":"4B2541E8-7A27-45D5-B77D-AE93C0430EA8@petdance.com","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.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-25T09:18:33Z","receivedAt":"2009-04-25T09:18:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 24 Apr 2009, Andy Lester wrote:\n\n> On Apr 24, 2009, at 7:07 PM, Johannes Schindelin wrote:\n> \n> >I dunno.  The most important part of CodingGuidelines is this:\n> >\n> > As for more concrete guidelines, just imitate the existing code\n> > (this is a good guideline, no matter which project you are\n> > contributing to).\n> >\n> >(And of course, this holds for the style of commit messages, too.)\n> \n> \n> Would you rather I not bother?  Far be it from me to try to force myself on\n> any project.\n\nSorry, Andy, I forgot to add the\n\nDisclaimer: if you are offended by constructive criticism, or likely to\nanswer with insults to the comments I offer, please stop reading this mail\nnow (and please do not answer my mail, either). :-)\n\nStill with me?  Good.  Nice to meet you.\n\nJust for the record: responding to a patch is my strongest way of saying\nthat I appreciate your work.\n\nThe thing with SubmittingPatches is: I think it is already too long for \npeople to quickly read and get stuff done.\n\nBut hey, I was wrong before, and I will be wrong again.  That's why I \noffered my opinion, and I _can_ be convinced of another opinion.\n\nCiao,\nDscho\n"},{"id":"112419","messageId":"49F5C377.9010200@vilain.net","threadId":"19031","inReplyTo":"4B2541E8-7A27-45D5-B77D-AE93C0430EA8@petdance.com","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.","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2009-04-27T14:38:47Z","receivedAt":"2009-04-27T14:38:47Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"Andy Lester wrote:\n> > I dunno.  The most important part of CodingGuidelines is this:\n> >     As for more concrete guidelines, just imitate the existing code\n> >     (this is a good guideline, no matter which project you are\n> >     contributing to).\n> > (And of course, this holds for the style of commit messages, too.)\n> Would you rather I not bother?  Far be it from me to try to force\n> myself on any project.\n\nBother?  What bother?  Do you think we're kidding? :-)\n\nSubject: [PATCH] SubmittingPatches: itemize and reflect upon well written changes\n\nThe SubmittingPatches file was trimmed down from a somewhat\noverwhelming set of requirements from the Linux Kernel equivalent;\nhowever perhaps a little of it can be returned without making the\ntext too long.\n\nSigned-off-by: Sam Vilain <sam@vilain.net>\n---\n <insert funny meta-circular joke here>\n\n Documentation/SubmittingPatches |   14 +++++++++++++-\n 1 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 8d818a2..76fc84d 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -6,9 +6,13 @@ Checklist (and a short version for the impatient):\n \t- check for unnecessary whitespace with \"git diff --check\"\n \t  before committing\n \t- do not check in commented out code or unneeded files\n-\t- provide a meaningful commit message\n \t- the first line of the commit message should be a short\n \t  description and should skip the full stop\n+\t- the body should provide a meaningful commit message, which:\n+\t\t- uses the imperative, present tense: \"change\",\n+\t\t  not \"changed\" or \"changes\".\n+\t\t- includes motivation for the change, and contrasts\n+\t\t  its implementation with previous behaviour\n \t- if you want your work included in git.git, add a\n \t  \"Signed-off-by: Your Name <you@example.com>\" line to the\n \t  commit message (or just use the option \"-s\" when\n@@ -62,6 +66,14 @@ Describe the technical detail of the change(s).\n \n If your description starts to get too long, that's a sign that you\n probably need to split up your commit to finer grained pieces.\n+That being said, patches which plainly describe the things that\n+help reviewers check the patch, and future maintainers understand\n+the code, are the most beautiful patches.  Descriptions that summarise\n+the point in the subject well, and describe the motivation for the\n+change, the approach taken by the change, and if relevant how this\n+differs substantially from the prior version, can be found on Usenet\n+archives back into the late 80's.  Consider it like good Netiquette,\n+but for code.\n \n Oh, another thing.  I am picky about whitespaces.  Make sure your\n changes do not trigger errors with the sample pre-commit hook shipped\n-- \n1.6.2.234.g28eec\n"},{"id":"112591","messageId":"20090429040946.GC14912@coredump.intra.peff.net","threadId":"19031","inReplyTo":"49F5C377.9010200@vilain.net","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.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-29T04:09:46Z","receivedAt":"2009-04-29T04:09:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2009 at 02:38:47AM +1200, Sam Vilain wrote:\n\n> Subject: [PATCH] SubmittingPatches: itemize and reflect upon well written changes\n> \n> The SubmittingPatches file was trimmed down from a somewhat\n> overwhelming set of requirements from the Linux Kernel equivalent;\n> however perhaps a little of it can be returned without making the\n> text too long.\n\nThis is an improvement, IMHO (and much less verbose than what I wrote to\nAndy earlier).\n\n-Peff\n"}]}