Re: [PATCH 1/5] New merge tests

[Jakub Narebski <jnareb@gmail.com>]

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

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

[...]
> +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