threads / patch / 36258

patchMake XDF_NEED_MINIMAL default in blame.

Subject: [PATCH] Make XDF_NEED_MINIMAL default in blame.

## tl;dr

3 messages between Mar 20, 2014 and Mar 20, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Michael Andreen· Mar 20, 2014, 20:18 UTC · lore

Currently git blame has a big problem finding copies and moves when you split up a big file into smaller ones. One example in the git repository is 2cf565c, which split the documentation into smaller files.

In 582aa00 XDF_NEED_MINIMAL was removed as the default for performance reasons, mainly for diff and rebase, but blame was also changed.

In 059a500 the problem with blame was noticed and the flag --minimal was introduced. However this flag is not documented and it is not possible to set when using "git gui blame".

Setting XDF_NEED_MINIMAL as default has a small performance impact when you run on a file with few modifications. However, if you run it on a file with a bigger number of modifications, the performance impact is small enough to not be noticable.

The previous behavior can still be activated with --no-minimal.

((2cf565c...))$ time PAGER=cat git blame -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m0.003s user 0m0.002s sys 0m0.000s

((2cf565c...))$ time PAGER=cat git blame --minimal -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m0.010s user 0m0.009s sys 0m0.000s

((2cf565c...))$ time PAGER=cat git blame -C -C -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m0.010s user 0m0.010s sys 0m0.000s

((2cf565c...))$ time PAGER=cat git blame --minimal -C -C -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m0.028s user 0m0.027s sys 0m0.000s

(master)$ time PAGER=cat git blame -C -C -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m2.338s user 0m2.283s sys 0m0.056s

(master)$ time PAGER=cat git blame --minimal -C -C -C -M
    Documentation/git-ls-files.txt > /dev/null

real 0m2.355s user 0m2.285s sys 0m0.069s

(master)$ time PAGER=cat git blame -C -M cache.h > /dev/null

real 0m1.755s user 0m1.730s sys 0m0.024s

(master)$ time PAGER=cat git blame --minimal -C -M cache.h > /dev/null

real 0m1.785s user 0m1.770s sys 0m0.014s

(master)$ time PAGER=cat git blame -C -C -C -M cache.h > /dev/null

real 0m31.515s user 0m30.810s sys 0m0.684s

(master)$ time PAGER=cat git blame --minimal -C -C -C -M cache.h > /dev/null

real 0m31.504s user 0m30.885s sys 0m0.598s

Signed-off-by: Michael Andreen <harv@ruin.nu>
---
There hasn't been any arguments against this patch. Just updated the message 
with a note about --no-minimal.

Applies cleanly on both master and maint.

 builtin/blame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to builtin/blame.c +1 −1

diff --git a/builtin/blame.c b/builtin/blame.c
index e5b5d71..0e7ebd0 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -42,7 +42,7 @@ static int show_root;
 static int reverse;
 static int blank_boundary;
 static int incremental;
-static int xdl_opts;
+static int xdl_opts = XDF_NEED_MINIMAL;
 static int abbrev = -1;
 static int no_whole_file_rename;

-- 
1.8.3.2

Junio C Hamano· Mar 20, 2014, 20:45 UTC · re: Michael Andreen · lore

Re: [PATCH] Make XDF_NEED_MINIMAL default in blame.

Michael Andreen <harv@ruin.nu> writes:

> There hasn't been any arguments against this patch. Just updated the message 
> with a note about --no-minimal.

There hasn't been any argument for this patch, either.

It is not like we are still in year 2007; timing result in a small project like Git itself is not a good enough argument to change a well established default at this late in the game, especially when there are ways like command line options for users to specify their preferred settings.

Michael Andreen· Mar 20, 2014, 21:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make XDF_NEED_MINIMAL default in blame.

On Thursday, March 20, 2014 01:45:21 PM Junio C Hamano wrote:

Show 7 quoted lines

> There hasn't been any argument for this patch, either.
> 
> It is not like we are still in year 2007; timing result in a small
> project like Git itself is not a good enough argument to change a
> well established default at this late in the game, especially when
> there are ways like command line options for users to specify their
> preferred settings.

The main reason why I submitted it is because the current behavior is very unintuitive when you split big files into smaller ones. Like the one that was done in 2cf565c.

If you do:

 $ git checkout 2cf565c
 $ git blame -C -C -C -M Documentation/git-ls-files.txt

then blame will not be able to detect any moves, even though big chunks have been moved without whitespace changes. Worse is that if you do

 $ git gui blame Documentation/git-ls-files.txt

then that won't see any movies either, and there is no way to make git gui blame use --minimal.

I spent a lot of time at $work trying to code-review a big split like this, wondering why "git gui blame" and "git blame -C -C -C -M" couldn't detect moves when complete functions had been moved with no changes (not even whitespace). So by making it default it will hopefully help someone else from losing time investigating this.

It wasn't until I did a bisect on git, finding 582aa00 and later googling for XDF_NEED_MINIMAL to find 059a500, that I found out about --minimal. Which is not documented anywhere.

I guess it can be argued for just documenting --minimal and either patching "git gui blame" to use it, or make it configurable. However I think the current default is really confusing, and the reason for changing it back in 2007 was for performance, so that was what I tried to focus on, hopefully showing that there aren't any noticeable differences.

/Michael