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

6 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:09

Johannes Schindelin [off-list ref] writes:
On Wed, 30 Jan 2008, Brandon Casey wrote:
quoted
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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:44:09

Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
On Wed, 30 Jan 2008, Brandon Casey wrote:
quoted
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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:44:09

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


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] filter-branch: assume HEAD if no revision supplied

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:44:09

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:
Junio C Hamano wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
On Wed, 30 Jan 2008, Brandon Casey wrote:
quoted
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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:44:09

Johannes Schindelin wrote:
Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:
quoted
Junio C Hamano wrote:
<snip>
quoted
quoted
(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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:44:09

Signed-off-by: Brandon Casey <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help