v2push: Correctly initialize nonfastforward in do_push.

Junio C Hamano <gitster@pobox.com> 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 ;-).

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.

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

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

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.

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

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

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.