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

Change ArborX hash to "hash-dirty" if worktree is not clean #558

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Oct 15, 2021

If a tree is dirty, it will print <hash>-dirty. This is helpful when we share logs.

One caveat is that if the HEAD points to an annotated tag, it will print a tag instead of a hash. However, I think we are only using lightweight tags, so I don't see much of an issue, and feel like it's worth doing this.

@dalg24
Copy link
Contributor

dalg24 commented Oct 15, 2021

Did you look how other projects deal with this?
For instance https://github.com/google/benchmark/blob/main/cmake/GetGitVersion.cmake which seems to be used in llvm-project as well.

I don't think it is an issue to use a tag when it exists.

@aprokop
Copy link
Contributor Author

aprokop commented Oct 15, 2021

I chose the path of least effort to improve status quo. It was not my intent to provide the final solution to the problem, which your reference may be.

I don't think it is an issue to use a tag when it exists.

The only concern I have about this is that tags maybe removed and readded, and correspond to different hashes. If we promise not to do that, it is not a concern. Or we can promise not to add annotated tags, in which case it's always a hash.

@aprokop aprokop marked this pull request as draft October 15, 2021 21:40
@aprokop aprokop force-pushed the update_arborx_hash branch from 98e5754 to 4b4b7d1 Compare October 16, 2021 00:10
@aprokop
Copy link
Contributor Author

aprokop commented Oct 16, 2021

Opted in favor of rev-parse and status -porcelain. This should be robust.
I'm not sure if the code in benchmark's git cmake brings any benefit to this:

      # Work out if the repository is dirty
      execute_process(COMMAND ${GIT_EXECUTABLE} update-index -q --refresh
          WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
          OUTPUT_QUIET
          ERROR_QUIET)
      execute_process(COMMAND ${GIT_EXECUTABLE} diff-index --name-only HEAD --
          WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
          OUTPUT_VARIABLE GIT_DIFF_INDEX
          ERROR_QUIET)
      string(COMPARE NOTEQUAL "${GIT_DIFF_INDEX}" "" GIT_DIRTY)
      if (${GIT_DIRTY})
          set(GIT_DESCRIBE_VERSION "${GIT_DESCRIBE_VERSION}-dirty")
      endif()

@aprokop aprokop marked this pull request as ready for review October 16, 2021 00:14
@aprokop aprokop added the API User visible interface modifications label Oct 16, 2021
cmake/SetupVersion.cmake Outdated Show resolved Hide resolved
@aprokop aprokop force-pushed the update_arborx_hash branch from d14ffe8 to 19459dc Compare October 16, 2021 00:18
@aprokop aprokop changed the title Use describe for ArborX hash to include worktree status Change ArborX hash to "hash-dirty" if worktree is not clean Oct 16, 2021
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Fine by me. I only have a minor comment.

cmake/SetupVersion.cmake Outdated Show resolved Hide resolved
@aprokop aprokop force-pushed the update_arborx_hash branch from 19459dc to 4c2ae15 Compare October 20, 2021 13:13
@aprokop
Copy link
Contributor Author

aprokop commented Oct 20, 2021

@dalg24 Changed to --untracked-files=no.

@aprokop aprokop merged commit 889e848 into arborx:master Oct 20, 2021
@aprokop aprokop deleted the update_arborx_hash branch October 20, 2021 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API User visible interface modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants