[PATCH] pack-objects: convert to use parse_options()

[PATCH] pack-objects: convert to use parse_options()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c |  317 +++++++++++++++++++++---------------------------
 1 files changed, 141 insertions(+), 176 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0f2e7b8..59fdb3f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -18,16 +18,11 @@
 #include "refs.h"
 #include "thread-utils.h"
 
-static const char pack_usage[] =
-  "git pack-objects [ -q | --progress | --all-progress ]\n"
-  "        [--all-progress-implied]\n"
-  "        [--max-pack-size=<n>] [--local] [--incremental]\n"
-  "        [--window=<n>] [--window-memory=<n>] [--depth=<n>]\n"
-  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
-  "        [--threads=<n>] [--non-empty] [--revs [--unpacked | --all]]\n"
-  "        [--reflog] [--stdout | base-name] [--include-tag]\n"
-  "        [--keep-unreachable | --unpack-unreachable]\n"
-  "        [< ref-list | < object-list]";
+static const char *pack_usage[] = {
+	"git pack-objects --stdout [options...] [< ref-list | < object-list]",
+	"git pack-objects [options...] base-name [< ref-list | < object-list]",
+	NULL
+};
 
 struct object_entry {
 	struct pack_idx_entry idx;
@@ -2305,183 +2300,140 @@ static void get_object_list(int ac, const char **av)
 		loosen_unused_packed_objects(&revs);
 }
 
+static int option_parse_index_version(const struct option *opt,
+				      const char *arg, int unset)
+{
+	char *c;
+	const char *val = arg;
+	pack_idx_opts.version = strtoul(val, &c, 10);
+	if (pack_idx_opts.version > 2)
+		die("unsupported index version %s", val);
+	if (*c == ',' && c[1])
+		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
+	if (*c || pack_idx_opts.off32_limit & 0x80000000)
+		die("bad index version %s", val);
+	return 0;
+}
+
+static int option_parse_ulong(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (unset)
+		die("option %s does not accept negative form",
+		    opt->long_name);
+
+	if (!git_parse_ulong(arg, opt->value))
+		die("unable to parse value '%s' for option %s",
+		    arg, opt->long_name);
+	return 0;
+}
+
+#define OPT_ULONG(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "n", (h),	\
+	  PARSE_OPT_NONEG, option_parse_ulong }
+
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
 	int thin = 0;
 	int all_progress_implied = 0;
