git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Pratyush Yadav <me@yadavpratyush.com>
Cc: Mark Levedahl <mlevedahl@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] git-gui: Perform rescan on window focus-in
Date: Fri, 2 Aug 2019 14:39:17 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1908021414530.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <cc5dddc7-e33e-2a2c-3205-6dd14edd0abd@yadavpratyush.com>

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

  reply	other threads:[~2019-08-02 12:39 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-28 15:17 Pratyush Yadav
2019-07-28 21:36 ` brian m. carlson
2019-07-28 22:10   ` Pratyush Yadav
2019-07-28 22:32     ` Pratyush Yadav
2019-07-28 22:49     ` brian m. carlson
2019-07-29  2:24       ` Mark Levedahl
2019-07-29  2:26       ` Mark Levedahl
2019-07-29  2:28       ` Mark Levedahl
2019-07-29  8:15         ` Pratyush Yadav
2019-07-31 19:42           ` Johannes Schindelin
2019-08-01 21:52             ` Pratyush Yadav
2019-08-02 12:39               ` Johannes Schindelin [this message]
2019-08-02 20:00                 ` Pratyush Yadav
2019-08-03 20:34                   ` Johannes Schindelin
2019-08-04 12:53                     ` Pratyush Yadav
2019-08-04 19:10                       ` Johannes Schindelin
2019-08-04 20:17                         ` Pratyush Yadav
2019-08-02 16:47               ` Junio C Hamano
2019-08-02 20:13                 ` Pratyush Yadav
2019-08-04 18:56                   ` Johannes Schindelin
2019-07-29  5:09       ` Junio C Hamano
2019-07-29  8:44         ` Pratyush Yadav
2019-07-28 21:44 ` Pratyush Yadav

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1908021414530.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@yadavpratyush.com \
    --cc=mlevedahl@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH] git-gui: Perform rescan on window focus-in' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).