* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Torsten Bögershausen @ 2016-09-13 14:44 UTC (permalink / raw)
To: Lars Schneider, Torsten Bögershausen
Cc: Git Mailing List, peff, gitster, sbeller, Johannes.Schindelin,
jnareb, mlbright, jacob.keller
In-Reply-To: <10020380-76ED-4371-A0BA-59D07AF33CE0@gmail.com>
On 12.09.16 11:49, Lars Schneider wrote:
>> How do we send pathnames the have '\n' ?
>> Not really recommended, but allowed.
>> And here I am a little bit lost, is each of the lines packed into
>> a pkt-line ?
>> command=smudge is packet as pkt-line and pathname= is packed into
>> another one ? (The we don't need the '\n' at all)
>
> Every line is a dedicated packet. That's why '\n' in a path name would
> not be a problem as the receiver is expected to read the entire packet
> when parsing the value (and the receiver knows the packet length, too).
>
> The '\n' at the end is required by the pkt-line format:
> "A non-binary line SHOULD BE terminated by an LF..."
> (see protocol-common.txt)
That is only the half part of the story:
A non-binary line SHOULD BE terminated by an LF, which if present
MUST be included in the total length. Receivers MUST treat pkt-lines
with non-binary data the same whether or not they contain the trailing
LF (stripping the LF if present, and not complaining when it is
missing).
How do we treat pathnames ?
They can have each byte value except '\0'.
What should a receiver do, which reads a string like "ABC\n\n" ?
Is it "ABC\n" or "ABC\n\n" ?
I would really consider to treat pathnames as binary, and not add a trailing '\n',
are there other opinions ?
^ permalink raw reply
* Re: [RFC/PATCH 01/17] diff: move line ending check into emit_hunk_header
From: René Scharfe @ 2016-09-13 14:42 UTC (permalink / raw)
To: Stefan Beller, gitster, peff, chriscool; +Cc: git
In-Reply-To: <20160913044613.1037-2-sbeller@google.com>
Am 13.09.2016 um 06:45 schrieb Stefan Beller:
> In a later patch, I want to propose an option to detect&color
> moved lines in a diff, which cannot be done in a one-pass over
> the diff. Instead we need to go over the whole diff twice,
> because we cannot detect the first line of the two corresponding
> lines (+ and -) that got moved.
>
> So to prepare the diff machinery for two pass algorithms
> (i.e. buffer it all up and then operate on the result),
> move all emissions to places, such that the only emitting
> function is emit_line_0.
>
> This patch moves code that is conceptually part of
> emit_hunk_header, but was using output in fn_out_consume,
> back to emit_hunk_header.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cc8e812..aa50b2d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -610,6 +610,9 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
> }
>
> strbuf_add(&msgbuf, line + len, org_len - len);
> + if (line[org_len - 1] != '\n')
> + strbuf_addch(&msgbuf, '\n');
> +
Using strbuf_complete_line() would be nicer.
> emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
> strbuf_release(&msgbuf);
> }
> @@ -1247,8 +1250,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
> len = sane_truncate_line(ecbdata, line, len);
> find_lno(line, ecbdata);
> emit_hunk_header(ecbdata, line, len);
> - if (line[len-1] != '\n')
> - putc('\n', o->file);
> return;
> }
>
>
^ permalink raw reply
* [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-13 14:26 UTC (permalink / raw)
To: git; +Cc: gitster, pclouds, peartben, Ben Peart
Teach git to avoid unnecessary merge during trivial checkout. When
running 'git checkout -b foo' git follows a common code path through
the expensive merge_working_tree even when it is unnecessary. As a
result, 95% of the time is spent in merge_working_tree doing the 2-way
merge between the new and old commit trees that is unneeded.
The time breakdown is as follows:
merge_working_tree <-- 95%
unpack_trees <-- 80%
traverse_trees <-- 50%
cache_tree_update <-- 17%
mark_new_skip_worktree <-- 10%
With a large repo, this cost is pronounced. Using "git checkout -b r"
to create and switch to a new branch costs 166 seconds (all times worst
case with a cold file system cache).
git.c:406 trace: built-in: git 'checkout' '-b' 'r'
read-cache.c:1667 performance: 17.442926555 s: read_index_from
name-hash.c:128 performance: 2.912145231 s: lazy_init_name_hash
read-cache.c:2208 performance: 4.387713335 s: write_locked_index
trace.c:420 performance: 166.458921289 s: git command:
'c:\Users\benpeart\bin\git.exe' 'checkout' '-b' 'r'
Switched to a new branch 'r'
By adding a test to skip the unnecessary call to merge_working_tree in
this case reduces the cost to 16 seconds.
git.c:406 trace: built-in: git 'checkout' '-b' 's'
read-cache.c:1667 performance: 16.100742476 s: read_index_from
trace.c:420 performance: 16.461547867 s: git command: 'c:\Users\benpeart\bin\git.exe' 'checkout' '-b' 's'
Switched to a new branch 's'
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
builtin/checkout.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..8b2f428 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -38,6 +38,10 @@ struct checkout_opts {
int ignore_skipworktree;
int ignore_other_worktrees;
int show_progress;
+ /*
+ * If new checkout options are added, needs_working_tree_merge
+ * should be updated accordingly.
+ */
const char *new_branch;
const char *new_branch_force;
@@ -460,11 +464,99 @@ static void setup_branch_path(struct branch_info *branch)
branch->path = strbuf_detach(&buf, NULL);
}
+static int needs_working_tree_merge(const struct checkout_opts *opts,
+ const struct branch_info *old,
+ const struct branch_info *new)
+{
+ /*
+ * We must do the merge if we are actually moving to a new
+ * commit tree.
+ */
+ if (!old->commit || !new->commit ||
+ oidcmp(&old->commit->tree->object.oid, &new->commit->tree->object.oid))
+ return 1;
+
+ /*
+ * opts->patch_mode cannot be used with switching branches so is
+ * not tested here
+ */
+
+ /*
+ * opts->quiet only impacts output so doesn't require a merge
+ */
+
+ /*
+ * Honor the explicit request for a three-way merge or to throw away
+ * local changes
+ */
+ if (opts->merge || opts->force)
+ return 1;
+
+ /*
+ * Checking out the requested commit may require updating the working
+ * directory and index, let the merge handle it.
+ */
+ if (opts->force_detach)
+ return 1;
+
+ /*
+ * opts->writeout_stage cannot be used with switching branches so is
+ * not tested here
+ */
+
+ /*
+ * Honor the explicit ignore requests
+ */
+ if (!opts->overwrite_ignore || opts->ignore_skipworktree ||
+ opts->ignore_other_worktrees)
+ return 1;
+
+ /*
+ * opts->show_progress only impacts output so doesn't require a merge
+ */
+
+ /*
+ * If we're not creating a new branch, by definition we're changing
+ * the existing one so need to do the merge
+ */
+ if (!opts->new_branch)
+ return 1;
+
+ /*
+ * new_branch_force is defined to "create/reset and checkout a branch"
+ * so needs to go through the merge to do the reset
+ */
+ if (opts->new_branch_force)
+ return 1;
+
+ /*
+ * A new orphaned branch requrires the index and the working tree to be
+ * adjusted to <start_point>
+ */
+ if (opts->new_orphan_branch)
+ return 1;
+
+ /*
+ * Remaining variables are not checkout options but used to track state
+ * that doesn't trigger the need for a merge.
+ */
+
+ return 0;
+}
+
static int merge_working_tree(const struct checkout_opts *opts,
struct branch_info *old,
struct branch_info *new,
int *writeout_error)
{
+ /*
+ * Optimize the performance of "git checkout -b foo" by avoiding
+ * the expensive merge, index and working directory updates if they
+ * are not needed.
+ */
+ if (!needs_working_tree_merge(opts, old, new))
+ return 0;
+
int ret;
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
--
2.10.0.windows.1
^ permalink raw reply related
* Re: [ANNOUNCE] Git User's Survey 2016
From: Jakub Narębski @ 2016-09-13 13:52 UTC (permalink / raw)
To: Kevin Daudt
Cc: git, Doug Rathbone, David Bainbridge, Stefan Beller,
Andrew Ardill, Eric Wong
In-Reply-To: <20160913133224.GB31326@ikke.info>
Hello Kevin,
On 13 September 2016 at 15:32, Kevin Daudt <me@ikke.info> wrote:
> On Mon, Sep 12, 2016 at 09:51:09PM +0200, Jakub Narębski wrote:
>>
>> P.P.S. Different announcements use different URLs (different channels)
>> to better track where one got information about this survey.
>>
>> Thanks in advance for taking time to answer the survey,
>
> Can we get a channel for the freenode/#git channel? I'm trying to announce the
> survey there too.
Here you have it:
https://survs.com/survey/c6wjrerw87
--
Jakub Narebski
^ permalink raw reply
* Re: [ANNOUNCE] Git User's Survey 2016
From: Kevin Daudt @ 2016-09-13 13:32 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Doug Rathbone, David Bainbridge, Stefan Beller,
Andrew Ardill, Eric Wong
In-Reply-To: <de39c03d-e84e-8e45-69cb-a2e01fd25df2@gmail.com>
On Mon, Sep 12, 2016 at 09:51:09PM +0200, Jakub Narębski wrote:
> Hello all,
>
>
> P.P.S. Different announcements use different URLs (different channels)
> to better track where one got information about this survey.
>
> Thanks in advance for taking time to answer the survey,
Can we get a channel for the freenode/#git channel? I'm trying to announce the
survey there too.
^ permalink raw reply
* RE: [PATCH v2] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-13 12:33 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: git, pclouds, 'Ben Peart'
In-Reply-To: <xmqqtwdkzwpp.fsf@gitster.mtv.corp.google.com>
> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Monday, September 12, 2016 4:32 PM
> To: Ben Peart <peartben@gmail.com>
> Cc: git@vger.kernel.org; pclouds@gmail.com; 'Ben Peart'
> <benpeart@microsoft.com>
> Subject: Re: [PATCH v2] checkout: eliminate unnecessary merge for trivial
> checkout
>
> "Ben Peart" <peartben@gmail.com> writes:
>
> > I completely agree that optimizing within merge_working_tree would
> > provide more opportunities for optimization. I can certainly move the
> > test into that function as a first step.
>
> Note that "optimizing more" was not the primary point of my response.
>
> Quite honestly, I'd rather see us speed up _ONLY_ obviously correct and
> commonly used cases, while leaving most cases that _MAY_ turn out to be
> optimizable (if we did careful analysis) unoptimized, and instead have
them
> handled by generic but known to be correct codepath, if it means we do NOT
> to have to spend mental bandwidth to analyze not-common case--that is a
> much better tradeoff.
>
> The suggestion to move the check one level down in the callchain was
> primarily to avoid the proposed optimization from being overly eager and
> ending up skipping necessary parts of what merge_working_tree() does (e.g.
> like I suspected in the review that the proposed patch skips the check for
> "you have unmerged entries" situation).
The check for unmerged entries makes complete sense when you are about
to attempt to merge different commit trees and generate an updated index
and working directory. This optimization however is trying to skip
those expensive steps for the specific case of creating a new branch and
switching to it. In this narrow (but common) case, all that needs to
happen is that a new ref is created and HEAD switched to it. Since
we're not doing a merge, I don't believe the check is necessary.
^ permalink raw reply
* Re: [PATCH] [git-p4.py] Add --checkpoint-period option to sync/clone
From: Luke Diamand @ 2016-09-13 8:10 UTC (permalink / raw)
To: Ori Rawlings; +Cc: Git Users, Vitor Antunes, Lars Schneider, Pete Wyckoff
In-Reply-To: <1473717733-65682-2-git-send-email-orirawlings@gmail.com>
On 12 September 2016 at 23:02, Ori Rawlings <orirawlings@gmail.com> wrote:
> Importing a long history from Perforce into git using the git-p4 tool
> can be especially challenging. The `git p4 clone` operation is based
> on an all-or-nothing transactionality guarantee. Under real-world
> conditions like network unreliability or a busy Perforce server,
> `git p4 clone` and `git p4 sync` operations can easily fail, forcing a
> user to restart the import process from the beginning. The longer the
> history being imported, the more likely a fault occurs during the
> process. Long enough imports thus become statistically unlikely to ever
> succeed.
That would never happen :-)
The usual thing that I find is that my Perforce login session expires.
>
> The underlying git fast-import protocol supports an explicit checkpoint
> command. The idea here is to optionally allow the user to force an
> explicit checkpoint every <x> seconds. If the sync/clone operation fails
> branches are left updated at the appropriate commit available during the
> latest checkpoint. This allows a user to resume importing Perforce
> history while only having to repeat at most approximately <x> seconds
> worth of import activity.
I think this ought to work, and could be quite useful. It would be
good to have some kind of test case for it though, and updated
documentation.
Luke
>
> Signed-off-by: Ori Rawlings <orirawlings@gmail.com>
> ---
> git-p4.py | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..40cb64f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2244,6 +2244,7 @@ class P4Sync(Command, P4UserMap):
> optparse.make_option("-/", dest="cloneExclude",
> action="append", type="string",
> help="exclude depot path"),
> + optparse.make_option("--checkpoint-period", dest="checkpointPeriod", type="int", help="Period in seconds between explict git fast-import checkpoints (by default, no explicit checkpoints are performed)"),
> ]
> self.description = """Imports from Perforce into a git repository.\n
> example:
> @@ -2276,6 +2277,7 @@ class P4Sync(Command, P4UserMap):
> self.tempBranches = []
> self.tempBranchLocation = "refs/git-p4-tmp"
> self.largeFileSystem = None
> + self.checkpointPeriod = -1
Or use None?
>
> if gitConfig('git-p4.largeFileSystem'):
> largeFileSystemConstructor = globals()[gitConfig('git-p4.largeFileSystem')]
> @@ -3031,6 +3033,8 @@ class P4Sync(Command, P4UserMap):
>
> def importChanges(self, changes):
> cnt = 1
> + if self.checkpointPeriod > -1:
> + self.lastCheckpointTime = time.time()
Could you just always set the lastCheckpointTime?
> for change in changes:
> description = p4_describe(change)
> self.updateOptionDict(description)
> @@ -3107,6 +3111,10 @@ class P4Sync(Command, P4UserMap):
> self.initialParent)
> # only needed once, to connect to the previous commit
> self.initialParent = ""
> +
> + if self.checkpointPeriod > -1 and time.time() - self.lastCheckpointTime > self.checkpointPeriod:
> + self.checkpoint()
> + self.lastCheckpointTime = time.time()
If you use time.time(), then this could fail to work as expected if
the system time goes backwards (e.g. NTP updates). However, Python 2
doesn't have access to clock_gettime() without jumping through hoops,
so perhaps we leave this as a bug until git-p4 gets ported to Python
3.
> except IOError:
> print self.gitError.read()
> sys.exit(1)
> --
> 2.7.4 (Apple Git-66)
>
^ permalink raw reply
* Re: [PATCH 1/2 v8] pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use
From: Kirill Smelkov @ 2016-09-13 7:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <xmqqd1k8wc6h.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 12, 2016 at 11:23:18PM -0700, Junio C Hamano wrote:
> Kirill Smelkov <kirr@nexedi.com> writes:
>
> > +static int want_found_object(int exclude, struct packed_git *p)
> > +{
> > + if (exclude)
> > + return 1;
> > + if (incremental)
> > + return 0;
> > +
> > + /*
> > + * When asked to do --local (do not include an object that appears in a
> > + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
> > + * an object that appears in a pack marked with .keep), finding a pack
> > + * that matches the criteria is sufficient for us to decide to omit it.
> > + * However, even if this pack does not satisfy the criteria, we need to
> > + * make sure no copy of this object appears in _any_ pack that makes us
> > + * to omit the object, so we need to check all the packs.
> > + *
> > + * We can however first check whether these options can possible matter;
> > + * if they do not matter we know we want the object in generated pack.
> > + * Otherwise, we signal "-1" at the end to tell the caller that we do
> > + * not know either way, and it needs to check more packs.
> > + */
> > + if (!ignore_packed_keep &&
> > + (!local || !have_non_local_packs))
> > + return 1;
> > +
> > + if (local && !p->pack_local)
> > + return 0;
> > + if (ignore_packed_keep && p->pack_local && p->pack_keep)
> > + return 0;
> > +
> > + /* we don't know yet; keep looking for more packs */
> > + return -1;
> > +}
>
> Moving this logic out to this helper made the main logic in the
> caller easier to grasp.
>
> > @@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,
> > off_t *found_offset)
> > {
> > struct packed_git *p;
> > + int want;
> >
> > if (!exclude && local && has_loose_object_nonlocal(sha1))
> > return 0;
> >
> > + /*
> > + * If we already know the pack object lives in, start checks from that
> > + * pack - in the usual case when neither --local was given nor .keep files
> > + * are present we will determine the answer right now.
> > + */
> > + if (*found_pack) {
> > + want = want_found_object(exclude, *found_pack);
> > + if (want != -1)
> > + return want;
> > + }
> >
> > for (p = packed_git; p; p = p->next) {
> > + off_t offset;
> > +
> > + if (p == *found_pack)
> > + offset = *found_offset;
> > + else
> > + offset = find_pack_entry_one(sha1, p);
> > +
> > if (offset) {
> > if (!*found_pack) {
> > if (!is_pack_valid(p))
> > @@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,
> > *found_offset = offset;
> > *found_pack = p;
> > }
> > + want = want_found_object(exclude, p);
> > + if (want != -1)
> > + return want;
> > }
> > }
>
> As Peff noted in his earlier review, however, MRU code needed to be
> grafted in to the caller (an update to the MRU list was done in the
> code that was moved to the want_found_object() helper). I think I
> did it correctly, which ended up looking like this:
>
> want = want_found_object(exclude, p);
> if (!exclude && want > 0)
> mru_mark(packed_git_mru, entry);
> if (want != -1)
> return want;
>
> I somewhat feel that it is ugly that the helper knows about exclude
> (i.e. in the original code, we immediately returned 1 without
> futzing with the MRU when we find an entry that is to be excluded,
> which now is done in the helper), and the caller also knows about
> exclude (i.e. the caller knows that the helper may return positive
> in two cases, it knows that MRU marking needs to happen only one of
> the two cases, and it also knows that "exclude" is what
> differentiates between the two cases) at the same time.
>
> But probably the reason why I feel it ugly is only because I knew
> how the original looked like. I dunno.
Junio, the code above is correct semantic merge of pack-mru and my
topic, because in pack-mru if found and exclude=1, 1 was returned
without marking found pack.
But I wonder: even if we exclude an object, we were still looking for it
in packs, and when we found it, we found the corresponding pack too. So,
that pack _was_ most-recently-used, and it is correct to mark it as MRU.
We can do the simplification in the follow-up patch after the merge, so
merge does not change semantics and it is all bisectable, etc.
Jeff?
^ permalink raw reply
* Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area
From: Kirill Smelkov @ 2016-09-13 6:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <xmqqpoo8yasu.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 12, 2016 at 04:10:09PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In other words, something along this line, perhaps.
> > ...
>
> Not quite. There is no guanratee that the user is using autoconf at
> all. It should be more like this, I think.
>
> t/perf/run | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/perf/run b/t/perf/run
> index aa383c2..7ec3734 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -30,7 +30,13 @@ unpack_git_rev () {
> }
> build_git_rev () {
> rev=$1
> - cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
> + for config in config.mak config.mak.autogen config.status
> + do
> + if test -f "../../$config"
> + then
> + cp "../../$config" "build/$rev/"
> + fi
> + done
> (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
> die "failed to build revision '$mydir'"
> }
Junio, thanks for encouraging feedback and for catching the *-isms. What
you propose is good (and we also automatically fix error when there was
no config.mak - it was working but cp was giving an error to stderr but
script was continuing normally).
I would amend your squash the following way:
* `test -f` -> `test -e`, because -f tests whether a file exists _and_
is regular file. Some people might have config.mak as a symlink for
example. We don't want to miss them too.
Please find updated patch below:
---- 8< ----
From: Kirill Smelkov <kirr@nexedi.com>
Subject: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends
to build area
Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.
See e.g.
http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
for example.
NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
---
t/perf/run | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index cfd7012..e8adeda 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,13 @@ unpack_git_rev () {
}
build_git_rev () {
rev=$1
- cp ../../config.mak build/$rev/config.mak
+ for config in config.mak config.mak.autogen config.status
+ do
+ if test -e "../../$config"
+ then
+ cp "../../$config" "build/$rev/"
+ fi
+ done
(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
die "failed to build revision '$mydir'"
}
--
2.9.2.701.gf965a18.dirty
^ permalink raw reply related
* Re: [PATCH 1/2 v8] pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use
From: Junio C Hamano @ 2016-09-13 6:23 UTC (permalink / raw)
To: Kirill Smelkov
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160910150110.18844-1-kirr@nexedi.com>
Kirill Smelkov <kirr@nexedi.com> writes:
> +static int want_found_object(int exclude, struct packed_git *p)
> +{
> + if (exclude)
> + return 1;
> + if (incremental)
> + return 0;
> +
> + /*
> + * When asked to do --local (do not include an object that appears in a
> + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
> + * an object that appears in a pack marked with .keep), finding a pack
> + * that matches the criteria is sufficient for us to decide to omit it.
> + * However, even if this pack does not satisfy the criteria, we need to
> + * make sure no copy of this object appears in _any_ pack that makes us
> + * to omit the object, so we need to check all the packs.
> + *
> + * We can however first check whether these options can possible matter;
> + * if they do not matter we know we want the object in generated pack.
> + * Otherwise, we signal "-1" at the end to tell the caller that we do
> + * not know either way, and it needs to check more packs.
> + */
> + if (!ignore_packed_keep &&
> + (!local || !have_non_local_packs))
> + return 1;
> +
> + if (local && !p->pack_local)
> + return 0;
> + if (ignore_packed_keep && p->pack_local && p->pack_keep)
> + return 0;
> +
> + /* we don't know yet; keep looking for more packs */
> + return -1;
> +}
Moving this logic out to this helper made the main logic in the
caller easier to grasp.
> @@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,
> off_t *found_offset)
> {
> struct packed_git *p;
> + int want;
>
> if (!exclude && local && has_loose_object_nonlocal(sha1))
> return 0;
>
> + /*
> + * If we already know the pack object lives in, start checks from that
> + * pack - in the usual case when neither --local was given nor .keep files
> + * are present we will determine the answer right now.
> + */
> + if (*found_pack) {
> + want = want_found_object(exclude, *found_pack);
> + if (want != -1)
> + return want;
> + }
>
> for (p = packed_git; p; p = p->next) {
> + off_t offset;
> +
> + if (p == *found_pack)
> + offset = *found_offset;
> + else
> + offset = find_pack_entry_one(sha1, p);
> +
> if (offset) {
> if (!*found_pack) {
> if (!is_pack_valid(p))
> @@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,
> *found_offset = offset;
> *found_pack = p;
> }
> + want = want_found_object(exclude, p);
> + if (want != -1)
> + return want;
> }
> }
As Peff noted in his earlier review, however, MRU code needed to be
grafted in to the caller (an update to the MRU list was done in the
code that was moved to the want_found_object() helper). I think I
did it correctly, which ended up looking like this:
want = want_found_object(exclude, p);
if (!exclude && want > 0)
mru_mark(packed_git_mru, entry);
if (want != -1)
return want;
I somewhat feel that it is ugly that the helper knows about exclude
(i.e. in the original code, we immediately returned 1 without
futzing with the MRU when we find an entry that is to be excluded,
which now is done in the helper), and the caller also knows about
exclude (i.e. the caller knows that the helper may return positive
in two cases, it knows that MRU marking needs to happen only one of
the two cases, and it also knows that "exclude" is what
differentiates between the two cases) at the same time.
But probably the reason why I feel it ugly is only because I knew
how the original looked like. I dunno.
^ permalink raw reply
* [RFC/PATCH 15/17] diff.c: convert diff_summary to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers diff_summary.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 62 +++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/diff.c b/diff.c
index 2b1ebcc..68da135 100644
--- a/diff.c
+++ b/diff.c
@@ -4382,67 +4382,71 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
}
}
-static void show_file_mode_name(FILE *file, const char *newdelete, struct diff_filespec *fs)
+static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs)
{
+ struct strbuf sb = STRBUF_INIT;
if (fs->mode)
- fprintf(file, " %s mode %06o ", newdelete, fs->mode);
+ strbuf_addf(&sb, " %s mode %06o ", newdelete, fs->mode);
else
- fprintf(file, " %s ", newdelete);
- write_name_quoted(fs->path, file, '\n');
+ strbuf_addf(&sb, " %s ", newdelete);
+
+ quote_c_style(fs->path, &sb, NULL, 0);
+ strbuf_addch(&sb, '\n');
+ emit_line(opt, NULL, NULL, sb.buf, sb.len);
+ strbuf_release(&sb);
}
-static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
- const char *line_prefix)
+static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,
+ int show_name)
{
if (p->one->mode && p->two->mode && p->one->mode != p->two->mode) {
- fprintf(file, "%s mode change %06o => %06o%c", line_prefix, p->one->mode,
- p->two->mode, show_name ? ' ' : '\n');
+ struct strbuf sb = STRBUF_INIT;
if (show_name) {
- write_name_quoted(p->two->path, file, '\n');
+ strbuf_addch(&sb, ' ');
+ quote_c_style(p->two->path, &sb, NULL, 0);
}
+ emit_line_fmt(opt, NULL, NULL,
+ " mode change %06o => %06o%s\n",
+ p->one->mode, p->two->mode,
+ show_name ? sb.buf : "");
+ strbuf_release(&sb);
}
}
-static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
- const char *line_prefix)
+static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
+ struct diff_filepair *p)
{
char *names = pprint_rename(p->one->path, p->two->path);
-
- fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
+ emit_line_fmt(opt, NULL, NULL, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
- show_mode_change(file, p, 0, line_prefix);
+ show_mode_change(opt, p, 0);
}
static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
{
- FILE *file = opt->file;
- const char *line_prefix = diff_line_prefix(opt);
-
switch(p->status) {
case DIFF_STATUS_DELETED:
- fputs(line_prefix, file);
- show_file_mode_name(file, "delete", p->one);
+ show_file_mode_name(opt, "delete", p->one);
break;
case DIFF_STATUS_ADDED:
- fputs(line_prefix, file);
- show_file_mode_name(file, "create", p->two);
+ show_file_mode_name(opt, "create", p->two);
break;
case DIFF_STATUS_COPIED:
- fputs(line_prefix, file);
- show_rename_copy(file, "copy", p, line_prefix);
+ show_rename_copy(opt, "copy", p);
break;
case DIFF_STATUS_RENAMED:
- fputs(line_prefix, file);
- show_rename_copy(file, "rename", p, line_prefix);
+ show_rename_copy(opt, "rename", p);
break;
default:
if (p->score) {
- fprintf(file, "%s rewrite ", line_prefix);
- write_name_quoted(p->two->path, file, ' ');
- fprintf(file, "(%d%%)\n", similarity_index(p));
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, " rewrite ");
+ quote_c_style(p->two->path, &sb, NULL, 0);
+ strbuf_addf(&sb, " (%d%%)\n", similarity_index(p));
+ emit_line(opt, NULL, NULL, sb.buf, sb.len);
}
- show_mode_change(file, p, !p->score, line_prefix);
+ show_mode_change(opt, p, !p->score);
break;
}
}
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 16/17] diff: buffer output in emit_line_0
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
emit_line_0 factors out the emission part into emit_line_emission,
and depending on the diff_options->use_buffer the emission
will be performed directly when calling emit_line_0 or after the
whole process is done, i.e. by buffering we have add the possibility
for a second pass over the whole output before doing the actual
output.
In 6440d34 (2012-03-14, diff: tweak a _copy_ of diff_options with
word-diff) we introduced a duplicate diff options struct for word
emissions as we may have different regex settings in there.
When buffering the output, we need to operate on just one buffer,
so we have to copy back the emissions of the word buffer into the main
buffer.
Unconditionally enable output via buffer in this patch as it yields
a great opportunity for testing, i.e. all the diff tests from the
test suite pass without having reordering issues (i.e. only parts
of the output got buffered, and we forgot to buffer other parts).
The test suite passes, which gives confidence that we converted all
functions to use emit_line_* for output.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++------------
diff.h | 18 ++++++++
2 files changed, 142 insertions(+), 27 deletions(-)
diff --git a/diff.c b/diff.c
index 68da135..e240e89 100644
--- a/diff.c
+++ b/diff.c
@@ -449,42 +449,86 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
-static void emit_line_0(struct diff_options *o, const char *set, const char *reset,
- int first, const char *line, int len)
+static void emit_line_emission(struct diff_options *o, struct line_emission *e)
{
- int has_trailing_newline, has_trailing_carriage_return;
- int nofirst;
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
+ if (e->len || e->first) {
+ if (e->set)
+ fputs(e->set, file);
+ if (e->first)
+ fputc(e->first, file);
+ if (e->whitespace_check) {
+ if (e->reset)
+ fputs(e->reset, file);
+ ws_check_emit(e->line, e->len, e->ws_rule,
+ file, e->set, e->reset, e->ws);
+ } else {
+ fwrite(e->line, e->len, 1, file);
+ if (e->reset)
+ fputs(e->reset, file);
+ }
+ }
+ if (e->has_trailing_carriage_return)
+ fputc('\r', file);
+ if (e->has_trailing_newline)
+ fputc('\n', file);
+}
+
+static void free_line_emission(struct line_emission *e)
+{
+ /*
+ * No need to free set, reset, ws as they always point to the
+ * diff_colors[] static array. We don't own that memory.
+ */
+ free((char*)e->line);
+}
+
+static void append_line_emission_to_buffer(struct diff_options *o,
+ struct line_emission *e)
+{
+ struct line_emission *f;
+ ALLOC_GROW(o->line_buffer,
+ o->line_buffer_nr + 1,
+ o->line_buffer_alloc);
+ f = &o->line_buffer[o->line_buffer_nr++];
+ memcpy(f, e, sizeof(*e));
+
+ /* duplicate the line for now as it is not a stable pointer */
+ f->line = xmemdupz(e->line, e->len);
+}
+
+static void emit_line_0(struct diff_options *o, const char *set,
+ const char *reset, int first, const char *line, int len)
+{
+ int nofirst;
+ struct line_emission e = LINE_EMISSION_INIT;
+
if (len == 0) {
- has_trailing_newline = (first == '\n');
- has_trailing_carriage_return = (first == '\r');
- nofirst = has_trailing_newline || has_trailing_carriage_return;
+ e.has_trailing_newline = (first == '\n');
+ e.has_trailing_carriage_return = (first == '\r');
+ nofirst = e.has_trailing_newline || e.has_trailing_carriage_return;
} else {
- has_trailing_newline = (len > 0 && line[len-1] == '\n');
- if (has_trailing_newline)
+ e.has_trailing_newline = (len > 0 && line[len-1] == '\n');
+ if (e.has_trailing_newline)
len--;
- has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
- if (has_trailing_carriage_return)
+ e.has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+ if (e.has_trailing_carriage_return)
len--;
nofirst = 0;
}
- if (len || !nofirst) {
- if (set)
- fputs(set, file);
- if (!nofirst)
- fputc(first, file);
- fwrite(line, len, 1, file);
- if (reset)
- fputs(reset, file);
- }
- if (has_trailing_carriage_return)
- fputc('\r', file);
- if (has_trailing_newline)
- fputc('\n', file);
+ e.set = set;
+ e.first = !nofirst ? first : 0;
+ e.line = line;
+ e.len = len;
+ e.reset = reset;
+ if (!o->use_buffer)
+ emit_line_emission(o, &e);
+ else
+ append_line_emission_to_buffer(o, &e);
}
static void emit_line(struct diff_options *o, const char *set, const char *reset,
@@ -540,9 +584,22 @@ static void emit_line_checked(const char *reset,
emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
- ws_check_emit(line, len, ecbdata->ws_rule,
- ecbdata->opt->file, set, reset, ws);
+ if (ecbdata->opt->use_buffer) {
+ struct line_emission e;
+ e.line = line;
+ e.len = len;
+ e.ws_rule = ecbdata->ws_rule;
+ e.set = set;
+ e.reset = reset;
+ e.ws = ws;
+ e.whitespace_check = 1;
+ e.first = sign;
+ append_line_emission_to_buffer(ecbdata->opt, &e);
+ } else {
+ emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
+ ws_check_emit(line, len, ecbdata->ws_rule,
+ ecbdata->opt->file, set, reset, ws);
+ }
}
}
@@ -1093,6 +1150,22 @@ static void diff_words_flush(struct emit_callback *ecbdata)
if (ecbdata->diff_words->minus.text.size ||
ecbdata->diff_words->plus.text.size)
diff_words_show(ecbdata->diff_words);
+
+ if (ecbdata->diff_words->opt->line_buffer_nr) {
+ int i;
+ for (i = 0; i < ecbdata->diff_words->opt->line_buffer_nr; i++) {
+ struct line_emission *e =
+ &ecbdata->diff_words->opt->line_buffer[i];
+ ALLOC_GROW(ecbdata->opt->line_buffer,
+ ecbdata->opt->line_buffer_nr + 1,
+ ecbdata->opt->line_buffer_alloc);
+ memcpy(&ecbdata->opt->line_buffer
+ [ecbdata->opt->line_buffer_nr],
+ e, sizeof(*e));
+ ecbdata->opt->line_buffer_nr++;
+ }
+ ecbdata->diff_words->opt->line_buffer_nr = 0;
+ }
}
static void diff_filespec_load_driver(struct diff_filespec *one)
@@ -1128,6 +1201,12 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
xcalloc(1, sizeof(struct diff_words_data));
ecbdata->diff_words->type = o->word_diff;
ecbdata->diff_words->opt = o;
+
+ /* Create our own buffer if needed. */
+ o->line_buffer = NULL;
+ o->line_buffer_nr = 0;
+ o->line_buffer_alloc = 0;
+
if (!o->word_regex)
o->word_regex = userdiff_word_regex(one);
if (!o->word_regex)
@@ -4649,11 +4728,29 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
+ /*
+ * TODO:
+ * For testing purposes we want to make sure the diff machinery
+ * works with the buffer. If there is anything emitted outside the
+ * emit_line_emission, then the order is screwed up and the tests
+ * will fail.
+ *
+ * We'll unset this flag in a later patch.
+ */
+ o->use_buffer = 1;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (check_pair_status(p))
diff_flush_patch(p, o);
}
+
+ if (o->use_buffer) {
+ for (i = 0; i < o->line_buffer_nr; i++) {
+ emit_line_emission(o, &o->line_buffer[i]);
+ free_line_emission(&o->line_buffer[i]);
+ }
+ o->line_buffer_nr = 0;
+ }
}
void diff_flush(struct diff_options *options)
diff --git a/diff.h b/diff.h
index cc5d038..4df6aa5 100644
--- a/diff.h
+++ b/diff.h
@@ -110,6 +110,20 @@ enum diff_words_type {
DIFF_WORDS_COLOR
};
+struct line_emission {
+ const char *set;
+ const char *line;
+ const char *ws;
+ const char *reset;
+ int first;
+ int len;
+ int whitespace_check;
+ unsigned ws_rule;
+ int has_trailing_carriage_return;
+ int has_trailing_newline;
+};
+#define LINE_EMISSION_INIT {NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0 }
+
struct diff_options {
const char *orderfile;
const char *pickaxe;
@@ -178,6 +192,10 @@ struct diff_options {
void *output_prefix_data;
int diff_path_counter;
+
+ int use_buffer;
+ struct line_emission *line_buffer;
+ int line_buffer_nr, line_buffer_alloc;
};
void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 11/17] diff.c: convert emit_binary_diff_body to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers emit_binary_diff_body.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/diff.c b/diff.c
index 1892c2b..ff1e829 100644
--- a/diff.c
+++ b/diff.c
@@ -2169,8 +2169,8 @@ static unsigned char *deflate_it(char *data,
return deflated;
}
-static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
- const char *prefix)
+static void emit_binary_diff_body(struct diff_options *o,
+ mmfile_t *one, mmfile_t *two)
{
void *cp;
void *delta;
@@ -2199,13 +2199,12 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
}
if (delta && delta_size < deflate_size) {
- fprintf(file, "%sdelta %lu\n", prefix, orig_size);
+ emit_line_fmt(o, 0, 0, "delta %lu\n", orig_size);
free(deflated);
data = delta;
data_size = delta_size;
- }
- else {
- fprintf(file, "%sliteral %lu\n", prefix, two->size);
+ } else {
+ emit_line_fmt(o, 0, 0, "literal %lu\n", two->size);
free(delta);
data = deflated;
data_size = deflate_size;
@@ -2214,8 +2213,9 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
/* emit data encoded in base85 */
cp = data;
while (data_size) {
+ int len;
int bytes = (52 < data_size) ? 52 : data_size;
- char line[70];
+ char line[71];
data_size -= bytes;
if (bytes <= 26)
line[0] = bytes + 'A' - 1;
@@ -2223,20 +2223,25 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
line[0] = bytes - 26 + 'a' - 1;
encode_85(line + 1, cp, bytes);
cp = (char *) cp + bytes;
- fprintf(file, "%s", prefix);
- fputs(line, file);
- fputc('\n', file);
+
+ len = strlen(line);
+ line[len++] = '\n';
+ line[len] = '\0';
+
+ emit_line(o, 0, 0, line, len);
}
- fprintf(file, "%s\n", prefix);
+ emit_line(o, 0, 0, "\n", 1);
free(data);
}
-static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two,
- const char *prefix)
+static void emit_binary_diff(struct diff_options *o,
+ mmfile_t *one, mmfile_t *two)
{
- fprintf(file, "%sGIT binary patch\n", prefix);
- emit_binary_diff_body(file, one, two, prefix);
- emit_binary_diff_body(file, two, one, prefix);
+ const char *s = "GIT binary patch\n";
+ const int len = strlen(s);
+ emit_line(o, 0, 0, s, len);
+ emit_binary_diff_body(o, one, two);
+ emit_binary_diff_body(o, two, one);
}
int diff_filespec_is_binary(struct diff_filespec *one)
@@ -2411,7 +2416,7 @@ static void builtin_diff(const char *name_a,
emit_line(o, 0, 0, header.buf, header.len);
strbuf_reset(&header);
if (DIFF_OPT_TST(o, BINARY))
- emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
+ emit_binary_diff(o, &mf1, &mf2);
else
emit_line_fmt(o, 0, 0, "Binary files %s and %s differ\n",
lbl[0], lbl[1]);
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 17/17] diff.c: color moved lines differently
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
When there is a lot of code moved around such as in 11979b9 (2005-11-18,
"http.c: reorder to avoid compilation failure.") for example, the review
process is quite hard, as it is not mentally challenging. It is rather a
tedious process, that gets boring quickly. However you still need to read
through all of the code to make sure the moved lines are there as supposed.
While it is trivial to color up a patch like the following
$ git show -p
commit 95b1fc60907fb9224bd785111ccd16e7e0aec4d1
Author: Stefan Beller <sbeller@google.com>
Date: Mon Sep 12 20:02:46 2016 -0700
move secure_foo from test.c to file2.c
diff --git a/file2.c b/file2.c
index 9163a0f..8e66dc0 100644
--- a/file2.c
+++ b/file2.c
@@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)
return memcpy(xmallocz(len), data, len);
}
-int secure_foo(struct user *u)
-{
- if (!u->is_allowed_foo)
- return;
- foo(u);
-}
-
char *xstrndup(const char *str, size_t len)
{
char *p = memchr(str, '\0', len);
diff --git a/test.c b/test.c
index a95e6fe..81eb0eb 100644
--- a/test.c
+++ b/test.c
@@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
return total;
}
+int secure_foo(struct user *u)
+{
+ if (!u->is_allowed_foo)
+ return;
+ foo(u);
+}
+
int xdup(int fd)
{
int ret = dup(fd);
as in this patch all lines that add or remove lines
should be colored in the new color that indicates moved
lines.
However the intention of this patch is to aid reviewers
to spotting permutations in the moved code. So consider the
following malicious move:
diff --git a/file2.c b/file2.c
index 9163a0f..8e66dc0 100644
--- a/file2.c
+++ b/file2.c
@@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)
return memcpy(xmallocz(len), data, len);
}
-int secure_foo(struct user *u)
-{
- if (!u->is_allowed_foo)
- return;
- foo(u);
-}
-
char *xstrndup(const char *str, size_t len)
{
char *p = memchr(str, '\0', len);
diff --git a/test.c b/test.c
index a95e6fe..a679c40 100644
--- a/test.c
+++ b/test.c
@@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
return total;
}
+int secure_foo(struct user *u)
+{
+ foo(u);
+ if (!u->is_allowed_foo)
+ return;
+}
+
int xdup(int fd)
{
int ret = dup(fd);
If the moved code is larger, it is easier to hide some permutation in the
code, which is why we would not want to color all lines as "moved" in this
case. So additionally to coloring lines differently that are added and
removed in the same diff, we need to tweak the algorithm a bit more.
As the reviewers attention should be brought to the places, where the
difference is introduced to the moved code, we will not color a moved
line if the previous line is a different moved line, i.e. in the code
above the line `foo()` is still colored as a regular added line, because
the line before the call to `foo` changed from `return` to `{`. The line
with the condition is also not colored as moved but as a new addition.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/config.txt | 12 +-
Documentation/diff-options.txt | 7 +
contrib/completion/git-completion.bash | 2 +
diff.c | 171 ++++++++++++++++++++----
diff.h | 10 +-
t/t4015-diff-whitespace.sh | 229 +++++++++++++++++++++++++++++++++
6 files changed, 402 insertions(+), 29 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..e0c7b91 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
'git-diff-{asterisk}' plumbing commands. Can be overridden on the
command line with the `--color[=<when>]` option.
+color.moved::
+ A boolean value, whether a diff should color moved lines
+ differently. The moved lines are searched for in the diff only.
+ Duplicated lines from somewhere in the project that are not
+ part of the diff are not colored as moved.
+ Defaults to true.
+
color.diff.<slot>::
Use customized color for diff colorization. `<slot>` specifies
which part of the patch to use the specified color, and is one
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
- `new` (added lines), `commit` (commit headers), or `whitespace`
- (highlighting whitespace errors).
+ `new` (added lines), `commit` (commit headers), `whitespace`
+ (highlighting whitespace errors), `movedFrom` (removed lines that
+ reappear), `movedTo` (added lines that were removed elsewhere).
color.decorate.<slot>::
Use customized color for 'git log --decorate' output. `<slot>` is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a873..13b6a2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -234,6 +234,13 @@ ifdef::git-diff[]
endif::git-diff[]
It is the same as `--color=never`.
+--[no-]color-moved::
+ Show moved blocks in a different color.
+ifdef::git-diff[]
+ It can be changed by the `diff.ui` and `color.diff`
+ configuration settings.
+endif::git-diff[]
+
--word-diff[=<mode>]::
Show a word diff, using the <mode> to delimit changed words.
By default, words are delimited by whitespace; see
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9c8f738..85032dd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2115,6 +2115,8 @@ _git_config ()
color.diff.old
color.diff.plain
color.diff.whitespace
+ color.diff.movedFrom
+ color.diff.movedTo
color.grep
color.grep.context
color.grep.filename
diff --git a/diff.c b/diff.c
index e240e89..f41cdcf 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_compaction_heuristic; /* experimental */
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
+static int diff_color_moved_default;
static int diff_context_default = 3;
static const char *diff_word_regex_cfg;
static const char *external_diff_cmd_cfg;
@@ -52,6 +53,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW, /* COMMIT */
GIT_COLOR_BG_RED, /* WHITESPACE */
GIT_COLOR_NORMAL, /* FUNCINFO */
+ GIT_COLOR_BLUE, /* MOVED FROM */
+ GIT_COLOR_MAGENTA, /* MOVED TO */
};
static int parse_diff_color_slot(const char *var)
@@ -72,6 +75,10 @@ static int parse_diff_color_slot(const char *var)
return DIFF_WHITESPACE;
if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
+ if (!strcasecmp(var, "movedfrom"))
+ return DIFF_FILE_MOVED_FROM;
+ if (!strcasecmp(var, "movedto"))
+ return DIFF_FILE_MOVED_TO;
return -1;
}
@@ -180,6 +187,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
diff_use_color_default = git_config_colorbool(var, value);
return 0;
}
+ if (!strcmp(var, "color.moved")) {
+ diff_color_moved_default = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "diff.context")) {
diff_context_default = git_config_int(var, value);
if (diff_context_default < 0)
@@ -287,6 +298,41 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
+struct moved_entry {
+ struct hashmap_entry ent;
+ char *line;
+ int hash_prev;
+};
+
+static int moved_entry_cmp(const struct moved_entry *a,
+ const struct moved_entry *b,
+ const void *unused)
+{
+ return strcmp(a->line, b->line) &&
+ a->hash_prev == b->hash_prev;
+}
+
+static struct moved_entry *prepare_entry(const char *line,
+ unsigned long len,
+ int ws_rule,
+ int hash_prev)
+{
+ int i;
+ struct strbuf sb = STRBUF_INIT;
+ struct moved_entry *ret = xmalloc(sizeof(*ret));
+
+ for (i = 0; i < len; i++) {
+ if (ws_rule && isspace(line[i]))
+ continue;
+ strbuf_addch(&sb, line[i]);
+ }
+
+ ret->ent.hash = memhash(sb.buf, sb.len);
+ ret->line = xmemdupz(line, len);
+ ret->hash_prev = hash_prev;
+ return ret;
+}
+
static char *quote_two(const char *one, const char *two)
{
int need_one = quote_c_style(one, NULL, NULL, 1);
@@ -449,12 +495,49 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
+static int should_color_as_moved(struct diff_options *o,
+ struct line_emission *e,
+ struct moved_entry *keydata,
+ struct hashmap *lookup)
+{
+ struct moved_entry *me;
+
+ if (!o->color_moved)
+ return 0;
+ if (e->first != '+' && e->first != '-')
+ return 0;
+
+ me = hashmap_get(lookup, keydata, keydata);
+
+ if (!me)
+ return 0;
+
+ if (me->hash_prev == 0 || o->hash_prev == 0)
+ return 1;
+
+ return (me->hash_prev == o->hash_prev);
+}
+
static void emit_line_emission(struct diff_options *o, struct line_emission *e)
{
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
+ if (o->color_moved && (e->first == '+' || e->first == '-')) {
+ struct moved_entry *keydata;
+ keydata = prepare_entry(e->line, e->len,
+ DIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);
+ if (e->first == '-' &&
+ should_color_as_moved(o, e, keydata, o->added_lines))
+ e->set = diff_get_color_opt(o, DIFF_FILE_MOVED_FROM);
+ if (e->first == '+' &&
+ should_color_as_moved(o, e, keydata, o->deleted_lines))
+ e->set = diff_get_color_opt(o, DIFF_FILE_MOVED_TO);
+
+ o->hash_prev = keydata->ent.hash;
+ }
+
if (e->len || e->first) {
if (e->set)
fputs(e->set, file);
@@ -568,67 +651,95 @@ static void emit_line_checked(const char *reset,
unsigned ws_error_highlight,
char sign)
{
+ int i;
+ struct line_emission *e;
const char *set = diff_get_color(ecbdata->color_diff, color);
const char *ws = NULL;
+ struct diff_options *o = ecbdata->opt;
- if (ecbdata->opt->ws_error_highlight & ws_error_highlight) {
+ if (o->ws_error_highlight & ws_error_highlight) {
ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
if (!*ws)
ws = NULL;
}
if (!ws)
- emit_line_0(ecbdata->opt, set, reset, sign, line, len);
+ emit_line_0(o, set, reset, sign, line, len);
else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
/* Blank line at EOF - paint '+' as well */
- emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
+ emit_line_0(o, ws, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- if (ecbdata->opt->use_buffer) {
- struct line_emission e;
- e.line = line;
- e.len = len;
- e.ws_rule = ecbdata->ws_rule;
- e.set = set;
- e.reset = reset;
- e.ws = ws;
- e.whitespace_check = 1;
- e.first = sign;
- append_line_emission_to_buffer(ecbdata->opt, &e);
+ if (o->use_buffer) {
+ emit_line_0(o, set, reset, sign, line, len);
+ i = o->line_buffer_nr - 1;
+ e = &o->line_buffer[i];
+ e->whitespace_check = 1;
+ e->ws_rule = ecbdata->ws_rule;
+ e->ws = ws;
} else {
- emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
+ emit_line_0(o, set, reset, sign, "", 0);
ws_check_emit(line, len, ecbdata->ws_rule,
- ecbdata->opt->file, set, reset, ws);
+ o->file, set, reset, ws);
}
}
+ if (o->color_moved) {
+ i = o->line_buffer_nr - 1;
+ e = &o->line_buffer[i];
+ }
}
static void emit_add_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
+ struct diff_options *o = ecbdata->opt;
ecbdata->lno_in_postimage++;
emit_line_checked(reset, ecbdata, line, len,
DIFF_FILE_NEW, WSEH_NEW, '+');
+ if (o->color_moved) {
+ struct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];
+ struct moved_entry *keydata = prepare_entry(l->line, l->len,
+ DIFF_XDL_TST(o, IGNORE_WHITESPACE),
+ o->hash_prev);
+ l = &o->line_buffer[o->line_buffer_nr - 2];
+ if (l->first == '+')
+ keydata->hash_prev = o->hash_prev;
+ hashmap_add(o->added_lines, keydata);
+ o->hash_prev = keydata->ent.hash;
+ }
}
static void emit_del_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
+ struct diff_options *o = ecbdata->opt;
ecbdata->lno_in_preimage++;
emit_line_checked(reset, ecbdata, line, len,
DIFF_FILE_OLD, WSEH_OLD, '-');
+ if (o->color_moved) {
+ struct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];
+ struct moved_entry *keydata = prepare_entry(l->line, l->len,
+ DIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);
+ l = &o->line_buffer[o->line_buffer_nr - 2];
+ if (l->first == '-')
+ keydata->hash_prev = o->hash_prev;
+ hashmap_add(o->deleted_lines, keydata);
+ o->hash_prev = keydata->ent.hash;
+ }
}
static void emit_context_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
+ struct diff_options *o = ecbdata->opt;
ecbdata->lno_in_postimage++;
ecbdata->lno_in_preimage++;
emit_line_checked(reset, ecbdata, line, len,
DIFF_CONTEXT, WSEH_CONTEXT, ' ');
+ o->hash_prev = 0;
}
static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3389,6 +3500,7 @@ void diff_setup(struct diff_options *options)
options->change = diff_change;
options->add_remove = diff_addremove;
options->use_color = diff_use_color_default;
+ options->color_moved = diff_color_moved_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
if (diff_compaction_heuristic)
@@ -3511,6 +3623,9 @@ void diff_setup_done(struct diff_options *options)
if (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)
die(_("--follow requires exactly one pathspec"));
+
+ if (!options->use_color || external_diff())
+ options->color_moved = 0;
}
static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
@@ -3961,6 +4076,10 @@ int diff_opt_parse(struct diff_options *options,
}
else if (!strcmp(arg, "--no-color"))
options->use_color = 0;
+ else if (!strcmp(arg, "--color-moved"))
+ options->color_moved = 1;
+ else if (!strcmp(arg, "--no-color-moved"))
+ options->color_moved = 0;
else if (!strcmp(arg, "--color-words")) {
options->use_color = 1;
options->word_diff = DIFF_WORDS_COLOR;
@@ -4728,16 +4847,15 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
- /*
- * TODO:
- * For testing purposes we want to make sure the diff machinery
- * works with the buffer. If there is anything emitted outside the
- * emit_line_emission, then the order is screwed up and the tests
- * will fail.
- *
- * We'll unset this flag in a later patch.
- */
- o->use_buffer = 1;
+
+ if (o->color_moved) {
+ o->use_buffer = 1;
+ o->deleted_lines = xmallocz(sizeof(*o->deleted_lines));
+ o->added_lines = xmallocz(sizeof(*o->added_lines));
+ hashmap_init(o->deleted_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);
+ hashmap_init(o->added_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);
+ }
+
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (check_pair_status(p))
@@ -4827,6 +4945,7 @@ void diff_flush(struct diff_options *options)
if (!options->file)
die_errno("Could not open /dev/null");
options->close_file = 1;
+ options->color_moved = 0;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (check_pair_status(p))
diff --git a/diff.h b/diff.h
index 4df6aa5..8c3f8f7 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
#include "tree-walk.h"
#include "pathspec.h"
#include "object.h"
+#include "hashmap.h"
struct rev_info;
struct diff_options;
@@ -193,9 +194,14 @@ struct diff_options {
int diff_path_counter;
+ int color_moved;
int use_buffer;
struct line_emission *line_buffer;
int line_buffer_nr, line_buffer_alloc;
+
+ struct hashmap *deleted_lines;
+ struct hashmap *added_lines;
+ int hash_prev;
};
void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
@@ -210,7 +216,9 @@ enum color_diff {
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
DIFF_WHITESPACE = 7,
- DIFF_FUNCINFO = 8
+ DIFF_FUNCINFO = 8,
+ DIFF_FILE_MOVED_TO = 9,
+ DIFF_FILE_MOVED_FROM = 10
};
const char *diff_get_color(int diff_use_color, enum color_diff ix);
#define diff_get_color_opt(o, ix) \
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157..2e3d7c9 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -934,4 +934,233 @@ test_expect_success 'the same with --ws-error-highlight' '
test_cmp expected current
'
+test_expect_success 'detect moved code, complete file' '
+ git reset --hard &&
+ cat <<-\EOF >test.c &&
+ #include<stdio.h>
+ main()
+ {
+ printf("Hello World");
+ }
+ EOF
+ git add test.c &&
+ git commit -m "add main function" &&
+ git mv test.c main.c &&
+ git diff HEAD --color-moved --no-renames | test_decode_color >actual &&
+ cat >expected <<-\EOF &&
+ <BOLD>diff --git a/main.c b/main.c<RESET>
+ <BOLD>new file mode 100644<RESET>
+ <BOLD>index 0000000..a986c57<RESET>
+ <BOLD>--- /dev/null<RESET>
+ <BOLD>+++ b/main.c<RESET>
+ <CYAN>@@ -0,0 +1,5 @@<RESET>
+ <BLUE>+<RESET><BLUE>#include<stdio.h><RESET>
+ <BLUE>+<RESET><BLUE>main()<RESET>
+ <BLUE>+<RESET><BLUE>{<RESET>
+ <BLUE>+<RESET><BLUE>printf("Hello World");<RESET>
+ <BLUE>+<RESET><BLUE>}<RESET>
+ <BOLD>diff --git a/test.c b/test.c<RESET>
+ <BOLD>deleted file mode 100644<RESET>
+ <BOLD>index a986c57..0000000<RESET>
+ <BOLD>--- a/test.c<RESET>
+ <BOLD>+++ /dev/null<RESET>
+ <CYAN>@@ -1,5 +0,0 @@<RESET>
+ <MAGENTA>-#include<stdio.h><RESET>
+ <MAGENTA>-main()<RESET>
+ <MAGENTA>-{<RESET>
+ <MAGENTA>-printf("Hello World");<RESET>
+ <MAGENTA>-}<RESET>
+ EOF
+
+ test_cmp expected actual
+'
+
+test_expect_success 'detect moved code, inside file' '
+ git reset --hard &&
+ cat <<-\EOF >main.c &&
+ #include<stdio.h>
+ int stuff()
+ {
+ printf("Hello ");
+ printf("World\n");
+ }
+
+ int secure_foo(struct user *u)
+ {
+ if (!u->is_allowed_foo)
+ return;
+ foo(u);
+ }
+
+ int main()
+ {
+ foo();
+ }
+ EOF
+ cat <<-\EOF >test.c &&
+ #include<stdio.h>
+ int bar()
+ {
+ printf("Hello World, but different\n");
+ }
+
+ int another_function()
+ {
+ bar();
+ }
+ EOF
+ git add main.c test.c &&
+ git commit -m "add main and test file" &&
+ cat <<-\EOF >main.c &&
+ #include<stdio.h>
+ int stuff()
+ {
+ printf("Hello ");
+ printf("World\n");
+ }
+
+ int main()
+ {
+ foo();
+ }
+ EOF
+ cat <<-\EOF >test.c &&
+ #include<stdio.h>
+ int bar()
+ {
+ printf("Hello World, but different\n");
+ }
+
+ int secure_foo(struct user *u)
+ {
+ if (!u->is_allowed_foo)
+ return;
+ foo(u);
+ }
+
+ int another_function()
+ {
+ bar();
+ }
+ EOF
+ git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
+ cat <<-\EOF >expected &&
+ <BOLD>diff --git a/main.c b/main.c<RESET>
+ <BOLD>index 27a619c..7cf9336 100644<RESET>
+ <BOLD>--- a/main.c<RESET>
+ <BOLD>+++ b/main.c<RESET>
+ <CYAN>@@ -5,13 +5,6 @@<RESET> <RESET>printf("Hello ");<RESET>
+ printf("World\n");<RESET>
+ }<RESET>
+ <RESET>
+ <MAGENTA>-int secure_foo(struct user *u)<RESET>
+ <MAGENTA>-{<RESET>
+ <MAGENTA>-if (!u->is_allowed_foo)<RESET>
+ <MAGENTA>-return;<RESET>
+ <MAGENTA>-foo(u);<RESET>
+ <MAGENTA>-}<RESET>
+ <MAGENTA>-<RESET>
+ int main()<RESET>
+ {<RESET>
+ foo();<RESET>
+ <BOLD>diff --git a/test.c b/test.c<RESET>
+ <BOLD>index 1dc1d85..e34eb69 100644<RESET>
+ <BOLD>--- a/test.c<RESET>
+ <BOLD>+++ b/test.c<RESET>
+ <CYAN>@@ -4,6 +4,13 @@<RESET> <RESET>int bar()<RESET>
+ printf("Hello World, but different\n");<RESET>
+ }<RESET>
+ <RESET>
+ <BLUE>+<RESET><BLUE>int secure_foo(struct user *u)<RESET>
+ <BLUE>+<RESET><BLUE>{<RESET>
+ <BLUE>+<RESET><BLUE>if (!u->is_allowed_foo)<RESET>
+ <BLUE>+<RESET><BLUE>return;<RESET>
+ <BLUE>+<RESET><BLUE>foo(u);<RESET>
+ <BLUE>+<RESET><BLUE>}<RESET>
+ <BLUE>+<RESET>
+ int another_function()<RESET>
+ {<RESET>
+ bar();<RESET>
+ EOF
+
+ test_cmp expected actual
+'
+
+test_expect_success 'detect permutations inside moved code, ' '
+ # reusing the move example from last test:
+ cat <<-\EOF >main.c &&
+ #include<stdio.h>
+ int stuff()
+ {
+ printf("Hello ");
+ printf("World\n");
+ }
+
+ int main()
+ {
+ foo();
+ }
+ EOF
+ cat <<-\EOF >test.c &&
+ #include<stdio.h>
+ int bar()
+ {
+ printf("Hello World, but different\n");
+ }
+
+ int secure_foo(struct user *u)
+ {
+ foo(u);
+ if (!u->is_allowed_foo)
+ return;
+ }
+
+ int another_function()
+ {
+ bar();
+ }
+ EOF
+ git diff HEAD --no-renames --color-moved| test_decode_color >actual &&
+ cat <<-\EOF >expected &&
+ <BOLD>diff --git a/main.c b/main.c<RESET>
+ <BOLD>index 27a619c..7cf9336 100644<RESET>
+ <BOLD>--- a/main.c<RESET>
+ <BOLD>+++ b/main.c<RESET>
+ <CYAN>@@ -5,13 +5,6 @@<RESET> <RESET>printf("Hello ");<RESET>
+ printf("World\n");<RESET>
+ }<RESET>
+ <RESET>
+ <MAGENTA>-int secure_foo(struct user *u)<RESET>
+ <MAGENTA>-{<RESET>
+ <RED>-if (!u->is_allowed_foo)<RESET>
+ <MAGENTA>-return;<RESET>
+ <RED>-foo(u);<RESET>
+ <RED>-}<RESET>
+ <MAGENTA>-<RESET>
+ int main()<RESET>
+ {<RESET>
+ foo();<RESET>
+ <BOLD>diff --git a/test.c b/test.c<RESET>
+ <BOLD>index 1dc1d85..2bedec9 100644<RESET>
+ <BOLD>--- a/test.c<RESET>
+ <BOLD>+++ b/test.c<RESET>
+ <CYAN>@@ -4,6 +4,13 @@<RESET> <RESET>int bar()<RESET>
+ printf("Hello World, but different\n");<RESET>
+ }<RESET>
+ <RESET>
+ <BLUE>+<RESET><BLUE>int secure_foo(struct user *u)<RESET>
+ <BLUE>+<RESET><BLUE>{<RESET>
+ <GREEN>+<RESET><GREEN>foo(u);<RESET>
+ <GREEN>+<RESET><GREEN>if (!u->is_allowed_foo)<RESET>
+ <BLUE>+<RESET><BLUE>return;<RESET>
+ <GREEN>+<RESET><GREEN>}<RESET>
+ <BLUE>+<RESET>
+ int another_function()<RESET>
+ {<RESET>
+ bar();<RESET>
+ EOF
+
+ test_cmp expected actual
+'
+
test_done
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 14/17] diff.c: convert diff_flush to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers diff_flush.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index 3940f79..2b1ebcc 100644
--- a/diff.c
+++ b/diff.c
@@ -4737,12 +4737,13 @@ void diff_flush(struct diff_options *options)
if (output_format & DIFF_FORMAT_PATCH) {
if (separator) {
- fprintf(options->file, "%s%c",
- diff_line_prefix(options),
- options->line_termination);
+ emit_line_0(options, NULL, NULL,
+ options->line_termination, "", 0);
if (options->stat_sep) {
/* attach patch instead of inline */
- fputs(options->stat_sep, options->file);
+ emit_line_0(options, NULL, NULL, 0,
+ options->stat_sep,
+ strlen(options->stat_sep));
}
}
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This prepares the code for submodules to go through the
emit_line_0 function.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 5 ++---
diff.h | 3 +++
submodule.c | 42 +++++++++++++++++-------------------------
submodule.h | 3 +--
4 files changed, 23 insertions(+), 30 deletions(-)
diff --git a/diff.c b/diff.c
index 3aa99d1..1892c2b 100644
--- a/diff.c
+++ b/diff.c
@@ -493,7 +493,7 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset
emit_line_0(o, set, reset, line[0], line+1, len-1);
}
-static void emit_line_fmt(struct diff_options *o,
+void emit_line_fmt(struct diff_options *o,
const char *set, const char *reset,
const char *fmt, ...)
{
@@ -2306,8 +2306,7 @@ static void builtin_diff(const char *name_a,
(!two->mode || S_ISGITLINK(two->mode))) {
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
- show_submodule_summary(o->file, one->path ? one->path : two->path,
- line_prefix,
+ show_submodule_summary(o, one->path ? one->path : two->path,
one->oid.hash, two->oid.hash,
two->dirty_submodule,
meta, del, add, reset);
diff --git a/diff.h b/diff.h
index 7883729..1699d9c 100644
--- a/diff.h
+++ b/diff.h
@@ -180,6 +180,9 @@ struct diff_options {
int diff_path_counter;
};
+void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,
+ const char *fmt, ...);
+
enum color_diff {
DIFF_RESET = 0,
DIFF_CONTEXT = 1,
diff --git a/submodule.c b/submodule.c
index 1b5cdfb..c817b46 100644
--- a/submodule.c
+++ b/submodule.c
@@ -304,8 +304,8 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
return prepare_revision_walk(rev);
}
-static void print_submodule_summary(struct rev_info *rev, FILE *f,
- const char *line_prefix,
+static void print_submodule_summary(struct rev_info *rev,
+ struct diff_options *o,
const char *del, const char *add, const char *reset)
{
static const char format[] = " %m %s";
@@ -317,24 +317,16 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
ctx.date_mode = rev->date_mode;
ctx.output_encoding = get_log_output_encoding();
strbuf_setlen(&sb, 0);
- strbuf_addstr(&sb, line_prefix);
- if (commit->object.flags & SYMMETRIC_LEFT) {
- if (del)
- strbuf_addstr(&sb, del);
- }
- else if (add)
- strbuf_addstr(&sb, add);
format_commit_message(commit, format, &sb, &ctx);
- if (reset)
- strbuf_addstr(&sb, reset);
- strbuf_addch(&sb, '\n');
- fprintf(f, "%s", sb.buf);
+ if (commit->object.flags & SYMMETRIC_LEFT)
+ emit_line_fmt(o, del, reset, "%s\n", sb.buf);
+ else if (add)
+ emit_line_fmt(o, add, reset, "%s\n", sb.buf);
}
strbuf_release(&sb);
}
-void show_submodule_summary(FILE *f, const char *path,
- const char *line_prefix,
+void show_submodule_summary(struct diff_options *o, const char *path,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
@@ -359,30 +351,30 @@ void show_submodule_summary(FILE *f, const char *path,
message = "(revision walker failed)";
if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
- fprintf(f, "%sSubmodule %s contains untracked content\n",
- line_prefix, path);
+ emit_line_fmt(o, 0, 0,
+ "Submodule %s contains untracked content\n", path);
if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
- fprintf(f, "%sSubmodule %s contains modified content\n",
- line_prefix, path);
+ emit_line_fmt(o, 0, 0,
+ "Submodule %s contains modified content\n", path);
if (!hashcmp(one, two)) {
strbuf_release(&sb);
return;
}
- strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
- find_unique_abbrev(one, DEFAULT_ABBREV));
+ strbuf_addf(&sb, "Submodule %s %s..", path,
+ find_unique_abbrev(one, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
if (message)
- strbuf_addf(&sb, " %s%s\n", message, reset);
+ strbuf_addf(&sb, " %s\n", message);
else
- strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
- fwrite(sb.buf, sb.len, 1, f);
+ strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+ emit_line_fmt(o, meta, reset, "%s", sb.buf);
if (!message) /* only NULL if we succeeded in setting up the walk */
- print_submodule_summary(&rev, f, line_prefix, del, add, reset);
+ print_submodule_summary(&rev, o, del, add, reset);
if (left)
clear_commit_marks(left, ~0);
if (right)
diff --git a/submodule.h b/submodule.h
index 2af9390..fefc0ab 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,8 +41,7 @@ int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
- const char *line_prefix,
+void show_submodule_summary(struct diff_options *o, const char *path,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 12/17] diff.c: convert show_stats to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
We call print_stat_summary from builtin/apply, so we still
need the version with a file pointer, so introduce
print_stat_summary_0 that uses emit_line_* machinery and
keep print_stat_summary with the same arguments around.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 87 +++++++++++++++++++++++++++++++++++++-----------------------------
diff.h | 4 +--
2 files changed, 51 insertions(+), 40 deletions(-)
diff --git a/diff.c b/diff.c
index ff1e829..00a5a4e 100644
--- a/diff.c
+++ b/diff.c
@@ -1465,20 +1465,19 @@ static int scale_linear(int it, int width, int max_change)
return 1 + (it * (width - 1) / max_change);
}
-static void show_name(FILE *file,
+static void show_name(struct strbuf *out,
const char *prefix, const char *name, int len)
{
- fprintf(file, " %s%-*s |", prefix, len, name);
+ strbuf_addf(out, " %s%-*s |", prefix, len, name);
}
-static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)
+static void show_graph(struct strbuf *out, char ch, int cnt, const char *set, const char *reset)
{
if (cnt <= 0)
return;
- fprintf(file, "%s", set);
- while (cnt--)
- putc(ch, file);
- fprintf(file, "%s", reset);
+ strbuf_addstr(out, set);
+ strbuf_addchars(out, ch, cnt);
+ strbuf_addstr(out, reset);
}
static void fill_print_name(struct diffstat_file *file)
@@ -1502,14 +1501,16 @@ static void fill_print_name(struct diffstat_file *file)
file->print_name = pname;
}
-int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
+void print_stat_summary_0(struct diff_options *options, int files,
+ int insertions, int deletions)
{
struct strbuf sb = STRBUF_INIT;
- int ret;
if (!files) {
assert(insertions == 0 && deletions == 0);
- return fprintf(fp, "%s\n", " 0 files changed");
+ strbuf_addstr(&sb, " 0 files changed");
+ emit_line_0(options, 0, 0, 0, sb.buf, sb.len);
+ return;
}
strbuf_addf(&sb,
@@ -1536,9 +1537,17 @@ int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
deletions);
}
strbuf_addch(&sb, '\n');
- ret = fputs(sb.buf, fp);
+ emit_line_0(options, 0, 0, 0, sb.buf, sb.len);
strbuf_release(&sb);
- return ret;
+}
+
+void print_stat_summary(FILE *fp, int files,
+ int insertions, int deletions)
+{
+ struct diff_options o;
+ memset(&o, 0, sizeof(o));
+ o.file = fp;
+ print_stat_summary_0(&o, files, insertions, deletions);
}
static void show_stats(struct diffstat_t *data, struct diff_options *options)
@@ -1548,13 +1557,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
int total_files = data->nr, count;
int width, name_width, graph_width, number_width = 0, bin_width = 0;
const char *reset, *add_c, *del_c;
- const char *line_prefix = "";
int extra_shown = 0;
+ struct strbuf out = STRBUF_INIT;
if (data->nr == 0)
return;
- line_prefix = diff_line_prefix(options);
count = options->stat_count ? options->stat_count : data->nr;
reset = diff_get_color_opt(options, DIFF_RESET);
@@ -1708,26 +1716,29 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
}
if (file->is_binary) {
- fprintf(options->file, "%s", line_prefix);
- show_name(options->file, prefix, name, len);
- fprintf(options->file, " %*s", number_width, "Bin");
+ show_name(&out, prefix, name, len);
+ strbuf_addf(&out, " %*s", number_width, "Bin");
if (!added && !deleted) {
- putc('\n', options->file);
+ strbuf_addch(&out, '\n');
+ emit_line(options, 0, 0, out.buf, out.len);
+ strbuf_reset(&out);
continue;
}
- fprintf(options->file, " %s%"PRIuMAX"%s",
+ strbuf_addf(&out, " %s%"PRIuMAX"%s",
del_c, deleted, reset);
- fprintf(options->file, " -> ");
- fprintf(options->file, "%s%"PRIuMAX"%s",
+ strbuf_addstr(&out, " -> ");
+ strbuf_addf(&out, "%s%"PRIuMAX"%s",
add_c, added, reset);
- fprintf(options->file, " bytes");
- fprintf(options->file, "\n");
+ strbuf_addstr(&out, " bytes\n");
+ emit_line(options, 0, 0, out.buf, out.len);
+ strbuf_reset(&out);
continue;
}
else if (file->is_unmerged) {
- fprintf(options->file, "%s", line_prefix);
- show_name(options->file, prefix, name, len);
- fprintf(options->file, " Unmerged\n");
+ show_name(&out, prefix, name, len);
+ strbuf_addstr(&out, " Unmerged\n");
+ emit_line(options, 0, 0, out.buf, out.len);
+ strbuf_reset(&out);
continue;
}
@@ -1750,14 +1761,15 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
add = total - del;
}
}
- fprintf(options->file, "%s", line_prefix);
- show_name(options->file, prefix, name, len);
- fprintf(options->file, " %*"PRIuMAX"%s",
+ show_name(&out, prefix, name, len);
+ strbuf_addf(&out, " %*"PRIuMAX"%s",
number_width, added + deleted,
added + deleted ? " " : "");
- show_graph(options->file, '+', add, add_c, reset);
- show_graph(options->file, '-', del, del_c, reset);
- fprintf(options->file, "\n");
+ show_graph(&out, '+', add, add_c, reset);
+ show_graph(&out, '-', del, del_c, reset);
+ strbuf_addch(&out, '\n');
+ emit_line(options, 0, 0, out.buf, out.len);
+ strbuf_reset(&out);
}
for (i = 0; i < data->nr; i++) {
@@ -1778,11 +1790,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (i < count)
continue;
if (!extra_shown)
- fprintf(options->file, "%s ...\n", line_prefix);
+ emit_line_0(options, 0, 0, 0, " ...\n", strlen(" ...\n"));
extra_shown = 1;
}
- fprintf(options->file, "%s", line_prefix);
- print_stat_summary(options->file, total_files, adds, dels);
+
+ print_stat_summary_0(options, total_files, adds, dels);
}
static void show_shortstats(struct diffstat_t *data, struct diff_options *options)
@@ -1794,7 +1806,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
for (i = 0; i < data->nr; i++) {
int added = data->files[i]->added;
- int deleted= data->files[i]->deleted;
+ int deleted = data->files[i]->deleted;
if (data->files[i]->is_unmerged ||
(!data->files[i]->is_interesting && (added + deleted == 0))) {
@@ -1804,8 +1816,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
dels += deleted;
}
}
- fprintf(options->file, "%s", diff_line_prefix(options));
- print_stat_summary(options->file, total_files, adds, dels);
+ print_stat_summary_0(options, total_files, adds, dels);
}
static void show_numstat(struct diffstat_t *data, struct diff_options *options)
diff --git a/diff.h b/diff.h
index 1699d9c..cc5d038 100644
--- a/diff.h
+++ b/diff.h
@@ -379,8 +379,8 @@ extern int parse_rename_score(const char **cp_p);
extern long parse_algorithm_value(const char *value);
-extern int print_stat_summary(FILE *fp, int files,
- int insertions, int deletions);
+extern void print_stat_summary(FILE *fp, int files,
+ int insertions, int deletions);
extern void setup_diff_pager(struct diff_options *);
#endif /* DIFF_H */
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 13/17] diff.c: convert word diffing to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers all code related to diffing words.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 56 ++++++++++++++++++++++++++++----------------------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/diff.c b/diff.c
index 00a5a4e..3940f79 100644
--- a/diff.c
+++ b/diff.c
@@ -828,37 +828,38 @@ struct diff_words_data {
struct diff_words_style *style;
};
-static int fn_out_diff_words_write_helper(FILE *fp,
+static int fn_out_diff_words_write_helper(struct diff_options *o,
struct diff_words_style_elem *st_el,
const char *newline,
size_t count, const char *buf,
const char *line_prefix)
{
- int print = 0;
+ struct strbuf sb = STRBUF_INIT;
while (count) {
char *p = memchr(buf, '\n', count);
- if (print)
- fputs(line_prefix, fp);
+
if (p != buf) {
- if (st_el->color && fputs(st_el->color, fp) < 0)
- return -1;
- if (fputs(st_el->prefix, fp) < 0 ||
- fwrite(buf, p ? p - buf : count, 1, fp) != 1 ||
- fputs(st_el->suffix, fp) < 0)
- return -1;
- if (st_el->color && *st_el->color
- && fputs(GIT_COLOR_RESET, fp) < 0)
- return -1;
+ if (st_el->color)
+ strbuf_addstr(&sb, st_el->color);
+ strbuf_addstr(&sb, st_el->prefix);
+ strbuf_add(&sb, buf, p ? p - buf : count);
+ strbuf_addstr(&sb, st_el->suffix);
+ if (st_el->color && *st_el->color)
+ strbuf_addstr(&sb, GIT_COLOR_RESET);
}
if (!p)
- return 0;
- if (fputs(newline, fp) < 0)
- return -1;
+ goto out;
+ strbuf_addstr(&sb, newline);
+ emit_line_0(o, 0, 0, 0, sb.buf, sb.len);
+ strbuf_reset(&sb);
count -= p + 1 - buf;
buf = p + 1;
- print = 1;
}
+out:
+ if (sb.len)
+ emit_line_0(o, 0, 0, 0, sb.buf, sb.len);
+ strbuf_release(&sb);
return 0;
}
@@ -936,25 +937,25 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
} else
plus_begin = plus_end = diff_words->plus.orig[plus_first].end;
- if (color_words_output_graph_prefix(diff_words)) {
- fputs(line_prefix, diff_words->opt->file);
- }
+ if (color_words_output_graph_prefix(diff_words))
+ emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
+
if (diff_words->current_plus != plus_begin) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words->opt,
&style->ctx, style->newline,
plus_begin - diff_words->current_plus,
diff_words->current_plus, line_prefix);
if (*(plus_begin - 1) == '\n')
- fputs(line_prefix, diff_words->opt->file);
+ emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
}
if (minus_begin != minus_end) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words->opt,
&style->old, style->newline,
minus_end - minus_begin, minus_begin,
line_prefix);
}
if (plus_begin != plus_end) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words->opt,
&style->new, style->newline,
plus_end - plus_begin, plus_begin,
line_prefix);
@@ -1050,8 +1051,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
/* special case: only removal */
if (!diff_words->plus.text.size) {
- fputs(line_prefix, diff_words->opt->file);
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words->opt,
&style->old, style->newline,
diff_words->minus.text.size,
diff_words->minus.text.ptr, line_prefix);
@@ -1077,8 +1077,8 @@ static void diff_words_show(struct diff_words_data *diff_words)
if (diff_words->current_plus != diff_words->plus.text.ptr +
diff_words->plus.text.size) {
if (color_words_output_graph_prefix(diff_words))
- fputs(line_prefix, diff_words->opt->file);
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ emit_line_0(diff_words->opt, 0, 0, 0, "", 0);
+ fn_out_diff_words_write_helper(diff_words->opt,
&style->ctx, style->newline,
diff_words->plus.text.ptr + diff_words->plus.text.size
- diff_words->current_plus, diff_words->current_plus,
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 09/17] diff.c: convert emit_rewrite_lines to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers emit_rewrite_lines.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index ee4c9d4..3aa99d1 100644
--- a/diff.c
+++ b/diff.c
@@ -690,7 +690,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
if (!endp) {
const char *context = diff_get_color(ecb->color_diff,
DIFF_CONTEXT);
- putc('\n', ecb->opt->file);
+ emit_line_0(ecb->opt, 0, 0, 0, "\n", 1);
emit_line_0(ecb->opt, context, reset, '\\',
nneof, strlen(nneof));
}
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers the remaining parts of fn_out_consume.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 5d57130..b4f4b75 100644
--- a/diff.c
+++ b/diff.c
@@ -493,6 +493,19 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset
emit_line_0(o, set, reset, line[0], line+1, len-1);
}
+static void emit_line_fmt(struct diff_options *o,
+ const char *set, const char *reset,
+ const char *fmt, ...)
+{
+ struct strbuf sb = STRBUF_INIT;
+ va_list ap;
+ va_start(ap, fmt);
+ strbuf_vaddf(&sb, fmt, ap);
+ va_end(ap);
+
+ emit_line(o, set, reset, sb.buf, sb.len);
+}
+
static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
{
if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
@@ -1217,7 +1230,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
struct diff_options *o = ecbdata->opt;
- const char *line_prefix = diff_line_prefix(o);
o->found_changes = 1;
@@ -1229,14 +1241,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
if (ecbdata->label_path[0]) {
const char *name_a_tab, *name_b_tab;
-
name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
-
- fprintf(o->file, "%s%s--- %s%s%s\n",
- line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
- fprintf(o->file, "%s%s+++ %s%s%s\n",
- line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
+ emit_line_fmt(o, meta, reset, "--- %s%s\n",
+ ecbdata->label_path[0], name_a_tab);
+ emit_line_fmt(o, meta, reset, "+++ %s%s\n",
+ ecbdata->label_path[1], name_b_tab);
ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
}
@@ -1277,7 +1287,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
diff_words_flush(ecbdata);
if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
emit_line(o, context, reset, line, len);
- fputs("~\n", o->file);
+ emit_line(o, 0, 0, "~\n", 2);
} else {
/*
* Skip the prefix character, if any. With
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 07/17] diff.c: convert builtin_diff to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers builtin_diff.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/diff.c b/diff.c
index b4f4b75..cbd4ba7 100644
--- a/diff.c
+++ b/diff.c
@@ -1234,8 +1234,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
o->found_changes = 1;
if (ecbdata->header) {
- fprintf(o->file, "%s", ecbdata->header->buf);
- strbuf_reset(ecbdata->header);
+ emit_line(o, 0, 0, ecbdata->header->buf, ecbdata->header->len);
+ strbuf_release(ecbdata->header);
ecbdata->header = NULL;
}
@@ -2333,7 +2333,7 @@ static void builtin_diff(const char *name_a,
b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
- strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
+ strbuf_addf(&header, "%sdiff --git %s %s%s\n", meta, a_one, b_two, reset);
if (lbl[0][0] == '/') {
/* /dev/null */
strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
@@ -2365,7 +2365,7 @@ static void builtin_diff(const char *name_a,
if (complete_rewrite &&
(textconv_one || !diff_filespec_is_binary(one)) &&
(textconv_two || !diff_filespec_is_binary(two))) {
- fprintf(o->file, "%s", header.buf);
+ emit_line(o, 0, 0, header.buf, header.len);
strbuf_reset(&header);
emit_rewrite_diff(name_a, name_b, one, two,
textconv_one, textconv_two, o);
@@ -2375,7 +2375,8 @@ static void builtin_diff(const char *name_a,
}
if (o->irreversible_delete && lbl[1][0] == '/') {
- fprintf(o->file, "%s", header.buf);
+ if (header.len)
+ emit_line(o, 0, 0, header.buf, header.len);
strbuf_reset(&header);
goto free_ab_and_return;
} else if (!DIFF_OPT_TST(o, TEXT) &&
@@ -2385,13 +2386,14 @@ static void builtin_diff(const char *name_a,
S_ISREG(one->mode) && S_ISREG(two->mode) &&
!DIFF_OPT_TST(o, BINARY)) {
if (!oidcmp(&one->oid, &two->oid)) {
- if (must_show_header)
- fprintf(o->file, "%s", header.buf);
+ if (must_show_header && header.len)
+ emit_line(o, 0, 0, header.buf, header.len);
goto free_ab_and_return;
}
- fprintf(o->file, "%s", header.buf);
- fprintf(o->file, "%sBinary files %s and %s differ\n",
- line_prefix, lbl[0], lbl[1]);
+ if (header.len)
+ emit_line(o, 0, 0, header.buf, header.len);
+ emit_line_fmt(o, 0, 0, "Binary files %s and %s differ\n",
+ lbl[0], lbl[1]);
goto free_ab_and_return;
}
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
@@ -2399,17 +2401,18 @@ static void builtin_diff(const char *name_a,
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size)) {
- if (must_show_header)
- fprintf(o->file, "%s", header.buf);
+ if (must_show_header && header.len)
+ emit_line(o, 0, 0, header.buf, header.len);
goto free_ab_and_return;
}
- fprintf(o->file, "%s", header.buf);
+ if (header.len)
+ emit_line(o, 0, 0, header.buf, header.len);
strbuf_reset(&header);
if (DIFF_OPT_TST(o, BINARY))
emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
else
- fprintf(o->file, "%sBinary files %s and %s differ\n",
- line_prefix, lbl[0], lbl[1]);
+ emit_line_fmt(o, 0, 0, "Binary files %s and %s differ\n",
+ lbl[0], lbl[1]);
o->found_changes = 1;
} else {
/* Crazy xdl interfaces.. */
@@ -2420,8 +2423,8 @@ static void builtin_diff(const char *name_a,
struct emit_callback ecbdata;
const struct userdiff_funcname *pe;
- if (must_show_header) {
- fprintf(o->file, "%s", header.buf);
+ if (must_show_header && header.len) {
+ emit_line(o, 0, 0, header.buf, header.len);
strbuf_reset(&header);
}
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 08/17] diff.c: convert emit_rewrite_diff to use emit_line_*
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
This covers emit_rewrite_diff.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/diff.c b/diff.c
index cbd4ba7..ee4c9d4 100644
--- a/diff.c
+++ b/diff.c
@@ -653,17 +653,17 @@ static void remove_tempfile(void)
}
}
-static void print_line_count(FILE *file, int count)
+static void add_line_count(struct strbuf *out, int count)
{
switch (count) {
case 0:
- fprintf(file, "0,0");
+ strbuf_addstr(out, "0,0");
break;
case 1:
- fprintf(file, "1");
+ strbuf_addstr(out, "1");
break;
default:
- fprintf(file, "1,%d", count);
+ strbuf_addf(out, "1,%d", count);
break;
}
}
@@ -714,7 +714,7 @@ static void emit_rewrite_diff(const char *name_a,
char *data_one, *data_two;
size_t size_one, size_two;
struct emit_callback ecbdata;
- const char *line_prefix = diff_line_prefix(o);
+ struct strbuf out = STRBUF_INIT;
if (diff_mnemonic_prefix && DIFF_OPT_TST(o, REVERSE_DIFF)) {
a_prefix = o->b_prefix;
@@ -752,20 +752,23 @@ static void emit_rewrite_diff(const char *name_a,
ecbdata.lno_in_preimage = 1;
ecbdata.lno_in_postimage = 1;
+ emit_line_fmt(o, metainfo, reset, "--- %s%s\n", a_name.buf, name_a_tab);
+ emit_line_fmt(o, metainfo, reset, "+++ %s%s\n", b_name.buf, name_b_tab);
+
lc_a = count_lines(data_one, size_one);
lc_b = count_lines(data_two, size_two);
- fprintf(o->file,
- "%s%s--- %s%s%s\n%s%s+++ %s%s%s\n%s%s@@ -",
- line_prefix, metainfo, a_name.buf, name_a_tab, reset,
- line_prefix, metainfo, b_name.buf, name_b_tab, reset,
- line_prefix, fraginfo);
+
+ strbuf_addstr(&out, "@@ -");
if (!o->irreversible_delete)
- print_line_count(o->file, lc_a);
+ add_line_count(&out, lc_a);
else
- fprintf(o->file, "?,?");
- fprintf(o->file, " +");
- print_line_count(o->file, lc_b);
- fprintf(o->file, " @@%s\n", reset);
+ strbuf_addstr(&out, "?,?");
+ strbuf_addstr(&out, " +");
+ add_line_count(&out, lc_b);
+ strbuf_addstr(&out, " @@\n");
+ emit_line(o, fraginfo, reset, out.buf, out.len);
+ strbuf_release(&out);
+
if (lc_a && !o->irreversible_delete)
emit_rewrite_lines(&ecbdata, '-', data_one, size_one);
if (lc_b)
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch, I want to propose an option to detect&color
moved lines in a diff, which cannot be done in a one-pass over
the diff. Instead we need to go over the whole diff twice,
because we cannot detect the first line of the two corresponding
lines (+ and -) that got moved.
So to prepare the diff machinery for two pass algorithms
(i.e. buffer it all up and then operate on the result),
move all emissions to places, such that the only emitting
function is emit_line_0.
In later patches we may pass lines that are not colored to
the central function emit_line_0, so we
need to emit the color only when it is non-NULL.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index b6a40ae..5d57130 100644
--- a/diff.c
+++ b/diff.c
@@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
}
if (len || !nofirst) {
- fputs(set, file);
+ if (set)
+ fputs(set, file);
if (!nofirst)
fputc(first, file);
fwrite(line, len, 1, file);
- fputs(reset, file);
+ if (reset)
+ fputs(reset, file);
}
if (has_trailing_carriage_return)
fputc('\r', file);
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 04/17] diff.c: factor out diff_flush_patch_all_file_pairs
From: Stefan Beller @ 2016-09-13 4:46 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
In a later patch we want to do more things before and after all filepairs
are flushed. So factor flushing out all file pairs into its own function
that the new code can be plugged in easily.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/diff.c b/diff.c
index 9d2e704..b6a40ae 100644
--- a/diff.c
+++ b/diff.c
@@ -4608,6 +4608,17 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
warning(rename_limit_advice, varname, needed);
}
+static void diff_flush_patch_all_file_pairs(struct diff_options *o)
+{
+ int i;
+ struct diff_queue_struct *q = &diff_queued_diff;
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (check_pair_status(p))
+ diff_flush_patch(p, o);
+ }
+}
+
void diff_flush(struct diff_options *options)
{
struct diff_queue_struct *q = &diff_queued_diff;
@@ -4702,11 +4713,7 @@ void diff_flush(struct diff_options *options)
}
}
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (check_pair_status(p))
- diff_flush_patch(p, options);
- }
+ diff_flush_patch_all_file_pairs(options);
}
if (output_format & DIFF_FORMAT_CALLBACK)
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
* [RFC/PATCH 03/17] diff.c: drop tautologous condition in emit_line_0
From: Stefan Beller @ 2016-09-13 4:45 UTC (permalink / raw)
To: gitster, peff, chriscool; +Cc: git, Stefan Beller
In-Reply-To: <20160913044613.1037-1-sbeller@google.com>
When `first` equals '\r', then it cannot equal '\n', which makes
`!has_trailing_newline` always true if `first` is '\r'.
Drop that check.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index 156c2aa..9d2e704 100644
--- a/diff.c
+++ b/diff.c
@@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
if (len == 0) {
has_trailing_newline = (first == '\n');
- has_trailing_carriage_return = (!has_trailing_newline &&
- (first == '\r'));
+ has_trailing_carriage_return = (first == '\r');
nofirst = has_trailing_newline || has_trailing_carriage_return;
} else {
has_trailing_newline = (len > 0 && line[len-1] == '\n');
--
2.10.0.21.g1da280f.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox