threads / patch / 42716
Subject: [PATCH 0/3] diff: add --relative-names
Nguyễn Thái Ngọc DuyFor 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
Nguyễn Thái Ngọc Duy[PATCH 1/3] diff.c: refactor strip_prefix()
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
Nguyễn Thái Ngọc Duy[PATCH 2/3] diff.c: separate "prefix" from RELATIVE_NAME (aka --relative)
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
Nguyễn Thái Ngọc Duy[PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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(-)
Documentation/diff-options.txt, diff.c, diff.h
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
Eric SunshineRe: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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>
Jeff KingRe: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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
Duy NguyenRe: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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
Jeff KingRe: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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
Duy NguyenRe: [PATCH/RFC 3/3] diff.c: add --relative-names to be used with --name-only
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