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

PATCH Superseded Petr Baudis 3 participants 10 messages Jun 15, 2016

Share & tools

Petr Baudis

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 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.

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

Cc Johannes Schindelin <[email protected]>

Signed-off-by Petr Baudis <[email protected]>

---

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

git-filter-branch.sh +20 ~~-16~~

index d04c346..dcfc6be 100755
@@ -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

t/t7003-filter-branch.sh +8 -0

index e26f726..a0ab096 100755
@@ -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

2016-06-15 22:44 UTC #1 lore raw

Hi,

Quoted 3 lines · Petr Baudis

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.

Quoted 2 lines

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.

t/t7003-filter-branch.sh test_expect_success 'result is really identical' ' · line 44

Quoted 4 lines

t/t7003-filter-branch.sh +4 -0

+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

author

2016-06-15 22:44 UTC #2 lore raw

Hi,

Quoted 8 lines · Johannes Schindelin

Quoted 2 lines · Petr Baudis
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.

Quoted 6 lines

t/t7003-filter-branch.sh test_expect_success 'result is really identical' ' · line 44
Quoted 4 lines
t/t7003-filter-branch.sh +4 -0
+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.

Signature · 3 lines

				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

2016-06-15 22:44 UTC #3 lore raw

Quoted 7 lines · Petr Baudis

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

author

2016-06-15 22:45 UTC #4 lore raw

PATCH 1/1 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 <[email protected]>

Signed-off-by Petr Baudis <[email protected]>

---

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(-)

git-filter-branch.sh +20 ~~-16~~

index d04c346..ddf7187 100755
@@ -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

t/t7003-filter-branch.sh +8 -0

index e26f726..a0ab096 100755
@@ -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

2016-06-15 22:45 UTC #5 lore raw

Quoted 10 lines · 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 <[email protected]>
Signed-off-by Petr Baudis <[email protected]>
---
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.

Johannes Schindelin

2016-06-15 22:45 UTC #6 lore raw

In reply to Petr Baudis

Hi,

Quoted 5 lines · 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 <[email protected]>

Funny, I did not get Cc:ed.

git-filter-branch.sh USAGE="[--env-filter <command>] [--tree-filter <command>] \ · line 102

Quoted 6 lines

git-filter-branch.sh +3 -3

-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

author

2016-06-15 22:45 UTC #7 lore raw

Hi,

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · 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 <[email protected]>
Funny, I did not get Cc:ed.

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

Quoted 10 lines

git-filter-branch.sh USAGE="[--env-filter <command>] [--tree-filter <command>] \ · line 102

Quoted 6 lines

git-filter-branch.sh +3 -3

-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.

Quoted 1 line

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

I kept the original indentation.

Petr "Pasky" Baudis

Petr Baudis

author

2016-06-15 22:45 UTC #8 lore raw

PATCH v3 1/1 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 <[email protected]>

Signed-off-by Petr Baudis <[email protected]>

---

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(-)

git-filter-branch.sh +20 ~~-16~~

index d04c346..507c50e 100755
@@ -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

t/t7003-filter-branch.sh +8 -0

index e26f726..a0ab096 100755
@@ -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

2016-06-15 22:45 UTC #9 lore raw

Thanks. Applied.