Re: [PATCH 1/2] Add for_each_remote() function, and extend remote_find_tracking()

Johannes Schindelin [off-list ref] writes:

The function for_each_remote() does exactly what the name suggests.

The function remote_find_tracking() was extended to be able to search
remote refs for a given local ref.  You have to set the parameter
"reverse" to true for that behavior.

Both changes are required for the next step: simplification of
git-branch's --track functionality.

Signed-off-by: Johannes Schindelin <redacted>
---

	You're right. I completely missed that functionality. Well, a
	few tweaks were needed. If this clashes too seriously with
	Daniel's work, I will gladly redo it after his changes are
	in "next".

No offence meant to Daniel, but I am inclined to postpone the
current round of changes from him to move the stuff further
to get us closer to built-in git-fetch until 1.5.3 final is
done.  The amount of C code changes otherwise would be a bit too
much for me to be comfortable between -rc0 and -rc1.

Just a minor style, but (you know what comes)...

I have to agree with Daniel here --- variable names src and dst
are quite confusing.  It seems to mean that "we search with
'src' to fill 'dst', but if reverse incoming refspec is given
reversed so matching refspec->src with what in fact is 'dst' in
the configuration file is fine".  Utterly confusing.

Even though this is a good opportunity for you to improve your
comment ratio in ohloh stats, I doubt any amount of explanation
can unconfuse readers of this code.

 		if (!fetch->dst)
 			continue;

 		if (fetch->pattern) {
+			if (!prefixcmp(refspec->src, src)) {
 				refspec->dst =
+					xmalloc(strlen(dst) +
 						strlen(refspec->src) -
+						strlen(src) + 1);
+				strcpy(refspec->dst, dst);
+				strcpy(refspec->dst + strlen(dst),
+				       refspec->src + strlen(src));
 				refspec->force = fetch->force;
 				return 0;
 			}
 		} else {
+			if (!strcmp(refspec->src, src)) {
+				refspec->dst = xstrdup(dst);
 				refspec->force = fetch->force;
 				return 0;
 			}
 		}
 	}
-	refspec->dst = NULL;
 	return -1;
 }

The original remote_find_tracking() took a single remote and a
refspec with src filled, and returned the given refspec after
filling its dst, if an appropriate tracking was configured for
the remote.  What you want to do is from the same remote
information and a refspec with dst filled to find a src branch
that would be stored to that dst.

In either case, incoming refspec would not have both src and dst
filled.  The caller has one side of the information and asking
for the other.  As Daniel suggests, the 'reverse' parameter is
not needed.  If you must have it, please do not call it
'reverse' -- it is more like "find_by_dst".

Following Daniel's suggestion might make the code a bit
lengthier, but I think that would not be as confusing.