[RFC] Should "log --cc" imply "log --cc -p"?

[RFC] Should "log --cc" imply "log --cc -p"?

[Junio C Hamano]

I think a natural way to ask reviewing the recent merges while
showing tricky ones would be to say:

	$ git log --first-parent --cc master..pu

But this does not to show what I expect to see, which is an output
of:

	$ git log --first-parent --cc -p master..pu

This is only a minor irritation, but I think it might make sense to
make it notice the --cc in the former and turn -p on automatically.

The same for

	$ git log --cc next~3..next

which may make sense to turn into "git log -p --cc next~3..next".

When deciding if the above makes sense, there are a few things to
know to be true as prerequisites for the discussion:

 * Neither of these

	$ git log --first-parent -p master..pu
	$ git log -p master..pu

   shows any patches, and it is not a bug.  No patches are shown for
   merges unless -m is given, and when -m is given, we give pairwise
   2-way diffs for the number of parents.

 * We recently tweaked this:

	$ git log --first-parent -m -p master..pu

   to omit diffs with second and later parents, as that is what the
   user wishes with --first-parent.

 * The "--cc" option, when comparing two trees (i.e. showing a
   non-merge commit), is designed to show a normal patch.  In other
   words, you can view "--cc" as a modifier when you request a patch
   output format with "-p".  For "git show", "--cc -p" is turned on
   by default, and giving "-m" explicity (i.e. "git show -m") you
   can turn it off and have it do "-m -p" instead.

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 04.02.2013 00:10:
> I think a natural way to ask reviewing the recent merges while
> showing tricky ones would be to say:
> 
> 	$ git log --first-parent --cc master..pu
> 
> But this does not to show what I expect to see, which is an output
> of:
> 
> 	$ git log --first-parent --cc -p master..pu
> 

I'm not Junio, but I guess he would respond that it's a matter of
expectations: "--cc" is a diff option, and just like any other diff
option, it has an effect on "log" only if "log" has been told to show a
diff.

Oh wait, you *are* Junio ;)

> This is only a minor irritation, but I think it might make sense to
> make it notice the --cc in the former and turn -p on automatically.

I think we have a clear distiction between rev-list/log options and diff
options. "log" comes without a diff, "-p" turns on diffs.

"log" passes diff-options to "diff". If the user gives a diff-option to
"log" we can conclude that he meant to request a diff and turn them on
automatically (as if "-p" was there also).

But I'm wondering whether that has adverse effects on scripts/aliases.
For example, I could have a special alias

newpu = log first-parent --cc next..pu

which allows me to use "git newpu" as well as "git newpu -p" to get
those new commits with or without diff in my preferred format, but not
any more after the change you suggest. (I could use a second alias, of
course; and yes, I'm (ab)using current option parser features.)

> The same for
> 
> 	$ git log --cc next~3..next
> 
> which may make sense to turn into "git log -p --cc next~3..next".
> 
> When deciding if the above makes sense, there are a few things to
> know to be true as prerequisites for the discussion:
> 
>  * Neither of these
> 
> 	$ git log --first-parent -p master..pu
> 	$ git log -p master..pu
> 
>    shows any patches, and it is not a bug.  No patches are shown for
>    merges unless -m is given, and when -m is given, we give pairwise
>    2-way diffs for the number of parents.

But diffs are on here ("-p"), it's just that the default diff option for
merges is to not display them. Well, I admit there's two different ways
of thinking here:

A) "git log -p" turns on diffs for all commits, and the default diff
options is the (non-existing) "--no-show" diff-option for merges.

B) "git log -p" applies "-p" to all commits except merge commits.

I'm strongly in the A camp, because I think that this gives a clearer
interface. A really describes the user facing side, whereas B is closer
to the implementation.

>  * We recently tweaked this:
> 
> 	$ git log --first-parent -m -p master..pu
> 
>    to omit diffs with second and later parents, as that is what the
>    user wishes with --first-parent.