-	uint32_t i;
-	const char **rp_av;
-	int rp_ac_alloc = 64;
-	int rp_ac;
+	const char *rp_av[6];
+	int rp_ac = 0;
+	int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
+	struct option pack_objects_options[] = {
+		OPT_SET_INT('q', "quiet", &progress,
+			    "do not show progress meter", 0),
+		OPT_SET_INT(0, "progress", &progress,
+			    "show progress meter", 1),
+		OPT_SET_INT(0, "all-progress", &progress,
+			    "show progress meter during object writing phase", 2),
+		OPT_BOOL(0, "all-progress-implied",
+			 &all_progress_implied,
+			 "similar to --all-progress when progress meter is shown"),
+		{ OPTION_CALLBACK, 0, "index-version", NULL, "version",
+		  "force generating pack index at a particular version",
+		  0, option_parse_index_version },
+		OPT_ULONG(0, "max-pack-size", &pack_size_limit,
+			  "maximum size of each output pack file"),
+		OPT_BOOL(0, "local", &local,
+			 "ignore borrowed objects from alternate object store"),
+		OPT_BOOL(0, "incremental", &incremental,
+			 "ignore packed objects"),
+		OPT_INTEGER(0, "window", &window,
+			    "limit pack window by objects"),
+		OPT_ULONG(0, "window-memory", &window_memory_limit,
+			  "limit pack window by memory"),
+		OPT_INTEGER(0, "depth", &depth,
+			    "limit pack window by maximum delta depth"),
+		OPT_BOOL(0, "reuse-delta", &reuse_delta,
+			 "reusing existing deltas"),
+		OPT_BOOL(0, "reuse-object", &reuse_object,
+			 "reusing existing objects"),
+		OPT_BOOL(0, "delta-base-offset", &allow_ofs_delta,
+			 "use OFS_DELTA objects"),
+		OPT_INTEGER(0, "threads", &delta_search_threads,
+			    "use threads when searching for best delta matches"),
+		OPT_BOOL(0, "non-empty", &non_empty,
+			 "only create if it would contain at least one object"),
+		OPT_BOOL(0, "revs", &use_internal_rev_list,
+			 "read revision arguments from standard output"),
+		{ OPTION_SET_INT, 0, "unpacked", &rev_list_unpacked, NULL,
+		  "limit the objects to those that are not already packed",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		{ OPTION_SET_INT, 0, "all", &rev_list_all, NULL,
+		  "include all refs under $GIT_DIR/refs directory",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		{ OPTION_SET_INT, 0, "reflog", &rev_list_reflog, NULL,
+		  "include objects referred by reflog entries",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		OPT_BOOL(0, "stdout", &pack_to_stdout,
+			 "output pack to stdout"),
+		OPT_BOOL(0, "include-tag", &include_tag,
+			 "include unasked-for annotated tags"),
+		OPT_BOOL(0, "keep-unreachable", &keep_unreachable,
+			 "keep unreachable objects"),
+		OPT_BOOL(0, "unpack-unreachable", &unpack_unreachable,
+			 "unpack unreachable objects"),
+		OPT_BOOL(0, "thin", &thin,
+			 "create thin packs"),
+		OPT_BOOL(0, "honor-pack-keep", &ignore_packed_keep,
+			 "ignore packs that have companion .keep file"),
+		OPT_INTEGER(0, "compression", &pack_compression_level,
+			    "pack compression level"),
+		OPT_SET_INT(0, "keep-true-parents", &grafts_replace_parents,
+			    "do not hide commits by grafts", 0),
+		OPT_END(),
+	};
 
 	read_replace_refs = 0;
 
-	rp_av = xcalloc(rp_ac_alloc, sizeof(*rp_av));
-
-	rp_av[0] = "pack-objects";
-	rp_av[1] = "--objects"; /* --thin will make it --objects-edge */
-	rp_ac = 2;
-
 	reset_pack_idx_option(&pack_idx_opts);
 	git_config(git_pack_config, NULL);
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
 
 	progress = isatty(2);
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, prefix, pack_objects_options,
+			     pack_usage, 0);
 
-		if (*arg != '-')
-			break;
+	rp_av[rp_ac++] = "pack-objects";
+	if (thin) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--objects-edge";
+	} else
+		rp_av[rp_ac++] = "--objects";
 
-		if (!strcmp("--non-empty", arg)) {
-			non_empty = 1;
-			continue;
-		}
-		if (!strcmp("--local", arg)) {
-			local = 1;
-			continue;
-		}
-		if (!strcmp("--incremental", arg)) {
-			incremental = 1;
-			continue;
-		}
-		if (!strcmp("--honor-pack-keep", arg)) {
-			ignore_packed_keep = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--compression=")) {
-			char *end;
-			int level = strtoul(arg+14, &end, 0);
-			if (!arg[14] || *end)
-				usage(pack_usage);
-			if (level == -1)
-				level = Z_DEFAULT_COMPRESSION;
-			else if (level < 0 || level > Z_BEST_COMPRESSION)
-				die("bad pack compression level %d", level);
-			pack_compression_level = level;
-			continue;
-		}
-		if (!prefixcmp(arg, "--max-pack-size=")) {
-			pack_size_limit_cfg = 0;
-			if (!git_parse_ulong(arg+16, &pack_size_limit))
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--window=")) {
-			char *end;
-			window = strtoul(arg+9, &end, 0);
-			if (!arg[9] || *end)
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--window-memory=")) {
-			if (!git_parse_ulong(arg+16, &window_memory_limit))
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--threads=")) {
-			char *end;
-			delta_search_threads = strtoul(arg+10, &end, 0);
-			if (!arg[10] || *end || delta_search_threads < 0)
-				usage(pack_usage);
-#ifdef NO_PTHREADS
-			if (delta_search_threads != 1)
-				warning("no threads support, "
-					"ignoring %s", arg);
-#endif
-			continue;
-		}
-		if (!prefixcmp(arg, "--depth=")) {
-			char *end;
-			depth = strtoul(arg+8, &end, 0);
-			if (!arg[8] || *end)
-				usage(pack_usage);
-			continue;
-		}
-		if (!strcmp("--progress", arg)) {
-			progress = 1;
-			continue;
-		}
-		if (!strcmp("--all-progress", arg)) {
-			progress = 2;
-			continue;
-		}
-		if (!strcmp("--all-progress-implied", arg)) {
-			all_progress_implied = 1;
-			continue;
-		}
-		if (!strcmp("-q", arg)) {
-			progress = 0;
-			continue;
-		}
-		if (!strcmp("--no-reuse-delta", arg)) {
-			reuse_delta = 0;
-			continue;
-		}
-		if (!strcmp("--no-reuse-object", arg)) {
-			reuse_object = reuse_delta = 0;
-			continue;
-		}
-		if (!strcmp("--delta-base-offset", arg)) {
-			allow_ofs_delta = 1;
-			continue;
-		}
-		if (!strcmp("--stdout", arg)) {
-			pack_to_stdout = 1;
-			continue;
-		}
-		if (!strcmp("--revs", arg)) {
-			use_internal_rev_list = 1;
-			continue;
-		}
-		if (!strcmp("--keep-unreachable", arg)) {
-			keep_unreachable = 1;
-			continue;
-		}
-		if (!strcmp("--unpack-unreachable", arg)) {
-			unpack_unreachable = 1;
-			continue;
-		}
-		if (!strcmp("--include-tag", arg)) {
-			include_tag = 1;
-			continue;
-		}
-		if (!strcmp("--unpacked", arg) ||
-		    !strcmp("--reflog", arg) ||
-		    !strcmp("--all", arg)) {
-			use_internal_rev_list = 1;
-			if (rp_ac >= rp_ac_alloc - 1) {
-				rp_ac_alloc = alloc_nr(rp_ac_alloc);
-				rp_av = xrealloc(rp_av,
-						 rp_ac_alloc * sizeof(*rp_av));
-			}
-			rp_av[rp_ac++] = arg;
-			continue;
-		}
-		if (!strcmp("--thin", arg)) {
-			use_internal_rev_list = 1;
-			thin = 1;
-			rp_av[1] = "--objects-edge";
-			continue;
-		}
-		if (!prefixcmp(arg, "--index-version=")) {
-			char *c;
-			pack_idx_opts.version = strtoul(arg + 16, &c, 10);
-			if (pack_idx_opts.version > 2)
-				die("bad %s", arg);
-			if (*c == ',')
-				pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
-			if (*c || pack_idx_opts.off32_limit & 0x80000000)
-				die("bad %s", arg);
-			continue;
-		}
-		if (!strcmp(arg, "--keep-true-parents")) {
-			grafts_replace_parents = 0;
-			continue;
-		}
-		usage(pack_usage);
+	if (rev_list_unpacked) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--unpacked";
+	}
+	if (rev_list_all) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--all";
+	}
+	if (rev_list_reflog) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--reflog";
 	}
 
 	/* Traditionally "pack-objects [options] base extra" failed;
@@ -2497,12 +2449,25 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	 * walker.
 	 */
 
