Unnecessary use of 'Commit' objects in views

[skeating]

There are several places in code where we use something like
entity.repo.get_commit(version.sha))
and pass the resulting commit object to an html view. Here it used to extract information about the commit - author/date/numfiles etc but there is no need for actual commit object. Since every commit records some information in the repocache surely it would be better to extend the information stored (some is there - some is not) so that there is no need to grab the actual commit.

What particularly confuses me is that the commit object gets passed to the url functions which seem to then create url based on tag if it exists or a reduced version of the sha. I haven't worked out the logic of this - but that also seems overkill for creating a url. The appropriate string is what is needed.

Note: I am talking only about views since these do not actually use anything other than attributes of the Commit. I realise other things may need the whole thing.

Disclaimer: I might be missing something crucial :-)

[jonc125]

As suggested above, what we want to do here is replace most uses of the actual entities.repository.Commit objects with repocache.CachedEntityVersion, extending the information stored there as necessary, so that everything that doesn't absolutely require hitting the disk (like reading a file's content, or adding a new commit) can work just with the DB cache. Also, the CachedEntityVersion API should be updated to match (i.e. use the same names as) the Commit class. This should build on the work in #218.

Changes required to CachedEntityVersion:

• Rename sha column to hexsha (or vice versa? Depends if we want to match the GitPython API... Currently 393 uses of hexsha, ~30 of sha, but sha is easier to switch to)
• Rename timestamp column to committed_at (or vice versa? Neither match GitPython! Only ~10 uses, so easy to switch. I think I prefer timestamp, which also avoids a DB migration.)
• Add author property to reference entity.entity.author.full_name (or annotate with it when we query? But that would mean changing every query where author info is used; possibly hard to do.)
• Add message column
• Add master_filename column
• Consider whether we want to cache the filenames property (perhaps as an ArrayField, which wouldn't need a specialised index as we don't query on it) - we'd need to be careful about updating it when ephemeral files change, in add_ephemeral_file.
  • We might just want the number: see e.g. with commit.filenames|length as numfiles in templates. This is actually the only use case I can find when we wouldn't need the full commit anyway, so caching just numfiles should be fine
• Check uses of parents (only used in record_experiments_to_run - low priority), ancestors, self_and_ancestors (only used in tests!) So I don't think we need to cache any of these.

Adding new items to the cache will require extending populate_entity_cache to store the info, updating if necessary, so that deployment fills the new columns. See e.g. the if visibility and visibility != version.visibility: code block which does this for visibility.

• We'll need to change get_commit in views (or most uses thereof at least) to return the cache object.

Should we also align Repository and CachedEntity? Which way?!

• commits vs versions (former only used by repocache and analyse_entity_versions.py; latter used all over the place, including URLs & many templates, ~400 times)
• latest_commit vs latest_version (former only used 3 times outside tests, 2 of which should use cache; latter is also used for experiment versions and in template context)
• get_commit() vs get_version() (5 vs 8 uses outside tests & entity views)

I think my inclination would be to keep these names as is, so it's clearer when you actually are getting a real commit.

Note that a commit is typically called version in context data, and typically here the cache object would be sufficient.

• For tags, it's probably actually easier for code to use the current repocache API than the Repository API! And most already does. There's only one use of tag_dict outside of tests and populating the cache.

[jonc125]

In #234 I've been spotting uses of Commit that we might want to replace (and adding TODO comments appropriately). So once the cache changes have been done we might want to review & merge #222 and #234 before continuing with this issue?

But there's also a discussion point from my latest commit. The JSON views for entities include info about file names & sizes. When you're viewing a single version it's not a big issue to get the full commit data. But for comparing you might be accessing several commits. However, this is just the comparison of model or protocol definitions, so the number of commits tends to be small, and typically all within the same repo (unlike for experiment result comparisons). So I'm still not sure it's worth caching the file names & sizes. Perhaps just leave as a note for future consideration if it turns out to be a performance issue?

[sroderick-g5sro]

It does seem that we only need the number of files rather than the full filenames. Shall I add numfiles as a new db field? not sure about the ephemeral_files. Do I need any special processing to handle these?

[jonc125]

Yes, just add a numfiles field to the cache. You shouldn't need to do anything special with the ephemeral files, just look at len(commit.filenames).