From: Junio C Hamano <gitster@pobox•com>
To: "Rudolf Polzer" <divVerent@alientrap•org>
Cc: git@vger•kernel.org, Ilari Liusvaara <ilari.liusvaara@elisanet•fi>
Subject: Re: [PATCH] git push --track
Date: Thu, 14 Jan 2010 21:47:22 -0800 [thread overview]
Message-ID: <7vvdf33onp.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <op.u6haiiiog402ra@nb-04> (Rudolf Polzer's message of "Wed\, 13 Jan 2010 16\:55\:20 +0100")
"Rudolf Polzer" <divVerent@alientrap•org> writes:
> On Wed, 13 Jan 2010 16:43:10 +0100, Ilari Liusvaara
> <ilari.liusvaara@elisanet•fi> wrote:
>
>> - Some lines look way too long (~160 chars, should be max 80 unles
>> it would linebreak error message).
>
> Yes, also I got told that I used the wrong braces style... well, fixed
> that.
You didn't, although you tried to "hide" one level, which is even worse.
If you see overlong lines in patch text, it often is that the added
codepath is too deeply nested, and it often becomes much easier to
understand if you split it into a separate smaller helper function.
For example, if you have
if (A) {
if (B) {
do something #1
if (C) {
do something #2
while (D) {
if (E) {
do something #3
}
}
}
}
}
it often is much easier to read if you did:
if (A)
helper(...)
and wrote a helper that is
helper()
{
if (!B)
return
do something #1
if (!C)
return
do something #2
while (D) {
if (!E)
continue
do something #3
}
}
>> - Should the tracking be set up even if only part of ref update suceeded
>> (for those that succeeded), not requiring all to succeed?
I think giving configuration to the ones that succeeded, while not doing
so for the ones that failed, would be the best.
>> - Is --track the best name for this?
Most probably not. "git branch --track" was already a mistake, whose
damage can be seen in the first message in this thread. I originally read
"this converts a local branch to a tracking branch", and went "Huh??? ---
Is this patch running 'mv refs/heads/frotz refs/remotes/origin/frotz'?
What's fun about it???"
> @@ -115,6 +116,36 @@ static int push_with_options(struct transport
> *transport, int flags)
> fprintf(stderr, "Pushing to %s\n", transport->url);
> err = transport_push(transport, refspec_nr, refspec, flags,
> &nonfastforward);
> + if (err == 0 && flags & TRANSPORT_PUSH_TRACK) {
Style:
- Have SP between syntactic keyword and open parenthesis.
- Never place an opening brace, except the one that begins a function
body, on its own line;
Also the overlong line is merely a symptom that you are putting too much
stuff in this function. The whole addition should probably be a helper
function.
> @@ -115,6 +116,33 @@ static int push_with_options(struct transport *transport, int flags)
> fprintf(stderr, "Pushing to %s\n", transport->url);
> err = transport_push(transport, refspec_nr, refspec, flags,
> &nonfastforward);
> + if (err == 0 && flags & TRANSPORT_PUSH_TRACK)
> + {
> + struct ref *remote_refs =
> + transport->get_refs_list(transport, 1);
You have already pushed by calling transport_push() before you got "err"
back. Do you need to make a second, separate call to ls-remote here and
if so why?
I have a feeling that it is more appropriate to have the additional code
in transport_push(), which gets ls-remote information, runs match_refs()
and finally calls transport->push_refs(). I think the extra branch
configuration would fit better inside the if block immediately after all
that happens, i.e.
if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
struct ref *ref;
for (ref = remote_refs; ref; ref = ref->next)
update_tracking_ref(transport->remote, ref, verbose);
+ if (flags & TRANSPORT_PUSH_RECONFIGURE_FORK)
+ configure_forked_branch(...);
}
in transport.c
> + if(!(flags & TRANSPORT_PUSH_DRY_RUN))
> + if(!match_refs(local_refs, &remote_refs, refspec_nr, refspec, match_flags))
Yuck; hiding the fact that you have an over-nested logic is not a way to
fix it.
next prev parent reply other threads:[~2010-01-15 5:49 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-13 15:12 [PATCH] git push --track Rudolf Polzer
2010-01-13 15:43 ` Ilari Liusvaara
2010-01-13 15:55 ` Rudolf Polzer
2010-01-13 16:27 ` Ilari Liusvaara
2010-01-13 16:37 ` Matthieu Moy
2010-01-14 5:21 ` Tay Ray Chuan
2010-01-14 7:00 ` Rudolf Polzer
2010-01-14 23:13 ` Junio C Hamano
2010-01-14 7:16 ` Jeff King
2010-01-15 5:47 ` Junio C Hamano [this message]
2010-01-15 14:00 ` Rudolf Polzer
2010-01-15 15:45 ` Miles Bader
2010-01-15 18:16 ` Junio C Hamano
2010-01-14 0:28 ` Miles Bader
2010-01-14 0:25 ` Miles Bader
2010-01-14 0:33 ` Johannes Schindelin
2010-01-14 0:36 ` Miles Bader
2010-01-14 0:46 ` Miles Bader
2010-01-14 7:01 ` Rudolf Polzer
2010-01-14 13:44 ` Martin Langhoff
2010-01-14 14:16 ` Johannes Schindelin
2010-01-14 14:25 ` Matthieu Moy
2010-01-14 14:35 ` Martin Langhoff
2010-01-14 15:27 ` Andreas Krey
2010-01-14 1:27 ` Tay Ray Chuan
2010-01-14 1:35 ` Miles Bader
2010-01-14 1:37 ` Tay Ray Chuan
2010-01-14 1:49 ` Miles Bader
2010-01-14 1:58 ` Tay Ray Chuan
2010-01-14 7:03 ` Rudolf Polzer
2010-01-14 23:46 ` Junio C Hamano
2010-01-15 0:30 ` Miles Bader
2010-01-15 18:18 ` Junio C Hamano
2010-01-15 18:54 ` Miles Bader
2010-01-15 13:26 ` Matthieu Moy
2010-01-14 6:41 ` Nanako Shiraishi
2010-01-14 7:08 ` Rudolf Polzer
2010-01-14 10:31 ` Johannes Schindelin
2010-01-14 22:27 ` Nanako Shiraishi
2010-01-14 23:50 ` Junio C Hamano
2010-01-15 13:44 ` Rudolf Polzer
2010-01-15 14:09 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vvdf33onp.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=divVerent@alientrap$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=ilari.liusvaara@elisanet$(echo .)fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox