| Message ID | 20210517155818.32224-6-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/9] t4013: test that "-m" alone has no effect in "git log" | expand |
Sergey Organov <sorganov@gmail.com> 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 ;-)
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> 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 ;-) Well, I'm not sure. If it's a problem, I think I can add -c/--cc parsing to diff-index that will simply imply -p. This way we will be more backward-compatible. Thanks, -- Sergey Organov.
Sergey Organov <sorganov@gmail.com> 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).
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> 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
diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 176fe7ff2b4e..cf09559e422d 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -2,6 +2,7 @@ #include "cache.h" #include "config.h" #include "diff.h" +#include "diff-merges.h" #include "commit.h" #include "revision.h" #include "builtin.h" @@ -27,6 +28,12 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) rev.abbrev = 0; prefix = precompose_argv_prefix(argc, argv, prefix); + /* + * We need no diff for merges options, and we need to avoid conflict + * with our own meaning of "-m". + */ + diff_merges_suppress_options_parsing(); + argc = setup_revisions(argc, argv, &rev, NULL); for (i = 1; i < argc; i++) { const char *arg = argv[i]; @@ -35,6 +42,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) option |= DIFF_INDEX_CACHED; else if (!strcmp(arg, "--merge-base")) option |= DIFF_INDEX_MERGE_BASE; + else if (!strcmp(arg, "-m")) + rev.match_missing = 1; else usage(diff_cache_usage); } diff --git a/diff-merges.c b/diff-merges.c index f3a9daed7e05..9ca00cdd0cc6 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -6,6 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *); static void set_separate(struct rev_info *revs); static diff_merges_setup_func_t set_to_default = set_separate; +static int suppress_parsing; static void suppress(struct rev_info *revs) { @@ -30,17 +31,6 @@ static void set_first_parent(struct rev_info *revs) revs->first_parent_merges = 1; } -static void set_m(struct rev_info *revs) -{ - /* - * To "diff-index", "-m" means "match missing", and to the "log" - * family of commands, it means "show default diff for merges". Set - * both fields appropriately. - */ - set_to_default(revs); - revs->match_missing = 1; -} - static void set_combined(struct rev_info *revs) { suppress(revs); @@ -101,14 +91,22 @@ int diff_merges_config(const char *value) return 0; } +void diff_merges_suppress_options_parsing(void) +{ + suppress_parsing = 1; +} + int diff_merges_parse_opts(struct rev_info *revs, const char **argv) { int argcount = 1; const char *optarg; const char *arg = argv[0]; + if (suppress_parsing) + return 0; + if (!strcmp(arg, "-m")) { - set_m(revs); + set_to_default(revs); } else if (!strcmp(arg, "-c")) { set_combined(revs); revs->combined_imply_patch = 1; @@ -155,6 +153,9 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs) void diff_merges_setup_revs(struct rev_info *revs) { + if (suppress_parsing) + return; + if (revs->combine_merges == 0) revs->dense_combined_merges = 0; if (revs->separate_merges == 0) diff --git a/diff-merges.h b/diff-merges.h index 09d9a6c9a4fb..b5d57f6563e3 100644 --- a/diff-merges.h +++ b/diff-merges.h @@ -11,6 +11,8 @@ struct rev_info; int diff_merges_config(const char *value); +void diff_merges_suppress_options_parsing(void); + int diff_merges_parse_opts(struct rev_info *revs, const char **argv); void diff_merges_suppress(struct rev_info *revs);
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 new diff_merges_suppress_options_parsing() could then be reused and called before invocations of setup_revisions() for other commands that don't need --diff-merges options, but that's outside of the scope of these patch series. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- builtin/diff-index.c | 9 +++++++++ diff-merges.c | 25 +++++++++++++------------ diff-merges.h | 2 ++ 3 files changed, 24 insertions(+), 12 deletions(-)