phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
MIT License
2 stars 5 forks source link

Prevent many git checkout opperations when unneeded #355

Open zepumph opened 3 months ago

zepumph commented 3 months ago

This has been biting @jonathanolson during his current maintenance release, for example, here is the current implementation of ReleaseBranch.usesChipper2():

    async usesChipper2() {
      await gitCheckout( this.repo, this.branch );
      const dependencies = await getDependencies( this.repo );
      await gitCheckout( 'chipper', dependencies.chipper.sha );

      const chipperVersion = ChipperVersion.getFromRepository();

      const result = chipperVersion.major !== 0 || chipperVersion.minor !== 0;

      await gitCheckout( this.repo, 'main' );
      await gitCheckout( 'chipper', 'main' );

      return result;
    }

Gross! Why does it need to checkout so many branches?!?!?! Let's try to reduce that so that ReleaseBranches can multitask better.

zepumph commented 3 months ago

@jonathanolson and I came to a solution that uses git cat-file blob. This is working really well in testing, and I think it would be a nice improvement for many, many usages of gitCheckout in perennial/js/common.

zepumph commented 3 months ago

I got to a commit point, but right now I'm just adjusting ReleaseBranch.usesChipper2(). More to come if we don't run into trouble.

Here is how I tested things:

( async () => {

  // TEST ONE
  const ReleaseBranch = require( '../perennial/js/common/ReleaseBranch' );

  const rb12 = new ReleaseBranch( 'acid-base-solutions', '1.2', [ 'phet' ], true );
  const rb13 = new ReleaseBranch( 'acid-base-solutions', '1.3', [ 'phet' ], true );
  console.log( 'acid-base-solutions 1.2 uses chipper 2.0: ', await rb12.usesChipper2() ); // -> false
  console.log( 'acid-base-solutions 1.3 uses chipper 2.0: ', await rb13.usesChipper2() ); // -> true

  /// TEST TWO
  const assert = require( 'assert' );
  const _ = require( 'lodash' );
  const gitCatFile = require( '../perennial/js/common/gitCatFile' );
  const gitCheckout = require( '../perennial/js/common/gitCheckout' );
  const loadJSON = async ( repo, file, branch ) => {
    return JSON.parse( await gitCatFile( repo, file, branch ) );
  };

  // acid base solutions
  // git checkout d1b56e98e01bec64769402037ef60e1e81f90b00
  const testSHA = 'd1b56e98e01bec64769402037ef60e1e81f90b00';
  await gitCheckout( 'acid-base-solutions', testSHA );

  const defaultHEADDependencies = await loadJSON( 'acid-base-solutions', 'dependencies.json' );
  const currentSHADependencies = await loadJSON( 'acid-base-solutions', 'dependencies.json', testSHA );
  const mainDependencies = await loadJSON( 'acid-base-solutions', 'dependencies.json', 'main' );

  assert( _.isEqual( defaultHEADDependencies, currentSHADependencies ) );
  assert( defaultHEADDependencies[ 'acid-base-solutions' ].sha !== mainDependencies[ 'acid-base-solutions' ].sha );
  await gitCheckout( 'acid-base-solutions', 'main' );

  // TEST THREE
  console.log( ( await gitCatFile( 'density-buoyancy-common', 'js/density/model/DensityIntroModel.ts', 'density-1.1' ) ).length, 'DensityIntroModel file chars on density-1.1 branch' );
  console.log( ( await gitCatFile( 'density-buoyancy-common', 'js/density/model/DensityIntroModel.ts' ) ).length, 'DensityIntroModel file chars on main branch' );
} )();
zepumph commented 3 months ago

@jonathanolson please report any trouble you find.

zepumph commented 3 months ago

@jonathanolson mentioned that we are already doing this with getFileFromBranch So likely this issue is moot, and we can get rid of gitCatFile.