[PATCH] filter-branch: assume HEAD if no revision supplied

[PATCH] filter-branch: assume HEAD if no revision supplied

[Brandon Casey]

filter-branch previously took the first non-option argument as the name for
a new branch. Since dfd05e38, it now takes a revision or a revision range
and modifies the current branch. Update to operate on HEAD by default to
conform with standard git interface practice.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


You may think that filter-branch _should_ require the user to specify the
revision. If so, then '--default HEAD' should probably be removed from the
other two places, and the usage and documentation should be updated to
remove the brackets around <rev-list options> so not to imply that it is
optional. The test for at least one non-option argument can still be
removed since it can currently be circumvented by placing -- as the
last argument and because it would allow format-patch to fail with
a much nicer message: "Which ref do you want to rewrite?".

fwiw the behavior this patch implements is what I expected.

-brandon


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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index ebf05ca..cd1eeee 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -114,7 +114,6 @@ orig_namespace=refs/original/
 force=
 while :
 do
-	test $# = 0 && usage
 	case "$1" in
 	--)
 		shift
@@ -210,7 +209,7 @@ GIT_WORK_TREE=.
 export GIT_DIR GIT_WORK_TREE
 
 # The refs should be updated if their heads were rewritten
-git rev-parse --no-flags --revs-only --symbolic-full-name "$@" |
+git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD "$@" |
 sed -e '/^^/d' >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
-- 
1.5.4.rc5.14.gaa8fc

Re: [PATCH] filter-branch: assume HEAD if no revision supplied

[Johannes Schindelin]

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

> filter-branch previously took the first non-option argument as the name 
> for a new branch. Since dfd05e38, it now takes a revision or a revision 
> range and modifies the current branch. Update to operate on HEAD by 
> default to conform with standard git interface practice.

FWIW I think the code wanted to let "git filter-branch" without options 
print the usage.

Ciao,
Dscho

Re: [PATCH] filter-branch: assume HEAD if no revision supplied

[Junio C Hamano]

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

> On Wed, 30 Jan 2008, Brandon Casey wrote:
>
>> filter-branch previously took the first non-option argument as the name 
>> for a new branch. Since dfd05e38, it now takes a revision or a revision 
>> range and modifies the current branch. Update to operate on HEAD by 
>> default to conform with standard git interface practice.
>
> FWIW I think the code wanted to let "git filter-branch" without options 
> print the usage.

That might be a valid safety concern to some folks.  Previously
we have seen people say "Whenever I see a command foo that I do
not know what it does, I type 'foo <Enter>' and expect it gives
the usage back.  So any new destructive command 'foo' should not
do a damage by using built-in default." (I think it was about
"git stash" without parameter).

By the way, I do not personally think it is worth to be heavily
supportive to the practice of trying an unknown command without
understanding, and I do not agree such a safety is necessarily a
good idea, especially if it makes normal use of the command more
cumbersome by people who understand what it does.

Even though "git stash" itself is not destrictive, you need to
know its "apply" subcommand to undo the action.  In that sense,
it is destructive to clueless people who blindly type whatever
command they see.

That's why we still allow you to say "git stash", but we removed
its "git stash <randam message>" syntax, which was risky when
subcommand name was misspelled even by people who know what the
command does.  I think we struck a good balance between
usability and safety there.  And I think we can do the same
here.

Perhaps "git filter-branch <Enter>" can be prevented as in the
current implementation while "git filter-branch --foo-filter
foo" can default to HEAD to satisfy both needs.  The command
without any filter is supposed to be mostly no-op (unless you
are trying to rewrite the history with grafts).

Re: [PATCH] filter-branch: assume HEAD if no revision supplied

