Re: [PATCH v3] Add an explicit GIT_DIR to the list of excludes

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2014, 20:42 UTC
Message-ID: <xmqqha4gi5vg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1400866411-14584-1-git-send-email-pasha.bolokhov@gmail.com>

Pasha Bolokhov <pasha.bolokhov@gmail.com> writes:

Show 12 quoted lines

> diff --git a/dir.c b/dir.c
> index 98bb50f..76969a7 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1588,6 +1588,26 @@ void setup_standard_excludes(struct dir_struct *dir)
>  {
>  	const char *path;
>  	char *xdg_path;
> +	const char *gitdir = get_git_dir();
> +
> +	/* Add git directory to the ignores first */
> +	if (strcmp(gitdir, ".git") != 0) {

It is more idiomatic and common to say

	if (strcmp(gitdir, ".git")) {

around here, I think.

>     /* "--git-dir" has been given */

... or it could have come from GIT_DIR environment, no?

Does this "additional exclude" need to kick in if GIT_DIR is set to "/home/pasha/w/.git"? That is, when gitdir is ".git" or ends with "/.git"?

> +		char ngit[PATH_MAX + 1];

Hmmm. As I recall that recently we had flurry of changes to eradicate PATH_MAX from our codebase, I am not happy to see an introduction of a new buffer that is fixed-size.

Show 54 quoted lines

> +		/*
> +		 * See if GIT_DIR is inside the work tree; need to normalize
> +		 * 'gitdir', whereas 'get_git_work_tree()' always appears
> +		 * absolute and normalized
> +		 */
> +		normalize_path_copy(ngit, real_path(absolute_path(gitdir)));
> +
> +		if (dir_inside_of(ngit, get_git_work_tree()) >= 0) {
> +			struct exclude_list *el = add_exclude_list(dir, EXC_CMDL,
> +							"--git-dir option");
> +
> +			add_exclude(gitdir, "", 0, el, 0);
> +		}
> +	}
>  
>  	dir->exclude_per_dir = ".gitignore";
>  	path = git_path("info/exclude");
> diff --git a/t/t2205-add-gitdir.sh b/t/t2205-add-gitdir.sh
> new file mode 100755
> index 0000000..0c99508
> --- /dev/null
> +++ b/t/t2205-add-gitdir.sh
> @@ -0,0 +1,84 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2014 Pasha Bolokhov
> +#
> +
> +test_description='alternative repository path specified by --git-dir is ignored by add and status'
> +
> +. ./test-lib.sh
> +
> +#
> +# Create a tree:
> +#
> +# 	a  b  c  d  subdir/
> +#
> +# subdir:
> +# 	e  f  g  h  meta/  ssubdir/
> +#
> +# subdir/meta:
> +# 	aa
> +#
> +# subdir/ssubdir:
> +# 	meta/
> +#
> +# subdir/ssubdir/meta:
> +# 	aaa
> +#
> +# Name the repository "meta" and see whether or not "git status" includes
> +# or ignores directories named "meta". The slightly deeper hierarchy is
> +# needed in order to be able to put the repository into "../meta", that is,
> +# outside the work tree and still have files called "meta" within the tree
> +#

It is not quite clear with this large blob of comment what are noises and what exactly are being tested. I think you have two directories called "meta", but which one is the repository? Or do you have yet another one next to {a,b,c,d,subdir} called "meta" that is not listed above?

Given that the reason why people use --git-dir is so that they can put it completely outside the working tree (in which case, the usual "start from cwd and go upwards while trying to see if there is .git/ that governs the working tree" logic would not work), readers would not expect to find the directory to be used as GIT_DIR in the hierarchy you are creating in the first place. Because of that, it is even more important to clearify which "meta" you mean to use as your GIT_DIR if you want to be understood by readers.