git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCHv4] repack: rewrite the shell script in C.

From
Johannes Sixt <j6t@kdbg.org>
Date
Aug 20, 2013, 13:31 UTC
Message-ID
<52136F9C.6030308@kdbg.org>
In-Reply-To
<1376954619-24314-1-git-send-email-stefanbeller@googlemail.com>
I didn't look at functions above cmd_repack.
Am 20.08.2013 01:23, schrieb Stefan Beller:
Show 22 quoted lines
> +int cmd_repack(int argc, const char **argv, const char *prefix) {
> +
> +	int pack_everything = 0;
> +	int pack_everything_but_loose = 0;
> +	int delete_redundant = 0;
> +	char *unpack_unreachable = NULL;
> +	int window = 0, window_memory = 0;
> +	int depth = 0;
> +	int max_pack_size = 0;
> +	int no_reuse_delta = 0, no_reuse_object = 0;
> +	int no_update_server_info = 0;
> +	int quiet = 0;
> +	int local = 0;
> +	char *packdir, *packtmp;
> +	struct child_process cmd;
> +	struct string_list_item *item;
> +	struct string_list existing_packs = STRING_LIST_INIT_DUP;
> +	struct stat statbuffer;
> +	int ext;
> +	char *exts[2] = {".idx", ".pack"};
> +
> +	struct option builtin_repack_options[] = {
Are the long forms of options your invention?
> +		OPT_BOOL('a', "all", &pack_everything,
> +				N_("pack everything in a single pack")),
> +		OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
> +				N_("same as -a, and turn unreachable objects loose")),

--all-but-loose does not express what the help text says. The long form of -A is --all --unpack-unreachable, so it is really just a short option for convenience. It does not need its own long form.

Show 6 quoted lines
> +		OPT_BOOL('d', "delete-redundant", &delete_redundant,
> +				N_("remove redundant packs, and run git-prune-packed")),
> +		OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
> +				N_("pass --no-reuse-delta to git-pack-objects")),
> +		OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
> +				N_("pass --no-reuse-object to git-pack-objects")),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
> +		OPT_BOOL('n', NULL, &no_update_server_info,
> +				N_("do not run git-update-server-info")),
No long option name?
> +		OPT__QUIET(&quiet, N_("be quiet")),
> +		OPT_BOOL('l', "local", &local,
> +				N_("pass --local to git-pack-objects")),
Good.
> +		OPT_STRING(0, "unpack-unreachable", &unpack_unreachable, N_("approxidate"),
> +				N_("with -A, do not loosen objects older than this Packing constraints")),

"Packing constraints" is a section heading, not a continuation of the previous help text.

> +		OPT_INTEGER(0, "window", &window,
> +				N_("size of the window used for delta compression")),

This help text is suboptimal as the option is a count, not a "size" in the narrow sense. But that can be changed later (as it would affect other tools as well, I guess).

Show 8 quoted lines
> +		OPT_INTEGER(0, "window-memory", &window_memory,
> +				N_("same as the above, but limit memory size instead of entries count")),
> +		OPT_INTEGER(0, "depth", &depth,
> +				N_("limits the maximum delta depth")),
> +		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
> +				N_("maximum size of each packfile")),
> +		OPT_END()
> +	};
Good.
Show 7 quoted lines
> +
> +	git_config(repack_config, NULL);
> +
> +	argc = parse_options(argc, argv, prefix, builtin_repack_options,
> +				git_repack_usage, 0);
> +
> +	sigchain_push_common(remove_pack_on_signal);
Good.
> +	packdir = mkpathdup("%s/pack", get_object_directory());
> +	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, getpid());
Should this not be
	packdir = xstrdup(git_path("pack"));
	packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));

Perhaps make packdir and packtmp global so that the strings need not be duplicated in get_pack_filenames and remove_temporary_files?

> +
> +	remove_temporary_files();
Yes, the shell script had this. But is it really necessary?
> +
> +	struct argv_array cmd_args = ARGV_ARRAY_INIT;
Declaration after statement.
Show 24 quoted lines
> +	argv_array_push(&cmd_args, "pack-objects");
> +	argv_array_push(&cmd_args, "--keep-true-parents");
> +	argv_array_push(&cmd_args, "--honor-pack-keep");
> +	argv_array_push(&cmd_args, "--non-empty");
> +	argv_array_push(&cmd_args, "--all");
> +	argv_array_push(&cmd_args, "--reflog");
> +
> +	if (window)
> +		argv_array_pushf(&cmd_args, "--window=%u", window);
> +
> +	if (window_memory)
> +		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
> +
> +	if (depth)
> +		argv_array_pushf(&cmd_args, "--depth=%u", depth);
> +
> +	if (max_pack_size)
> +		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
> +
> +	if (no_reuse_delta)
> +		argv_array_pushf(&cmd_args, "--no-reuse-delta");
> +
> +	if (no_reuse_object)
> +		argv_array_pushf(&cmd_args, "--no-reuse-object");

