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

[Junio C Hamano]

On Mon, Apr 13, 2015 at 8:29 AM, Koosha Khajehmoogahi koo...@posteo.de wrote:
 From: Junio C Hamano gits...@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.

 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Koosha Khajehmoogahi]

From: Junio C Hamano gits...@pobox.com

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.
var. Other log-family commands format-patch and cherry ignore the option
altogether (i.e. both on command line and on config settings).

[kk: wrote commit message]

Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Koosha Khajehmoogahi koo...@posteo.de
---
 builtin/log.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..c7a7aad 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int decoration_given;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = PATCH;
 static const char *fmt_pretty;
+static const char *log_merges;
 
 static const char * const builtin_log_usage[] = {
N_(git log [options] [revision range] [[--] path...]),
@@ -386,6 +387,9 @@ static int git_log_config(const char *var, const char 
*value, void *cb)
decoration_style = 0; /* maybe warn? */
return 0;
}
+   if (!strcmp(var, log.merges)) {
+   return git_config_string(log_merges, var, value);
+   }
if (!strcmp(var, log.showroot)) {
default_show_root = git_config_bool(var, value);
return 0;
@@ -628,6 +632,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
init_revisions(rev, prefix);
rev.always_show_header = 1;
+   if (log_merges  parse_merges_opt(rev, log_merges))
+   die(unknown config value for log.merges: %s, log_merges);
memset(opt, 0, sizeof(opt));
opt.def = HEAD;
opt.revarg_opt = REVARG_COMMITTISH;
-- 
2.3.3.263.g095251d.dirty

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html