[PATCH] filter-branch: use printf instead of echo -e

[PATCH] filter-branch: use printf instead of echo -e

[Michele Locati]

In order to echo a tab character, it's better to use printf instead of
"echo -e", because it's more portable (for instance, "echo -e" doesn't work
as expected on a Mac).

This solves the "fatal: Not a valid object name" error in git-filter-branch
when using the --state-branch option.

Signed-off-by: Michele Locati <michele@locati.it>
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..21d84eff3 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -627,7 +627,7 @@ then
 				print H "$_:$f\n" or die;
 			}
 			close(H) or die;' || die "Unable to save state")
-	state_tree=$(/bin/echo -e "100644 blob $state_blob\tfilter.map" | git mktree)
+	state_tree=$(printf '100644 blob %s\tfilter.map\n' "$state_blob" | git mktree)
 	if test -n "$state_commit"
 	then
 		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
-- 
2.16.2.windows.1

Re: [PATCH] filter-branch: use printf instead of echo -e

[CB Bailey]

On Mon, Mar 19, 2018 at 03:49:05PM +0100, Michele Locati wrote:
> In order to echo a tab character, it's better to use printf instead of
> "echo -e", because it's more portable (for instance, "echo -e" doesn't work
> as expected on a Mac).
> 
> This solves the "fatal: Not a valid object name" error in git-filter-branch
> when using the --state-branch option.
> 
> Signed-off-by: Michele Locati <michele@locati.it>
> ---
>  git-filter-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 1b7e4b2cd..21d84eff3 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -627,7 +627,7 @@ then
>  				print H "$_:$f\n" or die;
>  			}
>  			close(H) or die;' || die "Unable to save state")
> -	state_tree=$(/bin/echo -e "100644 blob $state_blob\tfilter.map" | git mktree)
> +	state_tree=$(printf '100644 blob %s\tfilter.map\n' "$state_blob" | git mktree)
>  	if test -n "$state_commit"
>  	then
>  		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")

I think the change from 'echo -e' to printf is good because of the
better portability reason that you cite.

Looking at the change, I am now curious as to why '/bin/echo' is used.
Testing on a Mac, bash's built in 'echo' recognizes '-e' whereas
'/bin/echo' does not. This is just an observation, I still prefer the
move to 'printf' that you suggest.

There are two further uses of '/bin/echo' in git-filter-branch.sh which
are portable (no "-e", just printing a word that cannot be confused for
an option). One is visible in your diff context and the other is just
below it. For consistency with other echos in git-filter-branch.sh, I
think that these should probably use simple 'echo' rather than
'/bin/echo' to use a builtin where available.

CB

[PATCH v2] filter-branch: use printf instead of echo -e

[Michele Locati]

In order to echo a tab character, it's better to use printf instead of
"echo -e", because it's more portable (for instance, "echo -e" doesn't work
as expected on a Mac).

This solves the "fatal: Not a valid object name" error in git-filter-branch
when using the --state-branch option.

Furthermore, let's switch from "/bin/echo" to just "echo", so that the
built-in echo command is used where available.

Signed-off-by: Michele Locati <michele@locati.it>
---
 git-filter-branch.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..98c76ec58 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -627,12 +627,12 @@ then
 				print H "$_:$f\n" or die;
 			}
 			close(H) or die;' || die "Unable to save state")
-	state_tree=$(/bin/echo -e "100644 blob $state_blob\tfilter.map" | git mktree)
+	state_tree=$(printf '100644 blob %s\tfilter.map\n' "$state_blob" | git mktree)
 	if test -n "$state_commit"
 	then
