Felipe ContrerasHi,
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
Felipe Contreras[PATCH 1/5] remote: trivial style cleanup
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
Junio C HamanoRe: [PATCH 1/5] remote: trivial style cleanup
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]);Felipe Contreras[PATCH 2/5] sequencer: trivial fix
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
Felipe Contreras[PATCH 3/5] test: trivial cleanups
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(-)
t/t3403-rebase-skip.sh, t/t3505-cherry-pick-empty.sh
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
Felipe Contreras[PATCH 4/5] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 4/5] test: improve rebase -q test
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
}
Jonathan NiederRe: [PATCH 4/5] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 4/5] test: improve rebase -q test
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.
Felipe ContrerasRe: [PATCH 4/5] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 4/5] test: improve rebase -q test
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.
Felipe ContrerasRe: [PATCH 4/5] test: improve rebase -q test
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
Felipe Contreras[PATCH 5/5] test: rebase: fix --interactive test
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