search

dalehenrich / filetree

Monticello repository for directory-based Monticello packages enabling the use of git, svn, etc. for managing Smalltalk source code.
https://github.com/CampSmalltalk/Cypress
MIT License
133 stars 26 forks source link

Git repository tests failing with Issue #90 bugfix #91

Closed dalehenrich closed 11 years ago

dalehenrich commented 11 years ago

@ThierryGoubier, [Here's the commit(https://github.com/dalehenrich/filetree/commit/fa26de3983fb98bde8c26fd0762fa0e5ec736c0f) for the build of the Issue #90 bufix.

And here are the tests that are failing.

I still cannot run the tests on my mac because of the git issues mentioned elsewhere and looking at the code I really cannot see how the changes I made could have caused the following error (from instrumentation in MCFileTreeIssue33Test>>testWrite in MCGitFileTreeIssue33Test>>testWrite:

Error reading repository properties (.filetree): /home/travis/dalehenrich-builderCI-3062fdf/builds/travisCI/temp :: Error: end of input expected

Of course, being on Travis I can't see the temp directory nor can I see the original repo.

Also don't quite understand how MCFileTreeIssue33Test>>testWrite (where I put my instrumentation to catch errors) given the fact that you have overridden the #testWrite method in the MCGitFileTreeIssue33Test class?

Several mysteries and I'm hamstrung not being able to run the git tests at all..so if you could take a look that'd be great.

Dale

dalehenrich commented 11 years ago

On closer inspection I see that MCFileTreeIssue33Test>>testWrite is also being run (whew!) so that explains why I'm seeing the logging information, but it doesn't explain why that test is getting errors in the directory /home/travis/dalehenrich-builderCI-3062fdf/builds/travisCI/temp ... it should only be working with the testRepositories ... but this does give me something to look at ... I started using the ../temp directory for a test for Issue #90 (not Issue #33) but that directory might be interfering with things .. I have an ensure block that is supposed to remove the directory after the test, but there might be some odd things going on pharo2.0 with monticello repositories that I will look into ... so for now don't bother looking at this stuff until I've got more information...

Thanks...

dalehenrich commented 11 years ago

Down to one error in git repo tests ... will see if I can figure it out ... tonight?

ThierryGoubier commented 11 years ago

Ok, I know what is happening (and why the test fails) : the temp/ repo is created with a default .tree extension instead of .package. A recent commit must have changed something on repo instantiation.

Pre-existing repos have the extension in their .filetree metadata, so we can only see the difference when creating a new one (temp/)

dalehenrich commented 11 years ago

Okay ... and that sort of thing shouldn't be happening ... the default extension should be .tree ...

Let me look into this, because it is clearly a result of the recent changes that I did ...

Thanks,

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, July 8, 2013 11:26:53 AM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| Ok, I know what is happening (and why the test fails) : the temp/ | repo is created with a default .tree extension instead of .package. | A recent commit must have changed something on repo instantiation. | Pre-existing repos have the extension in their .filetree metadata, so | we can only see the difference when creating a new one (temp/) | — | Reply to this email directly or view it on GitHub .

ThierryGoubier commented 11 years ago

I'm looking and trying to force the writeRepositoryProperties to update the properties inside the repo instance... No success yet.

dalehenrich commented 11 years ago

Don't worry ... I'll take a look and see what I can do ...

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, July 8, 2013 11:53:06 AM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| I'm looking and trying to force the writeRepositoryProperties to | update the properties inside the repo instance... No success yet. | — | Reply to this email directly or view it on GitHub .

ThierryGoubier commented 11 years ago

Ok, if I change MCFileTreeRepository>>writeRepositoryProperties

with

writeRepositoryProperties

    self fileUtils
        writeStreamFor: '.filetree'
        in: self directory
        do: [ :fileStream | 
            fileStream lineEndConvention: #lf.
            repositoryProperties
                at: 'packageExtension' put: self class defaultPackageExtension;
                at: 'propertyFileExtension' put: self propertyFileExtension.
            fileStream
                nextPutAll: '{ "packageExtension" : "' , self class defaultPackageExtension , '",';
                cr;
                nextPutAll: '  "propertyFileExtension" : "' , self propertyFileExtension , '" }';
                cr;
                yourself ]

Then the test pass. I need to check the other tests as well.

ThierryGoubier commented 11 years ago

Ok, all others tests are green. I have updated until issue90, so there is a good chance it will be OK for you as well.

dalehenrich commented 11 years ago

Ah, so we need to initialize the repositoryProperties when we write? ... I don't think that's exactly what should be happening ... the repositoryProperties should be initialized first and the writeRepositoryProperties should reflect the current settings ... so in looking at the code, I've just got that completely wrong ... so there are probably code paths where things don't match what is written and vice versa ...

I will want to revisit this logic anyway:)

Thanks for narrowing this down ...while the tests might be green, I think that the implementation is not quite what I want...

Thanks man!

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, July 8, 2013 11:58:55 AM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| Ok, if I change MCFileTreeRepository>>writeRepositoryProperties | with | writeRepositoryProperties self fileUtils writeStreamFor: '.filetree' | in: self directory do: [ : fileStream | fileStream | lineEndConvention: #lf . repositoryProperties at: 'packageExtension' | put: self class defaultPackageExtension ; at: | 'propertyFileExtension' put: self propertyFileExtension . fileStream | nextPutAll: '{ "packageExtension" : "' , self class | defaultPackageExtension , '",' ; cr ; nextPutAll: ' | "propertyFileExtension" : "' , self propertyFileExtension , '" }' ; | cr ; yourself ] | Then the test pass. I need to check the other tests as well. | — | Reply to this email directly or view it on GitHub .

ThierryGoubier commented 11 years ago

Maybe also why do we have that:

packageExtension
    ^ self repositoryProperties at: 'packageExtension' ifAbsent: [ '.tree' ]

compared to

propertyFileExtension
  ^ self repositoryProperties
    at: 'propertyFileExtension'
    ifAbsent: [ self class defaultPropertyFileExtension ]

because self class defaultPackageExtension is '.package'

dalehenrich commented 11 years ago

Yep ... there is support for reading multiple formats in the code and it might make sense for stripping out the code that supports the writing of multiple formats to simplify things a bit ...

Frankly when I made my changes yesterday to introduce the properFileExtension logic I thought things a bit more smoothly than I anticipated:)

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, July 8, 2013 12:32:47 PM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| Maybe also why do we have that: | packageExtension ^ self repositoryProperties at: 'packageExtension' | ifAbsent: [ '.tree' ] | compared to | propertyFileExtension ^ self repositoryProperties at: | 'propertyFileExtension' ifAbsent: [ self class | defaultPropertyFileExtension ] | because self class defaultPackageExtension is '.package' | — | Reply to this email directly or view it on GitHub .

ThierryGoubier commented 11 years ago

And you must have noticed how I gave up trying to read non-cypress formats in gitfiletree...

I'll have to add the .ston support as well to gitfiletree.

dalehenrich commented 11 years ago

I didn't notice that, but it makes sense ... I was thinking of making the monticello meta data optional as well (it is such a pain to deal with the conflicts) and for gitfiletree, I'd imagine that would be the default ...

BTW, I think I've already added the support for at least reading .ston files to gitfiletree ...

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, July 8, 2013 12:40:29 PM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| And you must have noticed how I gave up trying to read non-cypress | formats in gitfiletree... | I'll have to add the .ston support as well to gitfiletree. | — | Reply to this email directly or view it on GitHub .

ThierryGoubier commented 11 years ago

Thanks.

What you say got me thinking about getting rid of the version metadata, by rebuilding it via git. Would it be ok then if:

I could try as a first step to not load the version metadata and recreate a version history via git (maybe partial because I don't want to launch too many git commands). I'll explore the use of libgit2 via NB as well.

dalehenrich commented 11 years ago

Thierry,

I would be inclined to throw away the meta-data entirely ... in an earlier version of FileTree we did not store the meta data in the repository at all ... we could probably get away with treating each load as freshly created package (as far as meta data is concerned), but I don't recall the details so I'd have to do some spelunking ... there is a way to adopt a version history, so if one every needs to move from git-based back into mcz based one can do that ...

I think that UUIDs are expected to be in a certain format, so if you want to experiment, I suggest that you generate new meta data for the package when you bring it into the image as if it were a freshly created monticello package (which it is)...

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Tuesday, July 9, 2013 11:57:44 AM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| Thanks. | What you say got me thinking about getting rid of the version | metadata, by rebuilding it via git. Would it be ok then if:

| | the version number is computed out of the git history? | | the UUID is replaced by the git commitID? | | the author is unknown or nil (or the git author)? | | ancestors are the git commit ancestors ?

| I could try as a first step to not load the version metadata and | recreate a version history via git (maybe partial because I don't | want to launch too many git commands). I'll explore the use of | libgit2 via NB as well. | — | Reply to this email directly or view it on GitHub .

dalehenrich commented 11 years ago

@ThierryGoubier, last commit addressed the issue you raised in this comment

ThierryGoubier commented 11 years ago

Thanks, Dale!

dalehenrich commented 11 years ago

The work was done on the gemstone branch so I still need to test against pharo2.0 ..

Dale

----- Original Message -----

| From: "Thierry Goubier" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Wednesday, July 10, 2013 8:43:11 AM | Subject: Re: [filetree] Git repository tests failing with Issue #90 | bugfix (#91)

| Thanks, Dale! | — | Reply to this email directly or view it on GitHub .

dalehenrich commented 11 years ago

Pharo 2.0 tests green ... Need to propagate bugrixes to all platforms (in progress) then 1.0.4 might be ready for release...

dalehenrich commented 11 years ago

All builds (that matter) are green (415-420)

dalehenrich commented 11 years ago

forgot to merge in Squeak4.3 and got a green build ... Now we are good to go!