[BUG] t9801 and t9803 broken on next

[BUG] t9801 and t9803 broken on next

[Lars Schneider]

Hi,

t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 
"fast-import: implement unpack limit" might be the reason. (@gmane/292562).

Did anyone look into that already?

Thanks,
Lars

Re: [BUG] t9801 and t9803 broken on next

[Eric Wong]

Lars Schneider <larsxschneider@gmail.com> wrote:
> Hi,
> 
> t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 
> "fast-import: implement unpack limit" might be the reason. (@gmane/292562).
> 
> Did anyone look into that already?

Just took a look (no p4, so couldn't run tests) and I guess it's
because the default changed and it doesn't generate tiny packs.

The easiest change is probably to call:

	git config fastimport.unpackLimit 0

during setup like t9300 now does.

Re: [BUG] t9801 and t9803 broken on next

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> Lars Schneider <larsxschneider@gmail.com> wrote:
>> Hi,
>> 
>> t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 
>> "fast-import: implement unpack limit" might be the reason. (@gmane/292562).
>> 
>> Did anyone look into that already?
>
> Just took a look (no p4, so couldn't run tests) and I guess it's
> because the default changed and it doesn't generate tiny packs.

I looked at t9801 but I couldn't spot where it cares if the objects
are in a (tiny) pack or stored as individual loose objects.  All the
counting I saw was about rev-list/log output and they shouldn't care.

Are you saying that "git p4" itself breaks unless fast-import always
writes a new (tiny) packfile?  That sounds quite broken, and setting
unpacklimit to 0 does not sound like a sensible "fix".  Of course,
if the test script is somehow looking at the number of packs or
loose objects and declaring a failure, even when the resulting
history in p4 and git are correct, then that is a different issue,
and forcing to explode a tiny pack is a reasonable workaround.  I
couldn't quite tell which the case is.

Puzzled.  Please help.

> The easiest change is probably to call:
>
> 	git config fastimport.unpackLimit 0
>
> during setup like t9300 now does.

Re: [BUG] t9801 and t9803 broken on next

[Lars Schneider]

> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Eric Wong <e@80x24.org> writes:
> 
>> Lars Schneider <larsxschneider@gmail.com> wrote:
>>> Hi,
>>> 
>>> t9801 and t9803 seem to be broken on next. A quick bisect indicates that d9545c7 
>>> "fast-import: implement unpack limit" might be the reason. (@gmane/292562).
>>> 
>>> Did anyone look into that already?
>> 
>> Just took a look (no p4, so couldn't run tests) and I guess it's
>> because the default changed and it doesn't generate tiny packs.
> 
> I looked at t9801 but I couldn't spot where it cares if the objects
> are in a (tiny) pack or stored as individual loose objects.  All the
> counting I saw was about rev-list/log output and they shouldn't care.
> 
> Are you saying that "git p4" itself breaks unless fast-import always
> writes a new (tiny) packfile?  That sounds quite broken, and setting
> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
> if the test script is somehow looking at the number of packs or
> loose objects and declaring a failure, even when the resulting
> history in p4 and git are correct, then that is a different issue,
> and forcing to explode a tiny pack is a reasonable workaround.  I
> couldn't quite tell which the case is.
> 
> Puzzled.  Please help.

t9801 "import depot, branch detection" is the first test that fails
with a fast import error:
https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110

fast-import crash report:
    fast-import process: 77079
    parent process     : 77077
    at 2016-05-14 07:48:40 +0000

fatal: offset beyond end of packfile (truncated pack?)

Most Recent Commands Before Crash
---------------------------------
  commit refs/remotes/p4/master
  mark :1
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  M 100644 inline f1
  data 3
  
  commit refs/remotes/p4/master
  mark :2
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  M 100644 inline f2
  data 3
  
  commit refs/remotes/p4/master
  mark :4
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  M 100644 inline f4
  data 3
  
  checkpoint
  commit git-p4-tmp/6
  mark :6
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  M 100644 inline f1
  data 3
  M 100644 inline f2
  data 3
  M 100644 inline f4
  data 3
  
  checkpoint
  progress checkpoint
  commit refs/remotes/p4/depot/branch2
  mark :6
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  from 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345
* M 100644 inline f1

Active Branch LRU
-----------------
    active_branches = 2 cur, 5 max

  pos  clock name
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1)      4 git-p4-tmp/6
   2)      3 refs/remotes/p4/master

Inactive Branches
-----------------
git-p4-tmp/6:
  status      : active loaded
  tip commit  : 3aef1b3e7d821051780a89262f2e2fae33ed8af3
  old tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  cur tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  commit clock: 4
  last pack   : 0

refs/remotes/p4/master:
  status      : active loaded
  tip commit  : 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345
  old tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  cur tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  commit clock: 3
  last pack   : 0

refs/remotes/p4/depot/branch2:
  status      : loaded
  tip commit  : 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345
  old tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  cur tree    : 30a8495449245cdb9626ef48902ba672cf76576f
  commit clock: 0
  last pack   : 


Marks
-----
:1 47474c2e749d579c63b375231c018289aa00cd0f
:2 9c6b957c0b4d4991b8bbd17858aa2a271d2c76c3
:4 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345
:6 3aef1b3e7d821051780a89262f2e2fae33ed8af3

-------------------
END OF CRASH REPORT


> 
>> The easiest change is probably to call:
>> 
>> 	git config fastimport.unpackLimit 0
>> 
>> during setup like t9300 now does.
Unfortunately that seems not to help.

The problem seems to be related to the git-p4 branch import.
I don't have time this weekend to look further into it, but
I will try next week.

Cheers,
Lars

Re: [BUG] t9801 and t9803 broken on next

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Are you saying that "git p4" itself breaks unless fast-import always
>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>> if the test script is somehow looking at the number of packs or
>> loose objects and declaring a failure, even when the resulting
>> history in p4 and git are correct, then that is a different issue,
>> and forcing to explode a tiny pack is a reasonable workaround.  I
>> couldn't quite tell which the case is.
>> 
>> Puzzled.  Please help.
>
> t9801 "import depot, branch detection" is the first test that fails
> with a fast import error:
> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>
> fast-import crash report:
>     fast-import process: 77079
>     parent process     : 77077
>     at 2016-05-14 07:48:40 +0000
>
> fatal: offset beyond end of packfile (truncated pack?)

Hmm, does that suggest Eric's "explode them into loose instead of
keeping a small pack" insufficient?  It sounds like that somebody
wanted to read some data back from its packfile without knowing that
the updated code may make it available in a loose object form, which
would mean that somebody needs to be told about what is going on,
namely, these objects are not in a half-written pack but are found
as loose objects.

> The problem seems to be related to the git-p4 branch import.
> I don't have time this weekend to look further into it, but
> I will try next week.

Thanks.

Re: [BUG] t9801 and t9803 broken on next

[Lars Schneider]

> On 14 May 2016, at 20:15, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>>> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Are you saying that "git p4" itself breaks unless fast-import always
>>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>>> if the test script is somehow looking at the number of packs or
>>> loose objects and declaring a failure, even when the resulting
>>> history in p4 and git are correct, then that is a different issue,
>>> and forcing to explode a tiny pack is a reasonable workaround.  I
>>> couldn't quite tell which the case is.
>>> 
>>> Puzzled.  Please help.
>> 
>> t9801 "import depot, branch detection" is the first test that fails
>> with a fast import error:
>> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>> 
>> fast-import crash report:
>>    fast-import process: 77079
>>    parent process     : 77077
>>    at 2016-05-14 07:48:40 +0000
>> 
>> fatal: offset beyond end of packfile (truncated pack?)
> 
> Hmm, does that suggest Eric's "explode them into loose instead of
> keeping a small pack" insufficient?  It sounds like that somebody
> wanted to read some data back from its packfile without knowing that
> the updated code may make it available in a loose object form, which
> would mean that somebody needs to be told about what is going on,
> namely, these objects are not in a half-written pack but are found
> as loose objects.

I think that is pretty much the problem. Here is what is happening:

1.  git-p4 imports all changelists for the "main" branch

2.  git-p4 starts to import a second branch and creates a fast-import
    "progress checkpoint". This triggers:

    --> fast-import.c: cycle_packfile
    --> fast-import.c: end_packfile
    --> fast-import.c: loosen_small_pack

    At this point we have no packfile anymore.

3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
    to fast-import in order to create a new branch. This triggers:

    --> fast-import.c: parse_new_commit
    --> fast-import.c: load_branch
    --> fast-import.c: load_tree

    "load_tree" calls "find_object" and the result has a "pack_id" of 0.
    I think this indicates that the object is in the packfile. Shouldn't
    the "pack_id" be "MAX_PACK_ID" instead?

        myoe = find_object(sha1);
        if (myoe && myoe->pack_id != MAX_PACK_ID) {

    --> fast-import.c: gfi_unpack_entry

    In "gfi_unpack_entry" this condition evaluates to "true":

        struct packed_git *p = all_packs[oe->pack_id];
        if (p == pack_data && p->pack_size < (pack_size + 20)) 

    In the comment below Git thinks that the object is stored in the
    packfile. This seems wrong but the flow continues:

    --> sha1_filec: unpack_entry
    --> sha1_filec: unpack_object_header
    --> sha1_filec: use_pack

    At this point fast-import dies with "offset beyond end of packfile 
    (truncated pack?)".

I am no expert in "fast-import". Does anyone have a few hints how to
proceed?

Thanks,
Lars

Re: [BUG] t9801 and t9803 broken on next

[Eric Wong]

Lars Schneider <larsxschneider@gmail.com> wrote:
> I am no expert in "fast-import". Does anyone have a few hints how to
> proceed?

I don't know fast-import or the C internals of git well at all,
either.  But capturing the exact input to fast-import (using
tee) would be useful for non-p4 users to debug the problem
and would go a long way in reproducing a standalone test case.

I'm Python-illiterate , but hopefully this works
to capture the stdin fed to fast-import (totally untested):

--- a/git-p4.py
+++ b/git-p4.py
@@ -3366,7 +3366,8 @@ class P4Sync(Command, P4UserMap):
 
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
 
-        self.importProcess = subprocess.Popen(["git", "fast-import"],
+        cmd = [ "sh", "-c", "tee /tmp/gfi-in.$$ | git fast-import" ]
+        self.importProcess = subprocess.Popen(cmd,
                                               stdin=subprocess.PIPE,
                                               stdout=subprocess.PIPE,
                                               stderr=subprocess.PIPE);

Re: [BUG] t9801 and t9803 broken on next

[Jeff King]

On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote:

> I think that is pretty much the problem. Here is what is happening:
> 
> 1.  git-p4 imports all changelists for the "main" branch
> 
> 2.  git-p4 starts to import a second branch and creates a fast-import
>     "progress checkpoint". This triggers:
> 
>     --> fast-import.c: cycle_packfile
>     --> fast-import.c: end_packfile
>     --> fast-import.c: loosen_small_pack
> 
>     At this point we have no packfile anymore.
> 
> 3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
>     to fast-import in order to create a new branch. This triggers:
> 
>     --> fast-import.c: parse_new_commit
>     --> fast-import.c: load_branch
>     --> fast-import.c: load_tree
> 
>     "load_tree" calls "find_object" and the result has a "pack_id" of 0.
>     I think this indicates that the object is in the packfile. Shouldn't
>     the "pack_id" be "MAX_PACK_ID" instead?
> 
>         myoe = find_object(sha1);
>         if (myoe && myoe->pack_id != MAX_PACK_ID) {

Thanks for the analysis. I think this is definitely the problem.  After
fast-import finalizes a packfile (either at the end of the program or
due to a checkpoint), it never discards its internal mapping of objects
to pack locations. It _has_ to keep such a mapping before the pack is
finalized, because git's regular object database doesn't know about it.
But afterwards, it should be able to rely on the object database.

Retaining the mapping at the end of the program is obviously OK because
we won't make any more lookups in it.

Retaining it at a checkpoint when we keep the packfile is OK, because we
can (usually) still access that packfile (the exception would be if
somebody runs "git repack" while fast-import is running).

But obviously a checkpoint where we throw away the pack needs to clear
that mapping.

The patch below probably makes your case work, but there are a lot of
open questions:

  1. Should we always discard the mapping, even when not loosening
     objects? I can't think of a real downside to always using git's
     object lookups.

  2. Can we free memory associated with object_entry structs at this
     point? They won't be accessible via the hash, but might other bits
     of the code have kept pointers to them?

     I suspect it may screw up the statistics that fast-import prints at
     the end, but that's a minor point.

  3. I notice that a few other structs (branch and tag) hold onto the
     pack_id, which will now point to a pack we can't access. Does this
     matter? I don't think so, because checkpoint() seems to dump the
     branches and tags.

  4. In general, should we be loosening objects at checkpoints at all?

     I guess it is probably more efficient than creating a bunch of
     little packs. And it should probably not happen much at all outside
     of tests (i.e., callers should generally checkpoint after an
     appreciable amount of work is done).

diff --git a/fast-import.c b/fast-import.c
index 0e8bc6a..9bfbfb0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -597,6 +597,22 @@ static struct object_entry *insert_object(unsigned char *sha1)
 	return e;
 }
 
+static void clear_object_table(void)
+{
+	unsigned int h;
+	for (h = 0; h < ARRAY_SIZE(object_table); h++) {
+		/*
+		 * We can't individually free objects here
+		 * because they are allocated from a pool.
+		 */
+		object_table[h] = NULL;
+	}
+	/*
+	 * XXX maybe free object_entry_pool here,
+	 * or might something still be referencing them?
+	 */
+}
+
 static unsigned int hc_str(const char *s, size_t len)
 {
 	unsigned int r = 0;
@@ -1035,6 +1051,9 @@ discard_pack:
 	pack_data = NULL;
 	running = 0;
 
+	/* The objects are now available via git's regular lookups. */
+	clear_object_table();
+
 	/* We can't carry a delta across packfiles. */
 	strbuf_release(&last_blob.data);
 	last_blob.offset = 0;

Re: [BUG] t9801 and t9803 broken on next

[Lars Schneider]

On 17 May 2016, at 14:13, Jeff King <peff@peff.net> wrote:

> On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote:
> 
>> I think that is pretty much the problem. Here is what is happening:
>> 
>> 1.  git-p4 imports all changelists for the "main" branch
>> 
>> 2.  git-p4 starts to import a second branch and creates a fast-import
>>    "progress checkpoint". This triggers:
>> 
>>    --> fast-import.c: cycle_packfile
>>    --> fast-import.c: end_packfile
>>    --> fast-import.c: loosen_small_pack
>> 
>>    At this point we have no packfile anymore.
>> 
>> 3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
>>    to fast-import in order to create a new branch. This triggers:
>> 
>>    --> fast-import.c: parse_new_commit
>>    --> fast-import.c: load_branch
>>    --> fast-import.c: load_tree
>> 
>>    "load_tree" calls "find_object" and the result has a "pack_id" of 0.
>>    I think this indicates that the object is in the packfile. Shouldn't
>>    the "pack_id" be "MAX_PACK_ID" instead?
>> 
>>        myoe = find_object(sha1);
>>        if (myoe && myoe->pack_id != MAX_PACK_ID) {
> 
> Thanks for the analysis. I think this is definitely the problem.  After
> fast-import finalizes a packfile (either at the end of the program or
> due to a checkpoint), it never discards its internal mapping of objects
> to pack locations. It _has_ to keep such a mapping before the pack is
> finalized, because git's regular object database doesn't know about it.
> But afterwards, it should be able to rely on the object database.
> 
> Retaining the mapping at the end of the program is obviously OK because
> we won't make any more lookups in it.
> 
> Retaining it at a checkpoint when we keep the packfile is OK, because we
> can (usually) still access that packfile (the exception would be if
> somebody runs "git repack" while fast-import is running).
> 
> But obviously a checkpoint where we throw away the pack needs to clear
> that mapping.
> 
> The patch below probably makes your case work, but there are a lot of
> open questions:
Confirmed. The offending tests pass with your patch.


>  1. Should we always discard the mapping, even when not loosening
>     objects? I can't think of a real downside to always using git's
>     object lookups.
> 
>  2. Can we free memory associated with object_entry structs at this
>     point? They won't be accessible via the hash, but might other bits
>     of the code have kept pointers to them?
> 
>     I suspect it may screw up the statistics that fast-import prints at
>     the end, but that's a minor point.
> 
>  3. I notice that a few other structs (branch and tag) hold onto the
>     pack_id, which will now point to a pack we can't access. Does this
>     matter? I don't think so, because checkpoint() seems to dump the
>     branches and tags.
>  4. In general, should we be loosening objects at checkpoints at all?
> 
>     I guess it is probably more efficient than creating a bunch of
>     little packs. And it should probably not happen much at all outside
>     of tests (i.e., callers should generally checkpoint after an
>     appreciable amount of work is done).
I am not too familiar with the code and therefore I can't give a good
answer to your questions. However, I noticed that Shawn (CC'ed) wrote a 
lot of fast-import code. Maybe he can help?

>From my point of view little packs are no problem. I run fast-import on
a dedicated migration machine. After fast-import completion I run repack [1] 
before I upload the repo to its final location.


Thanks,
Lars

[1] https://gcc.gnu.org/ml/gcc/2007-12/msg00165.html


> 
> diff --git a/fast-import.c b/fast-import.c
> index 0e8bc6a..9bfbfb0 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -597,6 +597,22 @@ static struct object_entry *insert_object(unsigned char *sha1)
> 	return e;
> }
> 
> +static void clear_object_table(void)
> +{
> +	unsigned int h;
> +	for (h = 0; h < ARRAY_SIZE(object_table); h++) {
> +		/*
> +		 * We can't individually free objects here
> +		 * because they are allocated from a pool.
> +		 */
> +		object_table[h] = NULL;
> +	}
> +	/*
> +	 * XXX maybe free object_entry_pool here,
> +	 * or might something still be referencing them?
> +	 */
> +}
> +
> static unsigned int hc_str(const char *s, size_t len)
> {
> 	unsigned int r = 0;
> @@ -1035,6 +1051,9 @@ discard_pack:
> 	pack_data = NULL;
> 	running = 0;
> 
> +	/* The objects are now available via git's regular lookups. */
> +	clear_object_table();
> +
> 	/* We can't carry a delta across packfiles. */
> 	strbuf_release(&last_blob.data);
> 	last_blob.offset = 0;

Re: [BUG] t9801 and t9803 broken on next

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> From my point of view little packs are no problem. I run fast-import on
> a dedicated migration machine. After fast-import completion I run repack [1] 
> before I upload the repo to its final location.

How do you determine that many little packs are not a problem to
you, though?  Until they get repacked, they are where the
fast-import will get existing objects from.  The more little packs
you accumulate as you go, the more slow fast-import's access to them
has to become.

Re: [BUG] t9801 and t9803 broken on next

[Lars Schneider]

> On 19 May 2016, at 19:03, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> From my point of view little packs are no problem. I run fast-import on
>> a dedicated migration machine. After fast-import completion I run repack [1] 
>> before I upload the repo to its final location.
> 
> How do you determine that many little packs are not a problem to
> you, though?  Until they get repacked, they are where the
> fast-import will get existing objects from.  The more little packs
> you accumulate as you go, the more slow fast-import's access to them
> has to become.
True. But my particular use case is a one time operation for a given
repository. Therefore I don't care how long it takes. The only important
aspect is that the result is correct.

That being said, Peff's proposed fix looks correct to me but since I 
am no fast-import expert my opinion doesn't count too much.

- Lars

Re: [BUG] t9801 and t9803 broken on next

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote:
> 
> > I think that is pretty much the problem. Here is what is happening:
> > 
> > 1.  git-p4 imports all changelists for the "main" branch
> > 
> > 2.  git-p4 starts to import a second branch and creates a fast-import
> >     "progress checkpoint". This triggers:
> > 
> >     --> fast-import.c: cycle_packfile
> >     --> fast-import.c: end_packfile
> >     --> fast-import.c: loosen_small_pack
> > 
> >     At this point we have no packfile anymore.
> > 
> > 3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
> >     to fast-import in order to create a new branch. This triggers:
> > 
> >     --> fast-import.c: parse_new_commit
> >     --> fast-import.c: load_branch
> >     --> fast-import.c: load_tree
> > 
> >     "load_tree" calls "find_object" and the result has a "pack_id" of 0.
> >     I think this indicates that the object is in the packfile. Shouldn't
> >     the "pack_id" be "MAX_PACK_ID" instead?

Yes; I think that is correct.  Alternative patch to Jeff's
coming in reply to this message.

> >         myoe = find_object(sha1);
> >         if (myoe && myoe->pack_id != MAX_PACK_ID) {
> 
> Thanks for the analysis. I think this is definitely the problem.  After
> fast-import finalizes a packfile (either at the end of the program or
> due to a checkpoint), it never discards its internal mapping of objects
> to pack locations. It _has_ to keep such a mapping before the pack is
> finalized, because git's regular object database doesn't know about it.
> But afterwards, it should be able to rely on the object database.

Almost; but relying on marks is a problem since that set can contain
mark => object_entry mappings which the normal object database won't
know about.

> The patch below probably makes your case work, but there are a lot of
> open questions:
> 
>   1. Should we always discard the mapping, even when not loosening
>      objects? I can't think of a real downside to always using git's
>      object lookups.

I'm not sure.  It's safe to clear the top-level table, but it
might speedup some lookups for just oe->type if we keep it
around.

I decided to keep it, anyways, because the mark set references them.

>   2. Can we free memory associated with object_entry structs at this
>      point? They won't be accessible via the hash, but might other bits
>      of the code have kept pointers to them?

Yes, invalid entries are also held in "struct mark_set marks";
this is a major problem with merely clearing the top-level
object table.

>      I suspect it may screw up the statistics that fast-import prints at
>      the end, but that's a minor point.

I still need to check, on that; but yeah, minor.

>   3. I notice that a few other structs (branch and tag) hold onto the
>      pack_id, which will now point to a pack we can't access. Does this
>      matter? I don't think so, because checkpoint() seems to dump the
>      branches and tags.

I don't think it matters unless a crash log or core dump is
created; then it becomes confusing to the person tracking down a
problem, so I've invalidated pack_id.  This doesn't affect
dump_branches or dump_tags from what I can tell.

>   4. In general, should we be loosening objects at checkpoints at all?

I think so.  It should be useful to checkpoint to make objects
available to other read-only processes while leaving a
fast-import running indefinitely.

[RFC] fast-import: invalidate pack_id references after loosening

[Eric Wong]

When loosening a pack, the current pack_id gets reused when
checkpointing and the import does not terminate.  This causes
problems after checkpointing as the object table, branch, and
tag lists still contains pre-checkpoint references to the
recycled pack_id.

Merely clearing the object_table as suggested by Jeff King in
http://mid.gmane.org/20160517121330.GA7346@sigill.intra.peff.net
is insufficient as the marks set still contains references
to object entries.

Wrong pack_id references branch and tags lists do not cause
errors, but can lead to misleading crash reports and core dumps,
so they are also invalidated.

Signed-off-by: Eric Wong <e@80x24.org>
---
 I started writing a standalone test case; but testing with
 original failing cases would be greatly appreciated.

 Still learning my way around the fast-import code...
 Thanks.

 fast-import.c                       | 31 +++++++++++++++++++-
 t/t9302-fast-import-unpack-limit.sh | 57 +++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 0e8bc6a..b9db4b6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -597,6 +597,33 @@ static struct object_entry *insert_object(unsigned char *sha1)
 	return e;
 }
 
+static void invalidate_pack_id(unsigned int id)
+{
+	unsigned int h;
+	unsigned long lu;
+	struct tag *t;
+
+	for (h = 0; h < ARRAY_SIZE(object_table); h++) {
+		struct object_entry *e;
+
+		for (e = object_table[h]; e; e = e->next)
+			if (e->pack_id == id)
+				e->pack_id = MAX_PACK_ID;
+	}
+
+	for (lu = 0; lu < branch_table_sz; lu++) {
+		struct branch *b;
+
+		for (b = branch_table[lu]; b; b = b->table_next_branch)
+			if (b->pack_id == id)
+				b->pack_id = MAX_PACK_ID;
+	}
+
+	for (t = first_tag; t; t = t->next_tag)
+		if (t->pack_id == id)
+			t->pack_id = MAX_PACK_ID;
+}
+
 static unsigned int hc_str(const char *s, size_t len)
 {
 	unsigned int r = 0;
@@ -993,8 +1020,10 @@ static void end_packfile(void)
 				    cur_pack_sha1, pack_size);
 
 		if (object_count <= unpack_limit) {
-			if (!loosen_small_pack(pack_data))
+			if (!loosen_small_pack(pack_data)) {
+				invalidate_pack_id(pack_id);
 				goto discard_pack;
+			}
 		}
 
 		close(pack_data->pack_fd);
diff --git a/t/t9302-fast-import-unpack-limit.sh b/t/t9302-fast-import-unpack-limit.sh
index 0f686d2..a04de14 100755
--- a/t/t9302-fast-import-unpack-limit.sh
+++ b/t/t9302-fast-import-unpack-limit.sh
@@ -45,4 +45,61 @@ test_expect_success 'bigger packs are preserved' '
 	test $(find .git/objects/pack -type f | wc -l) -eq 2
 '
 
+test_expect_success 'lookups after checkpoint works' '
+	hello_id=$(echo hello | git hash-object --stdin -t blob) &&
+	id="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" &&
+	before=$(git rev-parse refs/heads/master^0) &&
+	(
+		cat <<-INPUT_END &&
+		blob
+		mark :1
+		data 6
+		hello
+
+		commit refs/heads/master
+		mark :2
+		committer $id
+		data <<COMMIT
+		checkpoint after this
+		COMMIT
+		from refs/heads/master^0
+		M 100644 :1 hello
+
+		# pre-checkpoint
+		cat-blob :1
+		cat-blob $hello_id
+		checkpoint
+		# post-checkpoint
+		cat-blob :1
+		cat-blob $hello_id
+		INPUT_END
+
+		n=0 &&
+		from=$before &&
+		while test x"$from" = x"$before"
+		do
+			if test $n -gt 30
+			then
+				echo >&2 "checkpoint did not update branch"
+				exit 1
+			else
+				n=$(($n + 1))
+			fi &&
+			sleep 1 &&
+			from=$(git rev-parse refs/heads/master^0)
+		done &&
+		cat <<-INPUT_END &&
+		commit refs/heads/master
+		committer $id
+		data <<COMMIT
+		make sure from "unpacked sha1 reference" works, too
+		COMMIT
+		from $from
+		INPUT_END
+		echo done
+	) | git -c fastimport.unpackLimit=100 fast-import --done &&
+	test $(find .git/objects/?? -type f | wc -l) -eq 6 &&
+	test $(find .git/objects/pack -type f | wc -l) -eq 2
+'
+
 test_done

Re: [RFC] fast-import: invalidate pack_id references after loosening

[Jeff King]

On Wed, May 25, 2016 at 10:54:02PM +0000, Eric Wong wrote:

> When loosening a pack, the current pack_id gets reused when
> checkpointing and the import does not terminate.  This causes
> problems after checkpointing as the object table, branch, and
> tag lists still contains pre-checkpoint references to the
> recycled pack_id.
> 
> Merely clearing the object_table as suggested by Jeff King in
> http://mid.gmane.org/20160517121330.GA7346@sigill.intra.peff.net
> is insufficient as the marks set still contains references
> to object entries.
> 
> Wrong pack_id references branch and tags lists do not cause
> errors, but can lead to misleading crash reports and core dumps,
> so they are also invalidated.

Nicely explained.

> +static void invalidate_pack_id(unsigned int id)
> +{
> +	unsigned int h;
> +	unsigned long lu;
> +	struct tag *t;
> +
> +	for (h = 0; h < ARRAY_SIZE(object_table); h++) {
> +		struct object_entry *e;
> +
> +		for (e = object_table[h]; e; e = e->next)
> +			if (e->pack_id == id)
> +				e->pack_id = MAX_PACK_ID;
> +	}
> +
> +	for (lu = 0; lu < branch_table_sz; lu++) {
> +		struct branch *b;
> +
> +		for (b = branch_table[lu]; b; b = b->table_next_branch)
> +			if (b->pack_id == id)
> +				b->pack_id = MAX_PACK_ID;
> +	}
> +
> +	for (t = first_tag; t; t = t->next_tag)
> +		if (t->pack_id == id)
> +			t->pack_id = MAX_PACK_ID;
> +}

This looks pretty straightforward. I do notice that we never shrink the
number of items in the object table when checkpointing, and so our
linear walk will grow ever larger. So if one were to checkpoint every
k-th object, it makes the whole operation quadratic in the number of
objects (actually, O(n^2/k) but k is a constant).

That's probably OK in practice, as I think the actual pack-write already
does a linear walk of the object table to generate the pack index. So
presumably nobody checkpoints often enough for it to be a problem. And
the solution would be to keep a separate list of pointers to objects for
the current pack-id, which would trivially fix both this case and the
one in create_index().  So we can punt on it until somebody actually
runs into it, I think.

-Peff

Re: [RFC] fast-import: invalidate pack_id references after loosening

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Wed, May 25, 2016 at 10:54:02PM +0000, Eric Wong wrote:
> > +	for (h = 0; h < ARRAY_SIZE(object_table); h++) {
> > +		struct object_entry *e;
> > +
> > +		for (e = object_table[h]; e; e = e->next)
> > +			if (e->pack_id == id)
> > +				e->pack_id = MAX_PACK_ID;
> > +	}

<snip>

> This looks pretty straightforward. I do notice that we never shrink the
> number of items in the object table when checkpointing, and so our
> linear walk will grow ever larger. So if one were to checkpoint every
> k-th object, it makes the whole operation quadratic in the number of
> objects (actually, O(n^2/k) but k is a constant).

Good point, I'll work on a separate patch to fix it.

> That's probably OK in practice, as I think the actual pack-write already
> does a linear walk of the object table to generate the pack index. So
> presumably nobody checkpoints often enough for it to be a problem. And
> the solution would be to keep a separate list of pointers to objects for
> the current pack-id, which would trivially fix both this case and the
> one in create_index().  So we can punt on it until somebody actually
> runs into it, I think.

I might checkpoint that much and run into the problem soon :)
Too tired now; maybe in a day or two I'll be able to make sense
of C again :x

Re: [RFC] fast-import: invalidate pack_id references after loosening

[Jeff King]

On Thu, May 26, 2016 at 08:02:36AM +0000, Eric Wong wrote:

> > That's probably OK in practice, as I think the actual pack-write already
> > does a linear walk of the object table to generate the pack index. So
> > presumably nobody checkpoints often enough for it to be a problem. And
> > the solution would be to keep a separate list of pointers to objects for
> > the current pack-id, which would trivially fix both this case and the
> > one in create_index().  So we can punt on it until somebody actually
> > runs into it, I think.
> 
> I might checkpoint that much and run into the problem soon :)
> Too tired now; maybe in a day or two I'll be able to make sense
> of C again :x

In theory the list of objects in the allocation pool are contiguous for
a particular pack. So you could break out of the double-loop at the
first non-matching object you see:

diff --git a/fast-import.c b/fast-import.c
index 83558dc..f214bd3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -900,10 +900,13 @@ static const char *create_index(void)
 	c = idx;
 	for (o = blocks; o; o = o->next_pool)
 		for (e = o->next_free; e-- != o->entries;)
 			if (pack_id == e->pack_id)
 				*c++ = &e->idx;
+			else
+				goto done;
+done:
 	last = idx + object_count;
 	if (c != last)
 		die("internal consistency error creating the index");
 
 	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);

But that seems to break some of the tests, so I think there is some case
which does not match my theory. :)

Another strategy is to note that we cross-check against object_count
here. So you could simply break out of the loop when we have found
object_count matches.

We don't keep similar counters for branches/tags (which have a similar
quadratic problem, though presumably much smaller "n"), but it would be
easy to keep an offset in start_packfile() and just start the iteration
there.

-Peff

Re: [RFC] fast-import: invalidate pack_id references after loosening

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Thu, May 26, 2016 at 08:02:36AM +0000, Eric Wong wrote:
> 
> > > That's probably OK in practice, as I think the actual pack-write already
> > > does a linear walk of the object table to generate the pack index. So
> > > presumably nobody checkpoints often enough for it to be a problem. And
> > > the solution would be to keep a separate list of pointers to objects for
> > > the current pack-id, which would trivially fix both this case and the
> > > one in create_index().  So we can punt on it until somebody actually
> > > runs into it, I think.
> > 
> > I might checkpoint that much and run into the problem soon :)
> > Too tired now; maybe in a day or two I'll be able to make sense
> > of C again :x

