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

Idea: explicitly look for merge conflict markers #23

Open
madig opened this issue Oct 23, 2018 · 27 comments
Open

Idea: explicitly look for merge conflict markers #23

madig opened this issue Oct 23, 2018 · 27 comments

Comments

@madig
Copy link
Contributor

madig commented Oct 23, 2018

Happens every so often. They are caught anyway because they break the XML, but the error messages may be cryptic.

Maybe adapt https://github.com/pre-commit/pre-commit-hooks/blob/master/pre_commit_hooks/check_merge_conflict.py to ufolint.

@anthrotype
Copy link

why not just use that?

@madig
Copy link
Contributor Author

madig commented Oct 23, 2018

Possible, but introduces pre-commit. Then you need to go and set up caches for it because the setup takes a minute or more...

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 23, 2018

Read every UFO text file and test for these markers? Or are they happening at a significant frequency in specific file area so that we can narrow the list?

@anthrotype
Copy link

pre-commit is primarily for local development. you install it once then it does its thing.
anyway, I don't use ufolint, you do what you need to do

@anthrotype
Copy link

more interesting if you turned ufolint into a pre-commit hook (ok now I shut up 😁)

@chrissimpkins
Copy link
Member

no that's an interesting idea Cosimo. Details?

@anthrotype
Copy link

https://pre-commit.com/
https://github.com/pre-commit/pre-commit
https://github.com/pre-commit/pre-commit-hooks

@chrissimpkins
Copy link
Member

@madig what are you seeing in the error message when this happens?

@anthrotype
Copy link

a pre-commit hook can be any script that exits with nonzero on failure; it can also modify files in the working directory (e.g. see black)

@chrissimpkins
Copy link
Member

Problem is getting contributors to set this up... :)

@anthrotype
Copy link

anthrotype commented Oct 23, 2018

i'm sure Nikolaus is already concocting something 😉

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

Problem is getting contributors to set this up... :)

This is a sticking point. I'm using a build tool wrapper internally at work but not everyone uses it, and people struggle with Git as is. I set up ufolint on the CI for two projects today, but the designers were very concerned about false positives preventing them from building the fonts on the CI (font developers are under constant time pressure), so I allow the job to fail and proceed to building anyway. Setting this up as a pre-commit hook would make little sense like that. We'll see.

what are you seeing in the error message when this happens?

test.ufo.zip

[...]
[FAIL] a test failed with error: not well-formed (invalid token): line 6, column 1
[...]

@chrissimpkins
Copy link
Member

  1. ufolint does not have false positives! Damn Brits. No wonder we forked America. :)
  2. why can't they build locally with their changes in order to proof the work that they've done? Is this where the merge conflicts are occurring as they attempt to pull down work from other designers in different branches so that they can see everyone's changes in a given day?
  3. UFO merges are headache inducing. I am looking forward to seeing your UFO merge tool... At the moment I don't let anyone commit anything other than *.glif files in a collaborative workflow. They either add the rest of the data in the UFO source to .gitignore or they .developerignore it by not hitting commit on anything other than a *.glif. This places the burden of glyph additions, removals, etc on the maintainer which is no fun but otherwise the source becomes a mess as the editors rearrange glyph name lists, add library metadata, etc. And we are not dealing with kerning, OT features, and the like. I can't even imagine.

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

  1. They hesitate with changes to the workflow due to the aforementioned pressure ;)
  2. Some build locally, some don't. We're talking about designers here, some of which are surprisingly CLI-savvy, the rest of which revolt in horror at the mere thought. They frequently merge stuff back and forth and every so often, someone (inadvertently) touches something they shouldn't have (hello color marks)... or rename the UFO and someone was on holidays and continues working on his old branch and tries to merge...
    1. Yes.
    2. Use ufonormalizer -m some.ufo to enforce order.
    3. One tricky area outside .glif files is metadata in e.g. fontinfo.plist. Another area is that most editors out there treat UFO as an afterthought and export whatever they want without any regard for round-tripping.
    4. Use shared feature files and include(...) them in the UFOs. Makes it quite manageable actually with plain git.
    5. Hinting data is a nightmare, stuff in data/ is tossed by most UFO im-/exporters and you must not commit the removals. Always rely on autohinting!

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 24, 2018