-	if (!pack_to_stdout)
-		base_name = argv[i++];
-
-	if (pack_to_stdout != !base_name)
-		usage(pack_usage);
+	if (!pack_to_stdout) {
+		if (!argc)
+			die("base name required if --stdout is not given");
+		base_name = argv[0];
+		argc--;
+	}
+	if (argc)
+		die("base name or --stdout are mutually exclusive");
 
+	if (!reuse_object)
+		reuse_delta = 0;
+	if (pack_compression_level == -1)
+		pack_compression_level = Z_DEFAULT_COMPRESSION;
+	else if (pack_compression_level < 0 || pack_compression_level > Z_BEST_COMPRESSION)
+		die("bad pack compression level %d", pack_compression_level);
+#ifdef NO_PTHREADS
+	if (delta_search_threads != 1)
+		warning("no threads support, ignoring %s", arg);
+#endif
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
-- 
1.7.8.36.g69ee2

Re: [PATCH] pack-objects: convert to use parse_options()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -2305,183 +2300,140 @@ static void get_object_list(int ac, const char **av)
>  		loosen_unused_packed_objects(&revs);
>  }
>  
> +static int option_parse_index_version(const struct option *opt,
> +				      const char *arg, int unset)
> +{
> +	char *c;
> +	const char *val = arg;
> +	pack_idx_opts.version = strtoul(val, &c, 10);
> +	if (pack_idx_opts.version > 2)
> +		die("unsupported index version %s", val);

Dying may have been the appropriate error path in the custom option
parser, but is it still so for a callback in the parse-options framework?

Or should this be a 'return error(_("..."), val)'?

> +	if (*c == ',' && c[1])
> +		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);

I do not think the original had this extra "&& c[1]" check and I didn't
see any explanation of this change in the log message, either.

> +static int option_parse_ulong(const struct option *opt,
> +			      const char *arg, int unset)
> +{
> +	if (unset)
> +		die("option %s does not accept negative form",
> +		    opt->long_name);
> +
> +	if (!git_parse_ulong(arg, opt->value))
> +		die("unable to parse value '%s' for option %s",
> +		    arg, opt->long_name);
> +	return 0;
> +}

Likewise on "die()".

