# [PATCH v2] push: Correctly initialize nonfastforward in do_push. 4 messages from 2009-09-16 to 2009-09-16. Participants: Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/20974 ## Matthieu Moy, 2009-09-16 17:26 Subject: Re: [PATCH] push: Correctly initialize nonfastforward in do_push. Message-ID: URL: https://gitlist.dev/e/vpqfxamby33.fsf%40bauges.imag.fr In-Reply-To: <7vpr9q24oa.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Matthieu Moy 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/ ``` ## Matthieu Moy, 2009-09-16 17:28 Subject: [PATCH v2] push: Correctly initialize nonfastforward in do_push. Message-ID: <1253122107-31378-1-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1253122107-31378-1-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2009-09-16 18:15 Subject: Re: [PATCH] push: Correctly initialize nonfastforward in do_push. Message-ID: <7vvdjizrgg.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdjizrgg.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Matthieu Moy 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. ``` ## Matthieu Moy, 2009-09-16 18:29 Subject: Re: [PATCH] push: Correctly initialize nonfastforward in do_push. Message-ID: URL: https://gitlist.dev/e/vpqskemagla.fsf%40bauges.imag.fr In-Reply-To: <7vvdjizrgg.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Matthieu Moy 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/ ```