From: Johannes Schindelin <Johannes.Schindelin@gmx.de> To: Pratyush Yadav <firstname.lastname@example.org> Cc: Mark Levedahl <email@example.com>, "brian m. carlson" <firstname.lastname@example.org>, git <email@example.com>, Christian Couder <firstname.lastname@example.org>, Junio C Hamano <email@example.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.firstname.lastname@example.org> (raw) In-Reply-To: <email@example.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 . We can > probably use watchman  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
next prev parent 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.firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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).