[PATCH 0/3] diff: add --relative-names

[PATCH 0/3] diff: add --relative-names

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

For more explanation head straight to 3/3. The first two patches are
preparation.

Nguyễn Thái Ngọc Duy (3):
  diff.c: refactor strip_prefix()
  diff.c: separate "prefix" from RELATIVE_NAME (aka --relative)
  diff.c: add --relative-names to be used with --name-only

 Documentation/diff-options.txt |  4 ++++
 diff.c                         | 54 +++++++++++++++++++++++++++++-------------
 diff.h                         |  1 +
 3 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.8.2.531.gd073806

[PATCH 1/3] diff.c: refactor strip_prefix()

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

By passing "struct diff_options *" to strip_prefix(), we can do some
more intelligent and repeated logic at one place. The removal of
"if (opt->prefix_length)" is just the beginning.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index fa78fc1..b4949a2 100644
--- a/diff.c
+++ b/diff.c
@@ -3147,8 +3147,15 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
 		hashclr(one->sha1);
 }
 
-static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
+static void strip_prefix(struct diff_options *opt,
+			 const char **namep,
+			 const char **otherp)
 {
+	int prefix_length = opt->prefix_length;
+
+	if (!prefix_length)
+		return;
+
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
 	if (*namep && **namep != '/') {
 		*namep += prefix_length;
@@ -3175,8 +3182,7 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 	name  = p->one->path;
 	other = (strcmp(name, p->two->path) ? p->two->path : NULL);
 	attr_path = name;
-	if (o->prefix_length)
-		strip_prefix(o->prefix_length, &name, &other);
+	strip_prefix(o, &name, &other);
 
 	if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
 		pgm = NULL;
@@ -3230,8 +3236,7 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
 	name = p->one->path;
 	other = (strcmp(name, p->two->path) ? p->two->path : NULL);
 
-	if (o->prefix_length)
-		strip_prefix(o->prefix_length, &name, &other);
+	strip_prefix(o, &name, &other);
 
 	diff_fill_sha1_info(p->one);
 	diff_fill_sha1_info(p->two);
@@ -3254,8 +3259,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	other = (strcmp(name, p->two->path) ? p->two->path : NULL);
 	attr_path = other ? other : name;
 
-	if (o->prefix_length)
-		strip_prefix(o->prefix_length, &name, &other);
+	strip_prefix(o, &name, &other);
 
 	diff_fill_sha1_info(p->one);
 	diff_fill_sha1_info(p->two);
@@ -4133,14 +4137,14 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
 		const char *name_a, *name_b;
 		name_a = p->one->path;
 		name_b = p->two->path;
-		strip_prefix(opt->prefix_length, &name_a, &name_b);
+		strip_prefix(opt, &name_a, &name_b);
 		write_name_quoted(name_a, opt->file, inter_name_termination);
 		write_name_quoted(name_b, opt->file, line_termination);
 	} else {
 		const char *name_a, *name_b;
 		name_a = p->one->mode ? p->one->path : p->two->path;
 		name_b = NULL;
-		strip_prefix(opt->prefix_length, &name_a, &name_b);
+		strip_prefix(opt, &name_a, &name_b);
 		write_name_quoted(name_a, opt->file, line_termination);
 	}
 }
@@ -4345,7 +4349,7 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 		const char *name_a, *name_b;
 		name_a = p->two->path;
 		name_b = NULL;
-		strip_prefix(opt->prefix_length, &name_a, &name_b);
+		strip_prefix(opt, &name_a, &name_b);
 		write_name_quoted(name_a, opt->file, opt->line_termination);
 	}
 }
-- 
2.8.2.531.gd073806

[PATCH 2/3] diff.c: separate "prefix" from RELATIVE_NAME (aka --relative)

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

So far "prefix" and "prefix_length" in struct diff_options are tied to
RELATIVE_NAME flag, making it tricky to add new features that need the
prefix.

