On Wed, Dec 16, 2020 at 05:27:13PM -0800, Junio C Hamano wrote:
> Johannes Altmanninger <aclopte@xxxxxxxxx> writes:
> > The diff_from_contents bit means that we have to look at content
> > changes to know if a path has changed - modulo ignored hunks.
>
> But your sentence without " - modulo ignored hunks" says that very
> clearly. So perhaps these three words can just go away?
My bad, this should rather be
The diff_from_contents bit means that we have to look at content
changes (modulo ignored hunks) to know if a path has changed.
probably dropping the three words makes it even less confusing.
> It's not like we were buggy when diff_from_contents bit is in effect
> for all output formats, is it?
Right, it's useless to set the bit here.
> > + opt->color_moved = 0;
>
> Why is color_moved so special? If we added some other new option,
> how would we make sure that the person who is adding that new option
> would remember to reset it here? And how does that person decide if
> his or her new option needs resetting or not in the first place?
I copied color_moved=0 from the DIFF_FORMAT_NO_OUTPUT section of
diff_flush(). It happens to work correctly in both places because no diff
contents are printed, but yeah it's not robust to future changes.
> Also I am not sure if "caching" the file handle to /dev/null is
> good idea or not. When will it be closed? Would repeated
> invocation of the diff machinery work well with it (think: "git log
> -w --name-only")
I believe that repeated invocations work fine, because the file is never
closed (which may be a problem?). The file could be made nilable if we still
need something like that.
> This somehow feels wrong. For one thing, RAW is about showing
> object names of preimage and postimage, which means it shows the
> preimage and postimage are either identical or different, and
> options like -w, -I, etc. that inspect and hide some textual
> differences should not affect its outcome at all.
Yeah, I realized late that -w --raw will have sort of unintuitive semantics.
This combination is probably rare but I guess users could set an alias for
diff -w.
> I am tempted to declare that just like "raw", "name-only" and
> "name-status" formats work with byte-for-byte equality
OK, I think it's better to keep the current (consistent) behavior. I don't
think it's worth to special-case "--name-only".
It's easy to read the names from a "diff -I" output anyway. This filter is
broken in various ways, but works well enough in practise:
diff -I | perl -ne 'print "$1\n" if /\+{3} b\/(.*)/'
> and "-w" and friends are ignored just like in "diff --name-status --patch"
> the "--patch" option is ignored.
For interactive use, it would be more helpful to reject useless options
instead of ignoring them. Though ignoring is probably desirable to allow
liberal use of these options, I'm not sure.
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
>
> Subject: [PATCH] diff: correct interaction between --exit-code and -I<pattern>
Looks good.
> +test_expect_success '-w and --exit-code interact sensibly' '
Maybe 'exit with 0 when all changes are ignored by -w' though either version
is fine because I think the intention of the test is already obvious.
