Re: [PATCH v2 2/2] git-gui: revert untracked files by deleting them

> On Mon, Nov 11, 2019 at 1:25 PM Pratyush Yadav me-at-yadavpratyush.com
> |GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> > On 07/11/19 07:05AM, Jonathan Gilbert via GitGitGadget wrote:
> > > --- /dev/null
> > > +++ b/lib/chord.tcl
> > > @@ -0,0 +1,137 @@
> >
> > The 'class' documentation [0] suggests adding a "package require TclOO".
> > But TclOO ships by default with Tcl 8.6 and above. So, I'm not really
> > sure if we need this.
> 
> I'm not super familiar with it. I just checked what Tcl version I was
> myself running, since it's only there because of the Git Gui
> installation bundled with Git for Windows, and it was 8.6, so I
> assumed it was fair game to use. It didn't occur to me that you could
> already have an older version of Tcl installed and have Git Gui use
> it. :-) So, if I'm understanding correctly, `TclOO` as a package could
> potentially be used to allow TclOO to be used with 8.4, the minimum
> supported version you mention below, and it just happened to work for
> me in my testing without that because I have 8.6 installed but that's
> technically newer than the supported baseline?
> 
> > Nice to see some good documentation!
> >
> > One nitpick: would it make more sense to have the documentation for a
> > method/constructor just above that method/constructor? This way, when
> > someone updates the code some time later, they'll also hopefully
> > remember to update the documentation. It is much more likely to be stale
> > if all of it just stays on the top.
> 
> Hmm, what do you think of both? I was thinking of the documentation as
> a single self-contained block that someone could read to put together
> an understanding of how the chord system fits together, and split out,
> it wouldn't have that readability. What about a more abstract
> description in a block at the top, and then more technically-detailed
> & specific descriptions attached to each method?

> > > +oo::class create SimpleChord {
> >
> > This comes from the TclOO package, right?
> >
> > git-gui has its own object-oriented system (lib/class.tcl). It was
> > written circa 2007. I suspect something like TclOO did not exist back
> > then.
> >
> > Why not use that? Does it have some limitations that TclOO does not
> > have? I do not mind using the "official" OO system. I just want to know
> > why exactly you made the choice.
> 
> Having limited experience with Tcl, I did a Google search for "tcl
> object oriented" and ended up writing code using TclOO because that's
> what came up. Do you think it makes sense to rework this to use
> `class.tcl`, or perhaps instead the opposite: have a policy of using
> the standard TclOO going forward, and let the rest of Git Gui
> organically upgrade itself to some hypothetical point in the future
> where class.tcl is no longer used by anything?

> > More importantly, TclOO ships as part of the core distribution with Tcl
> > 8.6, but as of now the minimum version required for git-gui is 8.4. So,
> > I think we should bump the minimum version (8.6 released circa 2012, so
> > most people should have caught up by now I hope).
> 
> If I understand correctly, you mentioned that TclOO was intrinsically
> available to me because I was using Tcl 8.6, and that the manual
> recommends `package require TclOO` -- does that package dependency
> permit the use of TclOO on 8.4? If so, could that be a way to avoid
> bumping the minimum version required? Simply in the interest of
> keeping the scope of the change limited. If not, then bumping the
> minimum required version to 8.6 from 2012 doesn't seem entirely
> unreasonable either. :-)

> > > +     variable Notes
> > > +     variable Body
> > > +     variable IsCompleted
> >
> > Nitpick: Please use snake_case, here and in other places.
> 
> Okay, yep -- I had copied the convention that I saw in TclOO examples,
> conscious of the fact that there might be a standard specific to
> object-oriented Tcl.
> 
> > > +     method notify_note_activation {} {
> >
> > Since this method is for internal use only, can it be made "private"?
> > Does the OO library support something like this?
> 
> I don't think so, because it's called from outside the class. What
> we'd be looking for is something like C++'s "friend" syntax. Tcl
> doesn't seem to have this. Though, I just did some further Googling,
> and saw a hint that it might be possible to bypass member security on
> a case-by-case basis, so that the method is private but `ChordNote` is
> able to call it anyway. I'll see if I can't figure this out. :-)

