patchfilter-branch: assume HEAD if no revision supplied

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

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

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

Ciao, Dscho

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

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

Junio C Hamano wrote:

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.

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

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

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.

Junio C Hamano wrote:

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

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

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

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

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

Johannes Schindelin wrote:

<snip>

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

-brandon

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

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

Hi,

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

Oh yes, oh yes.  Count me in!

;-)

Ciao, Dscho

Junio C Hamano wrote:

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

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

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

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

Hi,

On Wed, 30 Jan 2008, Brandon Casey wrote:

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

Ciao, Dscho

Johannes Schindelin wrote:

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

Brandon Casey wrote:

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 <ae@op5.se> writes:

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" ;-).

Hi,

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

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

Ciao, Dscho