> +#define OPT_ULONG(s, l, v, h) \
> +	{ OPTION_CALLBACK, (s), (l), (v), "n", (h),	\
> +	  PARSE_OPT_NONEG, option_parse_ulong }
> +
>  int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  {
>  	int use_internal_rev_list = 0;
>  	int thin = 0;
>  	int all_progress_implied = 0;
> -	uint32_t i;
> -	const char **rp_av;
> -	int rp_ac_alloc = 64;
> -	int rp_ac;
> +	const char *rp_av[6];

Nice stackframe shrinkage.

> +	int rp_ac = 0;
> +	int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
> +	struct option pack_objects_options[] = {
> +		OPT_SET_INT('q', "quiet", &progress,
> +			    "do not show progress meter", 0),
> +		OPT_SET_INT(0, "progress", &progress,
> +			    "show progress meter", 1),
> +		OPT_SET_INT(0, "all-progress", &progress,
> +			    "show progress meter during object writing phase", 2),

Sounds as if the progress is not shown during the counting phase.
Nothing ", too" at the end could not fix, though.

> +		OPT_BOOL(0, "all-progress-implied",
> +			 &all_progress_implied,
> +			 "similar to --all-progress when progress meter is shown"),

Hrm, interesting wording.

> +		{ OPTION_CALLBACK, 0, "index-version", NULL, "version",
> +		  "force generating pack index at a particular version",
> +		  0, option_parse_index_version },

Sounds as if you can generate a pack index at HEAD~24, but that is not
what you meant. "version" is too loaded a word that needs qualification.

Perhaps "write the pack index file in the specified idx format version".

"index" is also a loaded word, and our documentation tries to use "idx"
when we talk about the pack index (we say "index" when we mean the
dircache).

> +		OPT_INTEGER(0, "window", &window,
> +			    "limit pack window by objects"),
> +		OPT_ULONG(0, "window-memory", &window_memory_limit,
> +			  "limit pack window by memory"),
> +		OPT_INTEGER(0, "depth", &depth,
> +			    "limit pack window by maximum delta depth"),

These descriptions are somewhat interesting.

The "pack window" is limited by number of objects given by --window (and
by default this is set to 10), but if you give --window-memory, it could
further dynamically shrink it under memory pressure. If the logic were to
use this dynamic shrinkage without any limit on the number of objects in
flight when --window-memory and no --window is given, then you could say
"limit by memory" as if the "pack window" is counted in terms of number of
bytes in the window instead of number of objects and the description would
be accurate. But that is not quite the case as far as I know.

And --depth does not affect the size of the pack window at all. It rejects
an object with deep enough delta chain as a delta-base candidate to limit
the run-time performance penalty for the user of the resulting pack, but
does not stop the caller from trying the next object in the window.
"maximum length of delta chain allowed in the resulting pack", perhaps?

> +		OPT_BOOL(0, "reuse-delta", &reuse_delta,
> +			 "reusing existing deltas"),
> +		OPT_BOOL(0, "reuse-object", &reuse_object,
> +			 "reusing existing objects"),

s/reusing/reuse/???

> +		OPT_BOOL(0, "delta-base-offset", &allow_ofs_delta,
> +			 "use OFS_DELTA objects"),
> +		OPT_INTEGER(0, "threads", &delta_search_threads,
> +			    "use threads when searching for best delta matches"),
> +		OPT_BOOL(0, "non-empty", &non_empty,
> +			 "only create if it would contain at least one object"),

create what? "do not create an empty pack output"?

> +		OPT_BOOL(0, "revs", &use_internal_rev_list,
> +			 "read revision arguments from standard output"),

s/output/input/???

> +		{ OPTION_SET_INT, 0, "unpacked", &rev_list_unpacked, NULL,
> +		  "limit the objects to those that are not already packed",
> +		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },

s/already/yet/???

> +		{ OPTION_SET_INT, 0, "all", &rev_list_all, NULL,
> +		  "include all refs under $GIT_DIR/refs directory",
> +		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
> +		{ OPTION_SET_INT, 0, "reflog", &rev_list_reflog, NULL,
> +		  "include objects referred by reflog entries",
> +		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },

The latter is more correct---packfile does not pack refs, it packs
objects.

Perhaps former should match: "include objects reachable from any
reference". I would also prefer to avoid "$GIT_DIR/refs _directory_"
to discourage people from running "ls -R .git/refs/".

> +		OPT_BOOL(0, "stdout", &pack_to_stdout,
> +			 "output pack to stdout"),
> +		OPT_BOOL(0, "include-tag", &include_tag,
> +			 "include unasked-for annotated tags"),

"include tag objects that refer to objects to be packed"?

> +		OPT_BOOL(0, "keep-unreachable", &keep_unreachable,
> +			 "keep unreachable objects"),

What does this option mean these days (aka "where are they kept")?

IIRC, this was meant to be used in conjunction with the --unpacked= and
later with --kept-pack-only (see 03a9683d22) and then was later further
modified by 4d6acb70411.

I *think* this meant to include objects that are in packfiles that are not
marked with .keep even when they are not in the revision range to be
packed, so that we won't lose them during gc.

But I think --unpack-unreachable introduced by ca11b21 (let pack-objects
do the writing of unreachable objects as loose objects, 2008-05-14) that
writes these unreachable objects out of the pack to loose form made this
option unnecessary, and it remains unused ever since. It may not be a bad
idea to deprecate this option.

Obviously it is outside of the scope of this patch.

> +		OPT_BOOL(0, "unpack-unreachable", &unpack_unreachable,
> +			 "unpack unreachable objects"),
> +		OPT_BOOL(0, "thin", &thin,
> +			 "create thin packs"),
> +		OPT_BOOL(0, "honor-pack-keep", &ignore_packed_keep,
> +			 "ignore packs that have companion .keep file"),
> +		OPT_INTEGER(0, "compression", &pack_compression_level,
> +			    "pack compression level"),
> +		OPT_SET_INT(0, "keep-true-parents", &grafts_replace_parents,
> +			    "do not hide commits by grafts", 0),

It is noteworthy that this is truly "do not hide".

A graft entry can also add extra commits, but this option will not prevent
the command from including them, so that the result of repacking will keep
the union of parents in the real history (i.e. in commit objects) and
parents in the pretend history (i.e. added by grafts).

> +		OPT_END(),
> +	};
>  
>  	read_replace_refs = 0;
> ...
> -			continue;
> -		}
> -		usage(pack_usage);
> +	if (rev_list_unpacked) {
> +		use_internal_rev_list = 1;
> +		rp_av[rp_ac++] = "--unpacked";
> +	}
> +	if (rev_list_all) {
> +		use_internal_rev_list = 1;
> +		rp_av[rp_ac++] = "--all";
> +	}
> +	if (rev_list_reflog) {
> +		use_internal_rev_list = 1;
> +		rp_av[rp_ac++] = "--reflog";
>  	}

I'd prefer to have "unpacked" the last, to match the order of parameters
in which the typical caller, i.e. repack, calls us, but that is minor.

> @@ -2497,12 +2449,25 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	 * walker.
>  	 */
>  
> -	if (!pack_to_stdout)
> -		base_name = argv[i++];
> -
> -	if (pack_to_stdout != !base_name)
> -		usage(pack_usage);
> +	if (!pack_to_stdout) {
> +		if (!argc)
> +			die("base name required if --stdout is not given");
> +		base_name = argv[0];
> +		argc--;
> +	}
> +	if (argc)
> +		die("base name or --stdout are mutually exclusive");

Doesn't this change regress the most basic, i.e.

	$ git pack-objects
        usage: ...

Other than that, thanks for a pleasant and thought-provoking read.

Re: [PATCH] pack-objects: convert to use parse_options()

[Nguyen Thai Ngoc Duy]

I'm going to address your comments soon (thanks by the way, making up
summary line for each option from git-pack-objects.txt was painful)
but since you like thought-provoking, check this comment out:

	/* Traditionally "pack-objects [options] base extra" failed;
	 * we would however want to take refs parameter that would
	 * have been given to upstream rev-list ourselves, which means
	 * we somehow want to say what the base name is.  So the
	 * syntax would be:
	 *
	 * pack-objects [options] base <refs...>
	 *
	 * in other words, we would treat the first non-option as the
	 * base_name and send everything else to the internal revision
	 * walker.
	 */

