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

[Petr Spacek]

On 16.12.2015 12:24, Martin Kosek wrote:
> On 12/16/2015 12:01 PM, Petr Spacek wrote:
>> On 16.12.2015 11:15, Martin Kosek wrote:
>>> On 12/16/2015 10:02 AM, Petr Spacek wrote:
 On 16.12.2015 09:53, Jan Cholasta wrote:
> On 16.12.2015 09:45, Petr Spacek wrote:
>> On 11.12.2015 15:50, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 10.12.2015 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.
>
> 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! :-)
>>>
>>> Nice, but I would rather see this in ipatool
>>> (). Or is there any obvious 
>>> benefit
>>> in having this in freeipa itself that I'm missing?
>>
>> For me the obvious benefit is:
>> git clone
>> git am
>> make review
>>
>> Done.
>>
>> No need to find & learn other tool, no risk of using wrong version of 
>> the tool
>> to wrong version of source tree etc.
>
> AFAIK all IPA developers are supposed to use ipatool, and it already has a

 Good to know. How does a newcomer learn about it? Honestly I never used
 ipatool (or not even cloned it).
>>>
>>> ipatool targets rather upstream members with write access, so they are 
>>> hardly
>>> newcomers.
>>
>> I though that we want to make it easy to contribute, so why are you talking
>> about core developers?
> 
> I was mostly refering to ipatool's "push" command that I mostly used, but it's
> true there is also "start-review" or "am" commands that could be useful to
> others too.
> 
>> Shouldn't we make it easy to self-review own patches for everyone? Including
>> random people who want to submit few patches and go away? (Think how we can
>> apply usability principles to development.)
> 
> We should, I hope my reply did not suggest otherwise.

Hmm, so I finally tried ipatool and it blew up:

$ ../tool/ipatool --help
Configuration is specified in the file given by --config.
Run ipatool sample-config to see what needs to go in it.

$ ../tool/ipatool sample-config
Traceback (most recent call last):
  File "../tool/ipatool", line 701, in 
Context(docopt.docopt(__doc__)).run()
  File "../tool/ipatool", line 233, in __init__
with open(os.path.expanduser(options['--config'])) as conf_file:
FileNotFoundError: [Errno 2] No such file or directory:
'/home/pspacek/.ipa/toolconf.yaml'

... after a while I found out that it is too heavy Python magic inside (for me
as non-Pythonist) so I'm giving up.

Moreover it AFAIK does not work on tree with applied patches (which is normal
situation before you send the patched to list for review), so I'm not even
sure how I could integrate the review scripts into it.

For now I'm going to live with my review.sh scripts in following Git repo:
https://github.com/pspacek/ipareview.git

Pull requests or rewrites for ipatool are welcome.
You can take this as a Christmas present :-)

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Petr Spacek]

On 16.12.2015 11:15, Martin Kosek wrote:
> On 12/16/2015 10:02 AM, Petr Spacek wrote:
>> On 16.12.2015 09:53, Jan Cholasta wrote:
>>> On 16.12.2015 09:45, Petr Spacek wrote:
 On 11.12.2015 15:50, Jan Cholasta wrote:
> Hi,
>
> On 10.12.2015 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.
>>>
>>> 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! :-)
>
> Nice, but I would rather see this in ipatool
> (). Or is there any obvious 
> benefit
> in having this in freeipa itself that I'm missing?

 For me the obvious benefit is:
 git clone
 git am
 make review

 Done.

 No need to find & learn other tool, no risk of using wrong version of the 
 tool
 to wrong version of source tree etc.
>>>
>>> AFAIK all IPA developers are supposed to use ipatool, and it already has a
>>
>> Good to know. How does a newcomer learn about it? Honestly I never used
>> ipatool (or not even cloned it).
> 
> ipatool targets rather upstream members with write access, so they are hardly
> newcomers.

I though that we want to make it easy to contribute, so why are you talking
about core developers?

Shouldn't we make it easy to self-review own patches for everyone? Including
random people who want to submit few patches and go away? (Think how we can
apply usability principles to development.)

Petr^2 Spacek

