Michael J Gruber [off-list ref] writes:
Now, since external diff runs on smudged blobs, it appears as if we
mixed cleaned and smudged blobs when feeding external diffs; whereas
really, we mix "worktree blobs" and "smudged repo blobs", which is okay
as per our definition of clean/smudge: the difference is irrelevant by
definition.
It does not appear to "mix cleaned and smudged" to me (even though
before Dscho's commit that John pointed out, we did mix by mistake)
to me, but you arrived at the correct conclusion in the rest of your
sentence.
We treat "worktree files" and "smudged repo blobs" as "comparable"
because by definition the latter is what you get if you did a
"checkout" of the blob. Indeed, when we know a worktree file is an
unmodified checkout from a blob and we want to have a read-only
temporary file for a "smudged repo blob", we allow that worktree
file to be used as such.
So in that sense, the commit by Dscho that John pointed out earlier
was not something that changed the semantics; it merely made things
consistent (before that commit, we used to use clean version if we
do not have a usable worktree file).
It is a separate question which of clean or smudged an external diff
tool should be given to work on.
I still think that feeding cleaned blobs to external diff would be less
surprising (and should be the default, but maybe can't be changed any
more) and feeding smudged blobs should be the special case requiring a
special config.
Go back six years and make a review comment before 4e218f54 (Smudge
the files fed to external diff and textconv, 2009-03-21) was taken
;-). The argument against that commit may have gone like this:
* The current (that is, current as of 4e218f54^) code is
inconsistent, and your patch has a side effect of making it
consistent by always feeding smudged version.
* We however could make it consistent by always feeding clean
version (i.e. disable borrow-from-working-tree codepath when
driving external diff). And that gives us cleaner semantics; the
internal diff and external diff will both work on clean, not
smudged data.
* Of course, going the "clean" way would not help your cause of
allowing external diff to work on smudged version, so you would
need a separate patch on top of that "consistently feed 'clean'
version" fix to optionally allow "consistently feed 'smudge'
version" mode to help msysGit issue 177.
And I would have bought such an argument with 97% chance [*1*].
I do not think 6 years have changed things very much with respect to
the above three-bullet point argument, except that it would be too
late to set the default to 'clean' all of a sudden. So a plausible
way forward would be to
* introduce an option to feed 'clean' versions to external diff
drivers, perhaps with --ext-diff-clean=<driver> command line
option and GIT_EXTERNAL_DIFF_CLEAN environment variable, both of
which take precedence over existing --ext-diff/GIT_EXTERNAL_DIFF
* optionally add a configuration variable diff.feedCleanToExternal
that makes --ext-diff/GIT_EXTERNAL_DIFF behave as if their
'clean' siblings were given. Default it to false.
My gut feeling is that textconv should need a similar treatment for
consistency (after all, it goes through the same prepare_temp_file()
infrastructure).
[Footnote]
*1* The 3% reservation is that I am not entirely convinced that
"both internal and external get to work on the same 'clean'
representation gives us cleaner semantics" is always true.
Junio C Hamano venit, vidit, dixit 19.06.2015 19:03:
Michael J Gruber [off-list ref] writes:
quoted
Now, since external diff runs on smudged blobs, it appears as if we
mixed cleaned and smudged blobs when feeding external diffs; whereas
really, we mix "worktree blobs" and "smudged repo blobs", which is okay
as per our definition of clean/smudge: the difference is irrelevant by
definition.
It does not appear to "mix cleaned and smudged" to me (even though
before Dscho's commit that John pointed out, we did mix by mistake)
to me,
... neither to me. I appears as if you missed the past subjunctive ;)
but you arrived at the correct conclusion in the rest of your
sentence.
We treat "worktree files" and "smudged repo blobs" as "comparable"
because by definition the latter is what you get if you did a
"checkout" of the blob. Indeed, when we know a worktree file is an
unmodified checkout from a blob and we want to have a read-only
temporary file for a "smudged repo blob", we allow that worktree
file to be used as such.
So in that sense, the commit by Dscho that John pointed out earlier
was not something that changed the semantics; it merely made things
consistent (before that commit, we used to use clean version if we
do not have a usable worktree file).
It is a separate question which of clean or smudged an external diff
tool should be given to work on.
quoted
I still think that feeding cleaned blobs to external diff would be less
surprising (and should be the default, but maybe can't be changed any
more) and feeding smudged blobs should be the special case requiring a
special config.
Go back six years and make a review comment before 4e218f54 (Smudge
the files fed to external diff and textconv, 2009-03-21) was taken
;-). The argument against that commit may have gone like this:
* The current (that is, current as of 4e218f54^) code is
inconsistent, and your patch has a side effect of making it
consistent by always feeding smudged version.
* We however could make it consistent by always feeding clean
version (i.e. disable borrow-from-working-tree codepath when
driving external diff). And that gives us cleaner semantics; the
internal diff and external diff will both work on clean, not
smudged data.
* Of course, going the "clean" way would not help your cause of
allowing external diff to work on smudged version, so you would
need a separate patch on top of that "consistently feed 'clean'
version" fix to optionally allow "consistently feed 'smudge'
version" mode to help msysGit issue 177.
And I would have bought such an argument with 97% chance [*1*].
I do not think 6 years have changed things very much with respect to
the above three-bullet point argument, except that it would be too
late to set the default to 'clean' all of a sudden. So a plausible
way forward would be to
* introduce an option to feed 'clean' versions to external diff
drivers, perhaps with --ext-diff-clean=<driver> command line
option and GIT_EXTERNAL_DIFF_CLEAN environment variable, both of
which take precedence over existing --ext-diff/GIT_EXTERNAL_DIFF
* optionally add a configuration variable diff.feedCleanToExternal
that makes --ext-diff/GIT_EXTERNAL_DIFF behave as if their
'clean' siblings were given. Default it to false.
My gut feeling is that textconv should need a similar treatment for
consistency (after all, it goes through the same prepare_temp_file()
infrastructure).
[Footnote]
*1* The 3% reservation is that I am not entirely convinced that
"both internal and external get to work on the same 'clean'
representation gives us cleaner semantics" is always true.
With consistency stepping back behind compatibility, I don't expect any
defaults to change.
But a knob to change defaults would be nice, yes, and in that case for
external diff as well as textconv. A config variable should suffice
given that we have "git -c" these days.
Michael