ufonormalizer

https://github.com/unified-font-object/ufoNormalizer FFR

What does it do? The docs are... sparse :)

We're talking about designers here, some of which are surprisingly CLI-savvy, the rest of which revolt in horror at the mere thought

Have any of your designers tried the git GUI applications? Git Kraken is fantastic. I use it all the time. It is rare that I need to drop to the command line for routine work. It would give them visual representation of their branches vs. other remote branches and a set of buttons to click for branching/pulls/pushes. And notifies, adds support for simple conflict resolution when there is a merge conflict in a way that is less daunting than command line git. I'm sure that there are free tools that do the same, but I haven't tried so can't recommend.

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

Have any of your designers tried the git GUI applications?

Some do, me and a few others like to use Fork. One of the major problems with e.g. Glyphs in the workflow is that both Glyphs and glyphsLib are imperfect and designers regularly get swamped with humongous amounts of git diffs for innocuous changes depending on glyphsLib version, Glyphs version and moonphase. Then there's stuff generated by tools, e.g. things that work on hinting. What if you did many changes and did not commit in atomic steps? Are you pro enough yet to understand every change done to a UFO? Can you build and diff the textual changes to a .glif in your head? Many throw their hands up and just commit everything and I can understand that. Oh, hello there merge conflict!

Font development is and remains a major PITA because the (graphical) tooling just isn't there yet. And that's before you start dealing with application quirks (hello Office for Mac!)

What does it do? The docs are... sparse :)

Many things, don't know all of them, but formatting XML in one way and sorting keys are among them iirc. We use it for round-tripping to Glyphs and back due to reordering of keys, whitespace, ...

@anthrotype
Copy link

let's unilaterally and opportunistically declare that whatever is the output of fontTools.ufoLib is the "normalized" representation of a UFO data, and be done with ufonormalizers. Just sayin'

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

What to do about editors that reorder keys? Does ufoLib order them back?

@anthrotype
Copy link

what order are you talking about? the property list dictionary are always sorted alphabetically.

@anthrotype
Copy link

and plist arrays area inherently ordered, if authoring tools change their order, that's a bug

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

Hm. @moyogo, @belluzj: is there something specific in our Glyphs/Fontlab round-tripping that requires normalization that ufoLib does not do by itself?

@anthrotype
Copy link

tabs

@chrissimpkins
Copy link
Member

let's unilaterally and opportunistically declare that whatever is the output of fontTools.ufoLib is the "normalized" representation of a UFO data, and be done with ufonormalizers. Just sayin'

Maybe a pre-commit hook for that?

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

Worth a try I guess? A script that opens all UFOs, loads everything/marks everything dirty without actually doing anything and writes it back. Can even run on the CI first.

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 24, 2018

Are you pro enough yet to understand every change done to a UFO? Can you build and diff the textual changes to a .glif in your head?

No one will ever be able to do this. But that is where the tooling like the browser visual diff tool (can't remember name) should come in to play. There needs to be a visual diff level of automation because sometimes changes in the source text do not lead to visual differences, or only do at certain sizes. I have yet to find a tool that allows for simple visual diffs that include size specific rendering with TT instruction sets considered. That is a tool that I would love to see. I don't know how to make it...

@madig
Copy link
Contributor Author

madig commented Oct 24, 2018

Yes, and it's why there's still years of effort ahead of us :)

@chrissimpkins
Copy link
Member

Part of what makes type source special :) It is not simple to interpret for humans, even in text format. And then you build with it and pile on post compilation modifications as icing on the cake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants