# [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls. 16 messages from 2009-02-11 to 2009-02-11. Participants: Ted Pavlic, Shawn O. Pearce, Junio C Hamano. Thread: https://gitlist.dev/t/17724 ## Ted Pavlic, 2009-02-11 15:37 Subject: [PATCH 0/3] completion: Convention updates and DIRTYSTATE fix Message-ID: <1234366634-17900-1-git-send-email-ted@tedpavlic.com> URL: https://gitlist.dev/e/1234366634-17900-1-git-send-email-ted%40tedpavlic.com ``` Three more patches to git's bash completion script. The first two are non-critical: * The first patch changes two "git rev-parse" calls to "__gitdir" to match the convention used in the rest of the script. * The second patch changes "[...]" to "test ..." to match git convention. In the one case of "[...] || [...]", a "test || test" call is used. Alternatively, a "test ... -o ..." call could be used, but that might not be as readable. The third fixes an ugly error in the new GIT_PS1_DIRTYSTATE. In order to determine whether the branch name needs a "*" following it, the DIRTYSTATE implementation uses a "git diff." Because "git diff" is illegal when not in a working tree, this command gives an ugly error when changing directory to ".git". This patch detects this case and sets "--work-tree=..". Is there a better fix? Ted Pavlic (3): completion: For consistency, changed "git rev-parse" to __gitdir calls. completion: Change "if [...]" to "if test ..." to match git convention completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git contrib/completion/git-completion.bash | 101 +++++++++++++++----------------- 1 files changed, 48 insertions(+), 53 deletions(-) ``` ## Ted Pavlic, 2009-02-11 15:37 Subject: [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls. Message-ID: <1234366634-17900-2-git-send-email-ted@tedpavlic.com> URL: https://gitlist.dev/e/1234366634-17900-2-git-send-email-ted%40tedpavlic.com In-Reply-To: <1234366634-17900-1-git-send-email-ted@tedpavlic.com> ``` Signed-off-by: Ted Pavlic --- contrib/completion/git-completion.bash | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index f44f63c..6bbe09a 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -80,7 +80,7 @@ __gitdir () # returns text to add to bash PS1 prompt (includes branch name) __git_ps1 () { - local g="$(git rev-parse --git-dir 2>/dev/null)" + local g="$(__gitdir)" if [ -n "$g" ]; then local r local b @@ -1797,7 +1797,7 @@ _gitk () __git_has_doubledash && return local cur="${COMP_WORDS[COMP_CWORD]}" - local g="$(git rev-parse --git-dir 2>/dev/null)" + local g="$(__gitdir)" local merge="" if [ -f $g/MERGE_HEAD ]; then merge="--merge" -- 1.6.1.2.390.gba743 ``` ## Ted Pavlic, 2009-02-11 15:37 Subject: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Message-ID: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> URL: https://gitlist.dev/e/1234366634-17900-3-git-send-email-ted%40tedpavlic.com In-Reply-To: <1234366634-17900-2-git-send-email-ted@tedpavlic.com> ``` In the single case of: if [...] || [...] changed to: if test ... || test ... However, if test ... -o ... might be favorable (although arguably less readable). Also changed: if test ... then to: if test ...; then Signed-off-by: Ted Pavlic --- contrib/completion/git-completion.bash | 91 ++++++++++++++----------------- 1 files changed, 41 insertions(+), 50 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 6bbe09a..6772be7 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -61,15 +61,15 @@ esac # returns location of .git repo __gitdir () { - if [ -z "${1-}" ]; then - if [ -n "$__git_dir" ]; then + if test -z "${1-}"; then + if test -n "$__git_dir"; then echo "$__git_dir" - elif [ -d .git ]; then + elif test -d .git; then echo .git else git rev-parse --git-dir 2>/dev/null fi - elif [ -d "$1/.git" ]; then + elif test -d "$1/.git"; then echo "$1/.git" else echo "$1" @@ -81,42 +81,33 @@ __gitdir () __git_ps1 () { local g="$(__gitdir)" - if [ -n "$g" ]; then + if test -n "$g"; then local r local b - if [ -d "$g/rebase-apply" ] - then - if test -f "$g/rebase-apply/rebasing" - then + if test -d "$g/rebase-apply"; then + if test -f "$g/rebase-apply/rebasing"; then r="|REBASE" - elif test -f "$g/rebase-apply/applying" - then + elif test -f "$g/rebase-apply/applying"; then r="|AM" else r="|AM/REBASE" fi b="$(git symbolic-ref HEAD 2>/dev/null)" - elif [ -f "$g/rebase-merge/interactive" ] - then + elif test -f "$g/rebase-merge/interactive"; then r="|REBASE-i" b="$(cat "$g/rebase-merge/head-name")" - elif [ -d "$g/rebase-merge" ] - then + elif test -d "$g/rebase-merge"; then r="|REBASE-m" b="$(cat "$g/rebase-merge/head-name")" - elif [ -f "$g/MERGE_HEAD" ] - then + elif test -f "$g/MERGE_HEAD"; then r="|MERGING" b="$(git symbolic-ref HEAD 2>/dev/null)" else - if [ -f "$g/BISECT_LOG" ] - then + if test -f "$g/BISECT_LOG"; then r="|BISECTING" fi - if ! b="$(git symbolic-ref HEAD 2>/dev/null)" - then - if ! b="$(git describe --exact-match HEAD 2>/dev/null)" - then + if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then + if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then b="$(cut -c1-7 "$g/HEAD")..." fi fi @@ -138,7 +129,7 @@ __git_ps1 () fi fi - if [ -n "${1-}" ]; then + if test -n "${1-}"; then printf "$1" "${b##refs/heads/}$w$i$r" else printf " (%s)" "${b##refs/heads/}$w$i$r" @@ -164,7 +155,7 @@ __gitcomp_1 () __gitcomp () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ $# -gt 2 ]; then + if test $# -gt 2; then cur="$3" fi case "$cur" in @@ -184,7 +175,7 @@ __gitcomp () __git_heads () { local cmd i is_hash=y dir="$(__gitdir "${1-}")" - if [ -d "$dir" ]; then + if test -d "$dir"; then git --git-dir="$dir" for-each-ref --format='%(refname:short)' \ refs/heads return @@ -203,7 +194,7 @@ __git_heads () __git_tags () { local cmd i is_hash=y dir="$(__gitdir "${1-}")" - if [ -d "$dir" ]; then + if test -d "$dir"; then git --git-dir="$dir" for-each-ref --format='%(refname:short)' \ refs/tags return @@ -223,14 +214,14 @@ __git_refs () { local i is_hash=y dir="$(__gitdir "${1-}")" local cur="${COMP_WORDS[COMP_CWORD]}" format refs - if [ -d "$dir" ]; then + if test -d "$dir"; then case "$cur" in refs|refs/*) format="refname" refs="${cur%/*}" ;; *) - if [ -e "$dir/HEAD" ]; then echo HEAD; fi + if test -e "$dir/HEAD"; then echo HEAD; fi format="refname:short" refs="refs/tags refs/heads refs/remotes" ;; @@ -299,7 +290,7 @@ __git_remotes () __git_merge_strategies () { - if [ -n "$__git_merge_strategylist" ]; then + if test -n "$__git_merge_strategylist"; then echo "$__git_merge_strategylist" return fi @@ -385,7 +376,7 @@ __git_complete_revlist () __git_all_commands () { - if [ -n "$__git_all_commandlist" ]; then + if test -n "$__git_all_commandlist"; then echo "$__git_all_commandlist" return fi @@ -403,7 +394,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" __git_porcelain_commands () { - if [ -n "$__git_porcelain_commandlist" ]; then + if test -n "$__git_porcelain_commandlist"; then echo "$__git_porcelain_commandlist" return fi @@ -511,7 +502,7 @@ __git_aliased_command () local word cmdline=$(git --git-dir="$(__gitdir)" \ config --get "alias.$1") for word in $cmdline; do - if [ "${word##-*}" ]; then + if test "${word##-*}"; then echo $word return fi @@ -526,7 +517,7 @@ __git_find_subcommand () while [ $c -lt $COMP_CWORD ]; do word="${COMP_WORDS[c]}" for subcommand in $1; do - if [ "$subcommand" = "$word" ]; then + if test "$subcommand" = "$word"; then echo "$subcommand" return fi @@ -539,7 +530,7 @@ __git_has_doubledash () { local c=1 while [ $c -lt $COMP_CWORD ]; do - if [ "--" = "${COMP_WORDS[c]}" ]; then + if test "--" = "${COMP_WORDS[c]}"; then return 0 fi c=$((++c)) @@ -552,7 +543,7 @@ __git_whitespacelist="nowarn warn error error-all fix" _git_am () { local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)" - if [ -d "$dir"/rebase-apply ]; then + if test -d "$dir"/rebase-apply; then __gitcomp "--skip --resolved --abort" return fi @@ -636,7 +627,7 @@ _git_bisect () local subcommands="start bad good skip reset visualize replay log run" local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" return fi @@ -672,7 +663,7 @@ _git_branch () " ;; *) - if [ $only_local_ref = "y" -a $has_r = "n" ]; then + if test $only_local_ref = "y" -a $has_r = "n"; then __gitcomp "$(__git_heads)" else __gitcomp "$(__git_refs)" @@ -830,7 +821,7 @@ _git_fetch () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else case "$cur" in @@ -1088,7 +1079,7 @@ _git_pull () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else __gitcomp "$(__git_refs "${COMP_WORDS[2]}")" @@ -1099,7 +1090,7 @@ _git_push () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else case "$cur" in @@ -1125,7 +1116,7 @@ _git_push () _git_rebase () { local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)" - if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then + if test -d "$dir"/rebase-apply || test -d "$dir"/rebase-merge; then __gitcomp "--continue --skip --abort" return fi @@ -1416,7 +1407,7 @@ _git_remote () { local subcommands="add rename rm show prune update" local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" return fi @@ -1546,7 +1537,7 @@ _git_stash () { local subcommands='save list show apply clear drop pop create branch' local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" else local cur="${COMP_WORDS[COMP_CWORD]}" @@ -1576,7 +1567,7 @@ _git_submodule () __git_has_doubledash && return local subcommands="add status init update summary foreach sync" - if [ -z "$(__git_find_subcommand "$subcommands")" ]; then + if test -z "$(__git_find_subcommand "$subcommands")"; then local cur="${COMP_WORDS[COMP_CWORD]}" case "$cur" in --*) @@ -1598,7 +1589,7 @@ _git_svn () proplist show-ignore show-externals " local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" else local remote_opts="--username= --config-dir= --no-auth-cache" @@ -1690,7 +1681,7 @@ _git_tag () COMPREPLY=() ;; -*|tag) - if [ $f = 1 ]; then + if test $f = 1; then __gitcomp "$(__git_tags)" else COMPREPLY=() @@ -1718,7 +1709,7 @@ _git () c=$((++c)) done - if [ -z "$command" ]; then + if test -z "$command"; then case "${COMP_WORDS[COMP_CWORD]}" in --*) __gitcomp " --paginate @@ -1799,7 +1790,7 @@ _gitk () local cur="${COMP_WORDS[COMP_CWORD]}" local g="$(__gitdir)" local merge="" - if [ -f $g/MERGE_HEAD ]; then + if test -f $g/MERGE_HEAD; then merge="--merge" fi case "$cur" in @@ -1820,7 +1811,7 @@ complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \ # when the user has tab-completed the executable name and consequently # included the '.exe' suffix. # -if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then +if test Cygwin = "$(uname -o 2>/dev/null)"; then complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \ || complete -o default -o nospace -F _git git.exe fi -- 1.6.1.2.390.gba743 ``` ## Ted Pavlic, 2009-02-11 15:37 Subject: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <1234366634-17900-4-git-send-email-ted@tedpavlic.com> URL: https://gitlist.dev/e/1234366634-17900-4-git-send-email-ted%40tedpavlic.com In-Reply-To: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> ``` The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should be placed after the branch name. The "git diff" fails with an ugly error if the user has just changed directory into GIT_DIR. This patch uses "git rev-parse --is-inside-work-tree" to determine whether a "--work-tree=.." should be added to the "git diff". Signed-off-by: Ted Pavlic --- contrib/completion/git-completion.bash | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 6772be7..ffde82a 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -115,10 +115,14 @@ __git_ps1 () local w local i + local worktreestr if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then if test "$(git config --bool bash.showDirtyState)" != "false"; then - git diff --no-ext-diff --ignore-submodules \ + if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then + worktreestr="--work-tree=.." + fi + git ${worktreestr} diff --no-ext-diff --ignore-submodules \ --quiet --exit-code || w="*" if git rev-parse --quiet --verify HEAD >/dev/null; then git diff-index --cached --quiet \ -- 1.6.1.2.390.gba743 ``` ## Ted Pavlic, 2009-02-11 16:18 Subject: [PATCH 4/3] completion: More fixes to prevent unbound variable errors. Message-ID: <1234369132-22063-1-git-send-email-ted@tedpavlic.com> URL: https://gitlist.dev/e/1234369132-22063-1-git-send-email-ted%40tedpavlic.com In-Reply-To: <1234366634-17900-4-git-send-email-ted@tedpavlic.com> ``` Several functions make use of "test -n" and "test -z". In many cases, the variables being tested were declared with "local." However, several __variables are not, and so they must be replaced with their ${__-} equivalents. Signed-off-by: Ted Pavlic --- This patch depends on: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention" If that patch is not applied, I can submit a new patch. contrib/completion/git-completion.bash | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index ffde82a..055e4ac 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -62,7 +62,7 @@ esac __gitdir () { if test -z "${1-}"; then - if test -n "$__git_dir"; then + if test -n "${__git_dir-}"; then echo "$__git_dir" elif test -d .git; then echo .git @@ -294,7 +294,7 @@ __git_remotes () __git_merge_strategies () { - if test -n "$__git_merge_strategylist"; then + if test -n "${__git_merge_strategylist-}"; then echo "$__git_merge_strategylist" return fi @@ -380,7 +380,7 @@ __git_complete_revlist () __git_all_commands () { - if test -n "$__git_all_commandlist"; then + if test -n "${__git_all_commandlist-}"; then echo "$__git_all_commandlist" return fi @@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" __git_porcelain_commands () { - if test -n "$__git_porcelain_commandlist"; then + if test -n "${__git_porcelain_commandlist-}"; then echo "$__git_porcelain_commandlist" return fi -- 1.6.1.2.390.gba743 ``` ## Shawn O. Pearce, 2009-02-11 16:22 Subject: Re: [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls. Message-ID: <20090211162235.GE30949@spearce.org> URL: https://gitlist.dev/e/20090211162235.GE30949%40spearce.org In-Reply-To: <1234366634-17900-2-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic wrote: > Signed-off-by: Ted Pavlic > --- Acked-by: Shawn O. Pearce > contrib/completion/git-completion.bash | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index f44f63c..6bbe09a 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -80,7 +80,7 @@ __gitdir () > # returns text to add to bash PS1 prompt (includes branch name) > __git_ps1 () > { > - local g="$(git rev-parse --git-dir 2>/dev/null)" > + local g="$(__gitdir)" > if [ -n "$g" ]; then > local r > local b > @@ -1797,7 +1797,7 @@ _gitk () > __git_has_doubledash && return > > local cur="${COMP_WORDS[COMP_CWORD]}" > - local g="$(git rev-parse --git-dir 2>/dev/null)" > + local g="$(__gitdir)" > local merge="" > if [ -f $g/MERGE_HEAD ]; then > merge="--merge" > -- > 1.6.1.2.390.gba743 > -- Shawn. ``` ## Shawn O. Pearce, 2009-02-11 16:24 Subject: Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Message-ID: <20090211162432.GF30949@spearce.org> URL: https://gitlist.dev/e/20090211162432.GF30949%40spearce.org In-Reply-To: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic wrote: > In the single case of: > > if [...] || [...] > > changed to: > > if test ... || test ... NAK. This script only runs in bash. bash supports [...]. The prevailing convention in the script is to use [...]. Only 4 tests inside of __git_ps1 use "test", the rest of the code is using [...]. I would agree to a test->[...] conversion patch as its fairly small, but not this one. Too large, too much code churn, no benefit. -- Shawn. ``` ## Shawn O. Pearce, 2009-02-11 16:26 Subject: Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <20090211162612.GG30949@spearce.org> URL: https://gitlist.dev/e/20090211162612.GG30949%40spearce.org In-Reply-To: <1234366634-17900-4-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic wrote: > The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should > be placed after the branch name. The "git diff" fails with an ugly error > if the user has just changed directory into GIT_DIR. > > This patch uses "git rev-parse --is-inside-work-tree" to determine > whether a "--work-tree=.." should be added to the "git diff". I think it makes more sense to just drop the work tree stuff from the prompt if we aren't inside the work tree anymore. Meaning, we should behave as though bash.showDirtyState is false. > Signed-off-by: Ted Pavlic > --- > contrib/completion/git-completion.bash | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 6772be7..ffde82a 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -115,10 +115,14 @@ __git_ps1 () > > local w > local i > + local worktreestr > > if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then > if test "$(git config --bool bash.showDirtyState)" != "false"; then > - git diff --no-ext-diff --ignore-submodules \ > + if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then > + worktreestr="--work-tree=.." > + fi > + git ${worktreestr} diff --no-ext-diff --ignore-submodules \ > --quiet --exit-code || w="*" > if git rev-parse --quiet --verify HEAD >/dev/null; then > git diff-index --cached --quiet \ > -- > 1.6.1.2.390.gba743 > -- Shawn. ``` ## Shawn O. Pearce, 2009-02-11 16:28 Subject: Re: [PATCH 4/3] completion: More fixes to prevent unbound variable errors. Message-ID: <20090211162814.GH30949@spearce.org> URL: https://gitlist.dev/e/20090211162814.GH30949%40spearce.org In-Reply-To: <1234369132-22063-1-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic wrote: > Several functions make use of "test -n" and "test -z". In many cases, > the variables being tested were declared with "local." However, several > __variables are not, and so they must be replaced with their ${__-} > equivalents. > > Signed-off-by: Ted Pavlic > --- > > This patch depends on: > > <1234366634-17900-3-git-send-email-ted@tedpavlic.com> > "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to > match git convention" > > If that patch is not applied, I can submit a new patch. Looks OK to me, but I NAK'd the dependency, so you'll have to rebase it without the dependency in there. Or talk me into why that much churn is a good thing. > contrib/completion/git-completion.bash | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index ffde82a..055e4ac 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -62,7 +62,7 @@ esac > __gitdir () > { > if test -z "${1-}"; then > - if test -n "$__git_dir"; then > + if test -n "${__git_dir-}"; then > echo "$__git_dir" > elif test -d .git; then > echo .git > @@ -294,7 +294,7 @@ __git_remotes () > > __git_merge_strategies () > { > - if test -n "$__git_merge_strategylist"; then > + if test -n "${__git_merge_strategylist-}"; then > echo "$__git_merge_strategylist" > return > fi > @@ -380,7 +380,7 @@ __git_complete_revlist () > > __git_all_commands () > { > - if test -n "$__git_all_commandlist"; then > + if test -n "${__git_all_commandlist-}"; then > echo "$__git_all_commandlist" > return > fi > @@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" > > __git_porcelain_commands () > { > - if test -n "$__git_porcelain_commandlist"; then > + if test -n "${__git_porcelain_commandlist-}"; then > echo "$__git_porcelain_commandlist" > return > fi > -- > 1.6.1.2.390.gba743 > -- Shawn. ``` ## Ted Pavlic, 2009-02-11 16:36 Subject: Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Message-ID: <4992FE78.6000206@tedpavlic.com> URL: https://gitlist.dev/e/4992FE78.6000206%40tedpavlic.com In-Reply-To: <20090211162432.GF30949@spearce.org> ``` > NAK. > > This script only runs in bash. bash supports [...]. The > prevailing convention in the script is to use [...]. Only > 4 tests inside of __git_ps1 use "test", the rest of the code > is using [...]. So this trumps Documentation/CodingGuidelines, which says: - We prefer "test" over "[ ... ]". ? Thanks -- Ted -- Ted Pavlic Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ``` ## Junio C Hamano, 2009-02-11 16:46 Subject: Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Message-ID: <7vwsbwgbw6.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsbwgbw6.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic writes: > In the single case of: > > if [...] || [...] > > changed to: > > if test ... || test ... In [0/3] you talked about "git convention", but please match the local convention, especially inside contrib/ area. That is, consistency of the style within the same file (and files in vicinity), is more important. > Also changed: > > if test ... > then > > to: > > if test ...; then The prevailing style in bash completion script is to write "then" on the same line as "if", so I think this is a good example of matching the local convention (if you are trying to match "git convention", "then" is written on the same column as "if" on a line by itself for readability, so this change is going backwards). ``` ## Ted Pavlic, 2009-02-11 16:53 Subject: Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <4993028C.4010204@tedpavlic.com> URL: https://gitlist.dev/e/4993028C.4010204%40tedpavlic.com In-Reply-To: <20090211162612.GG30949@spearce.org> ``` >> This patch uses "git rev-parse --is-inside-work-tree" to determine >> whether a "--work-tree=.." should be added to the "git diff". > > I think it makes more sense to just drop the work tree stuff from > the prompt if we aren't inside the work tree anymore. Meaning, > we should behave as though bash.showDirtyState is false. I see. At first, it seemed like it would be useful to know if the working directory was dirty even when you're within .git. However, I guess that's problematic when your working tree is in some completely unpredictable location. It's probably a bad idea to assume that work-tree=.. So you're right... there's a logical problem with having showDirtyState turned on when within .git. It should be disabled there. So I'll check for "git rev-parse --is-inside-git-dir" and disable showDirtyState appropriately. --Ted -- Ted Pavlic Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ``` ## Junio C Hamano, 2009-02-11 16:56 Subject: Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <7vprhogbfa.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vprhogbfa.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1234366634-17900-4-git-send-email-ted@tedpavlic.com> ``` Ted Pavlic writes: > The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should > be placed after the branch name. The "git diff" fails with an ugly error > if the user has just changed directory into GIT_DIR. > > This patch uses "git rev-parse --is-inside-work-tree" to determine > whether a "--work-tree=.." should be added to the "git diff". Why ".."? What prevents you from "cd .git/refs/heads"? Your "is-inside-work-tree" might be a good change, but if you were to spend a letter to notify the users, "Warning: You are inside GIT_DIR! This is something unusual, proceed with caution." is a lot more important notice to give them than "You seem to have unstaged changes" notice. You have at least three possible states: * You are not in git repository at all; * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a repository with a work tree. * You are inside a work tree. The first should be quiet, the second should say "Proceed with caution, any 'rm -f file' or 'edit file' you do here should be to recover from unusual repository corruptoin only; you are welcome to look but don't touch.", and the last one is Ok to say "You have unstaged changes." ``` ## Junio C Hamano, 2009-02-11 17:14 Subject: Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Message-ID: <7vljscgak4.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljscgak4.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4992FE78.6000206@tedpavlic.com> ``` Ted Pavlic writes: >> NAK. >> >> This script only runs in bash. bash supports [...]. The >> prevailing convention in the script is to use [...]. Only >> 4 tests inside of __git_ps1 use "test", the rest of the code >> is using [...]. > > So this trumps Documentation/CodingGuidelines, which says: > > - We prefer "test" over "[ ... ]". > > ? No, this paragraph from Documentation/CodingGuidelines trumps you. As for more concrete guidelines, just imitate the existing code (this is a good guideline, no matter which project you are contributing to). It is always preferable to match the _local_ convention. New code added to git suite is expected to match the overall style of existing code. Modifications to existing code is expected to match the style the surrounding code already uses (even if it doesn't match the overall style of existing code). ``` ## Ted Pavlic, 2009-02-11 17:20 Subject: Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <499308DA.9090208@tedpavlic.com> URL: https://gitlist.dev/e/499308DA.9090208%40tedpavlic.com In-Reply-To: <7vprhogbfa.fsf@gitster.siamese.dyndns.org> ``` >> This patch uses "git rev-parse --is-inside-work-tree" to determine >> whether a "--work-tree=.." should be added to the "git diff". > > Why ".."? What prevents you from "cd .git/refs/heads"? It was my impression that --work-tree=.. was relative to the GIT_DIR. My tests seem to confirm that. Within the git work tree I do: echo "test">>COPYING then cd .git/refs/heads git --work=tree=.. diff and that produces a diff of COPYING showing the new line. ("man git" confused me about how WORK_TREE was supposed to work) > Your "is-inside-work-tree" might be a good change, but if you were to > spend a letter to notify the users, "Warning: You are inside GIT_DIR! This > is something unusual, proceed with caution." is a lot more important > notice to give them than "You seem to have unstaged changes" notice. Ok, so if "is-inside-git-dir" then send a special "!" flag (and no dirtyState flags)? Does that seem reasonable? Additionally, is it a good idea to echo the branch name when inside the git dir? That is, what does it "mean" to be on "master" when you're inside .git? > You have at least three possible states: > * You are not in git repository at all; > * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a > repository with a work tree. > * You are inside a work tree. It seems like (psuedocode)... if git rev-parse --is-inside-git-dir; then use '!' flag to indicate caution elif git rev-parse --is-inside-work-tree; then proceed as before (with '*' and '+' flags) else do nothing I think that handles those cases. No? Thanks -- --Ted -- Ted Pavlic Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ``` ## Shawn O. Pearce, 2009-02-11 18:01 Subject: Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Message-ID: <20090211180100.GJ30949@spearce.org> URL: https://gitlist.dev/e/20090211180100.GJ30949%40spearce.org In-Reply-To: <499308DA.9090208@tedpavlic.com> ``` Ted Pavlic wrote: > >> You have at least three possible states: >> * You are not in git repository at all; >> * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a >> repository with a work tree. >> * You are inside a work tree. > > It seems like (psuedocode)... > > if git rev-parse --is-inside-git-dir; then > use '!' flag to indicate caution > > elif git rev-parse --is-inside-work-tree; then > proceed as before (with '*' and '+' flags) > > else > do nothing > > I think that handles those cases. No? Yes, that looks right to me. -- Shawn. ```