Re: [PATCH] diffcore-pickaxe doc: document -S and -G properly

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 14, 2013, 18:44 UTC
Message-ID: <CALkWK0nR1=Pgv0AY78p7n17C-VVvbc6BfJ4y_Df1ciZoc-xR-Q@mail.gmail.com>
In-Reply-To: <CABURp0r808KeSRwhgPw98vUC+JSErvSYHFyAvn-=8JhQzn8e1Q@mail.gmail.com>

Phil Hord wrote:

Show 5 quoted lines

> References to git-log seem out of place to me here in git-diffcore.  I
> know it's only an example, but it seems that Git normally describes
> these 'reference selectors' more generically.  The generic description
> may be more confusing to new users, but this patch is not the place to
> consider whether it should change.

It's not for new users at all. The most useful application of -S and -G is in log. The translation from a log -G to a diffcore -G is not obvious at all, and warrants an explanation.

Oh, and for the user. No user is going to type out `man gitdiffcore` out of the blue: she's most probably led there from log, and we're connecting the dots for her.

> While the switches are called -S and -G, I do not think it is helpful
> to name the two pickaxe options as "the S kind" and "the G kind".

How do you describe something precisely without loss of meaning? You stop abstracting unnecessarily. Read the sources: you will literally see DIFF_PICKAXE_KIND_G there.

Show 7 quoted lines

>> +
>> +The S kind detects filepairs whose "result" side and "origin" side
>> +have different number of occurrences of specified string.  While
>> +rename detection works as usual, 'git log -S' cannot omit commits
>
> The "cannot omit" feels like a confusing double-negative.  How about
> "includes" instead?

Intended.  Omission is expected.

> Is it worth mentioning that something in the documentation is "worth
> mentioning"?

You don't have to nitpick style. We can allow this much creative freedom in documentation.

Show 8 quoted lines

>> +in effect, diffcore-pickaxe leaves only such filepairs that touch the
>> +specified string in its output.  When in effect, diffcore-pickaxe
>> +leaves all filepairs intact if there is such a filepair, or makes the
>> +output empty otherwise.  The latter behavior is designed to make
>> +reviewing of the changes in the context of the whole changeset easier.
>
> I find this description (from the original code, not from this commit)
> somewhat confusing.  It is written from the implementer's POV.

I explained the entire -S and -G thing in terms of filepairs (and yes, that's implementation detail). Why would I want to explain this in any other terms?

> Does
> this seem clearer to you?
> [...]

>From diff-options.txt (the more end-user side):

When -S or -G finds a change, show all the changes in that changeset, not just the files that contain the change in <string>.

Not clear enough?