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

-Xignore-all-space not respected #13

Open
borisbrodski opened this issue Dec 12, 2018 · 15 comments
Open

-Xignore-all-space not respected #13

borisbrodski opened this issue Dec 12, 2018 · 15 comments

Comments

@borisbrodski
Copy link

First of all, thank you for this awesome project!

We are using it for a quite a bit. It works flowerless with the exception of just one issue:

If we merge with

$ git merge -Xignore-all-space other-branch

the pom.xml files, that get processed my the pomutils merge engine get conflicts,
if they contain differences in space/tab indentation.

Put it simple, the -Xignore-all-space git merge flag doesn't get respected for the files, on which pomutils was involved.

  • Are there a trick to avoid it?

I'm looking into implementing an additional switch, like --expandtab 4, that will convert all tabs into 4 spaces in both files before exiting.

  • Do you have better suggestions?
  • Are you interested in a push request?
@cecom
Copy link
Owner

cecom commented Dec 12, 2018

Hi,

i had a quick look and it is not that easy. We only adjust the pom version value element, regardless of the position on the line.

Example:
branch1: <version>1.0</version>
branch2: ....<version>2.0</version> (where dots are spaces)

If you do a branch 2 --> branch1 merge, the result (before the real merge happens) will be:

  • if "our" strategie is used <version>1.0</version>
  • if "their" strategie is used <version>2.0</version>

If you do a branch 1 --> branch2 merge, the result (before the real merge happens) will be:

  • if "our" strategie is used ....<version>2.0</version> (where dots are spaces)
  • if "their" strategie is used ....<version>1.0</version> (where dots are spaces)

After we adjusted the value, we call the git merge driver git merge-file ... and this does not have any options like ignore-all-space.

I need to think about it a little bit more, if there is a solution....

@borisbrodski
Copy link
Author

Thank you for the quick response. I also couldn't find a simple solution to the missing --ignore-all-space switch. One complicated solution may be:

  • create a new temporary directory and git init there
  • Add base-file to a repo and commit it
  • Create branch1, add current-file and commit it
  • Create branch2 on the base commit, add other-file and commit it
  • Issue git merge branch1 -Xignore-all-space
  • Grab the merged file
  • Remove temporery directory

But if you would consider a simpler problem, like just confused tabs and spaces, the solution may be also much simpler. For example:

  • branch1: \t<version>1.0</version> (where \t is the tab)
  • branch2: ....<version>2.0</version> (where dots are spaces)

you may just come away with the simple TAB -> SPACE replacement. I already tested it by adding

  • --expandtab=<count-of-spaces>

option to the pomutils, that just replaces all TABs in

  • current
  • base
  • other

by 4 spaces (in my example). This solves my current problem. Of course, if TABs doesn't exactly match SPACES, this will fail as you have described.

@cecom
Copy link
Owner

cecom commented Dec 12, 2018

The problem on your first solution is, that would happen for all poms. So if you have a multi module project with 100 of poms that could take a while, because it would be done for every single pom.

Your Second solution does not work, because i dont have access to the real pom file. I manipulate the file via the maven-version-plugin classes and they dont provide any access to the real file.

Update: For your second solution, i only want to update the line where the version "problem" exists. But you meant to replace all \t in the complete file, correct?

@borisbrodski
Copy link
Author

Yes, I replace all \t into spaces. This already works here. Do you find it wrong to replace all \t ?

And yes, the performance will be definitely the problem. On the other hand, solving conflicts manually is always longer, that any automated procedure. Also I would not use this first complex solution by default.

@cecom
Copy link
Owner

cecom commented Dec 12, 2018

If there is no solution on your side (get a xml formatter for every developer so the pom.xml file is always correct formatted), then the way i would go would be:

  • add a property ala "preFormatXml" which defaults to "false"
  • if true, before we do anyhing with the pom, we format the complete pom with the formatter, so everything is alined after a template
  • after that we do our merge stuff.

That would solve \t and space issues. But im a litte bit unsure. There could also be some formatting changes in your pom, which you dont want...

@borisbrodski
Copy link
Author

Thank you!

This is working great so far. Instead of full format, I am just replacing tabs with spaces.

@cecom
Copy link
Owner

cecom commented Feb 27, 2019

so you changed your developers format rules and the problem does not exist anymore? So we can close this issue?

@borisbrodski
Copy link
Author

No, I extended pomutils to simply replace all tabs with spaces before passing files to the git merge.

@cecom
Copy link
Owner

cecom commented Feb 27, 2019

Ok, than send me a pull request and i will have a look. Plz add a test too. Have a look at my other tests, its easy to add them.

@cecom
Copy link
Owner

cecom commented Apr 23, 2020

@borisbrodski are you working on it anymore? Or can i close this ticket?

@borisbrodski
Copy link
Author

Thank you for reminding me of this issue. I will add some tests and create a pull request.

borisbrodski added a commit to borisbrodski/pomutils that referenced this issue Apr 28, 2020
Speficy

    --expandtab={count-of-space}

to replace all indentation tabs with specified amount of spaces
prior to the git merge.

Note: this is just a subset of the git's -Xignore-all-space
functionality. Misalignment after the tab replacement will still result in the
merge conflicts.

Closes: cecom#13
@borisbrodski
Copy link
Author

Please, check the pull request :)

@cecom
Copy link
Owner

cecom commented Jun 14, 2020

Hi Boris,

i had a look. What do you think, if i would change it to "Tabify" and "Untabify"? So we can get both worlds? The one who uses TABs can use the tabify and one who uses Spaces uses the Untabify. I would take your Code and change it a little bit :-)

@borisbrodski
Copy link
Author

Hi Cenom,

sure, go for the change. But please, accept pull request first and then make additional commit with your change. It allow me to merge back your change quickly (in my branch) and it will also preserve the authorship of the feature ;)

Thank you!

@cecom
Copy link
Owner

cecom commented Jun 14, 2020

For sure. I will take your branch and work on it.

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