[PATCH 0/1] I forgot to run format-patch with the -M flag

[PATCH 0/1] I forgot to run format-patch with the -M flag

[Eric Wong]

Now I can forget more often :)  Unless the similarity is too low.

I'm not too sure if I'd want to set similarity or find-copies-harder
into my config just yet, since they depend on the situation.  But I
usually want renames in my diffs.

-- 
Eric Wong

[PATCH] diff.c: respect diff.renames config option

[Eric Wong]

diff.renames is mentioned several times in the documentation,
but to my surprise it didn't do anything before this patch.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Documentation/config.txt |    5 +++++
 diff.c                   |   12 ++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f075f19..5290a8f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -114,6 +114,11 @@ diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the git diff option '-l'.
 
+diff.renames::
+	Tells git to detect renames.  If set to any boolean value, it
+	will enable basic rename detection.  If set to "copies" or
+	"copy", it will detect copies, as well.
+
 format.headers::
 	Additional email headers to include in a patch to be submitted
 	by mail.  See gitlink:git-format-patch[1].
diff --git a/diff.c b/diff.c
index 507e401..223622a 100644
--- a/diff.c
+++ b/diff.c
@@ -13,6 +13,7 @@ #include "xdiff-interface.h"
 
 static int use_size_cache;
 
+static int diff_detect_rename_default = 0;
 static int diff_rename_limit_default = -1;
 static int diff_use_color_default = 0;
 
