On Sun, 2017-03-19 at 15:14 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <[email protected]> writes:
>
> > diff --git a/diff.c b/diff.c
> > index be11e4ef2b..2afecfb939 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s,
> > unsigned int flags)
> > s->size = xsize_t(st.st_size);
> > if (!s->size)
> > goto empty;
> > - if (S_ISLNK(st.st_mode)) {
> > + if (S_ISLNK(s->mode)) {
> > struct strbuf sb = STRBUF_INIT;
> >
> > if (strbuf_readlink(&sb, s->path, s->size))
> > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s,
> > unsigned int flags)
> > s->should_free = 1;
> > return 0;
> > }
> > + if (S_ISLNK(st.st_mode)) {
> > + stat(s->path, &st);
> > + s->size = xsize_t(st.st_size);
>
> Doesn't this affect --no-index mode? We never need to do a wasteful
> stat() after lstat() and we are penalizing the normal codepath with
> this change, no?
the S_ISLNK(s->mode) conditional above is for the normal codepath,
which returns early. So the stat I added is only done for symlinks in
no_index mode.
> > @@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
> > else if (!strcmp(arg, "--no-follow")) {
> > DIFF_OPT_CLR(options, FOLLOW_RENAMES);
> > DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
> > - } else if (!strcmp(arg, "--color"))
> > + } else if (!strcmp(arg, "--dereference"))
> > + DIFF_OPT_SET(options, DEREFERENCE);
> > + else if (!strcmp(arg, "--no-dereference"))
> > + DIFF_OPT_CLR(options, DEREFERENCE);
> > + else if (!strcmp(arg, "--color"))
> > options->use_color = 1;
>
> Also shouldn't be some code to detect --[no-]dereference options
> given when --no-index is not in effect and error out? As the patch
> title says, this change should be a no-op for normal codepath and
> only affect the no-index hack.
But erroring out isn't a no-op. With the current patch you can do
--dereference without --no-index and it simply wouldn't affect
anything.
I don't mind either way, so I'll make it error out.
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
