[PATCH] log: support --full-diff=<pathspec>

[PATCH] log: support --full-diff=<pathspec>

[Nguyễn Thái Ngọc Duy]

I wanted to check if any patches that update builtin/fsck.c also
update 't' directory and git seemed not support this case
(true?). With this I can do

  git log --patch --full-diff="'builtin/fsck.c' 't'" -- builtin/fsck.c

I guess this may be something people find useful.

This patch is a bit inconvenient though because <pathspec> is parsed
with sq_dequote_to_argv() and all arguments must be wrapped by ''.
Also "full-diff" name does not make much sense when it comes with
pathspec.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c |   19 +++++++++++++++++++
 revision.h |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 8764dde..f508953 100644
--- a/revision.c
+++ b/revision.c
@@ -13,6 +13,7 @@
 #include "decorate.h"
 #include "log-tree.h"
 #include "string-list.h"
+#include "quote.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1531,6 +1532,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--full-diff")) {
 		revs->diff = 1;
 		revs->full_diff = 1;
+	} else if (!prefixcmp(arg, "--full-diff=")) {
+		revs->diff = 1;
+		revs->full_diff = 1;
+		revs->full_diff_opt = arg + strlen("--full-diff=");
 	} else if (!strcmp(arg, "--full-history")) {
 		revs->simplify_history = 0;
 	} else if (!strcmp(arg, "--relative-date")) {
@@ -1819,6 +1824,20 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			revs->prune = 1;
 		if (!revs->full_diff)
 			diff_tree_setup_paths(revs->prune_data.raw, &revs->diffopt);
+		else if (revs->full_diff_opt) {
+			const char **argv = NULL;
+			int alloc = 0, nr = 0;
+			char *arg;
+
+			arg = xstrdup(revs->full_diff_opt);
+			sq_dequote_to_argv(arg, &argv, &nr, &alloc);
+
+			ALLOC_GROW(argv, nr + 1, alloc);
+			argv[nr] = NULL;
+			argv = get_pathspec(revs->prefix, argv);
+
+			diff_tree_setup_paths(argv, &revs->diffopt);
+		}
 	}
 	if (revs->combine_merges)
 		revs->ignore_merges = 0;
