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

Base ref for new branches #33

Closed
paulius-p opened this issue Aug 21, 2020 · 7 comments · Fixed by #40
Closed

Base ref for new branches #33

paulius-p opened this issue Aug 21, 2020 · 7 comments · Fixed by #40

Comments

@paulius-p
Copy link

paulius-p commented Aug 21, 2020

When having a bigger codebase, which has PR merges quite frequently, the default setting for base parameter with master branch defeats the purpose of this action a bit, as the diff will include also the new changes from master merged after you've checkout your branch.
I've tried setting it to base: ${{ github.ref }}, which seems to work better as it's checks the diff with the last commit, but the problem is when you push your branch the first time. Then there's no commit to diff with, so in this case making base as master makes sense, just not sure how to have this condition in the workflow.

Maybe someone would have some suggestions?

Edit: or maybe good solution would be to use the default master base, but the diff would only check new changes from the branch, but not what the branch is missing from master as these changes are irrelevant and won't affect the build. Using something like git diff --name-only <ref>...HEAD

@dorny
Copy link
Owner

dorny commented Aug 21, 2020

I see your point. It will require some changes in the code. I will look into it as I get back from vacation next week.

@dorny
Copy link
Owner

dorny commented Aug 30, 2020

Thanks, for opening this issue. Indeed the desired behavior is what you proposed: git diff --name-status <ref>...HEAD. To make it work it's needed to fetch history of the two branches at least to the point it's possible to find their merge-base. As I tried to remove the need of fetching the whole history, I didn't realize consequences of using diff <ref> HEAD instead of diff <ref>...HEAD.

I will fix it soon.

@dorny
Copy link
Owner

dorny commented Sep 1, 2020

Fix is available on develop.
This was the last implementation change I planned to do before next release.
However I still have to rework the docs, so it will take couple of days until its out.

@dorny
Copy link
Owner

dorny commented Sep 14, 2020

Fixed in release v2.3.0

@dorny dorny closed this as completed Sep 14, 2020
@paulius-p
Copy link
Author

Hey @dorny, thanks for the update! It solves the use case when the base is your merge base and that's what I'm going to use, but for this condition:

 # All files are considered as added if there is no common ancestor with
 # base branch or no previous commit.

when the base is your current branch and there's no previous commit (new branch) I think it should default to merge base and not output that all files have changed.

@dorny dorny reopened this Sep 16, 2020
@dorny
Copy link
Owner

dorny commented Sep 16, 2020

The base input could accept multiple refs delimited by comma. Then it would try to use previous commit or find-merge base. If there is none, it would try to do it against next ref in the list. Marking all files as added would be the last fallback option.

For example: base: ${{ github.ref }},develop
Repository default branch would be included as last option by default, without specifying it.
So it should work for everybody without modification of their workflow files.

Any thoughts on this solution?

@paulius-p
Copy link
Author

Any thoughts on this solution?

@dorny seems reasonable to me. I can't think of a use case where anyone using this action would want all the files included as changed, so having these sane default fallbacks should be the way to go. 👍

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

Successfully merging a pull request may close this issue.

2 participants