[PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Schindelin]

As filter-branch could not care less about submodules' actual contents,
it does not make sense to check if the checked-out submodules are
up-to-date before running filter-branch.  So do not do it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-filter-branch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 8cbce4e..066f9c3 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -108,8 +108,8 @@ OPTIONS_SPEC=
 . git-sh-setup
 
 if [ "$(is_bare_repository)" = false ]; then
-	git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD -- ||
+	git diff-files --ignore-submodules --quiet &&
+	git diff-index --ignore-submodules --cached --quiet HEAD -- ||
 	die "Cannot rewrite branch(es) with a dirty working directory."
 fi
 
-- 
1.6.1.2.582.g3fdd5

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> As filter-branch could not care less about submodules' actual contents,
> it does not make sense to check if the checked-out submodules are
> up-to-date before running filter-branch.  So do not do it.

Sorry, but I am confused.  Is that because even the tree-filter does not
use the actual work tree but works in the temporary area .git-rewrite, and
a diverged submodule cannot possibly matter (and index-filter works solely
on the index anyway)?

If so, why do we even check dirtiness of anything at all?

This is not a "wouldn't this better?" proposal patch, but a "why isn't the
patch like this?" question patch.

 git-filter-branch.sh |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git i/git-filter-branch.sh w/git-filter-branch.sh
index eb62f71..dda32e0 100755
--- i/git-filter-branch.sh
+++ w/git-filter-branch.sh
@@ -107,12 +107,6 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 OPTIONS_SPEC=
 . git-sh-setup
 
-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=
 filter_tree=

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Schindelin]

Hi,

On Wed, 4 Feb 2009, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > As filter-branch could not care less about submodules' actual 
> > contents, it does not make sense to check if the checked-out 
> > submodules are up-to-date before running filter-branch.  So do not do 
> > it.
> 
> Sorry, but I am confused.  Is that because even the tree-filter does not 
> use the actual work tree but works in the temporary area .git-rewrite, 
> and a diverged submodule cannot possibly matter (and index-filter works 
> solely on the index anyway)?
> 
> If so, why do we even check dirtiness of anything at all?

I guess it is because we could update the working directory with read-tree 
-u -m HEAD at the end.  Actually, that is exactly what we do.

Submodules do not matter much here, as nothing will be overwritten, 
really, but a dirty working directory matters.

Ciao,
Dscho

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Sixt]

Junio C Hamano schrieb:
> If so, why do we even check dirtiness of anything at all?
> 
> This is not a "wouldn't this better?" proposal patch, but a "why isn't the
> patch like this?" question patch.
> 
>  git-filter-branch.sh |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git i/git-filter-branch.sh w/git-filter-branch.sh
> index eb62f71..dda32e0 100755
> --- i/git-filter-branch.sh
> +++ w/git-filter-branch.sh
> @@ -107,12 +107,6 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
>  OPTIONS_SPEC=
>  . git-sh-setup
>  
> -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=
>  filter_tree=

Because if the repository is non-bare, then filter-branch updates the
work-tree at the end of the run; we don't want to overwrite uncommitted
work in this case.

This behavior is a relic from cg-admin-rewritehist, I think. I've never
found it useful.

-- Hannes

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Because if the repository is non-bare, then filter-branch updates the
> work-tree at the end of the run; we don't want to overwrite uncommitted
> work in this case.
>
> This behavior is a relic from cg-admin-rewritehist, I think. I've never
> found it useful.

Ok, I think "read-tree -m -u HEAD" at the end sort of makes sense, if you
filtered the branch you are currently sitting on.  Does that mean we do
not have to barf on dirtyness if we can tell if the filter-branch will not
touch the current branch at all?  Again, I am not suggesting it as an
improvement, but I am trying to see if I am talking a total nonsense.

Is the reason why you haven't found it is useful is because you never
filter the current branch?

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Schindelin]

Hi,

On Wed, 4 Feb 2009, Junio C Hamano wrote:

> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > Because if the repository is non-bare, then filter-branch updates the
> > work-tree at the end of the run; we don't want to overwrite uncommitted
> > work in this case.
> >
> > This behavior is a relic from cg-admin-rewritehist, I think. I've never
> > found it useful.
> 
> Ok, I think "read-tree -m -u HEAD" at the end sort of makes sense, if you
> filtered the branch you are currently sitting on.  Does that mean we do
> not have to barf on dirtyness if we can tell if the filter-branch will not
> touch the current branch at all?  Again, I am not suggesting it as an
> improvement, but I am trying to see if I am talking a total nonsense.

That's correct.  Checking if we would touch the current branch is too 
expensive here, that's why we don't do it.

> Is the reason why you haven't found it is useful is because you never 
> filter the current branch?

Well, _I_ certainly do, and I find it very useful.

Ciao,
Dscho

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 4 Feb 2009, Junio C Hamano wrote:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>> > Because if the repository is non-bare, then filter-branch updates the
>> > work-tree at the end of the run; we don't want to overwrite uncommitted
>> > work in this case.
>> >
>> > This behavior is a relic from cg-admin-rewritehist, I think. I've never
>> > found it useful.
>> 
>> Ok, I think "read-tree -m -u HEAD" at the end sort of makes sense, if you
>> filtered the branch you are currently sitting on.  Does that mean we do
>> not have to barf on dirtyness if we can tell if the filter-branch will not
>> touch the current branch at all?  Again, I am not suggesting it as an
>> improvement, but I am trying to see if I am talking a total nonsense.
>
> That's correct.  Checking if we would touch the current branch is too 
> expensive here, that's why we don't do it.

Ok, so these exchange suggests that the commit log message needs a bit
more explanation why the check matters before describing why submodules
should not be checked.  Something like this, perhaps?

    At the end of filter-branch in a non-bare repository, the work tree is
    updated with "read-tree -m -u HEAD", to carry the change forward in
    case the current branch was rewritten.  In order to avoid losing any
    local change during this step, filter-branch refuses to work when
    there are local changes in the work tree.

    This "read-tree -m -u HEAD" operation does not affect what commit is
    checked out in a submodule (iow, it does not touch .git/HEAD in a
    submodule checkout), and checking if there is any local change to the
    submodule is not useful.

While I think it makes sense to ignore submodules for the diff-files
check, I do not think it is correct to do so in the check to see if you
have staged changes.  If you updated what commit should be checked out in
your index, and if you run "read-tree -m -u HEAD", it can conflict the
same way as a staged change to a regular blob.  The most trivial example
would be if your filtering were to remove any submodule.  Your work tree
change wanted to modify while the branch switching is to remove and you
have a modify/remove conflict right there.  Or am I again confused?

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Schindelin]

Hi,

On Wed, 4 Feb 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 4 Feb 2009, Junio C Hamano wrote:
> >
> >> Johannes Sixt <j.sixt@viscovery.net> writes:
> >> 
> >> > Because if the repository is non-bare, then filter-branch updates the
> >> > work-tree at the end of the run; we don't want to overwrite uncommitted
> >> > work in this case.
> >> >
> >> > This behavior is a relic from cg-admin-rewritehist, I think. I've never
> >> > found it useful.
> >> 
> >> Ok, I think "read-tree -m -u HEAD" at the end sort of makes sense, if you
> >> filtered the branch you are currently sitting on.  Does that mean we do
> >> not have to barf on dirtyness if we can tell if the filter-branch will not
> >> touch the current branch at all?  Again, I am not suggesting it as an
> >> improvement, but I am trying to see if I am talking a total nonsense.
> >
> > That's correct.  Checking if we would touch the current branch is too 
> > expensive here, that's why we don't do it.
> 
> Ok, so these exchange suggests that the commit log message needs a bit
> more explanation why the check matters before describing why submodules
> should not be checked.  Something like this, perhaps?
> 
>     At the end of filter-branch in a non-bare repository, the work tree is
>     updated with "read-tree -m -u HEAD", to carry the change forward in
>     case the current branch was rewritten.  In order to avoid losing any
>     local change during this step, filter-branch refuses to work when
>     there are local changes in the work tree.
> 
>     This "read-tree -m -u HEAD" operation does not affect what commit is
>     checked out in a submodule (iow, it does not touch .git/HEAD in a
>     submodule checkout), and checking if there is any local change to the
>     submodule is not useful.