diff --git a/revision.h b/revision.h
index 6aa53d1..baa709c 100644
--- a/revision.h
+++ b/revision.h
@@ -137,6 +137,7 @@ struct rev_info {
 	const char	*subject_prefix;
 	int		no_inline;
 	int		show_log_size;
+	const char      *full_diff_opt;
 
 	/* Filter by commit log message */
 	struct grep_opt	grep_filter;
-- 
1.7.3.1.256.g2539c.dirty

[PATCH] log: allow to specify diff pathspec in addition to prune pathspec

[Nguyễn Thái Ngọc Duy]

Pathspec in "git log -p <pathspec>" is used for both commit pruning
and diff generation. If --full-diff is given, then diff pathspec is
reset to generate complete diff.

This patch gives more control to diff generation. The first pathspec
in "git log -p -- <pathspec> -- <pathspec>" is used as commit pruning
as usual. The second one is used for diff generation. So --full-diff
now is essentially "git log -p -- <pathspec> --".

This form requires specifying "--" twice. If a file name happens to be
"--", it may be misunderstood as the second "--" marker. This is an
unfortunate consequence for this syntax. Users can still use "./--" or
similar to workaround this.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Version 2. Now it looks more acceptable.

 Documentation/git-log.txt |    9 +++++++--
 revision.c                |   28 +++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 249fc87..8e00dbc 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -9,7 +9,7 @@ git-log - Show commit logs
 SYNOPSIS
 --------
 [verse]
-'git log' [<options>] [<since>..<until>] [[\--] <path>...]
+'git log' [<options>] [<since>..<until>] [[\--] <path>... [\-- <path>...]]
 
 DESCRIPTION
 -----------
@@ -52,11 +52,12 @@ OPTIONS
 	commit was reached.
 
 --full-diff::
-	Without this flag, "git log -p <path>..." shows commits that
+	Without this flag, `git log -p <path>...` shows commits that
 	touch the specified paths, and diffs about the same specified
 	paths.  With this, the full diff is shown for commits that touch
 	the specified paths; this means that "<path>..." limits only
 	commits, and doesn't limit diff for those commits.
+	It is equivalent to `git log -p \-- <path>... \--`.
 +
 Note that this affects all diff-based output types, e.g. those
 produced by --stat etc.
@@ -76,6 +77,10 @@ produced by --stat etc.
 +
 To prevent confusion with options and branch names, paths may need to
 be prefixed with "\-- " to separate them from options or refnames.
++
+If the second "\--" is found, the following pathspec is used to limit
+diff generation. Note that this affects all diff-based output types,
+e.g. those produced by --stat etc.
 
 include::rev-list-options.txt[]
 
diff --git a/revision.c b/revision.c
index 8764dde..f560647 100644
--- a/revision.c
+++ b/revision.c
@@ -1682,20 +1682,37 @@ static int handle_revision_pseudo_opt(const char *submodule,
  */
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
 {
-	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
+	int i, flags, left, read_from_stdin, got_rev_arg = 0;
+	int seen_dashdash, seen_second_dashdash;
 	struct cmdline_pathspec prune_data;
+	struct cmdline_pathspec diff_data;
 	const char *submodule = NULL;
 
 	memset(&prune_data, 0, sizeof(prune_data));
+	memset(&diff_data, 0, sizeof(diff_data));
 	if (opt)
 		submodule = opt->submodule;
 
 	/* First, search for "--" */
-	seen_dashdash = 0;
+	seen_dashdash = seen_second_dashdash = 0;
 	for (i = 1; i < argc; i++) {
+		int i2;
 		const char *arg = argv[i];
 		if (strcmp(arg, "--"))
 			continue;
+
+		/* Search for second "--" */
+		for (i2 = i + 1; i2 < argc; i2++) {
+			const char *arg = argv[i2];
+			if (strcmp(arg, "--"))
+				continue;
+			argv[i2] = NULL;
+			if (argv[i2 + 1])
+				append_prune_data(&diff_data, argv + i2 + 1);
+			seen_second_dashdash = 1;
+			break;
+		}
+
 		argv[i] = NULL;
 		argc = i;
 		if (argv[i + 1])
@@ -1817,7 +1834,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		/* Can't prune commits with rename following: the paths change.. */
 		if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 			revs->prune = 1;
-		if (!revs->full_diff)
+		if (seen_second_dashdash) {
+			ALLOC_GROW(diff_data.path, diff_data.nr+1, diff_data.alloc);
+			diff_data.path[diff_data.nr++] = NULL;
+			diff_tree_setup_paths(diff_data.path, &revs->diffopt);
+		}
+		else if (!revs->full_diff)
 			diff_tree_setup_paths(revs->prune_data.raw, &revs->diffopt);
 	}
 	if (revs->combine_merges)
-- 
1.7.4.74.g639db

Re: [PATCH] log: allow to specify diff pathspec in addition to prune pathspec

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Pathspec in "git log -p <pathspec>" is used for both commit pruning
> and diff generation. If --full-diff is given, then diff pathspec is
> reset to generate complete diff.
>
> This patch gives more control to diff generation.  The first pathspec in
> "git log -p -- <pathspec> -- <pathspec>" is used as commit pruning
> as usual. The second one is used for diff generation. So --full-diff
> now is essentially "git log -p -- <pathspec> --".

I agree that giving more control to diff generation is a good idea, and
this certainly is better than the previous round that nobody reviewed
before you rerolled this round.

But I have doubts about declaring "--full-diff is equivalent to giving the
'output' pathspec that is empty".

Have you thought about the interaction between this and -M/-C options?

Re: [PATCH] log: allow to specify diff pathspec in addition to prune pathspec

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This form requires specifying "--" twice. If a file name happens to be
> "--", it may be misunderstood as the second "--" marker. This is an
> unfortunate consequence for this syntax. Users can still use "./--" or
> similar to workaround this.

This is not a new "problem" (and it is probably not a problem to begin
with).  If you had "--" in the working tree and you wanted to treat it as
a path, you said either one of these:

    $ git log HEAD ./--
    $ git log HEAD -- --

The latter is what your patch breaks, I suspect.

Also it forces existing scripts and programs that knew "everything in this
array is a pathspec" and added "--" before adding the contents of the
array to form a command line to drive "git log" family of commands to be
updated.

Re: [PATCH] log: allow to specify diff pathspec in addition to prune pathspec

[Nguyen Thai Ngoc Duy]

2011/11/5 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Pathspec in "git log -p <pathspec>" is used for both commit pruning
>> and diff generation. If --full-diff is given, then diff pathspec is
>> reset to generate complete diff.
>>
>> This patch gives more control to diff generation.  The first pathspec in
>> "git log -p -- <pathspec> -- <pathspec>" is used as commit pruning
>> as usual. The second one is used for diff generation. So --full-diff
>> now is essentially "git log -p -- <pathspec> --".
>
> I agree that giving more control to diff generation is a good idea, and
> this certainly is better than the previous round that nobody reviewed
> before you rerolled this round.
>
> But I have doubts about declaring "--full-diff is equivalent to giving the
> 'output' pathspec that is empty".

The equivalent command should be "git log -p -- <pathspec> -- :/" (in
case you're in subdir). What doubts specifically?

> Have you thought about the interaction between this and -M/-C options?

No, yes I may have problems there.

> This is not a new "problem" (and it is probably not a problem to begin
> with).  If you had "--" in the working tree and you wanted to treat it as
> a path, you said either one of these:
>
>    $ git log HEAD ./--
>    $ git log HEAD -- --
>
> The latter is what your patch breaks, I suspect.
>
> Also it forces existing scripts and programs that knew "everything in this
> array is a pathspec" and added "--" before adding the contents of the
> array to form a command line to drive "git log" family of commands to be
> updated.

We could only recognize the second "--" when a new argument is given
so it won't break existing use. But I'll need to look at -C/-M first.
Smells like --follow problem.
-- 
Duy