builtin difftool parsing issue

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

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

Hi Paul,

On Wed, 21 Dec 2016, Paul Sbarra wrote:

Thank you for the report! I fixed it locally.

I would have expected `git difftool --submodule=diff ...` to work... What are the problems?

Ciao, Johannes

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.

On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> wrote:

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=<format>, 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=<format>)

On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller <sbeller@google.com> wrote:

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