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

Update import merge tool #271

Merged
merged 90 commits into from
Jun 6, 2024
Merged

Update import merge tool #271

merged 90 commits into from
Jun 6, 2024

Conversation

benedikt-schesch
Copy link
Owner

No description provided.

@benedikt-schesch
Copy link
Owner Author

@mernst Any clue why the merge tool is crashing in the CI/CD pipeline?

@benedikt-schesch
Copy link
Owner Author

@mernst The number of incorrect merges is double the previous amount.

@mernst
Copy link
Collaborator

mernst commented May 13, 2024

OK, I will investigate.

Copy link
Collaborator

@mernst mernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good, but I have a couple of concerns; please see comments.

retVal=$?

# report conflicts
if [ "$retVal" -ne 0 ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any tool that involves "imports" needs to run regardless of the return value of git merge.

Copy link
Owner Author

@benedikt-schesch benedikt-schesch May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mernst Is it fine in the current version where I just always call it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The default version runs annotations, imports, and version-numbers.
(Defaults are listed at https://github.com/plume-lib/merging/#features .)

# Check if there are still conflicts
diffs=$(git diff --name-only --diff-filter=U)
if [ -z "$diffs" ]; then
git commit -m "Resolved conflicts"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that there was already a merge commit created by git merge. In that case, this should probably amend the merge commit (or maybe create a new, additional commit).
It may be necessary to run git add, or add a . argument to git commit.

Copy link
Collaborator

@mernst mernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks!

@@ -0,0 +1,50 @@
#!/usr/bin/env sh

# usage: ./gitmerge_ort_imports_ignorespace.sh <clone_dir> <branch-1> <branch-2> <git_strategy> <merge_strategy>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong usage message, it refers to gitmerge_ort_imports_ignorespace.sh.


echo "git_merge_plumelib: Merging $branch1 and $branch2 with git_strategy=$git_strategy and merge_strategy=$merge_strategy"

# shellcheck disable=SC2153 # "JAVA17_HOME is not a misspelling of "JAVA_HOME"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing " after JAVA17_HOME.

@benedikt-schesch benedikt-schesch force-pushed the update-import branch 2 times, most recently from fc54c17 to 03f8e15 Compare June 5, 2024 23:42
@benedikt-schesch benedikt-schesch merged commit 1ebf695 into main Jun 6, 2024
2 of 8 checks passed
@benedikt-schesch benedikt-schesch deleted the update-import branch June 6, 2024 17:24
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 this pull request may close these issues.

2 participants