{"thread":{"id":"44428","subject":"[PATCH 3/5] check-ref-format: Abolish leak of collapsed refname","startedAt":"2016-11-04T19:38:14Z","lastAt":"2016-12-20T07:30:56Z","messageCount":18,"participants":["Ian Jackson","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"305403","messageId":"20161104191358.28812-4-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"[PATCH 3/5] check-ref-format: Abolish leak of collapsed refname","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:56Z","receivedAt":"2016-11-04T19:38:14Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"collapse_slashes always returns a value from xmallocz.\n\nRight now this leak is not very interesting, since we only call\ncheck_one_ref_format once.\n\nSigned-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n---\n builtin/check-ref-format.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex f12c19c..020ebe8 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -63,8 +63,10 @@ static int check_one_ref_format(const char *refname)\n \t\t: check_refname_format(refname, flags);\n \tif (got)\n \t\treturn 1;\n-\tif (normalize)\n+\tif (normalize) {\n \t\tprintf(\"%s\\n\", refname);\n+\t\tfree((void*)refname);\n+\t}\n }\n \n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n-- \n2.10.1\n\n"},{"id":"305404","messageId":"20161104191358.28812-2-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"[PATCH 1/5] check-ref-format: Refactor out check_one_ref_format","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:54Z","receivedAt":"2016-11-04T19:42:43Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"We are going to want to reuse this.  No functional change right now.\n\nIt currently has a hidden memory leak if --normalize is used.\n\nSigned-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n---\n builtin/check-ref-format.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex eac4994..4d56caa 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -48,12 +48,22 @@ static int check_ref_format_branch(const char *arg)\n \treturn 0;\n }\n \n+static int normalize = 0;\n+static int flags = 0;\n+\n+static int check_one_ref_format(const char *refname)\n+{\n+\tif (normalize)\n+\t\trefname = collapse_slashes(refname);\n+\tif (check_refname_format(refname, flags))\n+\t\treturn 1;\n+\tif (normalize)\n+\t\tprintf(\"%s\\n\", refname);\n+}\n+\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tint normalize = 0;\n-\tint flags = 0;\n-\tconst char *refname;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_check_ref_format_usage);\n@@ -76,13 +86,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n \n-\trefname = argv[i];\n-\tif (normalize)\n-\t\trefname = collapse_slashes(refname);\n-\tif (check_refname_format(refname, flags))\n-\t\treturn 1;\n-\tif (normalize)\n-\t\tprintf(\"%s\\n\", refname);\n-\n-\treturn 0;\n+\treturn check_one_ref_format(argv[i]);\n }\n-- \n2.10.1\n\n"},{"id":"305405","messageId":"20161104191358.28812-3-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"[PATCH 2/5] check-ref-format: Refactor to make --branch code more common","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:55Z","receivedAt":"2016-11-04T19:44:52Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"We are going to want to permit other options with --branch.\n\nSo, replace the special case with just an entry for --branch in the\nparser for ordinary options, and check for option compatibility at the\nend.\n\nNo overall functional change.\n\nSigned-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n---\n builtin/check-ref-format.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 4d56caa..f12c19c 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)\n }\n \n static int normalize = 0;\n+static int check_branch = 0;\n static int flags = 0;\n \n static int check_one_ref_format(const char *refname)\n {\n+\tint got;\n+\n \tif (normalize)\n \t\trefname = collapse_slashes(refname);\n-\tif (check_refname_format(refname, flags))\n+\tgot = check_branch\n+\t\t? check_ref_format_branch(refname)\n+\t\t: check_refname_format(refname, flags);\n+\tif (got)\n \t\treturn 1;\n \tif (normalize)\n \t\tprintf(\"%s\\n\", refname);\n@@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_check_ref_format_usage);\n \n-\tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n-\t\treturn check_ref_format_branch(argv[2]);\n-\n \tfor (i = 1; i < argc && argv[i][0] == '-'; i++) {\n \t\tif (!strcmp(argv[i], \"--normalize\") || !strcmp(argv[i], \"--print\"))\n \t\t\tnormalize = 1;\n@@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\t\tflags &= ~REFNAME_ALLOW_ONELEVEL;\n \t\telse if (!strcmp(argv[i], \"--refspec-pattern\"))\n \t\t\tflags |= REFNAME_REFSPEC_PATTERN;\n+\t\telse if (!strcmp(argv[i], \"--branch\"))\n+\t\t\tcheck_branch = 1;\n \t\telse\n \t\t\tusage(builtin_check_ref_format_usage);\n \t}\n+\n+\tif (check_branch && (flags || normalize))\n+\t\tusage(builtin_check_ref_format_usage);\n+\n \tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n \n-- \n2.10.1\n\n"},{"id":"305406","messageId":"20161104191358.28812-6-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"[PATCH 5/5] check-ref-format: New --stdin option","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:58Z","receivedAt":"2016-11-04T20:24:39Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n---\n Documentation/git-check-ref-format.txt | 10 ++++++++--\n builtin/check-ref-format.c             | 34 +++++++++++++++++++++++++++++++---\n 2 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex e9a2657..5a213ce 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -10,8 +10,9 @@ SYNOPSIS\n [verse]\n 'git check-ref-format' [--report-errors] [--normalize]\n        [--[no-]allow-onelevel] [--refspec-pattern]\n-       <refname>\n-'git check-ref-format' [--report-errors] --branch <branchname-shorthand>\n+       <refname> | --stdin\n+'git check-ref-format' [--report-errors] --branch\n+       <branchname-shorthand> | --stdin\n \n DESCRIPTION\n -----------\n@@ -109,6 +110,11 @@ OPTIONS\n \tIf any ref does not check OK, print a message to stderr.\n         (By default, git check-ref-format is silent.)\n \n+--stdin::\n+\tInstead of checking on ref supplied on the command line,\n+\tread refs, one per line, from stdin.  The exit status is\n+\t0 if all the refs were OK.\n+\n \n EXAMPLES\n --------\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 559d5c2..87f52fa 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n+\tint use_stdin = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_check_ref_format_usage);\n@@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\t\tcheck_branch = 1;\n \t\telse if (!strcmp(argv[i], \"--report-errors\"))\n \t\t\treport_errors = 1;\n+\t\telse if (!strcmp(argv[i], \"--stdin\"))\n+\t\t\tuse_stdin = 1;\n \t\telse\n \t\t\tusage(builtin_check_ref_format_usage);\n \t}\n@@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tif (check_branch && (flags || normalize))\n \t\tusage(builtin_check_ref_format_usage);\n \n-\tif (! (i == argc - 1))\n-\t\tusage(builtin_check_ref_format_usage);\n+\tif (!use_stdin) {\n+\t\tif (! (i == argc - 1))\n+\t\t\tusage(builtin_check_ref_format_usage);\n+\n+\t\treturn check_one_ref_format(argv[i]);\n+\t} else {\n+\t\tchar buffer[2048];\n+\t\tint worst = 0;\n \n-\treturn check_one_ref_format(argv[i]);\n+\t\tif (! (i == argc))\n+\t\t\tusage(builtin_check_ref_format_usage);\n+\n+\t\twhile (fgets(buffer, sizeof(buffer), stdin)) {\n+\t\t\tchar *newline = strchr(buffer, '\\n');\n+\t\t\tif (!newline) {\n+\t\t\t\tfprintf(stderr, \"%s --stdin: missing final newline or line too long\\n\", *argv);\n+\t\t\t\texit(127);\n+\t\t\t}\n+\t\t\t*newline = 0;\n+\t\t\tint got = check_one_ref_format(buffer);\n+\t\t\tif (got > worst)\n+\t\t\t\tworst = got;\n+\t\t}\n+\t\tif (!feof(stdin)) {\n+\t\t\tperror(\"reading from stdin\");\n+\t\t\texit(127);\n+\t\t}\n+\t\treturn worst;\n+\t}\n }\n-- \n2.10.1\n\n"},{"id":"305407","messageId":"20161104191358.28812-5-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"[PATCH 4/5] check-ref-format: New --report-errors option","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:57Z","receivedAt":"2016-11-04T20:24:42Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n---\n Documentation/git-check-ref-format.txt |  8 ++++++--\n builtin/check-ref-format.c             | 10 ++++++++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex 8611a99..e9a2657 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -8,10 +8,10 @@ git-check-ref-format - Ensures that a reference name is well formed\n SYNOPSIS\n --------\n [verse]\n-'git check-ref-format' [--normalize]\n+'git check-ref-format' [--report-errors] [--normalize]\n        [--[no-]allow-onelevel] [--refspec-pattern]\n        <refname>\n-'git check-ref-format' --branch <branchname-shorthand>\n+'git check-ref-format' [--report-errors] --branch <branchname-shorthand>\n \n DESCRIPTION\n -----------\n@@ -105,6 +105,10 @@ OPTIONS\n \twith a status of 0.  (`--print` is a deprecated way to spell\n \t`--normalize`.)\n \n+--report-errors::\n+\tIf any ref does not check OK, print a message to stderr.\n+        (By default, git check-ref-format is silent.)\n+\n \n EXAMPLES\n --------\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 020ebe8..559d5c2 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -9,7 +9,7 @@\n \n static const char builtin_check_ref_format_usage[] =\n \"git check-ref-format [--normalize] [<options>] <refname>\\n\"\n-\"   or: git check-ref-format --branch <branchname-shorthand>\";\n+\"   or: git check-ref-format [<options>] --branch <branchname-shorthand>\";\n \n /*\n  * Return a copy of refname but with leading slashes removed and runs\n@@ -51,6 +51,7 @@ static int check_ref_format_branch(const char *arg)\n static int normalize = 0;\n static int check_branch = 0;\n static int flags = 0;\n+static int report_errors = 0;\n \n static int check_one_ref_format(const char *refname)\n {\n@@ -61,8 +62,11 @@ static int check_one_ref_format(const char *refname)\n \tgot = check_branch\n \t\t? check_ref_format_branch(refname)\n \t\t: check_refname_format(refname, flags);\n-\tif (got)\n+\tif (got) {\n+\t\tif (report_errors)\n+\t\t\tfprintf(stderr, \"bad ref format: %s\\n\", refname);\n \t\treturn 1;\n+\t}\n \tif (normalize) {\n \t\tprintf(\"%s\\n\", refname);\n \t\tfree((void*)refname);\n@@ -87,6 +91,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\t\tflags |= REFNAME_REFSPEC_PATTERN;\n \t\telse if (!strcmp(argv[i], \"--branch\"))\n \t\t\tcheck_branch = 1;\n+\t\telse if (!strcmp(argv[i], \"--report-errors\"))\n+\t\t\treport_errors = 1;\n \t\telse\n \t\t\tusage(builtin_check_ref_format_usage);\n \t}\n-- \n2.10.1\n\n"},{"id":"305408","messageId":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","threadId":"44428","inReplyTo":null,"subject":"[PATCH 0/5] git check-ref-format --stdin --report-errors","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-11-04T19:13:53Z","receivedAt":"2016-11-04T20:24:43Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"I wanted to be able to syntax check lots of proposed refs quickly\n(please don't ask why - it's complicated!)\n\nSo I added a --stdin option to git-check-ref-format.  Also it has\n--report-errors now too so you can get some kind of useful error\nmessage if it complains.\n\nIt's still not really a good batch mode but it's good enough for my\nuse case.  To improve it would involve a new command line option to\noffer a suitable stdout output format.\n\nThere are three small refactoring patches and the two patches with new\noptions and corresponding docs.\n\nThanks for your attention.\n\nFYI I am not likely to need this again in the near future: it's a\none-off use case.  So my effort for rework is probably limited.  I\nthought I'd share what I'd done in what I hope is a useful form,\nanyway.\n\nRegards,\nIan.\n"},{"id":"308007","messageId":"3e277bb8-bd1f-0d8c-47a7-9673ad711bce@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-2-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T08:27:41Z","receivedAt":"2016-12-19T08:36:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> We are going to want to reuse this.  No functional change right now.\n> \n> It currently has a hidden memory leak if --normalize is used.\n> \n> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n> ---\n>  builtin/check-ref-format.c | 26 ++++++++++++++------------\n>  1 file changed, 14 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index eac4994..4d56caa 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -48,12 +48,22 @@ static int check_ref_format_branch(const char *arg)\n>  \treturn 0;\n>  }\n>  \n> +static int normalize = 0;\n> +static int flags = 0;\n> +\n> +static int check_one_ref_format(const char *refname)\n> +{\n> +\tif (normalize)\n> +\t\trefname = collapse_slashes(refname);\n> +\tif (check_refname_format(refname, flags))\n> +\t\treturn 1;\n> +\tif (normalize)\n> +\t\tprintf(\"%s\\n\", refname);\n\nThis function needs to `return 0` if it gets to the end.\n\n> +}\n> +\n>  int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i;\n> -\tint normalize = 0;\n> -\tint flags = 0;\n> -\tconst char *refname;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage(builtin_check_ref_format_usage);\n> @@ -76,13 +86,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \tif (! (i == argc - 1))\n>  \t\tusage(builtin_check_ref_format_usage);\n>  \n> -\trefname = argv[i];\n> -\tif (normalize)\n> -\t\trefname = collapse_slashes(refname);\n> -\tif (check_refname_format(refname, flags))\n> -\t\treturn 1;\n> -\tif (normalize)\n> -\t\tprintf(\"%s\\n\", refname);\n> -\n> -\treturn 0;\n> +\treturn check_one_ref_format(argv[i]);\n>  }\n> \n\nMichael\n\n"},{"id":"308009","messageId":"e93ee78a-aa5e-27f6-9703-6efa385f487b@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-3-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T11:07:27Z","receivedAt":"2016-12-19T11:08:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> We are going to want to permit other options with --branch.\n> \n> So, replace the special case with just an entry for --branch in the\n> parser for ordinary options, and check for option compatibility at the\n> end.\n> \n> No overall functional change.\n> \n> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n> ---\n>  builtin/check-ref-format.c | 17 +++++++++++++----\n>  1 file changed, 13 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index 4d56caa..f12c19c 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)\n>  }\n>  \n>  static int normalize = 0;\n> +static int check_branch = 0;\n>  static int flags = 0;\n>  \n>  static int check_one_ref_format(const char *refname)\n>  {\n> +\tint got;\n\n`got` is an unusual name for this variable, and I don't really\nunderstand what the word means in this context. Is there a reason not to\nuse the more usual `err`?\n\n> +\n>  \tif (normalize)\n>  \t\trefname = collapse_slashes(refname);\n> -\tif (check_refname_format(refname, flags))\n> +\tgot = check_branch\n> +\t\t? check_ref_format_branch(refname)\n> +\t\t: check_refname_format(refname, flags);\n> +\tif (got)\n>  \t\treturn 1;\n>  \tif (normalize)\n>  \t\tprintf(\"%s\\n\", refname);\n> @@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage(builtin_check_ref_format_usage);\n>  \n> -\tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n> -\t\treturn check_ref_format_branch(argv[2]);\n> -\n>  \tfor (i = 1; i < argc && argv[i][0] == '-'; i++) {\n>  \t\tif (!strcmp(argv[i], \"--normalize\") || !strcmp(argv[i], \"--print\"))\n>  \t\t\tnormalize = 1;\n> @@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \t\t\tflags &= ~REFNAME_ALLOW_ONELEVEL;\n>  \t\telse if (!strcmp(argv[i], \"--refspec-pattern\"))\n>  \t\t\tflags |= REFNAME_REFSPEC_PATTERN;\n> +\t\telse if (!strcmp(argv[i], \"--branch\"))\n> +\t\t\tcheck_branch = 1;\n>  \t\telse\n>  \t\t\tusage(builtin_check_ref_format_usage);\n>  \t}\n> +\n> +\tif (check_branch && (flags || normalize))\n\nIs there a reason not to allow `--normalize` with `--branch`?\n(Currently, `git check-ref-format --branch` *does* allow input like\n`refs/heads/foo`.)\n\nBut note that simply allowing `--branch --normalize` without changing\n`check_one_ref_format()` would mean generating *two* lines of output per\nreference, so something else would have to change, too.\n\n> +\t\tusage(builtin_check_ref_format_usage);\n> +\n>  \tif (! (i == argc - 1))\n>  \t\tusage(builtin_check_ref_format_usage);\n>  \n> \n\nMichael\n\n"},{"id":"308010","messageId":"71960ab9-c42f-2db6-5359-58afb5a2a8fb@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-4-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 3/5] check-ref-format: Abolish leak of collapsed refname","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T11:09:55Z","receivedAt":"2016-12-19T11:11:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> collapse_slashes always returns a value from xmallocz.\n> \n> Right now this leak is not very interesting, since we only call\n> check_one_ref_format once.\n> \n> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n> ---\n>  builtin/check-ref-format.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index f12c19c..020ebe8 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -63,8 +63,10 @@ static int check_one_ref_format(const char *refname)\n>  \t\t: check_refname_format(refname, flags);\n>  \tif (got)\n>  \t\treturn 1;\n\nIf the function returns via the line above, then the memory will still\nbe leaked.\n\n> -\tif (normalize)\n> +\tif (normalize) {\n>  \t\tprintf(\"%s\\n\", refname);\n> +\t\tfree((void*)refname);\n> +\t}\n>  }\n>  \n>  int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n> \n\nMichael\n\n"},{"id":"308014","messageId":"9d2d25d8-e9cf-5d9f-8e7e-5d426e219344@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-6-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 5/5] check-ref-format: New --stdin option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T11:22:04Z","receivedAt":"2016-12-19T11:23:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n> ---\n>  Documentation/git-check-ref-format.txt | 10 ++++++++--\n>  builtin/check-ref-format.c             | 34 +++++++++++++++++++++++++++++++---\n>  2 files changed, 39 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\n> index e9a2657..5a213ce 100644\n> --- a/Documentation/git-check-ref-format.txt\n> +++ b/Documentation/git-check-ref-format.txt\n> @@ -10,8 +10,9 @@ SYNOPSIS\n>  [verse]\n>  'git check-ref-format' [--report-errors] [--normalize]\n>         [--[no-]allow-onelevel] [--refspec-pattern]\n> -       <refname>\n> -'git check-ref-format' [--report-errors] --branch <branchname-shorthand>\n> +       <refname> | --stdin\n> +'git check-ref-format' [--report-errors] --branch\n> +       <branchname-shorthand> | --stdin\n>  \n>  DESCRIPTION\n>  -----------\n> @@ -109,6 +110,11 @@ OPTIONS\n>  \tIf any ref does not check OK, print a message to stderr.\n>          (By default, git check-ref-format is silent.)\n>  \n> +--stdin::\n> +\tInstead of checking on ref supplied on the command line,\n> +\tread refs, one per line, from stdin.  The exit status is\n> +\t0 if all the refs were OK.\n> +\n>  \n>  EXAMPLES\n>  --------\n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index 559d5c2..87f52fa 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)\n>  int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i;\n> +\tint use_stdin = 0;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage(builtin_check_ref_format_usage);\n> @@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \t\t\tcheck_branch = 1;\n>  \t\telse if (!strcmp(argv[i], \"--report-errors\"))\n>  \t\t\treport_errors = 1;\n> +\t\telse if (!strcmp(argv[i], \"--stdin\"))\n> +\t\t\tuse_stdin = 1;\n>  \t\telse\n>  \t\t\tusage(builtin_check_ref_format_usage);\n>  \t}\n> @@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \tif (check_branch && (flags || normalize))\n>  \t\tusage(builtin_check_ref_format_usage);\n>  \n> -\tif (! (i == argc - 1))\n> -\t\tusage(builtin_check_ref_format_usage);\n> +\tif (!use_stdin) {\n> +\t\tif (! (i == argc - 1))\n> +\t\t\tusage(builtin_check_ref_format_usage);\n\nGiven the changes that you made to support `--stdin`, it would be pretty\neasy to support multiple command line arguments, now, too. (But this\nneedn't be part of your patch series.)\n\n> +\n> +\t\treturn check_one_ref_format(argv[i]);\n> +\t} else {\n> +\t\tchar buffer[2048];\n> +\t\tint worst = 0;\n>  \n> -\treturn check_one_ref_format(argv[i]);\n> +\t\tif (! (i == argc))\n> +\t\t\tusage(builtin_check_ref_format_usage);\n> +\n> +\t\twhile (fgets(buffer, sizeof(buffer), stdin)) {\n\n`strbuf_getline()` would make this a lot easier and also eliminate the\nneed to specify a buffer size.\n\n> +\t\t\tchar *newline = strchr(buffer, '\\n');\n> +\t\t\tif (!newline) {\n> +\t\t\t\tfprintf(stderr, \"%s --stdin: missing final newline or line too long\\n\", *argv);\n> +\t\t\t\texit(127);\n> +\t\t\t}\n> +\t\t\t*newline = 0;\n> +\t\t\tint got = check_one_ref_format(buffer);\n> +\t\t\tif (got > worst)\n> +\t\t\t\tworst = got;\n> +\t\t}\n> +\t\tif (!feof(stdin)) {\n> +\t\t\tperror(\"reading from stdin\");\n> +\t\t\texit(127);\n> +\t\t}\n> +\t\treturn worst;\n> +\t}\n>  }\n> \n\nMichael\n\n"},{"id":"308015","messageId":"561c0338-66cd-f806-7b3b-b422f98a1564@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-1-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 0/5] git check-ref-format --stdin --report-errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T11:29:40Z","receivedAt":"2016-12-19T11:30:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> I wanted to be able to syntax check lots of proposed refs quickly\n> (please don't ask why - it's complicated!)\n> \n> So I added a --stdin option to git-check-ref-format.  Also it has\n> --report-errors now too so you can get some kind of useful error\n> message if it complains.\n> \n> It's still not really a good batch mode but it's good enough for my\n> use case.  To improve it would involve a new command line option to\n> offer a suitable stdout output format.\n> \n> There are three small refactoring patches and the two patches with new\n> options and corresponding docs.\n> \n> Thanks for your attention.\n> \n> FYI I am not likely to need this again in the near future: it's a\n> one-off use case.  So my effort for rework is probably limited.  I\n> thought I'd share what I'd done in what I hope is a useful form,\n> anyway.\n\nThanks for your patches. I left some comments about the individual patches.\n\nI don't know whether this feature will be popular, but it's not a lot of\ncode to add it, so it would be OK with me.\n\nEspecially given that the output is not especially machine-readable, it\nmight be more consistent with other commands to call the new feature\n`--verbose` rather than `--report-errors`.\n\nIf it is thought likely that scripts will want to leave a pipe open to\nthis command and feed it one query at a time, then it would be helpful\nto flush stdout after each reference's result is written. If the\nopposite use case is common (mass processing of refnames), we could\nalways add a `--buffer` option like the one that `git cat-file --batch` has.\n\nMichael\n\n"},{"id":"308020","messageId":"22615.51843.744027.398293@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"e93ee78a-aa5e-27f6-9703-6efa385f487b@alum.mit.edu","subject":"Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-12-19T11:54:43Z","receivedAt":"2016-12-19T12:30:55Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Michael Haggerty writes (\"Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common\"):\n> On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> >  static int normalize = 0;\n> > +static int check_branch = 0;\n> >  static int flags = 0;\n> >  \n> >  static int check_one_ref_format(const char *refname)\n> >  {\n> > +\tint got;\n> \n> `got` is an unusual name for this variable, and I don't really\n> understand what the word means in this context. Is there a reason not to\n> use the more usual `err`?\n\nI have no real opinion about the name of this variable.  `err' is a\nfine name too.\n\n> > +\tif (check_branch && (flags || normalize))\n> \n> Is there a reason not to allow `--normalize` with `--branch`?\n> (Currently, `git check-ref-format --branch` *does* allow input like\n> `refs/heads/foo`.)\n\nIt was like that when I found it :-).  I wasn't sure why this\nrestriction was there so I left it alone.\n\nLooking at it again: AFAICT from the documentation --branch is a\ncompletely different mode.  The effect of --normalize is not to do\nadditional work, but simply to produce additional output.  It really\nmeans --print-normalized.  --branch already prints output, but AFAICT\nit does not collapse slashes.  This seems like a confusing collection\nof options.  But, sorting that out is beyond the scope of what I was\ntrying to do.\n\nIn my series I have at least managed not to make any of this any\nworse, I think: the --stdin option I introduce applies to both modes\nequally, and doesn't make future improvements to the conflict between\n--branch and --normalize any harder.\n\n(In _this_ patch, certainly, allowing --normalize with --branch would\nbe wrong, since _this_ patch is just refactoring.)\n\nThanks,\nIan.\n\n-- \nIan Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"308023","messageId":"22615.56956.698915.2223@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"3e277bb8-bd1f-0d8c-47a7-9673ad711bce@alum.mit.edu","subject":"Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-12-19T13:19:56Z","receivedAt":"2016-12-19T13:20:56Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Michael Haggerty writes (\"Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format\"):\n> On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> > +static int check_one_ref_format(const char *refname)\n...\n> This function needs to `return 0` if it gets to the end.\n\nIndeed it does.  I'm kind of surprised my compiler didn't spot that.\n\nThanks for the careful review!\n\nRegards,\nIan.\n\n-- \nIan Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"308027","messageId":"14d95a74-ad7e-a1dd-c3da-52afd53cede4@alum.mit.edu","threadId":"44428","inReplyTo":"20161104191358.28812-6-ijackson@chiark.greenend.org.uk","subject":"Re: [PATCH 5/5] check-ref-format: New --stdin option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-19T13:43:43Z","receivedAt":"2016-12-19T13:44:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/04/2016 08:13 PM, Ian Jackson wrote:\n> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>\n> ---\n>  Documentation/git-check-ref-format.txt | 10 ++++++++--\n>  builtin/check-ref-format.c             | 34 +++++++++++++++++++++++++++++++---\n>  2 files changed, 39 insertions(+), 5 deletions(-)\n> \n> [...]\n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index 559d5c2..87f52fa 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)\n>  int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i;\n> +\tint use_stdin = 0;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage(builtin_check_ref_format_usage);\n> @@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \t\t\tcheck_branch = 1;\n>  \t\telse if (!strcmp(argv[i], \"--report-errors\"))\n>  \t\t\treport_errors = 1;\n> +\t\telse if (!strcmp(argv[i], \"--stdin\"))\n> +\t\t\tuse_stdin = 1;\n>  \t\telse\n>  \t\t\tusage(builtin_check_ref_format_usage);\n>  \t}\n> @@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n>  \tif (check_branch && (flags || normalize))\n>  \t\tusage(builtin_check_ref_format_usage);\n>  \n> -\tif (! (i == argc - 1))\n> -\t\tusage(builtin_check_ref_format_usage);\n> +\tif (!use_stdin) {\n> +\t\tif (! (i == argc - 1))\n> +\t\t\tusage(builtin_check_ref_format_usage);\n> +\n> +\t\treturn check_one_ref_format(argv[i]);\n> +\t} else {\n> +\t\tchar buffer[2048];\n> +\t\tint worst = 0;\n>  \n> -\treturn check_one_ref_format(argv[i]);\n> +\t\tif (! (i == argc))\n> +\t\t\tusage(builtin_check_ref_format_usage);\n> +\n> +\t\twhile (fgets(buffer, sizeof(buffer), stdin)) {\n> +\t\t\tchar *newline = strchr(buffer, '\\n');\n> +\t\t\tif (!newline) {\n> +\t\t\t\tfprintf(stderr, \"%s --stdin: missing final newline or line too long\\n\", *argv);\n> +\t\t\t\texit(127);\n> +\t\t\t}\n> +\t\t\t*newline = 0;\n> +\t\t\tint got = check_one_ref_format(buffer);\n\nAnother minor point: project policy is not to mix declarations and code.\nPlease declare `got` at the top of the block.\n\n> +\t\t\tif (got > worst)\n> +\t\t\t\tworst = got;\n> +\t\t}\n> +\t\tif (!feof(stdin)) {\n> +\t\t\tperror(\"reading from stdin\");\n> +\t\t\texit(127);\n> +\t\t}\n> +\t\treturn worst;\n> +\t}\n>  }\n> \n\nMichael\n\n"},{"id":"308039","messageId":"22616.2400.364518.636144@chiark.greenend.org.uk","threadId":"44428","inReplyTo":"561c0338-66cd-f806-7b3b-b422f98a1564@alum.mit.edu","subject":"Re: [PATCH 0/5] git check-ref-format --stdin --report-errors","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2016-12-19T16:22:56Z","receivedAt":"2016-12-19T16:23:58Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Michael Haggerty writes (\"Re: [PATCH 0/5] git check-ref-format --stdin --report-errors\"):\n> Thanks for your patches. I left some comments about the individual patches.\n\nThanks for your review.\n\n> I don't know whether this feature will be popular, but it's not a lot of\n> code to add it, so it would be OK with me.\n\nGreat.\n\n> Especially given that the output is not especially machine-readable, it\n> might be more consistent with other commands to call the new feature\n> `--verbose` rather than `--report-errors`.\n\nSure.\n\n> If it is thought likely that scripts will want to leave a pipe open to\n> this command and feed it one query at a time, then it would be helpful\n> to flush stdout after each reference's result is written. If the\n> opposite use case is common (mass processing of refnames), we could\n> always add a `--buffer` option like the one that `git cat-file --batch` has.\n\nI think it should be unbuffered by default, so I will make that\nchange, along with the fixes from your other mails, and resubmit.\n\nRegards,\nIan.\n\n-- \nIan Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"308072","messageId":"xmqqlgvbpyku.fsf@gitster.mtv.corp.google.com","threadId":"44428","inReplyTo":"561c0338-66cd-f806-7b3b-b422f98a1564@alum.mit.edu","subject":"Re: [PATCH 0/5] git check-ref-format --stdin --report-errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-19T18:23:45Z","receivedAt":"2016-12-19T18:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Especially given that the output is not especially machine-readable, it\n> might be more consistent with other commands to call the new feature\n> `--verbose` rather than `--report-errors`.\n\nDon't we instead want to structure the output to be machine-readable\ninstead, given that check-ref-format is a very low level plumbing\ncommand that is primarily meant for scriptors?\n"},{"id":"308129","messageId":"52fb55b4-98ca-0e76-37bb-3536b7495c1b@alum.mit.edu","threadId":"44428","inReplyTo":"22615.56956.698915.2223@chiark.greenend.org.uk","subject":"Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-20T06:57:33Z","receivedAt":"2016-12-20T06:58:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/19/2016 02:19 PM, Ian Jackson wrote:\n> Michael Haggerty writes (\"Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format\"):\n>> On 11/04/2016 08:13 PM, Ian Jackson wrote:\n>>> +static int check_one_ref_format(const char *refname)\n> ...\n>> This function needs to `return 0` if it gets to the end.\n> \n> Indeed it does.  I'm kind of surprised my compiler didn't spot that.\n\nOur build system has a `DEVELOPER` option [1] that turns on lots of\nerrors and warnings, and you should turn it on if you haven't already:\n\n    echo DEVELOPER=1 >>config.mak\n\nWhat exactly it catches depends on what compiler you are using, but it\ndefinitely helps if you are using gcc, and I think also if you are using\nclang.\n\nMichael\n\n[1]\nhttps://github.com/git/git/blob/6610af872f6494a061780ec738c8713a034b848b/Documentation/CodingGuidelines#L174-L177\n"},{"id":"308130","messageId":"21317fc7-c1fb-8be0-eadf-90fed9486a48@alum.mit.edu","threadId":"44428","inReplyTo":"xmqqlgvbpyku.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/5] git check-ref-format --stdin --report-errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-20T07:29:31Z","receivedAt":"2016-12-20T07:30:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/19/2016 07:23 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Especially given that the output is not especially machine-readable, it\n>> might be more consistent with other commands to call the new feature\n>> `--verbose` rather than `--report-errors`.\n> \n> Don't we instead want to structure the output to be machine-readable\n> instead, given that check-ref-format is a very low level plumbing\n> command that is primarily meant for scriptors?\n\nOf course that would be the ideal. Let's think about what it would look\nlike. Given that the very purpose of the program is to decide whether\nits inputs are reasonable reference names or not, it would be important\nto make it bulletproof:\n\n* It could be fed some ugly garbage\n* It could be used for security-relevant checks\n\nOne obvious choice would be to use NUL separators, but that would make\nthe output mostly unreadable to humans.\n\nAnother would be to use LF to terminate each line of output, like\n\n    ok TAB refs/heads/foo LF\n    bad TAB refs/heads/bad SP name@@.lock LF\n\nFor the LF-terminated `--stdin` input, this should be unambiguous.\nHowever, it wouldn't necessarily work for arguments passed in via the\ncommand line, for for slight variations on `--stdin` like if we were to\nadd a `-z` option to allow the input to be NUL-terminated.\n\nThe 100% solution would probably be to support language-specific\nquoting, like the `--shell`/`--perl`/`--python`/`--tcl` options accepted\nby `for-each-ref`, probably with a fifth option for NUL-terminated\noutput. And it should probably also support a `-z` option to make its\ninput NUL-separated. Pretty much all of the infrastructure is already\nthere in `quote.h` and `quote.c`, and the option-parsing could be\ncribbed from `builtin/for-each-ref.c`, so it wouldn't even be *that*\nmuch work to implement.\n\nMichael\n\n"}]}