[BUG] t9801 and t9803 broken on next

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[BUG] t9801 and t9803 broken on next

larsxschneider
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Eric Wong-2
Lars Schneider <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Junio C Hamano
Eric Wong <[hidden email]> writes:

> Lars Schneider <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

larsxschneider

> On 13 May 2016, at 18:37, Junio C Hamano <[hidden email]> wrote:
>
> Eric Wong <[hidden email]> writes:
>
>> Lars Schneider <[hidden email]> 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 <[hidden email]> 1463212118 +0000
  data <<EOT
  M 100644 inline f1
  data 3
 
  commit refs/remotes/p4/master
  mark :2
  committer Dr. author <[hidden email]> 1463212118 +0000
  data <<EOT
  M 100644 inline f2
  data 3
 
  commit refs/remotes/p4/master
  mark :4
  committer Dr. author <[hidden email]> 1463212118 +0000
  data <<EOT
  M 100644 inline f4
  data 3
 
  checkpoint
  commit git-p4-tmp/6
  mark :6
  committer Dr. author <[hidden email]> 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 <[hidden email]> 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--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Junio C Hamano
Lars Schneider <[hidden email]> writes:

>> On 13 May 2016, at 18:37, Junio C Hamano <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

larsxschneider

> On 14 May 2016, at 20:15, Junio C Hamano <[hidden email]> wrote:
>
> Lars Schneider <[hidden email]> writes:
>
>>> On 13 May 2016, at 18:37, Junio C Hamano <[hidden email]> 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--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Luke Diamand
In reply to this post by Junio C Hamano
On 14 May 2016 at 19:15, Junio C Hamano <[hidden email]> wrote:

> Lars Schneider <[hidden email]> writes:
>
>>> On 13 May 2016, at 18:37, Junio C Hamano <[hidden email]> 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 <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Eric Wong-2
In reply to this post by larsxschneider
Lars Schneider <[hidden email]> 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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Jeff King
In reply to this post by larsxschneider
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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

larsxschneider

On 17 May 2016, at 14:13, Jeff King <[hidden email]> 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;

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Junio C Hamano
Lars Schneider <[hidden email]> 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.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

larsxschneider

> On 19 May 2016, at 19:03, Junio C Hamano <[hidden email]> wrote:
>
> Lars Schneider <[hidden email]> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Eric Wong-2
In reply to this post by Jeff King
Jeff King <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eric Wong-2
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@...
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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [BUG] t9801 and t9803 broken on next

Jeff King
In reply to this post by Eric Wong-2
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jeff King
In reply to this post by Eric Wong-2
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@...
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eric Wong-2
Jeff King <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...