[Brandon Casey]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Wed, 30 Jan 2008, Brandon Casey wrote:
>>
>>> filter-branch previously took the first non-option argument as the name 
>>> for a new branch. Since dfd05e38, it now takes a revision or a revision 
>>> range and modifies the current branch. Update to operate on HEAD by 
>>> default to conform with standard git interface practice.
>> FWIW I think the code wanted to let "git filter-branch" without options 
>> print the usage.
> 
> That might be a valid safety concern to some folks.  Previously
> we have seen people say "Whenever I see a command foo that I do
> not know what it does, I type 'foo <Enter>' and expect it gives
> the usage back.  So any new destructive command 'foo' should not
> do a damage by using built-in default." (I think it was about
> "git stash" without parameter).
> 
> By the way, I do not personally think it is worth to be heavily
> supportive to the practice of trying an unknown command without
> understanding, and I do not agree such a safety is necessarily a
> good idea, especially if it makes normal use of the command more
> cumbersome by people who understand what it does.
> 
> Even though "git stash" itself is not destrictive, you need to
> know its "apply" subcommand to undo the action.  In that sense,
> it is destructive to clueless people who blindly type whatever
> command they see.
> 
> That's why we still allow you to say "git stash", but we removed
> its "git stash <randam message>" syntax, which was risky when
> subcommand name was misspelled even by people who know what the
> command does.  I think we struck a good balance between
> usability and safety there.  And I think we can do the same
> here.
>
> Perhaps "git filter-branch <Enter>" can be prevented as in the
> current implementation while "git filter-branch --foo-filter
> foo" can default to HEAD to satisfy both needs.  The command
> without any filter is supposed to be mostly no-op (unless you
> are trying to rewrite the history with grafts).

That's what I was trying to do :)

The goal should be consistency in the user interface. New users will
always get confused. Lack of consistency could cause confusion for
experienced users. I sent a patch because it was intuitive to me for
filter-branch to operate on HEAD based on my git experience, and I was
surprised when it did not. For porcelain that take a revision argument
it seems common to default to HEAD.

I think the stash case is a little bit different because it was
actually causing problems for experienced users. A minor typo would
bite experienced people from time to time.

In that same consistency vein, I wonder if a user would be surprised
that 'git filter-branch' prints usage information but 'git filter-branch --'
operates on HEAD? Maybe the following patch would be better than the
compromise solution. (following in another email)

-brandon

PS. Please s/format-patch/filter-branch/ if I missed any. I keep
doing that.

[PATCH 1/2] filter-branch: only print usage information when no arguments supplied

[Brandon Casey]

Testing for whether command line arguments were supplied was being
performed during option parsing. This had the side effect of
printing usage information when a more appropriate error message
would have been printed had the script been allowed to continue.

Now this:

	git filter-branch

will print usage information.

And these:

	git filter-branch -d /tmp/work-dir
	git filter-branch <non-existant-revision>
	git filter-branch --
	git filter-branch -- <non-existant-revision>

will print a message informing the user that filter-branch did
not know which reference to rewrite. Without this patch the
one with '-d' would also print usage information.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


I prefer my original patch since I think it is consistent
with the git interface.

-brandon


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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index ebf05ca..5e3fe70 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -97,6 +97,8 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 OPTIONS_SPEC=
 . git-sh-setup
 
+test $# = 0 && usage
+
 git diff-files --quiet &&
 	git diff-index --cached --quiet HEAD -- ||
 	die "Cannot rewrite branch(es) with a dirty working directory."
@@ -114,7 +116,6 @@ orig_namespace=refs/original/
 force=
 while :
 do
-	test $# = 0 && usage
 	case "$1" in
 	--)
 		shift
-- 
1.5.4.rc5.14.gaa8fc

Re: [PATCH 1/2] filter-branch: only print usage information when no arguments supplied

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Testing for whether command line arguments were supplied was being
> performed during option parsing. This had the side effect of
> printing usage information when a more appropriate error message
> would have been printed had the script been allowed to continue.
>
> Now this:
>
> 	git filter-branch
>
> will print usage information.
>
> And these:
>
> 	git filter-branch -d /tmp/work-dir
> 	git filter-branch <non-existant-revision>
> 	git filter-branch --
> 	git filter-branch -- <non-existant-revision>
>
> will print a message informing the user that filter-branch did
> not know which reference to rewrite. Without this patch the
> one with '-d' would also print usage information.
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> I prefer my original patch since I think it is consistent
> with the git interface.

