Add --git argument with 7 inputs

[alerque]

Fixes #25.

I had a look at the argparse stuff in fdiff and I couldn't make heads or tails of where to put this. A --git argument makes sense, and argparse seems to have a way to specify the next N arguments belong to it, but I can't make sense of where to put the program logic or how to retrieve the parameters since none of the other arguments seem to be doing anything similar. I'm not really familiar with argparse!

Does this sketch look like the right general place to put this?

[alerque]

As of now, this actually works as a git diff driver. And only that way. I had to disable the other mode. Argparse won't even let me set required=False on them because it says it can't do that for positional parameters. Ugg.

[alerque]

I messed with this a good deal and got it to function, but I can't get the help message to be correct. Argparse doesn't support mutually exclusive groups with positional arguments so I'm processioning the arguments twice, only requiring the normal oldfile newfile args if --git was not present.

By the way there are two commits in here that fix things unrelated to this PR. Would you like those as separate PRs?

[alerque]

In testing this in a bunch of git repository situations, I ran across one thing Git is doing that fdiff doesn't currently handle that it will need to be remedied. In the event of a file creation or deletion, it uses tells an external git driver that one side of the comparison is /dev/null (as opposed to the temporary files normally created for this purpose). Unfortunately fonttools is throwing an error saying this empty file is not a valid font. I think we'll have to catch the case of empty files and bypass asking fonttools to load it and just spit out all the lines as added or deleted depending on which direction we're comparing.

[chrissimpkins]

LGTM. I will install a local copy of this and test it over the weekend. Will you add documentation for the changes that you added to support use with git and the new --git flag?

[chrissimpkins]

The CI fails are for a linting issue (too many blank lines in a module) and a stderr expected string for the mutually exclusive arguments that are changed here. I can update those to address the test fails.

[chrissimpkins]

By the way there are two commits in here that fix things unrelated to this PR. Would you like those as separate PRs?

Commits f27ca3a and what other one contains a bug fix?

[chrissimpkins]

In testing this in a bunch of git repository situations, I ran across one thing Git is doing that fdiff doesn't currently handle that it will need to be remedied. In the event of a file creation or deletion, it uses tells an external git driver that one side of the comparison is /dev/null (as opposed to the temporary files normally created for this purpose). Unfortunately fonttools is throwing an error saying this empty file is not a valid font. I think we'll have to catch the case of empty files and bypass asking fonttools to load it and just spit out all the lines as added or deleted depending on which direction we're comparing.

Sorry, just seeing this message. I will take a look over the weekend and be in touch so that we can discuss how to approach this. We may be able to mock a simple "empty" ttx XML file so that the contents all show up as added or deleted.

[chrissimpkins]

Rebased on master changes in d04f6dd

[chrissimpkins]

Fixed lint CI test fail in a82cc9c

[chrissimpkins]

060c775 updates the help message to include the seven positional arg types required for the --git flag:

usage: fdiff [-h] [--version] [-c] [-l LINES]
             [--include INCLUDE | --exclude EXCLUDE] [--head HEAD]
             [--tail TAIL] [--nomp] [--external EXTERNAL]
             [--git PATH OLD-FILE OLD-HEX OLD-MODE NEW-FILE NEW-HEX NEW-MODE]

An OpenType table diff tool for fonts.

optional arguments:
  -h, --help            show this help message and exit
  --version             show program's version number and exit
  -c, --color           ANSI escape code colored diff
  -l LINES, --lines LINES
                        Number of context lines (default 3)
  --include INCLUDE     Comma separated list of tables to include
  --exclude EXCLUDE     Comma separated list of tables to exclude
  --head HEAD           Display first n lines of output
  --tail TAIL           Display last n lines of output
  --nomp                Do not use multi process optimizations
  --external EXTERNAL   Run external diff tool command
  --git PATH OLD-FILE OLD-HEX OLD-MODE NEW-FILE NEW-HEX NEW-MODE
                        Act as a diff driver for git (takes 7 parameters)

[chrissimpkins]

ab4c414 should fix the unit test that broke on the new stderr string for the exclusive --include and --exclude options used in the same command. The exit status code becomes 2 in the automated error raised by argparse.

[chrissimpkins]

@alerque Caleb, are you able to provide an example of an approach so that I can reproduce the fontTools attempt to read a "missing file" that you indicated in #48 (comment)? Once I can reproduce this I will look into adding the exception handling so that we can find a way to mock a file/string that can be used for the diff.

[alerque]

