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

Process all parent commits in a merge commit, not only the 1st one? #346

Open
dimikot opened this issue Oct 24, 2024 · 5 comments
Open

Process all parent commits in a merge commit, not only the 1st one? #346

dimikot opened this issue Oct 24, 2024 · 5 comments

Comments

@dimikot
Copy link

dimikot commented Oct 24, 2024

Hi.

Can the action check not only the 1st parent of a merge commit for changed files, but ALL parents, searching for the one which has no changed files against, and for which the workflow passed?

Here is an illustration:

We use Merge button, and it produces merge commits (like m4). Each merge commit (m4 on the picture) has not one, but 2 parents: one on the main branch itself (m1) and another one on the feature branch (c3). We can see on the picture that the code in c3 will be identical to the code on m4, and since the workflow passes for c3, it can safely be skipped for m4 too.

In fact, the action behaves like this OFTEN, but sometimes, it still doesn't recognize the situation: instead of comparing the code between c3 and m4, it compares it between m1 and m4, which effectively disables skipping.

Here is likely where it is in the code:

iterSha = commit.parents?.length ? commit.parents[0]?.sha : null

I see that it only chooses the 1st parent in the list of parents. But for merge commits (like m4 on the picture above), there are 2 parents, and they BOTH are worth checking.

I think that GitHub API returns parents of a merge commit in an unpredictable order, so sometimes it guesses out the correct parent, and sometimes it does not.

@fkirc
Copy link
Owner

fkirc commented Oct 24, 2024

Hi,

One of the core requirements of skip-duplicate-actions is that it needs to make skip-decisions rapidly, it should not take more than two or three seconds, regardless of how big the commit-tree is.

That being said, if you submit a PR that traces multiple parents, I would definitely consider merging it if the code is not more complex than the rest of the code.

@dimikot
Copy link
Author

dimikot commented Oct 25, 2024

Theoretically, if we run both parents checks in parallel, that should not slow down things. And there is unlikely more than 2 parents (merges with Merge button press in GitHub UI are common, and they produce exactly 2 parents).

@fkirc
Copy link
Owner

fkirc commented Oct 25, 2024

I agree that it could be implemented efficiently, so the only question remaining is whether a PR could implement it without too much added complexity.

@dimikot
Copy link
Author

dimikot commented Oct 27, 2024

@fkirc I think there is actually a simpler way, without rechecking all parents, by still checking only one of them. If we order parent commits by their timestamps (the timestamp when they appeared in a branch), and then just check the most recent one, that should be fully enough to replicate the effect. At least, with a very high probability for sure.

@fkirc
Copy link
Owner

fkirc commented Oct 27, 2024

Makes sense, if we always choose the most recent parent-commit, then the backtracking-algorithm would remain basically unchanged

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

No branches or pull requests

2 participants