Re: [PATCH v6 2/3] git-gui: update status bar to track operations

[Jonathan Gilbert <logic@deltaq.org>]

On Sat, Nov 30, 2019 at 5:05 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> Hi Jonathan,
>
> Thanks for the re-roll.

You are most welcome :-)

> On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
> > +# Operation displayed by status mega-widget during _do_clone_checkout =>
> > +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
> > +# _do_validate_submodule_cloning. The status mega-widget is a difference
> > +# instance than that stored in $o_status in earlier operations.
>
> The last sentence doesn't make a lot of sense to me. What is "earlier
> operations"? If this refers to previous versions of this file, then I
> don't think such a comment belongs here. It should be in the commit
> message instead.

A clone starts out by calling `_do_clone2`, which, for `$clone_type`
of `hardlink`, creates a status "mega-widget" and uses it to track
linking and/or copying the underlying files. Then, this part of the UI
is destroyed. Later, the code calls into _do_clone_checkout, which
sets up its own, different view. This view _also_ uses a status
"mega-widget", but it's not the same one as before. This wasn't
obvious to me in my first read-through, and I erroneously wrote code
that assumed the widget objects would carry forward. As such, I felt
it might be useful to other readers to have this detail called out
up-front. In the context of `_do_clone_checkout`, the "earlier
operations" is what happens in `_do_clone2`.

> >               destroy $w_body
> > +
> > +             set o_status {}
>
> Should we be calling a destructor for this here? There is the '_delete'
> method in status_bar.tcl, but I don't see any usages of it so I'm not
> sure what exactly it is supposed to do.
>
> That said, the previous version of this file doesn't call any sort of
> destructor either, so maybe we should just leave it like it is for now.
> I dunno.

As far as I can tell, `destroy $w_body` automatically deletes the
entire subtree of UI components. I mentioned that I had written broken
code at first because I didn't realize the status widget got replaced
between `_do_clone2` and `_do_clone_checkout` -- that code encountered
an error that indicated that the status widget object no longer
existed at all. Thus, I have proceeded on the assumption that `destroy
$w_body` handles that particular detail, and all that's left is to
clear `o_status` of its dangling reference to the object that no
longer exists.

> > -method _do_validate_submodule_cloning {ok} {
> > [..]
> > -method _do_clone_submodules {} {
>
> Is there a reason for moving these two methods around? Not that its a
> bad thing, I'm just curious.

I touched on this in the cover letter. I'll just copy/paste that text
since it says it just as well as I could re-synthesize here :-)

* In `choose_repository.tcl`, there is a sequence of functions
involved performing the checkout on the clone: `_do_clone_checkout` =>
`_readtree_wait` => `_postcheckout_wait` => `_do_clone_submodules` =>
`_do_validate_submodule_cloning`. The functions have been re-ordered
in the source code to match the sequence in which they execute to
improve clarity.

Re-roll (final?) incoming.

Thanks,

Jonathan Gilbert