From: Johannes Sixt <j6t@kdbg.org>
Date: Wed, 21 Aug 2013 09:20:03 GMT
Subject: Re: [RFC PATCHv4] repack: rewrite the shell script in C.
Message-ID: <52148643.2050706@kdbg.org>
In-Reply-To: <5213EF74.7020408@googlemail.com>

Am 21.08.2013 00:36, schrieb Stefan Beller:
> I think I got all the suggestions except the
> use of git_path/mkpathdup.
> I replaced mkpathdup by mkpath where possible,
> but it's still not perfect.
> I'll wait for the dokumentation patch of Jonathan,
> before changing all these occurences forth and back
> again.

I trust Jonathan's judgement of how to use git_path, mkpath, and mkpathdup 
more than my own. So, please take my earlier comments in this regard with 
an appropriately large grain of salt.

> Below there is just the diff against RFC PATCHv4,
> however I'll send the whole patch as well.

Thanks, that is VERY helpful!

I'll comment here and have a look at the full patch later.

>...
>   int cmd_repack(int argc, const char **argv, const char *prefix) {
>

You should move the opening brace to the next line, which would then not 
be empty anymore.

>...
> @@ -217,34 +217,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
>   	argv_array_push(&cmd_args, packtmp);
>
>   	memset(&cmd, 0, sizeof(cmd));
> -	cmd.argv = argv_array_detach(&cmd_args, NULL);
> +	cmd.argv = cmd_args.argv;
>   	cmd.git_cmd = 1;
>   	cmd.out = -1;
>   	cmd.no_stdin = 1;
>
> -	if (run_command(&cmd))
> +	if (start_command(&cmd))
>   		return 1;

You should have an int ret here and use it like

	ret = start_command(&cmd);
	if (ret)
		return ret;

to retain any exit codes from the sub-process. I know, the script didn't 
preserve it:

	names=$(git pack-objects ...) || exit 1

but that was not idiomatic as it should have been written as

	names=$(git pack-objects ...) || exit

to forward the failure exit code.

>
> -	struct string_list names = STRING_LIST_INIT_DUP;
> -	struct string_list rollback = STRING_LIST_INIT_DUP;
> -
> -	char line[1024];
> -	int counter = 0;
> -	FILE *out = xfdopen(cmd.out, "r");

Nice! I missed these decl-after-stmt in my earlier review.

> +	count_packs = 0;
> +	out = xfdopen(cmd.out, "r");
>   	while (fgets(line, sizeof(line), out)) {
>   		/* a line consists of 40 hex chars + '\n' */
> -		assert(strlen(line) == 41);
> +		if (strlen(line) != 41)
> +			die("repack: Expecting 40 character sha1 lines only from pack-objects.");

I agree with Jonathan that you should use strbuf_getline() here.

>   		line[40] = '\0';
>   		string_list_append(&names, line);
> -		counter++;
> +		count_packs++;
>   	}
> -	if (!counter)
> -		printf("Nothing new to pack.\n");
> +	if (finish_command(&cmd))
> +		return 1;

Same as above here:

	ret = finish_command(&cmd);
	if (ret)
		return ret;

I would prefer to see

	argv_array_clear(&cmd_args);

here, i.e., at the end of the current use rather than later at the 
beginning of the next use. (Ditto for the other uses of cmd_args.)

>   	fclose(out);

This should happen before finish_command(). It doesn't matter if there are 
no errors, but if things go awry, closing the channel before 
finish_command() avoids deadlocks.

>
> +	if (!count_packs && !quiet)
> +		printf("Nothing new to pack.\n");
> +
>...
> @@ -301,33 +299,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
>   	for_each_string_list_item(item, &names) {
>   		for (ext = 0; ext < 2; ext++) {
>   			char *fname, *fname_old;
> +			struct stat statbuffer;
>   			fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
> -			fname_old = mkpathdup("%s-%s%s", packtmp, item->string, exts[ext]);
> -			stat(fname_old, &statbuffer);
> -			statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
> -			chmod(fname_old, statbuffer.st_mode);
> +			fname_old = mkpath("%s-%s%s", packtmp, item->string, exts[ext]);
> +			if (!stat(fname_old, &statbuffer)) {
> +				statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;

This is still wrong: it should be one of

			... &= ~S_IWUSR & ~S_IWGRP & ~S_IWOTH;
			... &= ~(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);
> +				die_errno(_("renaming '%s' failed"), fname_old);
>   			free(fname);
> -			free(fname_old);
>   		}
>   	}
>...

Everything else looks OK. But as I said, mkpath() may have to be reverted 
to mkpathdup() as per Jonathans comments.

-- Hannes