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.

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

Append random id to log files so they get ignored during git-diff #285

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kunalspathak
Copy link
Member

Today, when doing diff, we perform git diff on base and diff. These folders has .dasm as well as .log files. The .log files are comparable in size. For e.g., they are 520MB while .dasm files are 800MB. Once we do git diff, we then filter the diffs that are coming out of .dasm files. Thus, we unnecessarily spend time diffing the .log files. In #284, I added a way to ignore files if they are not present in both the folders. With that, I propose to add a random ID to the log files so they automatically get ignored when doing git diff and thus saving some time.

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a reasonable approach to me.

@BruceForstall
Copy link
Member

Seems like kind of a hack.

Is git diff recursive? If not (or we can invoke it to not recurse), could we put the log files in a log subdirectory? E.g., base\log\xxx.log, diff\log\xxx.log?

@kunalspathak
Copy link
Member Author

kunalspathak commented Sep 16, 2020

Seems like kind of a hack.

If not (or we can invoke it to not recurse), could we put the log files in a log subdirectory? E.g., base\log\xxx.log, diff\log\xxx.log?

I agree. Better way is to just create separate logs folder for base and diff. With that, it will create 2 new folders baselogs and difflogs that will contain log files. So when diffing, git diff diff base should just work fine.

dasmset_62
|-- base
|-- diff
|-- baselogs
|-- difflogs

Is git diff recursive?

git diff --no-index has limitation because it is done on non repo. If git diff is done on repo, we can do file filtering, recursive, etc.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Looks from the code like you've gone with something closer to my suggestion, namely:

dasmset_62
|-- base
     |-- logs
|-- diff
     |-- logs

The title of this PR is now inaccurate.

src/jit-dasm-pmi/jit-dasm-pmi.cs Outdated Show resolved Hide resolved
src/jit-dasm/jit-dasm.cs Outdated Show resolved Hide resolved
@AndyAyersMS
Copy link
Member

For more complex structures the diff / base output may be directory trees (eg if you run jit-diff on the test tree). What happens then?

You can test this by just creating a small directory tree and placing some test apps in the leaves and then using --assembly to point at the root of the tree.

@kunalspathak
Copy link
Member Author

Here is how it produces with my latest change. It is not very pleasant...Do you want me to find the common ancestor and create log folders in that?

binaries folder:

binaries
|--- 1
     |--- foo.dll
|--- 2
     |--- bar.dll
|--- 3
     |--- baz.dll
|--- projs.dll

Output:

dasmset_63
|--- base
     |--- projs.dasm
     |--- 1
          |--- foo.dasm
     |--- 1log
          |--- foo.log
     |--- 2
          |--- bar.dasm
     |--- 2log
          |--- bar.log
     |--- 3
          |--- baz.dasm
     |--- 3log
          |--- baz.log
    |--- projs.dasm
|---baselog
    |---projs.log
|--- diff
     |--- projs.dasm
     |--- 1
          |--- foo.dasm
     |--- 1log
          |--- foo.log
     |--- 2
          |--- bar.dasm
     |--- 2log
          |--- bar.log
     |--- 3
          |--- baz.dasm
     |--- 3log
          |--- baz.log
    |--- projs.dasm
|---difflog
    |---projs.log

@BruceForstall
Copy link
Member

Everything goes under a _rootPath, which is a user-specified argument, so you have to put everything in that path.

You could create a new _logsRootPath argument (e.g., "--logsOutputPath") and put everything in a tree under that. If passed the same directory as _rootPath, behavior would be the same as today. Or, if passed something different, the logs are separated. You would have to pass this additional argument in from jit-diff. It could default to the same as _rootPath.

Base automatically changed from master to main March 10, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants