search

ftilmann / latexdiff

Compares two latex files and marks up significant differences between them. Releases on www.ctan.org and mirrors
GNU General Public License v3.0
514 stars 72 forks source link

--filter-script option added #167

Closed jasonmccsmith closed 5 years ago

jasonmccsmith commented 5 years ago

As per Issue 165, --filter-script option added that is applied to both old and new files before diffing.

ftilmann commented 5 years ago

Sorry that it has taken me so long to look at this - very busy time at work. This looked good on inspection but I just tested it and it has the following issues:

  1. If filter-script option is not given (a) a warning Use of uninitialized value $filterscript in string ne at /home/tilmann/bin/latexdiff line 818. is issued. (b) latexdiff does not generate any diff file (this is obvious from the code), filter subroutine returns nothing when test for $filterscript is false or undefined

  2. The output of the filter function seems to be additionally printed out, which is undesirable as it removes command line history and obscures messages , e.g. latexdiff --filter-script=cat test-old.tex test-new.tex> diff.tex works as expected just as latexdiff without filter, except that additionally the contributing files are printed out.

  3. The status information on number of characters passed to the filter script and received from it should only be printed if verbose flag is set (add if $verbose) behind corresponding print statements.

  4. The usage information for filter-script option needs to be added (Note that there is the brief help shown with -h flag following line 3522 , and the possibly more detailed help in the docstring following l. 3771. The filter-string option is simple enough that the same text can be used, but the markup is a little different.

1,3 and 4 are relatively trivial to deal with, but 2 I am not immediately able to do myself, as I am unfamiliar with IPC, so it would be great if you could update the PR in the light of these comments

jasonmccsmith commented 5 years ago

I noticed the debug statement issue, sorry about that, thank you for identifying the others, I will fix and issue pull request.

On Mar 30, 2019, at 2:15 AM, ftilmann notifications@github.com wrote:

Sorry that it has taken me so long to look at this - very busy time at work. This looked good on inspection but I just tested it and it has the following issues:

If filter-script option is not given (a) a warning Use of uninitialized value $filterscript in string ne at /home/tilmann/bin/latexdiff line 818. is issued. (b) latexdiff does not generate any diff file (this is obvious from the code), filter subroutine returns nothing when test for $filterscript is false or undefined

The output of the filter function seems to be additionally printed out, which is undesirable as it removes command line history and obscures messages , e.g. latexdiff --filter-script=cat test-old.tex test-new.tex> diff.tex works as expected just as latexdiff without filter, except that additionally the contributing files are printed out.

The status information on number of characters passed to the filter script and received from it should only be printed if verbose flag is set (add if $verbose) behind corresponding print statements.

The usage information for filter-script option needs to be added (Note that there is the brief help shown with -h flag following line 3522 , and the possibly more detailed help in the docstring following l. 3771. The filter-string option is simple enough that the same text can be used, but the markup is a little different.

1,3 and 4 are relatively trivial to deal with, but 2 I am not immediately able to do myself, as I am unfamiliar with IPC, so it would be great if you could update the PR in the light of these comments

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ftilmann/latexdiff/pull/167#issuecomment-478224406, or mute the thread https://github.com/notifications/unsubscribe-auth/AFZ5jXyaM_0mj1rOE-2grZHa-YKwL06zks5vbys2gaJpZM4bHF9z.

jasonmccsmith commented 5 years ago

Getting back to this after a long absence...

1, 3, and 4 are completed.

filter is now applied for all files, and before flatten’s processing, internally. This will pick up all possible subfiles.

I was unable to see what you were describing with #2, however...

latexdiff base.tex rev.tex --filter-script=cat > diff.tex latexdiff base.tex rev.tex --filter-script=which cat > diff.tex

Both of these result in no output to the console, and no duplicated lines in the diff.tex file.

Can you expand on what you saw so I can replicate, before submitting a pull request? Was it STDERR messages? STDOUT gets consumed as the replacement for $text.

On Mar 30, 2019, at 2:15 AM, ftilmann notifications@github.com wrote:

Sorry that it has taken me so long to look at this - very busy time at work. This looked good on inspection but I just tested it and it has the following issues:

If filter-script option is not given (a) a warning Use of uninitialized value $filterscript in string ne at /home/tilmann/bin/latexdiff line 818. is issued. (b) latexdiff does not generate any diff file (this is obvious from the code), filter subroutine returns nothing when test for $filterscript is false or undefined

The output of the filter function seems to be additionally printed out, which is undesirable as it removes command line history and obscures messages , e.g. latexdiff --filter-script=cat test-old.tex test-new.tex> diff.tex works as expected just as latexdiff without filter, except that additionally the contributing files are printed out.

The status information on number of characters passed to the filter script and received from it should only be printed if verbose flag is set (add if $verbose) behind corresponding print statements.

The usage information for filter-script option needs to be added (Note that there is the brief help shown with -h flag following line 3522 , and the possibly more detailed help in the docstring following l. 3771. The filter-string option is simple enough that the same text can be used, but the markup is a little different.

1,3 and 4 are relatively trivial to deal with, but 2 I am not immediately able to do myself, as I am unfamiliar with IPC, so it would be great if you could update the PR in the light of these comments

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ftilmann/latexdiff/pull/167#issuecomment-478224406, or mute the thread https://github.com/notifications/unsubscribe-auth/AFZ5jXyaM_0mj1rOE-2grZHa-YKwL06zks5vbys2gaJpZM4bHF9z.

jasonmccsmith commented 5 years ago

On the off chance that it was STDERR, it is now ignored.

On May 26, 2019, at 12:06 AM, Jason McC. Smith js@ncpod.org wrote:

Getting back to this after a long absence...

1, 3, and 4 are completed.

filter is now applied for all files, and before flatten’s processing, internally. This will pick up all possible subfiles.

I was unable to see what you were describing with #2, however...

latexdiff base.tex rev.tex --filter-script=cat > diff.tex latexdiff base.tex rev.tex --filter-script=which cat > diff.tex

Both of these result in no output to the console, and no duplicated lines in the diff.tex file.

Can you expand on what you saw so I can replicate, before submitting a pull request? Was it STDERR messages? STDOUT gets consumed as the replacement for $text.

On Mar 30, 2019, at 2:15 AM, ftilmann <notifications@github.com mailto:notifications@github.com> wrote:

Sorry that it has taken me so long to look at this - very busy time at work. This looked good on inspection but I just tested it and it has the following issues:

If filter-script option is not given (a) a warning Use of uninitialized value $filterscript in string ne at /home/tilmann/bin/latexdiff line 818. is issued. (b) latexdiff does not generate any diff file (this is obvious from the code), filter subroutine returns nothing when test for $filterscript is false or undefined

The output of the filter function seems to be additionally printed out, which is undesirable as it removes command line history and obscures messages , e.g. latexdiff --filter-script=cat test-old.tex test-new.tex> diff.tex works as expected just as latexdiff without filter, except that additionally the contributing files are printed out.

The status information on number of characters passed to the filter script and received from it should only be printed if verbose flag is set (add if $verbose) behind corresponding print statements.

The usage information for filter-script option needs to be added (Note that there is the brief help shown with -h flag following line 3522 , and the possibly more detailed help in the docstring following l. 3771. The filter-string option is simple enough that the same text can be used, but the markup is a little different.

1,3 and 4 are relatively trivial to deal with, but 2 I am not immediately able to do myself, as I am unfamiliar with IPC, so it would be great if you could update the PR in the light of these comments

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ftilmann/latexdiff/pull/167#issuecomment-478224406, or mute the thread https://github.com/notifications/unsubscribe-auth/AFZ5jXyaM_0mj1rOE-2grZHa-YKwL06zks5vbys2gaJpZM4bHF9z.

ftilmann commented 5 years ago

I was unable to see what you were describing with #2, however... latexdiff base.tex rev.tex --filter-script=cat > diff.tex latexdiff base.tex rev.tex --filter-script=which cat > diff.tex Both of these result in no output to the console, and no duplicated lines in the diff.tex file.

For these commands I do see the output in the console, which must be on STDERR, as STDOUT is redirected to diff.tex (i.e. no duplicate lines in diff.tex) Not sure why we are seeing different behaviour, possibly a change of behaviour of IPC depending on version number. The man page gives (on Ubuntu 16.04 LTS version) perl v5.22.1 2018-11-19
Probably it's difficult and not worth tracking that down, especially I don't know how you would be able to replicate this. Your proposed work-around sounds a little dangerous: if there is a true error message, it would be good to be shown this. Still, for now I guess it is all that can be done.
Have you actually updated the pull request on github yet? I cannot see it on github, also not on jasoncsmith/latexdiff (might be my fault for not looking in the right place, of course).

jasonmccsmith commented 5 years ago

I have not updated the pull request, I wanted to confer with you first to see if I could replicate and fix.

I can un-ignore STDERR if you’d prefer... in the meantime I’ll try and deduce why we’re seeing differing output, at least.

On May 26, 2019, at 9:28 AM, ftilmann notifications@github.com wrote:

I was unable to see what you were describing with #2 https://github.com/ftilmann/latexdiff/issues/2, however... latexdiff base.tex rev.tex --filter-script=cat > diff.tex latexdiff base.tex rev.tex --filter-script=which cat > diff.tex Both of these result in no output to the console, and no duplicated lines in the diff.tex file.

For these commands I do see the output in the console, which must be on STDERR, as STDOUT is redirected to diff.tex (i.e. no duplicate lines in diff.tex) Not sure why we are seeing different behaviour, possibly a change of behaviour of IPC depending on version number. The man page gives (on Ubuntu 16.04 LTS version) perl v5.22.1 2018-11-19 Probably it's difficult and not worth tracking that down, especially I don't know how you would be able to replicate this. Your proposed work-around sounds a little dangerous: if there is a true error message, it would be good to be shown this. Still, for now I guess it is all that can be done. Have you actually updated the pull request on github yet? I cannot see it on github, also not on jasoncsmith/latexdiff (might be my fault for not looking in the right place, of course).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ftilmann/latexdiff/pull/167?email_source=notifications&email_token=ABLHTDLLDBAVOQELDPBWC5TPXK3D3A5CNFSM4GY4L5Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIJCSA#issuecomment-496013640, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLHTDLEKTVXXNHWQINAKBDPXK3D3ANCNFSM4GY4L5ZQ.