git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jonathan Gilbert <logic@deltaq.org>
To: "Pratyush Yadav me-at-yadavpratyush.com |GitHub Public/Example
	Allow|"  <172q77k4bxwj0zt@sneakemail.com>
Cc: Jonathan Gilbert via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Jonathan Gilbert <rcq8n2xf3v@liamekaens.com>,
	Jonathan Gilbert <JonathanG@iqmetrix.com>
Subject: Re: [PATCH v3 2/2] git-gui: revert untracked files by deleting them
Date: Sat, 16 Nov 2019 15:42:28 -0600
Message-ID: <CAPSOpYuv1d_yAgBnVBDXMwaA3B3QpmbVKtiSpexpJDwC6Bz33w@mail.gmail.com> (raw)
In-Reply-To: <20191116151113.mwbaendh6lgykfw3@yadavpratyush.com>

On Sat, Nov 16, 2019 at 9:11 AM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> > -             grid $w.msg - $w.vs -sticky news
> > -             grid $w.unlock $w.continue - -sticky se -padx 2 -pady 2
> > -             grid columnconfigure $w 0 -weight 1
> > -             grid rowconfigure $w 0 -weight 1
> > -
> > -             wm protocol $w WM_DELETE_WINDOW update
> > -             bind $w.continue <Visibility> "
> > -                     grab $w
> > -                     focus %W
> > -             "
> > -             wm deiconify $w
> > -             tkwait window $w
> > +     close $fd
> > +     $::main_status stop
>
> I didn't spot this earlier. Will this call to 'stop' interfere with the
> 'start' in 'delete_files'?

Hmm, I think this actually highlights a larger issue. Both
`write_checkout_index` and `delete_helper` display their progress in
the status bar, so if the user elects to do a check-out, and then
while it is still in progress asynchronously, elects to delete files,
they'll fight over who gets to set the status. If I'm understanding
correctly, this won't actually interfere with correct operation, but
of course it won't look very nice.

If they overlap in this manner, _then_ multiple calls to `stop` could
be made, though it does appear that `stop` is idempotent. The Tk
documentation states that `destroy` doesn't return any error if you
point it at a window that doesn't exist.

`start` is explicitly idempotent, only creating a new canvas if it
doesn't already have one.

I'll see what I can come up with for letting operations more cleanly
share the status bar.

