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

> From: Zakariyah Ali <zakariyahali100@gmail.com>
>
> Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
> ---
>     completion: hide dotfiles for selected path completion
>     
>     The completion helper for index paths uses git ls-files rather than
>     shell filename completion. As a result, leading-dot paths such as a
>     tracked .gitignore were offered even when the user had not started the
>     path with ..

Writing 'path with ".".' would have been easieer to grok.

>     Hide leading-dot path components for git rm, git mv, and git ls-files
>     when completing an empty path component. Explicit dot completion is
>     still preserved, so git rm . can still complete .gitignore.

I am not sure why this is a good idea.  If we said "git rm g<TAB>
and offered ".gitignore" as a candidate, it may be annoying, but
tracked (or untracked for that matter) ".gitignore" and "gitfoo"
should be treated the same way by "git rm <TAB>" no?

>     This removes the existing TODO expectations in t/t9902-completion.sh and
>     adds coverage for explicit dot completion.

In any case, all of the above should be in the proposed log message,
not below the three-dash line.

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

> This matches standard shell filename completion behavior, where dotfiles
> are hidden by default unless the user starts their input with a dot.

OK, with this rationale added, I no longer have problem with the
proposed new behaviour.

As I'm not going to give a serious review  on the patch body itself,
I would really appreciate somebody more knowledgeable on the
existing bach completion code than I am to take a look.

Thanks.

Dear Junio,

I hope you are doing well.

I wanted to briefly follow up on my recent patch submission (Message-ID: <pull.2311.v2.git.git.1779808987825.gitgitgadget@gmail.com>). Thank you for accepting the rationale for the new behavior! Since you mentioned it would be helpful for someone more familiar with the bash completion code to review the patch itself, I wanted to ask if there is anyone specific I should CC, or if I should simply wait for another reviewer to pick it up. I would be grateful if you would let me know if there is anything else needed from my side.

Also, thank you again for the detailed reviews and guidance on my recent Git contributions. Your feedback on patch structure, commit messaging, and contribution workflow has been extremely valuable, and I genuinely appreciate the time you invest in reviewing contributions from newer developers.

Separately, I also wanted to ask for your advice professionally. I am a software engineer with over four years of experience, currently looking for entry-level or internship opportunities where I can continue growing as a systems and open-source developer. If you happen to know of any relevant opportunities, or have suggestions on how I might better position myself through open-source work  or any other opportunities, I would sincerely appreciate any guidance.

Thank you again for your time and for maintaining such a high-quality development and review culture around Git.

Best regards,
Zakariyah Ali.

"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).