> But still, here you go:
> https://www.freeipa.org/page/Contribute/Repository#Process_tools
> 
>>> start-review command, so it would better fit in there. Or we could merge
>>> freeipa-tools into freeipa. My point is that I don't think having half of 
>>> the
>>> stuff in ipatool and the other half in IPA itself is a good thing to do.
>>
>> I agree with this in general.
>>
>> Would it make sense to at least have review target for make which executes
>> ipa-tool and if it is not installed it tells you where to grab it?
>>
>> Or possibly make ipatool submodule of ipa git tree, so there is no risk of
>> using wrong review tool for particular checkout?
> 
> Please do not overcomplicate it :-) ipatool works nicely at the moment, it is
> in a separate repo with other tools where every core developer can contribute
> and is easy to be update.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Martin Kosek]

On 12/16/2015 12:01 PM, Petr Spacek wrote:
> On 16.12.2015 11:15, Martin Kosek wrote:
>> On 12/16/2015 10:02 AM, Petr Spacek wrote:
>>> On 16.12.2015 09:53, Jan Cholasta wrote:
 On 16.12.2015 09:45, Petr Spacek wrote:
> On 11.12.2015 15:50, Jan Cholasta wrote:
>> Hi,
>>
>> On 10.12.2015 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.

 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! :-)
>>
>> Nice, but I would rather see this in ipatool
>> (). Or is there any obvious 
>> benefit
>> in having this in freeipa itself that I'm missing?
>
> For me the obvious benefit is:
> git clone
> git am
> make review
>
> Done.
>
> No need to find & learn other tool, no risk of using wrong version of the 
> tool
> to wrong version of source tree etc.

 AFAIK all IPA developers are supposed to use ipatool, and it already has a
>>>
>>> Good to know. How does a newcomer learn about it? Honestly I never used
>>> ipatool (or not even cloned it).
>>
>> ipatool targets rather upstream members with write access, so they are hardly
>> newcomers.
> 
> I though that we want to make it easy to contribute, so why are you talking
> about core developers?

I was mostly refering to ipatool's "push" command that I mostly used, but it's
true there is also "start-review" or "am" commands that could be useful to
others too.

> Shouldn't we make it easy to self-review own patches for everyone? Including
> random people who want to submit few patches and go away? (Think how we can
> apply usability principles to development.)

We should, I hope my reply did not suggest otherwise.

Martin

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Petr Spacek]

On 11.12.2015 15:50, Jan Cholasta wrote:
> Hi,
> 
> On 10.12.2015 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.
>>>
>>> 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! :-)
> 
> Nice, but I would rather see this in ipatool
> (). Or is there any obvious benefit
> in having this in freeipa itself that I'm missing?

For me the obvious benefit is:
git clone
git am
make review

Done.

No need to find & learn other tool, no risk of using wrong version of the tool
to wrong version of source tree etc.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

On 16.12.2015 09:45, Petr Spacek wrote:

On 11.12.2015 15:50, Jan Cholasta wrote:

Hi,

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

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! :-)


Nice, but I would rather see this in ipatool
(). Or is there any obvious benefit
in having this in freeipa itself that I'm missing?


For me the obvious benefit is:
git clone
git am
make review

Done.

No need to find & learn other tool, no risk of using wrong version of the tool
to wrong version of source tree etc.


AFAIK all IPA developers are supposed to use ipatool, and it already has 
a start-review command, so it would better fit in there. Or we could 
merge freeipa-tools into freeipa. My point is that I don't think having 
half of the stuff in ipatool and the other half in IPA itself is a good 
thing to do.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Petr Spacek]

On 16.12.2015 09:53, Jan Cholasta wrote:
> On 16.12.2015 09:45, Petr Spacek wrote:
>> On 11.12.2015 15:50, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 10.12.2015 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.
>
> 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! :-)
>>>
>>> Nice, but I would rather see this in ipatool
>>> (). Or is there any obvious 
>>> benefit
>>> in having this in freeipa itself that I'm missing?
>>
>> For me the obvious benefit is:
>> git clone
>> git am
>> make review
>>
>> Done.
>>
>> No need to find & learn other tool, no risk of using wrong version of the 
>> tool
>> to wrong version of source tree etc.
> 
> AFAIK all IPA developers are supposed to use ipatool, and it already has a

Good to know. How does a newcomer learn about it? Honestly I never used
ipatool (or not even cloned it).


> start-review command, so it would better fit in there. Or we could merge
> freeipa-tools into freeipa. My point is that I don't think having half of the
> stuff in ipatool and the other half in IPA itself is a good thing to do.

I agree with this in general.

Would it make sense to at least have review target for make which executes
ipa-tool and if it is not installed it tells you where to grab it?

