threads / patch / 46050

patchdoc: filter-branch does not require re-export of vars

Subject: [PATCH] doc: filter-branch does not require re-export of vars

## tl;dr

7 messages between May 26, 2017 and May 29, 2017. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Andreas Heiduk· May 26, 2017, 17:36 UTC · lore

The function `set_ident` in `filter-branch` exported the variables GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007. Therefore the filter scripts don't need to re-eport them again.

Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
---
 Documentation/git-filter-branch.txt | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Show changes to Documentation/git-filter-branch.txt +1 −4
diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index 6e4bb0220..7b695dbb7 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -86,8 +86,7 @@ OPTIONS
 	This filter may be used if you only need to modify the environment
 	in which the commit will be performed.  Specifically, you might
 	want to rewrite the author/committer name/email/time environment
-	variables (see linkgit:git-commit-tree[1] for details).  Do not forget
-	to re-export the variables.
+	variables (see linkgit:git-commit-tree[1] for details).
 
 --tree-filter <command>::
 	This is the filter for rewriting the tree and its contents.
@@ -340,12 +339,10 @@ git filter-branch --env-filter '
 	if test "$GIT_AUTHOR_EMAIL" = "root@localhost"
 	then
 		GIT_AUTHOR_EMAIL=john@example.com
-		export GIT_AUTHOR_EMAIL
 	fi
 	if test "$GIT_COMMITTER_EMAIL" = "root@localhost"
 	then
 		GIT_COMMITTER_EMAIL=john@example.com
-		export GIT_COMMITTER_EMAIL
 	fi
 ' -- --all
 --------------------------------------------------------
-- 
2.13.0
Jeff King· May 26, 2017, 18:37 UTC · re: Andreas Heiduk · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
> The function `set_ident` in `filter-branch` exported the variables
> GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
> Therefore the filter scripts don't need to re-eport them again.

Some old shells keep separate values for the internal and exporter versions of variables. I.e., this:

  foo=one
  export foo
  foo=two

would continue to export $foo as "one", even though it is "two" inside the script.

However, I think POSIX mandates the behavior you'd expect. And the only shell I know that misbehaves in this way is Solaris /bin/sh, which we have already declared too broken to support. According to

  https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export

