Re: [PATCH] git-gui: Perform rescan on window focus-in

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 2 Aug 2019, Pratyush Yadav wrote:

> On 8/1/19 1:12 AM, Johannes Schindelin wrote:
> >
> > I would be _extremely_ cautious to base an argument on one
> > particular setup, using on particular hardware with one particular
> > OS and one particular repository.
> >
>
> Agreed. That's why I asked for benchmarks from other people.
> Unfortunately, no one replied.

What stops _you_ from performing more tests yourself? There are tons of
real-world repositories out there, we even talk about options for large
repositories to test with in Git for Windows' Contributing Guidelines:
https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md#performance-tests

> I am worried about exactly this problem that other users will have
> performance problems. I usually reserve judgment till I see some
> actual benchmarks, but since in this case we aren't getting any, it is
> probably better to err on the side of caution.

Well, you proposed it without testing it first e.g. on Kotlin or even
Linux.  Personally, I would have chosen Chromium, as it is a pretty big
one.

Maybe you want to do that now.

> > When it comes to repositories that are worked on actively, git.git is
> > not actually a representative example, it is way smaller than what users
> > deal with.
>
> Out of curiosity, what would you consider large enough? The Linux kernel
> (855,753 commits as of writing this)?

As your patch is about refreshing the Git GUI, the number of Git commits
is irrelevant. The number of worktree files is relevant.

Unfortunately, you cannot search for this dimension on GitHub, but you
can search for "repository size", which is at least loosely correlated:

https://github.com/search?utf8=%E2%9C%93&q=size%3A%3E5000000&type=Repositories&ref=advsearch&l=&l=

Personally, I would probably have chosen either of these, for testing:

* https://github.com/chromium/chromium
* https://github.com/WebKit/webkit
* https://github.com/raspberrypi/firmware
* https://github.com/MicrosoftDocs/azure-docs
* https://github.com/facebookresearch/FAIR-Play

> > At this point, I am gently inclined against the presented approach, in
> > particular given that even context menus reportedly trigger the re-scan
> > (which I suspect might actually be a Linux-only issue, as context menus
> > are top-level windows on X11, at least if I remember correctly, and I
> > also seem to remember that they are dependent windows on Aqua and Win32,
> > just to add yet another argument against overfitting considerations onto
> > a single, specific setup).
>
> All right, the patch in its current state can't fly. So what is the correct
> way to do this? I see the following options:
>
> 1. Add this as an option that is disabled by default, but people who don't
> mind it can enable it. This is the easiest to implement. But I leave it to you
> and Junio (and anyone else who wants to pitch in :)) to decide if it is a good
> idea.

That would probably be much easier to get accepted.

> 2. Watch all files for changes. Atom does this for their git gui [0]. We can
> probably use watchman [1] for this, but this adds another external dependency.

I am currently looking at watchman, and it seems that it has its own
performance issues in big repositories (for which it is actually most
relevant). Besides, Windows support is kinda flaky, so I would rule this
out (Git is supported on many more platforms than watchman supports).

Besides, what your patch wants to do is not to know when things have
changed. Your patch wants to refresh the UI at opportune moments, and it
is unclear how watchman could help decide when to refresh.

> 3. Leave this feature out. I of course don't like this option very much, and
> will probably have to run a fork, but if it is better for the project, it is
> better for the project.

That would indeed be the safest.

I wonder, however, whether you can think of a better method to figure
out when to auto-refresh. Focus seems to be a low-hanging fruit, but as
you noticed it is not very accurate. Maybe if you combine it with a
timeout? Or maybe you can detect idle time in Tcl/Tk?

Ciao,
Johannes