[PATCH] push: Correctly initialize nonfastforward in do_push.

[PATCH] push: Correctly initialize nonfastforward in do_push.

[Matthieu Moy]

The variable is assigned unconditionally in print_push_status, but
print_push_status is not reached by all codepaths. In particular, this
fixes a bug where "git push ... nonexisting-branch" was complaining about
non-fast forward.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
(I'm the one to blame, sorry for introducing this bug)

 builtin-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 3cb1ee4..a73333b 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -140,7 +140,7 @@ static int do_push(const char *repo, int flags)
 		struct transport *transport =
 			transport_get(remote, url[i]);
 		int err;
-		int nonfastforward;
+		int nonfastforward = 0;
 		if (receivepack)
 			transport_set_option(transport,
 					     TRANS_OPT_RECEIVEPACK, receivepack);
-- 
1.6.5.rc1.11.g2d184.dirty

Re: [PATCH] push: Correctly initialize nonfastforward in do_push.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> The variable is assigned unconditionally in print_push_status, but
> print_push_status is not reached by all codepaths. In particular, this
> fixes a bug where "git push ... nonexisting-branch" was complaining about
> non-fast forward.

Hmm, the patch looks correct but I am scratching my head to see how this
is triggered.  "git push ... nonexisting-branch" seems to get:

	error: src refspec nonexisting-branch does not match any.
        error: failed to push some refs to '...'

Re: [PATCH] push: Correctly initialize nonfastforward in do_push.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> The variable is assigned unconditionally in print_push_status, but
>> print_push_status is not reached by all codepaths. In particular, this
>> fixes a bug where "git push ... nonexisting-branch" was complaining about
>> non-fast forward.
>
> Hmm, the patch looks correct but I am scratching my head to see how this
> is triggered.  "git push ... nonexisting-branch" seems to get:

Short answer: trust me, without the patch, you get the non-fast
forward (and valgrind tells you about conditional jump on
uninitialized value), with, you don't ;-).

Longer one:

int transport_push(struct transport *transport,
		   int refspec_nr, const char **refspec, int flags,
		   int * nonfastforward)
{
[...]
		if (match_refs(local_refs, &remote_refs,
			       refspec_nr, refspec, match_flags)) {
			return -1; /* <------------------------------ you stop here */
		}
[...]
		if (!quiet || push_had_errors(remote_refs))
			print_push_status(transport->url, remote_refs,
					verbose | porcelain, porcelain,
					nonfastforward); /* <----- you would have updated nonfastforward there */
[...]
}

Actually, my initial version probably had the condition of the second
if too. And with the first "return" statement in transport_push.
Writting this, I'm wondering if it wouldn't be a better coding style
to initialize nonfastforward to 0 within transport_push (in case other
callers to transport_push appear one day, they won't get the the same
bug). Second version of the patch is comming.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2] push: Correctly initialize nonfastforward in do_push.

[Matthieu Moy]

The variable is assigned unconditionally in print_push_status, but
print_push_status is not reached by all codepaths. In particular, this
fixes a bug where "git push ... nonexisting-branch" was complaining about
non-fast forward.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
New version initializing nonfastforward inside transport()

 transport.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/transport.c b/transport.c
index 4cb8077..18db3d3 100644
--- a/transport.c
+++ b/transport.c
@@ -871,6 +871,7 @@ int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
 		   int * nonfastforward)
 {
+	*nonfastforward = 0;
 	verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push)
-- 
1.6.5.rc1.11.g2d184.dirty

Re: [PATCH] push: Correctly initialize nonfastforward in do_push.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Short answer: trust me, without the patch, you get the non-fast
> forward (and valgrind tells you about conditional jump on
> uninitialized value), with, you don't ;-).

I understand valgrind one; I can trace the codepath with eyeballs without
it, and that is why I said it looks correct to begin with.

My puzzlement was that the following in the log message did not seem to
reproduce for me:

    ... where "git push ... nonexisting-branch" was complaining about
    non-fast forward.

I would be eventually writing an entry in the Release Notes about this
fix, and I do not want to say:

    "git push $there no-such-ref" incorrectly said no-such-ref does not
    fast forward; fixed.

when I know that command line would produce something entirely different
error.

Re: [PATCH] push: Correctly initialize nonfastforward in do_push.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Short answer: trust me, without the patch, you get the non-fast
>> forward (and valgrind tells you about conditional jump on
>> uninitialized value), with, you don't ;-).
>
> I understand valgrind one; I can trace the codepath with eyeballs without
> it, and that is why I said it looks correct to begin with.

(in case it wasn't clear, "you" in my message should have been read as
"one", no harm intended)

> My puzzlement was that the following in the log message did not seem to
> reproduce for me:
>     ... where "git push ... nonexisting-branch" was complaining about
>     non-fast forward.

It's comming from an uninitialized variable, so it may have worked
just "by chance". For me, it seems to do it reproducibly, with for
example:

$ mkdir repo
$ rm -rf repo/
$ git init repo
Initialized empty Git repository in /tmp/repo/.git/
$ cd repo; touch foo; git add .; git commit -m foo > /dev/null
$ git push . nonexisting-branch
error: src refspec nonexisting-branch does not match any.
error: failed to push some refs to '.'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes before pushing again.  See the 'non-fast forward'
section of 'git push --help' for details.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/