Junio C Hamano venit, vidit, dixit 19.06.2015 19:03: > Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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, ... 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. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in