Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: don't sanitize path separators in game & ROM names #1019

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Refactor: don't sanitize path separators in game & ROM names #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed7f1db
Refactor: don't sanitize path separators in game & ROM names
emmercm Mar 20, 2024
6613723
Merge branch 'main' into emmercm/dont-sanitize-paths
emmercm Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 10 additions & 16 deletions src/modules/candidatePatchGenerator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export default class CandidatePatchGenerator extends Module {
entryPath: unpatchedReleaseCandidate.getRomsWithFiles().length === 1
? extractedFileName
: outputFile.getEntryPath(),
size: patch.getSizeAfter() ?? 0,
size: patch.getSizeAfter(),
crc32: patch.getCrcAfter(),
fileHeader: outputFile.getFileHeader(),
patch: outputFile.getPatch(),
Expand All @@ -159,21 +159,18 @@ export default class CandidatePatchGenerator extends Module {
const dirName = path.dirname(outputFile.getFilePath());
outputFile = await File.fileOf({
filePath: path.join(dirName, extractedFileName),
size: patch.getSizeAfter() ?? 0,
size: patch.getSizeAfter(),
crc32: patch.getCrcAfter(),
fileHeader: outputFile.getFileHeader(),
patch: outputFile.getPatch(),
}, ChecksumBitmask.NONE); // don't calculate anything, the file doesn't exist
}

// Build a new ROM from the output file's info
let romName = path.basename(outputFile.getExtractedFilePath());
if (dat.getRomNamesContainDirectories()) {
romName = path.join(
path.dirname(rom.getName().replace(/[\\/]/g, path.sep)),
romName,
);
}
const romName = path.join(
path.dirname(rom.getName().replace(/[\\/]/g, path.sep)),
path.basename(outputFile.getExtractedFilePath()),
);
rom = new ROM({
name: romName,
size: outputFile.getSize(),
Expand All @@ -187,13 +184,10 @@ export default class CandidatePatchGenerator extends Module {
}));

// Build a new Game from the ROM's info
let gameName = patchedRomName;
if (dat.getRomNamesContainDirectories()) {
gameName = path.join(
path.dirname(unpatchedReleaseCandidate.getGame().getName().replace(/[\\/]/g, path.sep)),
gameName,
);
}
const gameName = path.join(
path.dirname(unpatchedReleaseCandidate.getGame().getName().replace(/[\\/]/g, path.sep)),
patchedRomName,
);
const patchedGame = unpatchedReleaseCandidate.getGame().withProps({
name: gameName,
});
Expand Down
1 change: 0 additions & 1 deletion src/modules/datCombiner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export default class DATCombiner extends Module {
`Combined DAT generated by ${Constants.COMMAND_NAME} v${Constants.COMMAND_VERSION}`,
...dats.map((dat) => dat.getName()),
].join('\n'),
romNamesContainDirectories: dats.some((dat) => dat.getRomNamesContainDirectories()),
});
}
}
6 changes: 1 addition & 5 deletions src/modules/datMergerSplitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import ProgressBar, { ProgressBarSymbol } from '../console/progressBar.js';
import ArrayPoly from '../polyfill/arrayPoly.js';
import DAT from '../types/dats/dat.js';
import Game from '../types/dats/game.js';
import Header from '../types/dats/logiqx/header.js';
import LogiqxDAT from '../types/dats/logiqx/logiqxDat.js';
import Machine from '../types/dats/mame/machine.js';
import Parent from '../types/dats/parent.js';
Expand Down Expand Up @@ -50,10 +49,7 @@ export default class DATMergerSplitter extends Module {

const newGames = dat.getParents()
.flatMap((parent) => this.mergeParent(dat, parent, gameNamesToGames));
const newDat = new LogiqxDAT(new Header({
...dat.getHeader(),
romNamesContainDirectories: this.options.getMergeRoms() === MergeMode.MERGED,
}), newGames);
const newDat = new LogiqxDAT(dat.getHeader(), newGames);
this.progressBar.logTrace(`${newDat.getNameShort()}: merged/split to ${newDat.getGames().length.toLocaleString()} game${newDat.getGames().length !== 1 ? 's' : ''}`);

this.progressBar.logTrace(`${newDat.getNameShort()}: done merging & splitting`);
Expand Down
1 change: 0 additions & 1 deletion src/modules/datScanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,6 @@ export default class DATScanner extends Scanner {
return new LogiqxDAT(new Header({
name: datName,
description: datName,
romNamesContainDirectories: true,
}), games);
}

Expand Down
10 changes: 0 additions & 10 deletions src/types/dats/dat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,16 +94,6 @@ export default abstract class DAT {
return this.getHeader().getDescription();
}

@Memoize()
getRomNamesContainDirectories(): boolean {
return this.getHeader().getRomNamesContainDirectories() ?? (
// Assume BIOS DATs know what they're doing with path characters
this.isBiosDat()
// At least 50% of games have at least one ROM with path characters
|| this.getGames().filter((game) => game.getRoms().some((rom) => rom.getName().match(/[\\/]/) !== null)).length > this.getGames().length / 2
);
}

/**
* Get a No-Intro style filename.
*/
Expand Down
12 changes: 0 additions & 12 deletions src/types/dats/logiqx/header.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ interface HeaderOptions {
readonly comment?: string;
readonly clrMamePro?: ClrMamePro;
// readonly romCenter?: RomCenter;

/**
* igir-custom properties.
*/
readonly romNamesContainDirectories?: boolean;
}

/**
Expand Down Expand Up @@ -78,8 +73,6 @@ export default class Header implements HeaderOptions {
@Transform(({ value }) => value || undefined)
readonly clrMamePro?: ClrMamePro;

readonly romNamesContainDirectories?: boolean;

constructor(options?: HeaderOptions) {
this.name = options?.name ?? '';
this.description = options?.description;
Expand All @@ -89,7 +82,6 @@ export default class Header implements HeaderOptions {
this.url = options?.url;
this.comment = options?.comment;
this.clrMamePro = options?.clrMamePro;
this.romNamesContainDirectories = options?.romNamesContainDirectories;
}

/**
Expand Down Expand Up @@ -129,10 +121,6 @@ export default class Header implements HeaderOptions {
return this.clrMamePro;
}

getRomNamesContainDirectories(): boolean | undefined {
return this.romNamesContainDirectories;
}

// Computed getters

/**
Expand Down
13 changes: 4 additions & 9 deletions src/types/outputFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ export default class OutputFactory {
return `${game.getName()}.zip`;
}

const romBasename = this.getRomBasename(options, dat, rom, inputFile);
const romBasename = this.getRomBasename(game, rom, inputFile);

if (
!(inputFile instanceof ArchiveEntry || FileFactory.isArchive(inputFile.getFilePath()))
Expand All @@ -512,7 +512,7 @@ export default class OutputFactory {
rom: ROM,
inputFile: File,
): string {
const romBasename = this.getRomBasename(options, dat, rom, inputFile);
const romBasename = this.getRomBasename(game, rom, inputFile);
if (!options.shouldZipFile(rom.getName())) {
return romBasename;
}
Expand All @@ -527,16 +527,11 @@ export default class OutputFactory {
}

private static getRomBasename(
options: Options,
dat: DAT,
game: Game,
rom: ROM,
inputFile: File,
): string {
let romNameSanitized = rom.getName();
if (!dat.getRomNamesContainDirectories()) {
romNameSanitized = romNameSanitized?.replace(/[\\/]/g, '_');
}

const romNameSanitized = rom.getName().replace(/[\\/]/g, path.sep);
const { base, ...parsedRomPath } = path.parse(romNameSanitized);

// Alter the output extension of the file
Expand Down
62 changes: 31 additions & 31 deletions test/modules/candidatePostProcessor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ it('should do nothing with no options', async () => {
path.join('Output', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'disk1_Cheerful.rom'),
path.join('Output', 'disk1_Confident.rom'),
path.join('Output', 'disk1_Cool.rom'),
path.join('Output', 'disk1', 'Cheerful.rom'),
path.join('Output', 'disk1', 'Confident.rom'),
path.join('Output', 'disk1', 'Cool.rom'),
]);
});

Expand All @@ -120,9 +120,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D', 'disk1_Cheerful.rom'),
path.join('Output', 'D', 'disk1_Confident.rom'),
path.join('Output', 'D', 'disk1_Cool.rom'),
path.join('Output', 'D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D', 'disk1', 'Confident.rom'),
path.join('Output', 'D', 'disk1', 'Cool.rom'),
]],
[2, [
path.join('Output', 'A1', 'Admirable.rom'),
Expand All @@ -140,9 +140,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D2', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D3', 'disk1_Cheerful.rom'),
path.join('Output', 'D3', 'disk1_Confident.rom'),
path.join('Output', 'D4', 'disk1_Cool.rom'),
path.join('Output', 'D3', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D3', 'disk1', 'Confident.rom'),
path.join('Output', 'D3', 'disk1', 'Cool.rom'),
]],
[3, [
path.join('Output', 'A1', 'Admirable.rom'),
Expand All @@ -160,9 +160,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D2', 'disk1_Cheerful.rom'),
path.join('Output', 'D2', 'disk1_Confident.rom'),
path.join('Output', 'D3', 'disk1_Cool.rom'),
path.join('Output', 'D2', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D2', 'disk1', 'Confident.rom'),
path.join('Output', 'D2', 'disk1', 'Cool.rom'),
]],
])('it should split the letter dirs based on limit: %s', async (dirLetterLimit, expectedFilePaths) => {
const options = new Options({
Expand Down Expand Up @@ -204,9 +204,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'A-D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'A-D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'A-D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'A-D', 'disk1_Cheerful.rom'),
path.join('Output', 'A-D', 'disk1_Confident.rom'),
path.join('Output', 'A-D', 'disk1_Cool.rom'),
path.join('Output', 'A-D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'A-D', 'disk1', 'Confident.rom'),
path.join('Output', 'A-D', 'disk1', 'Cool.rom'),
]],
[1, 2, [
path.join('Output', 'A-A1', 'Admirable.rom'),
Expand All @@ -224,9 +224,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D2', 'disk1_Cheerful.rom'),
path.join('Output', 'D-D3', 'disk1_Confident.rom'),
path.join('Output', 'D-D3', 'disk1_Cool.rom'),
path.join('Output', 'D-D2', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D-D2', 'disk1', 'Confident.rom'),
path.join('Output', 'D-D2', 'disk1', 'Cool.rom'),
]],
[1, 3, [
path.join('Output', 'A-A', 'Admirable.rom'),
Expand All @@ -240,13 +240,13 @@ describe('dirLetterGroup', () => {
path.join('Output', 'B-D', 'Dainty', 'Dainty.cue'),
path.join('Output', 'B-D', 'Daring', 'Daring (Track 01).bin'),
path.join('Output', 'B-D', 'Daring', 'Daring.cue'),
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling (Track 01).bin'),
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D1', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D1', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D1', 'disk1_Cheerful.rom'),
path.join('Output', 'D-D2', 'disk1_Confident.rom'),
path.join('Output', 'D-D2', 'disk1_Cool.rom'),
path.join('Output', 'D-D', 'Dazzling', 'Dazzling (Track 01).bin'),
path.join('Output', 'D-D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D-D', 'disk1', 'Confident.rom'),
path.join('Output', 'D-D', 'disk1', 'Cool.rom'),
]],
[2, 3, [
path.join('Output', 'AD-AD', 'Admirable.rom'),
Expand All @@ -264,9 +264,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'DA-DI', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'DA-DI', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'DA-DI', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'DA-DI', 'disk1_Cheerful.rom'),
path.join('Output', 'DI-DI', 'disk1_Confident.rom'),
path.join('Output', 'DI-DI', 'disk1_Cool.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Cheerful.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Confident.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Cool.rom'),
]],
[3, 4, [
path.join('Output', 'ADM-AMA', 'Admirable.rom'),
Expand All @@ -284,9 +284,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'DAR-DIS', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'DAR-DIS', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'DAR-DIS', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'DAR-DIS', 'disk1_Cheerful.rom'),
path.join('Output', 'DIS-DIS', 'disk1_Confident.rom'),
path.join('Output', 'DIS-DIS', 'disk1_Cool.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Cheerful.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Confident.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Cool.rom'),
]],
])('it should group based on count & limit: %s, %s', async (dirLetterCount, dirLetterLimit, expectedFilePaths) => {
const options = new Options({
Expand Down
57 changes: 0 additions & 57 deletions test/outputFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,63 +58,6 @@ test.each([
});
});

describe('ROM names with directories', () => {
const rom = new ROM({ name: 'subdir\\file.rom', size: 0, crc32: '00000000' });
const game = new Game({ name: 'Game', rom: [rom] });

describe('command: zip', () => {
const options = new Options({ commands: ['copy', 'zip'], output: os.devNull });

test.each([
[true, path.join('subdir', 'file.rom')],
[false, 'subdir_file.rom'],
])('should respect romNamesContainDirectories: %s', async (romNamesContainDirectories, expectedEntryPath) => {
const dat = new LogiqxDAT(new Header({ romNamesContainDirectories }), [game]);

const outputPath = OutputFactory.getPath(
options,
dat,
game,
dummyRelease,
rom,
await rom.toFile(),
);
expect(outputPath).toEqual({
root: '',
dir: os.devNull,
base: '',
name: 'Game',
ext: '.zip',
entryPath: expectedEntryPath,
});
});
});

describe.each([
[['copy']],
[['copy', 'extract']],
])('commands: %s', (commands) => {
const options = new Options({ commands, output: os.devNull });

test.each([
[true, path.join(os.devNull, 'subdir', 'file.rom')],
[false, path.join(os.devNull, 'subdir_file.rom')],
])('should respect romNamesContainDirectories: %s', async (romNamesContainDirectories, expectedFormattedPath) => {
const dat = new LogiqxDAT(new Header({ romNamesContainDirectories }), [game]);

const outputPath = OutputFactory.getPath(
options,
dat,
game,
dummyRelease,
rom,
await rom.toFile(),
);
expect(outputPath.format()).toEqual(expectedFormattedPath);
});
});
});

describe('token replacement', () => {
test.each([
['foo/{datName}/bar', path.join('foo', 'DAT _ Name', 'bar', 'Dummy.rom')],
Expand Down
Loading