Currently, we use the following --pretty custom format string in the log parser to output commit data for parsing and capturing:

git show --numstat --summary --pretty=format:"%H%n%P%n%aN <%ae>%n%cN <%ce>%n%ct%n%s%n%b%nENDOFOUTPUTGITMESSAGEHERE"

For a commit object, this outputs something like the following:

$ git show --numstat --summary --pretty=format:"%H%n%P%n%aN <%ae>%n%cN <%ce>%n%ct%n%s%n%b%nENDOFOUTPUTGITMESSAGEHERE" 435fa3
 435fa3a3b41d81ef557f720246a21c93d324da81 # commit id
 a48b3a85b0f87cae7c9755ee007dd705b71b32aa # parent commit id
 Sam Boyer <drupal@samboyer.org> # author info
 Sam Boyer <drupal@samboyer.org> # committer info
 1281119399 # timestamp
 So the SelectQuery::join() method isn't chainable. Who knew.  # commit message
 
 ENDOFOUTPUTGITMESSAGEHERE # fugly delimiter
 4       4       includes/controllers.inc # diffstat

Under a lot of circumstances, this is OK, and when I initially commit the entities refactor/the git log parser refactor, I'm going to leave it like this, but it's critical we fix it. There are several issues with that command as it currently exists, though only the last one (merging) is critical:

We don't deal smartly with dates.

Right now, dates are just dealt with as straight timestamps. AFAIK, there's no timezone conversion that happens when they're shown in this way, which means that we might have problems if commits come in from different timezones but we don't do proper timezone handling on the d.o side when we're parsing the metadata into our db.

The delimiter is fugly.
Using ENDOFGIT... whatever is not the way to do things. We need to look into using NUL chars more, probably, as that's the option provided by -z. Though that's not perfect, either.

It does not even pretend to capture full merge data.
If that command is passed a merge commit, it will output the numstat for only a single parent commit, e.g. for the merge commit 32d5d:

$ git show --numstat --summary --pretty=format:"%H%n%P%n%aN <%ae>%n%cN <%ce>%n%ct%n%s%n%b%nENDOFOUTPUTGITMESSAGEHERE" 32d5d
 32d5d3d0ab8eabb812fedef73f6dd1f4e74c288b
 e111ad0f0165c1b85def0dc89005c36be420793d 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0
 Sam Boyer <drupal@samboyer.org>
 Sam Boyer <drupal@samboyer.org>
 1281398349
 Merge branch 'master' into entities
 
 ENDOFOUTPUTGITMESSAGEHERE
 43      0       CHANGELOG.txt

Only if -m is passed will it output individual numstats against all parent commits:

$ git show -m --numstat --summary --pretty=format:"%H%n%P%n%aN <%ae>%n%cN <%ce>%n%ct%n%s%n%b%nENDOFOUTPUTGITMESSAGEHERE" 32d5d
 32d5d3d0ab8eabb812fedef73f6dd1f4e74c288b
 e111ad0f0165c1b85def0dc89005c36be420793d 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0
 Sam Boyer <drupal@samboyer.org>
 Sam Boyer <drupal@samboyer.org>
 1281398349
 Merge branch 'master' into entities
 
 ENDOFOUTPUTGITMESSAGEHERE
 43      0       CHANGELOG.txt
 
 32d5d3d0ab8eabb812fedef73f6dd1f4e74c288b
 e111ad0f0165c1b85def0dc89005c36be420793d 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0
 Sam Boyer <drupal@samboyer.org>
 Sam Boyer <drupal@samboyer.org>
 1281398349
 Merge branch 'master' into entities
 
 ENDOFOUTPUTGITMESSAGEHERE
 7       0       CHANGELOG.txt
 1       16      includes/VersioncontrolAccount.php
 40      17      includes/VersioncontrolBackend.php
 111     6       includes/VersioncontrolBranch.php
 5       23      includes/VersioncontrolItem.php
 12      26      includes/VersioncontrolLabel.php
 11      30      includes/VersioncontrolOperation.php
 64      139     includes/VersioncontrolRepository.php
 79      6       includes/VersioncontrolTag.php
 595     0       includes/controllers.inc
 1       1       versioncontrol.admin.inc
 1       0       versioncontrol.info
 26      1       versioncontrol.install
 79      71      versioncontrol.module
 7       6       versioncontrol_fakevcs/includes/classes.inc
  create mode 100644 includes/controllers.inc

The sum of these two outputs shows the true, full effect of the merge relative to the best common ancestor/merge base (see man git-merge-base to understand what that is). The first output are the changes introduces on 'master', the second the changes introduced on 'entities'. And for merge commits which require some manual resolution...well, I haven't even tested that yet. And it makes my head hurt.