I'd refrain from commenting on if it is consistent or not with
"the git interface".

But I would say I prefer your original better than this one.
Will apply.

Re: [PATCH 1/2] filter-branch: only print usage information when no arguments supplied

[Brandon Casey]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>>
>> I prefer my original patch since I think it is consistent
>> with the git interface.
> 
> I'd refrain from commenting on if it is consistent or not with
> "the git interface".

I'm not sure whether you're saying that _I_ should refrain from
making comments about "git interface" consistencies? If that's
the case, I hope you see I stated it as an opinion by saying
"I think" and I don't presume to have contributed anything but
a few minor patches. If I would have thought about it for more
than a split second I would have said "...since I think it is
more consistent with what other git tools do."

or whether you're just saying git is still in flux and there is
no "git interface"...

-brandon

Re: [PATCH 1/2] filter-branch: only print usage information when no arguments supplied

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Junio C Hamano wrote:
>> Brandon Casey <casey@nrlssc.navy.mil> writes:
>>>
>>> I prefer my original patch since I think it is consistent
>>> with the git interface.
>> 
>> I'd refrain from commenting on if it is consistent or not with
>> "the git interface".
>
> I'm not sure whether you're saying that _I_ should refrain from
> making comments about "git interface" consistencies?

No, I am just saying "I (Junio) is stupid and lazy, and I do not
want to think about it right now, so I will not judge on that
point".

[PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Brandon Casey]

This command requires a revision to be specified on the command line,
so remove '--default HEAD' from the arguments to git rev-list. They
are unnecessary.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 git-filter-branch.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5e3fe70..25f18f8 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -228,11 +228,10 @@ mkdir ../map || die "Could not create map/ directory"
 
 case "$filter_subdir" in
 "")
-	git rev-list --reverse --topo-order --default HEAD \
-		--parents "$@"
+	git rev-list --reverse --topo-order --parents "$@"
 	;;
 *)
-	git rev-list --reverse --topo-order --default HEAD \
+	git rev-list --reverse --topo-order \
 		--parents --full-history "$@" -- "$filter_subdir"
 esac > ../revs || die "Could not get the commits"
 commits=$(wc -l <../revs | tr -d " ")
-- 
1.5.4.rc5.14.gaa8fc

Re: [PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Johannes Schindelin]

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

> This command requires a revision to be specified on the command line, so 
> remove '--default HEAD' from the arguments to git rev-list. They are 
> unnecessary.

But I thought that you wanted "git filter-branch --msg-filter=rot13" to 
work on HEAD by default?

Ciao,
Dscho

Re: [PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Brandon Casey]

Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 30 Jan 2008, Brandon Casey wrote:
> 
>> This command requires a revision to be specified on the command line, so 
>> remove '--default HEAD' from the arguments to git rev-list. They are 
>> unnecessary.
> 
> But I thought that you wanted "git filter-branch --msg-filter=rot13" to 
> work on HEAD by default?

I do. But isn't that inconsistent with "git filter-branch" does _not_ work
on HEAD by default and instead prints out usage information?

If I do:

	git filter-branch -d /tmp/git_temp

and it is successful, I think I would also expect this to succeed:

	git filter-branch

So, I think the "operates on HEAD" by default is consistent with what other
git tools do, but I think it is not consistent for filter-branch to sometimes
operate on HEAD by default and sometimes error with usage information.

Disclaimer: I have only used filter-branch for two tasks.

-brandon

Re: [PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Andreas Ericsson]