> >       if {$update_index_cp >= $total_cnt} {
> > -             _close_updateindex $fd $after
> > +             if {[_close_updateindex_rescan_on_error $fd]} {
> > +                     unlock_index
> > +             }
> > +
> > +             uplevel #0 $after
>
> This changes when $after is called. If you pass it to 'rescan', it runs
> _after_ the rescan is finished. Now it runs "in parallel" with it. Are
> you sure that is the intended behaviour? Should we just stick to passing
> $after to rescan on failure?
>
> [..]
>
> While we're here, how about just moving this entire thing to
> '_close_updateindex_rescan_on_error', since the only two consumers of
> the function do the _exact_ same thing?
>
> This would also allow us to pass $after to 'rescan'. It would also
> hopefully make the code a bit easier to follow because you can clearly
> see that we only unlock the index when there is no error.
>
> Even better, unlock the index unconditionally in
> '_close_updateindex_rescan_on_error', and remove the 'unlock_index' call
> from 'rescan_on_error'. I generally prefer to keep locking/unlocking
> paths as simple as possible.

Hmm, yeah, this makes sense. Pass it `$after`, and then if it calls
`rescan`, it can hand it off, and `rescan` also (I'm assuming?)
implicitly unlocks the index. If it doesn't need to call `rescan`,
then `_close_updateindex_rescan_on_error` itself unlocks the index
_and_ invokes `$after`.

> >       if {$update_index_cp >= $total_cnt} {
> > -             _close_updateindex $fd $after
> > +             if {[catch {_close_updateindex $fd} err]} {
> > +                     uplevel #0 $capture_error [list $err]
> > +             }
> > +
> > +             uplevel #0 $after
> > +
>
> Nitpick: Please explicitly mention why we _don't_ want to unlock the
> index here.
>
> There are two function very similar to this one: 'write_update_index'
> and 'write_update_indexinfo'. This subtle but important difference is
> very easy to gloss over.

Hmm, so, this suggests a rename of
`_close_updateindex_rescan_on_error`, because (with the previous
proposal) it implicitly includes unlocking the index, whereas
`_close_updateindex` does not.

Thanks,

Jonathan Gilbert

  reply index

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30  6:48 [PATCH 0/2] " Jonathan Gilbert via GitGitGadget
2019-10-30  6:48 ` [PATCH 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-03  0:27   ` Pratyush Yadav
2019-10-30  6:48 ` [PATCH 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-03  7:44   ` Pratyush Yadav
2019-11-04 16:04     ` Jonathan Gilbert
2019-11-04 17:36     ` Jonathan Gilbert
2019-10-30  9:06 ` [PATCH 0/2] " Bert Wesarg
2019-10-30 17:16   ` Jonathan Gilbert
2019-11-03  1:12     ` Pratyush Yadav
2019-11-03  4:41       ` Jonathan Gilbert
2019-11-03  7:54         ` Pratyush Yadav
2019-11-07  7:05 ` [PATCH v2 " Jonathan Gilbert via GitGitGadget
2019-11-07  7:05   ` [PATCH v2 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-07  7:05   ` [PATCH v2 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-11 19:25     ` Pratyush Yadav
2019-11-11 21:55       ` Jonathan Gilbert
2019-11-11 22:59         ` Philip Oakley
2019-11-12  4:49           ` Jonathan Gilbert
2019-11-12 10:45             ` Philip Oakley
2019-11-12 16:29               ` Jonathan Gilbert
2019-11-26 11:22                 ` Philip Oakley
2019-11-12 19:35         ` Pratyush Yadav
2019-11-11 19:35   ` [PATCH v2 0/2] " Pratyush Yadav
2019-11-13  9:56   ` [PATCH v3 " Jonathan Gilbert via GitGitGadget
2019-11-13  9:56     ` [PATCH v3 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-13  9:56     ` [PATCH v3 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-16 15:11       ` Pratyush Yadav
2019-11-16 21:42         ` Jonathan Gilbert [this message]
2019-11-17  6:56     ` [PATCH v4 0/2] " Jonathan Gilbert via GitGitGadget
2019-11-17  6:56       ` [PATCH v4 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-17  6:56       ` [PATCH v4 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-24 13:09         ` Pratyush Yadav
2019-11-19 15:21       ` [PATCH v4 0/2] " Pratyush Yadav
2019-11-19 16:56         ` Jonathan Gilbert
2019-11-24 20:37       ` [PATCH v5 0/3] " Jonathan Gilbert via GitGitGadget
2019-11-24 20:37         ` [PATCH v5 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-24 20:37         ` [PATCH v5 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2019-11-27 21:55           ` Pratyush Yadav
2019-11-28  7:34             ` Jonathan Gilbert
2019-11-24 20:37         ` [PATCH v5 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-27 22:03           ` Pratyush Yadav
2019-11-28  8:30         ` [PATCH v6 0/3] " Jonathan Gilbert via GitGitGadget
2019-11-28  8:30           ` [PATCH v6 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-28  8:30           ` [PATCH v6 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2019-11-30 23:05             ` Pratyush Yadav
2019-12-01  2:12               ` Jonathan Gilbert
2019-12-01 11:43               ` Philip Oakley
2019-12-01 20:09                 ` Jonathan Gilbert
2019-11-28  8:30           ` [PATCH v6 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-12-01  2:28           ` [PATCH v7 0/3] " Jonathan Gilbert via GitGitGadget
2019-12-01  2:28             ` [PATCH v7 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-12-01  2:28             ` [PATCH v7 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2019-12-01  2:28             ` [PATCH v7 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-12-05 18:54             ` [PATCH v7 0/3] " Pratyush Yadav

Reply instructions:

You may reply publically 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=CAPSOpYuv1d_yAgBnVBDXMwaA3B3QpmbVKtiSpexpJDwC6Bz33w@mail.gmail.com \
    --to=logic@deltaq.org \
    --cc=172q77k4bxwj0zt@sneakemail.com \
    --cc=JonathanG@iqmetrix.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=rcq8n2xf3v@liamekaens.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git