Re: [PATCH] repack.c: rename and unlink pack file if it exists

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

Junio C Hamano <gitster@pobox.com> writes:

> This comment in builtin/repack.c:
> ...

Oops; there was supposed to be these lines before anythning else:

	From: Torsten Bögershausen <tboegi@web.de>
	Date: Sun Feb 2 16:09:56 2014 +0100

>     First see if there are packs of the same name and if so
>     if we can move them out of the way (this can happen if we
>     repacked immediately after packing fully).
>
> When a repo was fully repacked, and is repacked again, we may run
> into the situation that "new" packfiles have the same name as
> already existing ones (traditionally packfiles have been named after
> the list of names of objects in them, so repacking all the objects
> in a single pack would have produced a packfile with the same name).
>
> The logic is to rename the existing ones into filename like
> "old-XXX", create the new ones and then remove the "old-" ones.
> When something went wrong in the middle, this sequence is rolled
> back by renaming the "old-" files back.
>
> The renaming into "old-" did not work as designed, because
> file_exists() was done on the wrong file name.  This could give
> problems for a repo on a network share under Windows, as reported by
> Jochen Haag <zwanzig12@googlemail.com>.
>
> Formulate the filenames objects/pack/pack-XXXX.{pack,idx} correctly
> (the code originally lacked "pack-" prefix when checking if the file
> exists).
>
> Also rename the variables to match what they are used for:
> fname for the final name of the new packfile, fname_old for the name
> of the existing one, and fname_tmp for the temporary name pack-objects
> created the new packfile in.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Somehow this came to my private mailbox without Cc to list, so I
>    am forwarding it.
>
>    I think with 1190a1ac (pack-objects: name pack files after
>    trailer hash, 2013-12-05), repacking the same set of objects may
>    have less chance of producing colliding names, especially if you
>    are on a box with more than one core, but it still would be a
>    good idea to get this part right in the upcoming release.
>
>    The bug in the first hunk will cause us not to find any existing
>    file, even when there is a name clash.  And then we try to
>    immediately the final rename without any provision for rolling
>    back.  The other two hunks are pure noise renaming variables.
>
>    I think the patch looks correct, but I'd appreciate a second set
>    of eyeballs.
>
>    Thanks.
>
>  builtin/repack.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index bca7710..3b01353 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -260,7 +260,7 @@ 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;
> -			fname = mkpathdup("%s/%s%s", packdir,
> +			fname = mkpathdup("%s/pack-%s%s", packdir,
>  						item->string, exts[ext]);
>  			if (!file_exists(fname)) {
>  				free(fname);
> @@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	/* 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;
> +			char *fname, *fname_tmp;
>  			struct stat statbuffer;
>  			fname = mkpathdup("%s/pack-%s%s",
>  					packdir, item->string, exts[ext]);
> -			fname_old = mkpathdup("%s-%s%s",
> +			fname_tmp = mkpathdup("%s-%s%s",
>  					packtmp, item->string, exts[ext]);
> -			if (!stat(fname_old, &statbuffer)) {
> +			if (!stat(fname_tmp, &statbuffer)) {
>  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> -				chmod(fname_old, statbuffer.st_mode);
> +				chmod(fname_tmp, statbuffer.st_mode);
>  			}
> -			if (rename(fname_old, fname))
> -				die_errno(_("renaming '%s' failed"), fname_old);
> +			if (rename(fname_tmp, fname))
> +				die_errno(_("renaming '%s' into '%s' failed"), fname_tmp, fname);
>  			free(fname);
> -			free(fname_old);
> +			free(fname_tmp);
>  		}
>  	}
>  
>  	/* Remove the "old-" files */
>  	for_each_string_list_item(item, &names) {
>  		for (ext = 0; ext < 2; ext++) {
> -			char *fname;
> -			fname = mkpath("%s/old-pack-%s%s",
> +			char *fname_old;
> +			fname_old = mkpath("%s/old-%s%s",
>  					packdir,
>  					item->string,
>  					exts[ext]);
> -			if (remove_path(fname))
> -				warning(_("removing '%s' failed"), fname);
> +			if (remove_path(fname_old))
> +				warning(_("removing '%s' failed"), fname_old);
>  		}
>  	}