On Sunday, April 21, 2024 2:18 PM, Junio C Hamano wrote: >René Scharfe <l.s.r@xxxxxx> writes: > >> You can more easily reproduce it by setting the environment variable >> GIT_EXTERNAL_DIFF or the configuration setting diff.external -- no >> attributes needed. > >Indeed. > >A much simpler fix may be to declare that these two features are imcompatible and >fail the execution upfront, instead of just silently ignoring one of the two options. > >As a person who is very much used to the external diff not contributing to the exit >status (who also invented the external diff driver interface), I would be a wrong >person to judge if such a simplified approach is desirable, of course, but just >throwing it out as a food for thought. My suggestion would be to keep with a priority approach, where GIT_EXTERNAL_DIFF overrides diff.external, assuming they set hold to the same specification (the git-config page implies they do) and GIT_EXTERNAL_DIFF overrides diff.external as I would expect. The behaviour of the two should be identical and definitely not in conflict. From my experience with this feature, and what I would prefer to preserve, is a formal separation between the diff engine and the underlying textconv. If the textconv processor fails in some way, the git diff should also fail, but the exit code should reflect that git diff failed, not the status of the textconv processor. If the external diff wrapper fails, the completion should be passed up since git would/should/could delegate the diff processing to the external engine. The only real completion code requirement I see is that 0 = no difference, 1 = difference exists, other is an indeterminate failure. I might be restating the obvious, but this is an important capability, at least to me and my teams. We use this extensively for binary content diffs. It is also pretty important for structured document (e.g., MS-Word, RTF, PDF) and executable diffs that git does not handle out of the box. --Randall
