Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Junio C Hamano]

Dongcan Jiang  writes:

> At first, I also tried to only judge the value of "revs->no_walk &&
> revs->graph", but unfortunately, it failed to pass all cases in
> t4052-stat-output.sh.
> e.g. command "git show --stat --graph" failed to get the correct result.
>
> Finally, this is because that "revs->no_walk" gets set when it comes
> to "git show --stat".

When "git show" is given a range, it turns no-walk off and becomes a
command about a connected history.  Otherwise, it is a command about
discrete point(s).

Because "git show --graph A B C" and "git log --graph --no-walk A B
C" are moral equivalents, if we are forbidding the latter, we should
forbid the former.

"git show A" (and no other revs, just a single point), however,
could be thought of as an equivalent for "git log -1 A", even though
that interpretation is based on a wrong world view (because "show
one and stop" is not the reason why the result "git show A" gives
has only A and nothing else).  And it makes sort of sense to allow
"git show --graph A" that is an equivalent to "git log -1 --graph A"
under that interpretation.

I was tempted to say the existing test is wrong to expect that
"--graph" does anything meaningful for "git show" that is run for a
non-range.  But doing the "use of both is wrong for log" change
without special casing "show" (and instead "fixing" t4052) would be
a change in behaviour, which we try to avoid in general.

I'd prefer a solution that does not waste a new bit in revision
structure in general, but if we were to waste one bit to special
case "show", it shouldn't be "fail only for the log command".  It
should be "allow this stupidity only for the show command for
backward compatibility" bit instead, I think.

--
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] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Dongcan Jiang]

Hi, Eric and René

Thanks for your suggestions. Good ideas!

> Genuine question: Despite the GSoC micro-project mentioning only
> 'log', is it ever meaningful for these two options to be specified
> together? I suspect not, but it would be nice to hear from someone
> more familiar with the issue. If not specific to 'log', then the > patch
> can be simplified a good deal.

> Why only for git log?  Doesn't the justification given in the
> commit message above apply in general?

At first, I also tried to only judge the value of "revs->no_walk &&
revs->graph", but unfortunately, it failed to pass all cases in
t4052-stat-output.sh.
e.g. command "git show --stat --graph" failed to get the correct result.

Finally, this is because that "revs->no_walk" gets set when it comes
to "git show --stat". That's why I add the parameter
"revs->cmd_is_log" as judgement. Of course, it causes the limitation
you've mentioned. I will consider better solution in the next patch
edition as soon as possible.

Best Regards

Dongcan

2015-03-06 17:56 GMT+08:00 Eric Sunshine :
> On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang  wrote:
>> Forbid "log --graph --no-walk
>
> Style: drop capitalization in the Subject: line. Also prefix with the
> command or module being modified, followed by a colon. So:
>
> log: forbid combining --graph and --no-walk
>
> or:
>
> revision: forbid combining --graph and --no-walk
>
>> Because --graph is about connected history while --no-walk is about discrete 
>> points.
>
> Okay. You might also want to cite the wider discussion[1].
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/216083
>
>> revision.c: Judge whether --graph and --no-walk come together when running 
>> git-log.
>> buildin/log.c: Set git-log cmd flag.
>> Documentation/rev-list-options.txt: Add specification on the forbidden usage.
>
> No need to repeat in prose what the patch itself states more clearly
> and concisely.
>
> Also, such a change should be accompanied by new test(s).
>
>> Signed-off-by: Dongcan Jiang 
>> ---
>> diff --git a/Documentation/rev-list-options.txt 
>> b/Documentation/rev-list-options.txt
>> index 4ed8587..eea2c0a 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -679,6 +679,7 @@ endif::git-rev-list[]
>> given on the command line. Otherwise (if `sorted` or no argument
>> was given), the commits are shown in reverse chronological order
>> by commit time.
>> +   Cannot be combined with `--graph` when running git-log.
>>
>>  --do-walk::
>> Overrides a previous `--no-walk`.
>> @@ -781,6 +782,7 @@ you would get an output like this:
>> on the left hand side of the output.  This may cause extra lines
>> to be printed in between commits, in order for the graph history
>> to be drawn properly.
>> +   Cannot be combined with `--no-walk` when running git-log.
>
> Nice to see documentation updates. More below.
>
>>  This enables parent rewriting, see 'History Simplification' below.
>>  +
>> diff --git a/builtin/log.c b/builtin/log.c
>> index dd8f3fc..7bf5adb 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char 
>> *prefix)
>> git_config(git_log_config, NULL);
>>
>> init_revisions(&rev, prefix);
>> +   rev.cmd_is_log = 1;
>> rev.always_show_header = 1;
>> memset(&opt, 0, sizeof(opt));
>> opt.def = "HEAD";
>> diff --git a/revision.c b/revision.c
>> index 66520c6..5f62c89 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char 
>> *prefix)
>>
>> revs->commit_format = CMIT_FMT_DEFAULT;
>>
>> +   revs->cmd_is_log = 0;
>> +
>> init_grep_defaults();
>> grep_init(&revs->grep_filter, prefix);
>> revs->grep_filter.status_only = 1;
>> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, 
>> struct rev_info *revs, struct s
>>
>> if (revs->reflog_info && revs->graph)
>> die("cannot combine --walk-reflogs with --graph");
>> +   if (revs->no_walk && revs->graph && revs->cmd_is_log)
>
> Placing 'revs->cmd_is_log' first would make it clear at a glance that
> this restriction impacts 'log' only (but see question below):
>
> if (revs->cmd_is_log && revs->no_walk && revs->graph)
>
>> +   die("cannot combine --no-walk with --graph when running 
>> git-log");
>> if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>> die("cannot use --grep-reflog without --walk-reflogs");
>>
>> diff --git a/revision.h b/revision.h
>> index 0ea8b4e..255982a 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -146,6 +146,9 @@ struct rev_info {
>> track_first_time:1,
>> linear:1;
>>
>> +   /* cmd type */
>> +   unsigned int  cmd_is_log:1;
>
> Genuine question: Despite

Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[René Scharfe]

Am 06.03.2015 um 09:55 schrieb Dongcan Jiang:

Because --graph is about connected history while --no-walk is about discrete 
points.

revision.c: Judge whether --graph and --no-walk come together when running 
git-log.
buildin/log.c: Set git-log cmd flag.
Documentation/rev-list-options.txt: Add specification on the forbidden usage.

Signed-off-by: Dongcan Jiang 
---
  Documentation/rev-list-options.txt | 2 ++
  builtin/log.c  | 1 +
  revision.c | 4 
  revision.h | 3 +++
  4 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 4ed8587..eea2c0a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
given on the command line. Otherwise (if `sorted` or no argument
was given), the commits are shown in reverse chronological order
by commit time.
+   Cannot be combined with `--graph` when running git-log.

  --do-walk::
Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
on the left hand side of the output.  This may cause extra lines
to be printed in between commits, in order for the graph history
to be drawn properly.
+   Cannot be combined with `--no-walk` when running git-log.
  +
  This enables parent rewriting, see 'History Simplification' below.
  +
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..7bf5adb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
git_config(git_log_config, NULL);

init_revisions(&rev, prefix);
+   rev.cmd_is_log = 1;
rev.always_show_header = 1;
memset(&opt, 0, sizeof(opt));
opt.def = "HEAD";
diff --git a/revision.c b/revision.c
index 66520c6..5f62c89 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char 
*prefix)

revs->commit_format = CMIT_FMT_DEFAULT;

+   revs->cmd_is_log = 0;
+
init_grep_defaults();
grep_init(&revs->grep_filter, prefix);
revs->grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s

if (revs->reflog_info && revs->graph)
die("cannot combine --walk-reflogs with --graph");
+   if (revs->no_walk && revs->graph && revs->cmd_is_log)
+   die("cannot combine --no-walk with --graph when running 
git-log");


Why only for git log?  Doesn't the justification given in the commit 
message above apply in general?



if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
die("cannot use --grep-reflog without --walk-reflogs");

diff --git a/revision.h b/revision.h
index 0ea8b4e..255982a 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
track_first_time:1,
linear:1;

+   /* cmd type */
+   unsigned int  cmd_is_log:1;
+
enum date_mode date_mode;

unsigned intabbrev;



--
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] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Eric Sunshine]

On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang  wrote:
> Forbid "log --graph --no-walk

Style: drop capitalization in the Subject: line. Also prefix with the
command or module being modified, followed by a colon. So:

log: forbid combining --graph and --no-walk

or:

revision: forbid combining --graph and --no-walk

> Because --graph is about connected history while --no-walk is about discrete 
> points.