it sounds like there are some other antique shells which may do the same (it doesn't cover this case explicitly, but the "coexist" cases it mentions are likely to behave in this way).

At this point, I'd be inclined to remove the text as you suggest and either make a small note at the bottom of the page, or just omit it entirely and assume that anybody on an old non-POSIX shell can fend for themselves.

-Peff
Junio C Hamano· May 28, 2017, 23:52 UTC · re: Jeff King · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
>
>> The function `set_ident` in `filter-branch` exported the variables
>> GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
>> Therefore the filter scripts don't need to re-eport them again.
>
> Some old shells keep separate values for the internal and exporter
> versions of variables. I.e., this:
>
>   foo=one
>   export foo
>   foo=two
>
> would continue to export $foo as "one", even though it is "two" inside
> the script.

Yup, that sounds like a grandfather's war story at this point, though ;-)

Show 6 quoted lines
> However, I think POSIX mandates the behavior you'd expect. ...
> ...
> At this point, I'd be inclined to remove the text as you suggest and
> either make a small note at the bottom of the page, or just omit it
> entirely and assume that anybody on an old non-POSIX shell can fend for
> themselves.
Sounds good to me.  Thanks.
Samuel Lijin· May 29, 2017, 01:35 UTC · re: Jeff King · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

On Fri, May 26, 2017 at 2:37 PM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
>
> On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
>
> > The function `set_ident` in `filter-branch` exported the variables
> > GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
> > Therefore the filter scripts don't need to re-eport them again.
>
> Some old shells keep separate values for the internal and exporter
> versions of variables. I.e., this:
>
>   foo=one
>   export foo
>   foo=two
>
> would continue to export $foo as "one", even though it is "two" inside
> the script.
>
> However, I think POSIX mandates the behavior you'd expect. And the only
> shell I know that misbehaves in this way is Solaris /bin/sh, which we
> have already declared too broken to support.
Off-topic, but where is this explicitly documented?
Show 14 quoted lines
> According to
>
>   https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export
>
> it sounds like there are some other antique shells which may do the
> same (it doesn't cover this case explicitly, but the "coexist" cases it
> mentions are likely to behave in this way).
>
> At this point, I'd be inclined to remove the text as you suggest and
> either make a small note at the bottom of the page, or just omit it
> entirely and assume that anybody on an old non-POSIX shell can fend for
> themselves.
>
> -Peff
Jeff King· May 29, 2017, 04:20 UTC · re: Samuel Lijin · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

On Sun, May 28, 2017 at 09:35:30PM -0400, Samuel Lijin wrote:
Show 5 quoted lines
> > However, I think POSIX mandates the behavior you'd expect. And the only
> > shell I know that misbehaves in this way is Solaris /bin/sh, which we
> > have already declared too broken to support.
> 
> Off-topic, but where is this explicitly documented?

I couldn't find a place that mentioned it explicitly, but POSIX defines the concept as "the export attribute" of the variables. Which implies to me that the bit is tied to the variable itself, not its value.

It also says that the flag is cleared when a variable is unset, so:
  foo=one
  export foo
  sh -c 'echo $foo should be one'
  unset foo
  foo=two
  sh -c 'echo $foo is not exported'

That could potentially affect somebody writing a filter-branch snippet, but presumably if they are using "unset" they know what they're doing.

-Peff
Junio C Hamano· May 29, 2017, 04:27 UTC · re: Samuel Lijin · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

Samuel Lijin <sxlijin@gmail.com> writes:
Show 5 quoted lines
>> However, I think POSIX mandates the behavior you'd expect. And the only
>> shell I know that misbehaves in this way is Solaris /bin/sh, which we
>> have already declared too broken to support.
>
> Off-topic, but where is this explicitly documented?
One link I had readily available was
  https://public-inbox.org/git/7vei5qtnc5.fsf@alter.siamese.dyndns.org/

but there may be older discussions that were the actual process of our officially having "written its /bin/sh off as broken and unusable" if you dig further in the list archive.

Jeff King· May 29, 2017, 04:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] doc: filter-branch does not require re-export of vars

On Mon, May 29, 2017 at 01:27:45PM +0900, Junio C Hamano wrote:
Show 15 quoted lines
> Samuel Lijin <sxlijin@gmail.com> writes:
> 
> >> However, I think POSIX mandates the behavior you'd expect. And the only
> >> shell I know that misbehaves in this way is Solaris /bin/sh, which we
> >> have already declared too broken to support.
> >
> > Off-topic, but where is this explicitly documented?
> 
> One link I had readily available was
> 
>   https://public-inbox.org/git/7vei5qtnc5.fsf@alter.siamese.dyndns.org/
> 
> but there may be older discussions that were the actual process of
> our officially having "written its /bin/sh off as broken and
> unusable" if you dig further in the list archive.

Ah, I had taken Samuel's question as "where is the POSIX behavior documented". :)

The link above is a good explanation of the $() problem on Solaris. It also doesn't do ${parameter#word}. I couldn't find a specific moment of outlawing that /bin/sh, but there are several mentions of problems with it going back to the beginning. Here's one from 2007:

  http://public-inbox.org/git/7vabt9sasl.fsf@assigned-by-dhcp.cox.net/
We had already given up on it by then.

I don't think there's anything in CodingGuidelines (though t/README does call it "broken"), though it explicitly endorses $(), which rules out Solaris /bin/sh.

I think we found in the last year or two that there are some old versions of ksh that don't like our scripts (definitely ksh88, and maybe ksh93?). Some light reading for the curious:

  http://public-inbox.org/git/CALR6jEhviK9KZxR6R6xzkZ5EAO-RjWj3xYah_DOSDXhEjYsT-A@mail.gmail.com/
  http://public-inbox.org/git/CALR6jEjWjJA0X2qXsxqObqc_yxrgX87LYf8cmJ0MmJFF6PkmTQ@mail.gmail.com/
  http://public-inbox.org/git/xmqqinxt3kwq.fsf@gitster.mtv.corp.google.com/

The problems are around backslash-escaping, signal exit codes, and doubled parentheses. There are some patches, but I think we decided not to pursue it (I couldn't find that exact decision, but it's referenced later on).

-Peff

← back to recent threads