no_reuse_delta and no_reuse_object are mutually exclusive, according to the shell script version.

Show 11 quoted lines
> +
> +	if (pack_everything + pack_everything_but_loose == 0) {
> +		argv_array_push(&cmd_args, "--unpacked");
> +		argv_array_push(&cmd_args, "--incremental");
> +	} else {
> +		struct string_list fname_list = STRING_LIST_INIT_DUP;
> +		get_pack_filenames(packdir, &fname_list);
> +		for_each_string_list_item(item, &fname_list) {
> +			char *fname;
> +			fname = mkpathdup("%s/%s.keep", packdir, item->string);
> +			if (stat(fname, &statbuffer) && S_ISREG(statbuffer.st_mode)) {
			if (!stat(fname, &statbuffer) && ...

But you are using file_exists() later. That should be good enough here as well, no?

Show 23 quoted lines
> +				/* when the keep file is there, we're ignoring that pack */
> +			} else {
> +				string_list_append(&existing_packs, item->string);
> +			}
> +			free(fname);
> +		}
> +
> +		if (existing_packs.nr && delete_redundant) {
> +			if (unpack_unreachable)
> +				argv_array_pushf(&cmd_args, "--unpack-unreachable=%s", unpack_unreachable);
> +			else if (pack_everything_but_loose)
> +				argv_array_push(&cmd_args, "--unpack-unreachable");
> +		}
> +	}
> +
> +	if (local)
> +		argv_array_push(&cmd_args,  "--local");
> +	if (quiet)
> +		argv_array_push(&cmd_args,  "--quiet");
> +	if (delta_base_offset)
> +		argv_array_push(&cmd_args,  "--delta-base-offset");
> +
> +	argv_array_push(&cmd_args, packtmp);
Otherwise, argument setup looks fine.
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.argv = argv_array_detach(&cmd_args, NULL);
Is it necessary to detach the arguments?
Show 6 quoted lines
> +	cmd.git_cmd = 1;
> +	cmd.out = -1;
> +	cmd.no_stdin = 1;
> +
> +	if (run_command(&cmd))
> +		return 1;

You cannot run_command() and then later read its output! You must split it into start_command(), read stdout, finish_command().

> +
> +	struct string_list names = STRING_LIST_INIT_DUP;
> +	struct string_list rollback = STRING_LIST_INIT_DUP;
Declaration after statement.
Show 7 quoted lines
> +
> +	char line[1024];
> +	int counter = 0;
> +	FILE *out = xfdopen(cmd.out, "r");
> +	while (fgets(line, sizeof(line), out)) {
> +		/* a line consists of 40 hex chars + '\n' */
> +		assert(strlen(line) == 41);

You cannot make assertions about input that you read from an external command! You can die() if the expectation is not met. But I think that in this case the only necessary expectation is that a line is not empty.

BTW, don't we have strbuf functions to read from an fd linewise?
Show 6 quoted lines
> +		line[40] = '\0';
> +		string_list_append(&names, line);
> +		counter++;
> +	}
> +	if (!counter)
> +		printf("Nothing new to pack.\n");
This was 'say Nothing new to pack.'. say obeys --quiet, IIRC.
Show 13 quoted lines
> +	fclose(out);
> +
> +	int failed = 0;
> +	for_each_string_list_item(item, &names) {
> +		for (ext = 0; ext < 1; ext++) {
> +			char *fname, *fname_old;
> +			fname = mkpathdup("%s/%s%s", packdir, item->string, exts[ext]);
> +			if (!file_exists(fname)) {
> +				free(fname);
> +				continue;
> +			}
> +
> +			fname_old = mkpathdup("%s/old-%s%s", packdir, item->string, exts[ext]);

If you could use git_path() instead of mkpathdup() in these two cases, we would not need to free() the names.

Show 9 quoted lines
> +			if (file_exists(fname_old))
> +				unlink(fname_old);
> +
> +			if (rename(fname, fname_old)) {
> +				failed = 1;
> +				break;
> +			}
> +			free(fname_old);
> +			string_list_append_nodup(&rollback, fname);
Ah, we would need to allocate here then.
> +		}
> +		if (failed)
> +			/* set to last element to break for_each loop */
> +			item = names.items + names.nr;
A mere
			break;
doesn't do it here?
Show 7 quoted lines
> +	}
> +	if (failed) {
> +		struct string_list rollback_failure;
> +		for_each_string_list_item(item, &rollback) {
> +			char *fname, *fname_old;
> +			fname = mkpathdup("%s/%s", packdir, item->string);
> +			fname_old = mkpathdup("%s/old-%s", packdir, item->string);

I think it's possible to attach arbitrary data to each string_list item. We could attach the "%s/old-%s" name to the item name, then we wouldn't need to re-construct the names here.

Show 30 quoted lines
> +			if (rename(fname_old, fname))
> +				string_list_append(&rollback_failure, fname);
> +			free(fname);
> +			free(fname_old);
> +		}
> +
> +		if (rollback.nr) {
> +			int i;
> +			fprintf(stderr,
> +				"WARNING: Some packs in use have been renamed by\n"
> +				"WARNING: prefixing old- to their name, in order to\n"
> +				"WARNING: replace them with the new version of the\n"
> +				"WARNING: file.  But the operation failed, and the\n"
> +				"WARNING: attempt to rename them back to their\n"
> +				"WARNING: original names also failed.\n"
> +				"WARNING: Please rename them in $PACKDIR manually:\n");
> +			for (i = 0; i < rollback.nr; i++)
> +				fprintf(stderr, "WARNING:   old-%s -> %s\n",
> +					rollback.items[i].string,
> +					rollback.items[i].string);
> +		}
> +		exit(1);
> +	}
> +
> +	/* Now the ones with the same name are out of the way... */
> +	for_each_string_list_item(item, &names) {
> +		for (ext = 0; ext < 2; ext++) {
> +			char *fname, *fname_old;
> +			fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
> +			fname_old = mkpathdup("%s-%s%s", packtmp, item->string, exts[ext]);
Same here: git_path()?
> +			stat(fname_old, &statbuffer);

We ignore errors during chmod in the shell script. But this doesn't give you license to ignore stat() errors completely: If stat() fails, then don't chmod() below, either.

> +			statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
			statbuffer.st_mode &= ~(S_IWUSR|S_IWGRP|S_IWOTH);
> +			chmod(fname_old, statbuffer.st_mode);
> +			if (rename(fname_old, fname))
> +				die("Could not rename packfile: %s -> %s", fname_old, fname);
Use die_errno() here.
Show 11 quoted lines
> +			free(fname);
> +			free(fname_old);
> +		}
> +	}
> +
> +	/* Remove the "old-" files */
> +	for_each_string_list_item(item, &names) {
> +		char *fname;
> +		fname = mkpathdup("%s/old-pack-%s.idx", packdir, item->string);
> +		if (remove_path(fname))
> +			die("Could not remove file: %s", fname);
die_errno() makes sense here, too.
Show 5 quoted lines
> +		free(fname);
> +
> +		fname = mkpathdup("%s/old-pack-%s.pack", packdir, item->string);
> +		if (remove_path(fname))
> +			die("Could not remove file: %s", fname);
and here as well.
> +		free(fname);
Again git_path?
> +	}
> +
> +	/* End of pack replacement. */
Nit: A blank line should follow this comment.
Show 11 quoted lines
> +	if (delete_redundant) {
> +		sort_string_list(&names);
> +		for_each_string_list_item(item, &existing_packs) {
> +			char *sha1;
> +			size_t len = strlen(item->string);
> +			if (len < 40)
> +				continue;
> +			sha1 = item->string + len - 40;
> +			if (!string_list_has_string(&names, sha1))
> +				remove_pack(packdir, item->string);
> +		}
OK.
Show 7 quoted lines
> +		argv_array_clear(&cmd_args);
> +		argv_array_push(&cmd_args, "prune-packed");
> +		if (quiet)
> +			argv_array_push(&cmd_args, "--quiet");
> +
> +		memset(&cmd, 0, sizeof(cmd));
> +		cmd.argv = argv_array_detach(&cmd_args, NULL);
Again: is it necessary to detach?
Show 10 quoted lines
> +		cmd.git_cmd = 1;
> +		run_command(&cmd);
> +	}
> +
> +	if (!no_update_server_info) {
> +		argv_array_clear(&cmd_args);
> +		argv_array_push(&cmd_args, "update-server-info");
> +
> +		memset(&cmd, 0, sizeof(cmd));
> +		cmd.argv = argv_array_detach(&cmd_args, NULL);
Same here?
Show 5 quoted lines
> +		cmd.git_cmd = 1;
> +		run_command(&cmd);
> +	}
> +	return 0;
> +}