There's another problem here, too - each output doesn't actually identify which parent its diffing against. As far as I can tell they're output in the reverse of the order in which the parent commits appear in the second line (so 46fbcc first, then e111ad), but I don't know how well that holds up, or how it changes in the case of an octopus. Annoyingly, if you use one of the built-in formats like --pretty=raw, the parent is explicitly identified in each output:

 commit 32d5d3d0ab8eabb812fedef73f6dd1f4e74c288b (from e111ad0f0165c1b85def0dc89005c36be420793d)
 tree fa0a8e2ffde1dc9637ad024f8f0e3ddfc9f0f597
 parent e111ad0f0165c1b85def0dc89005c36be420793d
 parent 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0
 author Sam Boyer <drupal@samboyer.org> 1281398349 -0700
 committer Sam Boyer <drupal@samboyer.org> 1281398349 -0700
 
     Merge branch 'master' into entities
 
 43      0       CHANGELOG.txt
 
 commit 32d5d3d0ab8eabb812fedef73f6dd1f4e74c288b (from 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0)
 tree fa0a8e2ffde1dc9637ad024f8f0e3ddfc9f0f597
 parent e111ad0f0165c1b85def0dc89005c36be420793d
 parent 46fbcc8c816f73170e53c4b7cac2565d7c26b6e0
 author Sam Boyer <drupal@samboyer.org> 1281398349 -0700
 committer Sam Boyer <drupal@samboyer.org> 1281398349 -0700
 
     Merge branch 'master' into entities
 
 7       0       CHANGELOG.txt
 1       16      includes/VersioncontrolAccount.php
 40      17      includes/VersioncontrolBackend.php
 111     6       includes/VersioncontrolBranch.php
 5       23      includes/VersioncontrolItem.php
 12      26      includes/VersioncontrolLabel.php
 11      30      includes/VersioncontrolOperation.php
 64      139     includes/VersioncontrolRepository.php
 79      6       includes/VersioncontrolTag.php
 595     0       includes/controllers.inc
 1       1       versioncontrol.admin.inc
 1       0       versioncontrol.info
 26      1       versioncontrol.install
 79      71      versioncontrol.module
 7       6       versioncontrol_fakevcs/includes/classes.inc
  create mode 100644 includes/controllers.inc

Note the (from <commit id>) that appears on the first line of each output. I've even looked through the git source files, and as far as I can tell, there's no way to simulate that output with a custom --pretty=format.

A lot of this may end up being moot. But I want it all up here and documented in public, anyway, so that there's a clear launching point for discussion.

Comments

sdboyer’s picture

sdboyer commented
Title: Decide on a definitive output format for git-log/show » Decide on a definitive parseable output format for git-log/show

Slightly better title.

sdboyer’s picture

sdboyer commented
Priority: Critical » Normal

Gonna take this back to normal for now.

marvil07’s picture

marvil07 commented

Really good catch about merges(aka I need to open my eyes a little more).

All the mentioned points are IMHO really related with the decision to use our own functions to implement git interaction. I mean, we are implementing php for _our_ interaction, so it will be always limited.

This can be better if we use a general php wrapper for git. The problem is that after searching a little, I end up finding no general propose php api for git(like git-pyhon for python), in other to make git interaction cleaner.

So, maybe it would be better to focus on writing a general propose php api for git. After reading some of the code in the mentioned links I think git-python could be a really good start to make our own gitlib for php.

In the worst case, we just modify what we have, trying to keep that code independent from drupal APIs, as we were trying I think until someone start a decent php wrapper for git.

PS: I know I am almost out of the scope of this issue, but I needed to write this somewhere :-p

sdboyer’s picture

sdboyer commented

Heh, I don't actually see the strongest link between my original concern and the question of a wrapper for git interaction, but you know I don't disagree. Though...yeah, I guess I can see the connection. However, there is a baseline to follow - the one I wrote for svn, which I wrote specifically in preparation for writing just such a thing for git: http://github.com/sdboyer/svnlib . Between that and git-python, we should have everything we need, in terms of a basis for work.

sdboyer’s picture

sdboyer commented
Priority: Normal » Critical

Marking critical.

marvil07’s picture

marvil07 commented
Priority: Critical » Major

An update after #965930: Get the log parser finished. Really. got in:

About dates

Agree, we need to make sure timestamps are stored with the right timezones(not really sure if this is necessary since IIRC timestamp means "seconds since epoch", so seems to be a fixed time), but yep, make sure about it.

About delimiter

Not really sure if this is important now, maybe we should wait to #973890: Create a reposync plugin using libgit2 php binding, when anyway we are dropping all that code in favour of probably direct function/methods to get right data.

Merge data

After #975864: Merge {versioncontrol_item_revisions} and {versioncontrol_source_items} tables versioncontrol module is not anymore tracking merges referring item revisions to many source items(now only one), it is only flagging the item revision as VERSIONCONTROL_ACTION_MERGED if the revision have more than one parent. So, unless we start supporting merges on versioncontrol_git IMO this is not anymore needed.

PS: Maybe it would be better to split this in more specific issues, when we decide what to change.
PS2: changing to major, since log parser is working as expected for now, naturally some of these can be bugs, so leaving as major.

marvil07’s picture

marvil07 commented
Component: Code » Git interaction
sdboyer’s picture

sdboyer commented
Status: Active » Closed (won't fix)

This was me being too twitchy.