(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@xxxxxxxxxxxxxx>
>
> 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@xxxxxx>
> ---
> 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);
> }
> }
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html