[rush] Find rush.json location by using 'dirname' at most 10 times

[tianmagongyue]

I see rush try to find rush.json location by using 'Path.dirname' at most 10 times. Why was the final decision made ten times? It seems like a magic number. Why not 8 or 12 times?

for (let i: number = 0; i < 10; ++i) {
  const rushJsonFilename: string = path.join(currentFolder, RushConstants.rushJsonFilename);

  if (FileSystem.exists(rushJsonFilename)) {
    if (i > 0 && verbose) {
      // eslint-disable-next-line no-console
      console.log('Found configuration in ' + rushJsonFilename);
    }

    if (verbose) {
      // eslint-disable-next-line no-console
      console.log('');
    }

    return rushJsonFilename;
  }

  const parentFolder: string = path.dirname(currentFolder);
  if (parentFolder === currentFolder) {
    break;
  }

  currentFolder = parentFolder;
}

[iclanton]

The way this code was merged into main is slightly weird (it was a cherry-pick from a rush-next branch about eight years ago), so I'm not finding any PR discussion around this. I'm pretty sure it's an arbitrary number that @octogonz picked with the thought that the CWD will probably never be >10 levels deeper than the repo root.

That code likely can/should be refactored to just keep going until the disk root is reached. I might do that in the next few days, unless you, @tianmagongyue, are interested in doing that.

[tianmagongyue]

The way this code was merged into main is slightly weird (it was a cherry-pick from a rush-next branch about eight years ago), so I'm not finding any PR discussion around this. I'm pretty sure it's an arbitrary number that @octogonz picked with the thought that the CWD will probably never be >10 levels deeper than the repo root.

That code likely can/should be refactored to just keep going until the disk root is reached. I might do that in the next few days, unless you, @tianmagongyue, are interested in doing that.

OK, I can try to refactor this method with the idea 'keep going until the repo root is reached'. But it may take a little longer for me, and I need you or others to help me review the code.

[octogonz]

I was probably worried about the disk i/o cost in the negative case, for example if "rushx" is invoked repeatedly in a context where rush.json will never be found. Agree that this limit can be removed.

[tianmagongyue]

I was probably worried about the disk i/o cost in the negative case, for example if "rushx" is invoked repeatedly in a context where rush.json will never be found. Agree that this limit can be removed.

Got it

[octogonz]

Btw there is a specific way to reliably detect when you reach the root on NTFS vs Unix filesystems. The PackageJsonLookup API in this repo is a good example to copy

[tianmagongyue]

hi, I have refactored the tryFindRushJsonLocation function, and I need to be a collaborator of rushstack so that I can start a merge request. cc @iclanton @octogonz

[iclanton]

@tianmagongyue - did you create a fork of this repo and push your feature branch there? You shouldn't need any permissions to create a PR from a fork.

[tianmagongyue]

@tianmagongyue - did you create a fork of this repo and push your feature branch there? You shouldn't need any permissions to create a PR from a fork.

ok, I'll create a fork