Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	Not only because of ohloh am I proud that in spite of removing
> > 	more lines than I added, there were more comments added than
> > 	removed...
> 
> > diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
> > index 497903a..3f787a8 100644
> > --- a/builtin-rev-parse.c
> > +++ b/builtin-rev-parse.c
> > @@ -320,15 +320,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  				continue;
> >  			}
> >  			if (!strcmp(arg, "--show-cdup")) {
> > -				const char *pfx = prefix;
> > -				while (pfx) {
> > -					pfx = strchr(pfx, '/');
> > -					if (pfx) {
> > -						pfx++;
> > -						printf("../");
> > -					}
> > -				}
> > -				putchar('\n');
> > +				const char *work_tree = get_git_work_tree();
> > +				if (work_tree)
> > +					printf("%s\n", work_tree);
> >  				continue;
> 
> This changes semantics, I think.
> 
> It used to be relative "up" path when no funny work-tree stuff
> is used, but get_git_work_tree() now seems to return absolute,
> hence this option as well.  If it introduces regression to
> existing callers is up to what the caller does to the resulting
> path, though.  If it only is used to prefix other things
> (i.e. path="$(git rev-parse --show-cdup)$1"), the caller would
> be safe, but if the caller counted number of ../ in the return
> value to see how deep it is, or if the caller expected to see
> empty string in order to see if the process is at the toplevel,
> this change would become a regression.

I am somewhat negative on keeping _that_ much backwards compatibility.  
Scripts which depend on show-cdup being a relative path _will_ be broken 
by work-tree.  Is it worth it to detect those errors late?

> > @@ -62,15 +66,8 @@ static void setup_git_env(void)
> >  
> >  int is_bare_repository(void)
> >  {
> > +	/* if core.bare is not 'false', let's see if there is a work tree */
> > +	return is_bare_repository_cfg && !get_git_work_tree();
> >  }
> 
> I thought about making core.bare a tertiary, true/false/depends,
> but I think this makes more sense.

Actually, you made me think again, and I am more along those lines now:

	return is_bare_repository_cfg >= 0 ?
		is_bare_repository_cfg : !get_git_work_tree();

and according patch to get_git_work_tree to return NULL if 
is_bare_repository_cfg == 1.

Ciao,
Dscho