[Freeipa-devel] [PATCH 0069] Add 'review' target for make

[Petr Spacek]

On 11.12.2015 09:22, Lukas Slebodnik wrote:
> On (10/12/15 18:04), Petr Spacek wrote:
>> On 9.12.2015 15:30, Petr Spacek wrote:
>>> Hello,
>>>
>>> this patch automates some of sanity checks proposed by Petr Vobornik.
>>>
>>> 'make review' should be used in root of clean Git tree which has patches under
>>> review applied.
>>>
> +1 for automatisation
> -1 for name.
> 
> Your script does not review[1] anything. Code review(peer review) is a job
> for humans. What do you think about alternative
> names: "review-helper", "sanity-check" ...

I like short name but I respect your point, so I've updated the script to print:
"No problems detected using lint, pep8, ./make{api,aci}, and VERSION" at the end.
Now it should be clear what it did, so reviewer knows what steps were already
done and what can be skipped during further manual review.

Attached patch also adds 'assert-clean-tree' target which is run before lint
target, so no time is wasted if the tree is not clean. Parallel build will
execute and terminate both targets at the same time, but it saves time in both
cases, the output is just not so nice.

And IPA cannot be built in parallel anyway :-)

>>> Magic in review.sh attempts to detect nearest remote branch which can be used
>>> as diff base for review. Please see review.sh for further details.
>>
>> And here is the patch! :-)
>>
>> -- 
>> Petr^2 Spacek
> 
> [1] https://en.wikipedia.org/wiki/Code_review
> 
>>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>> From: Petr Spacek <pspacek at redhat.com>
>> Date: Tue, 8 Dec 2015 12:06:33 +0100
>> Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>>
>> - check if ACI.txt and API.txt are up-to-date
>> - check if VERSION was changed if API was changed
>> - pep8 --diff does not produce new errors when ran on diff from origin/branch
>> - make lint does not produce errors
>> ---
>> Makefile  |  4 ++++
>> review.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 68 insertions(+)
>> create mode 100755 review.sh
>>
>> diff --git a/Makefile b/Makefile
>> index d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>> 		(cd $$subdir && $(MAKE) check) || exit 1; \
>> 	done
>>
>> +# works only in Git tree
>> +review: version-update
>> +	./review.sh
>> +
>> bootstrap-autogen: version-update client-autogen
>> 	@echo "Building IPA $(IPA_VERSION)"
>> 	cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>> diff --git a/review.sh b/review.sh
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>> --- /dev/null
>> +++ b/review.sh
>> @@ -0,0 +1,64 @@
>> +#!/bin/bash
>> +is_tree_clean() {
>> +	test "$(git status --porcelain "$@")" == ""
>> +	return $?
>> +}
>> +
>> +log_error() {
>> +	echo "ERROR: ${1}, continuing ..."
>> +	errs=(${errs[@]} "ERROR: ${1}\n")
>> +}
>> +
>> +set -o errexit -o nounset #-o xtrace
>> +
>> +LOGFILE="$(mktemp --suff=.log)"
>> +exec > >(tee -i ${LOGFILE})
>> +exec 2>&1
>> +
>> +echo -n "make lint is running ... "
>> +make --silent lint || log_error "make lint failed"
>> +
>> +# Go backwards in history until you find a remote branch from which current
>> +# branch was created. This will be used as base for git diff.
>> +PATCHCNT=0
>> +BASEBRANCH=""
> If base branch means the remote branch which was used for cloning the cerrent
> git HEAD than you can simplify it a littl bit.
> git rev-parse --symbolic-full-name @{upstream}

Nice! I did not know this trick. Unfortunately it does not work for my
use-case: I clone upstream repo, pick a branch to base review on, create my
own branch from the upstream one, and then apply patches on top of that.

Imagine that I'm going to run the tool on branch 'review-tool'. Git history
tree looks like this:

* d036f3e (HEAD -> review-tool) fixup! Add 'review' target for make.
* c6d75b0 Makefile: disable parallel build
| * 667d50b (dkupka-review) dns: Add --auto-reverse option.
| * c85f176 dns: Check if domain already exists.
| * a55cd76 dns: do not add (forward)zone if it is already resolvable.
| * 0b0c529 (dns-no-forwarders) DNS: Make --no-forwarders option default.
|/
* d2e8470 Add 'review' target for make. It automates following tasks:
| * 101ffae (mbasti-review) Fix DNS tests: dns-resolve returns warning
| * 52393bf Add 'review' target for make. It automates following tasks:
|/
* 848912a (origin/master, master) add missing /ipaplatform/constants.py to
.gitignore

$ git rev-parse --symbolic-full-name @{upstream}
errors out like this:
fatal: no upstream configured for branch 'review-tool'

I definitely agree that the code above is just an ugly hack, but I would like
to keep current behavior so I do not need to change my workflow.

>> +CURRBRANCH="$(git branch --remote --contains)"
>> +while [ "${BASEBRANCH}" == "" ]
>> +do
>> +	BASEBRANCH="$(git branch --remote --contains "HEAD~${PATCHCNT}" | grep -v "^. ${CURRBRANCH}$" || :)"
>> +	PATCHCNT="$(expr "${PATCHCNT}" + 1)"
> Then for patch count you can use
> PATCHCNT=$(git log --oneline $BASEBRANCH..HEAD | wc -l)

-- 
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0069-3-Add-review-target-for-make.patch
Type: text/x-patch
Size: 781 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151211/19c2cefc/attachment.bin>