Re: [PATCH v2] completion: hide dotfiles for selected path completion

[Junio C Hamano <gitster@pobox.com>]

"Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -# __git_index_files accepts 1 or 2 arguments:
> +# __git_index_files accepts 1 to 4 arguments:
>  # 1: Options to pass to ls-files (required).
>  # 2: A directory path (optional).
>  #    If provided, only files within the specified directory are listed.
>  #    Sub directories are never recursed.  Path must have a trailing
>  #    slash.
>  # 3: List only paths matching this path component (optional).
> +# 4: Hide paths whose first component starts with a dot if this is
> +#    "hide-dotfiles" and the third argument is empty (optional).
>  __git_index_files ()
>  {
> -	local root="$2" match="$3"
> +	local root="$2" match="$3" hide_dotfiles="${4-}"
> +	local hide_dotfiles_awk=0
> +	if [ "$hide_dotfiles" = "hide-dotfiles" ] && [ -z "$match" ]; then
> +		hide_dotfiles_awk=1
> +	fi
>  
>  	__git_ls_files_helper "$root" "$1" "${match:-?}" |
> -	awk -F / -v pfx="${2//\\/\\\\}" '{
> +	awk -F / -v pfx="${2//\\/\\\\}" -v hide_dotfiles="$hide_dotfiles_awk" '{
>  		paths[$1] = 1
>  	}
>  	END {
>  		for (p in paths) {
>  			if (substr(p, 1, 1) != "\"") {
>  				# No special characters, easy!
> +				if (hide_dotfiles == 1 && substr(p, 1, 1) == ".")
> +					continue
>  				print pfx p
>  				continue
>  			}
> @@ -675,8 +683,10 @@ __git_index_files ()
>  				# We have seen the same directory unquoted,
>  				# skip it.
>  				continue
> -			else
> -				print pfx p
> +
> +			if (hide_dotfiles == 1 && substr(p, 1, 1) == ".")
> +				continue
> +			print pfx p
>  		}
>  	}

Having to repeat the same thing twice here is a bit unsatisfying,
but that is not a fault of this addition.  I suspect that it would
have been simpler to patch if the original were first simplified
into something like:

	for (p in paths) {
		if (substr(p, 1, 1) == "\"") {
			p = dequote(p);
			if ((p == "") || (p in paths))
				continue
		}
                print pfx p
	}

Then the new "ah, that thing begins with a dot" logic can be added
only once and at an obvious place.

> @@ -2164,7 +2176,7 @@ _git_ls_files ()
>  
>  	# XXX ignore options like --modified and always suggest all cached
>  	# files.
> -	__git_complete_index_file "--cached"
> +	__git_complete_index_file "--cached" hide-dotfiles
>  }

In this patch, it is hard to tell from the patch what _other_ calls
to the __git_complete_index_file helper lack hide-dotfiles flag
(i.e., they are to show everything including the path that begins
with a dot).  I will not try to be exhaustive, but for example
_git_add does not get hide-dotfiles but it is unclear why.  The same
for _clean, _commit.  But _mv does hide them.  The choice seems
arbitrary and incoherent.

A few ideas (some of them may be mutually incompatible)

 * Instead of "empty vs hide-dotfiles", perhaps make the 2nd option
   mandatory for __git_complete_index_file, e.g., "hide-" vs
   "include-" dotfiles, to make it easier to see in the patch which
   ones exclude and which ones include dotfiles.

 * Extend comments like we saw in the above hunk to say why we treat
   files that begin with dot specially.

 * Make __git_complete_index_file unconditionally hide the dotfiles
   when there is no match pattern for consistency (getting rid of
   the need to explay why).