Re: [PATCH] gitk: Use the --submodule option for diffs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jens Lehmann <Jens.Lehmann@xxxxxx> writes:

> Instead of just showing not-quite-helpful SHA-1 pairs display the first
> lines of the corresponding commit messages in the submodule (similar to
> the output of 'git submodule summary').
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@xxxxxx>
> ---
>
> This patch applies to 'next' and uses the new --submodule option of git
> diff to achieve a more meaningful output of submodule differences in
> gitk.
>
> Any objections against making this the default?

It looks like you are not making this the default, but are making it
mandatory.  That's quite different.

As long as gitk ships with matching version of git, I do not think it is a
huge problem to force "diff" to always run with --submodule option, but if
that is what the patch does, I'd prefer to see the patch says so, instead
of giving a false impression that there may be a way to disable it if one
wants to.

Looking at the patched text, I had to wonder where these $flags come from.
The callers of "diffcmd" give it to you, and I am not sure if all of them
want -p format diffs.  Specifically, what does gettreediffs do?  Does it
make sense to give --submodule to that invocation?

Yes, I know --submodule happens to be a no-op in non-patch format diffs,
but I do not think that is by design.  It is something somebody may notice
and correct it later, at which time this patch will be broken.

I also suspect that this might break getpatchid, but as long as all the
patches consistently change/break ids it would be Ok.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]