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

[DRAFT][clang-format] helper script for resolving downstream reformat… #80462

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pogo59
Copy link
Collaborator

@pogo59 pogo59 commented Feb 2, 2024

… conflicts

Copy link
Contributor

@brunodf-snps brunodf-snps left a comment

Choose a reason for hiding this comment

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

It is very close to the libcxx script, but implemented for explicitly running via git mergetool rather than automatically running on every merge. The idea is that if you get a conflict after merging a reformat, you run git mergetool and it will fix the conflicts for you. It takes an extra manual step, but doesn’t impose a cost on every merge forever.

Back for the libcxx script, I also remarked that I would prefer to run this kind of script in a manual step, but I did not know how.

Configuring the script as a mergetool seems like a great idea, but when testing this I notice some problems. When the conflicts are only due to formatting, I think it works as intended. But when there are other (actual) conflicts too, then these conflicts are silently marked as resolved (although the conflict markers remain in the file). As a user, I don't see there are conflicts (left) and it is very easy to make the mistake to check in the conflict markers.

I also tried a tweaked mergetool configuration, where I add the trustExitCode setting. Then git-mergetool detects from the exit code of git-merge-file that the merge is not complete, but it seems to restore everything in its original state: I'm left to resolve all the conflicts in the file, both those from formatting, and the actual one(s). Essentially, the script is no help then.

I think that git-mergetool expects that the tool (e.g. vimdiff) through user interaction will resolve the conflict completely (or abort). This works differently in the configuration as a merge driver: this can resolve the conflicts due to reformatting, and leave you with the other ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

This script should have mode 0755 (executable) instead of 0644 to be able to invoke it with the documented command-line.

@pogo59
Copy link
Collaborator Author

pogo59 commented Feb 6, 2024

Thanks for the testing! I tried this on our downstream repo but only with purely reformatting changes, not with a commit that also had non-reformatting changes. I will do more experimenting with a conflict that has a non-reformatting conflict as well.

If I can't get that to work in the mergetool context, I believe it would be possible to write a fully standalone script that DTRT, it will just be tedious to extract the 3 versions of the file that git-mergetool hands you for free.

@pogo59
Copy link
Collaborator Author

pogo59 commented Feb 6, 2024

I wasn't able to make mergetool preserve the partially-resolved file, so instead of driving it through mergetool it's now a standalone script.

@pogo59
Copy link
Collaborator Author

pogo59 commented Feb 6, 2024

Arguably should be in Python for portability, but I expect it will take me multiple days to translate. ;)

Comment on lines +51 to +53
git show "$BASE:$file" | $CLANG_FORMAT --style=file --assume-filename="$file" > "$MYTEMP/base"
git show "$OTHER:$file" | $CLANG_FORMAT --style=file --assume-filename="$file" > "$MYTEMP/other"
git show "$CURRENT:$file" | $CLANG_FORMAT --style=file --assume-filename="$file" > "$MYTEMP/current"
Copy link
Contributor

Choose a reason for hiding this comment

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

The technique of figuring out $BASE/$OTHER/$CURRENT commits and using them here is fragile (because likely incomplete) and also unneeded because git already has these 3 versions of the file in its index for an unresolved conflict.

To use the versions from the git index, I can suggest either this (syntax explained here):

git show ":1:$file" | $CLANG_FORMAT ... >"$MYTEMP/base"
git show ":2:$file" | $CLANG_FORMAT ... >"$MYTEMP/current"
git show ":3:$file" | $CLANG_FORMAT ... >"$MYTEMP/other"

Or this (explained here):

read -r basetmp currenttmp othertmp rem < <(git checkout-index --stage=all "$file")
$CLANG_FORMAT ... -i "$basetmp"
$CLANG_FORMAT ... -i "$currenttmp"
$CLANG_FORMAT ... -i "$othertmp"

In both versions, all the logic for figuring out $BASE/$OTHER/$CURRENT can be dropped. In the second version, git creates suitable temp files, so you don't need $MYTEMP (but you would still be responsible to cleanup the temp files).

Comment on lines +75 to +79
git status --porcelain | grep '^UU ' | cut -c 4- | \
while read -r file
do
resolve_one_file "$file"
done
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only work if you invoke the script from the top-level directory of the working copy? I think you can in any case get a valid filename by prefixing it with the output git rev-parse --show-cdup. But be sure to check if that is sufficient for the other commands inside resolve_one_file. (If too difficult, the script could error out when not in the top-level.)

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.

None yet

2 participants