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

Misc cleanups on git command invocation #2521

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Misc cleanups on git command invocation #2521

merged 2 commits into from
Oct 3, 2024

Conversation

cklin
Copy link
Contributor

@cklin cklin commented Oct 3, 2024

This PR contains two cleanups related to how the action invokes git commands.

  • The first commit extracts a runGitCommand() function, which reduces code duplication among functions that call git.
  • The second commit renames determineMergeBaseCommitOid() so that its name more clearly matches what it does.

This PR is intended to be a refactoring, but there are some changes to git-invocation error messages.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

The name suggests that the function computes the merge base, which for
Git means specifically the best common ancestors between multiple
commits or branches (see `git merge-base`).

But what the function actually does is to calculate the HEAD commit of
the PR base branch, as derived from the PR merge commit that the action
analyzes. So even though the function has to do with "merge" and "base",
using the term "merge base" is still misleading at best.

This commit renames the function to determineBaseBranchHeadCommitOid(),
which more clearly indicates what the function does.
@cklin cklin marked this pull request as ready for review October 3, 2024 16:34
@cklin cklin requested a review from a team as a code owner October 3, 2024 16:34
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor! Renaming seems a lot clearer now too 😸

@cklin cklin merged commit 8b33300 into main Oct 3, 2024
546 checks passed
@cklin cklin deleted the cklin/run-git-command branch October 3, 2024 20:40
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.

2 participants