threads / patch / 14605

patchgit-filter-branch.sh: Allow running in bare repositories

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

## tl;dr

10 messages between Jul 22, 2008 and Jul 23, 2008. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Petr Baudis· Jul 22, 2008, 22:37 UTC · lore

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(-)
Show changes to 2 files +28 −16

git-filter-branch.sh, t/t7003-filter-branch.sh

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
 '
Johannes Schindelin· Jul 22, 2008, 23:27 UTC · re: Petr Baudis · lore

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

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

Petr Baudis· Jul 22, 2008, 23:36 UTC · re: Johannes Schindelin · lore

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

  Hi,
On Wed, Jul 23, 2008 at 12:27:07AM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> 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.
Show 6 quoted lines
> > +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
Junio C Hamano· Jul 23, 2008, 00:09 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@suse.cz> writes:
Show 7 quoted lines
>   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.

Petr Baudis· Jul 23, 2008, 21:55 UTC · re: Junio C Hamano · lore

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(-)
Show changes to 2 files +28 −16

git-filter-branch.sh, t/t7003-filter-branch.sh

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
 '
Johannes Schindelin· Jul 23, 2008, 21:59 UTC · re: Petr Baudis · lore

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

Hi,
On Wed, 23 Jul 2008, Petr Baudis wrote:
Show 5 quoted lines
> 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.
Show 6 quoted lines
> -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

Petr Baudis· Jul 23, 2008, 22:06 UTC · re: Johannes Schindelin · lore

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

  Hi,
On Wed, Jul 23, 2008 at 10:59:03PM +0100, Johannes Schindelin wrote:
Show 9 quoted lines
> 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. :/
Show 10 quoted lines
> > -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
Petr Baudis· Jul 23, 2008, 22:15 UTC · re: Petr Baudis · lore

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

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(-)
Show changes to 2 files +28 −16

git-filter-branch.sh, t/t7003-filter-branch.sh

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
 '
Junio C Hamano· Jul 23, 2008, 22:29 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@suse.cz> writes:
Show 10 quoted lines
> 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.

← back to recent threads