Re: Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()

Re: Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()

[Torsten Bögershausen]

(It seems as if the mail went only to Junio, sorry)
On 2014-02-02 16.09, Torsten Bögershausen wrote:
> On 2014-01-29 19.17, Junio C Hamano wrote:
>> But after a closer inspection, I no longer think that hunk is an
>> improvement.  These new packfiles were created by pack-objects,
>> which finishes each packfile it produces by calling
>> finish_tmp_packfile(), and that is where adjust_shared_perm()
>> happens already.  As far as "pack-objects" that was called from
>> "repack" is concerned, these new packfiles are not "temporary"; they
>> are finished product.  It may be OK to remove them as part of
>> "rewind back to the original state, as a later phase of repack
>> failed" if we saw a failure (but note that the original
>> "git-repack.sh" didn't), but a plain vanilla rename(2) without any
>> frills is what we want to happen to them.
> Thanks for deeper inspection, I now suspect the root cause to be here:
>
> -- >8 --
> Subject: [PATCH v3] repack.c: Rename and unlink pack file if it exists
>
> This comment in builtin/repack.c:
>   * 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.
> indicates that when a repo was fully repacked, and is repacked again,
> we may run into the situation that "new" packfiles have the name
> (and content) as already existing ones.
>
> The logic is to rename the existing ones into filename like "old-XXX",
> create the new ones and remove the "old-" ones.
> When something went wrong, a manual roll-back could be done be renaming
> the "old-" files.
>
> 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>
>
> Solution:
> Create the right file name, like this:
>   fname = mkpathdup("%s/pack-%s%s", packdir,
> and when removing the old-files:
>   fname_old = mkpath("%s/old-%s%s",
>
> Rename the variables to match what they are used for:
> fname, fname_old and fname_tmp
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/repack.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 6284846..de69401 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -258,7 +258,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);
> @@ -314,33 +314,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);
>  		}
>  	}
>  

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.