Thanks for your work on this! I'll have a look to see what I can add including maybe documentation of how to configure git in a bit here.

Caleb, are you able to provide an example of an approach so that I can reproduce the fontTools attempt to read a "missing file" that you indicated

The setup for this is pretty simple. Get yourself a font file and stick it in a new directory.

touch README.md
git add README.md
git commit -m "Initial commit so wo have something to compare to"
git add myfont.ttf
git commit -m "Add font file"
git diff HEAD^..HEAD

Because the file myfont.ttf did not exist in the previous commit, the way git will tell the diff-driver about this is my having it compare /dev/null as the old file to /tmp/somethingoreother.ttf that is a copy of the HEAD version of the font file. Of course fonttools can read the "new" end of this, but the "old" end is not a valid font file.

[chrissimpkins]

Thanks for your work on this! I'll have a look to see what I can add including maybe documentation of how to configure git in a bit here.

Perfect! Thanks!

[chrissimpkins]

The setup for this is pretty simple.

Thank you. I will try to work on the fontTools exception this week.

[chrissimpkins]

Rebased on a fontTools bug fix @ 3d6c3f0

[alerque]

Should we also document usage as a Git diff-tool do you think? This has nothing to do with the changes here and my preference is strongly to use it as a diff-driver, but perhaps having both documented would clear up any possible confusion?

[chrissimpkins]

Should we also document usage as a Git diff-tool do you think? This has nothing to do with the changes here and my preference is strongly to use it as a diff-driver, but perhaps having both documented would clear up any possible confusion?

IMO it is fine to document only the recommended approach. Let's see if anyone wanders by who needs diff tool information.

[chrissimpkins]

It is possible to execute fdiff as a diff tool runner for other executables. Thoughts about using the git driver approach here with support for other diff tools (e.g., git calls fdiff which then calls diff -u)? I added this for large font diffs because the pure Python version is slow. fdiff serves to pass the ttx xml to the tool rather than having to do the dumps manually before diff'ing the ttx files with another executable.

If we can't make the args work, we'll need to gate the external diff path with a validation so that the user is aware and add this to the docs.

[chrissimpkins]

It is possible to execute fdiff as a diff tool runner for other executables. Thoughts about using the git driver approach here with support for other diff tools (e.g., git calls fdiff which then calls diff -u)? I added this for large font diffs because the pure Python version is slow. fdiff serves to pass the ttx xml to the tool rather than having to do the dumps manually before diff'ing the ttx files with another executable.

If we can't make the args work, we'll need to gate the external diff path with a validation so that the user is aware and add this to the docs.

Thoughts about this @alerque ? I will return to this over the weekend to see if we can get this ready to release. Still trying to figure out how to approach some tests for this in the CI. The git history dependency makes it tricky.

[chrissimpkins]

OK, I am now able to reproduce your issue with fontTools when a diff occurs at the introduction of a new font file (and presumably removal of a font file which I haven't tested yet). I'll continue work on this so that we can catch the exception and handle it with a mock string. I will likely just use an empty string as the comparison. This will show the entire TTX XML text as added (or removed in the case of the removal of a font) which is what we want. We're close!

[chrissimpkins]

Rebased on the v2.0.0 changes that landed in master today. Commit 3a1f283

[alerque]

As I understand it this is now just waiting on fonttools's handling of empty files, correct? Is there anything else I'm missing here?

[chrissimpkins]

As I understand it this is now just waiting on fonttools's handling of empty files, correct? Is there anything else I'm missing here?

Yep. I think that we are very close. We need to come up with an approach to the null files which I think will just be a mocked empty file so that everything is appropriately shown as an added or deleted line in the diff. And we need tests. I'll have time to continue work on it this weekend. If you have any thoughts feel free to push commits or weigh in on the thread!

[alerque]

One more case I didn't test yet is how git handles renames. I think it will just bridge the old and new content and/or display the rename info itself, but it's worth confirming.

[chrissimpkins]

Any thoughts about how to automate the testing so that they can be added in our unit tests in the CI?

[alerque]

Any thoughts about how to automate the testing so that they can be added in our unit tests in the CI?

If this was a shell thing, I would do something like...

• Create a dummy git repository in an empty directory.
• Write the config settings for it.
• Write a simple font file to it.
• Commit.
• Check that last commit patch had a bunch of + lines, no -.
• Write a simple variant of previous font file to it (minimal but real diff).
• Commit.
• Dump patch last commit to .actual file.
• Compare with test's .expected file.

I could easily mock that up in a shell script, but not sure how the process will play with Python's testing framework.