public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Sergey Organov <sorganov@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: Jeff King <peff@peff•net>, git@vger•kernel.org
Subject: Re: [PATCH v3] revision: add separate field for "-m" of "diff-index -m"
Date: Mon, 31 Aug 2020 22:41:51 +0300	[thread overview]
Message-ID: <87wo1eob5s.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqq4koihgpk.fsf@gitster.c.googlers.com> (Junio C. Hamano's message of "Mon, 31 Aug 2020 10:23:51 -0700")

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

next prev parent reply	other threads:[~2020-08-31 19:41 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-29 19:46 [PATCH] revision: add separate field for "-m" of "diff-index -m" Sergey Organov
2020-08-29 20:11 ` [PATCH v2] " Sergey Organov
2020-08-31  4:49   ` Junio C Hamano
2020-08-31 12:45     ` Sergey Organov
2020-08-31 17:00       ` Junio C Hamano
2020-08-31 12:53 ` [PATCH v3] " Sergey Organov
2020-08-31 17:23   ` Junio C Hamano
2020-08-31 19:41     ` Sergey Organov [this message]
2020-08-31 20:14 ` [PATCH v4] " Sergey Organov
2020-08-31 20:42   ` Junio C Hamano
2020-08-31 21:01     ` Sergey Organov
2020-10-23  4:03   ` [PATCH] revision: wording tweak in comment for parsing "-m" Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wo1eob5s.fsf@osv.gnss.ru \
    --to=sorganov@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=peff@peff$(echo .)net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox