[PATCH] push: Correctly initialize nonfastforward in do_push.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
(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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Matthieu Moy <[hidden email]> 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 '...'

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy-2
Junio C Hamano <[hidden email]> writes:

> Matthieu Moy <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy-2
Junio C Hamano <[hidden email]> writes:

> Matthieu Moy <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html