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

Hi,

Stefan Beller wrote:

Thanks for your work so far. This review will have mostly cosmetic notes. Hopefully others can try it out to see if the actual behavior is good.

As a first nit: in git, as usual in emails, the style in subject lines is not to end with a period. The above subject line is otherwise good (a nice summary that quickly explains the effect, which is handy in e.g. abbreviated changelogs from release announcements).

This is a place to explain

 - the motivation / intended positive effect of the patch
 - any noticeable behavior changes
 - complications and other hints for people looking back and trying
   to understand this code

Based on the discussion before, I think the motivation is to get closer to a goal of being able to have a core subset of git functionality built in to git. That would mean

 * people on Windows could get a copy of at least the core parts
   of Git without having to install a Unix-style shell

 * people deploying to servers don't have to rewrite the #! line
   or worry about the PATH and quality of installed POSIX
   utilities, if they are only using the built-in part written
   in C 

This patch is meant to be mostly a literal translation of the git-repack script; the intent is that later patches would start using more library facilities, but this patch is meant to be as close to a no-op as possible so it doesn't do that kind of thing.

This kind of changes-since-the-previous-iteration information that doesn't need to be recorded in the commit log for posterity goes after the "---" marker.

[...]

[...]

The "= 0" is automatic for statics without an initializer. The prevailing style in git is to leave it out.

Behavior change: in the script, wasn't the default "true"?

[...]

Style: argument list should have "void".  (In C89 and C99, an empty
argument list means "having unspecified arguments" instead of "having
no arguments" as in C++.)

pid_t is not guaranteed to be an "int", so this needs a cast.

The names "prefix" and "path" are quite generic. What does this function do? A comment could help, e.g.:

	/*
	 * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
	 */

What happens if the directory does not exist?

The git-repack script removes $PACKTMP-*, but this code matches $PACKTMP* instead. Intentional?

This leaks fname (see Documentation/technical/api-strbuf.txt for an explanation of strbuf_detach).

I wonder if it would make sense for buffers to share space here. E.g. something like

 {
	/*
	 * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
	 */

	struct strbuf buf = STRBUF_INIT;
	size_t dirlen, prefixlen;
	DIR *dir;
	struct dirent *e;

	/* .git/objects/pack */
	strbuf_addstr(&buf, get_object_directory());
	strbuf_addstr(&buf, "/pack");
	dir = opendir(buf.buf);
	if (!dir)
		... handle error ...

	/* .git/objects/pack/.tmp-$$-pack-* */
	dirlen = buf.len + 1;
	strbuf_addf(&buf, "/.tmp-%d-pack-", getpid());
	prefixlen = buf.len - dirlen;

	while ((e = readdir(dir))) {
		if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
			continue;
		strbuf_setlen(&buf, dirlen);
		strbuf_addstr(&buf, e->d_name);
		unlink(buf.buf);
	}
	if (closedir(dir))
		... handle error ...

	strbuf_release(&buf);
 }

I dunno.

[...]

Ideally a comment opening a function will save lazy readers the trouble of reading the body of the function, by explaining what the function is for and giving them some reliable summary of what its effect will be.

The above comment doesn't do either: it doesn't make it clear why the function exists, and it doesn't make the semantics precise: should fname_list be empty before this function is called? Are the filenames filling it absolute or relative? What happens if packdir is unreadable or doesn't exist? What happens to files without a .pack extension?

Also, the above comment is in the wrong part of the file to maximally help a lazy reader: it should be at the call site, so the reader doesn't have to look for the function's definition at all.

File-local function should be static.

Missing "const" on packdir.

Whenever I see the result of "mkpath" stored for more than a couple of lines, I fret a little (since it's easy to scribble over the rotating list of 4 get_pathname() buffers). Would doing

	dir = opendir(mkpath(...))

directly work? By the way, why does this function both compute packdir and take it as an argument?

Why not pass the string directly to suffixcmp and strlen?

What happens if the packdir does not exist or cannot be read, or another error occurs?

Can decrease the indent and deal with the boring case early by reversing the test:

		if (suffixcmp(...))
			continue;

[...]

Missing "const" on path. The * in pointers sticks to the variable name instead of its type.

String constants are allowed in C to be assigned to a char * for historical reasons, but it's never a good idea :), since they're not mutable.

Array index is being assigned twice. ARRAY_SIZE could make this clearer:

 {
	const char *exts[] = {".pack", ... };
	int i;

	for (i = 0; i < ARRAY_SIZE(exts); i++)
		unlink(mkpath("%s/pack-%s%s", packdir, sha1, exts[i]));
 }

Is the sha1 parameter actually a sha1?

It wasn't obvious to me at first what this function is for. Maybe a name like remove_redundant_pack() would work. E.g.:

	static void remove_redundant_pack(const char *pack_sha1)
	{
		const char *exts[] = {".pack", ".idx", ".keep"};
		struct strbuf buf = STRBUF_INIT;
		size_t plen;
		int i;

		strbuf_addf(&buf, "%s/pack-%s", get_object_directory(), pack_sha1);
		plen = buf.len;

		for (i = 0; i < ARRAY_SIZE(exts); i++) {
			strbuf_setlen(&buf, plen);
			strbuf_addstr(&buf, exts[i]);
			unlink(buf.buf);
		}
	}

[...]

Stray blank line.

Missing "const".

Fixed-size buffers are no fun. Better to use a strbuf with e.g. strbuf_getline (see Documentation/technical/api-strbuf.txt).

[...]

Do you mean
		... than this")),

		OPT_GROUP(N_("Packing constraints")),

?

[...]

This filename pattern also appears higher in the file. Maybe it's possible for them to share a constant or something? (If it's too much fuss, no need to bother.)

Style: I'd leave out these blank lines, so the reader can see more
of the arguments in one screenful.

Are these signed or unsigned integers? What happens if I pass a value < 0 or > INT_MAX?

[...]

Probably easier to read as

	if (!pack_everything && !pack_everything_but_loose) {

The git-repack script uses "find", which scans the directory recursively, though I'm not sure why. (Probably not important?)

Instead of building a temporary and potentially long string_list and then pruning it to build another list, why not build the list of packs without a corresponding .keep file in a single pass?

Simplifying, and avoiding braces around a single-line "if" body:

			if (!file_exists(mkpath(...)))
				string_list_append(...);

[...]

Long line.

[...]

Time to sleep.  Stopping here for this round.

Hope that helps, Jonathan