This change essentially follows the foot steps in cd676a5 (diff
--relative: output paths as relative to the current subdirectory -
2008-02-12) and makes sure that the prefix stripping/filtering only
happens when RELATIVE_NAME flag is set. The stripping is much simpler
because all stripping now goes through strip_prefix(). So the patch is
mostly about filtering.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index b4949a2..bd5d190 100644
--- a/diff.c
+++ b/diff.c
@@ -3153,7 +3153,7 @@ static void strip_prefix(struct diff_options *opt,
 {
 	int prefix_length = opt->prefix_length;
 
-	if (!prefix_length)
+	if (!prefix_length || !DIFF_OPT_TST(opt, RELATIVE_NAME))
 		return;
 
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
@@ -3335,8 +3335,6 @@ void diff_setup_done(struct diff_options *options)
 	if (DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		options->detect_rename = DIFF_DETECT_COPY;
 
-	if (!DIFF_OPT_TST(options, RELATIVE_NAME))
-		options->prefix = NULL;
 	if (options->prefix)
 		options->prefix_length = strlen(options->prefix);
 	else
@@ -4976,7 +4974,8 @@ void diff_addremove(struct diff_options *options,
 		addremove = (addremove == '+' ? '-' :
 			     addremove == '-' ? '+' : addremove);
 
-	if (options->prefix &&
+	if (DIFF_OPT_TST(options, RELATIVE_NAME) &&
+	    options->prefix &&
 	    strncmp(concatpath, options->prefix, options->prefix_length))
 		return;
 
@@ -5021,7 +5020,8 @@ void diff_change(struct diff_options *options,
 			new_dirty_submodule = tmp;
 	}
 
-	if (options->prefix &&
+	if (DIFF_OPT_TST(options, RELATIVE_NAME) &&
+	    options->prefix &&
 	    strncmp(concatpath, options->prefix, options->prefix_length))
 		return;
 
@@ -5048,7 +5048,8 @@ struct diff_filepair *diff_unmerge(struct diff_options *options, const char *pat
 	struct diff_filepair *pair;
 	struct diff_filespec *one, *two;
 
-	if (options->prefix &&
+	if (DIFF_OPT_TST(options, RELATIVE_NAME) &&
+	    options->prefix &&
 	    strncmp(path, options->prefix, options->prefix_length))
 		return NULL;
 
-- 
2.8.2.531.gd073806

[PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

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

The difference with --relative option is, this option does not filter
paths outside cwd. You can add two more chars " ." on your command
line for that.

This serves two purposes

 - user friendlier to copy/paste. When full paths are shown, you can
   still use ":/" magic to get around it, but unless you're a lefty
   you need to move your right hand between the keyboard and mouse for
   each path. Not very efficient.

 - script friendly. Strictly speaking a very light form of scripting
   from command line prompt when you just pipe paths to the next
   command. Paths relative to cwd would be much easier to pip this
   way.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 It would be great if --relative could be used for this (and it feels
 weird that the option performs both actions at once, stripping _and_
 filtering where filtering could easily be done with pathspec). But
 it's too late to change --relative behavior now.

 Documentation/diff-options.txt |  4 ++++
 diff.c                         | 21 ++++++++++++++++++---
 diff.h                         |  1 +
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index d9ae681..1554008 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -481,6 +481,10 @@ ifndef::git-format-patch[]
 	not in a subdirectory (e.g. in a bare repository), you
 	can name which subdirectory to make the output relative
 	to by giving a <path> as an argument.
+
+--relative-names::
+	When used with `--name-only`, paths are shown relative to
+	current working directory.
 endif::git-format-patch[]
 
 -a::
diff --git a/diff.c b/diff.c
index bd5d190..3a938ac 100644
--- a/diff.c
+++ b/diff.c
@@ -3335,6 +3335,13 @@ void diff_setup_done(struct diff_options *options)
 	if (DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		options->detect_rename = DIFF_DETECT_COPY;
 
+	/*
+	 * --relative can change "prefix", which may cause unintended
+	 * consequences for --relative-names
+	 */
+	if (DIFF_OPT_TST(options, RELATIVE_NAME) && options->relative_names)
+		die(_("--relative and --relative-names are mutually exclusive"));
+
 	if (options->prefix)
 		options->prefix_length = strlen(options->prefix);
 	else
@@ -3792,7 +3799,8 @@ int diff_opt_parse(struct diff_options *options,
 	else if (skip_prefix(arg, "--relative=", &arg)) {
 		DIFF_OPT_SET(options, RELATIVE_NAME);
 		options->prefix = arg;
-	}
+	} else if (!strcmp(arg, "--relative-names"))
+		options->relative_names = 1;
 
 	/* xdiff options */
 	else if (!strcmp(arg, "--minimal"))
@@ -4347,8 +4355,15 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 		const char *name_a, *name_b;
 		name_a = p->two->path;
 		name_b = NULL;
-		strip_prefix(opt, &name_a, &name_b);
-		write_name_quoted(name_a, opt->file, opt->line_termination);
+		if (opt->relative_names)
+			write_name_quoted_relative(name_a,
+						   opt->prefix, opt->file,
+						   opt->line_termination);
+		else {
+			strip_prefix(opt, &name_a, &name_b);
+			write_name_quoted(name_a, opt->file,
+					  opt->line_termination);
+		}
 	}
 }
 
diff --git a/diff.h b/diff.h
index 125447b..ad6423d 100644
--- a/diff.h
+++ b/diff.h
@@ -139,6 +139,7 @@ struct diff_options {
 	int dirstat_permille;
 	int setup;
 	int abbrev;
+	int relative_names;
 /* white-space error highlighting */
 #define WSEH_NEW 1
 #define WSEH_CONTEXT 2
-- 
2.8.2.531.gd073806

Re: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

[Eric Sunshine]

On Sun, Jun 26, 2016 at 1:16 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> The difference with --relative option is, this option does not filter
> paths outside cwd. You can add two more chars " ." on your command
> line for that.
>
> This serves two purposes
>
>  - user friendlier to copy/paste. When full paths are shown, you can
>    still use ":/" magic to get around it, but unless you're a lefty
>    you need to move your right hand between the keyboard and mouse for
>    each path. Not very efficient.
>
>  - script friendly. Strictly speaking a very light form of scripting
>    from command line prompt when you just pipe paths to the next
>    command. Paths relative to cwd would be much easier to pip this

s/pip/pipe/

>    way.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Re: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

[Jeff King]

On Sun, Jun 26, 2016 at 07:16:16PM +0200, Nguyễn Thái Ngọc Duy wrote:

> The difference with --relative option is, this option does not filter
> paths outside cwd. You can add two more chars " ." on your command
> line for that.

Another difference seems to be that it applies only to --name-only, and
not to other forms. I can see how "-p --relative-names" might be weird,
because you'll get:

  diff --git a/../foo/bar b/../foo/bar

or something. But surely things like --name-status would want to support
it?

>  It would be great if --relative could be used for this (and it feels
>  weird that the option performs both actions at once, stripping _and_
>  filtering where filtering could easily be done with pathspec). But
>  it's too late to change --relative behavior now.

I suspect the ".." weirdness above is one of the reasons that --relative
restricted itself to the current directory (but regardless, I agree that
we cannot change it now).

-Peff

Re: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

[Duy Nguyen]

On Mon, Jun 27, 2016 at 9:24 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Jun 26, 2016 at 07:16:16PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> The difference with --relative option is, this option does not filter
>> paths outside cwd. You can add two more chars " ." on your command
>> line for that.
>
> Another difference seems to be that it applies only to --name-only, and
> not to other forms. I can see how "-p --relative-names" might be weird,
> because you'll get:
>
>   diff --git a/../foo/bar b/../foo/bar
>
> or something. But surely things like --name-status would want to support
> it?

That's my plan :) Anything that does not start with a/ or b/ should
respect this new option.
-- 
Duy

Re: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

[Jeff King]

On Mon, Jun 27, 2016 at 09:33:18PM +0200, Duy Nguyen wrote:

> On Mon, Jun 27, 2016 at 9:24 PM, Jeff King <peff@peff.net> wrote:
> > On Sun, Jun 26, 2016 at 07:16:16PM +0200, Nguyễn Thái Ngọc Duy wrote:
> >
> >> The difference with --relative option is, this option does not filter
> >> paths outside cwd. You can add two more chars " ." on your command
> >> line for that.
> >
> > Another difference seems to be that it applies only to --name-only, and
> > not to other forms. I can see how "-p --relative-names" might be weird,
> > because you'll get:
> >
> >   diff --git a/../foo/bar b/../foo/bar
> >
> > or something. But surely things like --name-status would want to support
> > it?
> 
> That's my plan :) Anything that does not start with a/ or b/ should
> respect this new option.

I guess it seems a bit unfortunate that:

  git log -p --relative-names .

would not be equivalent to:

  git log -p --relative

I am not entirely convinced that "a/../Documentation/git.txt" is that
unreasonable. Sure, it looks weird, and probably nobody will be able to
apply the patch. But it is what the user asked for.

-Peff

Re: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only

[Duy Nguyen]

On Mon, Jun 27, 2016 at 9:35 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 27, 2016 at 09:33:18PM +0200, Duy Nguyen wrote:
>
>> On Mon, Jun 27, 2016 at 9:24 PM, Jeff King <peff@peff.net> wrote:
>> > On Sun, Jun 26, 2016 at 07:16:16PM +0200, Nguyễn Thái Ngọc Duy wrote:
>> >
>> >> The difference with --relative option is, this option does not filter
>> >> paths outside cwd. You can add two more chars " ." on your command
>> >> line for that.
>> >
>> > Another difference seems to be that it applies only to --name-only, and
>> > not to other forms. I can see how "-p --relative-names" might be weird,
>> > because you'll get:
>> >
>> >   diff --git a/../foo/bar b/../foo/bar
>> >
>> > or something. But surely things like --name-status would want to support
>> > it?
>>
>> That's my plan :) Anything that does not start with a/ or b/ should
>> respect this new option.
>
> I guess it seems a bit unfortunate that:
>
>   git log -p --relative-names .
>
> would not be equivalent to:
>
>   git log -p --relative
>
> I am not entirely convinced that "a/../Documentation/git.txt" is that
> unreasonable. Sure, it looks weird, and probably nobody will be able to
> apply the patch. But it is what the user asked for.

If we go that far I would drop a/ and b/ because this is already
un-patchable, and without the prefix, the user can  happily copy and
paste again. Gaah lots more work..
-- 
Duy