Brandon Casey wrote:
> Johannes Schindelin wrote:
>> Hi,
>>
>> On Wed, 30 Jan 2008, Brandon Casey wrote:
>>
>>> This command requires a revision to be specified on the command line, so 
>>> remove '--default HEAD' from the arguments to git rev-list. They are 
>>> unnecessary.
>> But I thought that you wanted "git filter-branch --msg-filter=rot13" to 
>> work on HEAD by default?
> 
> I do. But isn't that inconsistent with "git filter-branch" does _not_ work
> on HEAD by default and instead prints out usage information?
> 
> If I do:
> 
> 	git filter-branch -d /tmp/git_temp
> 
> and it is successful, I think I would also expect this to succeed:
> 
> 	git filter-branch
> 
> So, I think the "operates on HEAD" by default is consistent with what other
> git tools do, but I think it is not consistent for filter-branch to sometimes
> operate on HEAD by default and sometimes error with usage information.
> 

Well, if there's no filter specified it has nothing to do, so erroring out
in the no-arguments-at-all case would be sensible.

OTOH, it would be better to error out for the no-filter case explicitly,
which would also cause the no-arguments case to error out.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Well, if there's no filter specified it has nothing to do, so erroring out
> in the no-arguments-at-all case would be sensible.

Actually, not erroring out but just returning doing nothing (if
there truly isn't anything to do -- iow, the filtering operation
turns out to be identity function) would be more sensible.

No filter does not necessarily mean identity, by the way.  Think
"grafts" ;-).

Re: [PATCH 2/2] git-filter-branch.sh: don't use --default when calling rev-list

[Johannes Schindelin]

Hi,

On Thu, 31 Jan 2008, Junio C Hamano wrote:

> Andreas Ericsson <ae@op5.se> writes:
> 
> > Well, if there's no filter specified it has nothing to do, so erroring 
> > out in the no-arguments-at-all case would be sensible.
> 
> Actually, not erroring out but just returning doing nothing (if there 
> truly isn't anything to do -- iow, the filtering operation turns out to 
> be identity function) would be more sensible.
> 
> No filter does not necessarily mean identity, by the way.  Think 
> "grafts" ;-).

Yeah.  Having slept over it, I agree that there is a sensible default 
action, and only one.

Ciao,
Dscho

Re: [PATCH] filter-branch: assume HEAD if no revision supplied

[Johannes Schindelin]

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

> Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> >> On Wed, 30 Jan 2008, Brandon Casey wrote:
> >>
> >>> filter-branch previously took the first non-option argument as the name 
> >>> for a new branch. Since dfd05e38, it now takes a revision or a revision 
> >>> range and modifies the current branch. Update to operate on HEAD by 
> >>> default to conform with standard git interface practice.
> >> FWIW I think the code wanted to let "git filter-branch" without options 
> >> print the usage.
> > 
> > That might be a valid safety concern to some folks.  Previously
> > we have seen people say "Whenever I see a command foo that I do
> > not know what it does, I type 'foo <Enter>' and expect it gives
> > the usage back.  So any new destructive command 'foo' should not
> > do a damage by using built-in default." (I think it was about
> > "git stash" without parameter).
> > 
> > By the way, I do not personally think it is worth to be heavily
> > supportive to the practice of trying an unknown command without
> > understanding, and I do not agree such a safety is necessarily a
> > good idea, especially if it makes normal use of the command more
> > cumbersome by people who understand what it does.
> > 
> > Even though "git stash" itself is not destrictive, you need to
> > know its "apply" subcommand to undo the action.  In that sense,
> > it is destructive to clueless people who blindly type whatever
> > command they see.
> > 
> > That's why we still allow you to say "git stash", but we removed
> > its "git stash <randam message>" syntax, which was risky when
> > subcommand name was misspelled even by people who know what the
> > command does.  I think we struck a good balance between
> > usability and safety there.  And I think we can do the same
> > here.
> >
> > Perhaps "git filter-branch <Enter>" can be prevented as in the
> > current implementation while "git filter-branch --foo-filter
> > foo" can default to HEAD to satisfy both needs.  The command
> > without any filter is supposed to be mostly no-op (unless you
> > are trying to rewrite the history with grafts).
> 
> That's what I was trying to do :)

But then you would have to keep the test for $#, but enhance it like this:

