From: Tay Ray Chuan <rctay89@gmail.com>
Date: Sat, 27 Feb 2010 04:19:20 GMT
Subject: Re: [PATCH 3/4] git-push: make git push --porcelain print "Done"
Message-ID: <be6fef0d1002262019j2088d20dp89f1587e3921a1ba@mail.gmail.com>
In-Reply-To: <04fc63d0f8114f0cd721bcf3dff1f108e30b5308.1267243044.git.larry@elder-gods.org>

Hi,

On Sat, Feb 27, 2010 at 11:59 AM, Larry D'Anna <larry@elder-gods.org> wrote:
> diff --git a/transport.c b/transport.c
> index 32885f7..5b880d7 100644
> --- a/transport.c
> +++ b/transport.c
> [snip]

This hunk:

> @@ -1055,8 +1056,6 @@ int transport_push(struct transport *transport,
>                ret = transport->push_refs(transport, remote_refs, flags);
>                err = push_had_errors(remote_refs);
>
> -               ret |= err;
> -
>                if (!quiet || err)
>                        print_push_status(transport->url, remote_refs,
>                                        verbose | porcelain, porcelain,

and this hunk:

> @@ -1071,9 +1070,15 @@ int transport_push(struct transport *transport,
>                                update_tracking_ref(transport->remote, ref, verbose);
>                }
>
> -               if (!quiet && !ret && !refs_pushed(remote_refs))
> -                       fprintf(stderr, "Everything up-to-date\n");
> -               return ret;
> +
> +               if (porcelain) {
> +                       if (ret==0)
> +                               fprintf (stdout, "Done\n");
> +               } else
> +                       if (!quiet && !ret && !refs_pushed(remote_refs))
> +                               fprintf(stderr, "Everything up-to-date\n");
> +
> +               return ret | err;
>        }
>        return 1;
>  }

slightly changes the conditions under which "Everything up-to-date" is
printed, since you did away with "ret |= err". I guess you can
mitigate this by adding "!err" to the list of operands:

  if (!quiet && !ret && !err && !refs_pushed(remote_refs))
      ....

Even better, create yet another temporary variable:

  push_ret = transport->push_refs(transport, remote_refs, flags);
  err = push_had_errors(remote_refs);

  ret = push_ret | err;

That way, you can access transport->push_refs() status (what you
want), without messing with the other parts of the code and their
assumptions.

One last thing: could the if statements towards the end be "flattened"?

  if (porcelain && ret == 0)
      fprintf(stdout, "Done\n");
  else if (!quiet && !ret && !refs_pushed(remote_refs))
      fprintf(stderr, "Everything up-to-date\n");

(sorry about the lack of tabs, gmail doesn't let me type those easily.)

-- 
Cheers,
Ray Chuan