Re: [PATCH v2 2/5] log: honor log.merges= option
Koosha Khajehmoogahi writes: > On 04/08/2015 04:28 AM, Junio C Hamano wrote: > >> It is strange that you have to ask me to give you the reason why you >> chose it that way, isn't it? > > AFAIK, the only other command that supports --merges and --no-merges options > is > rev-list. This new feature aims to make a default behavior for the commands > that have these options. The command-line option is supported by the two > commands. > However, the config var is only used by git-log and rev-list ignores it. I > didn't > exclude rev-list for any particular reason. If we need, I could also handle > it in > rev-list. As rev-list is plumbing, it shouldn't be affected by the UI level customization knobs like this one; otherwise you will break people's scripts when end users choose to use the new knobs. Historically, "whatchanged" has been the way to ask for "log", with different default output format (but not different commit selection logic). I would think people who are used to "whatchanged" would expect that the command would pay attention to what "log" would. As I already mentioned with the reason why, I do not think "show" and "format-patch" should pay attention to it. There may be other commands from the "log" family (i.e. what is defined in builtin/log.c and/or uses get_revision() API to walk the commit graph), for which similar reasoning should be done to decide if each of them should or should not pay attention to it. 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
Re: [PATCH v2 2/5] log: honor log.merges= option
On 04/08/2015 04:28 AM, Junio C Hamano wrote: > Koosha Khajehmoogahi writes: > >> On 04/04/2015 10:00 PM, Junio C Hamano wrote: >>> Koosha Khajehmoogahi writes: >>> From: Junio C Hamano [kk: wrote commit message] >>> >>> Ehh, what exactly did you write ;-)? >>> >>> I think the most important thing that needs to be explained by the >>> log message for this change is that the variable is honored only by >>> log and it needs to explain why other Porcelain commands in the same >>> "log" family, like "whatchanged", should ignore the variable. >>> >> So, what would be the reason? > > It is strange that you have to ask me to give you the reason why you > chose it that way, isn't it? AFAIK, the only other command that supports --merges and --no-merges options is rev-list. This new feature aims to make a default behavior for the commands that have these options. The command-line option is supported by the two commands. However, the config var is only used by git-log and rev-list ignores it. I didn't exclude rev-list for any particular reason. If we need, I could also handle it in rev-list. -- 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
Re: [PATCH v2 2/5] log: honor log.merges= option
Koosha Khajehmoogahi writes: > On 04/04/2015 10:00 PM, Junio C Hamano wrote: >> Koosha Khajehmoogahi writes: >> >>> From: Junio C Hamano >>> >>> [kk: wrote commit message] >> >> Ehh, what exactly did you write ;-)? >> >> I think the most important thing that needs to be explained by the >> log message for this change is that the variable is honored only by >> log and it needs to explain why other Porcelain commands in the same >> "log" family, like "whatchanged", should ignore the variable. >> > So, what would be the reason? It is strange that you have to ask me to give you the reason why you chose it that way, isn't it? >> I think that we must not to allow format-patch and show to be >> affected by this variable, because it is silly if log.merges=only >> broke format-patch output or made "git show" silent. But I didn't >> think about others. Whoever is doing this change needs to explain >> in the log message the reason why it was decided that only "git log" >> should pay attention to it. >> -- 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
Re: [PATCH v2 2/5] log: honor log.merges= option
On 04/04/2015 10:00 PM, Junio C Hamano wrote: > Koosha Khajehmoogahi writes: > >> From: Junio C Hamano >> >> [kk: wrote commit message] > > Ehh, what exactly did you write ;-)? > > I think the most important thing that needs to be explained by the > log message for this change is that the variable is honored only by > log and it needs to explain why other Porcelain commands in the same > "log" family, like "whatchanged", should ignore the variable. > So, what would be the reason? > I think that we must not to allow format-patch and show to be > affected by this variable, because it is silly if log.merges=only > broke format-patch output or made "git show" silent. But I didn't > think about others. Whoever is doing this change needs to explain > in the log message the reason why it was decided that only "git log" > should pay attention to it. > signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 2/5] log: honor log.merges= option
On Fri, Apr 3, 2015 at 9:21 PM, Koosha Khajehmoogahi wrote:
> From: Junio C Hamano
>
> [kk: wrote commit message]
>
> Helped-by: Eris Sunshine
s/Eris/Eric/
> Signed-off-by: Koosha Khajehmoogahi
> ---
> 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 [] [] [[--] ...]"),
> @@ -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
Re: [PATCH v2 2/5] log: honor log.merges= option
Koosha Khajehmoogahi writes:
> From: Junio C Hamano
>
> [kk: wrote commit message]
Ehh, what exactly did you write ;-)?
I think the most important thing that needs to be explained by the
log message for this change is that the variable is honored only by
log and it needs to explain why other Porcelain commands in the same
"log" family, like "whatchanged", should ignore the variable.
I think that we must not to allow format-patch and show to be
affected by this variable, because it is silly if log.merges=only
broke format-patch output or made "git show" silent. But I didn't
think about others. Whoever is doing this change needs to explain
in the log message the reason why it was decided that only "git log"
should pay attention to it.
> Helped-by: Eris Sunshine
> Signed-off-by: Koosha Khajehmoogahi
> ---
> 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;
The variable name may want to be updated to mimic other variables
used in a similar way, e.g. default_show_root is used to store the
value from log.showroot.
Perhaps "default_show_merge" or something?
Thanks.
> static const char * const builtin_log_usage[] = {
> N_("git log [] [] [[--] ...]"),
> @@ -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;
--
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 v2 2/5] log: honor log.merges= option
From: Junio C Hamano
[kk: wrote commit message]
Helped-by: Eris Sunshine
Signed-off-by: Koosha Khajehmoogahi
---
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 [] [] [[--] ...]"),
@@ -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
