From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [RFC/PATCH 0/2] Color moved code differently
Date: Sat, 03 Sep 2016 23:41:47 -0700 [thread overview]
Message-ID: <xmqqpookqi8k.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAGZ79kYGnkcOxviukj9a8gyaERip5aunXcvsdH-UpBCb=vrVeQ@mail.gmail.com> (Stefan Beller's message of "Sat, 3 Sep 2016 22:23:35 -0700")
Stefan Beller <sbeller@google.com> writes:
>> * On 2/2, doing it at xdiff.c level may be limiting this good idea
>> to flourish to its full potential, as the interface is fed only
>> one diff_filepair at a time.
>
> I realized that after I implemented it. I agree we would want to have
> it function cross file.
>
> So from my current understanding of the code,
> * diffcore_std would call a new function diffcore_detect_moved(void)
> just before diffcore_apply_filter is called.
> * The new function diffcore_detect_moved would then check if the
> diff is a valid textual diff (i.e. real files, not submodules, but
> deletion/creation of one file is allowed)
> If so we generate the diff internally and as in 2/2 would
> hash all added/removed lines with context and store it.
I do not think you should step outside diff_flush(). Only when
producing textual diff, you would have to run the textual diff
twice by going over the q twice:
* The first pass would run diff_flush_patch(), which would call
into xdiff the usual way, but the callback from xdiff would
capture the removed lines and the added lines without making any
output.
* The second pass would run diff_flush_patch(), but the callback
from xdiff would be called with additional information, namely,
the removed and the added lines captured in the first pass.
* I suspect that the fn_out_consume() function that is used for a
normal case (i.e. when we are not doing this more expensive
"moved to/moved from" coloring) can be used for the second pass
above (i.e. the "priv" aka "ecbdata" may need to be extended so
that it can tell which mode of operation it is asked to perform),
but if there is not enough similarity between the second pass of
this "moved from/moved to" mode and the normal mode of output, it
is also OK to have two different callback functions, i.e. the
original one to be used in the normal mode, the second one that
knows the "these are moved without modification" coloring. The
callback for the first pass is sufficiently different and I think
it is better to invent a new callback function to be used in the
first pass, instead of reusing fn_out_consume().
The fn_out_consume() function working in the "second pass of
moved from/moved to mode" would inspect line[] and see if it is
an added or a removed line, and then:
- if it is an added line, and it appears as a removed line
elsewhere in the patchset (you obtained the information in the
first pass), you show it as "this was moved from elsewhere".
- if it is a removed line, and it appears as an added line
elsewhere in the patchset (you obtained the information in the
first pass), you show it as "this was moved to elsewhere".
Or something like that.
next prev parent reply other threads:[~2016-09-04 6:43 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-03 3:31 [RFC/PATCH 0/2] Color moved code differently Stefan Beller
2016-09-03 3:31 ` [PATCH 1/2] diff.c: emit duplicate lines with a different color Stefan Beller
2016-09-03 3:31 ` [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently Stefan Beller
2016-09-03 12:25 ` Jakub Narębski
2016-09-04 5:31 ` Stefan Beller
2016-09-04 10:35 ` Jakub Narębski
2016-09-04 6:48 ` Junio C Hamano
2016-09-03 7:00 ` [RFC/PATCH 0/2] Color moved code differently Junio C Hamano
2016-09-04 5:23 ` Stefan Beller
2016-09-04 6:41 ` Junio C Hamano [this message]
2016-09-04 8:28 ` Stefan Beller
2016-09-04 22:19 ` Junio C Hamano
2016-09-04 9:57 ` Jacob Keller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqpookqi8k.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).