Re: [PATCH 1/5] New merge tests

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 25, 2008, 09:37 UTC
Message-ID: <m3tzhqp96h.fsf@localhost.localdomain>
In-Reply-To: <402c10cd0804232243u700f4f6fv130d69283c40ff1d@mail.gmail.com>

"Sverre Hvammen Johansen" <hvammen@gmail.com> writes:

Show 5 quoted lines

> Introduce new merge tests for preparation of new features:
> 
>   --ff=<fast forward option>
>   Head reduction
>   --ff=only

I think you should describe here _what_ is tested by the new test(s), and how it is named.

BTW. the test itself is a bit short on comments...

> +create_merge_msgs() {

[...]

> +verify_diff() {

[...]

> +verify_merge() {

[...]

> +verify_head() {

[...]

> +verify_parents() {

It would be nice to have 1 or 2 lines description of those functions, perhaps with calling convention. See for example comments in t/test-lib.sh (some of which are in t/README instead ;-).

> +verify_merge() {
> +       verify_diff "$2" "$1" "[OOPS] bad merge result" &&
> +       if test $(git ls-files -u | wc -l) -gt 0

What are conventions used by other tests? Somehow I doublt is is "[OOPS]"...

Instead of
       if test $(git ls-files -u | wc -l) -gt 0
you should write IMHO
       if test -n "$(git ls-files -u)"
or just
       if test "$(git ls-files -u)"

[...]

Show 6 quoted lines

> +test_expect_success 'setup' '
> +       git add file &&
> +       test_tick &&
> +       git commit -m "commit 0" &&
> +       git tag c0 &&
> +       c0=$(git rev-parse HEAD) &&

[...]

> +'

It would be nice if you have provides, as comment to this step, ASCII-art graph of commits you want to have created.

BTW. instead of
       c0=$(git rev-parse HEAD) &&
you can use
       c0=$(git rev-parse c0^{}) &&

or even "c0^{commit}".

[...]

> +test_expect_success 'merge c1 with c0 and c0' '
> +       git reset --hard c1 &&
> +       git config branch.master.mergeoptions "" &&

Not "git config --unset branch.master.mergeoptions"?

-- 
Jakub Narebski
Poland
ShadeHawk on #git