New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix timestamps from git #43

Merged
merged 8 commits into from May 16, 2016

Conversation

Projects
None yet
2 participants
@hannesm
Collaborator

hannesm commented May 6, 2016

fixes #33 and #41

instead of traversing the history twice, we now do it only once (per key), using topological.fold (thus going through the commits in order from first to last).
we read whether the key is in the temporary store, and what the last value of the key was:
if there was no last value, and there is now one, we're lucky -- this is the created commit
if there was a last value, and its the same as now, there wasn't a change to that specific key
if there was a last value, and the one now is different, we adjust the updated commit
if there was a last value, and none now, we basically ignore (since we do this only over all current keys, deletion does not happen)

in HEAD~1 I had some debug output in case there's some issue with the code...

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 6, 2016

also updates to current irmin (0.11.0) and has some stylistic fixes

@hannesm hannesm force-pushed the hannesm:minor branch from a7ea53b to 0651c23 May 6, 2016

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 6, 2016

hmm, with irmin 0.10.1 this doesn't seem to work well...

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 8, 2016

for 0.10.1, I have another branch there since 0.11.0 doesn't like xen (see mirage/ocaml-git#151)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 8, 2016

(which I actually have deployed now on https://hannes.nqsb.io -- it first each key to a (timestamp, data) list (if the key is present in a commit), then sorts this list based on timestamp, and extracts the first (as created) and updated: the x from the reverse where data did not change between head..x)

@Engil

This comment has been minimized.

Owner

Engil commented May 8, 2016

Thank you for taking time with this issue. :)
This is great, it is indeed better to only traverse the graph once, even though I had some plans to remove Topological at some point since it is a bit slow… But will do for know. :)
I still find it weird that the repo is broken once, and only once a pull is done (if I understand the issue correctly)… this shouldn't change much thing, maybe I don't understand correctly the issue ?

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 8, 2016

I don't understand much... what imho should be done in a future version is to a) pull b) find all articles) c) go over each commit, look which articles are modified d) have the created and update timestamps (rather than go over each commit for each article, as done atm)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 8, 2016

(but I've decided to not touch the code anymore until I get git-1.8.0 (or above) working on Xen...) imho my last commit in the irmin-0.10.1 branch makes it much cleaner, although for irmin-0.11.0 I guess we can assume that the Topological,fold has commits sorted by time (and thus there's no need for a List.sort)..

@Engil

This comment has been minimized.

Owner

Engil commented May 8, 2016

Yup, this is something I had in mind, shouldn't be too hard to do.
Will merge (after testing a bit later today) if it's okay with you, or maybe you want to remove the irmin fix for 0.11 and stay on 0.10.1 for now ?

@hannesm

This comment has been minimized.

Collaborator

hannesm commented May 8, 2016

not sure what makes sense for Canopy master, I'll for sure stay on my irmin-0.10.1 branch until further notice... feel free to merge whatever you like ;) (both branches improve the situation imho, whereas this one only works with 0.11.0)...

@hannesm hannesm referenced this pull request May 16, 2016

Closed

Master doesn't build #55

@Engil Engil merged commit 056bf2b into Engil:master May 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hannesm hannesm deleted the hannesm:minor branch May 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment