Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Quoting Eric Sunshine <sunshine@xxxxxxxxxxxxxx>:

On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@xxxxxxxx> wrote:
On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
      while :
      do
              case "$1" in
              -C) dir="-C $2"; shift; shift ;;
              -b) bare="$2"; shift; shift ;;
+             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
@@ -36,6 +38,8 @@ test_rev_parse () {
      do
              expect="$1"
              test_expect_success "$name: $o" '
+                     test_when_finished "sane_unset GIT_DIR" &&
+                     eval $env &&

This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.

I didn't worry about it too much because the end result is effectively
the same and, with all the 'case' arms being short one-liners, I think
the code is a bit easier to read as-is; bundling 'test_when_finished'
into the 'env' assignment line would probably require wrapping the
line. I do like the improved encapsulation of your suggestion but
don't otherwise feel strongly about it.

Actually, I think we can have improved encapsulation and maintain
readability like this:

    case "$1" in
    ...
    -g) env="$2"; shift;  shift ;;
    ...
    esac

    ...
    test_expect_success "..." '
        if test -n "$env"
        do
            test_when_finished "unset GIT_DIR"
            GIT_DIR="$env"
            export GIT_DIR
        fi
        ...
    '

I wonder if is it really necessary to specify the path to the .git
directory via $GIT_DIR.  Would 'git --git-dir=/over/there' be just as
good?  If yes, then how about

  git ${gitdir:+--git-dir="$gitdir"} rev-parse ...


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]