4
\$\begingroup\$

I'd like to get a review for the function that cd into the git tree root or does nothing if we are outside of the repository.

reviewed version:

# cd to the root of the current git directory
# If $PWD is submodule, will cd to the root of the top ancestor
# It requires to stay in the current directory, if the root is . or unknown,
# and use cd only once, to have a way to do `cd -`
function cg {
  git_root() {
    local super_root
    local top
    top="$(git rev-parse --show-cdup)"
    top="${top:-./}"
    super_root="$(git rev-parse --show-superproject-working-tree)"
    if [[ "$super_root" ]]; then
      printf '%s' "$top../"
      ( cd "$top../" && git_root || return )
    fi
    printf '%s' "$top"
  }
  local git_root
  git_root="$(git_root)"
  [ "x${git_root}" != "x./" ] && cd "$(git_root)" && return || return 0
}

updated version:

#!/bin/bash
# cd to the root of the current git directory
# If $PWD is submodule, will cd to the root of the top ancestor
# It requires to stay in the current directory, if the root is . or unknown,
# and use cd only once, to have a way to do `cd -`
function cg {
  function git_root {
    local top; top="$(git rev-parse --show-cdup)"
    top="${top:-./}"
    local super_root; super_root="$(git rev-parse --show-superproject-working-tree)"
    if [[ "$super_root" ]]; then
      printf '%s' "$top../"
      ( cd "$top../" && git_root || return )
    fi
    printf '%s' "$top"
  }
  local tree_root
  tree_root="$(git_root)"
  [[ "x${tree_root}" != "x./" ]] && cd "${tree_root}" && return || return 0
}
\$\endgroup\$
4
  • \$\begingroup\$ just a suggestion, the single line cd "$(git rev-parse --show-toplevel || echo .)" does the exact thing :) Taken from oh-my-zsh plugin: github.com/ohmyzsh/ohmyzsh/blob/master/plugins/git/… \$\endgroup\$ – hjpotter92 Nov 26 '20 at 13:01
  • 2
    \$\begingroup\$ Please do not edit the question after it has been answered. See what should I not do. \$\endgroup\$ – pacmaninbw Nov 26 '20 at 13:21
  • 1
    \$\begingroup\$ @hjpotter92 no, sorry, it doesn't because it knows nothing about: a) symlinks; b) submodules. These two points forced me to invest time in the function above. \$\endgroup\$ – Felixoid Nov 26 '20 at 13:29
  • \$\begingroup\$ Thank you pacmaninbw, here's the updated version codereview.stackexchange.com/questions/252838/… \$\endgroup\$ – Felixoid Nov 30 '20 at 9:45
4
\$\begingroup\$

Great

  • 100% pass on shellcheck which means you're doing great with quoting everything that could be potentially problematic.
  • scoping variables
  • using [[ conditional for the if
  • nice explanation of what it is doing and why

Could be better

  • a sh-bang line at the top is a good idea for scripts, even if this would usually just be sourced during your login scripts.
  • Your inner function is defined with different syntax than your outer function.
  • You can scope and define a variable in one line. For example: local super_root="$(git rev-parse --show-superproject-working-tree)". This makes sure you don't fail to define a local variable or fail to scope a new variable. And it cuts out a line of code in each case.
  • Reusing git_root for a variable name and the function name is confusing. I initially wondered why you were trying to scope the function after you had just defined it.
  • use [[ conditional for conditional in the last line.
\$\endgroup\$
5
  • \$\begingroup\$ I know it's tagged bash, but I'd go the other way on your last comment, and turn the existing [[ into a [ test, so this function can more easily be adapted for other shells (I think that just leaves local as the only Bash feature used). \$\endgroup\$ – Toby Speight Nov 26 '20 at 11:18
  • \$\begingroup\$ Thank you for your review! \$\endgroup\$ – Felixoid Nov 26 '20 at 12:04
  • \$\begingroup\$ 1) sure, makes sense; 2) true, I've missread the name() ( code ) and name() { code } notations; 3) actually making one line local top=... cause complaints by shellcheck SC2155: Declare and assign separately to avoid masking return values. That's why I've decided to leave it this way; 4) yes, a little bit messy; 5) I see the point to make it POSIX-complaint, but this looks to me like a function for interactive shell, so I'll use [[ in the latest line too. \$\endgroup\$ – Felixoid Nov 26 '20 at 12:16
  • \$\begingroup\$ Thanks for reviewing the review! I like [[ for safety, but I'm not worried about portability like Toby. SC2155 applies because all your assignments involve subcommands, but you're already masking return values in a few places. \$\endgroup\$ – chicks Nov 26 '20 at 14:12
  • \$\begingroup\$ I like the cleaned up version, but to get a new review you're supposed to post a new question each time. \$\endgroup\$ – chicks Nov 26 '20 at 17:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.