Hi,
Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)
Still with me? Good. Nice to meet you.
Just for the record: responding to a patch is my strongest way of saying
that I appreciate your work.
On Thu, 5 Mar 2009, Finn Arne Gangstad wrote:
> Previously, git push [remote] with no arguments would behave like
> "git push <remote> :" if no push refspecs were configured for the remote.
> It may be too easy for novice users to write "git push" or
> "git push origin" by accident, so git will now push nothing, and give an
> error message in such cases.
>
> Teach git push a new option "--matching" that keeps the old behavior of
> pushing all matching branches when none are configured.
As others have commented, you cannot just go and fsck existing users over.
That is just not flying well.
IMHO you should always consider the downsides of your patch in addition to
the upsides, and not only for yourself, but also for others.
> @@ -63,10 +63,11 @@ the remote repository.
> The special refspec `:` (or `{plus}:` to allow non-fast forward updates)
> directs git to push "matching" branches: for every branch that exists on
> the local side, the remote side is updated if a branch of the same name
> -already exists on the remote side. This is the default operation mode
> +already exists on the remote side. Nothing will be pushed
The two spaces after the full stop were not actually a typo.
> if no explicit refspec is found (that is neither on the command line
> nor in any Push line of the corresponding remotes file---see below).
>
> +
> --all::
Please do not change the style of the surrounding text. We do not have
double empty lines there.
> diff --git a/builtin-push.c b/builtin-push.c
> index 122fdcf..ffc648d 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -48,6 +48,12 @@ static void set_refspecs(const char **refs, int nr)
> }
> }
>
> +
> +static int has_multiple_bits(unsigned int x)
> +{
> + return (x & (x - 1)) != 0;
> +}
> +
> static int do_push(const char *repo, int flags)
To spare you searching: HAS_MULTI_BITS(x) (it is defined in
git-compat-util.h).
And by removing your function, you also remove another double empty line.
> @@ -71,17 +77,24 @@ static int do_push(const char *repo, int flags)
> return error("--mirror can't be combined with refspecs");
> }
>
> - if ((flags & (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) ==
> - (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) {
> - return error("--all and --mirror are incompatible");
> + if (has_multiple_bits(flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR | TRANSPORT_PUSH_MATCHING))) {
> + return error("--all, --mirror and --matching are incompatible");
These are awfully long lines. Not so good.
> }
>
> - if (!refspec
> - && !(flags & TRANSPORT_PUSH_ALL)
> - && remote->push_refspec_nr) {
> - refspec = remote->push_refspec;
> - refspec_nr = remote->push_refspec_nr;
> + if ((flags & TRANSPORT_PUSH_MATCHING) && refspec) {
> + return error("--matching cannot be combined with refspecs");
> }
> +
> +
Yet another double empty line.
> + if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
> + if (remote->push_refspec_nr) {
> + refspec = remote->push_refspec;
> + refspec_nr = remote->push_refspec_nr;
> + } else if (!(flags & TRANSPORT_PUSH_MATCHING)) {
> + return error("No refspecs given and none configured for %s, nothing to push.", remote->name);
> + }
Long line and surplus curly brackets.
Just to make it clear, because many people misunderstand my comments: I
would not have spent my precious time writing this email if I did not
think that --matching is something we want to have.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