The last paragraph does not hold true anymore as far as I understand
the code. IOW it drops everything in argv after base name. Looking
back in history, the change was made by 8d1d8f8 (pack-objects: further
work on internal rev-list logic. - 2006-09-06). Nothing is mentioned
why it drops the remaining of argv. I suspect this is a regression and
we should reinstate the behavior as the comment says.
-- 
Duy

Re: [PATCH] pack-objects: convert to use parse_options()

[Nguyen Thai Ngoc Duy]

On Wed, Feb 1, 2012 at 9:30 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>        /* Traditionally "pack-objects [options] base extra" failed;
>         * we would however want to take refs parameter that would
>         * have been given to upstream rev-list ourselves, which means
>         * we somehow want to say what the base name is.  So the
>         * syntax would be:
>         *
>         * pack-objects [options] base <refs...>
>         *
>         * in other words, we would treat the first non-option as the
>         * base_name and send everything else to the internal revision
>         * walker.
>         */
>
> The last paragraph does not hold true anymore as far as I understand
> the code. IOW it drops everything in argv after base name. Looking
> back in history, the change was made by 8d1d8f8 (pack-objects: further
> work on internal rev-list logic. - 2006-09-06). Nothing is mentioned
> why it drops the remaining of argv. I suspect this is a regression and
> we should reinstate the behavior as the comment says.

Nope, false alarm. The comment was introduced in b5d97e6
(pack-objects: run rev-list equivalent internally. - 2006-09-04) and
the remaining of argv has been ignored ever since.
-- 
Duy

[PATCH v2 1/3] pack-objects: do not accept "--index-version=version,"

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c |    2 +-
 t/t5302-pack-index.sh  |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0f2e7b8..297f792 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2471,7 +2471,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			pack_idx_opts.version = strtoul(arg + 16, &c, 10);
 			if (pack_idx_opts.version > 2)
 				die("bad %s", arg);
-			if (*c == ',')
+			if (*c == ',' && c[1])
 				pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
 			if (*c || pack_idx_opts.off32_limit & 0x80000000)
 				die("bad %s", arg);
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index f8fa924..fe82025 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -74,6 +74,10 @@ test_expect_success 'index-pack --verify on index version 2' '
 '
 
 test_expect_success \
+    'pack-objects --index-version=2, is not accepted' \
+    'test_must_fail git pack-objects --index-version=2, test-3 <obj-list'
+
+test_expect_success \
     'index v2: force some 64-bit offsets with pack-objects' \
     'pack3=$(git pack-objects --index-version=2,0x40000 test-3 <obj-list)'
 