-		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
+		state_commit=$(echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
 	else
-		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" )
+		state_commit=$(echo "Sync" | git commit-tree "$state_tree" )
 	fi
 	git update-ref "$state_branch" "$state_commit"
 fi
-- 
2.16.2.windows.1

Re: [PATCH v2] filter-branch: use printf instead of echo -e

[Junio C Hamano]

Michele Locati <michele@locati.it> writes:

> In order to echo a tab character, it's better to use printf instead of
> "echo -e", because it's more portable (for instance, "echo -e" doesn't work
> as expected on a Mac).
>
> This solves the "fatal: Not a valid object name" error in git-filter-branch
> when using the --state-branch option.
>
> Furthermore, let's switch from "/bin/echo" to just "echo", so that the
> built-in echo command is used where available.
>
> Signed-off-by: Michele Locati <michele@locati.it>
> ---

Thanks; will queue.

>  git-filter-branch.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 1b7e4b2cd..98c76ec58 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -627,12 +627,12 @@ then
>  				print H "$_:$f\n" or die;
>  			}
>  			close(H) or die;' || die "Unable to save state")
> -	state_tree=$(/bin/echo -e "100644 blob $state_blob\tfilter.map" | git mktree)
> +	state_tree=$(printf '100644 blob %s\tfilter.map\n' "$state_blob" | git mktree)
>  	if test -n "$state_commit"
>  	then
> -		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
> +		state_commit=$(echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
>  	else
> -		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" )
> +		state_commit=$(echo "Sync" | git commit-tree "$state_tree" )
>  	fi
>  	git update-ref "$state_branch" "$state_commit"
>  fi

Re: [PATCH] filter-branch: use printf instead of echo -e

[Jeff King]

On Mon, Mar 19, 2018 at 03:39:46PM +0000, CB Bailey wrote:

> > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > index 1b7e4b2cd..21d84eff3 100755
> > --- a/git-filter-branch.sh
> > +++ b/git-filter-branch.sh
> > @@ -627,7 +627,7 @@ then
> >  				print H "$_:$f\n" or die;
> >  			}
> >  			close(H) or die;' || die "Unable to save state")
> > -	state_tree=$(/bin/echo -e "100644 blob $state_blob\tfilter.map" | git mktree)
> > +	state_tree=$(printf '100644 blob %s\tfilter.map\n' "$state_blob" | git mktree)
> >  	if test -n "$state_commit"
> >  	then
> >  		state_commit=$(/bin/echo "Sync" | git commit-tree "$state_tree" -p "$state_commit")
> 
> I think the change from 'echo -e' to printf is good because of the
> better portability reason that you cite.
> 
> Looking at the change, I am now curious as to why '/bin/echo' is used.
> Testing on a Mac, bash's built in 'echo' recognizes '-e' whereas
> '/bin/echo' does not. This is just an observation, I still prefer the
> move to 'printf' that you suggest.

