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

[Jeff King <peff@peff.net>]

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! :)

> 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