#89 tkdiff -rREV1 crashes

[Duncan Roe]

To reproduce:

cd /usr/src/tkdiff    # Or wherever
tkdiff -r182

which produces:

Error in startup script: can't read "tildechk": no such variable
    while executing
"Dbg " PAIRing SCM($Scm(1)/$Scm(2)) Fspec($pths) Rev($revs) Tchk($tildechk)""
    (procedure "assemble-args" line 112)
    invoked from within
"assemble-args"
    (procedure "main" line 72)
    invoked from within
"main"
    (file "/usr/bin/tkdiff" line 18249)

This started happening in 5.4. The fix is straightforward:

@@ -2524,7 +2524,11 @@
             }
         }


-        Dbg " PAIRing SCM($Scm(1)/$Scm(2)) Fspec($pths) Rev($revs) Tchk($tildechk)"
+        if {[info exists tildechk]} {
+            Dbg " PAIRing SCM($Scm(1)/$Scm(2)) Fspec($pths) Rev($revs) Tchk($tildechk)"
+        } {
+            Dbg " PAIRing SCM($Scm(1)/$Scm(2)) Fspec($pths) Rev($revs)"
+        }

         # UNLESS the input has been deemed UNUSABLE ...
         if {$Err} { set msg $tildechk

But now this happens:

Error in startup script: can't read "MSG": no such variable
    while executing
"if {$MSG!={}} { uplevel 1 set MSG "{$MSG}" }"
    (procedure "inquire-svn" line 97)
    invoked from within
"inquire-svn $revs"
    (procedure "assemble-args" line 157)
    invoked from within
"assemble-args"
    (procedure "main" line 72)
    invoked from within
"main"
    (file "/usr/bin/tkdiff" line 18253)

After some debugging, this appears to fix it:

@@ -2514,7 +2514,7 @@
             #       (because no definitive Rev-ID actually exists)
             #   THAT MEANS having to UN-count that it got passed, but always
             #   REWRITING it as REQUESTING the *appropriate* "latest" revision

-            if {$Scm($N) == "Vpath" || $pths < $N} {
+            if {$Scm($N) == "Vpath" && $pths < $N} {
                 if {$r($N) != ""} { incr revs -1 }
                 set r($N) 0 ; # Presumption is we want the LATEST Vpath version
                 # BUT - if BOTH SCMs come up as Vpath, then the LEFT side needs

Full patch is attached.

[Duncan Roe]

While I think this fix is correct (in that revs should not be decremented), plain tkdiff still fails at inquire-svn line 97 with define autoSrch {1} in .tkdiffrc. Will investigate further.
A mistaken tkdiff -v (hoping to see version number) also fails the same way.

[michael-m]

Hmmm. I tend to disagree - although it certainly needs more investigation. Note that the original failure is based on an undefined value (tildechk) but only because that value was requested via a Debugging message argument list. That suggests there is a program-flow that REACHES the Dbg statement, that also manages to avoid the code that establishes "tildechk". I'd want to identify that "flow" condition first and perhaps decide if "tildechk" should maybe simply be REMOVED from the Dbg statement, before making logic changes. This might simply be a situation where the Dbg statement simply EXISTED as-is, and then a necessary logic change was later made that enabled the new flow path. I agree the crash needs to be eliminated, but the change from an ORlogic to ANDlogic can have far reaching effects - particularly in the code suggested for modification, which is concerned primarily with interpreting the manipulation of revisions in a VPATH-as-SCM case. Nevertheless, a crash should NEVER occur, thus it warrants further work.

[Duncan Roe]

w.r.t. tildechk, sure, you could remove the Dbg. The flows look OK to me: Err is either set in the block starting if {$g(conflictset)} or inside the associated else block, within the block starting if {$N <= $pths} which is always false since $pths is 0, and which sets tildechk. So the later line if {$Err} { set msg $tildechk can only fire when tildechk exists.
The Dbg is outside the block starting if {$N <= $pths} so tildechk may or may not exist.

[Duncan Roe]

The change from an ORlogic to ANDlogic affects code which is new at 5.5. Looks to me like a simple typo: there is no way you want to be in that code with an SCM other than Vpath.

[michael-m]

OK. You win - cant argue that logic - But as far as its origin, I suspect it was a leftover from an earlier implementation attempt that should have been updated but got skipped. Mea Culpa.

[michael-m]

W.r.t. the glitch with MSG: its an admittedly ugly construct that is intended to optionally pass back messaging that may have occurred to the calling proc (hence the uplevels). What I find disconcerting when looking at this again, is I would think to work properly each layer of the call stack should have initialized its OWN local level as having empty values, such that the check at the end (where you say it dies) could then decide whether to pass the value UPWARD if anything happened in the current level. A double check of the OTHER "inquire" routines (Git, CVS) seems to show that none of them follows that approach - and THAT is disturbing. Will need to dive a bit deeper in the call chain to confirm if my hypothesis is correct. Perhaps they ALL need that initialization....

[Duncan Roe]

The patch below fixed it for me. But you are right about the others: tkdiff failed for me in a clean git repository. I found a CVS repo where cvs diff worked and tkdiff failed there as well. So the attached patch fixes all 3.

--- tkdiff      2022/08/29 02:46:16     1.6
+++ tkdiff      2022/08/29 04:07:07     1.7
@@ -3175,6 +3175,7 @@
     # rev  is what we will tell svn cat to access
     # cmit is how we express the range to 'svn diff'
     set cmit(2) [set rev(2) ""]

+    set MSG [set msg {}]

     if {$revs == 0} {
         # Sets up       BASE     ->   WC

When svnRC becomes 1, at most one of MSG and msg is set. But both are tested at the end of inquire-svn{}. So both must be set to empty string at inquire-svn{} start. inquire-svn{} is only called from assemble-args{} which already has MSG and msg. Same for CVS and git.
BTW tkdiff -v now works the same as tkdiff. Was there supposed to be a check for unrecognised options?

[michael-m]

Okay - so I cant explain why all the inquire-* procs seem to have lost their initializations for the MSG/msg variables, but they all need them for exactly the reasons already pointed out. Will be fixing that straight away (and because it AGAIN stops crashes, I suppose there will be a V5.5.2 appearing shortly as well).

Regarding the "-v", Tkdiff has this odd rule about command line arguments (invented long before I got here) that states:

Any arguments not understood to be valid TkDiff arguments are passed to the underlying Diff engine to be interpreted there.

V5.5 tweaked that rule slightly to honor that situation only until the preferences controlling the Diff args become modified from within a given session - at which time the "pass thru" from the command line is stopped.

The confusing aspect here is that "-v" is actually considered a VALID argument (its a synonym for "-r", something not readily advertised) but without an actual VALUE attached, it pretty much evaporates altogether. Again - this is long standing behavior, and thus is not permitted to be changed (to maintain historical compatibility). The closest thing to what you were looking for could have been "tkdiff -h", but even THAT doesn't actually STATE the current version - yet; but I can make that change!

And so, with that addressed, I think this ticket has run out of issues and is safe to close. Agreed?

[Duncan Roe]

Agreed

[michael-m]

LATE UPDATE

Its true that I earlier accepted the argument that the change from "OR" logic to "AND" SEEMED to support the notion that only VPATH-based files should be in the vicinity of the code being changed. But (it turns out) that was NOT the reason the "OR" was in place - it exists to recognize the case where VPATH is in effect but only a SINGLE FileSpec was provided, which REQUIRES an SCM to be involved - and if that IS VPATH, then the Rev data MUST be rewritten (because of how VPATH as an SCM works internally)!

In short - right bug LOCATION - wrong fix! The suggested fix simply changed the conditions to effectively void the problem being seen, yet introduce a new problem elsewhere (improper VPATH selections). This will be addressed in V5.5.3