{"thread":{"id":"39677","subject":"[PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","startedAt":"2015-06-19T09:10:56Z","lastAt":"2015-06-26T15:48:22Z","messageCount":51,"participants":["Charles Bailey","Jeff King","John Keeping","Remi Galan Alfonso","Junio C Hamano","Jakub Narębski","Duy Nguyen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"264288","messageId":"1434705059-2793-1-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":null,"subject":"Improvements to parse-options and a new filter-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T09:10:56Z","receivedAt":"2015-06-19T09:10:56Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"In my team we've been looking for a fast way to check a large number of\nrepositories for large files, which are typically unintentionally checked in\nbinaries, so that we can warn repository owners and help them tidy up as\ndesired.\n\nThere seem to be two main approaches to scripting this. The first is to do\nsomething revision-walk based such as `log --numstat` and the second is to scan\npack files using `verify-pack -v` and either to ensure that everything is packed\nor scan loose objects separately.\n\nThe revision walking tends to be slow and parsing verify-pack -v is awkward\nnot only because of the need to take account of multiple packs and loose\nobjects, but also because it is porcelainish. For example, at some point it\ngained a delta chain summary which needs to be snipped before the list of\npacked objects can be sorted and used.\n\nThe third patch in this series adds a new built in which makes this simple and\nfast. While implementing it, I found a couple of other improvements which I\nthink stand alone.\n\n[PATCH 1/3] Correct test-parse-options to handle negative ints\n\nI noticed that a printf in test-parse-options was using %u instead of %d for an\nint with the consequence that it wouldn't ever print a negative value correctly.\nI don't know that we do ever parse a negative integer as an option, but there's\nno reason that it shouldn't work so I fixed it and added a trivial test.\n\n[PATCH 2/3] Move unsigned long option parsing out of pack-objects.c\n\nI wanted to be able to parse options like --min-size=500k in my new command so I\nstarted to add OPT_ULONG, only to realise that it already existed but was\nprivate to pack-objects. I added OPT_ULONG support to parse-options based on the\nexisting OPT_INTEGER code, added new tests and changed pack-objects to use this\ninstead.\n"},{"id":"264290","messageId":"1434705059-2793-2-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434705059-2793-1-git-send-email-charles@hashpling.org","subject":"[PATCH 1/3] Correct test-parse-options to handle negative ints","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T09:10:57Z","receivedAt":"2015-06-19T09:10:57Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nFix the printf specification to treat 'integer' as the signed type that\nit is and add a test that checks that we parse negative option\narguments.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n t/t0040-parse-options.sh | 2 ++\n test-parse-options.c     | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b044785..ecb7417 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -151,6 +151,8 @@ test_expect_success 'short options' '\n \ttest_must_be_empty output.err\n '\n \n+test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n+\n cat > expect << EOF\n boolean: 2\n integer: 1729\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 5dabce6..7c492cf 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -82,7 +82,7 @@ int main(int argc, char **argv)\n \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n \n \tprintf(\"boolean: %d\\n\", boolean);\n-\tprintf(\"integer: %u\\n\", integer);\n+\tprintf(\"integer: %d\\n\", integer);\n \tprintf(\"timestamp: %lu\\n\", timestamp);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n-- \n2.4.0.53.g8440f74\n"},{"id":"264287","messageId":"1434705059-2793-3-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434705059-2793-1-git-send-email-charles@hashpling.org","subject":"[PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T09:10:58Z","receivedAt":"2015-06-19T09:10:58Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nThe unsigned long option parsing (including 'k'/'m'/'g' suffix parsing)\nis more widely applicable. Add support for OPT_ULONG to parse-options.h\nand change pack-objects.c use this support.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n builtin/pack-objects.c   | 17 -----------------\n parse-options.c          | 15 +++++++++++++++\n parse-options.h          |  5 ++++-\n t/t0040-parse-options.sh | 46 ++++++++++++++++++++++++++++++++++++++++++----\n test-parse-options.c     |  3 +++\n 5 files changed, 64 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 80fe8c7..5de76db 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2588,23 +2588,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_ulong(const struct option *opt,\n-\t\t\t      const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tdie(_(\"option %s does not accept negative form\"),\n-\t\t    opt->long_name);\n-\n-\tif (!git_parse_ulong(arg, opt->value))\n-\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n-\t\t    arg, opt->long_name);\n-\treturn 0;\n-}\n-\n-#define OPT_ULONG(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), \"n\", (h),\t\\\n-\t  PARSE_OPT_NONEG, option_parse_ulong }\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\ndiff --git a/parse-options.c b/parse-options.c\nindex 80106c0..76a5c3e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -180,6 +180,21 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n \t\treturn 0;\n \n+\tcase OPTION_ULONG:\n+\t\tif (unset) {\n+\t\t\t*(unsigned long *)opt->value = 0;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n+\t\t\t*(unsigned long *)opt->value = opt->defval;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (get_arg(p, opt, flags, &arg))\n+\t\t\treturn -1;\n+\t\tif (!git_parse_ulong(arg, opt->value))\n+\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n+\t\treturn 0;\n+\n \tdefault:\n \t\tdie(\"should not happen, someone must be hit on the forehead\");\n \t}\ndiff --git a/parse-options.h b/parse-options.h\nindex c71e9da..2ddb26f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -18,7 +18,8 @@ enum parse_opt_type {\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n \tOPTION_LOWLEVEL_CALLBACK,\n-\tOPTION_FILENAME\n+\tOPTION_FILENAME,\n+\tOPTION_ULONG\n };\n \n enum parse_opt_flags {\n@@ -129,6 +130,8 @@ struct option {\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n+#define OPT_ULONG(s, l, v, h)       { OPTION_ULONG, (s), (l), (v), N_(\"n\"), \\\n+\t\t\t\t      (h), PARSE_OPT_NONEG }\n #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n \t\t\t\t    { OPTION_CALLBACK, (s), (l), (v), (a), \\\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ecb7417..55b3dba 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -19,6 +19,8 @@ usage: test-parse-options <options>\n \n     -i, --integer <n>     get a integer\n     -j <n>                get a integer, too\n+    -u, --unsigned-long <n>\n+                          get an unsigned long\n     --set23               set integer to 23\n     -t <time>             get timestamp of <time>\n     -L, --length <str>    get length of <str>\n@@ -58,6 +60,7 @@ mv expect expect.err\n cat >expect.template <<EOF\n boolean: 0\n integer: 0\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -132,9 +135,32 @@ test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear\n \n test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n+test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n+\n+test_expect_success 'OPT_ULONG() simple' '\n+\tcheck \"unsigned long:\" 2345678 -u 2345678\n+'\n+\n+test_expect_success 'OPT_ULONG() kilo' '\n+\tcheck \"unsigned long:\" 239616 -u 234k\n+'\n+\n+test_expect_success 'OPT_ULONG() mega' '\n+\tcheck \"unsigned long:\" 104857600 -u 100m\n+'\n+\n+test_expect_success 'OPT_ULONG() giga' '\n+\tcheck \"unsigned long:\" 1073741824 -u 1g\n+'\n+\n+test_expect_success 'OPT_ULONG() 3giga' '\n+\tcheck \"unsigned long:\" 3221225472 -u 3g\n+'\n+\n cat > expect << EOF\n boolean: 2\n integer: 1729\n+unsigned long: 16384\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -145,7 +171,7 @@ file: prefix/my.file\n EOF\n \n test_expect_success 'short options' '\n-\ttest-parse-options -s123 -b -i 1729 -b -vv -n -F my.file \\\n+\ttest-parse-options -s123 -b -i 1729 -u 16k -b -vv -n -F my.file \\\n \t> output 2> output.err &&\n \ttest_cmp expect output &&\n \ttest_must_be_empty output.err\n@@ -156,6 +182,7 @@ test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n cat > expect << EOF\n boolean: 2\n integer: 1729\n+unsigned long: 16384\n timestamp: 0\n string: 321\n abbrev: 10\n@@ -166,9 +193,10 @@ file: prefix/fi.le\n EOF\n \n test_expect_success 'long options' '\n-\ttest-parse-options --boolean --integer 1729 --boolean --string2=321 \\\n-\t\t--verbose --verbose --no-dry-run --abbrev=10 --file fi.le\\\n-\t\t--obsolete > output 2> output.err &&\n+\ttest-parse-options --boolean --integer 1729 --unsigned-long 16k \\\n+\t\t--boolean --string2=321 --verbose --verbose --no-dry-run \\\n+\t\t--abbrev=10 --file fi.le --obsolete \\\n+\t\t> output 2> output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -182,6 +210,7 @@ test_expect_success 'missing required value' '\n cat > expect << EOF\n boolean: 1\n integer: 13\n+unsigned long: 0\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -204,6 +233,7 @@ test_expect_success 'intermingled arguments' '\n cat > expect << EOF\n boolean: 0\n integer: 2\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -232,6 +262,7 @@ test_expect_success 'ambiguously abbreviated option' '\n cat > expect << EOF\n boolean: 0\n integer: 0\n+unsigned long: 0\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -270,6 +301,7 @@ test_expect_success 'detect possible typos' '\n cat > expect <<EOF\n boolean: 0\n integer: 0\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -289,6 +321,7 @@ test_expect_success 'keep some options as arguments' '\n cat > expect <<EOF\n boolean: 0\n integer: 0\n+unsigned long: 0\n timestamp: 1\n string: (not set)\n abbrev: 7\n@@ -310,6 +343,7 @@ cat > expect <<EOF\n Callback: \"four\", 0\n boolean: 5\n integer: 4\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -338,6 +372,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '\n cat > expect <<EOF\n boolean: 1\n integer: 23\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -362,6 +397,7 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '\n cat > expect <<EOF\n boolean: 6\n integer: 0\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -392,6 +428,7 @@ test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n cat > expect <<EOF\n boolean: 0\n integer: 12345\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -410,6 +447,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '\n cat >expect <<EOF\n boolean: 0\n integer: 0\n+unsigned long: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 7c492cf..e592d7e 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -4,6 +4,7 @@\n \n static int boolean = 0;\n static int integer = 0;\n+static unsigned long unsigned_long = 0;\n static unsigned long timestamp;\n static int abbrev = 7;\n static int verbose = 0, dry_run = 0, quiet = 0;\n@@ -48,6 +49,7 @@ int main(int argc, char **argv)\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER('i', \"integer\", &integer, \"get a integer\"),\n \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n+\t\tOPT_ULONG('u', \"unsigned-long\", &unsigned_long, \"get an unsigned long\"),\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n \t\tOPT_DATE('t', NULL, &timestamp, \"get timestamp of <time>\"),\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n@@ -83,6 +85,7 @@ int main(int argc, char **argv)\n \n \tprintf(\"boolean: %d\\n\", boolean);\n \tprintf(\"integer: %d\\n\", integer);\n+\tprintf(\"unsigned long: %lu\\n\", unsigned_long);\n \tprintf(\"timestamp: %lu\\n\", timestamp);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n-- \n2.4.0.53.g8440f74\n"},{"id":"264289","messageId":"1434705059-2793-4-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434705059-2793-1-git-send-email-charles@hashpling.org","subject":"[PATCH 3/3] Add filter-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T09:10:59Z","receivedAt":"2015-06-19T09:10:59Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nfilter-objects is a command to scan all objects in the object database\nfor the repository and print the ids of those which match the given\ncriteria.\n\nThe current supported criteria are object type and the minimum size of\nthe object.\n\nThe guiding use case is to scan repositories quickly for large objects\nwhich may cause performance issues for users. The list of objects can\nthen be used to guide some future remediating action.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n Documentation/git-filter-objects.txt | 38 +++++++++++++++++++\n Makefile                             |  1 +\n builtin.h                            |  1 +\n builtin/filter-objects.c             | 73 ++++++++++++++++++++++++++++++++++++\n git.c                                |  1 +\n t/t8100-filter-objects.sh            | 67 +++++++++++++++++++++++++++++++++\n 6 files changed, 181 insertions(+)\n create mode 100644 Documentation/git-filter-objects.txt\n create mode 100644 builtin/filter-objects.c\n create mode 100755 t/t8100-filter-objects.sh\n\ndiff --git a/Documentation/git-filter-objects.txt b/Documentation/git-filter-objects.txt\nnew file mode 100644\nindex 0000000..c10ca01\n--- /dev/null\n+++ b/Documentation/git-filter-objects.txt\n@@ -0,0 +1,38 @@\n+git-filter-objects(1)\n+=====================\n+\n+NAME\n+----\n+git-filter-objects - Scan through all objects in the repository and print those\n+matching a given filter\n+\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git filter-objects' [-t <type> | --type=<type>] [--min-size=<size>]\n+\t[-v|--verbose]\n+\n+DESCRIPTION\n+-----------\n+Scans all objects in a repository - including any unreachable objects - and\n+print out the ids of all matching objects.  If `--verbose` is specified then\n+the object type and size is printed out as well as its id.\n+\n+OPTIONS\n+-------\n+-t::\n+--type::\n+\tOnly list objects whose type matches <type>.\n+\n+--min-size::\n+\tOnly list objects whose size exceeds <size> bytes.\n+\n+-v::\n+--verbose::\n+\tOutput in the followin format instead of just printing object ids:\n+\t<sha1> SP <type> SP <size>\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 149f1c7..a7c017f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -842,6 +842,7 @@ BUILTIN_OBJS += builtin/diff.o\n BUILTIN_OBJS += builtin/fast-export.o\n BUILTIN_OBJS += builtin/fetch-pack.o\n BUILTIN_OBJS += builtin/fetch.o\n+BUILTIN_OBJS += builtin/filter-objects.o\n BUILTIN_OBJS += builtin/fmt-merge-msg.o\n BUILTIN_OBJS += builtin/for-each-ref.o\n BUILTIN_OBJS += builtin/fsck.o\ndiff --git a/builtin.h b/builtin.h\nindex b87df70..5a15693 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -62,6 +62,7 @@ extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);\n extern int cmd_fast_export(int argc, const char **argv, const char *prefix);\n extern int cmd_fetch(int argc, const char **argv, const char *prefix);\n extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);\n+extern int cmd_filter_objects(int argc, const char **argv, const char *prefix);\n extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix);\n extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_format_patch(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/filter-objects.c b/builtin/filter-objects.c\nnew file mode 100644\nindex 0000000..c40d621\n--- /dev/null\n+++ b/builtin/filter-objects.c\n@@ -0,0 +1,73 @@\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"revision.h\"\n+#include \"parse-options.h\"\n+\n+#include <stdio.h>\n+\n+static int req_type;\n+static unsigned long min_size;\n+static int verbose;\n+\n+static int check_object(const unsigned char *sha1)\n+{\n+\tunsigned long size;\n+\tint type = sha1_object_info(sha1, &size);\n+\n+\tif (type < 0)\n+\t\treturn -1;\n+\n+\tif (size >= min_size && (!req_type || type == req_type)) {\n+\t\tif (verbose)\n+\t\t\tprintf(\"%s %s %lu\\n\", sha1_to_hex(sha1), typename(type), size);\n+\t\telse\n+\t\t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int check_loose_object(const unsigned char *sha1,\n+\t\t\t      const char *path,\n+\t\t\t      void *data)\n+{\n+\treturn check_object(sha1);\n+}\n+\n+static int check_packed_object(const unsigned char *sha1,\n+\t\t\t       struct packed_git *pack,\n+\t\t\t       uint32_t pos,\n+\t\t\t       void *data)\n+{\n+\treturn check_object(sha1);\n+}\n+\n+static char *opt_type;\n+static struct option builtin_filter_objects_options[] = {\n+\tOPT_ULONG(0, \"min-size\", &min_size, \"minimum size of object to show\"),\n+\tOPT_STRING('t', \"type\", &opt_type, NULL, \"type of objects to show\"),\n+\tOPT__VERBOSE(&verbose, \"show object type and size\"),\n+\tOPT_END()\n+};\n+\n+int cmd_filter_objects(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct packed_git *p;\n+\n+\targc = parse_options(argc, argv, prefix, builtin_filter_objects_options,\n+\t\t\t     NULL, 0);\n+\n+\tif (opt_type)\n+\t\treq_type = type_from_string(opt_type);\n+\n+\tfor_each_loose_object(check_loose_object, NULL, 0);\n+\n+\tprepare_packed_git();\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\topen_pack_index(p);\n+\t}\n+\n+\tfor_each_packed_object(check_packed_object, NULL, 0);\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex 44374b1..4c87afd 100644\n--- a/git.c\n+++ b/git.c\n@@ -403,6 +403,7 @@ static struct cmd_struct commands[] = {\n \t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t{ \"fetch\", cmd_fetch, RUN_SETUP },\n \t{ \"fetch-pack\", cmd_fetch_pack, RUN_SETUP },\n+\t{ \"filter-objects\", cmd_filter_objects, RUN_SETUP },\n \t{ \"fmt-merge-msg\", cmd_fmt_merge_msg, RUN_SETUP },\n \t{ \"for-each-ref\", cmd_for_each_ref, RUN_SETUP },\n \t{ \"format-patch\", cmd_format_patch, RUN_SETUP },\ndiff --git a/t/t8100-filter-objects.sh b/t/t8100-filter-objects.sh\nnew file mode 100755\nindex 0000000..4b0137b\n--- /dev/null\n+++ b/t/t8100-filter-objects.sh\n@@ -0,0 +1,67 @@\n+#!/bin/sh\n+\n+test_description='git filter-objects'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello, world >file &&\n+\tgit add file &&\n+\tgit commit -m \"initial\"\n+'\n+\n+test_expect_success 'filter by type' '\n+\tgit rev-parse HEAD >expected &&\n+\tgit filter-objects -t commit >result &&\n+\ttest_cmp expected result &&\n+\tgit rev-parse HEAD:file >expected &&\n+\tgit filter-objects -t blob >result &&\n+\ttest_cmp expected result &&\n+\tgit rev-parse HEAD^{tree} >expected &&\n+\tgit filter-objects -t tree >result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'filter by type after pack' '\n+\tgit repack -Ad &&\n+\tgit rev-parse HEAD >expected &&\n+\tgit filter-objects -t commit >result &&\n+\ttest_cmp expected result &&\n+\tgit rev-parse HEAD:file >expected &&\n+\tgit filter-objects -t blob >result &&\n+\ttest_cmp expected result &&\n+\tgit rev-parse HEAD^{tree} >expected &&\n+\tgit filter-objects -t tree >result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'verbose output' '\n+\techo $(git rev-parse HEAD) commit $(git cat-file -s HEAD) >expected &&\n+\tgit filter-objects -v -t commit >result &&\n+\ttest_cmp expected result &&\n+\techo $(git rev-parse HEAD:file) blob $(git cat-file -s HEAD:file) >expected &&\n+\tgit filter-objects -v -t blob >result &&\n+\ttest_cmp expected result &&\n+\techo $(git rev-parse HEAD^{tree}) tree $(git cat-file -s HEAD^{tree}) >expected &&\n+\tgit filter-objects -v -t tree >result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'filter on size' '\n+\tgit commit -F - --allow-empty <<-\\EOF &&\n+\t\tThis is a reasonably long commit message\n+\n+\t\tIt is designed to make sure that we create an object\n+\t\tthat is substantially larger than all the others.\n+\n+\t\tOur test file blob is a few bytes, our tree is similarly\n+\t\tsmall and our first commit is not too big.\n+\n+\t\tThis message alone is about 300 characters and a sample\n+\t\tcommit from it has been measured at 562 bytes.\n+\tEOF\n+\tgit rev-parse HEAD >expected &&\n+\tgit filter-objects --min-size=500 >result &&\n+\ttest_cmp expected result\n+'\n+\n+test_done\n-- \n2.4.0.53.g8440f74\n"},{"id":"264292","messageId":"20150619101010.GA15802@peff.net","threadId":"39677","inReplyTo":"1434705059-2793-4-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-19T10:10:10Z","receivedAt":"2015-06-19T10:10:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2015 at 10:10:59AM +0100, Charles Bailey wrote:\n\n> filter-objects is a command to scan all objects in the object database\n> for the repository and print the ids of those which match the given\n> criteria.\n> \n> The current supported criteria are object type and the minimum size of\n> the object.\n> \n> The guiding use case is to scan repositories quickly for large objects\n> which may cause performance issues for users. The list of objects can\n> then be used to guide some future remediating action.\n\nI've had to perform this exact same task. You can already do the\n\"filtering\" part pretty easily and efficiently with cat-file and a perl\nscript, like:\n\n  magically_generate_all_objects |\n  git cat-file --batch-check='%(objectsize) %(objectname)' |\n  perl -alne 'print $F[1] if $F[0] > 1234'\n\nThat's not as friendly as your filter-objects, but it's a lot more\nflexible (since you can ask cat-file for all sorts of information).\n\nObviously I've glossed over the \"how to get a list of objects\" part.\nIf you truly want all objects (not just reachable ones), or if \"rev-list\n--objects\" is too slow, the best way is:\n\n  objects() {\n    # loose objects\n    for i in objects/??/*; do\n       echo $i\n    done |\n    sed 's,objects/\\(..\\)/,\\1,'\n\n    # packed objects\n    for i in objects/pack/*.idx; do\n      git show-index <$i\n    done |\n    cut -d' ' -f2\n  }\n\nCertainly I'm not opposed to doing something less horrible there (and I\nam happy to see my for_each_*_object interface getting more callers!).\nI kind of wonder if we should make \"all objects, reachable or not\" an\noption for rev-list. I'm not sure if it would choke on adding them all\nto the \"pending\" list, though; it's not really made for that. But it\nwould enable neat things like:\n\n  git rev-list --all-the-objects --not --all\n\nto show you what's unreachable.\n\n-Peff\n"},{"id":"264295","messageId":"20150619103324.GA4093@hashpling.org","threadId":"39677","inReplyTo":"20150619101010.GA15802@peff.net","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T10:33:24Z","receivedAt":"2015-06-19T10:33:24Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Fri, Jun 19, 2015 at 06:10:10AM -0400, Jeff King wrote:\n> On Fri, Jun 19, 2015 at 10:10:59AM +0100, Charles Bailey wrote:\n> \n> > filter-objects is a command to scan all objects in the object database\n> > for the repository and print the ids of those which match the given\n> > criteria.\n> > \n> > The current supported criteria are object type and the minimum size of\n> > the object.\n> > \n> > The guiding use case is to scan repositories quickly for large objects\n> > which may cause performance issues for users. The list of objects can\n> > then be used to guide some future remediating action.\n> \n> I've had to perform this exact same task. You can already do the\n> \"filtering\" part pretty easily and efficiently with cat-file and a perl\n> script, like:\n> \n>   magically_generate_all_objects |\n>   git cat-file --batch-check='%(objectsize) %(objectname)' |\n>   perl -alne 'print $F[1] if $F[0] > 1234'\n> \n> That's not as friendly as your filter-objects, but it's a lot more\n> flexible (since you can ask cat-file for all sorts of information).\n> \n> Obviously I've glossed over the \"how to get a list of objects\" part.\n> If you truly want all objects (not just reachable ones), or if \"rev-list\n> --objects\" is too slow [...]\n\nSo, yes, performance is definitely an issue and I could have called this\ncommand \"git magically-generate-all-object-for-scripts\" but then, as it\nwas so easy to provide exactly the filtering that I was looking for in\nthe C code, I thought I would do that as well and then \"filter-objects\"\n(\"filter-all-objects\"?) seemed like a better name.\n\nIt's about an order of magnitude faster on the systems I've checked to\ndo a parameterless filter-objects then rev-list --all --objects,\nalthough I understand they do different things.\n\nI am also thinking about another piece that answers the question: \"which\ncommits introduce any of (or the first of) this list of objects?\". This\ncan be done by parseing a diff --raw for commits but I think it should\nbe possible to do this faster, too.\n\nCharles.\n"},{"id":"264297","messageId":"20150619105210.GA29755@peff.net","threadId":"39677","inReplyTo":"20150619103324.GA4093@hashpling.org","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-19T10:52:11Z","receivedAt":"2015-06-19T10:52:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2015 at 11:33:24AM +0100, Charles Bailey wrote:\n\n> > Obviously I've glossed over the \"how to get a list of objects\" part.\n> > If you truly want all objects (not just reachable ones), or if \"rev-list\n> > --objects\" is too slow [...]\n> \n> So, yes, performance is definitely an issue and I could have called this\n> command \"git magically-generate-all-object-for-scripts\" but then, as it\n> was so easy to provide exactly the filtering that I was looking for in\n> the C code, I thought I would do that as well and then \"filter-objects\"\n> (\"filter-all-objects\"?) seemed like a better name.\n\nRight, my point was only that it works for _your_ particular filter, but\nit would be nice to have something more general. And we already have\n\"cat-file --batch-check\". IOW, I think I would prefer the \"magical\" form\nbecause it's a better scripting building block. As you note,\n\"filter-objects\" without any filters is exactly that. Your 10 extra\nlines of C code are not exactly bloat, but I just wonder if other people\nwill find it all that useful.\n\n> It's about an order of magnitude faster on the systems I've checked to\n> do a parameterless filter-objects then rev-list --all --objects,\n> although I understand they do different things.\n\nRight, it's the object-opening and hash lookups that kill you in\n\"rev-list\", because it's actually walking the graph.\n\n> I am also thinking about another piece that answers the question: \"which\n> commits introduce any of (or the first of) this list of objects?\". This\n> can be done by parseing a diff --raw for commits but I think it should\n> be possible to do this faster, too.\n\nIf you care about \"introduce\", I think you have to traverse and do the\ndiffs. If you only care about \"contains\" (for example, because you want\nto know which path the blob is found at), you can find trees which\nmention it, then trees which mention that tree, and so on. I think that\nends up slower in practice, though.\n\nI have patches that implement a \"rev-list --find=$sha1\", which sets a\nbit on $sha1 and then traverses with --objects until we find it (or\nthem; you can specify multiple). It's pretty straightforward, but it\ndoes cost as much as \"git rev-list --objects\" in the worst case. Let me\nknow if you're interested and I can clean it up and post it.\n\n-Peff\n"},{"id":"264299","messageId":"20150619105228.GR18226@serenity.lan","threadId":"39677","inReplyTo":"20150619103324.GA4093@hashpling.org","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-06-19T10:52:28Z","receivedAt":"2015-06-19T10:52:28Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jun 19, 2015 at 11:33:24AM +0100, Charles Bailey wrote:\n> So, yes, performance is definitely an issue and I could have called this\n> command \"git magically-generate-all-object-for-scripts\" but then, as it\n> was so easy to provide exactly the filtering that I was looking for in\n> the C code, I thought I would do that as well and then \"filter-objects\"\n> (\"filter-all-objects\"?) seemed like a better name.\n\nBy analogy with \"git filter-branch\", I don't think \"filter-objects\" is a\ngood name here.  My preference would be \"ls-objects\".\n"},{"id":"264300","messageId":"69265919.629073.1434711805415.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39677","inReplyTo":"1434705059-2793-3-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-19T11:03:25Z","receivedAt":"2015-06-19T11:03:25Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Charles Bailey <cbailey32@bloomberg.net> writes:\n> test_expect_success 'long options' '\n> - test-parse-options --boolean --integer 1729 --boolean --string2=321 \\\n> - --verbose --verbose --no-dry-run --abbrev=10 --file fi.le\\\n> - --obsolete > output 2> output.err &&\n> + test-parse-options --boolean --integer 1729 --unsigned-long 16k \\\n> + --boolean --string2=321 --verbose --verbose --no-dry-run \\\n> + --abbrev=10 --file fi.le --obsolete \\\n> + > output 2> output.err &&\n> test_must_be_empty output.err &&\n> test_cmp expect output\n> '\n\nIt's trivial matter but the line:\n> + > output 2> output.err &&\nshould be written:\n> + >output 2>output.err &&\n\nIt was incorrectly written before but since \nyou are modifying the line, it might be a \ngood thing to change it now.\n\nRémi\n"},{"id":"264301","messageId":"20150619110408.GA4513@hashpling.org","threadId":"39677","inReplyTo":"20150619105228.GR18226@serenity.lan","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T11:04:08Z","receivedAt":"2015-06-19T11:04:08Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Fri, Jun 19, 2015 at 11:52:28AM +0100, John Keeping wrote:\n> On Fri, Jun 19, 2015 at 11:33:24AM +0100, Charles Bailey wrote:\n> > So, yes, performance is definitely an issue and I could have called this\n> > command \"git magically-generate-all-object-for-scripts\" but then, as it\n> > was so easy to provide exactly the filtering that I was looking for in\n> > the C code, I thought I would do that as well and then \"filter-objects\"\n> > (\"filter-all-objects\"?) seemed like a better name.\n> \n> By analogy with \"git filter-branch\", I don't think \"filter-objects\" is a\n> good name here.  My preference would be \"ls-objects\".\n\nI like that because it emphasises why I wrote it, the very basic\nfiltering is a nice additional feature.\n"},{"id":"264302","messageId":"20150619110604.GA4562@hashpling.org","threadId":"39677","inReplyTo":"69265919.629073.1434711805415.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-19T11:06:04Z","receivedAt":"2015-06-19T11:06:04Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Fri, Jun 19, 2015 at 01:03:25PM +0200, Remi Galan Alfonso wrote:\n> \n> It's trivial matter but the line:\n> > + > output 2> output.err &&\n> should be written:\n> > + >output 2>output.err &&\n> \n> It was incorrectly written before but since \n> you are modifying the line, it might be a \n> good thing to change it now.\n\nYes, I can fold this in. I just changed the wrapping and didn't spot\nthis style error.\n"},{"id":"264347","messageId":"xmqq7fqza8bo.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"1434705059-2793-3-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T17:58:51Z","receivedAt":"2015-06-19T17:58:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> diff --git a/parse-options.h b/parse-options.h\n> index c71e9da..2ddb26f 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -18,7 +18,8 @@ enum parse_opt_type {\n>  \tOPTION_INTEGER,\n>  \tOPTION_CALLBACK,\n>  \tOPTION_LOWLEVEL_CALLBACK,\n> -\tOPTION_FILENAME\n> +\tOPTION_FILENAME,\n> +\tOPTION_ULONG\n>  };\n\nPlease place it immediately after INTEGER, as they are conceptually\nsiblings---group similar things together.\n\n>  \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n>  \t\treturn 0;\n>  \n> +\tcase OPTION_ULONG:\n\nThis one is placed right from that point of view ;-)\n\n> +\t\tif (unset) {\n> +\t\t\t*(unsigned long *)opt->value = 0;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n> +\t\t\t*(unsigned long *)opt->value = opt->defval;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tif (get_arg(p, opt, flags, &arg))\n> +\t\t\treturn -1;\n> +\t\tif (!git_parse_ulong(arg, opt->value))\n> +\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n\nThis used to be:\n\n> -\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n> -\t\t    arg, opt->long_name);\n\nbut opterror() talks about which option, so there is no information\nloss by losing \"for option %s\" from here.  That means there is only\none difference for pack-objects:\n\n    $ git pack-objects --max-pack-size=1T\n    fatal: unable to parse value '1T' for option max-pack-size\n    $ ./git pack-objects --max-pack-size=1T\n    error: option `max-pack-size' expects a numerical value\n    usage: git pack-objects --stdout [options...\n    ... 30 more lines omitted ...\n\nEh, make that two:\n\n * We no longer say what value we did not like.  The user presumably\n   knows what he typed, so this is only a minor loss.\n\n * We used to stop without giving \"usage\", as the error message was\n   specific enough.  We now spew descriptions on other options\n   unrelated to the specific error the user may want to concentrate\n   on.  Perhaps this is a minor regression.\n\nI wonder if \"expects a numerical value\" is the best way to say this.\n\nPonder:\n\n - we do not take \"4.8\"\n - we do not take \"-4\".\n - people may not realize, from \"numerical\", that we take \"5M\".\n\nExcept for the minor nits above, I think this is a good change.\n\nThis is a totally unrelated tangent that does not have to be part of\nyour series, but we probably should take \"4.8M\"; I do not think we\ncurrently do.\n\nOh, and perhaps 1T, too.\n"},{"id":"264351","messageId":"xmqqsi9n8sef.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"20150619105210.GA29755@peff.net","subject":"Re: [PATCH 3/3] Add filter-objects command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T18:28:08Z","receivedAt":"2015-06-19T18:28:08Z","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> Right, my point was only that it works for _your_ particular\n> filter, but it would be nice to have something more general. And\n> we already have \"cat-file --batch-check\". IOW, I think I would\n> prefer the \"magical\" form because it's a better scripting building\n> block. As you note, \"filter-objects\" without any filters is\n> exactly that. Your 10 extra lines of C code are not exactly bloat,\n> but I just wonder if other people will find it all that useful.\n\nYup.  I do not mind a fast \"enumerate all objects\" but I suspect\nthat making \"all\" fast may turn out to be not so great a trade-off\nafter all, as you would need more work on the \"now we have all\ncoming from our input, let's filter with this and that criteria\"\ndownstream in general cases.  Graph-based filtering e.g. \"Oops, here\nis our whole customer database committed by mistake--which branch\nshould we rewrite to nuke?\" inherently is much more costly to do in\nthe downstream that essentially has to reconstruct the graph around\ninteresting parts of the history, and is better done by \"enumerate\"\nphase spending time to do the actual graph traversal.\n\nAnd \"filter-anything\" should not be the name for \"enumerate\" command\nthat comes on the upstream of a pipe.  You usually call what is\ndownstream of a pipe \"a filter\".\n"},{"id":"264352","messageId":"xmqqoakb8sdc.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"1434705059-2793-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 1/3] Correct test-parse-options to handle negative ints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T18:28:47Z","receivedAt":"2015-06-19T18:28:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> From: Charles Bailey <cbailey32@bloomberg.net>\n>\n> Fix the printf specification to treat 'integer' as the signed type that\n> it is and add a test that checks that we parse negative option\n> arguments.\n>\n> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>\n> ---\n\nMakes sense.  Will queue.\n\n>  t/t0040-parse-options.sh | 2 ++\n>  test-parse-options.c     | 2 +-\n>  2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index b044785..ecb7417 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -151,6 +151,8 @@ test_expect_success 'short options' '\n>  \ttest_must_be_empty output.err\n>  '\n>  \n> +test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n> +\n>  cat > expect << EOF\n>  boolean: 2\n>  integer: 1729\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 5dabce6..7c492cf 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -82,7 +82,7 @@ int main(int argc, char **argv)\n>  \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n>  \n>  \tprintf(\"boolean: %d\\n\", boolean);\n> -\tprintf(\"integer: %u\\n\", integer);\n> +\tprintf(\"integer: %d\\n\", integer);\n>  \tprintf(\"timestamp: %lu\\n\", timestamp);\n>  \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n>  \tprintf(\"abbrev: %d\\n\", abbrev);\n"},{"id":"264354","messageId":"xmqqh9q38ruq.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"xmqq7fqza8bo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T18:39:57Z","receivedAt":"2015-06-19T18:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Except for the minor nits above, I think this is a good change.\n\nOh, I forgot to mention one thing.  I am not sure if this should be\ncalled ULONG.  \"unsigned long\"-ness is not the most important part\nof this thing from the end-user's point of view, and also from the\npoint of view of the programmer who supports end-users by using this\nnew feature.\n\nIt is \"unlike OPT_INTEGER, the user can specify it as a human\nreadble scaled quantity\" that is the reason to use this new thing.\nI think we discussed to introduce OPT_HUMINT (HUM stands for HUMAN,\nobviously) or some name like that a few years ago to do exactly\nthis, but that is not a great name, either.\n\nI was tempted to suggest a name that has \"size\" in it, but because\nplaces that we may conceivably want to use it in the future would be\nto specify:\n\n - sizes, e.g. \"split the packfiles into 4.3G chunks\".\n\n - counts, e.g. \"show me the most recent 2k commits\".\n\n - bandwidth, e.g. \"limit the transfer to consume at most 2M bps\".\n\nwhich is not limited to size, it is not a very good idea, either.\n\nOPT_SCALED_ULONG(), or something with \"scaled\" in its name, perhaps?\n"},{"id":"264355","messageId":"558463B4.3080904@gmail.com","threadId":"39677","inReplyTo":"xmqq7fqza8bo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2015-06-19T18:47:16Z","receivedAt":"2015-06-19T18:47:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2015-06-19 o 19:58, Junio C Hamano pisze:\n> Charles Bailey <charles@hashpling.org> writes: \n[...]\n>> +\t\tif (!git_parse_ulong(arg, opt->value))\n>> +\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n> \n> This used to be:\n> \n>> -\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n>> -\t\t    arg, opt->long_name);\n> \n> but opterror() talks about which option, so there is no information\n> loss by losing \"for option %s\" from here.  That means there is only\n> one difference for pack-objects:\n> \n>     $ git pack-objects --max-pack-size=1T\n>     fatal: unable to parse value '1T' for option max-pack-size\n>     $ ./git pack-objects --max-pack-size=1T\n>     error: option `max-pack-size' expects a numerical value\n>     usage: git pack-objects --stdout [options...\n>     ... 30 more lines omitted ...\n> \n> Eh, make that two:\n> \n>  * We no longer say what value we did not like.  The user presumably\n>    knows what he typed, so this is only a minor loss.\n\nWell, in this case this is not a problem, but for longer commandline\ninvocation it might be hard to find the exact argument among all the\noptions (though I don't think there is any integer-accepting option\nthat can be repeated).\n> \n>  * We used to stop without giving \"usage\", as the error message was\n>    specific enough.  We now spew descriptions on other options\n>    unrelated to the specific error the user may want to concentrate\n>    on.  Perhaps this is a minor regression.\n> \n> I wonder if \"expects a numerical value\" is the best way to say this.\n\nIs \"expects numerical value\" easier to understand than \"unable to\nparse value\"?\n \n> Ponder:\n> \n>  - we do not take \"4.8\"\n\n   - we won't take locale specific \"4,8\" (for some locales)\n\n   - \"4O\" is not numerical... \"40\" is\n\n>  - we do not take \"-4\".\n>  - people may not realize, from \"numerical\", that we take \"5M\".\n> \n> Except for the minor nits above, I think this is a good change.\n> \n> This is a totally unrelated tangent that does not have to be part of\n> your series, but we probably should take \"4.8M\"; I do not think we\n> currently do.\n> \n> Oh, and perhaps 1T, too.\n> \n"},{"id":"264405","messageId":"55858737.6070207@gmail.com","threadId":"39677","inReplyTo":"xmqqh9q38ruq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2015-06-20T15:31:03Z","receivedAt":"2015-06-20T15:31:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2015-06-19 o 20:39, Junio C Hamano pisze:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Except for the minor nits above, I think this is a good change.\n> \n> Oh, I forgot to mention one thing.  I am not sure if this should be\n> called ULONG.  \"unsigned long\"-ness is not the most important part\n> of this thing from the end-user's point of view, and also from the\n> point of view of the programmer who supports end-users by using this\n> new feature.\n> \n> It is \"unlike OPT_INTEGER, the user can specify it as a human\n> readble scaled quantity\" that is the reason to use this new thing.\n> I think we discussed to introduce OPT_HUMINT (HUM stands for HUMAN,\n> obviously) or some name like that a few years ago to do exactly\n> this, but that is not a great name, either.\n\nOn the output side it is often called --human-readable (e.g. du(1)),\nI don't know how it is called on input side (e.g. in 'dd' and friends).\n\n> I was tempted to suggest a name that has \"size\" in it, but because\n> places that we may conceivably want to use it in the future would be\n> to specify:\n> \n>  - sizes, e.g. \"split the packfiles into 4.3G chunks\".\n> \n>  - counts, e.g. \"show me the most recent 2k commits\".\n> \n>  - bandwidth, e.g. \"limit the transfer to consume at most 2M bps\".\n> \n> which is not limited to size, it is not a very good idea, either.\n> \n> OPT_SCALED_ULONG(), or something with \"scaled\" in its name, perhaps?\n\nOPT_HUMAN_READABLE_INTEGER() is probably out as too long? ;-P\n\n-- \nJakub Narębski\n\n \n"},{"id":"264407","messageId":"20150620165138.GA27488@hashpling.org","threadId":"39677","inReplyTo":"xmqq7fqza8bo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-20T16:51:39Z","receivedAt":"2015-06-20T16:51:39Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote:\n> Charles Bailey <charles@hashpling.org> writes:\n> \n> Please place it immediately after INTEGER, as they are conceptually\n> siblings---group similar things together.\n\nSorry, this is a bad habit from working on projects where changing the\nvalue of existing enum identifiers cause bad things.\n\n> This used to be:\n> \n> > -\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n> > -\t\t    arg, opt->long_name);\n> \n> but opterror() talks about which option, so there is no information\n> loss by losing \"for option %s\" from here.  That means there is only\n> one difference for pack-objects:\n> \n>     $ git pack-objects --max-pack-size=1T\n>     fatal: unable to parse value '1T' for option max-pack-size\n>     $ ./git pack-objects --max-pack-size=1T\n>     error: option `max-pack-size' expects a numerical value\n>     usage: git pack-objects --stdout [options...\n>     ... 30 more lines omitted ...\n> \n> Eh, make that two:\n> \n>  * We no longer say what value we did not like.  The user presumably\n>    knows what he typed, so this is only a minor loss.\n> \n>  * We used to stop without giving \"usage\", as the error message was\n>    specific enough.  We now spew descriptions on other options\n>    unrelated to the specific error the user may want to concentrate\n>    on.  Perhaps this is a minor regression.\n> \n> I wonder if \"expects a numerical value\" is the best way to say this.\n\nI was aware that I was changing the error reporting for max-pack-size\nand window-memory but thought that by going with the existing behaviour\nof OPT_INTEGER I'd be going with a more established pattern.\n\nThese observations also seem to apply to OPT_INTEGER handling. Would\nthis be something that we'd want to fix too?\n\nCurrently git package-objects --depth=5.5 prints:\n\n    error: option `depth' expects a numerical value\n    usage: git pack-objects --stdout [options...\n    [... many more lines omitted ...]\n\nObviously, changing this to skip the full usage report would affect many\nexisting commands.\n\nAlso, I preserved the PARSE_OPT_NONEG flag for OPT_ULONG but would this\never not make sense for an OPT_INTEGER option?\n"},{"id":"264409","messageId":"xmqqzj3ujmqm.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"20150620165138.GA27488@hashpling.org","subject":"Re: [PATCH 2/3] Move unsigned long option parsing out of pack-objects.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-20T17:47:13Z","receivedAt":"2015-06-20T17:47:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> On Fri, Jun 19, 2015 at 10:58:51AM -0700, Junio C Hamano wrote:\n>\n>> Eh, make that two:\n>> \n>>  * We no longer say what value we did not like.  The user presumably\n>>    knows what he typed, so this is only a minor loss.\n>> \n>>  * We used to stop without giving \"usage\", as the error message was\n>>    specific enough.  We now spew descriptions on other options\n>>    unrelated to the specific error the user may want to concentrate\n>>    on.  Perhaps this is a minor regression.\n>> \n>> I wonder if \"expects a numerical value\" is the best way to say this.\n>\n> I was aware that I was changing the error reporting for max-pack-size\n> and window-memory but thought that by going with the existing behaviour\n> of OPT_INTEGER I'd be going with a more established pattern.\n\nThat is OK.  I just wanted to see that design decision explicitly\nrecorded in the proposed log message.\n\n> Currently git package-objects --depth=5.5 prints:\n>\n>     error: option `depth' expects a numerical value\n>     usage: git pack-objects --stdout [options...\n>     [... many more lines omitted ...]\n\nInteresting.  I get this instead:\n\n    git: 'package-objects' is not a git command. See 'git --help'.\n\n;-)  Jokes aside...\n\n> Obviously, changing this to skip the full usage report would affect many\n> existing commands.\n\nYes and I wouldn't suggest changing that in the same commit that\nexposes the human-readable quantity parsing to parse-options API.\nThat is why I said \"Perhaps this is a minor regression\".  It is a\nchange in behaviour, and it may make it slightly worse, but on the\nother hand it makes it in line with other types of options, so it\nmay be OK.\n\nIf we wanted to teach commands to omit \"usage\" when parsing of a\nsingle option failed, we should be doing that consistently for\neverybody, not just to pack-objects, and that is outside the scope\nof this patch, I would think.\n\n\tSide note: Just to make it clear, regarding anything I say\n\tis \"outside the scope of this patch\", I am not asking you to\n\tdo them as follow-up patches (as a precondition to accept\n\tthis patch).  For that matter, I am not convinced myself\n\tthat some of them are even worth doing.  And I am not asking\n\tyou _not_ to do these changes, ever, either.  I am just\n\tasking you _not_ to do any of them in _this_ patch.\n\n> Also, I preserved the PARSE_OPT_NONEG flag for OPT_ULONG but would this\n> ever not make sense for an OPT_INTEGER option?\n\nIt depends on what \"git cmd --depth=4 --no-depth\" should do.  In any\ncase, changing OPT_INT would be a separate topic outside the scope\nof this patch, I think.\n\nMy gut feeling is that\n\n    git pack-objects --max-pack-size=20m --no-max-pack-size\n\nshould be usable as a way to countermand a pack size limit given\nearlier on the command line to make it unlimited, but that is\ndefinitely a separate topic outside the scope of this patch (whose\npurpose is to make an existing callback available to other callers).\n"},{"id":"264447","messageId":"1434911144-6781-1-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434705059-2793-1-git-send-email-charles@hashpling.org","subject":"Improvements to integer option parsing","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T18:25:42Z","receivedAt":"2015-06-21T18:25:42Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"This is a re-roll of the first two patches in my previous series which used to\ninclude \"filter-objects\" which is now a separate topic.\n\n[PATCH 1/2] Correct test-parse-options to handle negative ints\n\nThe first one has changed only in that I've moved the additional test to a more\nlogical place in the test file.\n\n[PATCH 2/2] Move unsigned long option parsing out of pack-objects.c\n\nI've made the following changes to the second commit:\n\n- renamed this OPT_MAGNITUDE to try and convey something that is\nboth unsigned and might benefit from a 'scale' suffix. I'm expecting\nmore discussion on the name!\n\n- fixed the enum ordering to put this close to OPT_INTEGER\n\n- added documentation to api-parse-options.txt\n\n- marginally improved the opterror message on failed parses\n\n- noted the change in behavior for the error messages generated for\npack-objects' --max-pack-size and --window-memory in the commit message\n"},{"id":"264448","messageId":"1434911144-6781-2-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434911144-6781-1-git-send-email-charles@hashpling.org","subject":"[PATCH 1/2] Correct test-parse-options to handle negative ints","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T18:25:43Z","receivedAt":"2015-06-21T18:25:43Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nFix the printf specification to treat 'integer' as the signed type that\nit is and add a test that checks that we parse negative option\narguments.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n t/t0040-parse-options.sh | 2 ++\n test-parse-options.c     | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b044785..372d521 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -132,6 +132,8 @@ test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear\n \n test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n+test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n+\n cat > expect << EOF\n boolean: 2\n integer: 1729\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 5dabce6..7c492cf 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -82,7 +82,7 @@ int main(int argc, char **argv)\n \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n \n \tprintf(\"boolean: %d\\n\", boolean);\n-\tprintf(\"integer: %u\\n\", integer);\n+\tprintf(\"integer: %d\\n\", integer);\n \tprintf(\"timestamp: %lu\\n\", timestamp);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n-- \n2.4.0.53.g8440f74\n"},{"id":"264449","messageId":"1434911144-6781-3-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434911144-6781-1-git-send-email-charles@hashpling.org","subject":"[PATCH 2/2] Move unsigned long option parsing out of pack-objects.c","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T18:25:44Z","receivedAt":"2015-06-21T18:25:44Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nThe unsigned long option parsing (including 'k'/'m'/'g' suffix parsing)\nis more widely applicable. Add support for OPT_MAGNITUDE to\nparse-options.h and change pack-objects.c use this support.\n\nThe error behavior on parse errors follows that of OPT_INTEGER.\nThe name of the option that failed to parse is reported with a brief\nmessage describing the expect format for the option argument and then\nthe full usage message for the command invoked.\n\nThis is differs from the previous behavior for OPT_ULONG used in\npack-objects for --max-pack-size and --window-memory which used to\ndisplay the value supplied in the error message and did not display the\nfull usage message.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n Documentation/technical/api-parse-options.txt |  6 ++++\n builtin/pack-objects.c                        | 25 +++------------\n parse-options.c                               | 17 ++++++++++\n parse-options.h                               |  3 ++\n t/t0040-parse-options.sh                      | 45 ++++++++++++++++++++++++---\n test-parse-options.c                          |  3 ++\n 6 files changed, 73 insertions(+), 26 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 1f2db31..525cb2f 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -168,6 +168,12 @@ There are some macros to easily define options:\n \tIntroduce an option with integer argument.\n \tThe integer is put into `int_var`.\n \n+`OPT_MAGNITUDE(short, long, &unsigned_long_var, description)`::\n+\tIntroduce an option with a size argument. The argument must be a\n+\tnon-negative integer and may include a suffix of 'k', 'm' or 'g' to\n+\tscale the provided value by 1024, 1024^2 or 1024^3 respectively.\n+\tThe scaled value is put into `unsigned_long_var`.\n+\n `OPT_DATE(short, long, &int_var, description)`::\n \tIntroduce an option with date argument, see `approxidate()`.\n \tThe timestamp is put into `int_var`.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 80fe8c7..62cc16d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2588,23 +2588,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_ulong(const struct option *opt,\n-\t\t\t      const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tdie(_(\"option %s does not accept negative form\"),\n-\t\t    opt->long_name);\n-\n-\tif (!git_parse_ulong(arg, opt->value))\n-\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n-\t\t    arg, opt->long_name);\n-\treturn 0;\n-}\n-\n-#define OPT_ULONG(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), \"n\", (h),\t\\\n-\t  PARSE_OPT_NONEG, option_parse_ulong }\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -2627,16 +2610,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 0, \"index-version\", NULL, N_(\"version[,offset]\"),\n \t\t  N_(\"write the pack index file in the specified idx format version\"),\n \t\t  0, option_parse_index_version },\n-\t\tOPT_ULONG(0, \"max-pack-size\", &pack_size_limit,\n-\t\t\t  N_(\"maximum size of each output pack file\")),\n+\t\tOPT_MAGNITUDE(0, \"max-pack-size\", &pack_size_limit,\n+\t\t\t      N_(\"maximum size of each output pack file\")),\n \t\tOPT_BOOL(0, \"local\", &local,\n \t\t\t N_(\"ignore borrowed objects from alternate object store\")),\n \t\tOPT_BOOL(0, \"incremental\", &incremental,\n \t\t\t N_(\"ignore packed objects\")),\n \t\tOPT_INTEGER(0, \"window\", &window,\n \t\t\t    N_(\"limit pack window by objects\")),\n-\t\tOPT_ULONG(0, \"window-memory\", &window_memory_limit,\n-\t\t\t  N_(\"limit pack window by memory in addition to object limit\")),\n+\t\tOPT_MAGNITUDE(0, \"window-memory\", &window_memory_limit,\n+\t\t\t      N_(\"limit pack window by memory in addition to object limit\")),\n \t\tOPT_INTEGER(0, \"depth\", &depth,\n \t\t\t    N_(\"maximum length of delta chain allowed in the resulting pack\")),\n \t\tOPT_BOOL(0, \"reuse-delta\", &reuse_delta,\ndiff --git a/parse-options.c b/parse-options.c\nindex 80106c0..101b649 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n \t\treturn 0;\n \n+\tcase OPTION_MAGNITUDE:\n+\t\tif (unset) {\n+\t\t\t*(unsigned long *)opt->value = 0;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n+\t\t\t*(unsigned long *)opt->value = opt->defval;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (get_arg(p, opt, flags, &arg))\n+\t\t\treturn -1;\n+\t\tif (!git_parse_ulong(arg, opt->value))\n+\t\t\treturn opterror(opt,\n+\t\t\t\t\"expects a integer value with an optional k/m/g suffix\",\n+\t\t\t\tflags);\n+\t\treturn 0;\n+\n \tdefault:\n \t\tdie(\"should not happen, someone must be hit on the forehead\");\n \t}\ndiff --git a/parse-options.h b/parse-options.h\nindex c71e9da..ca865f6 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -16,6 +16,7 @@ enum parse_opt_type {\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n+\tOPTION_MAGNITUDE,\n \tOPTION_CALLBACK,\n \tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n@@ -129,6 +130,8 @@ struct option {\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n+#define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \\\n+\t\t\t\t      N_(\"n\"), (h), PARSE_OPT_NONEG }\n #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n \t\t\t\t    { OPTION_CALLBACK, (s), (l), (v), (a), \\\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 372d521..9be6411 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -19,6 +19,7 @@ usage: test-parse-options <options>\n \n     -i, --integer <n>     get a integer\n     -j <n>                get a integer, too\n+    -m, --magnitude <n>   get a magnitude\n     --set23               set integer to 23\n     -t <time>             get timestamp of <time>\n     -L, --length <str>    get length of <str>\n@@ -58,6 +59,7 @@ mv expect expect.err\n cat >expect.template <<EOF\n boolean: 0\n integer: 0\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -134,9 +136,30 @@ test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'\n \n+test_expect_success 'OPT_MAGNITUDE() simple' '\n+\tcheck magnitude: 2345678 -m 2345678\n+'\n+\n+test_expect_success 'OPT_MAGNITUDE() kilo' '\n+\tcheck magnitude: 239616 -m 234k\n+'\n+\n+test_expect_success 'OPT_MAGNITUDE() mega' '\n+\tcheck magnitude: 104857600 -m 100m\n+'\n+\n+test_expect_success 'OPT_MAGNITUDE() giga' '\n+\tcheck magnitude: 1073741824 -m 1g\n+'\n+\n+test_expect_success 'OPT_MAGNITUDE() 3giga' '\n+\tcheck magnitude: 3221225472 -m 3g\n+'\n+\n cat > expect << EOF\n boolean: 2\n integer: 1729\n+magnitude: 16384\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -147,8 +170,8 @@ file: prefix/my.file\n EOF\n \n test_expect_success 'short options' '\n-\ttest-parse-options -s123 -b -i 1729 -b -vv -n -F my.file \\\n-\t> output 2> output.err &&\n+\ttest-parse-options -s123 -b -i 1729 -m 16k -b -vv -n -F my.file \\\n+\t>output 2>output.err &&\n \ttest_cmp expect output &&\n \ttest_must_be_empty output.err\n '\n@@ -156,6 +179,7 @@ test_expect_success 'short options' '\n cat > expect << EOF\n boolean: 2\n integer: 1729\n+magnitude: 16384\n timestamp: 0\n string: 321\n abbrev: 10\n@@ -166,9 +190,10 @@ file: prefix/fi.le\n EOF\n \n test_expect_success 'long options' '\n-\ttest-parse-options --boolean --integer 1729 --boolean --string2=321 \\\n-\t\t--verbose --verbose --no-dry-run --abbrev=10 --file fi.le\\\n-\t\t--obsolete > output 2> output.err &&\n+\ttest-parse-options --boolean --integer 1729 --magnitude 16k \\\n+\t\t--boolean --string2=321 --verbose --verbose --no-dry-run \\\n+\t\t--abbrev=10 --file fi.le --obsolete \\\n+\t\t>output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -182,6 +207,7 @@ test_expect_success 'missing required value' '\n cat > expect << EOF\n boolean: 1\n integer: 13\n+magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -204,6 +230,7 @@ test_expect_success 'intermingled arguments' '\n cat > expect << EOF\n boolean: 0\n integer: 2\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -232,6 +259,7 @@ test_expect_success 'ambiguously abbreviated option' '\n cat > expect << EOF\n boolean: 0\n integer: 0\n+magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n@@ -270,6 +298,7 @@ test_expect_success 'detect possible typos' '\n cat > expect <<EOF\n boolean: 0\n integer: 0\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -289,6 +318,7 @@ test_expect_success 'keep some options as arguments' '\n cat > expect <<EOF\n boolean: 0\n integer: 0\n+magnitude: 0\n timestamp: 1\n string: (not set)\n abbrev: 7\n@@ -310,6 +340,7 @@ cat > expect <<EOF\n Callback: \"four\", 0\n boolean: 5\n integer: 4\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -338,6 +369,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '\n cat > expect <<EOF\n boolean: 1\n integer: 23\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -362,6 +394,7 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '\n cat > expect <<EOF\n boolean: 6\n integer: 0\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -392,6 +425,7 @@ test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n cat > expect <<EOF\n boolean: 0\n integer: 12345\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n@@ -410,6 +444,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '\n cat >expect <<EOF\n boolean: 0\n integer: 0\n+magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 7c492cf..2c8c8f1 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -4,6 +4,7 @@\n \n static int boolean = 0;\n static int integer = 0;\n+static unsigned long magnitude = 0;\n static unsigned long timestamp;\n static int abbrev = 7;\n static int verbose = 0, dry_run = 0, quiet = 0;\n@@ -48,6 +49,7 @@ int main(int argc, char **argv)\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_INTEGER('i', \"integer\", &integer, \"get a integer\"),\n \t\tOPT_INTEGER('j', NULL, &integer, \"get a integer, too\"),\n+\t\tOPT_MAGNITUDE('m', \"magnitude\", &magnitude, \"get a magnitude\"),\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n \t\tOPT_DATE('t', NULL, &timestamp, \"get timestamp of <time>\"),\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n@@ -83,6 +85,7 @@ int main(int argc, char **argv)\n \n \tprintf(\"boolean: %d\\n\", boolean);\n \tprintf(\"integer: %d\\n\", integer);\n+\tprintf(\"magnitude: %lu\\n\", magnitude);\n \tprintf(\"timestamp: %lu\\n\", timestamp);\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n-- \n2.4.0.53.g8440f74\n"},{"id":"264451","messageId":"20150621183026.GA7199@hashpling.org","threadId":"39677","inReplyTo":"1434911144-6781-3-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T18:30:26Z","receivedAt":"2015-06-21T18:30:26Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Jun 21, 2015 at 07:25:44PM +0100, Charles Bailey wrote:\n> From: Charles Bailey <cbailey32@bloomberg.net>\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 80106c0..101b649 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p,\n>  \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n>  \t\treturn 0;\n>  \n> +\tcase OPTION_MAGNITUDE:\n> +\t\tif (unset) {\n> +\t\t\t*(unsigned long *)opt->value = 0;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n> +\t\t\t*(unsigned long *)opt->value = opt->defval;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tif (get_arg(p, opt, flags, &arg))\n> +\t\t\treturn -1;\n> +\t\tif (!git_parse_ulong(arg, opt->value))\n> +\t\t\treturn opterror(opt,\n> +\t\t\t\t\"expects a integer value with an optional k/m/g suffix\",\n> +\t\t\t\tflags);\n> +\t\treturn 0;\n> +\n\nSpotted after sending:\ns/expects a integer/expects an integer/\n"},{"id":"264454","messageId":"1434914431-7745-1-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434705059-2793-1-git-send-email-charles@hashpling.org","subject":"Fast enumeration of objects","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T19:20:30Z","receivedAt":"2015-06-21T19:20:30Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"This is a re-casting of my previous filter-objects command but without\nany of the filtering so it is now just \"list-all-objects\".\n\nI have retained the \"--verbose\" option which outputs the same format as\nthe default \"cat-file --batch-check\" as it provides a useful performance\ngain to filtering though \"cat-file\" if this basic information is all\nthat is needed.\n\nThe motivating use case is to enable a script to quickly scan a large\nnumber of repositories for any large objects.\n\nI performed some test timings of some different commands on a clone of\nthe Linux kernel which was completely packed.\n\n\t$ time git rev-list --all --objects |\n\t\tcut -d\" \" -f1 |\n\t\tgit cat-file --batch-check |\n\t\tawk '{if ($3 >= 512000) { print $1 }}' |\n\t\twc -l\n\t958\n\n\treal    0m30.823s\n\tuser    0m41.904s\n\tsys     0m7.728s\n\nlist-all-objects gives a significant improvement:\n\n\t$ time git list-all-objects |\n\t\tgit cat-file --batch-check |\n\t\tawk '{if ($3 >= 512000) { print $1 }}' |\n\t\twc -l\n\t958\n\n\treal    0m9.585s\n\tuser    0m10.820s\n\tsys     0m4.960s\n\nskipping the cat-filter filter is a lesser but still significant\nimprovement:\n\n\t$ time git list-all-objects -v |\n\t\tawk '{if ($3 >= 512000) { print $1 }}' |\n\t\twc -l\n\t958\n\n\treal    0m5.637s\n\tuser    0m6.652s\n\tsys     0m0.156s\n\nThe old filter-objects could do the size filter a little be faster, but\nnot by much:\n\n\t$ time git filter-objects --min-size=500k |\n\t\twc -l\n\t958\n\n\treal    0m4.564s\n\tuser    0m4.496s\n\tsys     0m0.064s\n"},{"id":"264455","messageId":"1434914431-7745-2-git-send-email-charles@hashpling.org","threadId":"39677","inReplyTo":"1434914431-7745-1-git-send-email-charles@hashpling.org","subject":"[PATCH] Add list-all-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-21T19:20:31Z","receivedAt":"2015-06-21T19:20:31Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nlist-all-objects is a command to print the ids of all objects in the\nobject database of a repository. It is designed as a low overhead\ninterface for scripts that want to analyse all objects but don't require\nthe ordering implied by a revision walk.\n\nIt will list all objects, loose and packed, and will include unreachable\nobjects.\n\nlist-all-objects is faster that \"rev-list --all --objects\" but there is\nno guarantee as to the order in which objects will be listed.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n Documentation/git-list-all-objects.txt | 29 +++++++++++++++\n Makefile                               |  1 +\n builtin.h                              |  1 +\n builtin/list-all-objects.c             | 64 ++++++++++++++++++++++++++++++++++\n git.c                                  |  1 +\n t/t8100-list-all-objects.sh            | 45 ++++++++++++++++++++++++\n 6 files changed, 141 insertions(+)\n create mode 100644 Documentation/git-list-all-objects.txt\n create mode 100644 builtin/list-all-objects.c\n create mode 100755 t/t8100-list-all-objects.sh\n\ndiff --git a/Documentation/git-list-all-objects.txt b/Documentation/git-list-all-objects.txt\nnew file mode 100644\nindex 0000000..5f28d41\n--- /dev/null\n+++ b/Documentation/git-list-all-objects.txt\n@@ -0,0 +1,29 @@\n+git-list-all-objects(1)\n+=======================\n+\n+NAME\n+----\n+git-list-all-objects - List all objects in the repository.\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git list-all-objects' [-v|--verbose]\n+\n+DESCRIPTION\n+-----------\n+List the ids of all objects in a repository, including any unreachable objects.\n+If `--verbose` is specified then the object's type and size is printed out as\n+well as its id.\n+\n+OPTIONS\n+-------\n+\n+-v::\n+--verbose::\n+\tOutput in the followin format instead of just printing object ids:\n+\t<sha1> SP <type> SP <size>\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 149f1c7..cf4f0c3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -853,6 +853,7 @@ BUILTIN_OBJS += builtin/help.o\n BUILTIN_OBJS += builtin/index-pack.o\n BUILTIN_OBJS += builtin/init-db.o\n BUILTIN_OBJS += builtin/interpret-trailers.o\n+BUILTIN_OBJS += builtin/list-all-objects.o\n BUILTIN_OBJS += builtin/log.o\n BUILTIN_OBJS += builtin/ls-files.o\n BUILTIN_OBJS += builtin/ls-remote.o\ndiff --git a/builtin.h b/builtin.h\nindex b87df70..112bafb 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -74,6 +74,7 @@ extern int cmd_help(int argc, const char **argv, const char *prefix);\n extern int cmd_index_pack(int argc, const char **argv, const char *prefix);\n extern int cmd_init_db(int argc, const char **argv, const char *prefix);\n extern int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);\n+extern int cmd_list_all_objects(int argc, const char **argv, const char *prefix);\n extern int cmd_log(int argc, const char **argv, const char *prefix);\n extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);\n extern int cmd_ls_files(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/list-all-objects.c b/builtin/list-all-objects.c\nnew file mode 100644\nindex 0000000..3b43b02\n--- /dev/null\n+++ b/builtin/list-all-objects.c\n@@ -0,0 +1,64 @@\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"revision.h\"\n+#include \"parse-options.h\"\n+\n+#include <stdio.h>\n+\n+static int verbose;\n+\n+static int print_object(const unsigned char *sha1)\n+{\n+\tif (verbose) {\n+\t\tunsigned long size;\n+\t\tint type = sha1_object_info(sha1, &size);\n+\n+\t\tif (type < 0)\n+\t\t\treturn -1;\n+\n+\t\tprintf(\"%s %s %lu\\n\", sha1_to_hex(sha1), typename(type), size);\n+\t}\n+\telse\n+\t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n+\n+\treturn 0;\n+}\n+\n+static int check_loose_object(const unsigned char *sha1,\n+\t\t\t      const char *path,\n+\t\t\t      void *data)\n+{\n+\treturn print_object(sha1);\n+}\n+\n+static int check_packed_object(const unsigned char *sha1,\n+\t\t\t       struct packed_git *pack,\n+\t\t\t       uint32_t pos,\n+\t\t\t       void *data)\n+{\n+\treturn print_object(sha1);\n+}\n+\n+static struct option builtin_filter_objects_options[] = {\n+\tOPT__VERBOSE(&verbose, \"show object type and size\"),\n+\tOPT_END()\n+};\n+\n+int cmd_list_all_objects(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct packed_git *p;\n+\n+\targc = parse_options(argc, argv, prefix, builtin_filter_objects_options,\n+\t\t\t     NULL, 0);\n+\n+\tfor_each_loose_object(check_loose_object, NULL, 0);\n+\n+\tprepare_packed_git();\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\topen_pack_index(p);\n+\t}\n+\n+\tfor_each_packed_object(check_packed_object, NULL, 0);\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex 44374b1..81e8ae4 100644\n--- a/git.c\n+++ b/git.c\n@@ -417,6 +417,7 @@ static struct cmd_struct commands[] = {\n \t{ \"init\", cmd_init_db, NO_SETUP },\n \t{ \"init-db\", cmd_init_db, NO_SETUP },\n \t{ \"interpret-trailers\", cmd_interpret_trailers, RUN_SETUP },\n+\t{ \"list-all-objects\", cmd_list_all_objects, RUN_SETUP },\n \t{ \"log\", cmd_log, RUN_SETUP },\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n \t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\ndiff --git a/t/t8100-list-all-objects.sh b/t/t8100-list-all-objects.sh\nnew file mode 100755\nindex 0000000..a7b51ce\n--- /dev/null\n+++ b/t/t8100-list-all-objects.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='git list-all-objects'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello, world >file &&\n+\tgit add file &&\n+\tgit commit -m \"initial\"\n+'\n+\n+test_basic_repo_objects () {\n+\tgit cat-file --batch-check=\"%(objectname)\" <<-EOF >expected.unsorted &&\n+\t\tHEAD\n+\t\tHEAD:file\n+\t\tHEAD^{tree}\n+\tEOF\n+\tgit list-all-objects >all-objects.unsorted &&\n+\tsort expected.unsorted >expected &&\n+\tsort all-objects.unsorted >all-objects &&\n+\ttest_cmp all-objects expected\n+}\n+\n+test_expect_success 'list all objects' '\n+\ttest_basic_repo_objects\n+'\n+test_expect_success 'list all objects after pack' '\n+\tgit repack -Ad &&\n+\ttest_basic_repo_objects\n+'\n+\n+test_expect_success 'verbose output' '\n+\tgit cat-file --batch-check=\"%(objectname) %(objecttype) %(objectsize)\" \\\n+\t\t\t<<-EOF >expected.unsorted &&\n+\t\tHEAD\n+\t\tHEAD:file\n+\t\tHEAD^{tree}\n+\tEOF\n+\tgit list-all-objects -v >all-objects.unsorted &&\n+\tsort expected.unsorted >expected &&\n+\tsort all-objects.unsorted >all-objects &&\n+\ttest_cmp all-objects expected\n+'\n+\n+test_done\n-- \n2.4.0.53.g8440f74\n"},{"id":"264491","messageId":"20150622083543.GA12259@peff.net","threadId":"39677","inReplyTo":"1434914431-7745-1-git-send-email-charles@hashpling.org","subject":"Re: Fast enumeration of objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T08:35:44Z","receivedAt":"2015-06-22T08:35:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 21, 2015 at 08:20:30PM +0100, Charles Bailey wrote:\n\n> I performed some test timings of some different commands on a clone of\n> the Linux kernel which was completely packed.\n\nThanks for timing things. I think we can fairly easily improve a bit on\nwhat you have here. I'll go through my full analysis, but see the\nconclusions at the end.\n\n> \t$ time git rev-list --all --objects |\n> \t\tcut -d\" \" -f1 |\n> \t\tgit cat-file --batch-check |\n> \t\tawk '{if ($3 >= 512000) { print $1 }}' |\n> \t\twc -l\n> \t958\n> \n> \treal    0m30.823s\n> \tuser    0m41.904s\n> \tsys     0m7.728s\n> \n> list-all-objects gives a significant improvement:\n> \n> \t$ time git list-all-objects |\n> \t\tgit cat-file --batch-check |\n> \t\tawk '{if ($3 >= 512000) { print $1 }}' |\n> \t\twc -l\n> \t958\n> \n> \treal    0m9.585s\n> \tuser    0m10.820s\n> \tsys     0m4.960s\n\nThat makes sense; of course these two are not necessarily producing the\nsame answer (they do in your case because it's a fresh clone, and all of\nthe objects are reachable). I think that's an acceptable caveat.\n\nYou can speed up the second one by asking batch-check only for the parts\nyou care about:\n\n  git list-all-objects |\n  git cat-file --batch-check='%(objectsize) %(objectname)' |\n  awk '{if ($1 >= 512000) { print $2 }}' |\n  wc -l\n\nThat dropped my best-of-five timings for the same test down from 9.5s to\n7.0s. The answer should be the same. The reason is that cat-file will\nonly compute the items it needs to show, and the object-type is more\nexpensive to get than the size[1].\n\nReplacing awk with:\n\n  perl -alne 'print $F[0] if $F[1] > 512000'\n\ndropped that to 6.0s. That mostly means my awk sucks, but it is\ninteresting to note that not all of the extra time is pipe overhead\ninherent to this approach; your choice of processor matters, too.\n\nIf you're willing to get a slightly different answer, but one that is\noften just as useful, you can replace the \"%(objectsize)\" in the\ncat-file invocation with \"%(objectsize:disk)\". That gives you the actual\non-disk size of the object, which includes delta and zlib compression.\nFor 512K, that produces very different results (because files of that\nsize may actually be text file). But for most truly huge files, they\ntypically do not delta or compress at all, and the on-disk size is\nroughly the same.\n\nThat only shaves off 100-200 milliseconds, though.\n\n[1] If you are wondering why the size is cheaper than the type, it is\n    because of deltas. For base objects, we can get either immediately\n    from the pack entry's header. For a delta, to get the size we have\n    to open the object data; the expected size is part of the delta\n    data. So we pay the extra cost to zlib-inflate the first few bytes.\n    But finding the type works differently; the type in the pack header\n    is OFS_DELTA, so we have to walk back to the parent entry to find\n    the real type.  If that parent is a delta, we walk back recursively\n    until we hit a base object.\n\n    You'd think that would also make %(objectsize:disk) much cheaper\n    than %(objectsize), too. But the disk sizes require computing a\n    the pack revindex on the fly, which takes a few hundred milliseconds\n    on linux.git.\n\n> skipping the cat-filter filter is a lesser but still significant\n> improvement:\n> \n> \t$ time git list-all-objects -v |\n> \t\tawk '{if ($3 >= 512000) { print $1 }}' |\n> \t\twc -l\n> \t958\n> \n> \treal    0m5.637s\n> \tuser    0m6.652s\n> \tsys     0m0.156s\n\nThat's a pretty nice improvement over the piped version. But we cannot\ndo the same custom-format optimization there, because \"-v\" does not\nsupport it. It would be nice if it supported the full range of cat-file\nformatters.\n\nI did a hacky proof-of-concept, and that brought my 6.0s time down to\n4.9s.\n\nI also noticed that cat-file doesn't do any output buffering; this is\nbecause it may be used interactively, line by line, by a caller\ncontrolling both pipes. Replacing write_or_die() with fwrite in my\nproof-of-concept dropped the time to 3.7s.\n\nThat's faster still than your original (different machines, obviously,\nbut your times are similar to mine):\n\n> The old filter-objects could do the size filter a little be faster, but\n> not by much:\n> \n> \t$ time git filter-objects --min-size=500k |\n> \t\twc -l\n> \t958\n> \n> \treal    0m4.564s\n> \tuser    0m4.496s\n> \tsys     0m0.064s\n\nThis is likely caused by your use of sha1_object_info(), which always\ncomputes the type. Switching to the extended form would probably buy you\nanother 2 seconds or so.\n\nAlso, all my numbers are wall-clock times. The CPU time for my 3.7s time\nis actually 6.8s. Whereas doing it all in one process would probably\nrequire 3.0s or so of actual CPU time.\n\nSo my conclusions are:\n\n  1. Yes, the pipe/parsing overhead of a separate processor really is\n     measurable. That's hidden in the wall-clock time if you have\n     multiple cores, but you may care more about CPU time. I still think\n     the flexibility is worth it.\n\n  2. Cutting out the pipe to cat-file is worth doing, as it saves a few\n     seconds. Cutting out \"%(objecttype)\" saves a lot, too, and is worth\n     doing. We should teach \"list-all-objects -v\" to use cat-file's\n     custom formatters (alternatively, we could just teach cat-file a\n     \"--batch-all-objects\" option rather than add a new command).\n\n  3. We should teach cat-file a \"--buffer\" option to use fwrite. Even if\n     we end up with \"list-all-objects --format='%(objectsize)'\" for this\n     task, it would help all the other uses of cat-file.\n\n-Peff\n"},{"id":"264492","messageId":"20150622083822.GB12259@peff.net","threadId":"39677","inReplyTo":"1434914431-7745-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T08:38:22Z","receivedAt":"2015-06-22T08:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 21, 2015 at 08:20:31PM +0100, Charles Bailey wrote:\n\n> +OPTIONS\n> +-------\n> +\n> +-v::\n> +--verbose::\n> +\tOutput in the followin format instead of just printing object ids:\n> +\t<sha1> SP <type> SP <size>\n\ns/followin/&g/\n\n> +int cmd_list_all_objects(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct packed_git *p;\n> +\n> +\targc = parse_options(argc, argv, prefix, builtin_filter_objects_options,\n> +\t\t\t     NULL, 0);\n> +\n> +\tfor_each_loose_object(check_loose_object, NULL, 0);\n> +\n> +\tprepare_packed_git();\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\topen_pack_index(p);\n> +\t}\n\nYikes. The fact that you need to do this means that\nfor_each_packed_object is buggy, IMHO. I'll send a patch.\n\n-Peff\n"},{"id":"264494","messageId":"CACsJy8DGD6PLVJMFQGKyk9YGUn16G8+dLx2bMBn8fyjuXvfbBw@mail.gmail.com","threadId":"39677","inReplyTo":"1434914431-7745-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-22T09:57:28Z","receivedAt":"2015-06-22T09:57:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 22, 2015 at 2:20 AM, Charles Bailey <charles@hashpling.org> wrote:\n> From: Charles Bailey <cbailey32@bloomberg.net>\n>\n> list-all-objects is a command to print the ids of all objects in the\n> object database of a repository. It is designed as a low overhead\n> interface for scripts that want to analyse all objects but don't require\n> the ordering implied by a revision walk.\n>\n> It will list all objects, loose and packed, and will include unreachable\n> objects.\n\nNit picking, but perhaps we should allow to select object source:\nloose, packed, alternates.. These info are available now and cheap to\nget. It's ok not to do it now though.\n\nPersonally I would name this command \"find-objects\" (after unix\ncommand \"find\") where we could still filter objects _not_ based on\nobject content.\n-- \nDuy\n"},{"id":"264495","messageId":"20150622102433.GA12584@peff.net","threadId":"39677","inReplyTo":"CACsJy8DGD6PLVJMFQGKyk9YGUn16G8+dLx2bMBn8fyjuXvfbBw@mail.gmail.com","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:24:34Z","receivedAt":"2015-06-22T10:24:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 22, 2015 at 04:57:28PM +0700, Duy Nguyen wrote:\n\n> On Mon, Jun 22, 2015 at 2:20 AM, Charles Bailey <charles@hashpling.org> wrote:\n> > From: Charles Bailey <cbailey32@bloomberg.net>\n> >\n> > list-all-objects is a command to print the ids of all objects in the\n> > object database of a repository. It is designed as a low overhead\n> > interface for scripts that want to analyse all objects but don't require\n> > the ordering implied by a revision walk.\n> >\n> > It will list all objects, loose and packed, and will include unreachable\n> > objects.\n> \n> Nit picking, but perhaps we should allow to select object source:\n> loose, packed, alternates.. These info are available now and cheap to\n> get. It's ok not to do it now though.\n\nThere is already plumbing to do those individual operations if you want.\nAlthough some of the plumbing involves \"for i in objects/pack/*.pack\",\nwhich is perhaps a little less abstract than we'd like. :)\n\n> Personally I would name this command \"find-objects\" (after unix\n> command \"find\") where we could still filter objects _not_ based on\n> object content.\n\nI like that better than \"ls\", too, but I propose that we actually add\nthis as a feature to cat-file. I'll send patches in a moment.\n\n-Peff\n"},{"id":"264496","messageId":"20150622103321.GB12584@peff.net","threadId":"39677","inReplyTo":"20150622083822.GB12259@peff.net","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:33:21Z","receivedAt":"2015-06-22T10:33:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote:\n\n> > +\tprepare_packed_git();\n> > +\tfor (p = packed_git; p; p = p->next) {\n> > +\t\topen_pack_index(p);\n> > +\t}\n> \n> Yikes. The fact that you need to do this means that\n> for_each_packed_object is buggy, IMHO. I'll send a patch.\n\nHere's that patch. And since I did not want to pile work on Charles, I\nwent ahead and just implemented the patches I suggested in the other\nemail.\n\nWe may want to take patch 1 separately for the maint-track, as it is\nreally a bug-fix (albeit one that I do not think actually affects anyone\nin practice right now).\n\nPatches 2-5 are useful even if we go with Charles' command, as they make\ncat-file better (cleanups and he new buffer option).\n\nPatches 6-7 implement the cat-file option that would be redundant with\nlist-all-objects.\n\nBy the way, in addition to not showing objects in order,\nlist-all-objects (and my cat-file option) may show duplicates. Do we\nwant to \"sort -u\" for the user? It might be nice for them to always get\na de-duped and sorted list. Aside from the CPU cost of sorting, it does\nmean we'll allocate ~80MB for the kernel to store the sha1s. I guess\nthat's not too much when you are talking about the kernel repo. I took\nthe coward's way out and just mentioned the limitation in the\ndocumentation, but I'm happy to be persuaded.\n\n  [1/7]: for_each_packed_object: automatically open pack index\n  [2/7]: cat-file: minor style fix in options list\n  [3/7]: cat-file: move batch_options definition to top of file\n  [4/7]: cat-file: add --buffer option\n  [5/7]: cat-file: stop returning value from batch_one_object\n  [6/7]: cat-file: split batch_one_object into two stages\n  [7/7]: cat-file: add --batch-all-objects option\n\n-Peff\n"},{"id":"264497","messageId":"20150622104049.GA14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 1/7] for_each_packed_object: automatically open pack index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:40:50Z","receivedAt":"2015-06-22T10:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When for_each_packed_object is called, we call\nprepare_packed_git() to make sure we have the actual list of\npacks. But the latter does not actually open the pack\nindices, meaning that pack->nr_objects may simply be 0 if\nthe pack has not otherwise been used since the program\nstarted.\n\nIn practice, this didn't come up for the current callers,\nbecause they iterate the packed objects only after iterating\nall reachable objects (so for it to matter you would have to\nhave a pack consisting only of unreachable objects). But it\nis a dangerous and confusing interface that should be fixed\nfor future callers.\n\nNote that we do not end the iteration when a pack cannot be\nopened, but we do return an error. That lets you complete\nthe iteration even in actively-repacked repository where an\n.idx file may racily go away, but it also lets callers know\nthat they may not have gotten the complete list (which the\ncurrent reachability-check caller does care about).\n\nWe have to tweak one of the prune tests due to the changed\nreturn value; an earlier test creates bogus .idx files and\ndoes not clean them up. Having to make this tweak is a good\nthing; it means we will not prune in a broken repository,\nand the test confirms that we do not negatively impact a\nmore lenient caller, count-objects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c      | 7 ++++++-\n t/t5304-prune.sh | 1 +\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5038475..f1f0efb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3573,14 +3573,19 @@ int for_each_packed_object(each_packed_object_fn cb, void *data, unsigned flags)\n {\n \tstruct packed_git *p;\n \tint r = 0;\n+\tint pack_errors = 0;\n \n \tprepare_packed_git();\n \tfor (p = packed_git; p; p = p->next) {\n \t\tif ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n \t\t\tcontinue;\n+\t\tif (open_pack_index(p)) {\n+\t\t\tpack_errors = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tr = for_each_object_in_pack(p, cb, data);\n \t\tif (r)\n \t\t\tbreak;\n \t}\n-\treturn r;\n+\treturn r ? r : pack_errors;\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 0794d33..023d7c6 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -218,6 +218,7 @@ test_expect_success 'gc: prune old objects after local clone' '\n '\n \n test_expect_success 'garbage report in count-objects -v' '\n+\ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \t: >.git/objects/pack/foo &&\n \t: >.git/objects/pack/foo.bar &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264498","messageId":"20150622104056.GB14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 2/7] cat-file: minor style fix in options list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:40:56Z","receivedAt":"2015-06-22T10:40:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not put extra whitespace before the first macro\nargument.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 049a95f..6cbcccc 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -412,7 +412,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n \t\tOPT_CMDMODE(0, \"textconv\", &opt,\n \t\t\t    N_(\"for blob objects, run textconv on object's content\"), 'c'),\n-\t\tOPT_BOOL( 0, \"allow-unknown-type\", &unknown_type,\n+\t\tOPT_BOOL(0, \"allow-unknown-type\", &unknown_type,\n \t\t\t  N_(\"allow -s and -t to work with broken/corrupt objects\")),\n \t\t{ OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n \t\t\tN_(\"show info and content of objects fed from the standard input\"),\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264499","messageId":"20150622104102.GC14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 3/7] cat-file: move batch_options definition to top of file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:41:03Z","receivedAt":"2015-06-22T10:41:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"That way all of the functions can make use of it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 6cbcccc..d4101b7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -10,6 +10,13 @@\n #include \"streaming.h\"\n #include \"tree-walk.h\"\n \n+struct batch_options {\n+\tint enabled;\n+\tint follow_symlinks;\n+\tint print_contents;\n+\tconst char *format;\n+};\n+\n static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\tint unknown_type)\n {\n@@ -232,12 +239,6 @@ static void print_object_or_die(int fd, struct expand_data *data)\n \t}\n }\n \n-struct batch_options {\n-\tint enabled;\n-\tint follow_symlinks;\n-\tint print_contents;\n-\tconst char *format;\n-};\n \n static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t\t    struct expand_data *data)\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264500","messageId":"20150622104517.GD14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 4/7] cat-file: add --buffer option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:45:17Z","receivedAt":"2015-06-22T10:45:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a direct write() to output the results of --batch and\n--batch-check. This is good for processes feeding the input\nand reading the output interactively, but it introduces\nmeasurable overhead if you do not want this feature. For\nexample, on linux.git:\n\n  $ git rev-list --objects --all | cut -d' ' -f1 >objects\n  $ time git cat-file --batch-check='%(objectsize)' \\\n          <objects >/dev/null\n  real    0m5.440s\n  user    0m5.060s\n  sys     0m0.384s\n\nThis patch adds an option to use regular stdio buffering:\n\n  $ time git cat-file --batch-check='%(objectsize)' \\\n          --buffer <objects >/dev/null\n  real    0m4.975s\n  user    0m4.888s\n  sys     0m0.092s\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis selectively uses fwrite or write_or_die, depending on the buffer\nsetting. Another option would be to just always use fwrite(), and then\nselectively fflush(). It feels kind of wasteful in the non-buffered\ncase, as it's just another layer to write through. OTOH, the cost of\nwriting a line into the buffer only to flush is probably dwarfed by the\nsystem call of actually flushing.\n\nIf we went that direction, we could probably simplify the code a bit\n(both getting rid of the batch_write function I call here, and dropping\na bunch of existing fflush() calls, where we must flush any time we use\nprintf for its formatting capabilities).\n\nI also considered that the \"--buffer\" case is likely to be the common\none. We cannot flip the default, though, as it would break any existing\ncallers (who would need to specify \"--no-buffer\"). We can do the usual\ndeprecation dance, but I don't know if it is worth it for a plumbing\ncommand like this.\n\n Documentation/git-cat-file.txt |  7 +++++++\n builtin/cat-file.c             | 26 +++++++++++++++++++-------\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 319ab4c..0058bd4 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -69,6 +69,13 @@ OPTIONS\n \tnot be combined with any other options or arguments.  See the\n \tsection `BATCH OUTPUT` below for details.\n \n+--buffer::\n+\tNormally batch output is flushed after each object is output, so\n+\tthat a process can interactively read and write from\n+\t`cat-file`. With this option, the output uses normal stdio\n+\tbuffering; this is much more efficient when invoking\n+\t`--batch-check` on a large number of objects.\n+\n --allow-unknown-type::\n \tAllow -s or -t to query broken/corrupt objects of unknown type.\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d4101b7..741e100 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -14,6 +14,7 @@ struct batch_options {\n \tint enabled;\n \tint follow_symlinks;\n \tint print_contents;\n+\tint buffer_output;\n \tconst char *format;\n };\n \n@@ -211,14 +212,25 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n \treturn end - start + 1;\n }\n \n-static void print_object_or_die(int fd, struct expand_data *data)\n+static void batch_write(struct batch_options *opt, const void *data, int len)\n+{\n+\tif (opt->buffer_output) {\n+\t\tif (fwrite(data, 1, len, stdout) != len)\n+\t\t\tdie_errno(\"unable to write to stdout\");\n+\t} else\n+\t\twrite_or_die(1, data, len);\n+}\n+\n+static void print_object_or_die(struct batch_options *opt, struct expand_data *data)\n {\n \tconst unsigned char *sha1 = data->sha1;\n \n \tassert(data->info.typep);\n \n \tif (data->type == OBJ_BLOB) {\n-\t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n+\t\tif (opt->buffer_output)\n+\t\t\tfflush(stdout);\n+\t\tif (stream_blob_to_fd(1, sha1, NULL, 0) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n \t}\n \telse {\n@@ -234,12 +246,11 @@ static void print_object_or_die(int fd, struct expand_data *data)\n \t\tif (data->info.sizep && size != data->size)\n \t\t\tdie(\"object %s changed size!?\", sha1_to_hex(sha1));\n \n-\t\twrite_or_die(fd, contents, size);\n+\t\tbatch_write(opt, contents, size);\n \t\tfree(contents);\n \t}\n }\n \n-\n static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t\t    struct expand_data *data)\n {\n@@ -294,12 +305,12 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n-\twrite_or_die(1, buf.buf, buf.len);\n+\tbatch_write(opt, buf.buf, buf.len);\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\n-\t\tprint_object_or_die(1, data);\n-\t\twrite_or_die(1, \"\\n\", 1);\n+\t\tprint_object_or_die(opt, data);\n+\t\tbatch_write(opt, \"\\n\", 1);\n \t}\n \treturn 0;\n }\n@@ -415,6 +426,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"for blob objects, run textconv on object's content\"), 'c'),\n \t\tOPT_BOOL(0, \"allow-unknown-type\", &unknown_type,\n \t\t\t  N_(\"allow -s and -t to work with broken/corrupt objects\")),\n+\t\tOPT_BOOL(0, \"buffer\", &batch.buffer_output, N_(\"buffer --batch output\")),\n \t\t{ OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n \t\t\tN_(\"show info and content of objects fed from the standard input\"),\n \t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264501","messageId":"20150622104533.GE14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 5/7] cat-file: stop returning value from batch_one_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:45:33Z","receivedAt":"2015-06-22T10:45:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If batch_one_object returns an error code, we stop reading\ninput.  However, it will only do so if we feed it NULL,\nwhich cannot happen; we give it the \"buf\" member of a\nstrbuf, which is always non-NULL.\n\nWe did originally stop on other errors (like a missing\nobject), but this was changed in 3c076db (cat-file --batch /\n--batch-check: do not exit if hashes are missing,\n2008-06-09). These days we keep going for any per-object\nerror (and print \"missing\" when necessary).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 741e100..7d99c15 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -251,17 +251,14 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t}\n }\n \n-static int batch_one_object(const char *obj_name, struct batch_options *opt,\n-\t\t\t    struct expand_data *data)\n+static void batch_one_object(const char *obj_name, struct batch_options *opt,\n+\t\t\t     struct expand_data *data)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_context ctx;\n \tint flags = opt->follow_symlinks ? GET_SHA1_FOLLOW_SYMLINKS : 0;\n \tenum follow_symlinks_result result;\n \n-\tif (!obj_name)\n-\t   return 1;\n-\n \tresult = get_sha1_with_context(obj_name, flags, data->sha1, &ctx);\n \tif (result != FOUND) {\n \t\tswitch (result) {\n@@ -286,7 +283,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t\tbreak;\n \t\t}\n \t\tfflush(stdout);\n-\t\treturn 0;\n+\t\treturn;\n \t}\n \n \tif (ctx.mode == 0) {\n@@ -294,13 +291,13 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t       (uintmax_t)ctx.symlink_path.len,\n \t\t       ctx.symlink_path.buf);\n \t\tfflush(stdout);\n-\t\treturn 0;\n+\t\treturn;\n \t}\n \n \tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n-\t\treturn 0;\n+\t\treturn;\n \t}\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n@@ -312,7 +309,6 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\tprint_object_or_die(opt, data);\n \t\tbatch_write(opt, \"\\n\", 1);\n \t}\n-\treturn 0;\n }\n \n static int batch_objects(struct batch_options *opt)\n@@ -367,9 +363,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.rest = p;\n \t\t}\n \n-\t\tretval = batch_one_object(buf.buf, opt, &data);\n-\t\tif (retval)\n-\t\t\tbreak;\n+\t\tbatch_one_object(buf.buf, opt, &data);\n \t}\n \n \tstrbuf_release(&buf);\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264502","messageId":"20150622104540.GF14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 6/7] cat-file: split batch_one_object into two stages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:45:41Z","receivedAt":"2015-06-22T10:45:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are really two things going on in this function:\n\n  1. We convert the name we got on stdin to a sha1.\n\n  2. We look up and print information on the sha1.\n\nLet's split out the second half so that we can call it\nseparately.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 7d99c15..499ccda 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -251,10 +251,31 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t}\n }\n \n+static void batch_object_write(const char *obj_name, struct batch_options *opt,\n+\t\t\t       struct expand_data *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n+\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\tfflush(stdout);\n+\t\treturn;\n+\t}\n+\n+\tstrbuf_expand(&buf, opt->format, expand_format, data);\n+\tstrbuf_addch(&buf, '\\n');\n+\tbatch_write(opt, buf.buf, buf.len);\n+\tstrbuf_release(&buf);\n+\n+\tif (opt->print_contents) {\n+\t\tprint_object_or_die(opt, data);\n+\t\tbatch_write(opt, \"\\n\", 1);\n+\t}\n+}\n+\n static void batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\t\t     struct expand_data *data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_context ctx;\n \tint flags = opt->follow_symlinks ? GET_SHA1_FOLLOW_SYMLINKS : 0;\n \tenum follow_symlinks_result result;\n@@ -294,21 +315,7 @@ static void batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\treturn;\n \t}\n \n-\tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n-\t\tprintf(\"%s missing\\n\", obj_name);\n-\t\tfflush(stdout);\n-\t\treturn;\n-\t}\n-\n-\tstrbuf_expand(&buf, opt->format, expand_format, data);\n-\tstrbuf_addch(&buf, '\\n');\n-\tbatch_write(opt, buf.buf, buf.len);\n-\tstrbuf_release(&buf);\n-\n-\tif (opt->print_contents) {\n-\t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, \"\\n\", 1);\n-\t}\n+\tbatch_object_write(obj_name, opt, data);\n }\n \n static int batch_objects(struct batch_options *opt)\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264503","messageId":"20150622104559.GG14475@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 7/7] cat-file: add --batch-all-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T10:45:59Z","receivedAt":"2015-06-22T10:45:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It can sometimes be useful to examine all objects in the\nrepository. Normally this is done with \"git rev-list --all\n--objects\", but:\n\n  1. That shows only reachable objects. You may want to look\n     at all available objects.\n\n  2. It's slow. We actually open each object to walk the\n     graph. If your operation is OK with seeing unreachable\n     objects, it's an order of magnitude faster to just\n     enumerate the loose directories and pack indices.\n\nYou can do this yourself using \"ls\" and \"git show-index\",\nbut it's non-obvious.  This patch adds an option to\n\"cat-file --batch-check\" to operate on all available\nobjects (rather than reading names from stdin).\n\nThis is based on a proposal by Charles Bailey to provide a\nseparate \"git list-all-objects\" command. That is more\northogonal, as it splits enumerating the objects from\ngetting information about them. However, in practice you\nwill either:\n\n  a. Feed the list of objects directly into cat-file anyway,\n     so you can find out information about them. Keeping it\n     in a single process is more efficient.\n\n  b. Ask the listing process to start telling you more\n     information about the objects, in which case you will\n     reinvent cat-file's batch-check formatter.\n\nAdding a cat-file option is simple and efficient. And if you\nreally do want just the object names, you can always do:\n\n  git cat-file --batch-check='%(objectname)' --batch-all-objects\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-cat-file.txt |  8 ++++++++\n builtin/cat-file.c             | 44 ++++++++++++++++++++++++++++++++++++++++--\n t/t1006-cat-file.sh            | 27 ++++++++++++++++++++++++++\n 3 files changed, 77 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 0058bd4..6831b08 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -69,6 +69,14 @@ OPTIONS\n \tnot be combined with any other options or arguments.  See the\n \tsection `BATCH OUTPUT` below for details.\n \n+--batch-all-objects::\n+\tInstead of reading a list of objects on stdin, perform the\n+\trequested batch operation on all objects in the repository and\n+\tany alternate object stores (not just reachable objects).\n+\tRequires `--batch` or `--batch-check` be specified. Note that\n+\tthe order of the objects is unspecified, and there may be\n+\tduplicate entries.\n+\n --buffer::\n \tNormally batch output is flushed after each object is output, so\n \tthat a process can interactively read and write from\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 499ccda..95604c4 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -15,6 +15,7 @@ struct batch_options {\n \tint follow_symlinks;\n \tint print_contents;\n \tint buffer_output;\n+\tint all_objects;\n \tconst char *format;\n };\n \n@@ -257,7 +258,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n-\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\tprintf(\"%s missing\\n\", obj_name ? obj_name : sha1_to_hex(data->sha1));\n \t\tfflush(stdout);\n \t\treturn;\n \t}\n@@ -318,6 +319,34 @@ static void batch_one_object(const char *obj_name, struct batch_options *opt,\n \tbatch_object_write(obj_name, opt, data);\n }\n \n+struct object_cb_data {\n+\tstruct batch_options *opt;\n+\tstruct expand_data *expand;\n+};\n+\n+static int batch_object_cb(const unsigned char *sha1,\n+\t\t\t   struct object_cb_data *data)\n+{\n+\thashcpy(data->expand->sha1, sha1);\n+\tbatch_object_write(NULL, data->opt, data->expand);\n+\treturn 0;\n+}\n+\n+static int batch_loose_object(const unsigned char *sha1,\n+\t\t\t      const char *path,\n+\t\t\t      void *data)\n+{\n+\treturn batch_object_cb(sha1, data);\n+}\n+\n+static int batch_packed_object(const unsigned char *sha1,\n+\t\t\t       struct packed_git *pack,\n+\t\t\t       uint32_t pos,\n+\t\t\t       void *data)\n+{\n+\treturn batch_object_cb(sha1, data);\n+}\n+\n static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -345,6 +374,15 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->print_contents)\n \t\tdata.info.typep = &data.type;\n \n+\tif (opt->all_objects) {\n+\t\tstruct object_cb_data cb;\n+\t\tcb.opt = opt;\n+\t\tcb.expand = &data;\n+\t\tfor_each_loose_object(batch_loose_object, &cb, 0);\n+\t\tfor_each_packed_object(batch_packed_object, &cb, 0);\n+\t\treturn 0;\n+\t}\n+\n \t/*\n \t * We are going to call get_sha1 on a potentially very large number of\n \t * objects. In most large cases, these will be actual object sha1s. The\n@@ -436,6 +474,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n \t\tOPT_BOOL(0, \"follow-symlinks\", &batch.follow_symlinks,\n \t\t\t N_(\"follow in-tree symlinks (used with --batch or --batch-check)\")),\n+\t\tOPT_BOOL(0, \"batch-all-objects\", &batch.all_objects,\n+\t\t\t N_(\"show all objects with --batch or --batch-check\")),\n \t\tOPT_END()\n \t};\n \n@@ -460,7 +500,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(cat_file_usage, options);\n \t}\n \n-\tif (batch.follow_symlinks && !batch.enabled) {\n+\tif ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {\n \t\tusage_with_options(cat_file_usage, options);\n \t}\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 93a4794..2b4220a 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -547,4 +547,31 @@ test_expect_success 'git cat-file --batch --follow-symlink returns correct sha a\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cat-file --batch-all-objects shows all objects' '\n+\t# make new repos so we now the full set of objects; we will\n+\t# also make sure that there are some packed and some loose\n+\t# objects, some referenced and some not, and that there are\n+\t# some available only via alternates.\n+\tgit init all-one &&\n+\t(\n+\t\tcd all-one &&\n+\t\techo content >file &&\n+\t\tgit add file &&\n+\t\tgit commit -qm base &&\n+\t\tgit rev-parse HEAD HEAD^{tree} HEAD:file &&\n+\t\tgit repack -ad &&\n+\t\techo not-cloned | git hash-object -w --stdin\n+\t) >expect.unsorted &&\n+\tgit clone -s all-one all-two &&\n+\t(\n+\t\tcd all-two &&\n+\t\techo local-unref | git hash-object -w --stdin\n+\t) >>expect.unsorted &&\n+\tsort <expect.unsorted >expect &&\n+\tgit -C all-two cat-file --batch-all-objects \\\n+\t\t\t\t--batch-check=\"%(objectname)\" >actual.unsorted &&\n+\tsort <actual.unsorted >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264504","messageId":"20150622110632.GA26436@peff.net","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"[PATCH 8/7] cat-file: sort and de-dup output of --batch-all-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T11:06:32Z","receivedAt":"2015-06-22T11:06:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 22, 2015 at 06:33:21AM -0400, Jeff King wrote:\n\n> By the way, in addition to not showing objects in order,\n> list-all-objects (and my cat-file option) may show duplicates. Do we\n> want to \"sort -u\" for the user? It might be nice for them to always get\n> a de-duped and sorted list. Aside from the CPU cost of sorting, it does\n> mean we'll allocate ~80MB for the kernel to store the sha1s. I guess\n> that's not too much when you are talking about the kernel repo. I took\n> the coward's way out and just mentioned the limitation in the\n> documentation, but I'm happy to be persuaded.\n\nThe patch below does the sort/de-dup. I'd probably just squash it into\npatch 7, though.\n\nI did have one additional thought, though. We are treating this as two\nseparate operations: \"what are the sha1s in the repo\" and \"show me\ninformation about this sha1\". But by integrating with cat-file, we could\nactually show information not just about a particular sha1, but about a\nparticular on-disk object.\n\nE.g., if there are duplicates of a particular object, some formatters\nlike \"%(objectsize:disk)\" and \"%(deltabase)\" pick one arbitrarily to\nshow. I don't know if anybody actually cares about that in practice, but\nif we show duplicates, we could give the accurate information for each\ninstance (and in fact we could give other information like loose vs\npacked, which file contains the object, etc).\n\nI tend to think that the lack of de-duping is sufficiently confusing\nthat it should be the default, and we can always add a \"no really, show\nme the duplicates\" option later. It is not as simple as skipping the\nde-dup step. We'd have to actually avoid calling sha1_object_info, and\nuse the information found in the loose/pack traversal (which would in\nturn require exposing the low-level bits of sha1_object_info).\n\n-- >8 --\nSubject: cat-file: sort and de-dup output of --batch-all-objects\n\nThe sorting we could probably live without, but printing\nduplicates is just a hassle for the user, who must then\nde-dup themselves (or risk a wrong answer if they are doing\nsomething like counting objects with a particular property).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-cat-file.txt |  3 +--\n builtin/cat-file.c             | 22 +++++++++++++++-------\n t/t1006-cat-file.sh            |  3 +--\n 3 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 6831b08..3105fc0 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -74,8 +74,7 @@ OPTIONS\n \trequested batch operation on all objects in the repository and\n \tany alternate object stores (not just reachable objects).\n \tRequires `--batch` or `--batch-check` be specified. Note that\n-\tthe order of the objects is unspecified, and there may be\n-\tduplicate entries.\n+\tthe objects are visited in order sorted by their hashes.\n \n --buffer::\n \tNormally batch output is flushed after each object is output, so\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 95604c4..07baad1 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -9,6 +9,7 @@\n #include \"userdiff.h\"\n #include \"streaming.h\"\n #include \"tree-walk.h\"\n+#include \"sha1-array.h\"\n \n struct batch_options {\n \tint enabled;\n@@ -324,19 +325,19 @@ struct object_cb_data {\n \tstruct expand_data *expand;\n };\n \n-static int batch_object_cb(const unsigned char *sha1,\n-\t\t\t   struct object_cb_data *data)\n+static void batch_object_cb(const unsigned char sha1[20], void *vdata)\n {\n+\tstruct object_cb_data *data = vdata;\n \thashcpy(data->expand->sha1, sha1);\n \tbatch_object_write(NULL, data->opt, data->expand);\n-\treturn 0;\n }\n \n static int batch_loose_object(const unsigned char *sha1,\n \t\t\t      const char *path,\n \t\t\t      void *data)\n {\n-\treturn batch_object_cb(sha1, data);\n+\tsha1_array_append(data, sha1);\n+\treturn 0;\n }\n \n static int batch_packed_object(const unsigned char *sha1,\n@@ -344,7 +345,8 @@ static int batch_packed_object(const unsigned char *sha1,\n \t\t\t       uint32_t pos,\n \t\t\t       void *data)\n {\n-\treturn batch_object_cb(sha1, data);\n+\tsha1_array_append(data, sha1);\n+\treturn 0;\n }\n \n static int batch_objects(struct batch_options *opt)\n@@ -375,11 +377,17 @@ static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \n \tif (opt->all_objects) {\n+\t\tstruct sha1_array sa = SHA1_ARRAY_INIT;\n \t\tstruct object_cb_data cb;\n+\n+\t\tfor_each_loose_object(batch_loose_object, &sa, 0);\n+\t\tfor_each_packed_object(batch_packed_object, &sa, 0);\n+\n \t\tcb.opt = opt;\n \t\tcb.expand = &data;\n-\t\tfor_each_loose_object(batch_loose_object, &cb, 0);\n-\t\tfor_each_packed_object(batch_packed_object, &cb, 0);\n+\t\tsha1_array_for_each_unique(&sa, batch_object_cb, &cb);\n+\n+\t\tsha1_array_clear(&sa);\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 2b4220a..18dbdc8 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -569,8 +569,7 @@ test_expect_success 'cat-file --batch-all-objects shows all objects' '\n \t) >>expect.unsorted &&\n \tsort <expect.unsorted >expect &&\n \tgit -C all-two cat-file --batch-all-objects \\\n-\t\t\t\t--batch-check=\"%(objectname)\" >actual.unsorted &&\n-\tsort <actual.unsorted >actual &&\n+\t\t\t\t--batch-check=\"%(objectname)\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.4.4.719.g3984bc6\n"},{"id":"264507","messageId":"20150622113821.GA31118@hashpling.org","threadId":"39677","inReplyTo":"20150622083822.GB12259@peff.net","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-22T11:38:21Z","receivedAt":"2015-06-22T11:38:21Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote:\n> On Sun, Jun 21, 2015 at 08:20:31PM +0100, Charles Bailey wrote:\n> \n> > +\tprepare_packed_git();\n> > +\tfor (p = packed_git; p; p = p->next) {\n> > +\t\topen_pack_index(p);\n> > +\t}\n> \n> Yikes. The fact that you need to do this means that\n> for_each_packed_object is buggy, IMHO. I'll send a patch.\n\nI'm glad you said that; the interface did seem a bit warty at the time\nbut as I \"fixed\" this early in my hacking I didn't remeber to revisit\nthis and ask if it was actually intentional.\n"},{"id":"264580","messageId":"xmqq381jh6jr.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"20150622083543.GA12259@peff.net","subject":"Re: Fast enumeration of objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T19:44:24Z","receivedAt":"2015-06-22T19:44:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ...\n> So my conclusions are:\n>\n>   1. Yes, the pipe/parsing overhead of a separate processor really is\n>      measurable. That's hidden in the wall-clock time if you have\n>      multiple cores, but you may care more about CPU time. I still think\n>      the flexibility is worth it.\n>\n>   2. Cutting out the pipe to cat-file is worth doing, as it saves a few\n>      seconds. Cutting out \"%(objecttype)\" saves a lot, too, and is worth\n>      doing. We should teach \"list-all-objects -v\" to use cat-file's\n>      custom formatters (alternatively, we could just teach cat-file a\n>      \"--batch-all-objects\" option rather than add a new command).\n>\n>   3. We should teach cat-file a \"--buffer\" option to use fwrite. Even if\n>      we end up with \"list-all-objects --format='%(objectsize)'\" for this\n>      task, it would help all the other uses of cat-file.\n\nAll sounds very sensible.\n"},{"id":"264596","messageId":"20150622214818.GA18677@hashpling.org","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-22T21:48:19Z","receivedAt":"2015-06-22T21:48:19Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Jun 22, 2015 at 06:33:21AM -0400, Jeff King wrote:\n> On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote:\n> \n> > > +\tprepare_packed_git();\n> > > +\tfor (p = packed_git; p; p = p->next) {\n> > > +\t\topen_pack_index(p);\n> > > +\t}\n> > \n> > Yikes. The fact that you need to do this means that\n> > for_each_packed_object is buggy, IMHO. I'll send a patch.\n> \n> Here's that patch. And since I did not want to pile work on Charles, I\n> went ahead and just implemented the patches I suggested in the other\n> email.\n\nI have to say that I think that adding this functionality to cat-file\nmakes a lot of sense. If it only catted files it might be a stretch but\nas it's already grown --batch-check functionality, it now seems a\nreasonable extension. I'm not particularly attached to having a\nstandalone \"list-all-objects\" command per se.\n"},{"id":"264597","messageId":"xmqqk2uvfm5p.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"20150622103321.GB12584@peff.net","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T21:50:10Z","receivedAt":"2015-06-22T21:50:10Z","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 Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote:\n>\n>> > +\tprepare_packed_git();\n>> > +\tfor (p = packed_git; p; p = p->next) {\n>> > +\t\topen_pack_index(p);\n>> > +\t}\n>> \n>> Yikes. The fact that you need to do this means that\n>> for_each_packed_object is buggy, IMHO. I'll send a patch.\n>\n> Here's that patch. And since I did not want to pile work on Charles, I\n> went ahead and just implemented the patches I suggested in the other\n> email.\n>\n> We may want to take patch 1 separately for the maint-track, as it is\n> really a bug-fix (albeit one that I do not think actually affects anyone\n> in practice right now).\n\nHmph, add_unseen_recent_objects_to_traversal() is the only existing\nuser, and before d3038d22 (prune: keep objects reachable from recent\nobjects, 2014-10-15) added that function, for-each-packed-object\nexisted but had no callers.\n\nAnd the objects not beeing seen by that function (due to lack of\n\"open\") would matter only for pruning purposes, which would mean\nyou have to be calling into the codepath when running a full repack,\nso you would have opened all the packs that matter anyway (if you\nhave a \"old cruft archive\" pack that contains only objects that\nare unreachable, you may not have opened that pack, though, and you\nmay prune the thing away prematurely).\n\nSo, I think I can agree that this would unlikely affect anybody in\npractice.\n\n> Patches 2-5 are useful even if we go with Charles' command, as they make\n> cat-file better (cleanups and he new buffer option).\n>\n> Patches 6-7 implement the cat-file option that would be redundant with\n> list-all-objects.\n>\n> By the way, in addition to not showing objects in order,\n> list-all-objects (and my cat-file option) may show duplicates. Do we\n> want to \"sort -u\" for the user? It might be nice for them to always get\n> a de-duped and sorted list. Aside from the CPU cost of sorting, it does\n> mean we'll allocate ~80MB for the kernel to store the sha1s. I guess\n> that's not too much when you are talking about the kernel repo. I took\n> the coward's way out and just mentioned the limitation in the\n> documentation, but I'm happy to be persuaded.\n>\n>   [1/7]: for_each_packed_object: automatically open pack index\n>   [2/7]: cat-file: minor style fix in options list\n>   [3/7]: cat-file: move batch_options definition to top of file\n>   [4/7]: cat-file: add --buffer option\n>   [5/7]: cat-file: stop returning value from batch_one_object\n>   [6/7]: cat-file: split batch_one_object into two stages\n>   [7/7]: cat-file: add --batch-all-objects option\n>\n> -Peff\n"},{"id":"264598","messageId":"xmqqfv5jfljk.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"20150621183026.GA7199@hashpling.org","subject":"Re: [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T22:03:27Z","receivedAt":"2015-06-22T22:03:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> On Sun, Jun 21, 2015 at 07:25:44PM +0100, Charles Bailey wrote:\n>> From: Charles Bailey <cbailey32@bloomberg.net>\n>> \n>> diff --git a/parse-options.c b/parse-options.c\n>> index 80106c0..101b649 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p,\n>>  \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n>>  \t\treturn 0;\n>>  \n>> +\tcase OPTION_MAGNITUDE:\n>> +\t\tif (unset) {\n>> +\t\t\t*(unsigned long *)opt->value = 0;\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n>> +\t\t\t*(unsigned long *)opt->value = opt->defval;\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\t\tif (get_arg(p, opt, flags, &arg))\n>> +\t\t\treturn -1;\n>> +\t\tif (!git_parse_ulong(arg, opt->value))\n>> +\t\t\treturn opterror(opt,\n>> +\t\t\t\t\"expects a integer value with an optional k/m/g suffix\",\n>> +\t\t\t\tflags);\n>> +\t\treturn 0;\n>> +\n>\n> Spotted after sending:\n> s/expects a integer/expects an integer/\n\nThanks.\n"},{"id":"264599","messageId":"20150622220350.GB18677@hashpling.org","threadId":"39677","inReplyTo":"20150622110632.GA26436@peff.net","subject":"Re: [PATCH 8/7] cat-file: sort and de-dup output of --batch-all-objects","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-22T22:03:50Z","receivedAt":"2015-06-22T22:03:50Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Jun 22, 2015 at 07:06:32AM -0400, Jeff King wrote:\n> On Mon, Jun 22, 2015 at 06:33:21AM -0400, Jeff King wrote:\n> \n> > By the way, in addition to not showing objects in order,\n> > list-all-objects (and my cat-file option) may show duplicates. Do we\n> > want to \"sort -u\" for the user? It might be nice for them to always get\n> > a de-duped and sorted list. Aside from the CPU cost of sorting, it does\n> > mean we'll allocate ~80MB for the kernel to store the sha1s. I guess\n> > that's not too much when you are talking about the kernel repo. I took\n> > the coward's way out and just mentioned the limitation in the\n> > documentation, but I'm happy to be persuaded.\n> \n> The patch below does the sort/de-dup. I'd probably just squash it into\n> patch 7, though.\n\nWoah, 8 out of 7! Did you get a chance to measure the performance hit of\nthe sort? If not, I may test it out when I next get the chance.\n"},{"id":"264600","messageId":"xmqqbng7flbq.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"1434911144-6781-3-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T22:08:09Z","receivedAt":"2015-06-22T22:08:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> From: Charles Bailey <cbailey32@bloomberg.net>\n>\n> The unsigned long option parsing (including 'k'/'m'/'g' suffix parsing)\n> is more widely applicable. Add support for OPT_MAGNITUDE to\n> parse-options.h and change pack-objects.c use this support.\n>\n> The error behavior on parse errors follows that of OPT_INTEGER.\n> The name of the option that failed to parse is reported with a brief\n> message describing the expect format for the option argument and then\n> the full usage message for the command invoked.\n>\n> This is differs from the previous behavior for OPT_ULONG used in\n\ns/is //; (locally fixed--no need to resend).\n"},{"id":"264601","messageId":"xmqq7fqvfl9g.fsf@gitster.dls.corp.google.com","threadId":"39677","inReplyTo":"1434911144-6781-1-git-send-email-charles@hashpling.org","subject":"Re: Improvements to integer option parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T22:09:31Z","receivedAt":"2015-06-22T22:09:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> This is a re-roll of the first two patches in my previous series which used to\n> include \"filter-objects\" which is now a separate topic.\n>\n> [PATCH 1/2] Correct test-parse-options to handle negative ints\n>\n> The first one has changed only in that I've moved the additional test to a more\n> logical place in the test file.\n>\n> [PATCH 2/2] Move unsigned long option parsing out of pack-objects.c\n>\n> I've made the following changes to the second commit:\n>\n> - renamed this OPT_MAGNITUDE to try and convey something that is\n> both unsigned and might benefit from a 'scale' suffix. I'm expecting\n> more discussion on the name!\n\nI think that name is very sensible.\n\n> - marginally improved the opterror message on failed parses\n\nI'd queue with \"s/a integer/a non-negative integer/\".\n\n> - noted the change in behavior for the error messages generated for\n> pack-objects' --max-pack-size and --window-memory in the commit message\n\nThanks.  Queued.\n"},{"id":"264606","messageId":"7A2F4AE4-EB17-4FAE-A51A-7D6587FC5FCE@hashpling.org","threadId":"39677","inReplyTo":"xmqq7fqvfl9g.fsf@gitster.dls.corp.google.com","subject":"Re: Improvements to integer option parsing","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-06-22T22:42:55Z","receivedAt":"2015-06-22T22:42:55Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"\n> On 22 Jun 2015, at 23:09, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Charles Bailey <charles@hashpling.org> writes:\n>> \n>> - marginally improved the opterror message on failed parses\n> \n> I'd queue with \"s/a integer/a non-negative integer/\".\n\nHa! That's what I had before I submitted, but then the source line got quite long (which could have been split) and the generated message got quite long as well so I cropped it. This was probably the source of the grammar mistake.\n\nIf you're happy with the longer message, I am happy with it too.\n"},{"id":"264608","messageId":"20150622234624.GA13709@peff.net","threadId":"39677","inReplyTo":"20150622220350.GB18677@hashpling.org","subject":"Re: [PATCH 8/7] cat-file: sort and de-dup output of --batch-all-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T23:46:25Z","receivedAt":"2015-06-22T23:46:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 22, 2015 at 11:03:50PM +0100, Charles Bailey wrote:\n\n> > The patch below does the sort/de-dup. I'd probably just squash it into\n> > patch 7, though.\n> \n> Woah, 8 out of 7! Did you get a chance to measure the performance hit of\n> the sort? If not, I may test it out when I next get the chance.\n\nNo, that last patch was my \"eh, one more thing before bed\" patch. ;)\n\nIt's easy enough to time, though. Running:\n\n  git cat-file --batch-all-objects \\\n               --batch-check='%(objectsize) %(objectname)' \\\n\t       --buffer >/dev/null\n\non linux.git, my best-of-five goes from (no sorting):\n\n  real    0m3.604s\n  user    0m3.556s\n  sys     0m0.048s\n\nto (with sorting):\n\n  real    0m4.053s\n  user    0m4.004s\n  sys     0m0.052s\n\nSo it does matter, but not too much. We could de-dup with a hash table,\nwhich might be a little faster, but I doubt it would make much\ndifference.  It's also mostly in sorted order already; it's possible\nthat a merge sort would behave a little better. I'm not sure how deep\nit's worth going into that rabbit hole.\n\n-Peff\n"},{"id":"264609","messageId":"20150622235001.GB13709@peff.net","threadId":"39677","inReplyTo":"xmqqk2uvfm5p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Add list-all-objects command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-22T23:50:01Z","receivedAt":"2015-06-22T23:50:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 22, 2015 at 02:50:10PM -0700, Junio C Hamano wrote:\n\n> > We may want to take patch 1 separately for the maint-track, as it is\n> > really a bug-fix (albeit one that I do not think actually affects anyone\n> > in practice right now).\n> \n> Hmph, add_unseen_recent_objects_to_traversal() is the only existing\n> user, and before d3038d22 (prune: keep objects reachable from recent\n> objects, 2014-10-15) added that function, for-each-packed-object\n> existed but had no callers.\n\nI think that is because it was added by d3038d22^. :)\n\n> And the objects not beeing seen by that function (due to lack of\n> \"open\") would matter only for pruning purposes, which would mean\n> you have to be calling into the codepath when running a full repack,\n> so you would have opened all the packs that matter anyway (if you\n> have a \"old cruft archive\" pack that contains only objects that\n> are unreachable, you may not have opened that pack, though, and you\n> may prune the thing away prematurely).\n\nExactly, that matches my analysis.\n\n> So, I think I can agree that this would unlikely affect anybody in\n> practice.\n\nYep. I am OK if we do not even worry about it for \"maint\", then.\n\n-Peff\n"},{"id":"264933","messageId":"CAPig+cT-VC7eQgLec+ATux76GHdRBVwG9BqcR9QiqXntf+s4eg@mail.gmail.com","threadId":"39677","inReplyTo":"20150622104559.GG14475@peff.net","subject":"Re: [PATCH 7/7] cat-file: add --batch-all-objects option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-26T06:56:58Z","receivedAt":"2015-06-26T06:56:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 22, 2015 at 6:45 AM, Jeff King <peff@peff.net> wrote:\n> [...] This patch adds an option to\n> \"cat-file --batch-check\" to operate on all available\n> objects (rather than reading names from stdin).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 93a4794..2b4220a 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -547,4 +547,31 @@ test_expect_success 'git cat-file --batch --follow-symlink returns correct sha a\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success 'cat-file --batch-all-objects shows all objects' '\n> +       # make new repos so we now the full set of objects; we will\n\ns/now/know/\n\n> +       # also make sure that there are some packed and some loose\n> +       # objects, some referenced and some not, and that there are\n> +       # some available only via alternates.\n> +       git init all-one &&\n> +       (\n> +               cd all-one &&\n> +               echo content >file &&\n> +               git add file &&\n> +               git commit -qm base &&\n> +               git rev-parse HEAD HEAD^{tree} HEAD:file &&\n> +               git repack -ad &&\n> +               echo not-cloned | git hash-object -w --stdin\n> +       ) >expect.unsorted &&\n> +       git clone -s all-one all-two &&\n> +       (\n> +               cd all-two &&\n> +               echo local-unref | git hash-object -w --stdin\n> +       ) >>expect.unsorted &&\n> +       sort <expect.unsorted >expect &&\n> +       git -C all-two cat-file --batch-all-objects \\\n> +                               --batch-check=\"%(objectname)\" >actual.unsorted &&\n> +       sort <actual.unsorted >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.4.4.719.g3984bc6\n"},{"id":"264955","messageId":"20150626154822.GA30273@peff.net","threadId":"39677","inReplyTo":"CAPig+cT-VC7eQgLec+ATux76GHdRBVwG9BqcR9QiqXntf+s4eg@mail.gmail.com","subject":"Re: [PATCH 7/7] cat-file: add --batch-all-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-26T15:48:22Z","receivedAt":"2015-06-26T15:48:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 26, 2015 at 02:56:58AM -0400, Eric Sunshine wrote:\n\n> > +test_expect_success 'cat-file --batch-all-objects shows all objects' '\n> > +       # make new repos so we now the full set of objects; we will\n> \n> s/now/know/\n\nYeah. I don't think this series otherwise needs re-rolled. Here it is in\nan autosquash-able form:\n\n-- >8 --\nSubject: [PATCH] fixup! cat-file: add --batch-all-objects option\n\n---\n t/t1006-cat-file.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 18dbdc8..4f38078 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -548,7 +548,7 @@ test_expect_success 'git cat-file --batch --follow-symlink returns correct sha a\n '\n \n test_expect_success 'cat-file --batch-all-objects shows all objects' '\n-\t# make new repos so we now the full set of objects; we will\n+\t# make new repos so we know the full set of objects; we will\n \t# also make sure that there are some packed and some loose\n \t# objects, some referenced and some not, and that there are\n \t# some available only via alternates.\n-- \n2.5.0.rc0.336.g8460790\n"}]}