Subject: Re: Consecutive git gc fails on Windows network share

From: Stefan Beller <[email-masked]>
Date: Mon, 20 Jan 2014 22:52:27 +0100

On 20.01.2014 18:16, Torsten Bögershausen wrote:
> (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
>  
> 

I'm sorry for breaking repack there. The implementation sounds
reasonably to me.

Thanks for reporting.
Do you want to prepare an upstream patch?

Thanks,
Stefan

-- 
-- 
*** 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 [email-masked]
To unsubscribe from this group, send email to
[email-masked]
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 [email-masked].
For more options, visit https://groups.google.com/groups/opt_out.