This is an extension of a previous thread and part of continuing work to improve fossil's reporting of renames: https://fossil-scm.org/forum/forumpost/5a4c530e6bc56d75

James reported that fossil {changes,status} fails to report a file is renamed when it is both renamed and edited. This was one of the things that caught me when I first started using fossil, too, and something I think needs to be improved.

Further discussions with larrybr@ in /chat also reiterated the importance of not misleading through omission, which is what happens when we we report a renamed and modified file as just modified.

However, stephan@ raised concern over the possible impact on users who might be consuming such output in scripts even though we make no guarantee of stable output here.

As such, before committing, I thought an RFC would be good for further input.

The below diff appends |RENAMED to the concurrent modified state as demonstrated in this paste: http://yy.jamsek.net/g8cc

Otherwise, there are no other output changes.

I think this is an improvement; however, I'm not married to the syntax. Maybe +RENAMED would be better. Nonetheless, I think the renamed state should also be reported in such cases.

Index: src/checkin.c
=======================================================================
hash - aba435cc20a83fa95ba30bbae545f4b1de419ede83f97b4ed0b8b1acb9cbaaff
hash + ba8b2ff41aae8d9a4522c1d1d1d52600bcaf5f97866965b60c85b51be8e340a3
--- src/checkin.c
+++ src/checkin.c
@@ -220,6 +220,7 @@ static void status_report(
     const char *zOrigName = 0;
     char *zFullName = mprintf("%s%s", g.zLocalRoot, zPathname);
     int isMissing = !file_isfile_or_link(zFullName);
+    int bRenamed = 0;
 
     /* Determine the file change classification, if any. */
     if( isDeleted ){
@@ -270,9 +271,6 @@ static void status_report(
     }else if( (flags & (C_EDITED | C_CHANGED)) && isChnged
            && (isChnged<2 || isChnged>9) ){
       zClass = "EDITED";
-    }else if( (flags & C_RENAMED) && isRenamed ){
-      zClass = "RENAMED";
-      zOrigName = db_column_text(&q,8);
     }else if( (flags & C_UNCHANGED) && isManaged && !isNew
                                     && !isChnged && !isRenamed ){
       zClass = "UNCHANGED";
@@ -279,14 +277,22 @@ static void status_report(
     }else if( (flags & C_EXTRA) && !isManaged ){
       zClass = "EXTRA";
     }
+    if( (flags & C_RENAMED) && isRenamed ){
+      bRenamed = 1;
+      zOrigName = db_column_text(&q,8);
+    }
 
     /* Only report files for which a change classification was determined. */
-    if( zClass ){
+    if( zClass || bRenamed ){
       if( flags & C_COMMENT ){
         blob_append(report, "# ", 2);
       }
       if( flags & C_CLASSIFY ){
-        blob_appendf(report, "%-10s ", zClass);
+        if( zClass!=0 && bRenamed ){
+          blob_appendf(report, "%s|RENAMED ", zClass);
+        }else{
+          blob_appendf(report, "%-10s ", zClass ? zClass : "RENAMED");
+        }
       }
       if( flags & C_MTIME ){
         blob_append(report, zMtime, -1);

Maybe +RENAMED would be better.

I am for the plus sign, too. But any of them is an improvement.

I maintain fossil vscode extension and was just thinking about file classes. I don't like +RENAMED because what if a file is renamed, modified and marked EXECUTABLE? I'd print comma separated statuses.

what if a file is renamed, modified and marked EXECUTABLE? I'd print comma separated statuses.

At present, that will only show EDITED, or with the proposed diff EDITED+RENAMED, because of how fossil tracks state in the vfile checkout table. We use an integer to denote state (e.g., metadata like xbit, modified, merged), but we can also identify renames by the name change obviously. Without more invasive changes to the vfile scanning code, we will only be showing at most two state changes, one of which will always be RENAMED.

That said, I don't mind the comma delimiter so provided Daniel and ravbc do not object to using the comma, I'll go with that. I'll commit in a day or so if there are no objections.

I don't mind the comma. Anyway, the delimiter can be easily changed later if necessary, adding the functionality is more important.

This breaks vc-fossil as well, and many other programs that expect the output to be more or less what it is ("EDITED[:space:]*" or whatever). Changing it "later" is also problematic, since now you need to cater for the old and new behavior, temporary solutions tend to have an issue that they are never temporary.

It also opens a can of worms on permutations.

This breaks vc-fossil as well, and many other programs that expect the output to be ...

My built-in Devil's Advocate feels compelled to point out that we've never guaranteed stable CLI output. When we can keep it useful without breaking anyone's scripts, great, but we generally don't lose any sleep over changing it without notice when there's a compelling reason to do so.

That said, we're perfectly happy to work with scripters to help accommodate them (insofar as doing so does not force us to maintain backwards compatibility of CLI output forever).

Perhaps (just spitballing here) what we need is a radically new output format which would be more future-friendly to such changes? (No idea what that might look like, just tossing the idea out there.) If we're going break it, we might as well go all-out, right?

Stephan raised this in chat, too, but as he explains again here, CLI output has never been guaranteed. And as wyoung has rightly explained, it is not intended for such consumption either.

[...] we've never guaranteed stable CLI output. When we can keep it useful without breaking anyone's scripts, great, but we generally don't lose any sleep over changing it without notice when there's a compelling reason to do so.

Further, I agree with larrybr's comment in /chat regarding misleading output. Reporting a renamed file as simply EDITED is highly misleading. This is compelling enough to warrant the change imo.

And we have changed precisely this output before (cf. 1b8cfdb01) to provide more accurate reporting--so there is precedence.

There is also no concern over further permutations; as explained up thread, without more invasive changes, there will only ever be ,RENAMED appended to at most one of the existing modified states. So I do not think this is cause for blocking this either.

Also, it should not be any less trivial than this change for third-party maintainers to accommodate.

I am strongly in support of the change and think it should go ahead, but as Stephan remarks, we are happy to help script maintainers where needed. It is worth reiterating that CLI output is not guaranteed though--and human readable output isn't intended to be consumed by third-party tools in any case.

Just brainstorming here, perhaps it could be possible to satisfy both cases ?

What about having 2 separate lines when there is a rename and a modification.

Like one line could only say "EDITED", and another one "RENAMED" ?

Hmm...I don't think I would object to this. Thanks for giving it some thought. It is not as good a solution as the initial proposal, imo, but if it's a more feasible change for third-party maintainers, it should be considered. OTOH, duplicate paths may produce another set of problems for such consumers. In any case, absent strong support for this approach, I'll commit the initial diff albeit with senyai's suggestion (i.e., a comma vice pipe delimiter) soon.

I think we have a solution that won't impact third-party consumers, and provide the improved, accurate reporting:

EDITED  original_name  ->  path/to/changed_name

That is, drop the RENAMED annotation, but add the name transition syntax we use in the just renamed case. This clearly shows the versioned file is both renamed and edited.

ETA: below diff and pastelink to example new output: http://yy.jamsek.net/fha1

I'm happy to go ahead with this if it is indeed more feasible for third-party scripts.

Index: src/checkin.c
=======================================================================
hash - aba435cc20a83fa95ba30bbae545f4b1de419ede83f97b4ed0b8b1acb9cbaaff
hash + c61f97ee17483d57436f9514098be6eff02523e635fdff181a8b744213573dd5
--- src/checkin.c
+++ src/checkin.c
@@ -270,14 +270,17 @@ static void status_report(
     }else if( (flags & (C_EDITED | C_CHANGED)) && isChnged
            && (isChnged<2 || isChnged>9) ){
       zClass = "EDITED";
-    }else if( (flags & C_RENAMED) && isRenamed ){
-      zClass = "RENAMED";
-      zOrigName = db_column_text(&q,8);
     }else if( (flags & C_UNCHANGED) && isManaged && !isNew
                                     && !isChnged && !isRenamed ){
       zClass = "UNCHANGED";
     }else if( (flags & C_EXTRA) && !isManaged ){
       zClass = "EXTRA";
+    }
+    if( (flags & C_RENAMED) && isRenamed ){
+      zOrigName = db_column_text(&q,8);
+      if( zClass==0 ){
+        zClass = "RENAMED";
+      }
     }
 
     /* Only report files for which a change classification was determined. */

That looks good.

...Unless, of course, many other programs that expect "RENAMED" would be therethrough severely and irreparably broken. sardonic

ETA: below diff and pastelink to example new output: http://yy.jamsek.net/fha1

I cannot access the link.

Regarding details of how mv and edits are displayed by fossil status:
Anybody who is parsing Fossil's status output should be using fossil json status .

I do not see in the changes whether the json report suffered the same problem. Is that working similarly?

(A side issue: I was a little disappointed to see little code sharing between the human-prettified and JSON reporting.)

I do not see in the changes whether the json report suffered the same problem. Is that working similarly?

Good question. That was coded a decade ago and i don't recall ;). json is not on by default in the trunk builds. It has to be enabled using the configure flag --json.

(A side issue: I was a little disappointed to see little code sharing between the human-prettified and JSON reporting.)

They've diverged, certainly. The JSON bits were initially added in late 2011, back before sqlite natively supported JSON, and haven't evolved all that much since then. "Best case" would be that "someone" (who, me?) go back and reimplement the JSON API to use sqlite's JSON support. That would allow us to get rid of the 3rd-party (==me) cson_amalgamation.[ch] JSON bits.

Once my relocation is finalized (that's now foreseeable within the next 8-ish weeks) that would be a good thing for me to look into more deeply.

Given that json output has not been touched in the timeline (as far as I can see), I would say that this shows that the deficiency reported in this thread persists with respect to "fossil json status" output.

Most definitely. i have marked this thread for revisiting and will resolve that once my internet bandwidth limitations are lifted (sometime in the next 3ish weeks). Assuming Mark doesn't beat me to it ;).

Assuming Mark doesn't beat me to it ;)

:)

The below diff gives fossil json status the same treatment so now a renamed and edited file is reported as both renamed and edited, and both the renamed file name and prior file name are shown in the report too.

An example of the new output can be seen in this paste (first: no changes, second: renamed, third: edited+renamed, last: edited): http://yy.jamsek.net/72ca

Index: src/json_status.c
=======================================================================
hash - 91593f5f65d4c146ebbcde800979b8e9a2623ae2e331ecb6d5aee4c3ff8f4f45
hash + 6b0b99247c64db4073fae83da552c2f1f90859824a70c50e36fbd03f2985d94f
--- src/json_status.c
+++ src/json_status.c
@@ -62,6 +62,7 @@ cson_value * json_page_status(){
       json_set_err( FSL_JSON_E_UNKNOWN, "Can this even happen?" );
       return 0;
   }
+  vfile_check_signature(vid, 0);
   /* TODO: dupe show_common_info() state */
   tmpO = cson_new_object();
   cson_object_set(oPay, "checkout", cson_object_value(tmpO));
@@ -95,12 +96,14 @@ cson_value * json_page_status(){
   cson_object_set( oPay, "files", cson_array_value( aFiles ) );
 
   db_prepare(&q,
-    "SELECT pathname, deleted, chnged, rid, coalesce(origname!=pathname,0)"
+    "SELECT pathname, deleted, chnged, rid, "
+    "     coalesce(origname!=pathname,0), origname"
     "  FROM vfile "
     " WHERE is_selected(id)"
     "   AND (chnged OR deleted OR rid=0 OR pathname!=origname) ORDER BY 1"
   );
   while( db_step(&q)==SQLITE_ROW ){
+    cson_array *aStatuses = NULL;
     const char *zPathname = db_column_text(&q,0);
     int isDeleted = db_column_int(&q, 1);
     int isChnged = db_column_int(&q,2);
@@ -114,8 +117,6 @@ cson_value * json_page_status(){
     }else if( isNew ){
       zStatus = "new" /* maintenance reminder: MUST come
                          BEFORE the isChnged checks. */;
-    }else if( isRenamed ){
-      zStatus = "renamed";
     }else if( !file_isfile_or_link(zFullName) ){
       if( file_access(zFullName, F_OK)==0 ){
         zStatus = "notAFile";
@@ -139,13 +140,26 @@ cson_value * json_page_status(){
         zStatus = "edited";
       }
     }
-
     oFile = cson_new_object();
     cson_array_append( aFiles, cson_object_value(oFile) );
+    if( isRenamed ){
+      if( *zStatus!='?' ){
+        aStatuses = cson_new_array();
+        cson_object_set( oFile, "status", cson_array_value( aStatuses ) );
+        cson_array_append(aStatuses,
+            cson_value_new_string(zStatus, strlen(zStatus)));
+        cson_array_append(aStatuses, cson_value_new_string("renamed", 7));
+      }else{
+        zStatus = "renamed";
+      }
+      cson_object_set( oFile, "priorName",
+          cson_sqlite3_column_to_value(q.pStmt,5));
+    }
     /* optimization potential: move these keys into cson_strings
        to take advantage of refcounting. */
     cson_object_set( oFile, "name", json_new_string( zPathname ) );
-    cson_object_set( oFile, "status", json_new_string( zStatus ) );
+    cson_object_set( oFile, "status", aStatuses!=NULL ?
+        cson_array_value(aStatuses) : json_new_string( zStatus ) );
 
     free(zFullName);
   }

fossil json requires a specially built fossil, which not all users have nor what most distributions supply.

JSON output is also not trivial to parse, depending on what you are doing and where. You might need to require third party utilities (jq), or even end up writing your own parse. In vc-fossil.el, one could use json mode which comes with Emacs to handle the output, but fossil would have to make the --json option enabled by default for a while before I'd change the interfaces or do some fall back hacks so not to break for users of old versions of fossil.