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

Problem with multiline CUSTOMDIFCMD #204

Closed weirdNox closed 4 years ago

weirdNox commented 4 years ago

Hello there! First of all, thanks for this utility!

I believe I've encountered a bug while using this on a complex document. I have a command pushRisk which builds a token list for a table based on the arguments. I am using the command like this:

\pushRisk{arg1}{arg2}{arg3}{
  Long text 1
}{
  Long text 2
}

arg1, arg2, and arg3 are also used for building IDs that may be used elsewhere, so they can't be diffed. The last 2 arguments are text, and could be diffed.

Ideally, if only the last 2 arguments change, they would be diffed with the previous version. However, if the first 3 arguments change, two entries need to exist, one for the previous version (with deleted markup), and one for the new version (with new markup).

So, I have resorted to using CUSTOMDIFCMD to make it use ADDpushRisk and DELpushRisk. The problem is that the deleted version becomes like this:

\DELpushRisk{arg1}{arg2}{arg3}{
%DIFDELCMD <   Long text 1
%DIFDELCMD < }{
%DIFDELCMD <   Long text 2
%DIFDELCMD < }

I've never used Perl before, but if I understood the code correctly, you are adding the comments to all lines initially (as this is a non-safe cmd), and then you only replace the comment on the first one when you change pushRisk to DELpushRisk. Due to how the change is done, if I hardcode pushRisk in KEEPCMDLIST, no comments are added, but pushRisk stops being converted to DELpushRisk.

The easiest way I see to solve this problem is to iterate again over the braces when changing cmd -> DELcmd and remove the following DEL comments while still inside this command. I cannot help any further, as Perl looks chinese to me :P

Semi-related comments:

ftilmann commented 4 years ago

The CUSTOMDIFCMD functionality was contributed (by @jprotze ) so I have to familiarise myself with the workings of this part, so the main point should be possible to address. Although in some sense it is trivial to do, if you want to help please construct a full compilable MWE (Minimum Working Example, old file + new file).

ftilmann commented 4 years ago

I think the manual is wrong, because even though it says textcmds check only the last argument, they were diffing all arguments for me in other commands (which also broke other ID systems, similar to this one).

The way described by the manual is certainly how it is supposed to work. If it is diffing all arguments, it's technically a bug, because this would break many commands. I have never experienced anything like it. Are you sure that there is no space or comment between the arguments (in which case latex would not recognise them as arguments, or you would have to use --allow-spaces argument). If you can demonstrate the effect with an MWE, then I can try and see what is wrong. Please open a new issue in this case.

ftilmann commented 4 years ago

It would be cool if I could define something like \pushRisk{.}{.}{.*} to be a text command, so if the first 3 args stay the same, the last 2 would be textually-diffed; otherwise, it could fallback to being a CUSTOMDIFCMD. It may be very hard to implement this, but it looks like a nice feature from a user point of view (and it would let you define the number of text arguments a command has, instead of being all of them or only the last).

That's a feature request. I agree it would be nice to have but as you already suspected not so straightforward to implement. Feel free to open a new issue on this, though, ideally with an MWE demonstrating desired output, but don't expect it to be addressed by me in the immediate future

weirdNox commented 4 years ago

Thanks for the fast response!

Here is the MWE you requested: customdifcmd_test.zip I reproduced it using version 1.3.1.1, with the command latexdiff -c CUSTOMDIFCMD=cmd old.tex new.tex > diff.tex

Just so it is clear, the output is currently like this:

\DELcmd{Test old}{
%DIFDELCMD <   Long text in here!
%DIFDELCMD < 

%DIFDELCMD <   With several lines.
%DIFDELCMD < }
%DIFDELCMD <  %%%

When it should have been:

\DELcmd{Test old}{
  Long text in here!

  With several lines.
}

(Just noticed a new pull request is already created... that was fast! :tada:)

I am going to test the other issue, if I can reproduce it, I'll create a new one. Thanks!

jprotze commented 4 years ago

@weirdNox does #205 solve your issue?

weirdNox commented 4 years ago

I believe so! Thanks!

weirdNox commented 4 years ago

Actually, with further testing, it looks like a new bug has surfaced (but maybe it was already there before): customdifcmd_test2.zip

Note that when it deletes a block of several CUSTOMDIFCMDs, it only keeps the last one (as DELcmd), when it should have converted them all.

ftilmann commented 4 years ago

@jprotze thanks for creating a patch with lightning speed. Next week I will be rather busy with work, but I am aiming to test and merge next weekend.

ftilmann commented 4 years ago

The patch #205 in the end caused some issues on verification. The last commit fixes these and also processes the example in customdifcmd_test2.zip correctly.

weirdNox commented 4 years ago

Awesome! I'll need this utility again in a week or so, and this will be very useful, thanks a lot!