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

Handle exit code when calling resolve-import-conflicts #217

Closed
wants to merge 9 commits into from

Conversation

mernst
Copy link
Collaborator

@mernst mernst commented Sep 25, 2023

No description provided.

@benedikt-schesch
Copy link
Owner

@mernst You don't think it's better like this in one script?

@benedikt-schesch
Copy link
Owner

@mernst I am not a fan of the structured you proposed with calling another script, it creates too much dependencies between the scripts and too much calling around in my opinion. I looked at the script and made the return values explicit because it makes it easier to understand what we are trying to do here. For me in the current way it is a lot more intuitive.
I have a problem with gitmerge_resolve.sh, the scripts only seems to remove some conflict markers but the exit statement seems to preserve the old return value so in the end it will still be marked as a merge failure, thus this script is not really doing anything since it won't be tested due to merge conflict. I am not 100% sure what the desired behavior is here.
That is why I think making the return values explicit is important.

@mernst
Copy link
Collaborator Author

mernst commented Sep 26, 2023

I was trying to leave AST-Merging-Evaluation functionality (like printing "Conflict" and running merge commit --abort out of resolve-import-conflicts which is independently useful and exists at https://github.com/plume-lib/git-scripts. We might eventually want to use the version from there. In the meanwhile, see #218, which is a better alternative to this pull request.


MERGE_SCRIPTS_DIR="$(cd "$(dirname "$0")" && pwd -P)"
clone_dir=$1
branch1=$2
branch2=$3
strategy="-s ort"
"$MERGE_SCRIPTS_DIR"/gitmerge.sh "$clone_dir" "$branch1" "$branch2" "$strategy"
if ! "$MERGE_SCRIPTS_DIR"/gitmerge.sh "$clone_dir" "$branch1" "$branch2" "$strategy"; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@benedikt-schesch Why is this necessary? The script should exit with the same status as the last command in it, so this if construct seems unnecessary (throughout).

@benedikt-schesch
Copy link
Owner

#218 Is better

@mernst mernst deleted the v2-update-resolve-import-conflicts-status branch October 1, 2023 20:00
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