Re: [PATCH 4/4] filter-branch: fix variable export logic

From: Jeff King <peff@peff.net>
Date: May 14, 2008, 04:57 UTC
Message-ID: <20080514045717.GA16592@sigill.intra.peff.net>
In-Reply-To: <7v7idxr00i.fsf@gitster.siamese.dyndns.org>

On Tue, May 13, 2008 at 09:18:53PM -0700, Junio C Hamano wrote:

> I was confused by this description ("short-circuit logic"), but I do not
> think there is no short-circuit going on.  This is a simple ignorance of
> shell syntax.

Sorry, I meant "you might think we are not going to run this because of short circuit, but we always will." So I think the original author was confused.

But please feel free to reword in a way that makes more sense.

> I have a mild suspecion that this was simply an artifcat of a careless
> conversion from export var="$val" form we did in the past.  I should have
> caught them back then.

Nope, it was originally that way (46eb449). Thank goodness for git-blame! :)

Show 13 quoted lines

> The patch is fine, but I find this easier to read:
> 
> +test -z "$ORIG_GIT_DIR" || {
> +	GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR
> +}
> +test -z "$ORIG_GIT_WORK_TREE" || {
> +	GIT_WORK_TREE="$ORIG_GIT_WORK_TREE" &&
> +	export GIT_WORK_TREE
> +}
> +test -z "$ORIG_GIT_INDEX_FILE" || {
> +	GIT_INDEX_FILE="$ORIG_GIT_INDEX_FILE" &&
> +	export GIT_INDEX_FILE
> +}

Yes, that is easier to read. Although what also confused me at first was the double negation. It is trying to say "if the original existed, restore it", but it is written as "the original has no content, OR restore it". So

  if test -n "$ORIG_GIT_DIR"
  then
    ...
  fi

would be even clearer, though I'm not sure if "-n" has any portability concerns.

-Peff