That made --first-parent into a dual-purpose option, i.e. it modifies
the rev-listing to one parent as well as the diff creation. It does not
turn on diffs by itself.

>  * The "--cc" option, when comparing two trees (i.e. showing a
>    non-merge commit), is designed to show a normal patch.  In other
>    words, you can view "--cc" as a modifier when you request a patch
>    output format with "-p".  For "git show", "--cc -p" is turned on
>    by default, and giving "-m" explicity (i.e. "git show -m") you
>    can turn it off and have it do "-m -p" instead.
> 

Yes, I really think of --cc is a diff-option, and that this point of
view gives the clearest ui.

We could argue that any diff-option could switch on diffs (imply -p),
but that change seems to be quite radical. "show" having "-p" as a
default is quite natural, but that is different.

Having only "--cc" imply "-p" would be too much special case auto-magic
for my taste. We have too many of these already.

I really think we should leave it as is.

Michael

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> But diffs are on here ("-p"), it's just that the default diff option for
> merges is to not display them. Well, I admit there's two different ways
> of thinking here:
>
> A) "git log -p" turns on diffs for all commits, and the default diff
> options is the (non-existing) "--no-show" diff-option for merges.
>
> B) "git log -p" applies "-p" to all commits except merge commits.
>
> I'm strongly in the A camp,...

I can personally be trained to think either way, but I suspect that
we already came halfway to

  C) "-p" asks for two-way patches, and "-c/--cc" ask for n-way
     patches (including but not limited to n==2); it is not that -p
     asks for patch generation and others modify the finer behaviour
     of patch generation.

"git log/diff-files -U8" do not need "-p" to enable textual patches,
for example.  It is "I already told you that I want 8-line context.
For what else, other than showing textual diff, do you think I told
you that for?" and replacing "8-line context" with various other
options that affect patch generation will give us a variety of end
user complaints that would tell us that C) is more intuitive to
them.

But I do not feel very strongly that I am *right* on this issue, so
I won't do anything about it hastily right now.

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 04.02.2013 17:36:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> But diffs are on here ("-p"), it's just that the default diff option for
>> merges is to not display them. Well, I admit there's two different ways
>> of thinking here:
>>
>> A) "git log -p" turns on diffs for all commits, and the default diff
>> options is the (non-existing) "--no-show" diff-option for merges.
>>
>> B) "git log -p" applies "-p" to all commits except merge commits.
>>
>> I'm strongly in the A camp,...
> 
> I can personally be trained to think either way, but I suspect that
> we already came halfway to
> 
>   C) "-p" asks for two-way patches, and "-c/--cc" ask for n-way
>      patches (including but not limited to n==2); it is not that -p
>      asks for patch generation and others modify the finer behaviour
>      of patch generation.
> 
> "git log/diff-files -U8" do not need "-p" to enable textual patches,
> for example.  It is "I already told you that I want 8-line context.

That's a good point that I wasn't aware of.

> For what else, other than showing textual diff, do you think I told
> you that for?" and replacing "8-line context" with various other
> options that affect patch generation will give us a variety of end
> user complaints that would tell us that C) is more intuitive to
> them.
> 
> But I do not feel very strongly that I am *right* on this issue, so
> I won't do anything about it hastily right now.

I'm not sure how many of these we have already, but to me it indicates
that we should turn diffs on for log with any diff option that only
makes sense with a diff. That would make the ui more consistent without
taking anything away.

For scripting/aliasing purposes, we have "-s" in order to override any
implied "-p" (as I just learned).

Michael

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Jeff King]

On Mon, Feb 04, 2013 at 08:36:43AM -0800, Junio C Hamano wrote:

> "git log/diff-files -U8" do not need "-p" to enable textual patches,
> for example.  It is "I already told you that I want 8-line context.
> For what else, other than showing textual diff, do you think I told
> you that for?" and replacing "8-line context" with various other
> options that affect patch generation will give us a variety of end
> user complaints that would tell us that C) is more intuitive to
> them.

Yeah, I'd agree with this. My feeling is that when there are two
options, A and B, and A is a no-op if B is not also specified, that it
makes sense for A to imply B. We do it in several places already (and I
just added some for "git branch --list" recently).

Is "--cc" a no-op when "-p" is not specified? Certainly "-c" is not, but
I do not think you are proposing that. At first glance, "--cc" is
nonsensical without "-p", but what about other xdiff callers? For
example, in:

  git log --cc --stat

the "--cc" is significant. So I don't think it is right for "--cc" to
always imply "-p". But if the rule kicked in only when no other format
had been specified, that might make sense.

-Peff

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   git log --cc --stat
>
> the "--cc" is significant. So I don't think it is right for "--cc" to
> always imply "-p". But if the rule kicked in only when no other format
> had been specified, that might make sense.

Yes, it was my mistake that I left it unsaid.  Obviously the
description of the "minor irritation" must be qualified with "unless
no other output options like --raw, --stat, etc. are given".

Thanks.

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "git log/diff-files -U8" do not need "-p" to enable textual patches,
> for example.  It is "I already told you that I want 8-line context.
> For what else, other than showing textual diff, do you think I told
> you that for?" and replacing "8-line context" with various other
> options that affect patch generation will give us a variety of end
> user complaints that would tell us that C) is more intuitive to
> them.

On a related note I think "--full-diff" should imply "-p" too.

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Jeff King]

On Tue, Feb 05, 2013 at 11:16:52AM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > "git log/diff-files -U8" do not need "-p" to enable textual patches,
> > for example.  It is "I already told you that I want 8-line context.
> > For what else, other than showing textual diff, do you think I told
> > you that for?" and replacing "8-line context" with various other
> > options that affect patch generation will give us a variety of end
> > user complaints that would tell us that C) is more intuitive to
> > them.
> 
> On a related note I think "--full-diff" should imply "-p" too.

I don't think that is in the same class. --full-diff is quite useful for
many other diff formats. E.g.:

  git log --full-diff --raw Makefile

If you are proposing to default to "-p" when "--full-diff" is used but
no format is given, that is a slightly different thing. The --full-diff
in such a case is indeed useless, but I do not think it necessarily
follows that "-p" was what the user wanted.

-Peff

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 5, 2013 at 12:22 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 05, 2013 at 11:16:52AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > "git log/diff-files -U8" do not need "-p" to enable textual patches,
>> > for example.  It is "I already told you that I want 8-line context.
>> > For what else, other than showing textual diff, do you think I told
>> > you that for?" and replacing "8-line context" with various other
>> > options that affect patch generation will give us a variety of end
>> > user complaints that would tell us that C) is more intuitive to
>> > them.
>>
>> On a related note I think "--full-diff" should imply "-p" too.
>
> I don't think that is in the same class. --full-diff is quite useful for
> many other diff formats. E.g.:
>
>   git log --full-diff --raw Makefile
>
> If you are proposing to default to "-p" when "--full-diff" is used but
> no format is given, that is a slightly different thing. The --full-diff
> in such a case is indeed useless, but I do not think it necessarily
> follows that "-p" was what the user wanted.

You're right. I didn't notice that it could work with other things besides -p.

On a related note then, it's a bit confusing that it's called
"--full-diff" when it doesn't actually show a diff.

Re: [RFC] Should "log --cc" imply "log --cc -p"?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On a related note then, it's a bit confusing that it's called
> "--full-diff" when it doesn't actually show a diff.

It is too late to change the name of the option, but we could add a
synonym if it makes easier to understand what the option is for.  It
is about saying "my pathspec are only to limit the commits to be
shown and do not limit the diff output".  In that sense, the current
name that asks to give me the diff output fully is not that wrong,
but I wouldn't be surprised if other people can come up with better
names.