"Rudolf Polzer" <divVerent@xxxxxxxxxxxxx> writes:
> On Wed, 13 Jan 2010 16:43:10 +0100, Ilari Liusvaara
> <ilari.liusvaara@xxxxxxxxxxx> 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.
--
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