@@ -120,6 +121,16 @@ int git_diff_config(const char *var, con
 			diff_use_color_default = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.renames")) {
+		if (!value)
+			diff_detect_rename_default = DIFF_DETECT_RENAME;
+		else if (!strcasecmp(value, "copies") ||
+			 !strcasecmp(value, "copy"))
+			diff_detect_rename_default = DIFF_DETECT_COPY;
+		else
+			diff_detect_rename_default = git_config_bool(var,value);
+		return 0;
+	}
 	if (!strncmp(var, "diff.color.", 11)) {
 		int slot = parse_diff_color_slot(var, 11);
 		diff_colors[slot] = parse_diff_color_value(value, var);
@@ -1429,6 +1440,7 @@ void diff_setup(struct diff_options *opt
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
 	options->color_diff = diff_use_color_default;
+	options->detect_rename = diff_detect_rename_default;
 }
 
 int diff_setup_done(struct diff_options *options)
-- 
1.4.1.g3dc65

[PATCH] builtin-log: respect diff configuration options

[Eric Wong]

The log commands are all capable of generating diffs, so we
should respect those configuration options for diffs here.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 builtin-log.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 864c6cd..698b71e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -47,6 +47,7 @@ int cmd_whatchanged(int argc, const char
 {
 	struct rev_info rev;
 
+	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.diff = 1;
 	rev.diffopt.recursive = 1;
@@ -61,6 +62,7 @@ int cmd_show(int argc, const char **argv
 {
 	struct rev_info rev;
 
+	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.diff = 1;
 	rev.diffopt.recursive = 1;
@@ -77,6 +79,7 @@ int cmd_log(int argc, const char **argv,
 {
 	struct rev_info rev;
 
+	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, envp, &rev);
@@ -102,7 +105,7 @@ static int git_format_config(const char 
 		strcat(extra_headers, value);
 		return 0;
 	}
-	return git_default_config(var, value);
+	return git_diff_config(var, value);
 }
 
 
@@ -234,6 +237,7 @@ int cmd_format_patch(int argc, const cha
 	struct diff_options patch_id_opts;
 	char *add_signoff = NULL;
 
+	git_config(git_format_config);
 	init_revisions(&rev);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
@@ -243,7 +247,6 @@ int cmd_format_patch(int argc, const cha
 	rev.diffopt.msg_sep = "";
 	rev.diffopt.recursive = 1;
 
-	git_config(git_format_config);
 	rev.extra_headers = extra_headers;
 
 	/*
-- 
1.4.1.g3dc65

Re: [PATCH] builtin-log: respect diff configuration options

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> The log commands are all capable of generating diffs, so we
> should respect those configuration options for diffs here.
>
> Signed-off-by: Eric Wong <normalperson@yhbt.net>

Makes sense.  This makes "git log -p" more colorful ;-).

Re: [PATCH] diff.c: respect diff.renames config option

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> diff.renames is mentioned several times in the documentation,
> but to my surprise it didn't do anything before this patch.

I am really reluctant to do this at this low-level, especially
since there is no way to defeat the settings from the command
line.

> @@ -120,6 +121,16 @@ int git_diff_config(const char *var, con
>  			diff_use_color_default = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "diff.renames")) {
> +		if (!value)
> +			diff_detect_rename_default = DIFF_DETECT_RENAME;
> +		else if (!strcasecmp(value, "copies") ||
> +			 !strcasecmp(value, "copy"))
> +			diff_detect_rename_default = DIFF_DETECT_COPY;
> +		else
> +			diff_detect_rename_default = git_config_bool(var,value);
> +		return 0;
> +	}

I suspect this works only because DIFF_DETECT_RENAME happens to
be 1?

Re: [PATCH] diff.c: respect diff.renames config option

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Nevertheless, it's still opt -in via repo-config, and most people will
> find renames useful unless they need to export to non-git
> systems.

I am more worried about somebody who opts-in finds breakage of
commands that happen to internally use low-level diff machinery
and expect the diff machinery does not automagically do funny
rename detection without being told.

For example, revision walking with path pruning uses diff
machinery without renames.  I do not know what happens if I
override it with diff.renames to allow rename detection but I
fear something might horribly break.

That is why I said I do not want this at _that_ low level.  I do
not have objections to have the configuration at a layer closer
to the UI, e.g. things in builtin-log.c and builtin-diff.c.

Re: [PATCH] diff.c: respect diff.renames config option

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> I am more worried about somebody who opts-in finds breakage of
> commands that happen to internally use low-level diff machinery
> and expect the diff machinery does not automagically do funny
> rename detection without being told.
> ...
> That is why I said I do not want this at _that_ low level.  I do
> not have objections to have the configuration at a layer closer
> to the UI, e.g. things in builtin-log.c and builtin-diff.c.

Upon closer look I think the revision pruning code is OK.  So
let's cook this as is in "next" and see what happens.

[PATCH] Fix several places where diff.renames in config can be problematic

[Eric Wong]

git-cvsexportcommit.perl:
git-cvsserver.perl:
	CVS can't handle renames, so we best not show them to
	users.

templates/hooks--update:
	replace diffstat calls with git diff --stat

There may be other places where users having diff.renames can be
problematic, too.  diff.renames is opt-in, but maybe some users
have enabled it in their configs previously because it's been
in examples for a long time...

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Junio C Hamano <junkio@cox.net> wrote:
 > Junio C Hamano <junkio@cox.net> writes:
 > 
 > > I am more worried about somebody who opts-in finds breakage of
 > > commands that happen to internally use low-level diff machinery
 > > and expect the diff machinery does not automagically do funny
 > > rename detection without being told.
 > > ...
 > > That is why I said I do not want this at _that_ low level.  I do
 > > not have objections to have the configuration at a layer closer
 > > to the UI, e.g. things in builtin-log.c and builtin-diff.c.
 > 
 > Upon closer look I think the revision pruning code is OK.  So
 > let's cook this as is in "next" and see what happens.

 Cool.  I've found these that could be potential issues.  There could be
 more, and possibly many in 3rd-party scripts outside our control, too.

 git-cvsexportcommit.perl |    8 +++++---
 git-cvsserver.perl       |    2 +-
 templates/hooks--update  |    4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index d1051d0..09ff2cc 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -91,7 +91,8 @@ close MSG;
 $? && die "Error extracting the commit message";
 
 my (@afiles, @dfiles, @mfiles, @dirs);
-my @files = safe_pipe_capture('git-diff-tree', '-r', $parent, $commit);
+my @files = safe_pipe_capture('git-diff-tree','--no-renames','-r',
+				$parent, $commit);
 #print @files;
 $? && die "Error in git-diff-tree";
 foreach my $f (@files) {
@@ -175,7 +176,8 @@ foreach my $d (@dirs) {
 
 print "'Patching' binary files\n";
 
-my @bfiles = grep(m/^Binary/, safe_pipe_capture('git-diff-tree', '-p', $parent, $commit));
+my @bfiles = grep(m/^Binary/, safe_pipe_capture('git-diff-tree','--no-renames',
+						'-p', $parent, $commit));
 @bfiles = map { chomp } @bfiles;
 foreach my $f (@bfiles) {
     # check that the file in cvs matches the "old" file
@@ -206,7 +208,7 @@ ## apply non-binary changes
 my $fuzz = $opt_p ? 0 : 2;
 
 print "Patching non-binary files\n";
-print `(git-diff-tree -p $parent -p $commit | patch -p1 -F $fuzz ) 2>&1`;
+print `(git-diff-tree --no-renames -p $parent -p $commit | patch -p1 -F $fuzz ) 2>&1`;
 
 my $dirtypatch = 0;
 if (($? >> 8) == 2) {
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 5ccca4f..ae878ea 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2295,7 +2295,7 @@ sub update
 
         if ( defined ( $lastpicked ) )
         {
-            my $filepipe = open(FILELIST, '-|', 'git-diff-tree', '-r', $lastpicked, $commit->{hash}) or die("Cannot call git-diff-tree : $!");
+            my $filepipe = open(FILELIST, '-|', 'git-diff-tree', '--no-merges', '-r', $lastpicked, $commit->{hash}) or die("Cannot call git-diff-tree : $!");
             while ( <FILELIST> )
             {
                 unless ( /^:\d{6}\s+\d{3}(\d)\d{2}\s+[a-zA-Z0-9]{40}\s+([a-zA-Z0-9]{40})\s+(\w)\s+(.*)$/o )
diff --git a/templates/hooks--update b/templates/hooks--update
index d7a8f0a..76d5ac2 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -60,7 +60,7 @@ then
 			echo "Changes since $prev:"
 			git rev-list --pretty $prev..$3 | $short
 			echo ---
-			git diff $prev..$3 | diffstat -p1
+			git diff --stat $prev..$3
 			echo ---
 		fi
 		;;
@@ -75,7 +75,7 @@ else
 	base=$(git-merge-base "$2" "$3")
 	case "$base" in
 	"$2")
-		git diff "$3" "^$base" | diffstat -p1
+		git diff --stat "$3" "^$base"
 		echo
 		echo "New commits:"
 		;;
-- 
1.4.1.gc57e

Re: [PATCH] Fix several places where diff.renames in config can be problematic

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> -my @files = safe_pipe_capture('git-diff-tree', '-r', $parent, $commit);
> +my @files = safe_pipe_capture('git-diff-tree','--no-renames','-r',
> +				$parent, $commit);

I changed my mind.

-- >8 --
diff: do not use configuration magic at the core-level

The Porcelainish has become so much usable as the UI that there
is not much reason people should be using the core programs by
hand anymore.  At this point we are better off making the
behaviour of the core programs predictable by keeping them
unaffected by the configuration variables.  Otherwise they will
become very hard to use as reliable building blocks.

For example, "git-commit -a" internally uses git-diff-files to
figure out the set of paths that need to be updated in the
index, and we should never allow diff.renames that happens to be
in the configuration to interfere (or slow down the process).

The UI level configuration such as showing renamed diff and
coloring are still honored by the Porcelainish ("git log" family
and "git diff"), but not by the core anymore.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-diff-files.c  |    2 +-
 builtin-diff-index.c  |    2 +-
 builtin-diff-stages.c |    2 +-
 builtin-diff-tree.c   |    2 +-
 builtin-diff.c        |    2 +-
 builtin-log.c         |    8 ++++----
 diff.c                |    8 +++++++-
 diff.h                |    2 +-
 8 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index a655eea..81ac2fe 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -18,7 +18,7 @@ int cmd_diff_files(int argc, const char 
 	struct rev_info rev;
 	int silent = 0;
 
-	git_config(git_diff_config);
+	git_config(git_default_config); /* no "diff" UI options */
 	init_revisions(&rev);
 	rev.abbrev = 0;
 
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index b37c9e8..a1fa1b8 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -15,7 +15,7 @@ int cmd_diff_index(int argc, const char 
 	int cached = 0;
 	int i;
 
-	git_config(git_diff_config);
+	git_config(git_default_config); /* no "diff" UI options */
 	init_revisions(&rev);
 	rev.abbrev = 0;
 
diff --git a/builtin-diff-stages.c b/builtin-diff-stages.c
index 30931fe..9c62702 100644
--- a/builtin-diff-stages.c
+++ b/builtin-diff-stages.c
@@ -61,7 +61,7 @@ int cmd_diff_stages(int ac, const char *
 	const char *prefix = setup_git_directory();
 	const char **pathspec = NULL;
 
-	git_config(git_diff_config);
+	git_config(git_default_config); /* no "diff" UI options */
 	read_cache();
 	diff_setup(&diff_options);
 	while (1 < ac && av[1][0] == '-') {
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index ae1cde9..b610668 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -67,7 +67,7 @@ int cmd_diff_tree(int argc, const char *
 	static struct rev_info *opt = &log_tree_opt;
 	int read_stdin = 0;
 
-	git_config(git_diff_config);
+	git_config(git_default_config); /* no "diff" UI options */
 	nr_sha1 = 0;
 	init_revisions(opt);
 	opt->abbrev = 0;
diff --git a/builtin-diff.c b/builtin-diff.c
index d520c7c..1df531b 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -250,7 +250,7 @@ int cmd_diff(int argc, const char **argv
 	 * Other cases are errors.
 	 */
 
-	git_config(git_diff_config);
+	git_config(git_diff_ui_config);
 	init_revisions(&rev);
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/builtin-log.c b/builtin-log.c
index 698b71e..dd5a5a2 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -47,7 +47,7 @@ int cmd_whatchanged(int argc, const char
 {
 	struct rev_info rev;
 
-	git_config(git_diff_config);
+	git_config(git_diff_ui_config);
 	init_revisions(&rev);
 	rev.diff = 1;
 	rev.diffopt.recursive = 1;
@@ -62,7 +62,7 @@ int cmd_show(int argc, const char **argv
 {
 	struct rev_info rev;
 
-	git_config(git_diff_config);
+	git_config(git_diff_ui_config);
 	init_revisions(&rev);
 	rev.diff = 1;
 	rev.diffopt.recursive = 1;
@@ -79,7 +79,7 @@ int cmd_log(int argc, const char **argv,
 {
 	struct rev_info rev;
 
-	git_config(git_diff_config);
+	git_config(git_diff_ui_config);
 	init_revisions(&rev);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, envp, &rev);
@@ -105,7 +105,7 @@ static int git_format_config(const char 
 		strcat(extra_headers, value);
 		return 0;
 	}
-	return git_diff_config(var, value);
+	return git_diff_ui_config(var, value);
 }
 
 
diff --git a/diff.c b/diff.c
index 1bf1ed0..493650c 100644
--- a/diff.c
+++ b/diff.c
@@ -102,7 +102,13 @@ static const char *parse_diff_color_valu
 	die("bad config value '%s' for variable '%s'", value, var);
 }
 
-int git_diff_config(const char *var, const char *value)
+/*
+ * These are to give UI layer defaults.
+ * The core-level commands such as git-diff-files should
+ * never be affected by the setting of diff.renames
+ * the user happens to have in the configuration file.
+ */
+int git_diff_ui_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "diff.renamelimit")) {
 		diff_rename_limit_default = git_config_int(var, value);
diff --git a/diff.h b/diff.h
index 8ab0448..a06f959 100644
--- a/diff.h
+++ b/diff.h
@@ -123,7 +123,7 @@ #define DIFF_SETUP_REVERSE      	1
 #define DIFF_SETUP_USE_CACHE		2
 #define DIFF_SETUP_USE_SIZE_CACHE	4
 
-extern int git_diff_config(const char *var, const char *value);
+extern int git_diff_ui_config(const char *var, const char *value);
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);

Re: [PATCH] Fix several places where diff.renames in config can be problematic

[Eric Wong]

Junio C Hamano <junkio@cox.net> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > -my @files = safe_pipe_capture('git-diff-tree', '-r', $parent, $commit);
> > +my @files = safe_pipe_capture('git-diff-tree','--no-renames','-r',
> > +				$parent, $commit);
> 
> I changed my mind.
> 
> -- >8 --
> diff: do not use configuration magic at the core-level
> 
> The Porcelainish has become so much usable as the UI that there
> is not much reason people should be using the core programs by
> hand anymore.  At this point we are better off making the
> behaviour of the core programs predictable by keeping them
> unaffected by the configuration variables.  Otherwise they will
> become very hard to use as reliable building blocks.
> 
> For example, "git-commit -a" internally uses git-diff-files to
> figure out the set of paths that need to be updated in the
> index, and we should never allow diff.renames that happens to be
> in the configuration to interfere (or slow down the process).
> 
> The UI level configuration such as showing renamed diff and
> coloring are still honored by the Porcelainish ("git log" family
> and "git diff"), but not by the core anymore.

Full ack on this.  I was ready to let my diff.renames patch drop
if there were too many potential incompatibilities/breakages,
but this should alleviate that.

I should work on breaking out of the habit of using git
diff-{index,tree} in my day-to-day use and finally start using git diff
more to save some keystrokes.

-- 
Eric Wong

[PATCH] templates/hooks--update: replace diffstat calls with git diff --stat

[Eric Wong]

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 This was part of the previous patch, but useful on its own since
 we have our own internal diffstat

 templates/hooks--update |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/templates/hooks--update b/templates/hooks--update
index d7a8f0a..76d5ac2 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -60,7 +60,7 @@ then
 			echo "Changes since $prev:"
 			git rev-list --pretty $prev..$3 | $short
 			echo ---
-			git diff $prev..$3 | diffstat -p1
+			git diff --stat $prev..$3
 			echo ---
 		fi
 		;;
@@ -75,7 +75,7 @@ else
 	base=$(git-merge-base "$2" "$3")
 	case "$base" in
 	"$2")
-		git diff "$3" "^$base" | diffstat -p1
+		git diff --stat "$3" "^$base"
 		echo
 		echo "New commits:"
 		;;
-- 
1.4.1.g6a62

Re: [PATCH] diff.c: respect diff.renames config option

[Junio C Hamano]

In the same spirit...

From: Junio C Hamano <junkio@cox.net>
Date: Fri, 7 Jul 2006 05:27:24 -0700
Subject: [PATCH] diff.c: --no-color to defeat diff.color configuration.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 2534fce..39e608f 100644
--- a/diff.c
+++ b/diff.c
@@ -1622,6 +1622,8 @@ int diff_opt_parse(struct diff_options *
 	}
 	else if (!strcmp(arg, "--color"))
 		options->color_diff = 1;
+	else if (!strcmp(arg, "--no-color"))
+		options->color_diff = 0;
 	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
 		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
 	else if (!strcmp(arg, "-b") || !strcmp(arg, "--ignore-space-change"))
-- 
1.4.1.gfff4c

[PATCH] update Documentation/diff-options.txt

[Junio C Hamano]

We've been lazy and left things undocumented for some time.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 Documentation/diff-options.txt |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index f523ec2..fdcfd68 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -4,18 +4,21 @@
 -u::
 	Synonym for "-p".
 
+--raw::
+	Generate the raw format.
+
 --patch-with-raw::
-	Generate patch but keep also the default raw diff output.
+	Synonym for "-p --raw".
 
 --stat::
-	Generate a diffstat instead of a patch.
+	Generate a diffstat.
 
 --summary::
 	Output a condensed summary of extended header information
 	such as creations, renames and mode changes.
 
 --patch-with-stat::
-	Generate patch and prepend its diffstat.
+	Synonym for "-p --stat".
 
 -z::
 	\0 line termination on output
@@ -26,11 +29,26 @@
 --name-status::
 	Show only names and status of changed files.
 
+--color::
+	Show colored diff.
+
+--no-color::
+	Turn off colored diff, even when the configuration file
+	gives the default to color output.
+
+--no-renames::
+	Turn off rename detection, even when the configuration
+	file gives the default to do so.
+
 --full-index::
 	Instead of the first handful characters, show full
 	object name of pre- and post-image blob on the "index"
 	line when generating a patch format output.	
 
+--binary::
+	In addition to --full-index, output "binary diff" that
+	can be applied with "git apply".
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	name in diff-raw format output and diff-tree header
-- 
1.4.1.gfff4c