Re: [PATCH v9 5/5] Speed up log -L... -M
On Sat, Mar 23, 2013 at 5:04 AM, Jeff King wrote: > On Sat, Mar 23, 2013 at 06:58:48AM +0100, Thomas Rast wrote: >> Eric Sunshine writes: >> > On Thu, Mar 21, 2013 at 8:52 AM, Thomas Rast wrote: >> >> This is a bit hacky and should really be replaced by equivalent >> >> support in --follow, and just using that. However, in the meantime it >> > >> > s/using/use/ >> >> I'm not a native speaker, but I really think 'using' is more correct >> here. > > Cannot...resist...grammar discussion. > > I think you are both potentially right. > > You might consider the two items "equivalent support" and "using that" > to be two noun phrases that are objects of the preposition "by", and > that the writer simply omits the second "by" after the "and". In which > case you are making a noun phrase from a verb phrase, and would want to > use the gerund form "using". And the sentence, simplifying out some > modifiers and adding the missing "by" (which is fine to omit, but the > parts of speech become much clearer with it there), looks like: > > ...should be replaced by equivalent support, and by using that. > > However, you could also argue that the final clause is a second verb > phrase for "this should" which just omits the extra "should" (which is > also OK in a list. In which case "use" acts as a verb, and parses as: > > ...should be replaced by equivalent support, and this should just use > that. > > So I think it is correct either way, and though it parses slightly > differently, the overall meaning is the same. > > Phew. Totally not worth that much discussion, but for some reason I find > these sorts of ambiguous language cases interesting. Wishing to avoid bike-shedding the commit message, I suggested s/using/use/ as a minor change to help clarify the grammar a bit. However, perhaps it could be rephrased as: This is a bit hacky and should really be replaced by equivalent support in --follow, which can then be employed instead. -- ES -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 5/5] Speed up log -L... -M
On Sat, Mar 23, 2013 at 06:58:48AM +0100, Thomas Rast wrote: > Eric Sunshine writes: > > > On Thu, Mar 21, 2013 at 8:52 AM, Thomas Rast wrote: > >> This is a bit hacky and should really be replaced by equivalent > >> support in --follow, and just using that. However, in the meantime it > > > > s/using/use/ > > I'm not a native speaker, but I really think 'using' is more correct > here. Cannot...resist...grammar discussion. I think you are both potentially right. You might consider the two items "equivalent support" and "using that" to be two noun phrases that are objects of the preposition "by", and that the writer simply omits the second "by" after the "and". In which case you are making a noun phrase from a verb phrase, and would want to use the gerund form "using". And the sentence, simplifying out some modifiers and adding the missing "by" (which is fine to omit, but the parts of speech become much clearer with it there), looks like: ...should be replaced by equivalent support, and by using that. However, you could also argue that the final clause is a second verb phrase for "this should" which just omits the extra "should" (which is also OK in a list. In which case "use" acts as a verb, and parses as: ...should be replaced by equivalent support, and this should just use that. So I think it is correct either way, and though it parses slightly differently, the overall meaning is the same. Phew. Totally not worth that much discussion, but for some reason I find these sorts of ambiguous language cases interesting. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 5/5] Speed up log -L... -M
Eric Sunshine writes:
> On Thu, Mar 21, 2013 at 8:52 AM, Thomas Rast wrote:
>> This is a bit hacky and should really be replaced by equivalent
>> support in --follow, and just using that. However, in the meantime it
>
> s/using/use/
I'm not a native speaker, but I really think 'using' is more correct
here. But feel free to suggest a better wording. The intention is that
we should proceed in two steps: 'git log --follow' first needs to learn
to adjust its pathspec filter as it walks revisions, much like I did
here. Then this patch should be reverted in favor of just enabling
--follow.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 5/5] Speed up log -L... -M
On Thu, Mar 21, 2013 at 8:52 AM, Thomas Rast wrote: > This is a bit hacky and should really be replaced by equivalent > support in --follow, and just using that. However, in the meantime it s/using/use/ > speeds up 'log -M -L' by an order of magnitude. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 5/5] Speed up log -L... -M
So far log -L only used the implicit diff filtering by pathspec. If
the user specifies -M, we cannot do that, and so we simply handed the
whole diff queue (which is approximately 'git show --raw') to
diffcore_std().
Unfortunately this is very slow. We can optimize a lot if we throw
out files that we know cannot possibly be interesting, in the same
spirit that the pathspec filtering reduces the number of files.
However, in this case, we have to be more careful. Because we want to
look out for renames, we need to keep all filepairs where something
was deleted.
This is a bit hacky and should really be replaced by equivalent
support in --follow, and just using that. However, in the meantime it
speeds up 'log -M -L' by an order of magnitude.
Signed-off-by: Thomas Rast
---
line-log.c | 56
1 file changed, 52 insertions(+), 4 deletions(-)
diff --git a/line-log.c b/line-log.c
index 81c8d74..4327327 100644
--- a/line-log.c
+++ b/line-log.c
@@ -750,7 +750,50 @@ static void move_diff_queue(struct diff_queue_struct *dst,
DIFF_QUEUE_CLEAR(src);
}
-static void queue_diffs(struct diff_options *opt,
+static void filter_diffs_for_paths(struct line_log_data *range, int
keep_deletions)
+{
+ int i;
+ struct diff_queue_struct outq;
+ DIFF_QUEUE_CLEAR(&outq);
+
+ for (i = 0; i < diff_queued_diff.nr; i++) {
+ struct diff_filepair *p = diff_queued_diff.queue[i];
+ struct line_log_data *rg = NULL;
+
+ if (!DIFF_FILE_VALID(p->two)) {
+ if (keep_deletions)
+ diff_q(&outq, p);
+ else
+ diff_free_filepair(p);
+ continue;
+ }
+ for (rg = range; rg; rg = rg->next) {
+ if (!strcmp(rg->spec->path, p->two->path))
+ break;
+ }
+ if (rg)
+ diff_q(&outq, p);
+ else
+ diff_free_filepair(p);
+ }
+ free(diff_queued_diff.queue);
+ diff_queued_diff = outq;
+}
+
+static inline int diff_might_be_rename(void)
+{
+ int i;
+ for (i = 0; i < diff_queued_diff.nr; i++)
+ if (!DIFF_FILE_VALID(diff_queued_diff.queue[i]->one)) {
+ /* fprintf(stderr, "diff_might_be_rename found creation
of: %s\n", */
+ /* diff_queued_diff.queue[i]->two->path); */
+ return 1;
+ }
+ return 0;
+}
+
+static void queue_diffs(struct line_log_data *range,
+ struct diff_options *opt,
struct diff_queue_struct *queue,
struct commit *commit, struct commit *parent)
{
@@ -766,7 +809,12 @@ static void queue_diffs(struct diff_options *opt,
DIFF_QUEUE_CLEAR(&diff_queued_diff);
diff_tree(&desc1, &desc2, "", opt);
- diffcore_std(opt);
+ if (opt->detect_rename) {
+ filter_diffs_for_paths(range, 1);
+ if (diff_might_be_rename())
+ diffcore_std(opt);
+ filter_diffs_for_paths(range, 0);
+ }
move_diff_queue(queue, &diff_queued_diff);
if (tree1)
@@ -1050,7 +1098,7 @@ static int process_ranges_ordinary_commit(struct rev_info
*rev, struct commit *c
if (commit->parents)
parent = commit->parents->item;
- queue_diffs(&rev->diffopt, &queue, commit, parent);
+ queue_diffs(range, &rev->diffopt, &queue, commit, parent);
changed = process_all_files(&parent_range, rev, &queue, range);
if (parent)
add_line_range(rev, parent, parent_range);
@@ -1075,7 +1123,7 @@ static int process_ranges_merge_commit(struct rev_info
*rev, struct commit *comm
for (i = 0; i < nparents; i++) {
parents[i] = p->item;
p = p->next;
- queue_diffs(&rev->diffopt, &diffqueues[i], commit, parents[i]);
+ queue_diffs(range, &rev->diffopt, &diffqueues[i], commit,
parents[i]);
}
for (i = 0; i < nparents; i++) {
--
1.8.2.241.gee8bb87
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
