FROMLIST files handled inconsistently

[rbeyer]

ISIS version(s) affected: 3.8.0 and onwards (probably existant for many years)

Description
Many ISIS programs take a 'FROMLIST' argument that is expected to be a list of filenames. These files are not handled consistently in the ISIS codebase.

For example, the cubeit documentation indicates that the last line in the FROMLIST file must be an empty line (but in actuality there can be a blank line or not). Other programs, like hiccdstitch, also accept this file format. However, if you try and give a FROMLIST with a trailing blank line to hijitter, you will get the following cryptic error:

**PROGRAMMER ERROR** There was a problem with calculating the inputs for program [cubeatt]. Please verify your program is not setting outputs for branches that don't have input.

This is because the hijitter program uses the Isis::Pipeline object, which instead of treating the 'FROMLIST' as an Isis::FileList object (like cubeit and hiccdstitch) it initializes an Isis::TextFile object and calls the GetLineNoFilter() method (isis/src/base/objs/Pipeline/Pipeline.cpp, line 381) which treats the last blank line as a filename, and then cubeatt chokes on the blank file name it is given.

Possible Solution
It is not clear to me why Isis::Pipeline initializes its FROMLIST argument as an Isis::TextFile object instead of an Isis::FileList object that can deal with comments and blank lines (isis/src/base/objs/FileList/FileList.cpp). Simply changing this behavior is probably the easiest fix to make things coherent, but care should be taken to make sure that it won't break things, and that the 'flavor' of the FROMLIST that hijitter users (and automatic processing pipelines run by missions) are accustomed to can still be used. Likewise for all other programs which use an Isis::Pipeline object under the hood and take FROMLIST arguments.

Since cubeit can handle a file without the trailing blank line (even though its documentation says it can't), then switching the Isis::Pipeline class to use Isis::FileList instead of Isis::TextFile should result in no differences for existing use cases.

I think there is also an edge case where a file terminates (EOF) without a newline character. In this case there can be good text (another filename) after a 'newline' but there is no ending newline, just an EOF after the filename. In this case, that last filename will get quietly ignored.

I suspect this is why the cubeit documentation indicates that a trailing blank line is required, even though it isn't.

The problem is that before the end of a while loop pass (while loop starts at line 110 in FileList.cpp), in.getline() is called, which slurps up the line into buf but if it reaches EOL before it finds a newline, it sets the eofbit, so that at the top of the next while pass, when in.eof() is tested, it yields true and breaks out of the loop, even though there is a good string in buf.

If the logic of the while loop in the read() function is modified to examine the contents of buf even after in.eof() is True (or before it is tested), then this edge case could also be avoided.

Such a file (with three items, but only two newlines) can be created via Python like so:

from pathlib import Path
t = '\n'.join(['a.cub', 'b.cub', 'c.cub'])
Path('/some/path/to/fromlist.txt').write_text(t)

Very unlikely that someone would accidentally create something like this.

I found this one concrete case of hijitter not using an Isis:FileList object to deal with its FROMLIST parameter, but there may be others. The codebase should be scrubbed (and the documentation on FROMLIST parameters--like in cubeit should be normalized).

[jlaura]

I am going with an enhancement label on this one (though please disagree.). The description as a bug is great and easily reproduced. I do not think we have 'breaking' code though the identified differences in capability definitely look at way. Just organizing and this seems to be an issue in the gray area between the two.

Thanks for the detailed post!

[rbeyer]

That's a fine classification.

[ascbot]

I am a bot that cleans up old issues that do not have activity.

This issue has not received feedback in the last six months. I am going to add the inactive label to
this issue. If this is still a pertinent issue, please add a comment or add an emoji to an existing comment.

I will post again in five months with another reminder and will close this issue on it's birthday unless it has
some activity.

[ascbot]

I am a bot that cleans up old issues that do not have activity.

This issue has not received feedback in the last eleven months! If this is still a pertinent issue, please add a comment or add an emoji to an existing comment.

In one month I will close this issue on it's birthday unless it has some activity.

[rbeyer]

No relevant code has been changed, and so this is still a legitimate enhancement request. As I reviewed this today, I want to be clear that the code in hijitter/main.cpp seems to use Isis::FileList correctly, but hijitter uses the Isis::Pipeline object, and I think it is the Pipeline.cpp code that uses Isis::TextFile incorrectly, for example in line 351:

    TextFile filelist(FileName(ui.GetFileName(inputParam)).expanded());

line 377, and elsewhere.

This is subtle, because it is not apparently obvious that hijitter/main.cpp is doing anything wrong, because it seems to be using Isis::FileList correctly.

Again, addressing this issue in the Isis::Pipeline object would close this Issue, but there might be other similar issues in the codebase.

[acpaquette]

@rbeyer The fix is up for this. It may be beneficial for some potential user testing to see if this fixes the issue/makes things consistent.

Regarding cubeit and its fromlist parameter. It can take a list file without a trailing new line, but as you pointed out the last line of the list is ignored. It looks like the last decision to be made before this can be closed is should the FileList be able to handle a list with no trailing new line OR should we gracefully fail with no new line character

[rbeyer]

My opinion (unless this fundamentally breaks other things) is that no final newline should be needed (but should be tolerated), the newlines are just separating list elements. If we think of "newlines" as commas, and require a final trailing newline, then we require this:

A,B,C,

and would reject this:

A,B,C

or would only read A & B, which would also be dumb. Whatever reads either of these lists should collect three elements.