-- 
1.7.8.36.g69ee2

[PATCH v2 2/3] pack-objects: remove bogus comment

[Nguyễn Thái Ngọc Duy]

The comment was introduced in b5d97e6 (pack-objects: run rev-list
equivalent internally. - 2006-09-04), stating that

git pack-objects [options] base-name <refs...>

is acceptable and refs should be passed into rev-list. But that's not
true. All arguments after base-name are ignored.

Remove the comment and reject this syntax (i.e. no more arguments after
base name)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c |   15 +--------------
 t/t5300-pack-object.sh |    4 ++++
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 297f792..80e3114 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2484,23 +2484,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		usage(pack_usage);
 	}
 
-	/* Traditionally "pack-objects [options] base extra" failed;
-	 * we would however want to take refs parameter that would
-	 * have been given to upstream rev-list ourselves, which means
-	 * we somehow want to say what the base name is.  So the
-	 * syntax would be:
-	 *
-	 * pack-objects [options] base <refs...>
-	 *
-	 * in other words, we would treat the first non-option as the
-	 * base_name and send everything else to the internal revision
-	 * walker.
-	 */
-
 	if (!pack_to_stdout)
 		base_name = argv[i++];
 
-	if (pack_to_stdout != !base_name)
+	if (pack_to_stdout != !base_name || argv[i])
 		usage(pack_usage);
 
 	if (!pack_to_stdout && !pack_size_limit)
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 602806d..d9d856b 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -38,6 +38,10 @@ test_expect_success \
     'pack without delta' \
     'packname_1=$(git pack-objects --window=0 test-1 <obj-list)'
 
+test_expect_success \
+    'pack-objects with bogus arguments' \
+    'test_must_fail git pack-objects --window=0 test-1 blah blah <obj-list'
+
 rm -fr .git2
 mkdir .git2
 
-- 
1.7.8.36.g69ee2

[PATCH v2 3/3] pack-objects: convert to use parse_options()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Regarding die() vs error() in option_parse_*, nope, using error()
 makes parse_options() print command usage, which is long and distracts
 users from the real error message. I'm going to stick with die().

 builtin/pack-objects.c |  315 +++++++++++++++++++++---------------------------
 1 files changed, 139 insertions(+), 176 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 80e3114..e21e5af 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -18,16 +18,11 @@
 #include "refs.h"
 #include "thread-utils.h"
 
-static const char pack_usage[] =
-  "git pack-objects [ -q | --progress | --all-progress ]\n"
-  "        [--all-progress-implied]\n"
-  "        [--max-pack-size=<n>] [--local] [--incremental]\n"
-  "        [--window=<n>] [--window-memory=<n>] [--depth=<n>]\n"
-  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
-  "        [--threads=<n>] [--non-empty] [--revs [--unpacked | --all]]\n"
-  "        [--reflog] [--stdout | base-name] [--include-tag]\n"
-  "        [--keep-unreachable | --unpack-unreachable]\n"
-  "        [< ref-list | < object-list]";
+static const char *pack_usage[] = {
+	"git pack-objects --stdout [options...] [< ref-list | < object-list]",
+	"git pack-objects [options...] base-name [< ref-list | < object-list]",
+	NULL
+};
 
 struct object_entry {
 	struct pack_idx_entry idx;
@@ -2305,191 +2300,159 @@ static void get_object_list(int ac, const char **av)
 		loosen_unused_packed_objects(&revs);
 }
 
+static int option_parse_index_version(const struct option *opt,
+				      const char *arg, int unset)
+{
+	char *c;
+	const char *val = arg;
+	pack_idx_opts.version = strtoul(val, &c, 10);
+	if (pack_idx_opts.version > 2)
+		die(_("unsupported index version %s"), val);
+	if (*c == ',' && c[1])
+		pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
+	if (*c || pack_idx_opts.off32_limit & 0x80000000)
+		die(_("bad index version '%s'"), val);
+	return 0;
+}
+
+static int option_parse_ulong(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (unset)
+		die(_("option %s does not accept negative form"),
+		    opt->long_name);
+
+	if (!git_parse_ulong(arg, opt->value))
+		die(_("unable to parse value '%s' for option %s"),
+		    arg, opt->long_name);
+	return 0;
+}
+
+#define OPT_ULONG(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "n", (h),	\
+	  PARSE_OPT_NONEG, option_parse_ulong }
+
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
 	int thin = 0;
 	int all_progress_implied = 0;