> > > +     method unknown {} {
> >
> > I'm a bit lost here. This method is named 'unknown', but searching for
> > 'unknown' in this patch just gives me two results: this line here, and
> > then one in a comment at the start of the file.
> >
> > From what I understand looking at the code, it some sort of a "default"
> > method, and is called when you run just `$chord_note`. How exactly is
> > this method designated to be the default?
> >
> > Also, "unknown" makes little sense in this context. Can you rename it to
> > something more meaningful? Maybe something like "activate_note"?
> 
> I think it's the fact that it is named `unknown` that makes it the
> "default" method. I think this just needs documentary comments next to
> it. The TclOO documentation says:

> > obj unknown ?methodName? ?arg ...?
> > This method is called when an attempt to invoke the method methodName on
> > object obj fails. The arguments that the user supplied to the method are
> > given as arg arguments. If methodName is absent, the object was invoked with
> > no method name at all (or any other arguments).
> 
> It was based on that last sentence that I interpreted `unknown` as,
> "This is a mechanism for making an object that can be called like a
> method."

> > > +             if {!$IsActivated} {
> > > +                     set IsActivated 1
> > > +                     $Chord notify_note_activation
> > > +             }
> > > +     }
> > > +}
> >
> > From what I understand, the "Note" object is effectively used as a
> > count. There is no other state associated with it. When I first heard of
> > your description of this abstraction, I assumed that a Note would also
> > store a script to execute with it. So, when you "activate" a note, it
> > would first execute the script, and then mark itself as "activated", and
> > notify the chord. Would that abstraction make more sense?
> >
> > I don't really mind keeping it this way, but I wonder if that design
> > would make the abstraction easier to wrap your head around.
> 
> I learned about the concept of chords and notes from an experimental
> language that Microsoft created many years back called "Polyphonic C#"
> (which in turn got rolled into "Cw" (C-omega)), and in that
> abstraction, the idea was that, well, as a baseline, for starters, we
> have methods and each one, conceptually, has an entrypoint with a
> certain set of parameters, and when you call that entrypoint, the
> parameters are all set and the body runs. With a "chord", you have
> more than one entrypoint attached to the same body -- the entrypoints
> themselves don't have any logic associated with them individually.
> Each note has its own parameter list, and when all the notes have been
> called, the body is run with _all_ of those parameters.
> 
> I drew some ASCII art, don't know if it'll translate in the message,
> but here goes :-)
> 
> Basic method (or, if you will, a "chord" with only one "note"):
> 
>            (caller)
>               |
>     void Add(int X, int Y)
>               |
>       { output(X + Y) }
> 
> A "chord" with two "notes":
> 
>         (caller)                (caller)
>             |                       |
>     void AddX(int X)         void AddY(int Y)
>             |                       |
>             `-----------.-----------'
>                         |
>                 { output(X + Y) }
> 
> The specific details differ from what I've written here. In Polyphonic
> C#, you don't have to instantiate a chord, you simply start calling
> methods, and the runtime matches up complete sets dynamically. (Just
> thinking through the implications of this, if the notes aren't all
> called at exactly the same rate this obviously leads very easily to
> bugs that chew up all memory on incomplete chords. :-P) Also,
> Microsoft's language has parameters to each of the notes that are
> _all_ passed to the body at once. My implementation here is a "simple"
> chord, I didn't bother with arguments, as they aren't needed in this
> usage :-) I also found it much simpler to think of implementing the
> chord with the activations being explicit instead of implicit. So
> instead of saying up front, "Here is my method body and here are its 3
> entrypoints", with this implementation the chord is a dynamic object,
> you say "Here is my method body" and get back a thing that you can
> start tacking entrypoints onto.
> 
> But, a "note" in a SimpleChord isn't a counter, it's a latch. The
> chord itself is acting sort of like a counter, in that all the notes
> need to be activated, but because the notes are latches, activating a
> note repeatedly has the same effect as activating it once. There's no
> way for one note to interfere with other notes, which wouldn't be the
> case if it literally were just a counter.

> It seems to me that a chord where each note has a script of its own is
> actually basically just a class with methods, I guess with a common
> joined epilogue?:
> 
>         (caller)                (caller)
>             |                       |
>     void AddX(int X)         void AddY(int Y)
>             |                       |
>    { script for AddX }      {script for AddY }
>             |                       |
>             `-----------.-----------'
>                         |
>                 { common tail?? }

