Re: [PATCH] rev-parse --namespace

From: Jeff King <peff@peff.net>
Date: Jan 17, 2010, 19:08 UTC
Message-ID: <20100117190827.GA31536@coredump.intra.peff.net>
In-Reply-To: <20100117164057.GA20554@Knoppix>

On Sun, Jan 17, 2010 at 06:40:57PM +0200, Ilari Liusvaara wrote:

Show 17 quoted lines

> On Sun, Jan 17, 2010 at 11:27:12AM -0500, Jeff King wrote:
> > On Sun, Jan 17, 2010 at 03:45:31PM +0200, Ilari Liusvaara wrote:
> > 
> > > Add --namespace=<namespace> option to rev-parse and everything that
> > > accepts its options. This option matches all refs in some subnamespace
> > > of refs hierarchy, and is useful for selecting everything reachable from
> > > one or few, but not all remotes (--namespace=remotes/foo).
> > 
> > If I understand it correctly, isn't the same as
> > 
> >   git for-each-ref refs/remotes/foo
> 
> Nope. Compare:
> 
> 'git log --branches --not --namespace=remotes/origin' 
> 
> with whatever you would have to currently type...

OK, that makes sense and is actually useful. That is the sort of motivation that should go in the commit message. Mentioning "rev-parse" is a bit of a red herring.

As for the patch text itself, it looks correct, though I have a few nits:

Show 12 quoted lines

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 82045a2..af4605a 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -112,6 +112,10 @@ OPTIONS
>  --remotes::
>  	Show tag refs found in `$GIT_DIR/refs/remotes`.
>  
> +--namespace=namespace-prefix::
> +	Show refs found in `$GIT_DIR/namespace-prefix`. If namespace
> +	specified lacks leading 'refs/', it is automatically prepended.
> +

This is not a new problem you are introducing, as you are just following the template of of --remotes and others above, but is "show" really the right word? In the rev-list page these entries talk about "pretend as if these refs were given on the command line". Isn't that also the case here? If I say "git rev-parse --not --remotes" (or --namespace=), I will get "^"-lines.

Also, I notice in the context that the remotes entry says "Show tag refs" which is obviously wrong. Again, not your problem, but something to clean up while in the area.

Show 5 quoted lines

> +			if (!prefixcmp(arg, "--namespace=")) {
> +				set_for_each_namespace(arg + 12);
> +				for_each_namespace_ref(show_reference, NULL);
> +				continue;
> +			}

I was going to complain about the use of a global variable here when we could simply pass the namespace into the function, but I see that in the other call here:

Show 13 quoted lines

> diff --git a/revision.c b/revision.c
> index 25fa14d..9e1d960 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1352,6 +1352,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>  				handle_refs(revs, flags, for_each_remote_ref);
>  				continue;
>  			}
> +			if (!prefixcmp(arg, "--namespace=")) {
> +				set_for_each_namespace(arg + 12);
> +				handle_refs(revs, flags, for_each_namespace_ref);
> +				continue;
> +			}

...that giving the for_each_namespace_ref function a different signature than the other for_each_* functions would complicate things. OTOH, handle_refs is only a 4 line helper to set up the callback struct, so perhaps it is better to simply duplicate it rather than introduce a new global.

Too bad we can't do currying in C. :)

Show 22 quoted lines

> +void set_for_each_namespace(const char *ref_namespace)
> +{
> +	size_t alloclen, origlen;
> +	if (prefix_namespace)
> +		free(prefix_namespace);
> +
> +	/* Compute the length of true prefix. */
> +	origlen = alloclen = strlen(ref_namespace);
> +	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
> +		alloclen++;
> +	if (prefixcmp(ref_namespace, "refs/"))
> +		alloclen += 5;		/* 'refs/' */
> +
> +	/* Allocate and build it (assume alloclen is "small") */
> +	prefix_namespace = xmalloc(alloclen + 1);
> +	*prefix_namespace = 0;
> +	if (prefixcmp(ref_namespace, "refs/"))
> +		strcat(prefix_namespace, "refs/");
> +	strcat(prefix_namespace, ref_namespace);
> +	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
> +		strcat(prefix_namespace, "/");
> +}

Wouldn't this be much simpler and more readable using a strbuf? As in:

  strbuf_reset(&prefix_namespace);
  if (prefixcmp(ref_namespace, "refs/"))
    strbuf_addstr(&prefix_namespace, "refs/");
  strbuf_addstr(&prefix_namespace, ref_namespace);
  if (prefix_namespace.buf[prefix_namespace.len-1] != '/')
    strbuf_addch(&prefix_namespace, '/');

Show 14 quoted lines

> +test_expect_success 'setup' '
> +
> +	commit master &&
> +	git checkout -b subspace/one master
> +	commit one &&
> +	git checkout -b subspace/two master
> +	commit two &&
> +	git checkout -b other/three master
> +	commit three &&
> +	git checkout -b someref master
> +	commit some &&
> +	git checkout master
> +	commit master2
> +'

Missing '&&' on the checkout lines?

Show 5 quoted lines

> +test_expect_success 'rev-parse --namespace=refs/heads/subspace/' '
> +
> +	test 2 = $(git rev-parse --namespace=refs/heads/subspace/ | wc -l)
> +
> +'

This is perhaps a minor nit, but I really prefer to write this by putting the expected output in a file and using test_cmp, because:

  1. You actually test that rev-parse exits correctly.

  2. You test that it actually produced the correct output, and not
     simply two lines which are presumably correct.

  3. If it _does_ fail, the output from test_cmp is a nicely readable
     diff between expected and actual output. Your test produces no
     output.

We also had some problems with different implementations of "wc" producing different amounts of whitespace, but I don't think that is a problem here since the shell should eat any whitespace outside of quotation marks.

-Peff