| Message ID | 20200831125350.26472-1-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] revision: add separate field for "-m" of "diff-index -m" | expand |
Sergey Organov <sorganov@gmail.com> writes: > Add separate 'diff_index_match_missing' field for diff-index to use and set it > when we encounter "-m" option. This field won't then be cleared when another > meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be > affected by future option(s) that might drive 'ignore_merges' field. > > Use this new field from diff-lib:do_oneway_diff() instead of reusing > 'ignore_merges' field. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- Much easier to reason about. As I said, I think we would ideally want to detect and diagnose --[no-]diff-merges on the command line of "diff" or "diff-{files,index,trees}" as an error, but for now this is a good first step. > } else if (!strcmp(arg, "-m")) { > revs->ignore_merges = 0; > + /* > + * Backward compatibility wart - "diff-index -m" does > + * not mean "do not ignore merges", but "match_missing", > + * so set separate flag for it. > + */ > + revs->diff_index_match_missing = 1; Half the wart has been removed thanks to this patch and the rest of the code can look at the right field for their purpose. The parsing, unless we make a bigger change that allows us to detect and diagnose "diff-index --no-diff-merges" as an error, still needs to be tricky and may deserve a comment. The comment should apply to and treat both fields equally, perhaps like this: } else if (!strcmp(arg, "-m")) { /* * To "diff-index", "-m" means "match missing", and to * the "log" family of commands, it means "keep merges". * Set both fields appropriately. */ revs->ignore_merges = 0; revs->match_missing = 1; } By the way, let's drop diff_index_ prefix from the name of the new field. I do not see a strong reason to object to a possible update to "diff-files" to match the behaviour of "diff-index". In a sparsely checked out working tree (e.g. start from "clone --no-checkout"), you can check out only paths that you want to modify, edit them, and then "git diff-files -m" would be able to show useful result without having to show deletions to all other files you are not interested in. And that is exactly the same use case as "git diff-index -m HEAD" was invented for. Thanks. > diff --git a/revision.h b/revision.h > index c1e5bcf139d7..5ae8254ffaed 100644 > --- a/revision.h > +++ b/revision.h > @@ -188,6 +188,7 @@ struct rev_info { > unsigned int diff:1, > full_diff:1, > show_root_diff:1, > + diff_index_match_missing:1, > no_commit_id:1, > verbose_header:1, > combine_merges:1,
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Add separate 'diff_index_match_missing' field for diff-index to use >> and set it >> when we encounter "-m" option. This field won't then be cleared when another >> meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be >> affected by future option(s) that might drive 'ignore_merges' field. >> >> Use this new field from diff-lib:do_oneway_diff() instead of reusing >> 'ignore_merges' field. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- > > Much easier to reason about. As I said, I think we would ideally > want to detect and diagnose --[no-]diff-merges on the command line > of "diff" or "diff-{files,index,trees}" as an error, but for now > this is a good first step. > >> } else if (!strcmp(arg, "-m")) { >> revs->ignore_merges = 0; >> + /* >> + * Backward compatibility wart - "diff-index -m" does >> + * not mean "do not ignore merges", but "match_missing", >> + * so set separate flag for it. >> + */ >> + revs->diff_index_match_missing = 1; > > Half the wart has been removed thanks to this patch and the rest of > the code can look at the right field for their purpose. The parsing, > unless we make a bigger change that allows us to detect and diagnose > "diff-index --no-diff-merges" as an error, still needs to be tricky > and may deserve a comment. > > The comment should apply to and treat both fields equally, perhaps > like this: > > } else if (!strcmp(arg, "-m")) { > /* > * To "diff-index", "-m" means "match missing", and to > * the "log" family of commands, it means "keep merges". > * Set both fields appropriately. > */ > revs->ignore_merges = 0; > revs->match_missing = 1; > } > > By the way, let's drop diff_index_ prefix from the name of the new > field. I do not see a strong reason to object to a possible update > to "diff-files" to match the behaviour of "diff-index". > > In a sparsely checked out working tree (e.g. start from "clone > --no-checkout"), you can check out only paths that you want to > modify, edit them, and then "git diff-files -m" would be able to > show useful result without having to show deletions to all other > files you are not interested in. And that is exactly the same use > case as "git diff-index -m HEAD" was invented for. Fine with me, thanks! I'll re-roll soon. Thanks, -- Sergey
diff --git a/diff-lib.c b/diff-lib.c index 50521e2093fc..f2aee78e7aa2 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -405,14 +405,8 @@ static void do_oneway_diff(struct unpack_trees_options *o, /* if the entry is not checked out, don't examine work tree */ cached = o->index_only || (idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx))); - /* - * Backward compatibility wart - "diff-index -m" does - * not mean "do not ignore merges", but "match_missing". - * - * But with the revision flag parsing, that's found in - * "!revs->ignore_merges". - */ - match_missing = !revs->ignore_merges; + + match_missing = revs->diff_index_match_missing; if (cached && idx && ce_stage(idx)) { struct diff_filepair *pair; diff --git a/revision.c b/revision.c index 96630e31867d..64b16f7d1033 100644 --- a/revision.c +++ b/revision.c @@ -2345,6 +2345,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->diffopt.flags.tree_in_recursive = 1; } else if (!strcmp(arg, "-m")) { revs->ignore_merges = 0; + /* + * Backward compatibility wart - "diff-index -m" does + * not mean "do not ignore merges", but "match_missing", + * so set separate flag for it. + */ + revs->diff_index_match_missing = 1; } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) { if (!strcmp(optarg, "off")) { revs->ignore_merges = 1; diff --git a/revision.h b/revision.h index c1e5bcf139d7..5ae8254ffaed 100644 --- a/revision.h +++ b/revision.h @@ -188,6 +188,7 @@ struct rev_info { unsigned int diff:1, full_diff:1, show_root_diff:1, + diff_index_match_missing:1, no_commit_id:1, verbose_header:1, combine_merges:1,
Add separate 'diff_index_match_missing' field for diff-index to use and set it when we encounter "-m" option. This field won't then be cleared when another meaning of "-m" is reverted (e.g., by "--no-diff-merges"), nor it will be affected by future option(s) that might drive 'ignore_merges' field. Use this new field from diff-lib:do_oneway_diff() instead of reusing 'ignore_merges' field. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- v3: improve commit message v2: rebased from 'maint' onto 'master' diff-lib.c | 10 ++-------- revision.c | 6 ++++++ revision.h | 1 + 3 files changed, 9 insertions(+), 8 deletions(-)