Okay. You might also want to cite the wider discussion[1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083

> revision.c: Judge whether --graph and --no-walk come together when running 
> git-log.
> buildin/log.c: Set git-log cmd flag.
> Documentation/rev-list-options.txt: Add specification on the forbidden usage.

No need to repeat in prose what the patch itself states more clearly
and concisely.

Also, such a change should be accompanied by new test(s).

> Signed-off-by: Dongcan Jiang 
> ---
> diff --git a/Documentation/rev-list-options.txt 
> b/Documentation/rev-list-options.txt
> index 4ed8587..eea2c0a 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -679,6 +679,7 @@ endif::git-rev-list[]
> given on the command line. Otherwise (if `sorted` or no argument
> was given), the commits are shown in reverse chronological order
> by commit time.
> +   Cannot be combined with `--graph` when running git-log.
>
>  --do-walk::
> Overrides a previous `--no-walk`.
> @@ -781,6 +782,7 @@ you would get an output like this:
> on the left hand side of the output.  This may cause extra lines
> to be printed in between commits, in order for the graph history
> to be drawn properly.
> +   Cannot be combined with `--no-walk` when running git-log.

Nice to see documentation updates. More below.

>  This enables parent rewriting, see 'History Simplification' below.
>  +
> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..7bf5adb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char 
> *prefix)
> git_config(git_log_config, NULL);
>
> init_revisions(&rev, prefix);
> +   rev.cmd_is_log = 1;
> rev.always_show_header = 1;
> memset(&opt, 0, sizeof(opt));
> opt.def = "HEAD";
> diff --git a/revision.c b/revision.c
> index 66520c6..5f62c89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char 
> *prefix)
>
> revs->commit_format = CMIT_FMT_DEFAULT;
>
> +   revs->cmd_is_log = 0;
> +
> init_grep_defaults();
> grep_init(&revs->grep_filter, prefix);
> revs->grep_filter.status_only = 1;
> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct 
> rev_info *revs, struct s
>
> if (revs->reflog_info && revs->graph)
> die("cannot combine --walk-reflogs with --graph");
> +   if (revs->no_walk && revs->graph && revs->cmd_is_log)

Placing 'revs->cmd_is_log' first would make it clear at a glance that
this restriction impacts 'log' only (but see question below):

if (revs->cmd_is_log && revs->no_walk && revs->graph)

> +   die("cannot combine --no-walk with --graph when running 
> git-log");
> if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
> die("cannot use --grep-reflog without --walk-reflogs");
>
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..255982a 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -146,6 +146,9 @@ struct rev_info {
> track_first_time:1,
> linear:1;
>
> +   /* cmd type */
> +   unsigned int  cmd_is_log:1;

Genuine question: Despite the GSoC micro-project mentioning only
'log', is it ever meaningful for these two options to be specified
together? I suspect not, but it would be nice to hear from someone
more familiar with the issue. If not specific to 'log', then the patch
can be simplified a good deal.

> enum date_mode date_mode;
>
> unsigned intabbrev;
> --
--
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] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Dongcan Jiang]

Because --graph is about connected history while --no-walk is about discrete 
points.

revision.c: Judge whether --graph and --no-walk come together when running 
git-log.
buildin/log.c: Set git-log cmd flag.
Documentation/rev-list-options.txt: Add specification on the forbidden usage.

Signed-off-by: Dongcan Jiang 
---
 Documentation/rev-list-options.txt | 2 ++
 builtin/log.c  | 1 +
 revision.c | 4 
 revision.h | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 4ed8587..eea2c0a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
given on the command line. Otherwise (if `sorted` or no argument
was given), the commits are shown in reverse chronological order
by commit time.
+   Cannot be combined with `--graph` when running git-log.
 
 --do-walk::
Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
on the left hand side of the output.  This may cause extra lines
to be printed in between commits, in order for the graph history
to be drawn properly.
+   Cannot be combined with `--no-walk` when running git-log.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..7bf5adb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
git_config(git_log_config, NULL);
 
init_revisions(&rev, prefix);
+   rev.cmd_is_log = 1;
rev.always_show_header = 1;
memset(&opt, 0, sizeof(opt));
opt.def = "HEAD";
diff --git a/revision.c b/revision.c
index 66520c6..5f62c89 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char 
*prefix)
 
revs->commit_format = CMIT_FMT_DEFAULT;
 
+   revs->cmd_is_log = 0;
+
init_grep_defaults();
grep_init(&revs->grep_filter, prefix);
revs->grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
 
if (revs->reflog_info && revs->graph)
die("cannot combine --walk-reflogs with --graph");
+   if (revs->no_walk && revs->graph && revs->cmd_is_log)
+   die("cannot combine --no-walk with --graph when running 
git-log");
if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
die("cannot use --grep-reflog without --walk-reflogs");
 
diff --git a/revision.h b/revision.h
index 0ea8b4e..255982a 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
track_first_time:1,
linear:1;
 
+   /* cmd type */
+   unsigned int  cmd_is_log:1;
+
enum date_mode date_mode;
 
unsigned intabbrev;
-- 
2.3.1.251.g83036f8

--
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