OK, I guess I was too tired and not thinking clearly.

I now figure it's not worth it to checkpoint frequently and have
a very-long-lived fast-import process.  The object table and
marks set will grow indefinitely, so forking off another gfi
with a new set of marks is better for my case[1], anyways.


Junio: I think my RFC can go into pu as-is and replace
Jeff's object_table discarding patch.


[1] inotify watching a Maildir + fast-import

> In theory the list of objects in the allocation pool are contiguous for
> a particular pack. So you could break out of the double-loop at the
> first non-matching object you see:
> 
> diff --git a/fast-import.c b/fast-import.c
> index 83558dc..f214bd3 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -900,10 +900,13 @@ static const char *create_index(void)
>  	c = idx;
>  	for (o = blocks; o; o = o->next_pool)
>  		for (e = o->next_free; e-- != o->entries;)
>  			if (pack_id == e->pack_id)
>  				*c++ = &e->idx;
> +			else
> +				goto done;
> +done:
>  	last = idx + object_count;
>  	if (c != last)
>  		die("internal consistency error creating the index");
>  
>  	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);
> 
> But that seems to break some of the tests, so I think there is some case
> which does not match my theory. :)

Yes, it's probably because duplicate objects from the object
store will create entries with e->pack_id == MAX_PACK_ID

