push vs PR commit

[yarikoptic]

I had mentioned that I had only github actions in one folder and apparently travis + appveyor gets sorted out into another:

(tinuous-dev) datalad@smaug:/mnt/datasets/datalad/ci/logs/2021/05$ ls 24/pr/5693/*
24/pr/5693/5a27faf:
appveyor-6046-failed  appveyor-6046-success  travis-11955-success

24/pr/5693/d524cd9:
 github-Benchmarks-3127-success   github-CrippledFS-1683-success   github-Docs-3127-success   github-Extensions-3127-success   github-Linters-103-success  'github-Shellcheck on scripts-1314-success'  'github-Test on macOS-1364-success'

because:

(git)lena:~datalad/datalad-master[nf-foreach]git
$> git show d524cd9    
commit d524cd9c8af5a57192999a34647bfb970bd0e80e (refs/pull/origin/5693/head)
Author: Kyle Meyer <kyle@kyleam.com>
Date:   Mon May 24 09:24:43 2021 -0400

    TST: test_fileinfo: Be more specific about status() return value
    
    Re: https://github.com/datalad/datalad/pull/5693#pullrequestreview-666722514

....
$> git show 5a27faf    
commit 5a27fafa8616eb6ab62f6b2ff6b67c6ff39ffb71 (refs/pull/origin/5693/merge)
Merge: f63ed567d d524cd9c8
Author: Kyle Meyer <kyle@kyleam.com>
Date:   Mon May 24 09:28:35 2021 -0400

    Merge d524cd9c8af5a57192999a34647bfb970bd0e80e into f63ed567dbc79cc8b22a51b7e00f9fb2419dcb66

so 5a27faf is the merge of that PR commit into corresponding branch.

For the purpose of tinuous I would have liked to have all of those under the same commit/directory for the PR or it would be hard to impossible to bring them together into coherent collection of the CI logs for a given state of the PR. I wonder if that is possible?! is the original commit of the PR branch returned by travis and appveyor? then may be we could have refcommit and refcommit_abbrev or alike.

[jwodder]

@yarikoptic Appveyor's build objects have a pullRequestHeadCommitId field that contains the hash of the commit on the PR branch, but there does not appear to be an equivalent field for Travis.

[yarikoptic]

sad, could you please file and issue with travis? (I am still hopeful to look forward to just have those resolved instead of us coming up with workarounds)

[jwodder]

@yarikoptic Support ticket filed.

[yarikoptic]

one possible workaround for this and #42 (commit time) could be: establish a local clone of the repository with the fetch of all PRs refs, as I mentioned in #42 (comment) . It would unlikely work for historical PRs.

[yarikoptic]

I wonder if a lightweight solution could be to just get the corresponding PR pull/head refs, e.g. like

curl -H "accept: application/json" https://github.com/datalad/datalad/refs/head/101 

(but this doesn't work... just trying to mimic on what git is likely to be doing, may be agent needs to be overriden) then we could mate both commits (PR head and merge). Another strategy could be to take information about parent commit from a CI which supports it before considering a CI which doesn't

[yarikoptic]

Please choose any path you like @jwodder to go forward on this one.

IMHO local cached clone might be best since would allow for other interrogations and might help reduce needed amount of API calls -- so we could interrogate clone first (e.g. for push-vs-pr commit , or commit date, or git describe etc) and then resort to API or other calls where possible, and then just say 'None' if no way to figure out.

for this specific issue I think we should use push commit as the commit (and None if not known) and then add prcommit with the same logic of possibly None if unknown or not relevant. Then I would try {commit_abbrev}-{prcommit_abbrev} in my config. I expect to be able to group all CIs by {commit_abbrev} while still seeing difference whenever it would be for different merges (prcommits).

[jwodder]

@yarikoptic I don't think a local clone would work. Aren't the merge commits something created on the fly by Travis rather than something that actually exists in the repo? How did 5a27faf end up in the datalad repo?

Also, getting the PR head from the GitHub API wouldn't work, as it could have changed since the build was run.

[jwodder]

@yarikoptic I think merge_commit would be a clearer name than prcommit (assuming that you want it to be the commit that the build is run on for merged builds).

[jwodder]

@yarikoptic Do we want to add a field with the same meaning as {commit} currently has? That seems like it might be useful.

[yarikoptic]

@yarikoptic I don't think a local clone would work. Aren't the merge commits something created on the fly by Travis rather than something that actually exists in the repo? How did 5a27faf end up in the datalad repo?

we fetch all those merges too, see #42 (comment) referenced above.

Also, getting the PR head from the GitHub API wouldn't work, as it could have changed since the build was run.

yes. For those - None since indeed nothing we can do about (wilder alternative would be to figure out original PR clone url and add as a remote, but I am ok to not pursue it that far). I expect it would be ok in > 90% of cases . Also... got me thinking -- there might be a way to request remote to fetch a specific object/commit -- please check if possible. Then it could be one extra way to get commit until it eventually GCed

@yarikoptic I think merge_commit would be a clearer name than prcommit (assuming that you want it to be the commit that the build is run on for merged builds).

by itself it is a bit misleading. pull_merge_commit would be descriptive but way too long... but ok -- let's go with merge_commit. I might now like it over prcommit ;)

@yarikoptic Do we want to add a field with the same meaning as {commit} currently has? That seems like it might be useful.

sorry, not following -- I think we are already talking about adding a new field {merge_commit} with the meaning similar (not exactly the same).

Re _abbrev -- let me file a dedicated issue. I think we could/should refactor that to avoid exponential composition here.

[jwodder]

@yarikoptic

sorry, not following -- I think we are already talking about adding a new field {merge_commit} with the meaning similar (not exactly the same).

You said that "we should use push commit as the commit (and None if not known)", which I took to mean that, for Travis PR builds, {commit} would be None instead of the merge commit that the build was performed on. This is a change to the meaning of {commit}, and I can envision some users desiring a placeholder with the current semantics of "the actual commit that the CI ran on, regardless of how it was created."