Skip to content

Add quickgogen command to gogen touched directories/files #75

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bin/quickgogen
26 changes: 26 additions & 0 deletions lib/git.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ function get_current_branch() {
echo "${ref#refs/heads/}"
}

# Gets the main branch or dies.
function get_main_branch_or_die() {
# From https://stackoverflow.com/questions/28666357/git-how-to-get-default-branch#comment92366240_50056710
local main_branch
main_branch="$(git remote show origin | grep "HEAD branch" | sed 's/.*: //')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to || die here as well? (Yes, doing || after an assignment is perfectly legal)

[[ -n "${main_branch}" ]] || die "Failed to get main branch"
echo "${main_branch}"
}

# Gets the diffbase off of the remote's main branch or dies.
function get_diffbase_or_die() {
diffbase="$(git merge-base HEAD "origin/${main_branch}")"
[[ $? -eq 0 ]] || die "Failed to determine diffbase"
Comment on lines +25 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
diffbase="$(git merge-base HEAD "origin/${main_branch}")"
[[ $? -eq 0 ]] || die "Failed to determine diffbase"
local diffbase
diffbase="$(git merge-base HEAD "origin/${main_branch}")" || die "Failed to determine diffbase"

echo "${diffbase}"
}

function get_remote() {
local remote
remote="$(git config --get remote.origin.url)"
Expand Down Expand Up @@ -67,3 +83,13 @@ function branch_exists() {
local branch="$1"
git show-ref --verify --quiet "refs/heads/$branch"
}

# TODO(do-not-merge): How return multi-line output properly?
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this works now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this referred to

# Requires $gitroot.
# Returns the list of .go file paths in $gitroot (with $gitroot prefix)
# that have a comment saying they are generated files.
function quick_get_generated_files {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function quick_get_generated_files {
function quick_get_generated_files() {

echo "$(git -C "$gitroot" \
grep -l '^// Code generated by .* DO NOT EDIT\.' -- '*.go' | \
sed -e "s@^@${gitroot}/@")"
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer

Suggested change
sed -e "s@^@${gitroot}/@")"
awk -v PREFIX="${gitroot}/" '{print PREFIX $0}')"

You never know who might depend on using @ as a character in their directory names..

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add new line

64 changes: 64 additions & 0 deletions scripts/dev/quickgogen.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#!/usr/bin/env bash

# Usage: Runs gogen for all Go files that have changed between the current code and master.

SCRIPT="$(python -c 'import os, sys; print(os.path.realpath(sys.argv[1]))' "${BASH_SOURCE[0]}")"
source "$(dirname "$SCRIPT")/../../lib/common.sh"
source "$(dirname "$SCRIPT")/../../lib/git.sh"

gitroot="$(git rev-parse --show-toplevel)"
[[ $? -eq 0 ]] || die "Current directory is not a git repository."

if [[ -f "${gitroot}/go.mod" ]]; then
export GO111MODULE=on
fi

# Various code generation helpers are expected to be in the PATH when called by go generate.
export PATH="$PATH:${gitroot}/tools/generate-helpers"

main_branch="$(get_main_branch_or_die)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, $(...) will always spawn a subshell, and there's only exiting a single shell - no exception control flow (other than signal/trap hackery). That means the die is basically nothing more than a return in this context; since the script doesn't have -e set, execution will actually continue.

diffbase="$(get_diffbase_or_die)"

generated_files="$(git -C "$gitroot" grep -l '^// Code generated by .* DO NOT EDIT\.' -- '*.go' | sed -e "s@^@${gitroot}/@")"
Copy link
Contributor

Choose a reason for hiding this comment

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

See above wrt awk instead of sed


IFS=$'\n' read -d '' -r -a changed_files < <(
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It might've been inconsistent in the scripts before, but here you're using tab indent vs 2 spaces in the previous file. Imho 2 spaces is much easier on the eyes. We typically only use tabs where prescribed by the language (Go, Makefile). Quite a shame, given that tab indent is way superior to space indent, but eh...

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, looks like I asked you to use this indent back in August? :P

git diff "$diffbase" --name-status . |
sed -n -E -e "s@^[AM][[:space:]]+|^R[^[:space:]]*[[:space:]]+[^[:space:]]+[[:space:]]+@${gitroot}/@p" ;
echo "$generated_files" ; echo "$generated_files"
} | sort | uniq -u) || true

function private_gogen() {
local status=0
local changed_files=("$@")
local gofiles

IFS=$'\n' read -d '' -r -a gofiles < <(
printf '%s\n' "${changed_files[@]}" |
grep '\.go$'
)
[[ "${#gofiles[@]}" == 0 ]] && return 0

IFS=$'\n' read -d '' -r -a godirs < <(
for f in "${gofiles[@]}"; do dirname "$f"; done |
sort | uniq)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sort | uniq)
sort -u)

slightly more concise, and just as POSIX-compliant


einfo "Running go generate..."
Copy link
Contributor

Choose a reason for hiding this comment

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

indent changes again

Copy link
Author

Choose a reason for hiding this comment

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

Done

for dir in "${godirs[@]}"; do
einfo "...Generating for ${dir}"
( cd "$dir" && go generate "./" ) && (( status == 0 ))
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to remark that this won't work due to $PATH, and then realized that the export PATH= line is present above, and that private_gogen is derived from gogen.sh. Given that gogen is part of the workflow repo, I wonder if the whole script could be simplified slightly if we were to just rely on it? We won't save much, but for consistency it would be nice to just delegate to gogen <list of dirs with changed go files that aren't generated> at the end

status=$?
done

return "${status}"
}

[[ "${#changed_files[@]}" -eq 0 ]] && { ewarn "No relevant changes found in current directory."; exit 0; }
status=0

private_gogen "${changed_files[@]}" && (( status == 0 ))
Copy link
Contributor

Choose a reason for hiding this comment

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

status is always 0 here

status=$?

[[ "${status}" -eq 0 ]] && exit 0
efatal "An error occurred while generating files"
exit 1
9 changes: 3 additions & 6 deletions scripts/dev/quickstyle.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
SCRIPT="$(python -c 'import os, sys; print(os.path.realpath(sys.argv[1]))' "${BASH_SOURCE[0]}")"
source "$(dirname "$SCRIPT")/../../lib/common.sh"
source "$(dirname "$SCRIPT")/../../setup/packages.sh"
source "$(dirname "$SCRIPT")/../../lib/git.sh"

check_dependencies

Expand Down Expand Up @@ -222,12 +223,8 @@ if [[ -f "${gitroot}/go.mod" ]]; then
export GO111MODULE=on
fi

# https://stackoverflow.com/a/44750379
main_branch="$(git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@')"
[[ -n "${main_branch}" ]] || die "Failed to get main branch"

diffbase="$(git merge-base HEAD "origin/${main_branch}")"
[[ $? -eq 0 ]] || die "Failed to determine diffbase"
main_branch="$(get_main_branch_or_die)"
diffbase="$(get_diffbase_or_die)"

generated_files="$(git -C "$gitroot" grep -l '^// Code generated by .* DO NOT EDIT\.' -- '*.go' | sed -e "s@^@${gitroot}/@")"

Expand Down