Add quickgogen command to gogen touched directories/files #75

ebensh · 2021-08-03T13:01:56Z

Tested:

Checked roxhelp --list-all to confirm the expected comment was listed next to the command
Ran the command from the root of rox repo after making changes to a datastore file, checked that the correct mocks were re-generated.

misberner

I like it! However, wdyt about the following: rather than basing this on top of the smart-branch tool and marker commits, follow the quickstyle model of identifying where the current branch diverged from master, and use that as a basis for "what has changed"?

In effect, I think the differences would be:

quickgogen would work for people not using smart-branch, smart-gogen would not
If you have multiple smart branches stacked on top of each other, smart-gogen would only consider files changed in the top branch, quickgogen would consider all files since master
For somebody using smart-branch with only a single branch on top of master, the behavior would be the same.

WDYT?

misberner · 2021-08-10T19:46:34Z

scripts/dev/quickgogen.sh

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


This might be a quickstyle artifact, but the indent changes here

I'm not sure which part you're referring to, sorry if I'm missing something obvious. Do you mean the file redirect subshell (is this the right term?) shouldn't be indented? This line has two spaces, which matches the opening IFS= line

Ahhh never mind, I see now - tabs v spaces. Judging from quickstyle we prefer tabs over spaces.

FWIW quickstyle didn't do anything about this when run, and nothing in goland highlighted the difference to me. If there's an option I can turn on somewhere for quickstyle I'll try to find one, same for goland.

misberner · 2021-08-10T19:46:47Z

scripts/dev/quickgogen.sh

+		for f in "${gofiles[@]}"; do dirname "$f"; done |
+    sort | uniq)
+
+	einfo "Running go generate..."


indent changes again

misberner · 2021-08-10T19:47:26Z

scripts/dev/quickgogen.sh

+
+[[ "${status}" -eq 0 ]] && exit 0
+efatal "An error occurred while generating files"
+exit 1


add a newline at the end of file

misberner · 2021-08-10T19:49:35Z

scripts/dev/quickgogen.sh

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


This probably won't work because the PATH setting is missing. You need to copy over the

export PATH="$PATH:${gitroot}/tools/generate-helpers"

from gogen.sh.

Good call; done.

viswajithiii · 2021-08-10T20:46:13Z

scripts/dev/quickgogen.sh

+fi
+
+# From https://stackoverflow.com/questions/28666357/git-how-to-get-default-branch#comment92366240_50056710
+main_branch="$(git remote show origin | grep "HEAD branch" | sed 's/.*: //')"


Can we reuse some of this stuff between quickgogen and quickstyle?

To be honest my bash-fu is quite weak and the first two times I tried this it came out squirrely.

I've moved main_branch and diffbase to git.sh, they return single strings and seem easy enough.

I don't know e.g. how to move something like the processing of changed_files there, because as soon as I echo the variable to return it (or print it out) I think the new lines are going to be converted to spaces, correct?

I welcome the opportunity to learn here. How can I capture multi-line string output from a function call in bash?

…t_generated_files unclear - multiline output

ebensh

I'd like to either review and finish this or let it die. Please let me know your preference @viswajithiii and @misberner - is it even worth cleaning this up?

misberner · 2022-02-04T14:22:43Z

I'd like to either review and finish this or let it die. Please let me know your preference @viswajithiii and @misberner - is it even worth cleaning this up?

I do think it's valuable. I'll try to make another pass today or on Monday.

scripts/dev/quickstyle.sh

viswajithiii · 2022-02-04T17:43:06Z

lib/git.sh

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


Add new line

viswajithiii · 2022-02-04T17:43:14Z

lib/git.sh

@@ -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?


I assume this works now?

Not sure what this referred to

misberner · 2022-02-08T00:33:09Z

lib/git.sh

+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/.*: //')"


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

misberner · 2022-02-08T00:33:35Z

lib/git.sh

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


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"

misberner · 2022-02-08T00:35:25Z

lib/git.sh

+# 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 {


Suggested change

function quick_get_generated_files {

function quick_get_generated_files() {

misberner · 2022-02-08T00:35:32Z

lib/git.sh

@@ -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?


Not sure what this referred to

misberner · 2022-02-08T00:37:03Z

lib/git.sh

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


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..

misberner · 2022-02-08T00:41:59Z

scripts/dev/quickgogen.sh

+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}/@")"


See above wrt awk instead of sed

misberner · 2022-02-08T00:44:12Z

scripts/dev/quickgogen.sh

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


Suggested change

sort | uniq)

sort -u)

slightly more concise, and just as POSIX-compliant

misberner · 2022-02-08T00:48:33Z

scripts/dev/quickgogen.sh

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


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

misberner · 2022-02-08T00:48:55Z

scripts/dev/quickgogen.sh

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


status is always 0 here

scripts/dev/quickstyle.sh

Co-authored-by: Viswajith Venugopal <[email protected]>

ebensh requested a review from misberner August 3, 2021 13:02

misberner reviewed Aug 3, 2021

View reviewed changes

ebensh changed the title ~~Add smart-gogen command to gogen touched directories/files~~ Add quickgogen command to gogen touched directories/files Aug 4, 2021

ebensh requested a review from misberner August 6, 2021 12:51

misberner suggested changes Aug 10, 2021

View reviewed changes

viswajithiii reviewed Aug 10, 2021

View reviewed changes

ebensh added 12 commits September 13, 2021 15:40

X-Smart-Branch-Parent: master

822d744

Add smart-gogen as copy of smart-diff

b6dd362

Change comments and behavior to initial smart-gogen script, need to test

4469e7c

Add symlink to bin directory

c5b8346

Update usage comment

5083bdb

Updated comment to reflect actual behavior

a5c15cc

Rename smart-gogen and bin symlink to quickgogen

40a0261

First attempt at quickgogen

5a8ba19

Change einfo to have ... prefix

fee046e

Add new line to end of file, add path export for generate helpers

bd6b9a8

Convert each double space to tab

4702597

Try to increase code reuse - main_branch and diffbase added, quick_ge…

3a15d1b

…t_generated_files unclear - multiline output

ebensh force-pushed the evan/add-smart-gogen branch from 8aadcce to 3a15d1b Compare September 13, 2021 14:41

ebensh requested review from viswajithiii and misberner September 13, 2021 14:41

Merge branch 'master' into evan/add-smart-gogen

d6dcf1d

ebensh commented Feb 3, 2022

View reviewed changes

viswajithiii reviewed Feb 4, 2022

View reviewed changes

misberner suggested changes Feb 8, 2022

View reviewed changes

Quote branch and diffbase names in variable substitution

affd5e3

Co-authored-by: Viswajith Venugopal <[email protected]>

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

	function quick_get_generated_files {
	function quick_get_generated_files() {

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

Add quickgogen command to gogen touched directories/files #75

Are you sure you want to change the base?

Add quickgogen command to gogen touched directories/files #75

Uh oh!

Conversation

ebensh commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

misberner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebensh left a comment

Choose a reason for hiding this comment

Uh oh!

misberner commented Feb 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ebensh commented Aug 3, 2021 •

edited

Loading