Re: [msysGit] Consecutive git gc fails on Windows network share

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



(No top-posting, please see my comments below)
On 2014-01-20 15.02, Jochen wrote:
>On 01/16/2014 10:55 AM, Jochen Haag wrote:
> Hello,
>
> we want to report the following issue, because it seems to be not an
> intended behaviour. Maybe someone can have a look at it at some point.
> It is not urgent for us.
>
> After upgrading from Git version 1.8.1.msysgit.1 to 1.8.5.2.msysgit.0 we
> observed that a consecutive git gc fails, in case the Git repo is
> located on a Windows network share. Operating system used is Windows 7
> 64 bit SP1.
>
> In case git gc fails temporary packs and index remain in .git/objects/pack/.
>
> "Consecutive" means, that git gc is called on an already packed
> repository (e.g. git gc is called more than once).
>
> This is the output given in case of error:
>
> U:\GitEnv>git gc
> Counting objects: 139, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (71/71), done.
> Writing objects: 100% (139/139), done.
> Total 139 (delta 65), reused 139 (delta 65)
> fatal: renaming
> '.git/objects/pack/.tmp-7920-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.pack'
> failed: Permission denied
> error: failed to run repack
>
> And here the output with GIT_TRACE = 1:
>
> U:\GitEnv>git gc
> trace: built-in: git 'gc'
> trace: run_command: 'pack-refs' '--all' '--prune'
> trace: built-in: git 'pack-refs' '--all' '--prune'
> trace: run_command: 'reflog' 'expire' '--all'
> trace: built-in: git 'reflog' 'expire' '--all'
> trace: run_command: 'repack' '-d' '-l' '-A'
> '--unpack-unreachable=2.weeks.ago'
> trace: built-in: git 'repack' '-d' '-l' '-A'
> '--unpack-unreachable=2.weeks.ago'
> trace: run_command: 'pack-objects' '--keep-true-parents'
> '--honor-pack-keep' '--non-empty' '--all' '--reflog'
> '--unpack-unreachable=2.weeks.ago' '--local' '--delta-base-offset'
> '.git/objects/pack/.tmp-7612-pack'
> trace: built-in: git 'pack-objects' '--keep-true-parents'
> '--honor-pack-keep' '--non-empty' '--all' '--reflog'
> '--unpack-unreachable=2.weeks.ago' '--local' '--delta-base-offset'
> '.git/objects/pack/.tmp-7612-pack'
> Counting objects: 139, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (71/71), done.
> Writing objects: 100% (139/139), done.
> Total 139 (delta 65), reused 139 (delta 65)
> fatal: renaming
> '.git/objects/pack/.tmp-7612-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.pack'
> failed: Permission denied
> error: failed to run repack
>
> After running git gc twice, this is what is left in .git/objects/pack/:
>
> U:\GitEnv\.git\objects\pack>ls -al
> total 53676
> drwxr-xr-x    1 hgj2fe   Administ        0 Jan 16 10:43 .
> drwxr-xr-x    1 hgj2fe   Administ        0 Jan 14 12:52 ..
> -r--r--r--    1 hgj2fe   Administ     4964 Jan 16 10:43
> .tmp-7612-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.idx
> -r--r--r--    1 hgj2fe   Administ 18315618 Jan 16 10:43
> .tmp-7612-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.pack
> -r--r--r--    1 hgj2fe   Administ     4964 Jan 16 10:40
> .tmp-7920-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.idx
> -r--r--r--    1 hgj2fe   Administ 18315618 Jan 16 10:40
> .tmp-7920-pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.pack
> -r--r--r--    1 hgj2fe   Administ     4964 Jan 14 12:52
> pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.idx
> -r--r--r--    1 hgj2fe   Administ 18315618 Jan 14 12:52
> pack-ad9d069e7c27ce19cc5b6cde82b4a38bedbf5884.pack
>
> In case we remove the read-only flag of the pack and index manually
> before running git gc again, no problem occurs:
>
> U:\GitEnv\.git\objects\pack>git gc
> trace: built-in: git 'gc'
> trace: run_command: 'pack-refs' '--all' '--prune'
> trace: built-in: git 'pack-refs' '--all' '--prune'
> trace: run_command: 'reflog' 'expire' '--all'
> trace: built-in: git 'reflog' 'expire' '--all'
> trace: run_command: 'repack' '-d' '-l' '-A'
> '--unpack-unreachable=2.weeks.ago'
> trace: built-in: git 'repack' '-d' '-l' '-A'
> '--unpack-unreachable=2.weeks.ago'
> trace: run_command: 'pack-objects' '--keep-true-parents'
> '--honor-pack-keep' '--non-empty' '--all' '--reflog'
> '--unpack-unreachable=2.weeks.ago' '--local' '--delta-base-offset'
> 'U:/GitEnv/.git/objects/pack/.tmp-3736-pack'
> trace: built-in: git 'pack-objects' '--keep-true-parents'
> '--honor-pack-keep' '--non-empty' '--all' '--reflog'
> '--unpack-unreachable=2.weeks.ago' '--local' '--delta-base-offset'
> 'U:/GitEnv/.git/objects/pack/.tmp-3736-pack'
> Counting objects: 139, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (71/71), done.
> Writing objects: 100% (139/139), done.
> Total 139 (delta 65), reused 139 (delta 65)
> trace: run_command: 'prune-packed'
> trace: built-in: git 'prune-packed'
> trace: run_command: 'update-server-info'
> trace: built-in: git 'update-server-info'
> trace: run_command: 'prune' '--expire' '2.weeks.ago'
> trace: built-in: git 'prune' '--expire' '2.weeks.ago'
> trace: run_command: 'rerere' 'gc'
> trace: built-in: git 'rerere' 'gc'
>
> The problem might be related to this commit:
> https://github.com/msysgit/git/commit/a1bbc6c0176f1fa2d4aa571cc0183a1f0ff9b285
>
>
> Best regards,
>
> Jochen
------
> 
> 
> Am Freitag, 17. Januar 2014 19:02:07 UTC+1 schrieb Torsten Bögershausen:
> 
> 
>     So, please, what happens if you revert that commit?
>     It could be good if you can test it and share the results with the list
>     /Torsten
> 
> 
> Instead of reverting we did some more analysis.
> 
> In repack.c we found the following code:
> 
>         /* 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;
>                         struct stat statbuffer;
>                         fname = mkpathdup("%s/pack-%s%s",
>                                         packdir, item->string, exts[ext]);
>                         fname_old = mkpathdup("%s-%s%s",
>                                         packtmp, item->string, exts[ext]);
>                         if (!stat(fname_old, &statbuffer)) {
>                                 statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>                                 chmod(fname_old, statbuffer.st_mode);
>                         }
>                         if (rename(fname_old, fname))
>                                 die_errno(_("renaming '%s' failed"), fname_old);
>                         free(fname);
>                         free(fname_old);
>                 }
>         }
> 
> The rename command replaces a mv -f command of the original shell script. Obviously the -f option can also override a read-only file but rename can not on a network share.
> 
> We changed the code as followed:
> 
>         /* 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;
>                         struct stat statbuffer;
>                         fname = mkpathdup("%s/pack-%s%s",
>                                         packdir, item->string, exts[ext]);
>                         fname_old = mkpathdup("%s-%s%s",
>                                         packtmp, item->string, exts[ext]);
>                         if (!stat(fname_old, &statbuffer)) {
>                                 statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>                                 chmod(fname_old, statbuffer.st_mode);
>                         }
>+++                         if (!stat(fname, &statbuffer)) {
>+++                                 statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
>+++                                 chmod(fname, statbuffer.st_mode);
>+++                         }
>                         if (rename(fname_old, fname))
>                                 die_errno(_("renaming '%s' failed"), fname_old);
>                         free(fname);
>                         free(fname_old);
>                 }
>         }
> 
> Before rename is called the destination file is made writeable. This worked for us. We are not sure if this is a good implementation.

Jochen,
thanks for sharing the code changes with us.

I allowed me to 
a) reconstruct the original mail,
b) add "+++" at the places where you added the stat() and chmod(),
c) and to send the question "is this a good implementation ?" to upstream git.

I think your implementation makes sense.
/Torsten
 

--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]