Re: [PATCH 3/3] rev-parse: simplify parsing of ref options

From: Jeff King <peff@peff.net>
Date: Mar 15, 2017, 21:38 UTC
Message-ID: <20170315213818.qfccs2atfijngaxr@sigill.intra.peff.net>
In-Reply-To: <xmqq8to6dz76.fsf@gitster.mtv.corp.google.com>

On Wed, Mar 15, 2017 at 02:09:33PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > This actually drops the last caller for for_each_branch_ref(). I'm not
> > sure if we shoulder consider cleaning up the proliferation of
> > for_each_ref() helpers.
> 
> That is certainly a good thing to do (but outside this series).
> 
> I am wondering if "git diff" could have chosen a better way to
> arrange deleted and inserted lines.  The first two if statements
> in the preimage corresponds to the new opt-with-value(branches)
> thing, the next two are repalced by opt-with-value(tags), an
> odd-man-out "--glob=" thing is replaced with handle_ref_opt(NULL),
> and then two "--remotes" thing are paired with the final
> opt-with-value(remotes) thing.  
> 
> That would break the usual expectation of seeing all "-" first and
> then "+" when there is no intervening " " context lines, so such an
> output might break tools, though (I do not think "git apply" would
> choke).

I think in this case you could still show it better with context lines between. Something like:

@@ -749,42 +758,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_ref_in("refs/bisect/good", anti_reference, NULL);
 				continue;
 			}
-			if (skip_prefix(arg, "--branches=", &arg)) {
-				for_each_glob_ref_in(show_reference, arg,
-					"refs/heads/", NULL);
-				clear_ref_exclusion(&ref_excludes);
-				continue;
-			}
-			if (!strcmp(arg, "--branches")) {
-				for_each_branch_ref(show_reference, NULL);
-				clear_ref_exclusion(&ref_excludes);
+			if (opt_with_value(arg, "--branches", &arg)) {
+				handle_ref_opt(arg, "refs/heads/");
 				continue;
 			}
-			if (skip_prefix(arg, "--tags=", &arg)) {
-				for_each_glob_ref_in(show_reference, arg,
-					"refs/tags/", NULL);
-				clear_ref_exclusion(&ref_excludes);
-			if (!strcmp(arg, "--tags")) {
-				for_each_tag_ref(show_reference, NULL);
-				clear_ref_exclusion(&ref_excludes);
+			if (opt_with_value(arg, "--tags", &arg)) {
+				handle_ref_opt(arg, "refs/tags/");
 				continue;
 			}
 			if (skip_prefix(arg, "--glob=", &arg)) {
-				for_each_glob_ref(show_reference, arg, NULL);
-				clear_ref_exclusion(&ref_excludes);
+				handle_ref_opt(arg, NULL);
 				continue;
 			}
-			if (skip_prefix(arg, "--remotes=", &arg)) {
-				for_each_glob_ref_in(show_reference, arg,
-					"refs/remotes/", NULL);
-				clear_ref_exclusion(&ref_excludes);
-			if (!strcmp(arg, "--remotes")) {
-				for_each_remote_ref(show_reference, NULL);
-				clear_ref_exclusion(&ref_excludes);
+			if (opt_with_value(arg, "--remotes", &arg)) {
+				handle_ref_opt(arg, "refs/remotes/");
 				continue;
 			}
 			if (skip_prefix(arg, "--exclude=", &arg)) {

I have no idea what heuristic might generate that diff, though. Using
--patience and --histogram does provided different results than Myers
for this case, but none produces the output above. The other two try to
push the new "--branches" lines up as a replacement for "--branches=",
but then you get a clump of "--branches", "--tags=", and "--tags".

-Peff