# builtin difftool parsing issue 5 messages from 2016-12-22 to 2017-01-04. Participants: Paul Sbarra, Johannes Schindelin, Stefan Beller, Jacob Keller. Thread: https://gitlist.dev/t/44753 ## Paul Sbarra, 2016-12-22 04:53 Subject: builtin difftool parsing issue Message-ID: URL: https://gitlist.dev/e/CAGf%2BdShpkPvsC8wQN6mWmYeMZ3%3Di-ZOzDNSM1aa0rinKW6%2B-%2Bg%40mail.gmail.com ``` Sadly, I haven't been able to figure out how to get the mbox file from this tread into gmail, but wanted to report a parsing issue I've found with the builtin difftool. Original Patch: https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t > + *status = *++p; > + if (!status || p[1]) > + return error("unexpected trailer: '%s'", p); > + return 0; The p[1] null check assumes the status is only one character long, but git-diff's raw output format shows that a numeric value can follow in the copy-edit and rename-edit cases. I'm looking forward to seeing the builtin difftool land. I came across it while investigating adding --submodule=diff (expanding on diff's recent addition) support and this looks more promising then the perl script. Hopefully I will make some progress. Any tips/pointers would be greatly appreciated. Thanks ``` ## Johannes Schindelin, 2017-01-02 16:12 Subject: Re: builtin difftool parsing issue Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1701021712010.3469%40virtualbox In-Reply-To: ``` Hi Paul, On Wed, 21 Dec 2016, Paul Sbarra wrote: > Sadly, I haven't been able to figure out how to get the mbox file from > this tread into gmail, but wanted to report a parsing issue I've found > with the builtin difftool. > > Original Patch: > https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t > > > + *status = *++p; > > + if (!status || p[1]) > > + return error("unexpected trailer: '%s'", p); > > + return 0; > > The p[1] null check assumes the status is only one character long, but > git-diff's raw output format shows that a numeric value can follow in > the copy-edit and rename-edit cases. Thank you for the report! I fixed it locally. > I'm looking forward to seeing the builtin difftool land. I came across it > while investigating adding --submodule=diff (expanding on diff's > recent addition) support and this looks more promising then the perl > script. Hopefully I will make some progress. Any tips/pointers would > be greatly appreciated. I would have expected `git difftool --submodule=diff ...` to work... What are the problems? Ciao, Johannes ``` ## Paul Sbarra, 2017-01-02 19:05 Subject: Re: builtin difftool parsing issue Message-ID: URL: https://gitlist.dev/e/CAGf%2BdSjM9nuroeSM9mkQmO3ho4XcZhLo1CR76q-jbeQ-WNGG%2BQ%40mail.gmail.com In-Reply-To: ``` > I would have expected `git difftool --submodule=diff ...` to work... What > are the problems? The docs for difftool state... "git difftool is a frontend to git diff and accepts the same options and arguments." which could lead a user to expect passing --submodule=diff to have a similar behavior for difftool. It would be especially useful when combined with --dir-diff. Unfortunately, due to the way the left/right directories are built up, difftool needs to handle this option itself. Currently a file representing the submodule directory is created that contains the hash. if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) { strbuf_reset(&buf); strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&loid)); add_left_or_right(&submodules, src_path, buf.buf, 0); strbuf_reset(&buf); strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&roid)); if (!oidcmp(&loid, &roid)) strbuf_addstr(&buf, "-dirty"); add_left_or_right(&submodules, dst_path, buf.buf, 1); continue; } To achieve the desired behavior a diff command would need to be run within the submodule. A further complication is whether submodules should be processed recursively. I'm not sure whether or not diff handles them recursively. I believe the logic to parse and build up the files would need to be factored out such that it could be called for the super-project as well as each submodule change. This is all out of scope for your effort as the existing (perl-based) difftool doesn't do this either. However, it's a feature that would provide a significant simplification to the workflow used at the office to review changes. ``` ## Stefan Beller, 2017-01-03 18:47 Subject: Re: builtin difftool parsing issue Message-ID: URL: https://gitlist.dev/e/CAGZ79kaMnFUefiMagU82euxjeQdnc9KdgSwyDvkDp--QT-MbCA%40mail.gmail.com In-Reply-To: ``` On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra wrote: >> I would have expected `git difftool --submodule=diff ...` to work... What >> are the problems? > > The docs for difftool state... > "git difftool is a frontend to git diff and accepts the same options > and arguments." I think such a sentence in the man page is dangerous, as nobody was caught this issue until now. There have been numerous authors and reviewers that touched "diff --submodule=, but as there is no back-reference, hinting that the patch is only half done, and the difftool also needs to implement such an option. We should reword the man page either as "git difftool is a frontend to git diff and accepts the most(?) options and arguments." or even be explicit and list the arguments instead. There we could also describe differences if any (e.g. the formats available might be different for --submodule=) ``` ## Jacob Keller, 2017-01-04 01:05 Subject: Re: builtin difftool parsing issue Message-ID: URL: https://gitlist.dev/e/CA%2BP7%2BxqaOXagpXb53raPcD19uZRgYrOYH6O9pDPsAxE6TsMkvg%40mail.gmail.com In-Reply-To: ``` On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller wrote: > On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra wrote: >>> I would have expected `git difftool --submodule=diff ...` to work... What >>> are the problems? >> >> The docs for difftool state... >> "git difftool is a frontend to git diff and accepts the same options >> and arguments." > > I think such a sentence in the man page is dangerous, as nobody > was caught this issue until now. There have been numerous authors > and reviewers that touched "diff --submodule=, but as there > is no back-reference, hinting that the patch is only half done, and the > difftool also needs to implement such an option. > > We should reword the man page either as > > "git difftool is a frontend to git diff and accepts the most(?) options > and arguments." > > or even be explicit and list the arguments instead. There we could also > describe differences if any (e.g. the formats available might be different > for --submodule=) I agree with updating the documentation. Difftool would require extensive work to support the --submodule format, and I'm not sure it's worth it. Thanks, Jake ```