Or possibly make ipatool submodule of ipa git tree, so there is no risk of
using wrong review tool for particular checkout?

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Martin Kosek]

On 12/16/2015 10:02 AM, Petr Spacek wrote:
> On 16.12.2015 09:53, Jan Cholasta wrote:
>> On 16.12.2015 09:45, Petr Spacek wrote:
>>> On 11.12.2015 15:50, Jan Cholasta wrote:
 Hi,

 On 10.12.2015 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.
>>
>> 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! :-)

 Nice, but I would rather see this in ipatool
 (). Or is there any obvious 
 benefit
 in having this in freeipa itself that I'm missing?
>>>
>>> For me the obvious benefit is:
>>> git clone
>>> git am
>>> make review
>>>
>>> Done.
>>>
>>> No need to find & learn other tool, no risk of using wrong version of the 
>>> tool
>>> to wrong version of source tree etc.
>>
>> AFAIK all IPA developers are supposed to use ipatool, and it already has a
> 
> Good to know. How does a newcomer learn about it? Honestly I never used
> ipatool (or not even cloned it).

ipatool targets rather upstream members with write access, so they are hardly
newcomers.

But still, here you go:
https://www.freeipa.org/page/Contribute/Repository#Process_tools

>> start-review command, so it would better fit in there. Or we could merge
>> freeipa-tools into freeipa. My point is that I don't think having half of the
>> stuff in ipatool and the other half in IPA itself is a good thing to do.
> 
> I agree with this in general.
> 
> Would it make sense to at least have review target for make which executes
> ipa-tool and if it is not installed it tells you where to grab it?
> 
> Or possibly make ipatool submodule of ipa git tree, so there is no risk of
> using wrong review tool for particular checkout?

Please do not overcomplicate it :-) ipatool works nicely at the moment, it is
in a separate repo with other tools where every core developer can contribute
and is easy to be update.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Lukas Slebodnik]

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

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

>+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)

LS

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

Hi,

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

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! :-)


Nice, but I would rather see this in ipatool 
(). Or is there any obvious 
benefit in having this in freeipa itself that I'm missing?


Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Lukas Slebodnik]

On (11/12/15 09:46), Petr Spacek wrote:
>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.
>
"review-helper", "sanity-check" are not long.

long would be
"aotomated-part-of-sanity-checks-from-FreeIPA-developers's-check-list"

And pdf document
https://pvoborni.fedorapeople.org/FreeIPAdeveloperschecklist.pdf
already mentioned "Sanity checks"

Please consider to not use confusing name.
Neither as make target nor as name of script.

BTW I cannot see sanity check for spec file.
If it will be added to FreeIPAdeveloperschecklist.pdf
than you might add it to the sript as well.

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

Re: [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 
>> 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 
>> ..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 

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

[Petr Spacek]

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.
> 
> 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
From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
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 ..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=""
+CURRBRANCH="$(git branch --remote --contains)"
+while [ "${BASEBRANCH}" == "" ]
+do
+	BASEBRANCH="$(git branch --remote --contains "HEAD~${PATCHCNT}" | grep -v "^. ${CURRBRANCH}$" || :)"
+	PATCHCNT="$(expr "${PATCHCNT}" + 1)"
+done
+echo "Detected base branch: ${BASEBRANCH}"
+echo -n "Checks will be made against following base commit: "
+git log -1 --oneline ${BASEBRANCH}
+
+# gather all errors in one array and print error summary at the end
+declare -a errs
+errs=("Summary of detected errors:\n")
+
+is_tree_clean || log_error "Git tree must be clean before you start review"
+
+./makeapi
+is_tree_clean "API.txt" || log_error "./makeapi changed something"
+
+./makeaci
+is_tree_clean "ACI.txt" || log_error "./makeaci changed something"
+
+git diff ${BASEBRANCH} -U0 | pep8 --diff || log_error "PEP8 --diff failed"
+
+# if API.txt is changed require change in VERSION
+if ! git diff ${BASEBRANCH} --quiet -- API.txt;
+then
+	git diff ${BASEBRANCH} --quiet -- VERSION && log_error "API.txt was changed without a change in VERSION"
+fi
+
+# print error summary
+if [ "${#errs[*]}" != "1" ]
+then
+	echo -e "${errs[*]}"
+	echo "Please see ${LOGFILE}"
+	exit 1
+else
+	rm "${LOGFILE}"
+	echo "review.sh did not detect any problem"
+fi
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code