> Another strategy is to note that we cross-check against object_count
> here. So you could simply break out of the loop when we have found
> object_count matches.

I'm worried that could fail to detect bugs which should've led
us to die of the internal consistency error.


What we could probably do more safely is limit the scan to only
recent object pools (new ones since the previous create_index call
and the last one scanned).

This passes tests, but I could be missing something:

--- a/fast-import.c
+++ b/fast-import.c
@@ -926,14 +926,16 @@ static const char *create_index(void)
 	struct pack_idx_entry **idx, **c, **last;
 	struct object_entry *e;
 	struct object_entry_pool *o;
+	static struct object_entry_pool *last_seen;
 
 	/* Build the table of object IDs. */
 	ALLOC_ARRAY(idx, object_count);
 	c = idx;
-	for (o = blocks; o; o = o->next_pool)
+	for (o = blocks; o; o = (o == last_seen) ? NULL : o->next_pool)
 		for (e = o->next_free; e-- != o->entries;)
 			if (pack_id == e->pack_id)
 				*c++ = &e->idx;
+	last_seen = blocks;
 	last = idx + object_count;
 	if (c != last)
 		die("internal consistency error creating the index");

Re: [BUG] t9801 and t9803 broken on next

[Jeff King]

On Wed, May 25, 2016 at 10:49:07PM +0000, Eric Wong wrote:

> > Thanks for the analysis. I think this is definitely the problem.  After
> > fast-import finalizes a packfile (either at the end of the program or
> > due to a checkpoint), it never discards its internal mapping of objects
> > to pack locations. It _has_ to keep such a mapping before the pack is
> > finalized, because git's regular object database doesn't know about it.
> > But afterwards, it should be able to rely on the object database.
> 
> Almost; but relying on marks is a problem since that set can contain
> mark => object_entry mappings which the normal object database won't
> know about.

Ah, thanks for finding that. I had a feeling there might be other users
of the object_entry structs, but I didn't dig.

Given that and your other responses here, I agree that just invalidating
the pack_id is probably the most sensible route.

-Peff

Re: [BUG] t9801 and t9803 broken on next

[Luke Diamand]

On 14 May 2016 at 19:15, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> On 13 May 2016, at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Are you saying that "git p4" itself breaks unless fast-import always
>>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>>> if the test script is somehow looking at the number of packs or
>>> loose objects and declaring a failure, even when the resulting
>>> history in p4 and git are correct, then that is a different issue,
>>> and forcing to explode a tiny pack is a reasonable workaround.  I
>>> couldn't quite tell which the case is.
>>>
>>> Puzzled.  Please help.
>>
>> t9801 "import depot, branch detection" is the first test that fails
>> with a fast import error:
>> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>>
>> fast-import crash report:
>>     fast-import process: 77079
>>     parent process     : 77077
>>     at 2016-05-14 07:48:40 +0000
>>
>> fatal: offset beyond end of packfile (truncated pack?)
>
> Hmm, does that suggest Eric's "explode them into loose instead of
> keeping a small pack" insufficient?  It sounds like that somebody
> wanted to read some data back from its packfile without knowing that
> the updated code may make it available in a loose object form, which
> would mean that somebody needs to be told about what is going on,
> namely, these objects are not in a half-written pack but are found
> as loose objects.
>
>> The problem seems to be related to the git-p4 branch import.
>> I don't have time this weekend to look further into it, but
>> I will try next week.

The last thing that fast-import is sent before it crashes is this:

checkpoint
  progress checkpoint
  commit refs/remotes/p4/depot/branch2
  mark :6
  committer Dr. author <author@example.com> 1463212118 +0000
  data <<EOT
  from 1ab0d5b43b6070682f8bd6f3fd674d4fcddd9345

i.e. creating a new branch, referring to an existing object. Is it
possible that fast-import can't find 1ab0d... because of the reasons
given by Junio above? I don't know enough about fast-import tbh.

I will see if I can get fast-import to tell me a bit more about why
it's failing.

Luke