Skip to contentSkip to conversation

I forgot to run format-patch with the -M flag

PATCH New Eric Wong 2 participants 13 messages Jun 15, 2016 1 message not in the archive Documentation

lore
Share & tools
lore

Eric Wong

author
#0 lore raw

cover I forgot to run format-patch with the -M flag

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.

Signature · 1 line
Eric Wong
author
#1 lore raw

PATCH 1/1 diff.c: respect diff.renames config option

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

---
 Documentation/config.txt |    5 +++++
 diff.c                   |   12 ++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
Documentation/config.txt +5 -0
index f075f19..5290a8f 100644
@@ -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.c +12 -0
index 507e401..223622a 100644
@@ -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)
Signature · 1 line
1.4.1.g3dc65
author
#2 lore raw

PATCH 1/1 builtin-log: respect diff configuration options

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

---
 builtin-log.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
builtin-log.c +5 -2
index 864c6cd..698b71e 100644
@@ -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;
 
 	/*
Signature · 1 line
1.4.1.g3dc65
#3 lore raw

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

Quoted 4 lines · Eric Wong

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

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

#4 lore raw

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

In reply to Eric Wong
Quoted 2 lines · Eric Wong

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.

Quoted 4 lines
diff.c +0 -0
@@ -120,6 +121,16 @@ int git_diff_config(const char *var, con
 			diff_use_color_default = git_config_bool(var, value);
 		return 0;
 	}

diff.c int git_diff_config(const char *var, con · line 133

Quoted 10 lines
diff.c +10 -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?

A message that is not in the archive (sent off-list or not yet received): 20060707110123.GA23400@soma

#6 lore raw

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

Quoted 3 lines · Eric Wong

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.

#7 lore raw

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

Quoted 8 lines · Junio C Hamano

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.

author
#8 lore raw

PATCH 1/1 Fix several places where diff.renames in config can be problematic

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

---

Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Junio C Hamano

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.

Quoted 2 lines

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(-)
git-cvsexportcommit.perl +5 -3
index d1051d0..09ff2cc 100755
@@ -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) {
git-cvsserver.perl +1 -1
index 5ccca4f..ae878ea 100755
@@ -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 )
templates/hooks--update +2 -2
index d7a8f0a..76d5ac2 100644
@@ -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:"
 		;;
Signature · 1 line
1.4.1.gc57e
#9 lore raw

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

Review

Quoted 3 lines · Eric Wong
(quoted diff) +2 -1
-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.

---
 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(-)
builtin-diff-files.c +1 -1
index a655eea..81ac2fe 100644
@@ -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;
 
builtin-diff-index.c +1 -1
index b37c9e8..a1fa1b8 100644
@@ -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;
 
builtin-diff-stages.c +1 -1
index 30931fe..9c62702 100644
@@ -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] == '-') {
builtin-diff-tree.c +1 -1
index ae1cde9..b610668 100644
@@ -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;
builtin-diff.c +1 -1
index d520c7c..1df531b 100644
@@ -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);
builtin-log.c +4 -4
index 698b71e..dd5a5a2 100644
@@ -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.c +7 -1
index 1bf1ed0..493650c 100644
@@ -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.h +1 -1
index 8ab0448..a06f959 100644
@@ -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 *);
author
#10 lore raw

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

Quoted 18 lines · Junio C Hamano

Review

Quoted 3 lines · Eric Wong
(quoted diff) +2 -1
-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.

Quoted 8 lines

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.

Signature · 1 line
Eric Wong
author
#11 lore raw

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

---
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(-)
templates/hooks--update +2 -2
index d7a8f0a..76d5ac2 100644
@@ -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:"
 		;;
Signature · 1 line
1.4.1.g6a62
#12 lore raw

PATCH 1/1 update Documentation/diff-options.txt

In reply to Junio C Hamano

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

---
 Documentation/diff-options.txt |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)
Documentation/diff-options.txt +21 -3
index f523ec2..fdcfd68 100644
@@ -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
Signature · 1 line
1.4.1.gfff4c
#13 lore raw

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

In reply to Junio C Hamano

In the same spirit...

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

---
 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
diff.c +2 -0
index 2534fce..39e608f 100644
@@ -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"))
Signature · 1 line
1.4.1.gfff4c