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]

I'm lost. This is a total newbie Python dev question, but what's the easiest way to test this in real life (as in get an executable in same path I can test with) while fiddling with the code. It is not obvious to me how to even build and run this short of installing it to my system using python setup.py install. Even if I do that with a custom --root argument to stash it somewhere by itself the resulting executable can't find the right python paths. How can this be build and run locally as my user from the project directory during testing?

[chrissimpkins]

Thanks for pushing this Caleb! I should have some time to take a look at it tonight and respond to the general outline question that you initially posted. I've been thinking through how to change the argument order/number with argparse too. I haven't done this based on an option before so I'll have to hit the docs.

For the editable Python install path (i.e., development installs), uninstall any copy that you installed with python setup.py install and then reinstall in a virtual environment with the following from the root (note that this project requires a Py3.6+ interpreter):

$ python3 -m venv .venv
$ source .venv/bin/activate
$ pip install -r requirements.txt -e ".[dev]"

or you can use the make target:

$ python3 -m venv .venv
$ source .venv/bin/activate
$ make install-dev

This installation will give you all of the testing dependencies.

Let me know if you have any problems with either of those approaches.

[chrissimpkins]

You can run the unit tests with:

$ make test-unit

and the entire test harness (lints, type checks, unit tests) with:

$ make test

[alerque]

Thanks, that got me started with some way to run the version I'm hacking on.

[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]

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 https://github.com/source-foundry/fdiff/pull/48/commits/f27ca3aa58ac1aca1faf79e912b14a61dcd30cb8 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 https://github.com/source-foundry/fdiff/commit/d04f6dd530a800dc9349e9a4d1157499564439ec

[chrissimpkins]

Fixed lint CI test fail in https://github.com/source-foundry/fdiff/pull/48/commits/a82cc9cac27a946bd96acb6c10e05b007be70f91

[chrissimpkins]

060c7754d443bb1ea40eebcc64a729337c5df76e 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]

ab4c41444c560e8ba758751df1112e0972ad8b11 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 https://github.com/source-foundry/fdiff/pull/48#issuecomment-614731773? 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 @ https://github.com/source-foundry/fdiff/commit/3d6c3f0f373529f3b0ec9482b8575c3a54cf8985

[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]

@alerque I am going to return to this PR to see if I can wrap things up and push a release over the weekend Caleb. Do you have any feedback re: https://github.com/source-foundry/fdiff/pull/48#issuecomment-619099805?

[alerque]

@alerque I am going to return to this PR to see if I can wrap things up and push a release over the weekend Caleb. Do you have any feedback re: #48 (comment)?

I might be able to poke at this a little tomorrow, but Sat/Sun are my least available days, just a heads up.

Sorry I haven't responded to your earlier query. I wasn't actually entirely sure you were even asking when I first read it but I think I get it now.

If we can determine with some plumbing command the default utility and args that git uses (or just shell back to git itself) I think it would probably be better over all to default to that approach. If we can't, then assuming the availability of diff -u is probably not a good idea. People that are going to customize their font diffs might have other tools in place.

Probably the best thing is to let the arguments to set that up be parsed at the same time as --git ... so that it would be configurable on a case by case basis.

Also just an interesting thought ... it occurred to me one could skip fdiff altogether by using ttx as a smudge/clean filter! I wonder if Git has the concept of a smudge/clean filter but only as a pre-diff hook. Must research.

[chrissimpkins]

Definitely was not intended to poke you for work, weekend or not. I am willing to take it from here and will request your review once the rest is implemented. I recognize that you don't develop in Python. I am trying to get your work merged and want to make sure that I understand how to tackle this so that it works for you before I dive in.

[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 https://github.com/source-foundry/fdiff/commit/3a1f283055f0665d5eb017953f25c0a2e7b2dd0a

[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!