Re: [PATCH] repack: rewrite the shell script in C (squashing proposal)

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <stefanbeller@googlemail.com> writes:

> @@ -41,18 +35,16 @@ static void remove_temporary_files(void)
>  	DIR *dir;
>  	struct dirent *e;
>  
> +	dir = opendir(packdir);
> +	if (!dir)
>  		return;
>  
> +	strbuf_addstr(&buf, packdir);
> +
> +	/* dirlen holds the length of the path before the file name */
>  	dirlen = buf.len + 1;
> +	strbuf_addf(&buf, "%s", packtmp);
> +	/* prefixlen holds the length of the prefix */

Thanks to the name of the variable that is self-describing, this
comment does not add much value.

But it misses the whole point of my suggestion in the earlier
message to phrase these like so:

        /* Point at the slash at the end of ".../objects/pack/" */
	dirlen = strlen(packdir) + 1;
        /* Point at the dash at the end of ".../.tmp-%d-pack-" */
        prefixlen = buf.len - dirlen;

to clarify what the writer considers as "the prefix" is, which may
be quite different from what the readers think "the prefix" is.  In
".tmp-2342-pack-0d8beaa5b76e824c9869f0d1f1b19ec7acf4982f.pack", is
the prefix ".tmp-2342-", ".tmp-2342-pack", or ".tmp-2342-pack-"?

>  int cmd_repack(int argc, const char **argv, const char *prefix)
>  {
> ...
>  	packdir = mkpathdup("%s/pack", get_object_directory());
>  	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
>  
> +	sigchain_push_common(remove_pack_on_signal);
> +
>  	argv_array_push(&cmd_args, "pack-objects");
>  	argv_array_push(&cmd_args, "--keep-true-parents");
>  	argv_array_push(&cmd_args, "--honor-pack-keep");
> ...
> +					rollback_failure.items[i].string,
> +					rollback_failure.items[i].string);
>  		}
>  		exit(1);
>  	}

The scripted version uses

    trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15

so remove_temporary_files() needs to be called before exiting from
the program without getting killed by a signal.

Thanks.