> The whole point is that the notes are conceptually different "headers"
> into _the same_ body. When you call a note of a chord, it is because
> you want the _chord_'s script to run, and the chord is acting as a
> construct that says "okay, yes, I'll satisfy your request that I
> execute, but you'll have to wait, because I'm going to satisfy _all_
> your requests in one go".
> 
> > >       $::main_status stop
> > > -     unlock_index
> > > -     uplevel #0 $after
> >
> > There is a call to unlock_index in the body of the if statement above
> > too. Do we want to remove that too, or should it be left alone?
> >
> > That codepath seems to be taken when a major error happens, and we just
> > resign to our fate and get a fresh start by doing a rescan and syncing
> > the repo state. So it is quite likely whatever operation we were doing
> > failed spectacularly.
> >
> > Maybe the answer is to swallow the bitter pill and introduce a
> > switch/boolean in `_close_updateindex` that controls whether the index
> > is unlocked or not. We unlock it when the if statement is not taken, and
> > keep the current codepath when it is. I call it a "bitter pill" because
> > I'm usually not a huge fan of adding knobs like that in functions. Makes
> > the function harder to reason about and makes it more bug prone.
> >
> > If you can think of a better/cleaner way of working around this,
> > suggestions are welcome!
> 
> Hmm, so, yeah, the entire if statement only occurs if it can't close
> the file descriptor. Is that something that actually happens? If so,
> then it should perhaps be throwing an exception, because having
> started a rescan is probably more than the caller bargained for. That
> would prevent the callers from unlocking the index out from under the
> rescan, and also cancel any other processing they might be doing that
> is probably making bad assumptions with a rescan running.

> > >       if {$update_index_cp >= $total_cnt} {
> > > -             _close_updateindex $fd $after
> > > +             _close_updateindex $fd $do_unlock_index $after
> >
> > _close_updateindex takes only one argument, and you pass it 3.
> > $do_unlock_index does not seem to be defined anywhere. $after is
> > evaluated just after this line, and _close_updateindex doesn't accept
> > the argument anyway. I suspect this is a leftover from a different
> > approach you tried before this one.
> 
> It is indeed, oops!
> 
> > Also, unlike all the other places where _close_updateindex is used, this
> > one does not make a call to unlock_index. Is that intended? IIUC, it
> > should be intended, since this is the part which uses the "chord", but a
> > confirmation would be nice.
> 
> Intentional, yes. I'll see if there's a concise way to document this.
> 
> > > +     # Common "after" functionality that waits until multiple asynchronous
> > > +     # operations are complete (by waiting for them to activate their notes
> > > +     # on the chord).
> >
> > Nitpick: mention what the "multiple asynchronous operations" are exactly
> > (i.e, they are the deletion and index checkout operations).
> 
> Okeydoke.
> 
> > >       set after {}
> >
> > 'after' seems to be an unused variable. This line can be deleted.
> 
> Good catch.
> 
> > > +             if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
> >
> > Nitpick: please split the line into two.
> 
> Will do.
> 
> > > +                     set error_text "Encountered errors deleting files:\n"
> >
> > Wrap the string in a `mc [...]` so it can be translated some time in the
> > future.
> 
> Ah, yes, I did that with most messages, this was an oversight.
> 
> > > +proc MAX_VERBOSE_FILES_IN_DELETION_ERROR {} { return 10; }
> >
> > Why use a procedure, and not a global variable? My guess is to make it
> > impossible for some code to change this value by mistake. Do I guess
> > correctly?
> 
> A variable is by definition not a constant. This is the pattern that
> came up when I did a search for how one makes a constant in Tcl. ""\_(
> ``_/ )_/""
> 
> Making it a procedure means that if someone wants to put actual logic
> behind it in the future, it's already being called as a proc.

> > Wew! This took longer than I expected ;)
> >
> > Tested on Linux. Works fine after fixing the extra arguments passed to
> > `_close_updateindex`. Thanks.
> 
> Yeah, I did run things as I was changing them to verify, and felt like
> I covered everything, I'm surprised I didn't bump into that, obviously
> I didn't cover everything after all. Perfect demonstration of why
> developers should never be exclusively responsible for testing their
> own code :-D
> 
> Let me know w.r.t. which OO framework to employ and what that means
> for minimum required versions and/or package references.
> 
> Thanks very much,
> 
> Jonathan Gilbert