Re: [PATCH v1 5/9] diff-merges: move specific diff-index "-m" handling to diff-index

Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

Move specific handling of "-m" for diff-index to diff-index.c, so
diff-merges is left to handle only diff for merges options.

Being a better design by itself, this is especially essential in
preparation for letting -m imply -p, as "diff-index -m" obviously
should not imply -p, as it's entirely unrelated.

To handle this, in addition to moving specific diff-index "-m" code
out of diff-merges, we introduce new

  diff_merges_suppress_options_parsing()

and call it before generic options processing in cmd_diff_index().

This change has a small but obvious fallout.

    $ git diff-index -c --cached HEAD^

now starts failing loudly.  Earlier, it silently fell back to
"combined" diff of one parent, which is "-p".

I think the end result is good (and luckily, "DIFF FORMAT FOR
MERGES" section explicitly limits "-c" and "--cc" to diff-tree,
diff-files and diff (and by implication excludes diff-index) so I am
sure there are small but non-zero number of people somewhere in the
world who has "diff-index -c" in their scripts that suddenly starts
failing with the version of Git with this change, but we can just
say their use was broken ;-)

Having said all that, I have to wonder if it still is needed to keep
the "diff-index -m" working, or we would be better off breaking it
to avoid a change like this that makes us bend over backwards to
work around the command line parsing infrastructure.

The only reason why "diff-index -m" exists is because it was part of
the idea Linus had for the merge implementation that we ended up
deciding not taking, where merges and possibly other bulk operations
that would affect the working tree is done in a separate, temporary
directory that is sparsely populated, the user is asked to edit away
conflicts in the temporary directory and expected to monitor his or
her own progress using "diff-index -m".  Our plan was to populate
such a temporary directory with only paths that are involved in the
operation in progress, without instantiating paths that are not
touched, so "treat missing files as if they haven't been modified"
was a handy ingredient for such a mode of operation.

But we ended up going with a different design, in which the main
working tree area is used to perform merges and to resolve
conflicts, which made this "pretend missing files as unmodified"
unnecessary feature.  In the end, we made a good move, as the
current design allows users to verify their changes in the context
of a full checkout (e.g. "make" would not have been a good way to
validate the conflict resolution if it is done in a separate
temporary directory that is sparsely populated with only the paths
involved in the merge---you need all files for building, including
the ones that are not modified, and "make" does not know to treat
missing files as if they are unmodified).

This could be a good thing to do, but as I wrote in the description,
there are in fact other commands that don't need diff-merges options and
currently just eat them silently instead of bailing out.

It's likely that 90% of uses of setup_revisions() don't need
diff-merges, so a feature to exclude diff-merges parsing seems to be
useful by itself.

Thanks,

-- Sergey Organov