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

From: Sergey Organov <[email-masked]>
Date: Tue, 18 May 2021 00:00:15 +0300

Junio C Hamano <[email-masked]> writes:

> Sergey Organov <[email-masked]> writes:
>
>> 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