threads / discuss / 26393

Q: does index-pack work in place on windows?

Subject: Q: does index-pack work in place on windows?

## tl;dr

7 messages between Feb 2, 2011 and Feb 4, 2011.

replies: 6people: 3as markdown or json

Junio C Hamano· Feb 2, 2011, 20:21 UTC · lore

... or any other (operating / file) system where you cannot unlink a file that is open?

When you run
    $ git clone git://some.where/repo/sitory.git local
    $ cd local
    $ git index-pack .git/objects/pack/pack-*.pack

there is a call to write_idx_file() in builtin/index-pack.c, that feeds the correct (and existing) name of the corresponding pack idx file.

The callee in pack-write.c, after sorting the list of objects contained in the pack, does this:

	if (!index_name) {
		static char tmpfile[PATH_MAX];
		fd = odb_mkstemp(tmpfile, sizeof(tmpfile), "pack/tmp_idx_XXXXXX");
		index_name = xstrdup(tmpfile);
	} else {
		unlink(index_name);
		fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
	}
 
and then writes out the pack index to the file descriptor.  But index-pack
uses the usual has_sha1_file() and read_sha1_file() interface to validate
the "existing" objects, and is likely to have mmapped the .idx file when
it called use_pack_window().  Which makes me suspect that this unlink (or
the open that immediately follows) may fail on systems that do not allow
unlink on inode that has still users.
Erik Faye-Lund· Feb 2, 2011, 20:30 UTC · re: Junio C Hamano · lore

Re: Q: does index-pack work in place on windows?

On Wed, Feb 2, 2011 at 9:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> ... or any other (operating / file) system where you cannot unlink a file
> that is open?
>
> When you run
>
>    $ git clone git://some.where/repo/sitory.git local
>    $ cd local
>    $ git index-pack .git/objects/pack/pack-*.pack
>
> there is a call to write_idx_file() in builtin/index-pack.c, that feeds
> the correct (and existing) name of the corresponding pack idx file.
>
> The callee in pack-write.c, after sorting the list of objects contained in
> the pack, does this:
>
>        if (!index_name) {
>                static char tmpfile[PATH_MAX];
>                fd = odb_mkstemp(tmpfile, sizeof(tmpfile), "pack/tmp_idx_XXXXXX");
>                index_name = xstrdup(tmpfile);
>        } else {
>                unlink(index_name);
>                fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>        }
>
> and then writes out the pack index to the file descriptor.  But index-pack
> uses the usual has_sha1_file() and read_sha1_file() interface to validate
> the "existing" objects, and is likely to have mmapped the .idx file when
> it called use_pack_window().  Which makes me suspect that this unlink (or
> the open that immediately follows) may fail on systems that do not allow
> unlink on inode that has still users.
...and you're right:

$ git index-pack .git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.pack Unlink of file '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx' failed. Should I try again? (y/n)? n fatal: unable to create '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx': File exists

Nicolas Pitre· Feb 3, 2011, 05:32 UTC · re: Erik Faye-Lund · lore

Re: Q: does index-pack work in place on windows?

On Wed, 2 Feb 2011, Erik Faye-Lund wrote:
Show 41 quoted lines
> On Wed, Feb 2, 2011 at 9:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > ... or any other (operating / file) system where you cannot unlink a file
> > that is open?
> >
> > When you run
> >
> >    $ git clone git://some.where/repo/sitory.git local
> >    $ cd local
> >    $ git index-pack .git/objects/pack/pack-*.pack
> >
> > there is a call to write_idx_file() in builtin/index-pack.c, that feeds
> > the correct (and existing) name of the corresponding pack idx file.
> >
> > The callee in pack-write.c, after sorting the list of objects contained in
> > the pack, does this:
> >
> >        if (!index_name) {
> >                static char tmpfile[PATH_MAX];
> >                fd = odb_mkstemp(tmpfile, sizeof(tmpfile), "pack/tmp_idx_XXXXXX");
> >                index_name = xstrdup(tmpfile);
> >        } else {
> >                unlink(index_name);
> >                fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >        }
> >
> > and then writes out the pack index to the file descriptor.  But index-pack
> > uses the usual has_sha1_file() and read_sha1_file() interface to validate
> > the "existing" objects, and is likely to have mmapped the .idx file when
> > it called use_pack_window().  Which makes me suspect that this unlink (or
> > the open that immediately follows) may fail on systems that do not allow
> > unlink on inode that has still users.
> 
> ...and you're right:
> 
> $ git index-pack
> .git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.pack
> Unlink of file '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx'
> failed. Should I try again? (y/n)? n
> fatal: unable to create
> '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx':
> File exists
Why would you do such thing in the first place?

