[PATCH] git-filter-branch.sh: Allow running in bare repositories

[PATCH] git-filter-branch.sh: Allow running in bare repositories

[Petr Baudis]

Commit 46eb449c restricted git-filter-branch to non-bare repositories
unnecessarily; git-filter-branch can work on bare repositories just
fine.

This also fixes suspicious shell boolean expression during a check
for dirty working tree.

Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 git-filter-branch.sh     |   36 ++++++++++++++++++++----------------
 t/t7003-filter-branch.sh |    8 ++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index d04c346..dcfc6be 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -97,9 +97,11 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 OPTIONS_SPEC=
 . git-sh-setup
 
-git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD -- ||
-	die "Cannot rewrite branch(es) with a dirty working directory."
+if [ "$(is_bare_repository)" = false ]; then
+	(git diff-files --quiet &&
+	 git diff-index --cached --quiet HEAD --) ||
+		die "Cannot rewrite branch(es) with a dirty working directory."
+fi
 
 tempdir=.git-rewrite
 filter_env=
@@ -434,18 +436,20 @@ rm -rf "$tempdir"
 
 trap - 0
 
-unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
-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
-}
-git read-tree -u -m HEAD
+if [ "$(is_bare_repository)" = false ]; then
+	unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
+	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
+	}
+	git read-tree -u -m HEAD
+fi
 
 exit $ret
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index e26f726..a0ab096 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -38,6 +38,14 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'rewrite bare repository identically' '
+	(git config core.bare true && cd .git && git-filter-branch branch)
+'
+git config core.bare false
+test_expect_success 'result is really identical' '
+	test $H = $(git rev-parse HEAD)
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git-filter-branch -f --tree-filter "mv d doh || :" HEAD
 '

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Petr Baudis wrote:

> Commit 46eb449c restricted git-filter-branch to non-bare repositories
> unnecessarily; git-filter-branch can work on bare repositories just
> fine.

I think this is fine.

> This also fixes suspicious shell boolean expression during a check
> for dirty working tree.

If you are talking about X && Y || Z, it is well established (and should 
not be suspicious to a shell hacker like the creator of Cogito) that Z is 
executed if either X fails, or X succeeds and Y fails.

> +test_expect_success 'rewrite bare repository identically' '
> +	(git config core.bare true && cd .git && git-filter-branch branch)
> +'
> +git config core.bare false

Any reason why this is done outside the test?

Ciao,
Dscho

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Petr Baudis]

  Hi,

On Wed, Jul 23, 2008 at 12:27:07AM +0100, Johannes Schindelin wrote:
> On Wed, 23 Jul 2008, Petr Baudis wrote:
> 
> > This also fixes suspicious shell boolean expression during a check
> > for dirty working tree.
> 
> If you are talking about X && Y || Z, it is well established (and should 
> not be suspicious to a shell hacker like the creator of Cogito) that Z is 
> executed if either X fails, or X succeeds and Y fails.

  um, oops. I actually never got to know these by heart since I learnt
to expliciply group the expressions early on. I guess my only excuse is
that I've stumbled at 0bdf93cbf earlier and understood it the _wrong_
way around since I'm getting really sleepy. ;-)

  I still think my change improves the code readibility so it could be
kept, but I'm fairly neutral on this.

> > +test_expect_success 'rewrite bare repository identically' '
> > +	(git config core.bare true && cd .git && git-filter-branch branch)
> > +'
> > +git config core.bare false
> 
> Any reason why this is done outside the test?

  If the test fails in the middle, not resetting this might negatively
affect the rest of the testsuite.

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

>   um, oops. I actually never got to know these by heart since I learnt
> to expliciply group the expressions early on. I guess my only excuse is
> that I've stumbled at 0bdf93cbf earlier and understood it the _wrong_
> way around since I'm getting really sleepy. ;-)
>
>   I still think my change improves the code readibility so it could be
> kept, but I'm fairly neutral on this.

I cannot be neutral when a patch introduces unnecessary fork.

The quality of shell scripts in git.git seems to have deteriorated over
time but I do not think we would want to spend too much maintainer time to
go back and fix all of them.  Please don't make things worse, at least.

[PATCH] git-filter-branch.sh: Allow running in bare repositories

[Petr Baudis]

Commit 46eb449c restricted git-filter-branch to non-bare repositories
unnecessarily; git-filter-branch can work on bare repositories just
fine.

Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Petr Baudis <pasky@suse.cz>
---

  I have my own opinion about the readability-fork ratio in this particular
case, but there's no use arguing about this. ;-)

 git-filter-branch.sh     |   36 ++++++++++++++++++++----------------
 t/t7003-filter-branch.sh |    8 ++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index d04c346..ddf7187 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -97,9 +97,11 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 OPTIONS_SPEC=
 . git-sh-setup
 
-git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD -- ||
-	die "Cannot rewrite branch(es) with a dirty working directory."
+if [ "$(is_bare_repository)" = false ]; then
+	git diff-files --quiet &&
+		git diff-index --cached --quiet HEAD --) ||
+		die "Cannot rewrite branch(es) with a dirty working directory.
+fi
 
 tempdir=.git-rewrite
 filter_env=
@@ -434,18 +436,20 @@ rm -rf "$tempdir"
 
 trap - 0
 
-unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
-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
-}
-git read-tree -u -m HEAD
+if [ "$(is_bare_repository)" = false ]; then
+	unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
+	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
+	}
+	git read-tree -u -m HEAD
+fi
 
 exit $ret
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index e26f726..a0ab096 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -38,6 +38,14 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'rewrite bare repository identically' '
+	(git config core.bare true && cd .git && git-filter-branch branch)
+'
+git config core.bare false
+test_expect_success 'result is really identical' '
+	test $H = $(git rev-parse HEAD)
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git-filter-branch -f --tree-filter "mv d doh || :" HEAD
 '

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Petr Baudis wrote:

> Commit 46eb449c restricted git-filter-branch to non-bare repositories
> unnecessarily; git-filter-branch can work on bare repositories just
> fine.
> 
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Funny, I did not get Cc:ed.

> -git diff-files --quiet &&
> -	git diff-index --cached --quiet HEAD -- ||
> -	die "Cannot rewrite branch(es) with a dirty working directory."
> +if [ "$(is_bare_repository)" = false ]; then
> +	git diff-files --quiet &&
> +		git diff-index --cached --quiet HEAD --) ||

                                                       ^

I doubt this has ever passed the test suite, let alone run.

Besides, this extra-funny extra indent is quite brutal on my eyes.

Ciao,
Dscho

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Petr Baudis]

  Hi,

On Wed, Jul 23, 2008 at 10:59:03PM +0100, Johannes Schindelin wrote:
> On Wed, 23 Jul 2008, Petr Baudis wrote:
> 
> > Commit 46eb449c restricted git-filter-branch to non-bare repositories
> > unnecessarily; git-filter-branch can work on bare repositories just
> > fine.
> > 
> > Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> Funny, I did not get Cc:ed.

  sorry, I have to copy the Cc around manually with StGIT. :/

> > -git diff-files --quiet &&
> > -	git diff-index --cached --quiet HEAD -- ||
> > -	die "Cannot rewrite branch(es) with a dirty working directory."
> > +if [ "$(is_bare_repository)" = false ]; then
> > +	git diff-files --quiet &&
> > +		git diff-index --cached --quiet HEAD --) ||
> 
>                                                        ^
> 
> I doubt this has ever passed the test suite, let alone run.

  It did... provided that I didn't make after modifying the script.

> Besides, this extra-funny extra indent is quite brutal on my eyes.

  I kept the original indentation.

				Petr "Pasky" Baudis

[PATCHv3] git-filter-branch.sh: Allow running in bare repositories

[Petr Baudis]

Commit 46eb449c restricted git-filter-branch to non-bare repositories
unnecessarily; git-filter-branch can work on bare repositories just
fine.

Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Petr Baudis <pasky@suse.cz>
---

  Sorry for the spam. This time I actually did make git-filter-branch
before running the testsuite.

 git-filter-branch.sh     |   36 ++++++++++++++++++++----------------
 t/t7003-filter-branch.sh |    8 ++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index d04c346..507c50e 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -97,9 +97,11 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 OPTIONS_SPEC=
 . git-sh-setup
 
-git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD -- ||
-	die "Cannot rewrite branch(es) with a dirty working directory."
+if [ "$(is_bare_repository)" = false ]; then
+	git diff-files --quiet &&
+		git diff-index --cached --quiet HEAD -- ||
+		die "Cannot rewrite branch(es) with a dirty working directory."
+fi
 
 tempdir=.git-rewrite
 filter_env=
@@ -434,18 +436,20 @@ rm -rf "$tempdir"
 
 trap - 0
 
-unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
-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
-}
-git read-tree -u -m HEAD
+if [ "$(is_bare_repository)" = false ]; then
+	unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE
+	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
+	}
+	git read-tree -u -m HEAD
+fi
 
 exit $ret
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index e26f726..a0ab096 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -38,6 +38,14 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'rewrite bare repository identically' '
+	(git config core.bare true && cd .git && git-filter-branch branch)
+'
+git config core.bare false
+test_expect_success 'result is really identical' '
+	test $H = $(git rev-parse HEAD)
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git-filter-branch -f --tree-filter "mv d doh || :" HEAD
 '

Re: [PATCHv3] git-filter-branch.sh: Allow running in bare repositories

[Junio C Hamano]

Thanks.  Applied.

Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Commit 46eb449c restricted git-filter-branch to non-bare repositories
> unnecessarily; git-filter-branch can work on bare repositories just
> fine.
>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Petr Baudis <pasky@suse.cz>
> ---
>
>   I have my own opinion about the readability-fork ratio in this particular
> case, but there's no use arguing about this. ;-)

Grouping commands is perfectly fine when you think the readers may find it
easlier to follow the logic if you grouped them.  Use { } for that kind of
grouping; I do not have any problem with that.

Use of subshell ( ) is often done by inexperienced people or by careless
people without thinking.  Sometimes you would explicitly want to have a
subshell (e.g. when you want to chdir to do something there but do not
want to affect the main program), and sometimes you don't (e.g. you are
grouping just for precedence, and want assignments and side effects done
inside the group visible by the main program).

Careless uses of ( ) wastes reviewer's time because the code inside has to
be studied to find out if the writer really wanted to have an isolated
separate process that subshell gives, or just being plain careless.