case "$#,$filter_env,$filter_tree,$filter_index,$filter_parent,\
$filter_msg,$filter_commit,$filter_tag_name,$filter_subdir" in
0,,,,,cat,'git commit-tree "$@"',)
	usage
esac

Yes, it's ugly.

Another method would be having the test _before_ the while loop. ;-)

Ciao,
Dscho

Re: [PATCH] filter-branch: assume HEAD if no revision supplied

[Brandon Casey]

Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 30 Jan 2008, Brandon Casey wrote:
> 
>> Junio C Hamano wrote:

<snip>
>>> (unless you are trying to rewrite the history with grafts).
>> That's what I was trying to do :)
> 
> But then you would have to keep the test for $#, but enhance it like this:
> 
> case "$#,$filter_env,$filter_tree,$filter_index,$filter_parent,\
> $filter_msg,$filter_commit,$filter_tag_name,$filter_subdir" in
> 0,,,,,cat,'git commit-tree "$@"',)
> 	usage
> esac

I meant I was trying to rewrite the history with grafts. :)

-brandon

[PATCH] filter-branch docs: remove brackets so not to imply revision arg is optional

[Brandon Casey]

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Documentation/git-filter-branch.txt |    2 +-
 git-filter-branch.sh                |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index e22dfa5..6145322 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	[--msg-filter <command>] [--commit-filter <command>]
 	[--tag-name-filter <command>] [--subdirectory-filter <directory>]
 	[--original <namespace>] [-d <directory>] [-f | --force]
-	[<rev-list options>...]
+	<rev-list options>
 
 DESCRIPTION
 -----------
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 25f18f8..7f71523 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -92,7 +92,7 @@ USAGE="[--env-filter <command>] [--tree-filter <command>] \
 [--msg-filter <command>] [--commit-filter <command>] \
 [--tag-name-filter <command>] [--subdirectory-filter <directory>] \
 [--original <namespace>] [-d <directory>] [-f | --force] \
-[<rev-list options>...]"
+<rev-list options>"
 
 OPTIONS_SPEC=
 . git-sh-setup
-- 
1.5.4.rc5.14.gaa8fc

Re: [PATCH] filter-branch docs: remove brackets so not to imply revision arg is optional

[Junio C Hamano]

I think this is good for 1.5.4.  I am not sure about the
"assuming HEAD" one.  Personally I am not very fond of the
idiot-proofing, but there may be many idiots around here, so...

Re: [PATCH] filter-branch docs: remove brackets so not to imply revision arg is optional

[Johannes Schindelin]

Hi,

On Wed, 30 Jan 2008, Junio C Hamano wrote:

> I think this is good for 1.5.4.  I am not sure about the "assuming HEAD" 
> one.  Personally I am not very fond of the idiot-proofing, but there may 
> be many idiots around here, so...

Oh yes, oh yes.  Count me in!

;-)

Ciao,
Dscho

Re: [PATCH] filter-branch docs: remove brackets so not to imply revision arg is optional

[Brandon Casey]

Junio C Hamano wrote:
> I think this is good for 1.5.4.  I am not sure about the
> "assuming HEAD" one.  Personally I am not very fond of the
> idiot-proofing, but there may be many idiots around here, so...


I see you applied both this patch _and_
"filter-branch: assume HEAD if no revision supplied"

The latter patch _does_ make the revision arg optional, so this
"filter-branch docs" patch is unnecessary.

-brandon

Re: [PATCH] filter-branch docs: remove brackets so not to imply revision arg is optional

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> The latter patch _does_ make the revision arg optional, so this
> "filter-branch docs" patch is unnecessary.

You are of course right.  I changed my mind and forgot to revert
that one.

Here is what I'll do.

[PATCH] Revert "filter-branch docs: remove brackets so not to imply revision arg is optional"

This reverts commit c41b439244c51b30c60953192816afc91e552578, as
we decided to default to HEAD when revision parameters are missing
and they are no longer mandatory.

 Documentation/git-filter-branch.txt |    2 +-
 git-filter-branch.sh                |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
-- 
1.5.4.rc5.16.gc0279