Re: [PATCH v3 2/5] log: honor log.merges= option

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2015, 06:07 UTC
Message-ID: <CAPc5daU+fk+d_tUiCmy0uwx7N5PYwLhqg6CmYGyaik_2VzQnMg@mail.gmail.com>
In-Reply-To: <1428938968-19013-2-git-send-email-koosha@posteo.de>

On Mon, Apr 13, 2015 at 8:29 AM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:

> From: Junio C Hamano <gitster@pobox.com>

Let's not call this *my* change. The mechanism I did write, but I think the most important part of the change is to decide which commands in the log family should honor the configuration variable and which ones should not. As I said in the beginning, I am not convinced that letting "git log" and "git log" alone is the right choice. Make it "Helped-by" or something instead, to make it clear that I helped the implementation but not the design.

Show 6 quoted lines

> The config. variable is honored only by log. Other log family commands
> including show, shortlog and rev-list are affected only from the
> equivalent command line option (i.e. --merges=). Since these commands
> are somehow different representations of log command, we like to have
> the same behavior from them as we do from log itself. However, to not
> break the default output of them, we do not consider the the config.

That is a weak argument, isn't it? If we really wanted not to break the default output, then "git log" should not honor the configuration variable, either. And I do not agree with "we like to have the same behaviour" in the first place.

My preference, as I already hinted in previous review rounds, is:

 - "log" and "whatchanged" are the same things and should behave the
   same way with respect to this configuration variable;

 - "rev-list" is plumbing and should never pay attention to a UI level
   configuration variable like this one;

 - "show" is given exact commit object(s) and is asked to show them;
   not showing what was explicitly asked to be shown only because
   the user has "do not show merges" configuration does not make
   much sense, so the configuration variable should be ignored;

 - The revision graph traversal done by "format-patch" and "cherry-pick"
   is primarily to determine the set of commits to process and of a
   different nature from the traversal done by "log", even though they
   share the same underlying code. It is not appropriate to pay attention
   to this configuration variable.

 - I am neutral as to "shortlog". As long as it pays attention to the
   new command line override, I think it is OK if it paid attention to
   the configuration variable, primarily because people most of the
   time use "shortlog --no-merges" with the current version of Git
   _anyway_.

Thanks.