allow to fail atime job in GHCI / use merge commits to allow branch deletion

[jangorecki]

You can try using the continue-on-error directive at the job level. Maybe base it off labels (then apply that label to this PR) since using pull_request.number would be redundant; e.g. say we name the label 'ignore-atime-failure':

continue-on-error: ${{ contains(github.event.pull_request.labels.*.name, 'ignore-atime-failure') }}

Originally posted by @Anirban166 in #7361 (comment)

[tdhock]

I'm not sure this is a good idea.
If the atime job is failing there is probably a reason that needs fixing. for example I see this PR https://github.com/Rdatatable/data.table/actions/runs/18464808691/job/52604142621?pr=7361 has atime error below,

Error in value[[3L]](cond) : 
  Error in revparse_single(object, branch): Error in 'git2r_revparse_single': Requested object could not be found

 when trying to checkout ffe431fbc1fe2d52ed9499f78e7e16eae4d71a93
Calls: <Anonymous> ... tryCatch -> tryCatchList -> tryCatchOne -> <Anonymous>
Timing stopped at: 16.42 3.21 17.99
Execution halted

this means that somebody deleted a branch with a commit required in an atime test.
Searching for "ffe43" in https://github.com/Rdatatable/data.table/blob/master/.ci/atime/tests.R#L47 shows this line

  Fast = "ffe431fbc1fe2d52ed9499f78e7e16eae4d71a93" # Last commit of the PR (https://github.com/Rdatatable/data.table/pull/4386/commits) where the performance was improved.

which has a reference to the PR #4386 that had the deleted branch. The fix which I just did was going to that PR page and clicking "Restore branch" button at the bottom.

[jangorecki]

That's a fair point, how should we know if a branch is safe to delete?

[MichaelChirico]

I have been trying to use the atime label to mark branches as required for the GHA

[jangorecki]

Ok, does that mean we should keep those branches forever? This feels suboptimal. I know many users who do git fetch origin rather than specific branch and it pollutes their git checkout experience... That why I cleaned up old merged branches recently.

[tdhock]

yes with the current setup we have to keep old branches with atime commits forever.

[MichaelChirico]

I agree it would be better but I think the benefit of {atime} outweighs this cost for now. Open to ideas for how else to make it work.

[jangorecki]

Instead of pointing to a commit from a branch atime could point to a merge commit. That will be always available in master.
That seems to be the optimal way.

Alternatively we could have a fork that keeps all branches needed for atime.

[tdhock]

using merge commits sounds like a good idea to me.

[MichaelChirico]

My memory is that doesn't work somehow, but I could be wrong, please test it out.

[ben-schwen]

My memory is that doesn't work somehow, but I could be wrong, please test it out.

Used this #7480 resp. #7482 and it works like a charm. I think we can document this and then close here!

[MichaelChirico]

I guess the downside of this approach is the atime test is always added in a separate PR, which requires splitting it off before merge:

• file PR including performance regression test
• upon PR approval, split the atime portion off to its own PR, revert in the current PR

it would be nice to keep the performance test tightly coupled with the code it's intended for, but I think on balance the current approach is preferred to keeping the old branches around indefinitely.

[ben-schwen]

The third option we have, is to squash manually, but this seems more error prone :/