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

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

[Jari Aalto]

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

[PATCH 0/5] mixed bag of minor "git mv" fixes

[Jeff King]

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

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

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

[PATCH 1/5] docs: mention "-k" for both forms of "git mv"

[Jeff King]

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

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index b8db373..4be7a71 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -15,7 +15,7 @@ DESCRIPTION
 -----------
 This script is used to move or rename a file, directory or symlink.
 
- git mv [-f] [-n] <source> <destination>
+ git mv [-f] [-n] [-k] <source> <destination>
  git mv [-f] [-n] [-k] <source> ... <destination directory>
 
 In the first form, it renames <source>, which must exist and be either
-- 
1.7.8.13.g74677

Re: [PATCH 1/5] docs: mention "-k" for both forms of "git mv"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

>  Documentation/git-mv.txt |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
> index b8db373..4be7a71 100644
> --- a/Documentation/git-mv.txt
> +++ b/Documentation/git-mv.txt
> @@ -15,7 +15,7 @@ DESCRIPTION
>  -----------
>  This script is used to move or rename a file, directory or symlink.
>  
> - git mv [-f] [-n] <source> <destination>
> + git mv [-f] [-n] [-k] <source> <destination>
>   git mv [-f] [-n] [-k] <source> ... <destination directory>
>  
>  In the first form, it renames <source>, which must exist and be either

[PATCH 2/5] mv: honor --verbose flag

[Jeff King]

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

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 4be7a71..e3c8448 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -15,8 +15,8 @@ DESCRIPTION
 -----------
 This script is used to move or rename a file, directory or symlink.
 
- git mv [-f] [-n] [-k] <source> <destination>
- git mv [-f] [-n] [-k] <source> ... <destination directory>
+ git mv [-v] [-f] [-n] [-k] <source> <destination>
+ git mv [-v] [-f] [-n] [-k] <source> ... <destination directory>
 
 In the first form, it renames <source>, which must exist and be either
 a file, symlink or directory, to <destination>.
@@ -40,6 +40,10 @@ OPTIONS
 --dry-run::
 	Do nothing; only show what would happen
 