Right. Moving them to just "echo -e" would work on systems where /bin/sh
is bash, but not elsewhere (e.g., Debian systems with "dash" whose
built-in echo doesn't understand "-e").

So my guess as to why /bin/echo was used is that on Linux systems it's
_more_ predictable and portable, because you know you're always going to
get the GNU coreutils version, which knows "-e". Even if you're using a
non-bash shell.

But on non-Linux systems, who knows what system "echo" you'll get. :)

Author cc'd in case there's something more interesting going on.

-Peff

Re: [PATCH] filter-branch: use printf instead of echo -e

[Ian Campbell]

On Tue, 2018-03-20 at 00:22 -0400, Jeff King wrote:
> Author cc'd in case there's something more interesting going on.

That code was written years ago, if I had a good reason at the time
I've forgotten what it was and I can't think of a fresh one now.
Switching to printf seems like a reasonable thing to do.

Perhaps switch the remaining `/bin/echo` (there are two without `-e`)
uses to just `echo` for consistency with the rest of the file?

Ian.

Re: [PATCH] filter-branch: use printf instead of echo -e

[Michele Locati]

Il 20/03/2018 10:53, Ian Campbell ha scritto:
> On Tue, 2018-03-20 at 00:22 -0400, Jeff King wrote:
>> Author cc'd in case there's something more interesting going on.
> 
> That code was written years ago, if I had a good reason at the time
> I've forgotten what it was and I can't think of a fresh one now.
> Switching to printf seems like a reasonable thing to do.
> 
> Perhaps switch the remaining `/bin/echo` (there are two without `-e`)
> uses to just `echo` for consistency with the rest of the file?
> 
> Ian.
> 

Thanks for reply, Ian.
Charles already suggested to replace /bin/echo with echo, and I already 
updated the patch accordingly: see
https://marc.info/?l=git&m=152147479517707&w=2
Junio already queued that PATCH-v2.

--
Michele

Re: [PATCH v2] filter-branch: use printf instead of echo -e

[Johannes Schindelin]

Hi Michele,

On Mon, 19 Mar 2018, Michele Locati wrote:

> [...]
> -- 
> 2.16.2.windows.1

Yaaaaay!

Out of curiosity: did the CONTRIBUTING.md file help that was recently
turned into a guide how to contribute to Git (for Windows) by Derrick
Stolee?

Ciao,
Johannes

Re: [PATCH v2] filter-branch: use printf instead of echo -e

[Michele Locati]

Il 21/03/2018 22:50, Johannes Schindelin ha scritto:
> Hi Michele,
> 
> On Mon, 19 Mar 2018, Michele Locati wrote:
> 
>> [...]
>> -- 
>> 2.16.2.windows.1
> 
> Yaaaaay!
> 
> Out of curiosity: did the CONTRIBUTING.md file help that was recently
> turned into a guide how to contribute to Git (for Windows) by Derrick
> Stolee?

Well, no. Here's what led me here...

The people behind the concrete5 CMS asked support for improving the 
following procedure:

concrete5 is stored in
https://github.com/concrete5/concrete5
In order to make it installable with Composer (a PHP package manager), 
we need to extract its "/concrete" directory, and push the results to 
https://github.com/concrete5/concrete5-core
We previously used "git filter-branch --all" with this script:
https://github.com/concrete5/core_splitter/blob/70879e676b95160f7fc5d0ffc22b8f7420b0580b/bin/splitcore

That script is executed every time someone pushes concrete5/concrete5, 
and it took very long time (3~5 minutes).

I noticed on the git-filter-branch manual page that there's a 
"--state-branch", and it seemed quite interesting.
So, I asked in git@vger.kernel.org how to use it, and the author of that 
feature (Ian Campbell) told me to look at some code he wrote.
So, I wrote
https://github.com/concrete5/incremental-filter-branch
which wraps "git filter-branch --filter-state", and it's used in
https://github.com/concrete5/core_splitter/blob/99bbbcea1ab90572a04864ccb92327532ab6a42c/bin/splitcore
(it not yet in production: Korvin wants to be sure everything works well 
before adopting it)
The time required for the operation dropped from 3~5 minutes to ~10 
seconds ;)

While writing that script, I noticed a couple of things that could be 
improved in "git-filter-branch", so I submitted
https://marc.info/?l=git&m=152111905428554&w=2
(so that the script could run without problems if there's nothing to do)
and
https://marc.info/?l=git&m=152147095416175&w=2
(so that  --filter-state could be used on Mac and other non-Linux systems).
How did I learn how to submit to git? I searched for "git src", landed 
on https://github.com/git/git, read Documentation/SubmittingPatches and 
used git send-email.

And yes, all that on Windows (and WSL).


> 
> Ciao,
> Johannes

Ciao!

--
Michele

Re: [PATCH v2] filter-branch: use printf instead of echo -e

[Johannes Schindelin]

Hi Michele,

On Thu, 22 Mar 2018, Michele Locati wrote:

> Il 21/03/2018 22:50, Johannes Schindelin ha scritto:
> > 
> > On Mon, 19 Mar 2018, Michele Locati wrote:
> > 
> > > [...]
> > > -- 
> > > 2.16.2.windows.1
> > 
> > Yaaaaay!
> > 
> > Out of curiosity: did the CONTRIBUTING.md file help that was recently
> > turned into a guide how to contribute to Git (for Windows) by Derrick
> > Stolee?
> 
> Well, no. Here's what led me here...
> 
> [...]

Thank you for taking the time to tell the story!

Ciao,
Johannes