On Sat, Nov 30, 2019 at 5:05 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| [off-list ref] wrote:

Hi Jonathan,

Thanks for the re-roll.

You are most welcome :-)

On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:

quoted

+# 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`.

quoted

              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.

quoted

-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