threads / patch / 4792
Subject: [PATCH] builtin-log: respect diff configuration options
[PATCH 0/1] 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.
-- Eric Wong
[PATCH] 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.
Signed-off-by: Eric Wong <normalperson@yhbt.net> --- Documentation/config.txt | 5 +++++ diff.c | 12 ++++++++++++ 2 files changed, 17 insertions(+), 0 deletions(-)
Documentation/config.txt, diff.c
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
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
Junio C HamanoRe: [PATCH] builtin-log: respect diff configuration options
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 ;-).
Junio C HamanoRe: [PATCH] diff.c: respect diff.renames config option
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?
Junio C HamanoRe: [PATCH] diff.c: respect diff.renames config option
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.
Junio C HamanoRe: [PATCH] diff.c: respect diff.renames config option
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
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(-)
git-cvsexportcommit.perl, git-cvsserver.perl, templates/hooks--update
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
Junio C HamanoRe: [PATCH] Fix several places where diff.renames in config can be problematic
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(-)
builtin-diff-files.c, builtin-diff-index.c, builtin-diff-stages.c, builtin-diff-tree.c, builtin-diff.c, builtin-log.c, diff.c, diff.h
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
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
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
Junio C HamanoRe: [PATCH] diff.c: respect diff.renames config option
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
Junio C Hamano[PATCH] update Documentation/diff-options.txt
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