+-v::
+--verbose::
+	Report the names of files as they are moved.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/mv.c b/builtin/mv.c
index 5efe6c5..11abaf5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -59,6 +59,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	int i, newfd;
 	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
 	struct option builtin_mv_options[] = {
+		OPT__VERBOSE(&verbose, "be verbose"),
 		OPT__DRY_RUN(&show_only, "dry run"),
 		OPT__FORCE(&force, "force move/rename even if target exists"),
 		OPT_BOOLEAN('k', NULL, &ignore_errors, "skip move/rename errors"),
-- 
1.7.8.13.g74677

Re: [PATCH 2/5] mv: honor --verbose flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

>  Documentation/git-mv.txt |    8 ++++++--
>  builtin/mv.c             |    1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
> index 4be7a71..e3c8448 100644
> --- a/Documentation/git-mv.txt
> +++ b/Documentation/git-mv.txt
> @@ -15,8 +15,8 @@ DESCRIPTION
>  -----------
>  This script is used to move or rename a file, directory or symlink.
>  
> - git mv [-f] [-n] [-k] <source> <destination>
> - git mv [-f] [-n] [-k] <source> ... <destination directory>
> + git mv [-v] [-f] [-n] [-k] <source> <destination>
> + git mv [-v] [-f] [-n] [-k] <source> ... <destination directory>
>  
>  In the first form, it renames <source>, which must exist and be either
>  a file, symlink or directory, to <destination>.
> @@ -40,6 +40,10 @@ OPTIONS
>  --dry-run::
>  	Do nothing; only show what would happen
>  
> +-v::
> +--verbose::
> +	Report the names of files as they are moved.
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 5efe6c5..11abaf5 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -59,6 +59,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	int i, newfd;
>  	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
>  	struct option builtin_mv_options[] = {
> +		OPT__VERBOSE(&verbose, "be verbose"),
>  		OPT__DRY_RUN(&show_only, "dry run"),
>  		OPT__FORCE(&force, "force move/rename even if target exists"),
>  		OPT_BOOLEAN('k', NULL, &ignore_errors, "skip move/rename errors"),

Re: [PATCH 2/5] mv: honor --verbose flag

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Heh. It means nobody exercised the codepaths that are inside "if (verbose)",
> so it may uncover old bugs, no?

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

[PATCH 3/5] mv: make non-directory destination error more clear

[Jeff King]

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

>From the user's perspective, that makes no sense. They just
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(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 11abaf5..ae6c30c 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -94,7 +94,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		destination = copy_pathspec(dest_path[0], argv, argc, 1);
 	} else {
 		if (argc != 1)
-			usage_with_options(builtin_mv_usage, builtin_mv_options);
+			die("destination '%s' is not a directory", dest_path[0]);
 		destination = dest_path;
 	}
 
-- 
1.7.8.13.g74677

Re: [PATCH 3/5] mv: make non-directory destination error more clear

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Instead, let's show an error message like:
>
>   $ git mv one two three
>   fatal: destination 'three' is not a directory

Makes perfect sense.

> 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(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 11abaf5..ae6c30c 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -94,7 +94,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  		destination = copy_pathspec(dest_path[0], argv, argc, 1);
>  	} else {
>  		if (argc != 1)
> -			usage_with_options(builtin_mv_usage, builtin_mv_options);
> +			die("destination '%s' is not a directory", dest_path[0]);
>  		destination = dest_path;
>  	}

[PATCH 4/5] mv: improve overwrite warning

[Jeff King]

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

diff --git a/builtin/mv.c b/builtin/mv.c
index ae6c30c..c9ecb03 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("%s; will overwrite!"), bad);
+					warning(_("%s (will overwrite), source=%s, destination=%s"),
+						bad, src, dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

Re: [PATCH 4/5] mv: improve overwrite warning

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This message looks overly long to me, but I wanted to match the existing
> messages. Another option would be just:
>
>   warning: overwriting 'three/one'

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

>  builtin/mv.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index ae6c30c..c9ecb03 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				 * check both source and destination
>  				 */
>  				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
> -					warning(_("%s; will overwrite!"), bad);
> +					warning(_("%s (will overwrite), source=%s, destination=%s"),
> +						bad, src, dst);
>  					bad = NULL;
>  				} else
>  					bad = _("Cannot overwrite");

Re: [PATCH 4/5] mv: improve overwrite warning

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > This message looks overly long to me, but I wanted to match the existing
> > messages. Another option would be just:
> >
> >   warning: overwriting 'three/one'
> 
> Yes, I think it makes perfect sense to drop the ugly "source=one destination=two"
> cruft, both for single-source and multiple-source cases.

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

[PATCHv2 4/5] mv: improve overwrite warning

[Jeff King]

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

diff --git a/builtin/mv.c b/builtin/mv.c
index ae6c30c..8dd5a45 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,7 +177,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("%s; will overwrite!"), bad);
+					warning(_("overwriting '%s'"), dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

[PATCHv2 5/5] mv: be quiet about overwriting

[Jeff King]

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

diff --git a/builtin/mv.c b/builtin/mv.c
index 8dd5a45..2a144b0 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("overwriting '%s'"), dst);
+					if (verbose)
+						warning(_("overwriting '%s'"), dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

[PATCH 5/5] mv: be quiet about overwriting

[Jeff King]

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

diff --git a/builtin/mv.c b/builtin/mv.c
index c9ecb03..b6e7e4f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -177,8 +177,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				 * check both source and destination
 				 */
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-					warning(_("%s (will overwrite), source=%s, destination=%s"),
-						bad, src, dst);
+					if (verbose)
+						warning(_("%s (will overwrite), source=%s, destination=%s"),
+							bad, src, dst);
 					bad = NULL;
 				} else
 					bad = _("Cannot overwrite");
-- 
1.7.8.13.g74677

Re: [PATCH 5/5] mv: be quiet about overwriting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

>  builtin/mv.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index c9ecb03..b6e7e4f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -177,8 +177,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				 * check both source and destination
>  				 */
>  				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
> -					warning(_("%s (will overwrite), source=%s, destination=%s"),
> -						bad, src, dst);
> +					if (verbose)
> +						warning(_("%s (will overwrite), source=%s, destination=%s"),
> +							bad, src, dst);
>  					bad = NULL;
>  				} else
>  					bad = _("Cannot overwrite");