-	uint32_t i;
-	const char **rp_av;
-	int rp_ac_alloc = 64;
-	int rp_ac;
+	const char *rp_av[6];
+	int rp_ac = 0;
+	int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
+	struct option pack_objects_options[] = {
+		OPT_SET_INT('q', "quiet", &progress,
+			    "do not show progress meter", 0),
+		OPT_SET_INT(0, "progress", &progress,
+			    "show progress meter", 1),
+		OPT_SET_INT(0, "all-progress", &progress,
+			    "show progress meter during object writing phase", 2),
+		OPT_BOOL(0, "all-progress-implied",
+			 &all_progress_implied,
+			 "similar to --all-progress when progress meter is shown"),
+		{ OPTION_CALLBACK, 0, "index-version", NULL, "version[,offset]",
+		  "write the pack index file in the specified idx format version",
+		  0, option_parse_index_version },
+		OPT_ULONG(0, "max-pack-size", &pack_size_limit,
+			  "maximum size of each output pack file"),
+		OPT_BOOL(0, "local", &local,
+			 "ignore borrowed objects from alternate object store"),
+		OPT_BOOL(0, "incremental", &incremental,
+			 "ignore packed objects"),
+		OPT_INTEGER(0, "window", &window,
+			    "limit pack window by objects"),
+		OPT_ULONG(0, "window-memory", &window_memory_limit,
+			  "limit pack window by memory in addition to object limit"),
+		OPT_INTEGER(0, "depth", &depth,
+			    "maximum length of delta chain allowed in the resulting pack"),
+		OPT_BOOL(0, "reuse-delta", &reuse_delta,
+			 "reuse existing deltas"),
+		OPT_BOOL(0, "reuse-object", &reuse_object,
+			 "reuse existing objects"),
+		OPT_BOOL(0, "delta-base-offset", &allow_ofs_delta,
+			 "use OFS_DELTA objects"),
+		OPT_INTEGER(0, "threads", &delta_search_threads,
+			    "use threads when searching for best delta matches"),
+		OPT_BOOL(0, "non-empty", &non_empty,
+			 "do not create an empty pack output"),
+		OPT_BOOL(0, "revs", &use_internal_rev_list,
+			 "read revision arguments from standard input"),
+		{ OPTION_SET_INT, 0, "unpacked", &rev_list_unpacked, NULL,
+		  "limit the objects to those that are not yet packed",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		{ OPTION_SET_INT, 0, "all", &rev_list_all, NULL,
+		  "include objects reachable from any reference",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		{ OPTION_SET_INT, 0, "reflog", &rev_list_reflog, NULL,
+		  "include objects referred by reflog entries",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		OPT_BOOL(0, "stdout", &pack_to_stdout,
+			 "output pack to stdout"),
+		OPT_BOOL(0, "include-tag", &include_tag,
+			 "include tag objects that refer to objects to be packed"),
+		OPT_BOOL(0, "keep-unreachable", &keep_unreachable,
+			 "keep unreachable objects"),
+		OPT_BOOL(0, "unpack-unreachable", &unpack_unreachable,
+			 "unpack unreachable objects"),
+		OPT_BOOL(0, "thin", &thin,
+			 "create thin packs"),
+		OPT_BOOL(0, "honor-pack-keep", &ignore_packed_keep,
+			 "ignore packs that have companion .keep file"),
+		OPT_INTEGER(0, "compression", &pack_compression_level,
+			    "pack compression level"),
+		OPT_SET_INT(0, "keep-true-parents", &grafts_replace_parents,
+			    "do not hide commits by grafts", 0),
+		OPT_END(),
+	};
 
 	read_replace_refs = 0;
 
-	rp_av = xcalloc(rp_ac_alloc, sizeof(*rp_av));
-
-	rp_av[0] = "pack-objects";
-	rp_av[1] = "--objects"; /* --thin will make it --objects-edge */
-	rp_ac = 2;
-
 	reset_pack_idx_option(&pack_idx_opts);
 	git_config(git_pack_config, NULL);
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
 
 	progress = isatty(2);
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, prefix, pack_objects_options,
+			     pack_usage, 0);
 
-		if (*arg != '-')
-			break;
-
-		if (!strcmp("--non-empty", arg)) {
-			non_empty = 1;
-			continue;
-		}
-		if (!strcmp("--local", arg)) {
-			local = 1;
-			continue;
-		}
-		if (!strcmp("--incremental", arg)) {
-			incremental = 1;
-			continue;
-		}
-		if (!strcmp("--honor-pack-keep", arg)) {
-			ignore_packed_keep = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--compression=")) {
-			char *end;
-			int level = strtoul(arg+14, &end, 0);
-			if (!arg[14] || *end)
-				usage(pack_usage);
-			if (level == -1)
-				level = Z_DEFAULT_COMPRESSION;
-			else if (level < 0 || level > Z_BEST_COMPRESSION)
-				die("bad pack compression level %d", level);
-			pack_compression_level = level;
-			continue;
-		}
-		if (!prefixcmp(arg, "--max-pack-size=")) {
-			pack_size_limit_cfg = 0;
-			if (!git_parse_ulong(arg+16, &pack_size_limit))
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--window=")) {
-			char *end;
-			window = strtoul(arg+9, &end, 0);
-			if (!arg[9] || *end)
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--window-memory=")) {
-			if (!git_parse_ulong(arg+16, &window_memory_limit))
-				usage(pack_usage);
-			continue;
-		}
-		if (!prefixcmp(arg, "--threads=")) {
-			char *end;
-			delta_search_threads = strtoul(arg+10, &end, 0);
-			if (!arg[10] || *end || delta_search_threads < 0)
-				usage(pack_usage);
-#ifdef NO_PTHREADS
-			if (delta_search_threads != 1)
-				warning("no threads support, "
-					"ignoring %s", arg);
-#endif
-			continue;
-		}
-		if (!prefixcmp(arg, "--depth=")) {
-			char *end;
-			depth = strtoul(arg+8, &end, 0);
-			if (!arg[8] || *end)
-				usage(pack_usage);
-			continue;
-		}
-		if (!strcmp("--progress", arg)) {
-			progress = 1;
-			continue;
-		}
-		if (!strcmp("--all-progress", arg)) {
-			progress = 2;
-			continue;
-		}
-		if (!strcmp("--all-progress-implied", arg)) {
-			all_progress_implied = 1;
-			continue;
-		}
-		if (!strcmp("-q", arg)) {
-			progress = 0;
-			continue;
-		}
-		if (!strcmp("--no-reuse-delta", arg)) {
-			reuse_delta = 0;
-			continue;
-		}
-		if (!strcmp("--no-reuse-object", arg)) {
-			reuse_object = reuse_delta = 0;
-			continue;
-		}
-		if (!strcmp("--delta-base-offset", arg)) {
-			allow_ofs_delta = 1;
-			continue;
-		}
-		if (!strcmp("--stdout", arg)) {
-			pack_to_stdout = 1;
-			continue;
-		}
-		if (!strcmp("--revs", arg)) {
-			use_internal_rev_list = 1;
-			continue;
-		}
-		if (!strcmp("--keep-unreachable", arg)) {
-			keep_unreachable = 1;
-			continue;
-		}
-		if (!strcmp("--unpack-unreachable", arg)) {
-			unpack_unreachable = 1;
-			continue;
-		}
-		if (!strcmp("--include-tag", arg)) {
-			include_tag = 1;
-			continue;
-		}
-		if (!strcmp("--unpacked", arg) ||
-		    !strcmp("--reflog", arg) ||
-		    !strcmp("--all", arg)) {
-			use_internal_rev_list = 1;
-			if (rp_ac >= rp_ac_alloc - 1) {
-				rp_ac_alloc = alloc_nr(rp_ac_alloc);
-				rp_av = xrealloc(rp_av,
-						 rp_ac_alloc * sizeof(*rp_av));
-			}
-			rp_av[rp_ac++] = arg;
-			continue;
-		}
-		if (!strcmp("--thin", arg)) {
-			use_internal_rev_list = 1;
-			thin = 1;
-			rp_av[1] = "--objects-edge";
-			continue;
-		}
-		if (!prefixcmp(arg, "--index-version=")) {
-			char *c;
-			pack_idx_opts.version = strtoul(arg + 16, &c, 10);
-			if (pack_idx_opts.version > 2)
-				die("bad %s", arg);
-			if (*c == ',' && c[1])
-				pack_idx_opts.off32_limit = strtoul(c+1, &c, 0);
-			if (*c || pack_idx_opts.off32_limit & 0x80000000)
-				die("bad %s", arg);
-			continue;
-		}
-		if (!strcmp(arg, "--keep-true-parents")) {
-			grafts_replace_parents = 0;
-			continue;
-		}
-		usage(pack_usage);
+	if (argc) {
+		base_name = argv[0];
+		argc--;
 	}
+	if (pack_to_stdout != !base_name || argc)
+		usage_with_options(pack_usage, pack_objects_options);
 
-	if (!pack_to_stdout)
-		base_name = argv[i++];
+	rp_av[rp_ac++] = "pack-objects";
+	if (thin) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--objects-edge";
+	} else
+		rp_av[rp_ac++] = "--objects";
 
-	if (pack_to_stdout != !base_name || argv[i])
-		usage(pack_usage);
+	if (rev_list_all) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--all";
+	}
+	if (rev_list_reflog) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--reflog";
+	}
+	if (rev_list_unpacked) {
+		use_internal_rev_list = 1;
+		rp_av[rp_ac++] = "--unpacked";
+	}
 
+	if (!reuse_object)
+		reuse_delta = 0;
+	if (pack_compression_level == -1)
+		pack_compression_level = Z_DEFAULT_COMPRESSION;
+	else if (pack_compression_level < 0 || pack_compression_level > Z_BEST_COMPRESSION)
+		die("bad pack compression level %d", pack_compression_level);
+#ifdef NO_PTHREADS
+	if (delta_search_threads != 1)
+		warning("no threads support, ignoring %s", arg);
+#endif
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
-- 
1.7.8.36.g69ee2

Re: [PATCH v2 1/3] pack-objects: do not accept "--index-version=version,"

[Junio C Hamano]

Thanks; will queue all three in the series.