In my opinion, it is good that you keep a large function that resembles the structure of the shell script because it is easier to review. But ultimately, it should be factored into smaller functions.

-- Hannes
Previous: Stefan BellerNext: Stefan Beller
Message 24 of 72 in “Rewriting git-repack in C”
  1. Rewriting git-repack in CStefan Beller, Aug 13, 2013
  2. repack: rewrite the shell script in C.Stefan Beller, Aug 13, 2013
  3. Matthieu MoyAug 14, 2013
  4. Stefan BellerAug 14, 2013
  5. repack: rewrite the shell script in C.Stefan Beller, Aug 14, 2013
  6. Antoine PelisseAug 14, 2013
  7. Stefan BellerAug 14, 2013
  8. Jeff KingAug 14, 2013
  9. Martin FickAug 14, 2013
  10. Stefan BellerAug 14, 2013
  11. Martin FickAug 14, 2013
  12. Junio C HamanoAug 14, 2013
  13. Martin FickAug 14, 2013
  14. Junio C HamanoAug 15, 2013
  15. [RFC PATCHv2] repack: rewrite the shell script in C.Stefan Beller, Aug 16, 2013
  16. René ScharfeAug 17, 2013
  17. Kyle J. McKayAug 17, 2013
  18. Stefan BellerAug 18, 2013
  19. [RFC PATCHv3] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  20. Kyle J. McKayAug 18, 2013
  21. René ScharfeAug 18, 2013
  22. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  23. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 19, 2013
  24. Johannes SixtAug 20, 2013
  25. Stefan BellerAug 20, 2013
  26. Johannes SixtAug 20, 2013
  27. René ScharfeAug 20, 2013
  28. Stefan BellerAug 20, 2013
  29. repack: rewrite the shell script in C.Stefan Beller, Aug 20, 2013
  30. Jonathan NiederAug 21, 2013
  31. Stefan BellerAug 21, 2013
  32. Stefan BellerAug 21, 2013
  33. 1/2 repack: rewrite the shell script in CStefan Beller, Aug 21, 2013
  34. 2/2 repack: retain the return value of pack-objectsStefan Beller, Aug 21, 2013
  35. Junio C HamanoAug 21, 2013
  36. Matthieu MoyAug 21, 2013
  37. Stefan BellerAug 21, 2013
  38. Junio C HamanoAug 21, 2013
  39. Stefan BellerAug 21, 2013
  40. Johannes SixtAug 22, 2013
  41. Jonathan NiederAug 22, 2013
  42. Matthieu MoyAug 21, 2013
  43. Stefan BellerAug 21, 2013
  44. Matthieu MoyAug 21, 2013
  45. Stefan BellerAug 21, 2013
  46. Matthieu MoyAug 21, 2013
  47. Johannes SixtAug 22, 2013
  48. Johannes SixtAug 22, 2013
  49. repack: rewrite the shell script in C (squashing proposal)Stefan Beller, Aug 22, 2013
  50. Junio C HamanoAug 22, 2013
  51. Jonathan NiederAug 20, 2013
  52. Johannes SixtAug 21, 2013
  53. Stefan BellerAug 20, 2013
  54. Jonathan NiederAug 20, 2013
  55. Dokumenting api-paths.txtStefan Beller, Aug 20, 2013
  56. Jonathan NiederAug 20, 2013
  57. Stefan BellerAug 21, 2013
  58. Junio C HamanoAug 22, 2013
  59. Junio C HamanoAug 14, 2013
  60. Matthieu MoyAug 14, 2013
  61. Stefan BellerAug 15, 2013
  62. Duy NguyenAug 15, 2013
  63. Junio C HamanoAug 14, 2013
  64. Matthieu MoyAug 14, 2013
  65. Martin FickAug 14, 2013
  66. Martin FickAug 15, 2013
  67. Stefan BellerAug 15, 2013
  68. Martin FickAug 15, 2013
  69. Duy NguyenAug 15, 2013
  70. Junio C HamanoAug 14, 2013
  71. Stefan BellerAug 15, 2013
  72. Matthieu MoyAug 14, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.