{"thread":{"id":"42881","subject":"[PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","startedAt":"2016-07-19T19:06:08Z","lastAt":"2016-08-17T17:46:17Z","messageCount":23,"participants":["René Scharfe","Junio C Hamano","Stefan Beller","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291754","messageId":"578E7A07.8080006@web.de","threadId":"42881","inReplyTo":null,"subject":"[PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-19T19:05:43Z","receivedAt":"2016-07-19T19:06:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use a string constant instead of an empty strbuf to shorten the code\nand make it easier to read.\n\nSigned-off-by: Rene Scharfe \n---\n... unless someone can come up with a suitable non-empty string to feed\nto git_config_from_mem() as its name parameter.\n\n submodule-config.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex db1847f..44eb162 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -397,7 +397,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tconst unsigned char *commit_sha1, const char *key,\n \t\tenum lookup_type lookup_type)\n {\n-\tstruct strbuf rev = STRBUF_INIT;\n \tunsigned long config_size;\n \tchar *config;\n \tunsigned char sha1[20];\n@@ -448,7 +447,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.commit_sha1 = commit_sha1;\n \tparameter.gitmodules_sha1 = sha1;\n \tparameter.overwrite = 0;\n-\tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n+\tgit_config_from_mem(parse_config, \"submodule-blob\", \"\",\n \t\t\tconfig, config_size, ¶meter);\n \tfree(config);\n \n-- \n2.9.2\n\n"},{"id":"291757","messageId":"xmqqd1m9fnhj.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"578E7A07.8080006@web.de","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T19:15:36Z","receivedAt":"2016-07-19T19:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Use a string constant instead of an empty strbuf to shorten the code\n> and make it easier to read.\n>\n> Signed-off-by: Rene Scharfe \n> ---\n> ... unless someone can come up with a suitable non-empty string to feed\n> to git_config_from_mem() as its name parameter.\n>\n> submodule-config.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index db1847f..44eb162 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -397,7 +397,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \t\tconst unsigned char *commit_sha1, const char *key,\n> \t\tenum lookup_type lookup_type)\n> {\n> -\tstruct strbuf rev = STRBUF_INIT;\n> \tunsigned long config_size;\n> \tchar *config;\n> \tunsigned char sha1[20];\n> @@ -448,7 +447,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \tparameter.commit_sha1 = commit_sha1;\n> \tparameter.gitmodules_sha1 = sha1;\n> \tparameter.overwrite = 0;\n> -\tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n> +\tgit_config_from_mem(parse_config, \"submodule-blob\", \"\",\n> \t\t\tconfig, config_size, ¶meter);\n> \tfree(config);\n\nOh.\n\nI wondered if we used to do something useful with rev and later that\nuseful thing was removed leaving an always-empty strbuf, but it\nappears that this strbuf was introduced to always hold an empty\nstring and nothing else, which shows the (lack of) quality of\nreviews in this area X-<.\n\nWill apply. Thanks.\n"},{"id":"291761","messageId":"CAGZ79kYAH2GtpmXfUrFd5s-g=-=n0CPSa7EKFvABtVz5GXX-aw@mail.gmail.com","threadId":"42881","inReplyTo":"xmqqd1m9fnhj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-19T19:24:36Z","receivedAt":"2016-07-19T19:24:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 19, 2016 at 12:15 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n>\n>> Use a string constant instead of an empty strbuf to shorten the code\n>> and make it easier to read.\n>>\n>> Signed-off-by: Rene Scharfe \n>> ---\n>> ... unless someone can come up with a suitable non-empty string to feed\n>> to git_config_from_mem() as its name parameter.\n\nThanks!\n\n> I wondered if we used to do something useful with rev and later that\n> useful thing was removed leaving an always-empty strbuf, but it\n> appears that this strbuf was introduced to always hold an empty\n> string and nothing else, which shows the (lack of) quality of\n> reviews in this area X-<.\n\nI am to blame for this. :(\n\n>\n> Will apply. Thanks.\n"},{"id":"291792","messageId":"20160720082515.GA823@book.hvoigt.net","threadId":"42881","inReplyTo":"578E7A07.8080006@web.de","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-20T08:25:15Z","receivedAt":"2016-07-20T08:31:55Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Tue, Jul 19, 2016 at 09:05:43PM +0200, René Scharfe wrote:\n> Use a string constant instead of an empty strbuf to shorten the code\n> and make it easier to read.\n\nThis must have been some oversight from my original code. I also can not\nsee any purpose.\n\n> Signed-off-by: Rene Scharfe \n> ---\n> ... unless someone can come up with a suitable non-empty string to feed\n> to git_config_from_mem() as its name parameter.\n\nIf we would want to be absolutely correct we could use something like\n\"SHA1:.gitmodules\". E.g. like we use to lookup the blob in\ngitmodule_sha1_from_commit():\n\n\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n\nAnd now I see where this was leftover from... before extracting this\nfunction this code was filling the strbuf.\n\nHow about this instead?\n\n---8<------\nSubject: [PATCH] fix passing a name for config from submodules\n\nIn commit 959b5455 we implemented the initial version of the submodule\nconfig cache. During development of that initial version we extracted\nthe function gitmodule_sha1_from_commit(). During that process we missed\nthat the strbuf rev was still used in config_from() and now is left\nempty. Lets fix this by also returning this string.\n\nSigned-off-by: Heiko Voigt \n---\n\nIts not exactly pretty with all the releases before the returns but\nthis is what I could quickly come up with...\n\n submodule-config.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 077db40..dccea59 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -371,9 +371,9 @@ static int parse_config(const char *var, const char *value, void *data)\n }\n \n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n-\t\t\t\t unsigned char *gitmodules_sha1)\n+\t\t\t\t unsigned char *gitmodules_sha1,\n+\t\t\t\t struct strbuf *rev)\n {\n-\tstruct strbuf rev = STRBUF_INIT;\n \tint ret = 0;\n \n \tif (is_null_sha1(commit_sha1)) {\n@@ -381,11 +381,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\treturn 1;\n \t}\n \n-\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n-\tif (get_sha1(rev.buf, gitmodules_sha1) >= 0)\n+\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tif (get_sha1(rev->buf, gitmodules_sha1) >= 0)\n \t\tret = 1;\n \n-\tstrbuf_release(&rev);\n \treturn ret;\n }\n \n@@ -420,8 +419,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\treturn entry->config;\n \t}\n \n-\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n+\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n@@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n \t\tbreak;\n \t}\n-\tif (submodule)\n+\tif (submodule) {\n+\t\tstrbuf_release(&rev);\n \t\treturn submodule;\n+\t}\n \n \tconfig = read_sha1_file(sha1, &type, &config_size);\n-\tif (!config)\n+\tif (!config) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tif (type != OBJ_BLOB) {\n+\t\tstrbuf_release(&rev);\n \t\tfree(config);\n \t\treturn NULL;\n \t}\n@@ -450,6 +456,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.overwrite = 0;\n \tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n \t\t\tconfig, config_size, ¶meter);\n+\tstrbuf_release(&rev);\n \tfree(config);\n \n \tswitch (lookup_type) {\n-- \n2.4.2.387.gf86f31a\n\n"},{"id":"291930","messageId":"57911AFF.7030107@web.de","threadId":"42881","inReplyTo":"20160720082515.GA823@book.hvoigt.net","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-21T18:57:03Z","receivedAt":"2016-07-21T18:57:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.07.2016 um 10:25 schrieb Heiko Voigt:\n> Hi,\n>\n> On Tue, Jul 19, 2016 at 09:05:43PM +0200, René Scharfe wrote:\n>> Use a string constant instead of an empty strbuf to shorten the code\n>> and make it easier to read.\n>\n> This must have been some oversight from my original code. I also can not\n> see any purpose.\n>\n>> Signed-off-by: Rene Scharfe \n>> ---\n>> ... unless someone can come up with a suitable non-empty string to feed\n>> to git_config_from_mem() as its name parameter.\n>\n> If we would want to be absolutely correct we could use something like\n> \"SHA1:.gitmodules\". E.g. like we use to lookup the blob in\n> gitmodule_sha1_from_commit():\n>\n> \tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n>\n> And now I see where this was leftover from... before extracting this\n> function this code was filling the strbuf.\n>\n> How about this instead?\n\nI like it.\n\n> ---8<------\n> Subject: [PATCH] fix passing a name for config from submodules\n>\n> In commit 959b5455 we implemented the initial version of the submodule\n> config cache. During development of that initial version we extracted\n> the function gitmodule_sha1_from_commit(). During that process we missed\n> that the strbuf rev was still used in config_from() and now is left\n> empty. Lets fix this by also returning this string.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n>\n> Its not exactly pretty with all the releases before the returns but\n> this is what I could quickly come up with...\n\nIndeed.\n\n> submodule-config.c | 23 +++++++++++++++--------\n> 1 file changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 077db40..dccea59 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -371,9 +371,9 @@ static int parse_config(const char *var, const char *value, void *data)\n> }\n>\n> static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n> -\t\t\t\t unsigned char *gitmodules_sha1)\n> +\t\t\t\t unsigned char *gitmodules_sha1,\n> +\t\t\t\t struct strbuf *rev)\n> {\n> -\tstruct strbuf rev = STRBUF_INIT;\n> \tint ret = 0;\n>\n> \tif (is_null_sha1(commit_sha1)) {\n> @@ -381,11 +381,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n> \t\treturn 1;\n> \t}\n>\n> -\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n> -\tif (get_sha1(rev.buf, gitmodules_sha1) >= 0)\n> +\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n> +\tif (get_sha1(rev->buf, gitmodules_sha1) >= 0)\n> \t\tret = 1;\n>\n> -\tstrbuf_release(&rev);\n> \treturn ret;\n> }\n>\n> @@ -420,8 +419,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \t\treturn entry->config;\n> \t}\n>\n> -\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n> +\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n> +\t\tstrbuf_release(&rev);\n> \t\treturn NULL;\n> +\t}\n>\n> \tswitch (lookup_type) {\n> \tcase lookup_name:\n> @@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n> \t\tbreak;\n> \t}\n> -\tif (submodule)\n> +\tif (submodule) {\n> +\t\tstrbuf_release(&rev);\n> \t\treturn submodule;\n> +\t}\n>\n> \tconfig = read_sha1_file(sha1, &type, &config_size);\n> -\tif (!config)\n> +\tif (!config) {\n> +\t\tstrbuf_release(&rev);\n> \t\treturn NULL;\n> +\t}\n>\n> \tif (type != OBJ_BLOB) {\n> +\t\tstrbuf_release(&rev);\n> \t\tfree(config);\n> \t\treturn NULL;\n> \t}\n\nA separate patch could combine the previous two conditionals; free(NULL) \nis allowed.\n\n> @@ -450,6 +456,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> \tparameter.overwrite = 0;\n> \tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n> \t\t\tconfig, config_size, ¶meter);\n> +\tstrbuf_release(&rev);\n> \tfree(config);\n>\n> \tswitch (lookup_type) {\n>\n\n"},{"id":"292092","messageId":"20160725143706.GA4872@sandbox","threadId":"42881","inReplyTo":"57911AFF.7030107@web.de","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-25T14:37:07Z","receivedAt":"2016-07-25T14:37:20Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Thu, Jul 21, 2016 at 08:57:03PM +0200, René Scharfe wrote:\n> >diff --git a/submodule-config.c b/submodule-config.c\n> >index 077db40..dccea59 100644\n> >--- a/submodule-config.c\n> >+++ b/submodule-config.c\n\n[...]\n\n> >@@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> > \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n> > \t\tbreak;\n> > \t}\n> >-\tif (submodule)\n> >+\tif (submodule) {\n> >+\t\tstrbuf_release(&rev);\n> > \t\treturn submodule;\n> >+\t}\n> >\n> > \tconfig = read_sha1_file(sha1, &type, &config_size);\n> >-\tif (!config)\n> >+\tif (!config) {\n> >+\t\tstrbuf_release(&rev);\n> > \t\treturn NULL;\n> >+\t}\n> >\n> > \tif (type != OBJ_BLOB) {\n> >+\t\tstrbuf_release(&rev);\n> > \t\tfree(config);\n> > \t\treturn NULL;\n> > \t}\n> \n> A separate patch could combine the previous two conditionals; free(NULL) is\n> allowed.\n\nThats a good idea. Can send a patch, but I noticed that your change is\nalready in next and my change has not been picked up. So I am unsure if\nI should base my patches on next or if Junio wants to keep your change?\n\nCheers Heiko\n"},{"id":"292093","messageId":"xmqq60rt933m.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"20160725143706.GA4872@sandbox","subject":"Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-25T14:58:21Z","receivedAt":"2016-07-25T14:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> Thats a good idea. Can send a patch, but I noticed that your change is\n> already in next and my change has not been picked up. So I am unsure if\n> I should base my patches on next or if Junio wants to keep your change?\n\nWhen you noticed something you want to touch up in 'next' but not in\n'master', please try to find which topic the target of your\ntouching-up comes from.\n\n\tgit log --first-parent master..next\n\nI often use --oneline for , because I am reasonably\nfamiliar with the topics in 'next', but people may start from \"git\nblame\" output, finding the commit that left the text you are\ntouching up, then where it was merged to 'next'. That would give\nyou\n\ncommit 59dbd5834007c1e4dff25770da095babcb69a009\nMerge: 679f992 508a285\nAuthor: Junio C Hamano \nDate: Tue Jul 19 13:43:44 2016 -0700\n\n Merge branch 'rs/submodule-config-code-cleanup' into next\n \n Code cleanup.\n \n * rs/submodule-config-code-cleanup:\n submodule-config: use explicit empty string instead of strbuf in config_from()\n\nThe second parent 508a285c (submodule-config: use explicit empty\nstring instead of strbuf in config_from(), 2016-07-19) is a good\nplace to base your patch.\n\nThanks.\n\n\n\n"},{"id":"292187","messageId":"20160726094925.GB3347@book.hvoigt.net","threadId":"42881","inReplyTo":"xmqq60rt933m.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 2/2] submodule-config: combine error checking if clauses","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-26T09:49:25Z","receivedAt":"2016-07-26T09:49:35Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"So we have less return handling code.\n\nSigned-off-by: Heiko Voigt \n---\n submodule-config.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 853989e..cb9bf8f 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -416,12 +416,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t}\n \n \tconfig = read_sha1_file(sha1, &type, &config_size);\n-\tif (!config) {\n-\t\tstrbuf_release(&rev);\n-\t\treturn NULL;\n-\t}\n-\n-\tif (type != OBJ_BLOB) {\n+\tif (!config || type != OBJ_BLOB) {\n \t\tstrbuf_release(&rev);\n \t\tfree(config);\n \t\treturn NULL;\n-- \n2.4.2.387.gf86f31a\n\n"},{"id":"292188","messageId":"20160726094913.GA3347@book.hvoigt.net","threadId":"42881","inReplyTo":"xmqq60rt933m.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/2] fix passing a name for config from submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-26T09:49:13Z","receivedAt":"2016-07-26T09:51:11Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"In commit 959b5455 we implemented the initial version of the submodule\nconfig cache. During development of that initial version we extracted\nthe function gitmodule_sha1_from_commit(). During that process we missed\nthat the strbuf rev was still used in config_from() and now is left\nempty. Lets fix this by also returning this string.\n\nSigned-off-by: Heiko Voigt \n---\nThis is based on 508a285 in next.\n\n submodule-config.c | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 1210b26..853989e 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -349,9 +349,9 @@ static int parse_config(const char *var, const char *value, void *data)\n }\n \n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n-\t\t\t\t unsigned char *gitmodules_sha1)\n+\t\t\t\t unsigned char *gitmodules_sha1,\n+\t\t\t\t struct strbuf *rev)\n {\n-\tstruct strbuf rev = STRBUF_INIT;\n \tint ret = 0;\n \n \tif (is_null_sha1(commit_sha1)) {\n@@ -359,11 +359,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\treturn 1;\n \t}\n \n-\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n-\tif (get_sha1(rev.buf, gitmodules_sha1) >= 0)\n+\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tif (get_sha1(rev->buf, gitmodules_sha1) >= 0)\n \t\tret = 1;\n \n-\tstrbuf_release(&rev);\n \treturn ret;\n }\n \n@@ -375,6 +374,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tconst unsigned char *commit_sha1, const char *key,\n \t\tenum lookup_type lookup_type)\n {\n+\tstruct strbuf rev = STRBUF_INIT;\n \tunsigned long config_size;\n \tchar *config;\n \tunsigned char sha1[20];\n@@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\treturn entry->config;\n \t}\n \n-\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n+\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n@@ -408,14 +410,19 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n \t\tbreak;\n \t}\n-\tif (submodule)\n+\tif (submodule) {\n+\t\tstrbuf_release(&rev);\n \t\treturn submodule;\n+\t}\n \n \tconfig = read_sha1_file(sha1, &type, &config_size);\n-\tif (!config)\n+\tif (!config) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tif (type != OBJ_BLOB) {\n+\t\tstrbuf_release(&rev);\n \t\tfree(config);\n \t\treturn NULL;\n \t}\n@@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.commit_sha1 = commit_sha1;\n \tparameter.gitmodules_sha1 = sha1;\n \tparameter.overwrite = 0;\n-\tgit_config_from_mem(parse_config, \"submodule-blob\", \"\",\n+\tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n \t\t\tconfig, config_size, ¶meter);\n+\tstrbuf_release(&rev);\n \tfree(config);\n \n \tswitch (lookup_type) {\n-- \n2.4.2.387.gf86f31a\n\n"},{"id":"292240","messageId":"CAGZ79kaOf3NRAXh+krM=onwswSjAF3yy_zpa1d+9CFOBNke6-w@mail.gmail.com","threadId":"42881","inReplyTo":"20160726094913.GA3347@book.hvoigt.net","subject":"Re: [PATCH 1/2] fix passing a name for config from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-26T17:22:07Z","receivedAt":"2016-07-26T17:22:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt wrote:\n\nThanks for continuing on the submodule cache!\n\n> In commit 959b5455 we implemented the initial version of the submodule\n\nUsually we refer to the commit by a triple of \"abbrev. sha1 (date, subject).\nSee d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free)\nfor an example. Or ce41720ca (2015-04-02, blame, log: format usage strings\nsimilarly to those in documentation).\n\nApparently we put the subject first and then the date. I always did it\nthe other way\nround, to there is no strict coding guide line, though it helps a lot to have an\nunderstanding for a) how long are we in the \"broken\" state already as well as\nb) what was the rationale for introducing it.\n\n\n\n> @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> return entry->config;\n> }\n>\n> - if (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n> + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n> + strbuf_release(&rev);\n> return NULL;\n\nThis is a reoccuring pattern below. Maybe it might make sense to\njust do a s/return.../ goto out/ and at that label we cleanup `rev` and `config`\nand return a result value?\nThere are currently 6 early returns (not counting the 3 from the last switch),\n4 of them return NULL, so that would result in just a \"goto out\", whereas 2\nreturn an actual value, they would need to assign the result value first before\njumping out of the logic. I dunno, just food for though.\n\n\n\n> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> parameter.commit_sha1 = commit_sha1;\n> parameter.gitmodules_sha1 = sha1;\n> parameter.overwrite = 0;\n> - git_config_from_mem(parse_config, \"submodule-blob\", \"\",\n> + git_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n> config, config_size, ¶meter);\n\nOk, this is the actual fix. Do you want to demonstrate its impact by adding\none or two tests that failed before and now work?\n(As I was using the submodule config API most of the time with null_sha1\nto indicate we'd be looking at the current .gitmodules file in the worktree,\nthe actual bug may have not manifested in the users of this API.\nBut still, it would be nice to see what was broken?)\n\nThanks,\nStefan\n"},{"id":"292241","messageId":"CAGZ79kZ4YKxGapve0rrzP7ZUtYtgwz7e6Zh0HirmLOYp1tr3BA@mail.gmail.com","threadId":"42881","inReplyTo":"20160726094925.GB3347@book.hvoigt.net","subject":"Re: [PATCH 2/2] submodule-config: combine error checking if clauses","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-26T17:24:44Z","receivedAt":"2016-07-26T17:24:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt wrote:\n> So we have less return handling code.\n>\n> Signed-off-by: Heiko Voigt \n\nThanks,\nStefan\n"},{"id":"292285","messageId":"xmqqmvl4xdlm.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"CAGZ79kaOf3NRAXh+krM=onwswSjAF3yy_zpa1d+9CFOBNke6-w@mail.gmail.com","subject":"Re: [PATCH 1/2] fix passing a name for config from submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-26T22:02:13Z","receivedAt":"2016-07-26T22:02:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>> parameter.commit_sha1 = commit_sha1;\n>> parameter.gitmodules_sha1 = sha1;\n>> parameter.overwrite = 0;\n>> - git_config_from_mem(parse_config, \"submodule-blob\", \"\",\n>> + git_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n>> config, config_size, ¶meter);\n>\n> Ok, this is the actual fix. Do you want to demonstrate its impact by adding\n> one or two tests that failed before and now work?\n> (As I was using the submodule config API most of the time with null_sha1\n> to indicate we'd be looking at the current .gitmodules file in the worktree,\n> the actual bug may have not manifested in the users of this API.\n> But still, it would be nice to see what was broken?)\n\nSounds like a good idea. I'll keep these two queued on 'pu' and see\nif Heiko (or somebody else) can find time to do that, so that we can\nreplace them with an improved version when it happens.\n\nThanks.\n"},{"id":"292395","messageId":"20160728111636.GA7760@sandbox","threadId":"42881","inReplyTo":"CAGZ79kaOf3NRAXh+krM=onwswSjAF3yy_zpa1d+9CFOBNke6-w@mail.gmail.com","subject":"Re: [PATCH 1/2] fix passing a name for config from submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-28T11:17:11Z","receivedAt":"2016-07-28T11:17:23Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote:\n> On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt wrote:\n> \n> Thanks for continuing on the submodule cache!\n\nNo worries. Its my code so I am happy to fix any bugs in it. Although it\nwas obviously perfect from the beginning ;)\n\n> > In commit 959b5455 we implemented the initial version of the submodule\n> \n> Usually we refer to the commit by a triple of \"abbrev. sha1 (date, subject).\n> See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free)\n> for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings\n> similarly to those in documentation).\n> \n> Apparently we put the subject first and then the date. I always did it\n> the other way\n> round, to there is no strict coding guide line, though it helps a lot to have an\n> understanding for a) how long are we in the \"broken\" state already as well as\n> b) what was the rationale for introducing it.\n\nAh ok did not know about this format. Will change that. I also will\nfollow-up with a patch to document this in SubmittingPatches so we can\npoint others to that...\n\n> > @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> > return entry->config;\n> > }\n> >\n> > - if (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n> > + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n> > + strbuf_release(&rev);\n> > return NULL;\n> \n> This is a reoccuring pattern below. Maybe it might make sense to\n> just do a s/return.../ goto out/ and at that label we cleanup `rev` and `config`\n> and return a result value?\n> There are currently 6 early returns (not counting the 3 from the last switch),\n> 4 of them return NULL, so that would result in just a \"goto out\", whereas 2\n> return an actual value, they would need to assign the result value first before\n> jumping out of the logic. I dunno, just food for though.\n\nI also though about that but was not sure whether it would actually make\nthings simple. Will look into that as the second patch.\n\n> > @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> > parameter.commit_sha1 = commit_sha1;\n> > parameter.gitmodules_sha1 = sha1;\n> > parameter.overwrite = 0;\n> > - git_config_from_mem(parse_config, \"submodule-blob\", \"\",\n> > + git_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n> > config, config_size, ¶meter);\n> \n> Ok, this is the actual fix. Do you want to demonstrate its impact by adding\n> one or two tests that failed before and now work?\n> (As I was using the submodule config API most of the time with null_sha1\n> to indicate we'd be looking at the current .gitmodules file in the worktree,\n> the actual bug may have not manifested in the users of this API.\n> But still, it would be nice to see what was broken?)\n\nThis popped up because of Rene's cleanup patch and I wanted to provide\na patch of what was originally supposed to go in there.\n\nThe name (originally representing the filename of the parsed config) is\nput into the structure that represents the source. I had a quick look\nand it seems to mostly be used in error messages. E.g.:\n\n * in error message git_parse_source() to reference the file\n * error message in git_die_config_linenr() (filename is derived from\n name)\n\nThere is some more, but I will add a test which checks whether the error\nmessage actually contains a reference to the blob instead of nothing.\n\nE.g. looks like this:\n\n\terror: bad config line 7 in submodule-blob 39a7458d2b5b3e3d1938b01ff2645b14c94ac284:.gitmodules\n\ninstead of this\n\n\terror: bad config line 7 in submodule-blob\n\nThat might be quite helpful to find out where the error is when you have\none in the history. So we are fixing a real bug here (not just a\ntheoretical one).\n\nCheers Heiko\n"},{"id":"292399","messageId":"20160728124910.GA2530@sandbox","threadId":"42881","inReplyTo":"xmqqmvl4xdlm.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-28T12:49:11Z","receivedAt":"2016-07-28T12:49:23Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Commit 959b5455 (submodule: implement a config API for lookup of\n.gitmodules values, 2015-08-18) implemented the initial version of the\nsubmodule config cache. During development of that initial version we\nextracted the function gitmodule_sha1_from_commit(). During that process\nwe missed that the strbuf rev was still used in config_from() and now is\nleft empty. Lets fix this by also returning this string.\n\nThis means that now when reading .gitmodules from revisions, the error\nmessages also contain a reference to the blob they are from.\n\nSigned-off-by: Heiko Voigt \n---\nHere you go. Now including a test.\n\n submodule-config.c | 26 +++++++++++++++++---------\n t/t7411-submodule-config.sh | 11 +++++++++++\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 1210b26..853989e 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -349,9 +349,9 @@ static int parse_config(const char *var, const char *value, void *data)\n }\n \n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n-\t\t\t\t unsigned char *gitmodules_sha1)\n+\t\t\t\t unsigned char *gitmodules_sha1,\n+\t\t\t\t struct strbuf *rev)\n {\n-\tstruct strbuf rev = STRBUF_INIT;\n \tint ret = 0;\n \n \tif (is_null_sha1(commit_sha1)) {\n@@ -359,11 +359,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\treturn 1;\n \t}\n \n-\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n-\tif (get_sha1(rev.buf, gitmodules_sha1) >= 0)\n+\tstrbuf_addf(rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tif (get_sha1(rev->buf, gitmodules_sha1) >= 0)\n \t\tret = 1;\n \n-\tstrbuf_release(&rev);\n \treturn ret;\n }\n \n@@ -375,6 +374,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tconst unsigned char *commit_sha1, const char *key,\n \t\tenum lookup_type lookup_type)\n {\n+\tstruct strbuf rev = STRBUF_INIT;\n \tunsigned long config_size;\n \tchar *config;\n \tunsigned char sha1[20];\n@@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\treturn entry->config;\n \t}\n \n-\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n+\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n@@ -408,14 +410,19 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n \t\tbreak;\n \t}\n-\tif (submodule)\n+\tif (submodule) {\n+\t\tstrbuf_release(&rev);\n \t\treturn submodule;\n+\t}\n \n \tconfig = read_sha1_file(sha1, &type, &config_size);\n-\tif (!config)\n+\tif (!config) {\n+\t\tstrbuf_release(&rev);\n \t\treturn NULL;\n+\t}\n \n \tif (type != OBJ_BLOB) {\n+\t\tstrbuf_release(&rev);\n \t\tfree(config);\n \t\treturn NULL;\n \t}\n@@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.commit_sha1 = commit_sha1;\n \tparameter.gitmodules_sha1 = sha1;\n \tparameter.overwrite = 0;\n-\tgit_config_from_mem(parse_config, \"submodule-blob\", \"\",\n+\tgit_config_from_mem(parse_config, \"submodule-blob\", rev.buf,\n \t\t\tconfig, config_size, ¶meter);\n+\tstrbuf_release(&rev);\n \tfree(config);\n \n \tswitch (lookup_type) {\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex fc97c33..400e2b1 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -82,6 +82,17 @@ test_expect_success 'error in one submodule config lets continue' '\n \t)\n '\n \n+test_expect_success 'error message contains blob reference' '\n+\t(cd super &&\n+\t\tsha1=$(git rev-parse HEAD) &&\n+\t\ttest-submodule-config \\\n+\t\t\tHEAD b \\\n+\t\t\tHEAD submodule \\\n+\t\t\t\t2>actual_err &&\n+\t\tgrep \"submodule-blob $sha1:.gitmodules\" actual_err >/dev/null\n+\t)\n+'\n+\n cat >super/expect_url <\n---\nThis is an updated cleanup patch. Now with goto so we have simpler code.\n\n submodule-config.c | 31 ++++++++++++-------------------\n 1 file changed, 12 insertions(+), 19 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 853989e..a887574 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -376,7 +376,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n {\n \tstruct strbuf rev = STRBUF_INIT;\n \tunsigned long config_size;\n-\tchar *config;\n+\tchar *config = NULL;\n \tunsigned char sha1[20];\n \tenum object_type type;\n \tconst struct submodule *submodule = NULL;\n@@ -397,10 +397,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\treturn entry->config;\n \t}\n \n-\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {\n-\t\tstrbuf_release(&rev);\n-\t\treturn NULL;\n-\t}\n+\tif (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))\n+\t\tgoto out;\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n@@ -410,22 +408,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t\tsubmodule = cache_lookup_path(cache, sha1, key);\n \t\tbreak;\n \t}\n-\tif (submodule) {\n-\t\tstrbuf_release(&rev);\n-\t\treturn submodule;\n-\t}\n+\tif (submodule)\n+\t\tgoto out;\n \n \tconfig = read_sha1_file(sha1, &type, &config_size);\n-\tif (!config) {\n-\t\tstrbuf_release(&rev);\n-\t\treturn NULL;\n-\t}\n-\n-\tif (type != OBJ_BLOB) {\n-\t\tstrbuf_release(&rev);\n-\t\tfree(config);\n-\t\treturn NULL;\n-\t}\n+\tif (!config || type != OBJ_BLOB)\n+\t\tgoto out;\n \n \t/* fill the submodule config into the cache */\n \tparameter.cache = cache;\n@@ -445,6 +433,11 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tdefault:\n \t\treturn NULL;\n \t}\n+\n+out:\n+\tstrbuf_release(&rev);\n+\tfree(config);\n+\treturn submodule;\n }\n \n static const struct submodule *config_from_path(struct submodule_cache *cache,\n-- \n2.0.2.832.g083c931\n\n"},{"id":"292401","messageId":"20160728125002.GC2530@sandbox","threadId":"42881","inReplyTo":"xmqqmvl4xdlm.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 3/3] submodule-config: fix test binary crashing when no arguments given","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-28T12:50:05Z","receivedAt":"2016-07-28T12:50:16Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Since arg[0] will be NULL without any argument here and starts_with()\ndoes not like NULL-pointers.\n\nSigned-off-by: Heiko Voigt \n---\nA small fix I found while developing the test.\n\n test-submodule-config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-submodule-config.c b/test-submodule-config.c\nindex dab8c27..a4e4098 100644\n--- a/test-submodule-config.c\n+++ b/test-submodule-config.c\n@@ -23,7 +23,7 @@ int main(int argc, char **argv)\n \n \targ++;\n \tmy_argc--;\n-\twhile (starts_with(arg[0], \"--\")) {\n+\twhile (arg[0] && starts_with(arg[0], \"--\")) {\n \t\tif (!strcmp(arg[0], \"--url\"))\n \t\t\toutput_url = 1;\n \t\tif (!strcmp(arg[0], \"--name\"))\n-- \n2.0.2.832.g083c931\n\n"},{"id":"292402","messageId":"20160728125514.GA2813@sandbox","threadId":"42881","inReplyTo":"20160728111636.GA7760@sandbox","subject":"[PATCH] document how to reference previous commits","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-07-28T12:55:14Z","receivedAt":"2016-07-28T12:55:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To reference previous commits people used to put just the abbreviated\nSHA-1 into commit messages. This is what has evolved as a more\nstable format for referencing commits. So lets document it for everyone\nto lookup when needed.\n\nSigned-off-by: Heiko Voigt \n---\nOn Thu, Jul 28, 2016 at 01:16:36PM +0200, Heiko Voigt wrote:\n> On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote:\n> > Usually we refer to the commit by a triple of \"abbrev. sha1 (date, subject).\n> > See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free)\n> > for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings\n> > similarly to those in documentation).\n> > \n> > Apparently we put the subject first and then the date. I always did it\n> > the other way\n> > round, to there is no strict coding guide line, though it helps a lot to have an\n> > understanding for a) how long are we in the \"broken\" state already as well as\n> > b) what was the rationale for introducing it.\n> \n> Ah ok did not know about this format. Will change that. I also will\n> follow-up with a patch to document this in SubmittingPatches so we can\n> point others to that...\n\nHere we go. Made this a seperate patch, since it is not really connected\nto the submodule-config-fix series.\n\nCheers Heiko\n\n Documentation/SubmittingPatches | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex e8ad978..500230c 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood\n without external resources. Instead of giving a URL to a mailing list\n archive, summarize the relevant points of the discussion.\n \n+If you want to reference a previous commit in the history of a stable\n+branch use the format \"abbreviated sha1 (subject, date)\". So for example\n+like this: \"Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n+noticed [...]\".\n+\n \n (3) Generate your patch using Git tools out of your commits.\n \n-- \n2.0.2.832.g083c931\n\n"},{"id":"292408","messageId":"xmqqeg6du7tp.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"20160728111636.GA7760@sandbox","subject":"Re: [PATCH 1/2] fix passing a name for config from submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T14:59:46Z","receivedAt":"2016-07-28T14:59:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n>> Apparently we put the subject first and then the date. I always did it\n>> the other way\n>> round, to there is no strict coding guide line,...\n\nPlease don't say \"this is not spelled out in the guidelines, so I\ncan do whatever I like, *EVEN* *THOUGH* I know that is different\nfrom what most others do\".\n\nWithout the \"follow the best-current-practice and be consistent\",\nwhich I think is in an early part of the guideline document (and if\nnot, it should be spelled out as the most important rule), the\nguidelines document will become too long that nobody would read.\n\n>> understanding for a) how long are we in the \"broken\" state already as well as\n>> b) what was the rationale for introducing it.\n>\n> Ah ok did not know about this format. Will change that.\n\nIn other words,\n\n In commit 959b5455 we implemented the initial version of ...\n\nwould become something like:\n\n 959b5455 (submodule: implement a config API for lookup of\n .gitmodules values, 2015-08-17) implemented the initial version\n of ...\n\nThanks.\n"},{"id":"292410","messageId":"xmqq60rpu61h.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"20160728125514.GA2813@sandbox","subject":"Re: [PATCH] document how to reference previous commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T15:38:18Z","receivedAt":"2016-07-28T15:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> @@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood\n> without external resources. Instead of giving a URL to a mailing list\n> archive, summarize the relevant points of the discussion.\n> \n> +If you want to reference a previous commit in the history of a stable\n> +branch use the format \"abbreviated sha1 (subject, date)\". So for example\n> +like this: \"Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n> +noticed [...]\".\n> +\n\nI earlier said that our goal should not be to spell out all the\nconventions, which would lead to unreadably long document, but this\ndoes not look too bad. Small additions however tend to accumulate\nover time, though ;-)\n\nI wondered if we need to also say \"why\", but we probably shouldn't.\n\nThanks.\n"},{"id":"292411","messageId":"CAGZ79kYm3ERZu4LX4DavT2o=HrShr3=boExJhfGe=yitru1OOg@mail.gmail.com","threadId":"42881","inReplyTo":"xmqq60rpu61h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] document how to reference previous commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T15:57:16Z","receivedAt":"2016-07-28T15:57:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 28, 2016 at 8:38 AM, Junio C Hamano wrote:\n> Heiko Voigt writes:\n>\n>> @@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood\n>> without external resources. Instead of giving a URL to a mailing list\n>> archive, summarize the relevant points of the discussion.\n>>\n>> +If you want to reference a previous commit in the history of a stable\n>> +branch use the format \"abbreviated sha1 (subject, date)\". So for example\n>> +like this: \"Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n>> +noticed [...]\".\n>> +\n>\n\nThanks!\n"},{"id":"292425","messageId":"CAGZ79kbjDvOziGn+202gOc19Y6omwin9CGhhL3YydS=1qSPjwg@mail.gmail.com","threadId":"42881","inReplyTo":"20160728124910.GA2530@sandbox","subject":"Re: [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T16:26:26Z","receivedAt":"2016-07-28T16:26:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 28, 2016 at 5:49 AM, Heiko Voigt wrote:\n> Commit 959b5455 (submodule: implement a config API for lookup of\n> .gitmodules values, 2015-08-18) implemented the initial version of the\n> submodule config cache. During development of that initial version we\n> extracted the function gitmodule_sha1_from_commit(). During that process\n> we missed that the strbuf rev was still used in config_from() and now is\n> left empty. Lets fix this by also returning this string.\n>\n> This means that now when reading .gitmodules from revisions, the error\n> messages also contain a reference to the blob they are from.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> Here you go. Now including a test.\n\nAll 3 patches look good to me, thanks!\n\n>\n> +test_expect_success 'error message contains blob reference' '\n> + (cd super &&\n> + sha1=$(git rev-parse HEAD) &&\n> + test-submodule-config \\\n> + HEAD b \\\n> + HEAD submodule \\\n> + 2>actual_err &&\n> + grep \"submodule-blob $sha1:.gitmodules\" actual_err >/dev/null\n\nMakes sense!\n"},{"id":"299537","messageId":"20160817113602.GB6080@sandbox","threadId":"42881","inReplyTo":"20160728125514.GA2813@sandbox","subject":"Re: [PATCH] document how to reference previous commits","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-08-17T11:36:02Z","receivedAt":"2016-08-17T11:36:13Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Thu, Jul 28, 2016 at 02:55:14PM +0200, Heiko Voigt wrote:\n> To reference previous commits people used to put just the abbreviated\n> SHA-1 into commit messages. This is what has evolved as a more\n> stable format for referencing commits. So lets document it for everyone\n> to lookup when needed.\n\nA quick ping about this patch. Maybe you missed to include it Junio? I\ncan not find any reference to it in the cooking mails and in your\nrepository.\n\nCheers Heiko\n"},{"id":"299559","messageId":"xmqqlgzv9tm1.fsf@gitster.mtv.corp.google.com","threadId":"42881","inReplyTo":"20160817113602.GB6080@sandbox","subject":"Re: [PATCH] document how to reference previous commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T17:45:58Z","receivedAt":"2016-08-17T17:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> On Thu, Jul 28, 2016 at 02:55:14PM +0200, Heiko Voigt wrote:\n>> To reference previous commits people used to put just the abbreviated\n>> SHA-1 into commit messages. This is what has evolved as a more\n>> stable format for referencing commits. So lets document it for everyone\n>> to lookup when needed.\n>\n> A quick ping about this patch. Maybe you missed to include it Junio? I\n> can not find any reference to it in the cooking mails and in your\n> repository.\n\nThat's because I was undecided.\n"}]}