Junio C Hamano <gits...@pobox.com> writes:
> This comment in builtin/repack.c:
> ...
Oops; there was supposed to be these lines before anythning else:
From: Torsten Bögershausen <tbo...@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 <zwanzi...@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 <tbo...@web.de>
> Signed-off-by: Junio C Hamano <gits...@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);
> }
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