If the pack exists along with its index on disk, there is no point recreating it. Maybe index-pack should just bail out early in that case.

Nicolas
Junio C Hamano· Feb 3, 2011, 07:24 UTC · re: Nicolas Pitre · lore

Re: Q: does index-pack work in place on windows?

Nicolas Pitre <nico@fluxnic.net> writes:
Show 13 quoted lines
>> $ git index-pack
>> .git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.pack
>> Unlink of file '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx'
>> failed. Should I try again? (y/n)? n
>> fatal: unable to create
>> '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx':
>> File exists
>
> Why would you do such thing in the first place?
>
> If the pack exists along with its index on disk, there is no point 
> recreating it.  Maybe index-pack should just bail out early in that 
> case.

I am trying to see if an index-pack with slight modification would be a good replacement for verify-pack.

Nicolas Pitre· Feb 4, 2011, 01:14 UTC · re: Junio C Hamano · lore

Re: Q: does index-pack work in place on windows?

On Wed, 2 Feb 2011, Junio C Hamano wrote:
Show 18 quoted lines
> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> $ git index-pack
> >> .git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.pack
> >> Unlink of file '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx'
> >> failed. Should I try again? (y/n)? n
> >> fatal: unable to create
> >> '.git/objects/pack/pack-d634271f4d7ca70c00148e967a343c3c46cd7705.idx':
> >> File exists
> >
> > Why would you do such thing in the first place?
> >
> > If the pack exists along with its index on disk, there is no point 
> > recreating it.  Maybe index-pack should just bail out early in that 
> > case.
> 
> I am trying to see if an index-pack with slight modification would be a
> good replacement for verify-pack.

OK, then you don't intend to reuse it as is. If you really wanted to have index-pack work in place then you would have to use free_pack_by_name() somewhere before writing the new pack index out, but that is still a dubious use case.

index-pack _could_ be a replacement for verify-pack. It certainly can validate a pack since it is its purpose, possibly faster than verify-pack. You'd still have to compare the existing pack index against the one index-pack creates without overwriting that original index, taking into accound index version differences, etc.

However index-pack won't tell you what is broken in the pack when corruptions are to be found. It will simply tell you at the very end that the checksum hash doesn't match, or that some deltas were not resolved, that kind of thing, whereas verify-pack has the ability to tell you exactly what doesn't match with the info in the index or the like as soon as it encounters a problem.

Nicolas
Junio C Hamano· Feb 4, 2011, 01:59 UTC · re: Nicolas Pitre · lore

Re: Q: does index-pack work in place on windows?

Nicolas Pitre <nico@fluxnic.net> writes:
Show 10 quoted lines
> On Wed, 2 Feb 2011, Junio C Hamano wrote:
>
>> I am trying to see if an index-pack with slight modification would be a
>> good replacement for verify-pack.
> ...
> index-pack _could_ be a replacement for verify-pack.  It certainly can 
> validate a pack since it is its purpose, possibly faster than 
> verify-pack.  You'd still have to compare the existing pack index 
> against the one index-pack creates without overwriting that original 
> index, taking into accound index version differences, etc.

We already know index-pack is a lot faster when you have a lot of deep deltas, as it works from a base to its immediate delta children while pinning that base, as opposed to verify-pack that verifies each and every object in the pack in the index order, inflating and then applying potentially long delta chains repeatedly---the only thing that could be helping it right now is the in-core delta base cache.

> However index-pack won't tell you what is broken in the pack when 
> corruptions are to be found.

Yes, but at that point, you are pretty much lost anyway, as the only thing you can do to salvage salvageable parts of the broken pack, if you still trust its associated .idx file, is to walk the table of contents and ask for each individual object; knowing where the .pack or .idx is broken with the current verify-pack does not help you very much.

Nicolas Pitre· Feb 4, 2011, 02:45 UTC · re: Junio C Hamano · lore

Re: Q: does index-pack work in place on windows?

On Thu, 3 Feb 2011, Junio C Hamano wrote:
Show 10 quoted lines
> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > However index-pack won't tell you what is broken in the pack when 
> > corruptions are to be found.
> 
> Yes, but at that point, you are pretty much lost anyway, as the only thing
> you can do to salvage salvageable parts of the broken pack, if you still
> trust its associated .idx file, is to walk the table of contents and ask
> for each individual object; knowing where the .pack or .idx is broken with
> the current verify-pack does not help you very much.

Well... given that you do have to compare both the generated index and the on-disk index at some point, it is then possible to identify corrupted objects and their location. And with pack index version 2 including a CRC for the packed data, you should be able to tell if the corruption is actually in the pack or in the index (or both if the final pack/index SHA1 sum doesn't match).

Nicolas

← back to recent threads