[PATCH 0/5] Trivial patches

[PATCH 0/5] Trivial patches

[Felipe Contreras]

Hi,

Here's a bunch of trivial patches.

Felipe Contreras (5):
  remote: trivial style cleanup
  sequencer: trivial fix
  test: trivial cleanups
  test: improve rebase -q test
  test: rebase: fix --interactive test

 remote.c                      |  3 +--
 sequencer.c                   |  7 +++++--
 t/t3400-rebase.sh             |  1 +
 t/t3403-rebase-skip.sh        |  7 ++++---
 t/t3404-rebase-interactive.sh |  2 +-
 t/t3505-cherry-pick-empty.sh  | 18 +++++-------------
 6 files changed, 17 insertions(+), 21 deletions(-)

-- 
1.8.3.rc3.312.g47657de

[PATCH 1/5] remote: trivial style cleanup

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 remote.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 68eb99b..e71f66d 100644
--- a/remote.c
+++ b/remote.c
@@ -1474,8 +1474,7 @@ struct branch *branch_get(const char *name)
 		ret->remote = remote_get(ret->remote_name);
 		if (ret->merge_nr) {
 			int i;
-			ret->merge = xcalloc(sizeof(*ret->merge),
-					     ret->merge_nr);
+			ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
 			for (i = 0; i < ret->merge_nr; i++) {
 				ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
 				ret->merge[i]->src = xstrdup(ret->merge_name[i]);
-- 
1.8.3.rc3.312.g47657de

[PATCH 2/5] sequencer: trivial fix

[Felipe Contreras]

We should free objects before leaving.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sequencer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..7eeae2f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		rerere(opts->allow_rerere_auto);
 	} else {
 		int allow = allow_empty(opts, commit);
-		if (allow < 0)
-			return allow;
+		if (allow < 0) {
+			res = allow;
+			goto leave;
+		}
 		if (!opts->no_commit)
 			res = run_git_commit(defmsg, opts, allow);
 	}
 
+leave:
 	free_message(&msg);
 	free(defmsg);
 
-- 
1.8.3.rc3.312.g47657de

[PATCH 3/5] test: trivial cleanups

[Felipe Contreras]

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3403-rebase-skip.sh       |  7 ++++---
 t/t3505-cherry-pick-empty.sh | 18 +++++-------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 826500b..3968020 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -64,10 +64,11 @@ test_expect_success 'rebase with --merge' '
 
 test_expect_success 'rebase --skip with --merge' '
 	git rebase --skip
-	'
+'
 
-test_expect_success 'merge and reference trees equal' \
-	'test -z "`git diff-tree skip-merge skip-reference`"'
+test_expect_success 'merge and reference trees equal' '
+	test -z "`git diff-tree skip-merge skip-reference`"
+'
 
 test_expect_success 'moved back to branch correctly' '
 	test refs/heads/skip-merge = $(git symbolic-ref HEAD)
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index a0c6e30..fbdc47c 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -28,29 +28,21 @@ test_expect_success setup '
 '
 
 test_expect_success 'cherry-pick an empty commit' '
-	git checkout master && {
-		git cherry-pick empty-branch^
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch^
 '
 
 test_expect_success 'index lockfile was removed' '
-
 	test ! -f .git/index.lock
-
 '
 
 test_expect_success 'cherry-pick a commit with an empty message' '
-	git checkout master && {
-		git cherry-pick empty-branch
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch
 '
 
 test_expect_success 'index lockfile was removed' '
-
 	test ! -f .git/index.lock
-
 '
 
 test_expect_success 'cherry-pick a commit with an empty message with --allow-empty-message' '
@@ -101,7 +93,7 @@ test_expect_success 'cherry-pick a no-op with --keep-redundant' '
 	git reset --hard &&
 	git checkout fork^0 &&
 	git cherry-pick --keep-redundant-commits master &&
-	git show -s --format='%s' >actual &&
+	git show -s --format=%s >actual &&
 	echo "add file2 on master" >expect &&
 	test_cmp expect actual
 '
-- 
1.8.3.rc3.312.g47657de

[PATCH 4/5] test: improve rebase -q test

[Felipe Contreras]

Let's show the output so it's clear why it failed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3400-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..fb39531 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
 test_expect_success 'rebase -q is quiet' '
 	git checkout -b quiet topic &&
 	git rebase -q master >output.out 2>&1 &&
+	cat output.out &&
 	test ! -s output.out
 '
 
-- 
1.8.3.rc3.312.g47657de

[PATCH 5/5] test: rebase: fix --interactive test

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a58406d..79e8d3c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -692,7 +692,7 @@ test_expect_success 'rebase -i can copy notes' '
 	test_commit n2 &&
 	test_commit n3 &&
 	git notes add -m"a note" n3 &&
-	git rebase --onto n1 n2 &&
+	git rebase -i --onto n1 n2 &&
 	test "a note" = "$(git notes show HEAD)"
 '
 
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 1/5] remote: trivial style cleanup

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  remote.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 68eb99b..e71f66d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1474,8 +1474,7 @@ struct branch *branch_get(const char *name)
>  		ret->remote = remote_get(ret->remote_name);
>  		if (ret->merge_nr) {
>  			int i;
> -			ret->merge = xcalloc(sizeof(*ret->merge),
> -					     ret->merge_nr);
> +			ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));

Yeah, calloc is nmemb first and then size, so this makes sense.

>  			for (i = 0; i < ret->merge_nr; i++) {
>  				ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
>  				ret->merge[i]->src = xstrdup(ret->merge_name[i]);

Re: [PATCH 4/5] test: improve rebase -q test

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Let's show the output so it's clear why it failed.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t3400-rebase.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index b58fa1a..fb39531 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
>  test_expect_success 'rebase -q is quiet' '
>  	git checkout -b quiet topic &&
>  	git rebase -q master >output.out 2>&1 &&
> +	cat output.out &&
>  	test ! -s output.out
>  '

It is one thing to avoid squelching output that naturally comes out
of command being tested unnecessarily, so that "./txxxx-*.sh -v"
output can be used for debugging.  I however am not sure if adding
"cat" to random places like this is a productive direction for us to
go in.

A more preferrable alternative may be adding something like this to
test-lib.sh and call it from here and elsewhere (there are about 50
places that do "test ! -s <filename>"), perhaps?

        test_must_be_an_empty_file () {
                if test -s "$1"
                then
                        cat "$1"
                        false
                fi
        }

Re: [PATCH 4/5] test: improve rebase -q test

[Jonathan Nieder]

Junio C Hamano wrote:

> A more preferrable alternative may be adding something like this to
> test-lib.sh and call it from here and elsewhere (there are about 50
> places that do "test ! -s <filename>"), perhaps?
>
>         test_must_be_an_empty_file () {
>                 if test -s "$1"
>                 then
>                         cat "$1"
>                         false
>                 fi
>         }

I generally just use the two-liner

	>empty &&
	test_cmp empty output

directly in cases like this.

Thanks,
Jonathan

Re: [PATCH 4/5] test: improve rebase -q test

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> A more preferrable alternative may be adding something like this to
>> test-lib.sh and call it from here and elsewhere (there are about 50
>> places that do "test ! -s <filename>"), perhaps?
>>
>>         test_must_be_an_empty_file () {
>>                 if test -s "$1"
>>                 then
>>                         cat "$1"
>>                         false
>>                 fi
>>         }
>
> I generally just use the two-liner
>
> 	>empty &&
> 	test_cmp empty output
>
> directly in cases like this.

That would work, too.

Re: [PATCH 4/5] test: improve rebase -q test

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Let's show the output so it's clear why it failed.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  t/t3400-rebase.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> > index b58fa1a..fb39531 100755
> > --- a/t/t3400-rebase.sh
> > +++ b/t/t3400-rebase.sh
> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
> >  test_expect_success 'rebase -q is quiet' '
> >  	git checkout -b quiet topic &&
> >  	git rebase -q master >output.out 2>&1 &&
> > +	cat output.out &&
> >  	test ! -s output.out
> >  '
> 
> It is one thing to avoid squelching output that naturally comes out
> of command being tested unnecessarily, so that "./txxxx-*.sh -v"
> output can be used for debugging.  I however am not sure if adding
> "cat" to random places like this is a productive direction for us to
> go in.
> 
> A more preferrable alternative may be adding something like this to
> test-lib.sh and call it from here and elsewhere (there are about 50
> places that do "test ! -s <filename>"), perhaps?
> 
>         test_must_be_an_empty_file () {
>                 if test -s "$1"
>                 then
>                         cat "$1"
>                         false
>                 fi
>         }

Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
for hypothetical "ideal" situations that we'll never reach.

-- 
Felipe Contreras

Re: [PATCH 4/5] test: improve rebase -q test

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > Let's show the output so it's clear why it failed.
>> >
>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> > ---
>> >  t/t3400-rebase.sh | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> > index b58fa1a..fb39531 100755
>> > --- a/t/t3400-rebase.sh
>> > +++ b/t/t3400-rebase.sh
>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
>> >  test_expect_success 'rebase -q is quiet' '
>> >  	git checkout -b quiet topic &&
>> >  	git rebase -q master >output.out 2>&1 &&
>> > +	cat output.out &&
>> >  	test ! -s output.out
>> >  '
>> 
>> It is one thing to avoid squelching output that naturally comes out
>> of command being tested unnecessarily, so that "./txxxx-*.sh -v"
>> output can be used for debugging.  I however am not sure if adding
>> "cat" to random places like this is a productive direction for us to
>> go in.
>> 
>> A more preferrable alternative may be adding something like this to
>> test-lib.sh and call it from here and elsewhere (there are about 50
>> places that do "test ! -s <filename>"), perhaps?
>> 
>>         test_must_be_an_empty_file () {
>>                 if test -s "$1"
>>                 then
>>                         cat "$1"
>>                         false
>>                 fi
>>         }
>
> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
> for hypothetical "ideal" situations that we'll never reach.

That's too bad.  Addition of "cat" where there does not need one is
clearly not an obvious fix anyway.

Re: [PATCH 4/5] test: improve rebase -q test

[Felipe Contreras]

On Wed, May 29, 2013 at 11:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>> > Let's show the output so it's clear why it failed.
>>> >
>>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> > ---
>>> >  t/t3400-rebase.sh | 1 +
>>> >  1 file changed, 1 insertion(+)
>>> >
>>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>>> > index b58fa1a..fb39531 100755
>>> > --- a/t/t3400-rebase.sh
>>> > +++ b/t/t3400-rebase.sh
>>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
>>> >  test_expect_success 'rebase -q is quiet' '
>>> >    git checkout -b quiet topic &&
>>> >    git rebase -q master >output.out 2>&1 &&
>>> > +  cat output.out &&
>>> >    test ! -s output.out
>>> >  '
>>>
>>> It is one thing to avoid squelching output that naturally comes out
>>> of command being tested unnecessarily, so that "./txxxx-*.sh -v"
>>> output can be used for debugging.  I however am not sure if adding
>>> "cat" to random places like this is a productive direction for us to
>>> go in.
>>>
>>> A more preferrable alternative may be adding something like this to
>>> test-lib.sh and call it from here and elsewhere (there are about 50
>>> places that do "test ! -s <filename>"), perhaps?
>>>
>>>         test_must_be_an_empty_file () {
>>>                 if test -s "$1"
>>>                 then
>>>                         cat "$1"
>>>                         false
>>>                 fi
>>>         }
>>
>> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected
>> for hypothetical "ideal" situations that we'll never reach.
>
> That's too bad.  Addition of "cat" where there does not need one is
> clearly not an obvious fix anyway.

If you are an actual real user of this code; a developer that is
running the test; and the test finally achieves it's designed goal of
detecting a failure, you would be left scratching your head wondering
what's the problem if running './test -v' doesn't show anything, even
after you have added debugging code to narrow down the issue.

I had to add that cat line not once, but more than two times in
different lines of development.

So yeah, a cat is needed, and the fact you don't see that amazes me,
specially after you have reprimanded me for using 'grep -q' instead of
'grep' for this very reason.

-- 
Felipe Contreras