git 1.7.7.3: BUG - please make git mv -f quiet

Every time I do:

    git mv -f FROM TO

Git displays:

    warning: destination exists; will overwrite!

Please don't display anything other than errors (no write permission....).

The "-f" is like with mv(1), cp(1); there is nothing than can be done afterwards, so the message is redundant and obstructing.

If messages are needed, please add option:

    -v, --verbose

Jari

On Sun, Dec 11, 2011 at 11:22:37PM +0200, Jari Aalto wrote:

I'm inclined to agree. Outputting a warning just because we did what the user asked us to is unnecessarily chatty.

When I looked into it, though, it seems that "git mv" is somewhat neglected, and this trival one-line patch turned into a 5-patch series of fixes.

  [1/5]: docs: mention "-k" for both forms of "git mv"
  [2/5]: mv: honor --verbose flag
  [3/5]: mv: make non-directory destination error more clear
  [4/5]: mv: improve overwrite warning
  [5/5]: mv: be quiet about overwriting

-Peff

The "git mv" synopsis shows two forms: renaming a file, and moving files into a directory. They can both make use of the "-k" flag to ignore errors, so mention it in both places.

Signed-off-by: Jeff King <peff@peff.net>
---
I can kind of see the rationale for the original content. Using "-k" is
a lot more useful if you are actually doing multiple renames, so it
makes more sense in the second form. But it is still useful in the first
form as a shorthand for "git mv 2>/dev/null || true".

I actually would rather just see:

  git mv [options] <source> <destination>
  git mv [options] <source>... <destination>

but if we are going to go that route, we should probably decide on a style and convert all of the descriptions at the same time.

 Documentation/git-mv.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Jeff King <peff@peff.net> writes:

Also most commands when they use these multi-line synopsis style to differenciate different invocation contexts do take different set of options for different contexts, so we would need to update the option descriptions to say "this option only makes sense in this context", etc.

The code for a verbose flag has been here since "git mv" was converted to C many years ago, but actually getting the "-v" flag from the command line was accidentally lost in the transition.

Signed-off-by: Jeff King <peff@peff.net>
---
This has been broken since 2006, so I guess nobody really cares. But
it's simple to fix.

 Documentation/git-mv.txt |    8 ++++++--
 builtin/mv.c             |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

Heh. It means nobody exercised the codepaths that are inside "if (verbose)", so it may uncover old bugs, no?

On Mon, Dec 12, 2011 at 11:53:28AM -0800, Junio C Hamano wrote:

I thought that at first, too, but actually there is only one code path currently enabled by "verbose", and it is to print "Renaming ...". You can also exercise that code path with "--dry-run" (and the whole path consists of only a single printf, so hopefully we didn't manage to squeeze any bugs in there).

Once upon a time, the verbose flag was passed on to add_file_to_index, but that was dropped when the code switched to using rename_index_entry_at in 81dc230 (git-mv: Keep moved index entries inact, 2008-07-21).

-Peff

If you try to "git mv" multiple files onto another non-directory file, you confusingly get the "usage" message:

  $ touch one two three
  $ git add .
  $ git mv one two three
  usage: git mv [options] <source>... <destination>
  [...]

gave parameters that exactly match that usage!

This behavior dates back to the original C version of "git mv", which had a usage message like:

  usage: git mv (<source> <destination> | <source>...  <destination>)

This was slightly less confusing, because it at least mentions that there are two ways to invoke (but it still isn't clear why what the user provided doesn't work).

Instead, let's show an error message like:

  $ git mv one two three
  fatal: destination 'three' is not a directory

We could leave the usage message in place, too, but it doesn't actually help here. It contains no hints that there are two forms, nor that multi-file form requires that the endpoint be a directory. So it just becomes useless noise that distracts from the real error.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Jeff King <peff@peff.net> writes:

Makes perfect sense.

When we try to "git mv" over an existing file, the error message is fairly informative:

  $ git mv one two
  fatal: destination exists, source=one, destination=two

When the user forces the overwrite, we give a warning:

  $ git mv -f one two
  warning: destination exists; will overwrite!

This is less informative, but still sufficient in the simple rename case, as there is only one rename happening.

But when moving files from one directory to another, it becomes useless:

  $ mkdir three
  $ touch one two three/one
  $ git add .
  $ git mv one two three
  fatal: destination exists, source=one, destination=three/one
  $ git mv -f one two three
  warning: destination exists; will overwrite!

The first message is helpful, but the second one gives us no clue about what was overwritten. Instead, let's mirror the first form more closely, with:

  $ git mv -f one two three
  warning: destination exists (will overwrite), source=one, destination=three/one

Signed-off-by: Jeff King <peff@peff.net>
---
This message looks overly long to me, but I wanted to match the existing
messages. Another option would be just:

  warning: overwriting 'three/one'

 builtin/mv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Jeff King <peff@peff.net> writes:

Yes, I think it makes perfect sense to drop the ugly "source=one destination=two" cruft, both for single-source and multiple-source cases.

On Mon, Dec 12, 2011 at 11:57:31AM -0800, Junio C Hamano wrote:

Yeah, the more I look at the message in the patch I sent, the uglier it gets. Here's a re-rolled 4 and 5 with the nicer format.

-Peff

When we try to "git mv" over an existing file, the error message is fairly informative:

  $ git mv one two
  fatal: destination exists, source=one, destination=two

When the user forces the overwrite, we give a warning:

  $ git mv -f one two
  warning: destination exists; will overwrite!

This is less informative, but still sufficient in the simple rename case, as there is only one rename happening.

But when moving files from one directory to another, it becomes useless:

  $ mkdir three
  $ touch one two three/one
  $ git add .
  $ git mv one two three
  fatal: destination exists, source=one, destination=three/one
  $ git mv -f one two three
  warning: destination exists; will overwrite!

The first message is helpful, but the second one gives us no clue about what was overwritten. Let's mention the name of the destination file:

  $ git mv -f one two three
  warning: overwriting 'three/one'

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

When a user asks us to force a mv and overwrite the destination, we print a warning. However, since a typical use would be:

  $ git mv one two
  fatal: destination exists, source=one, destination=two
  $ git mv -f one two
  warning: overwriting 'two'

this warning is just noise. We already know we're overwriting; that's why we gave -f!

This patch silences the warning unless "--verbose" is given.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

When a user asks us to force a mv and overwrite the destination, we print a warning. However, since a typical use would be:

  $ git mv one two
  fatal: destination exists, source=one, destination=two
  $ git mv -f one two
  warning: destination exists (will overwrite), source=one, destination=two

this warning is just noise. We already know we're overwriting; that's why we gave -f!

This patch silences the warning unless "--verbose" is given.

Signed-off-by: Jeff King <peff@peff.net>
---
You could perhaps argue that it is useful in the case of moving multiple
files into a directory (since it tells you _which_ files were
overwritten). We could turn the warning on in that case, but I'm
inclined to leave it. If the user cares about this information, they can
use "-v" along with "-f".

 builtin/mv.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

Makes sense, but I also think even under verbose mode we should avoid the future tense. I.e. something like this:

    $ git mv -v -f one two
    warning: overwriting two
    $ git mv -v -f one two three
    warning: overwriting three/one