Skip to content
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
Merged

fix timestamps from git #43

merged 8 commits into from
May 16, 2016

Conversation

hannesm
Copy link
Collaborator

@hannesm 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
Copy link
Collaborator Author

hannesm commented May 6, 2016

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

@hannesm
Copy link
Collaborator Author

hannesm commented May 6, 2016

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

@hannesm
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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)

@abbysmal
Copy link
Owner

abbysmal 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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@abbysmal
Copy link
Owner

abbysmal 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
Copy link
Collaborator Author

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 mentioned this pull request May 16, 2016
@abbysmal abbysmal merged commit 056bf2b into abbysmal:master May 16, 2016
@hannesm hannesm deleted the minor branch May 16, 2016 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

last updated is after published!?
2 participants