builtin difftool parsing issue

builtin difftool parsing issue

[Paul Sbarra]

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

Re: builtin difftool parsing issue

[Johannes Schindelin]

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

Re: builtin difftool parsing issue

[Paul Sbarra]

> 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.

Re: builtin difftool parsing issue

[Stefan Beller]

On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> 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=<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>)

Re: builtin difftool parsing issue

[Jacob Keller]

On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> 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=<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>)

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