Re: [PATCH 1/5] Add a new option 'merges' to revision.c
On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi koo...@posteo.de wrote:
Subject: Add a new option 'merges' to revision.c
For the subject, mention the area you're touching, followed by a
colon, followed by a short but meaningful summary of the change. Drop
capitalization.
revision: add --merges={show|only|hide} option
revision.c: add a new option 'merges' with
No need to mention revision.c here since the patch itself shows this clearly.
Considering that there is already a --merges option, it is somewhat
misleading and not terribly clear to say only that you're adding a new
option 'merges'. Better would be to spell out --merges= explicitly.
possible values of 'only', 'show' and 'hide'.
The option is used when showing the list of commits.
The value 'only' lists only merges. The value 'show'
is the default behavior which shows the commits as well
as merges and the value 'hide' makes it just list commit
items.
Signed-off-by: Koosha Khajehmoogahi koo...@posteo.de
Considering that Junio actually wrote this patch[1], it would be more
correct and considerate to attribute it to him. You can do so by
ensuring that there is a From: header at the very top of the commit
message before mailing out the patch:
From: Junio C Hamano gits...@pobox.com
The customary way to indicate that you wrote the commit message and
made a few small adjustments to Junio's patch would be with a
bracketed comment (starting with your initials) just before your
sign-off. Something like this:
[kk: wrote commit message; added a couple missing
{min|max}_parents assignments]
[1]: http://article.gmane.org/gmane.comp.version-control.git/265597
---
diff --git a/revision.c b/revision.c
index 66520c6..edb7bed 100644
--- a/revision.c
+++ b/revision.c
@@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs,
const char *pattern)
add_grep(revs, pattern, GREP_PATTERN_BODY);
}
+int parse_merges_opt(struct rev_info *revs, const char *param)
+{
+ if (!strcmp(param, show)) {
+ revs-min_parents = 0;
+ revs-max_parents = -1;
+ } else if (!strcmp(param, only)) {
+ revs-min_parents = 2;
+ revs-max_parents = -1;
+ } else if (!strcmp(param, hide)) {
+ revs-min_parents = 0;
+ revs-max_parents = 1;
+ } else {
+ return -1;
+ }
+ return 0;
+}
+
static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
int *unkc, const char **unkv)
{
@@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs-show_all = 1;
} else if (!strcmp(arg, --remove-empty)) {
revs-remove_empty_trees = 1;
+ } else if (starts_with(arg, --merges=)) {
+ if (parse_merges_opt(revs, arg + 9))
+ die(unknown option: %s, arg);
} else if (!strcmp(arg, --merges)) {
+ revs-max_parents = -1;
revs-min_parents = 2;
} else if (!strcmp(arg, --no-merges)) {
+ revs-min_parents = 0;
revs-max_parents = 1;
} else if (starts_with(arg, --min-parents=)) {
revs-min_parents = atoi(arg+14);
diff --git a/revision.h b/revision.h
index 0ea8b4e..f9df58c 100644
--- a/revision.h
+++ b/revision.h
@@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv,
struct rev_info *revs,
extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t
*ctx,
const struct option *options,
const char * const usagestr[]);
+extern int parse_merges_opt(struct rev_info *, const char *);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
extern int handle_revision_arg(const char *arg, struct rev_info *revs,
--
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
[PATCH 1/5] Add a new option 'merges' to revision.c
revision.c: add a new option 'merges' with
possible values of 'only', 'show' and 'hide'.
The option is used when showing the list of commits.
The value 'only' lists only merges. The value 'show'
is the default behavior which shows the commits as well
as merges and the value 'hide' makes it just list commit
items.
Signed-off-by: Koosha Khajehmoogahi koo...@posteo.de
---
revision.c | 22 ++
revision.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/revision.c b/revision.c
index 66520c6..edb7bed 100644
--- a/revision.c
+++ b/revision.c
@@ -1678,6 +1678,23 @@ static void add_message_grep(struct rev_info *revs,
const char *pattern)
add_grep(revs, pattern, GREP_PATTERN_BODY);
}
+int parse_merges_opt(struct rev_info *revs, const char *param)
+{
+ if (!strcmp(param, show)) {
+ revs-min_parents = 0;
+ revs-max_parents = -1;
+ } else if (!strcmp(param, only)) {
+ revs-min_parents = 2;
+ revs-max_parents = -1;
+ } else if (!strcmp(param, hide)) {
+ revs-min_parents = 0;
+ revs-max_parents = 1;
+ } else {
+ return -1;
+ }
+ return 0;
+}
+
static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
int *unkc, const char **unkv)
{
@@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs-show_all = 1;
} else if (!strcmp(arg, --remove-empty)) {
revs-remove_empty_trees = 1;
+ } else if (starts_with(arg, --merges=)) {
+ if (parse_merges_opt(revs, arg + 9))
+ die(unknown option: %s, arg);
} else if (!strcmp(arg, --merges)) {
+ revs-max_parents = -1;
revs-min_parents = 2;
} else if (!strcmp(arg, --no-merges)) {
+ revs-min_parents = 0;
revs-max_parents = 1;
} else if (starts_with(arg, --min-parents=)) {
revs-min_parents = atoi(arg+14);
diff --git a/revision.h b/revision.h
index 0ea8b4e..f9df58c 100644
--- a/revision.h
+++ b/revision.h
@@ -240,6 +240,7 @@ extern int setup_revisions(int argc, const char **argv,
struct rev_info *revs,
extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t
*ctx,
const struct option *options,
const char * const usagestr[]);
+extern int parse_merges_opt(struct rev_info *, const char *);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
extern int handle_revision_arg(const char *arg, struct rev_info *revs,
--
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 1/5] Add a new option 'merges' to revision.c
Koosha Khajehmoogahi koo...@posteo.de writes:
@@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs-show_all = 1;
} else if (!strcmp(arg, --remove-empty)) {
revs-remove_empty_trees = 1;
+ } else if (starts_with(arg, --merges=)) {
+ if (parse_merges_opt(revs, arg + 9))
+ die(unknown option: %s, arg);
This one makes sense to me (so does what the parse_merges_opt()
does).
} else if (!strcmp(arg, --merges)) {
+ revs-max_parents = -1;
revs-min_parents = 2;
But is this change warranted? An existing user who is not at all
interested in the new --merges= option may be relying on the fact
that --merges does not affect the value of max_parents and she can
say log --max-parents=2 --merges to see only the two-way merges,
for example. This change just broke her, and I do not see why it is
a good thing.
} else if (!strcmp(arg, --no-merges)) {
+ revs-min_parents = 0;
revs-max_parents = 1;
Likewise.
--
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 1/5] Add a new option 'merges' to revision.c
Koosha Khajehmoogahi koo...@posteo.de writes:
} else if (!strcmp(arg, --merges)) {
+ revs-max_parents = -1;
revs-min_parents = 2;
But is this change warranted? An existing user who is not at all
interested in the new --merges= option may be relying on the fact
that --merges does not affect the value of max_parents and she can
say log --max-parents=2 --merges to see only the two-way merges,
for example. This change just broke her, and I do not see why it is
a good thing.
The point is that if you have your log.merges conf option set to 'hide'
and you use git log --merges (two mutually conflicting options),
git will silently exit without anything to show.
That is not an excuse to break --merges and --no-merges for
existing users who do not care about setting log.merges option to
anything.
The whole point of introducing a new --merges=show option was so
that people can sanely countermand log.merges configuration from the
command line without breaking --merges and --no-merges, wasn't 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 1/5] Add a new option 'merges' to revision.c
On 03/23/2015 12:31 AM, Junio C Hamano wrote:
Koosha Khajehmoogahi koo...@posteo.de writes:
@@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs-show_all = 1;
} else if (!strcmp(arg, --remove-empty)) {
revs-remove_empty_trees = 1;
+} else if (starts_with(arg, --merges=)) {
+if (parse_merges_opt(revs, arg + 9))
+die(unknown option: %s, arg);
This one makes sense to me (so does what the parse_merges_opt()
does).
In fact this change was written by you in your previous kind review :-)
I will add a 'From: Junio C Hamano gits...@pobox.com' header to my next patch.
} else if (!strcmp(arg, --merges)) {
+revs-max_parents = -1;
revs-min_parents = 2;
But is this change warranted? An existing user who is not at all
interested in the new --merges= option may be relying on the fact
that --merges does not affect the value of max_parents and she can
say log --max-parents=2 --merges to see only the two-way merges,
for example. This change just broke her, and I do not see why it is
a good thing.
The point is that if you have your log.merges conf option set to 'hide'
and you use git log --merges (two mutually conflicting options),
git will silently exit without anything to show. We need to clear the
max_parents set by parse_merges_opt() as the user should be able to continue
to use --merges without problem. But, your point is also valid.
} else if (!strcmp(arg, --no-merges)) {
+revs-min_parents = 0;
revs-max_parents = 1;
Likewise.
Similarly, without this, if log.merges is set to 'only' and you use git log
--no-merges,
you will still see the merges in the output.
--
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
