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

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

[Junio C Hamano]

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

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

[Erik Faye-Lund]

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

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

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1991 bytes --]

On Wed, 2 Feb 2011, Erik Faye-Lund wrote:

> 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

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

[Junio C Hamano]

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.

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

[Nicolas Pitre]

On Wed, 2 Feb 2011, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

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

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

[Nicolas Pitre]

On Thu, 3 Feb 2011, Junio C Hamano wrote:

> 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