Great!

> While I think it makes sense to ignore submodules for the diff-files
> check, I do not think it is correct to do so in the check to see if you
> have staged changes.  If you updated what commit should be checked out in
> your index, and if you run "read-tree -m -u HEAD", it can conflict the
> same way as a staged change to a regular blob.  The most trivial example
> would be if your filtering were to remove any submodule.  Your work tree
> change wanted to modify while the branch switching is to remove and you
> have a modify/remove conflict right there.  Or am I again confused?

I removed that change, and added a few words why it makes sense to check 
that.

Thanks,
Dscho

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Sixt]

Junio C Hamano schrieb:
> Is the reason why you haven't found it is useful is because you never
> filter the current branch?

Sort of. When I did some serious branch filtering in the past, I always
did it in a bare repository.

-- Hannes

[PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Johannes Schindelin]

At the end of filter-branch in a non-bare repository, the work tree is
updated with "read-tree -m -u HEAD", to carry the change forward in case
the current branch was rewritten.  In order to avoid losing any local
change during this step, filter-branch refuses to work when there are
local changes in the work tree.

This "read-tree -m -u HEAD" operation does not affect what commit is
checked out in a submodule (iow, it does not touch .git/HEAD in a
submodule checkout), and checking if there is any local change to the
submodule is not useful.

Staged submodules _are_ considered to be 'dirty', however,  as the
"read-tree -m -u HEAD" could result in loss of staged information
otherwise.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-filter-branch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index b75d0ba..9ffa655 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -108,8 +108,8 @@ OPTIONS_SPEC=
 . git-sh-setup
 
 if [ "$(is_bare_repository)" = false ]; then
-	git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD -- ||
+	git diff-files --ignore-submodules --quiet &&
+	git diff-index --ignore-submodules --cached --quiet HEAD -- ||
 	die "Cannot rewrite branch(es) with a dirty working directory."
 fi
 
-- 
1.6.1.1.598.g140d5

Re: [PATCH] filter-branch: do not consider diverging submodules a 'dirty worktree'

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> At the end of filter-branch in a non-bare repository, the work tree is
> updated with "read-tree -m -u HEAD", to carry the change forward in case
> the current branch was rewritten.  In order to avoid losing any local
> change during this step, filter-branch refuses to work when there are
> local changes in the work tree.
>
> This "read-tree -m -u HEAD" operation does not affect what commit is
> checked out in a submodule (iow, it does not touch .git/HEAD in a
> submodule checkout), and checking if there is any local change to the
> submodule is not useful.
>
> Staged submodules _are_ considered to be 'dirty', however,  as the
> "read-tree -m -u HEAD" could result in loss of staged information
> otherwise.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks.  I still have one question.

By the last paragraph, do you mean "diff-index --cached" should be run
without --ignore-submodules?  That does not seem to match the text of the
patch.

> ---
>  git-filter-branch.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index b75d0ba..9ffa655 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -108,8 +108,8 @@ OPTIONS_SPEC=
>  . git-sh-setup
>  
>  if [ "$(is_bare_repository)" = false ]; then
> -	git diff-files --quiet &&
> -	git diff-index --cached --quiet HEAD -- ||
> +	git diff-files --ignore-submodules --quiet &&
> +	git diff-index --ignore-submodules --cached --quiet HEAD -- ||
>  	die "Cannot rewrite branch(es) with a dirty working directory."
>  fi
>  
> -- 
> 1.6.1.1.598.g140d5