From: Johan Herland <johan@herland.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 3/6] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file
Date: Wed, 27 Apr 2011 04:02:10 +0200 [thread overview]
Message-ID: <201104270402.10658.johan@herland.net> (raw)
In-Reply-To: <7vy62xezqy.fsf@alter.siamese.dyndns.org>
On Tuesday 26 April 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > +--dirstat[=<arg1,arg2,...>]::
> > + Output the distribution of relative amount of changes for each
> > + sub-directory. The behavior of `--dirstat` can be customized by
> > + passing it a comma separated list of arguments. The defaults
> > + are controlled by the `diff.dirstat` configuration variable (see
> > + linkgit:git-config[1]). The following arguments are available:
>
> These "arguments" feel more like "options" (or "parameters"), no? Your
> code in diff.c also calls it "opt". The second line of the proposed log
> message has the same issue.
I have tried to consistently use "option" for referring to the entire
"--dirstat=whatever" entity, and then use "argument" for referring to
each comma-separated token following "--dirstat=". I based this on the
function naming in diff.c, which uses "diff_opt_parse()" to parse diff
options, "stat_opt()" to parse the '--stat*' options, and "opt_arg()"
to parse arguments to options (i.e. "--option=argument").
To me, "argument" and "parameter" are synonyms, but English is not my
first language. I'll replace "argument" with "parameter" in the re-roll.
I.e. "option" refers to the option name AND the option parameters, while
"parameters" refers to the option parameters only.
> > +--
> > +`changes`;;
> > + Compute the dirstat numbers by counting the lines that have been
> > + removed from the source, or added to the destination. This ignores
> > + the amount of pure code movements within a file. In other words,
> > + rearranging lines in a file is not counted as much as other changes.
> > + This is the default `--dirstat` behavior.
>
> "default behavior when no option is given"?
"default behavior when no parameter is given"?
> > +`cumulative`;;
> > + Count changes in a child directory for the parent directory as well.
> > + Note that when using `cumulative`, the sum of the percentages
> > + reported may exceed 100%. The default (non-cumulative) behavior can
> > + be specified with the `noncumulative` argument.
>
> So the later one wins? I.e. --dirstat=cumulative,noncumulative from the
> command line (which seems silly), or more importantly with
>
> [alias]
> dstat = diff --dirstat=cumulative
>
> and you can say "git dstat --dirstat=noncumulative A..B"?
Indeed. The intention is that dirstat parameters are parsed in order
(first from config, then from command line), and the later parameters
override earlier (conflicting) parameters.
> > diff --git a/diff.c b/diff.c
> > index cfbfa92..08aaa47 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3144,6 +3144,72 @@ static int stat_opt(struct diff_options
> > *options, const char **av)
> >
> > return argcount;
> >
> > }
>
> /*
> * Document what the return value from this function means here.
> */
> > +static int dirstat_opt(struct diff_options *options, const char **av)
>
> Do you have to pass "const char **av", or just "const char *arg"?
dirstat_opt() was modeled on stat_opt(). dirstat_opt() obviously needs
just "const char *arg". Will fix.
> > +{
> > + const char *p, *arg = av[0];
> > + char *mangled = NULL;
> > + char sep = '=';
> > +
> > + if (!strcmp(arg, "--cumulative")) /* deprecated */
> > + /* handle '--cumulative' like '--dirstat=cumulative' */
> > + p = "=cumulative";
> > + else if (!strcmp(arg, "--dirstat-by-file") ||
> > + !prefixcmp(arg, "--dirstat-by-file=")) { /* deprecated */
> > + /* handle '--dirstat-by-file=*' like '--dirstat=files,*' */
> > + mangled = xstrdup(arg + 2);
> > + memcpy(mangled, "--dirstat=files", 15);
> > + if (mangled[15]) {
> > + assert(mangled[15] == '=');
> > + mangled[15] = ',';
> > + }
> > + arg = mangled;
> > + p = mangled + 9;
>
> I understand you wanted to reuse the while() loop below, but I do not
> think it is worth it. Isn't it easier to read if you handled the above
> cases in their if/else body and return?
>
> if (--cumulative) {
> options->output_format |= DIFF_FORMAT_DIRSTAT;
> DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
> return 1;
> }
> if (--dirstat-by-file) {
> options->output_format |= DIFF_FORMAT_DIRSTAT;
> DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> return 1;
> }
> ...
>
> Even better, probably they can be left to diff_opt_parse() without
> calling this function, as you are deprecating them and do not have to
> allow them to take the opt1,opt2,... form of parameter.
I understand, but politely disagree: Patch 6/6 complicates the logic
that DIFF_OPT_SET()/CLR() various bits in the diff options. I'd rather
keep that logic in one place, than duplicate it into diff_opt_parse().
> > + }
> > + else if (!prefixcmp(arg, "-X"))
> > + p = arg + 2;
> > + else if (!prefixcmp(arg, "--dirstat"))
> > + p = arg + 9;
> > + else
> > + return 0;
> > +
> > + options->output_format |= DIFF_FORMAT_DIRSTAT;
> > +
> > + while (*p) {
> > + if (*p != sep)
>
> What happens to "diff -X3 A..B"?
Oops. Will fix, and add testcases verifying the fix.
> > + die("Missing argument separator ('%c'), at index %lu of '%s'",
> > + sep, p - arg, arg);
>
> Don't you need to cast (p-arg) for %lu from ptrdiff type here?
Copied PD_FMT from builtin/mktag.c instead.
> It probably is more common to say s/index/char/;
Indeed.
> > + if (!prefixcmp(p, "changes")) {
> > + p += 7;
> > + DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> > + }
> > + else if (!prefixcmp(p, "files")) {
> > + p += 5;
> > + DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> > + }
> > + else if (!prefixcmp(p, "noncumulative")) {
> > + p += 13;
> > + DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> > + }
> > + else if (!prefixcmp(p, "cumulative")) {
> > + p += 10;
> > + DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
> > + }
> > + else if (isdigit(*p)) {
> > + char *end;
> > + options->dirstat_percent = strtoul(p, &end, 10);
> > + assert(end > p);
> > + p = end;
> > + }
>
> That's a senseless assert(), isn't it?
>
> You already know the first letter is a digit, so assert(p < end) will
> always be true. You may want to check that this particular option is all
> digit by checking (*end == '\0' || *end == ',') but that is done at the
> beginning of this loop anyway, so I don't think there is anything to
> check here.
True. I guess I just wanted a sanity check that aborts, rather than
entering an infinite loop in case I got my logic wrong somewhere...
Removed in the re-roll.
> > + else
> > + die("Unknown --dirstat argument '%s'", p);
>
> The function parses dirstat_OPT, but this says argument?
Again, the "option" refers to the option name ("--dirstat") AND its
s/arguments/parameters/ ("changes,noncumulative,3")
Your other comments (that I felt no need to comment on) will also be
incorporated in the re-roll.
Thanks for the feedback!
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
next prev parent reply other threads:[~2011-04-27 2:02 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-07 13:49 BUG? in --dirstat when rearranging lines in a file Johan Herland
2011-04-07 14:56 ` Linus Torvalds
2011-04-07 22:43 ` Junio C Hamano
2011-04-07 22:59 ` Linus Torvalds
2011-04-08 14:46 ` Johan Herland
2011-04-08 14:48 ` [PATCH 1/3] --dirstat: Document shortcomings compared to --stat or regular diff Johan Herland
2011-04-08 19:50 ` Junio C Hamano
2011-04-08 14:50 ` [PATCH 2/3] --dirstat-by-file: Make it faster and more correct Johan Herland
2011-04-08 14:55 ` [RFC/PATCH 3/3] Teach --dirstat to not completely ignore rearranged lines Johan Herland
2011-04-08 15:04 ` BUG? in --dirstat when rearranging lines in a file Linus Torvalds
2011-04-08 19:56 ` Junio C Hamano
2011-04-10 22:48 ` [PATCHv2 0/3] --dirstat fixes Johan Herland
2011-04-10 22:48 ` [PATCHv2 1/3] --dirstat: Describe non-obvious differences relative to --stat or regular diff Johan Herland
2011-04-10 22:48 ` [PATCHv2 2/3] --dirstat-by-file: Make it faster and more correct Johan Herland
2011-04-11 18:14 ` Junio C Hamano
2011-04-10 22:48 ` [PATCHv2 3/3] Teach --dirstat to not completely ignore rearranged lines within a file Johan Herland
2011-04-11 21:38 ` Junio C Hamano
2011-04-11 21:56 ` Johan Herland
2011-04-11 22:08 ` Junio C Hamano
2011-04-12 9:22 ` Johan Herland
2011-04-12 9:24 ` [PATCH 4/3] --dirstat: In case of renames, use target filename instead of source filename Johan Herland
2011-04-12 14:59 ` Linus Torvalds
2011-04-12 9:26 ` [RFC/PATCH 5/3] Alternative --dirstat implementation, based on diffstat analysis Johan Herland
2011-04-12 14:46 ` Linus Torvalds
2011-04-12 15:08 ` Linus Torvalds
2011-04-12 22:03 ` Johan Herland
2011-04-12 22:12 ` Linus Torvalds
2011-04-12 22:22 ` Junio C Hamano
2011-04-26 0:01 ` [PATCH 0/6] --dirstat fixes, part 2 Johan Herland
2011-04-26 0:01 ` [PATCH 1/6] Add several testcases for --dirstat and friends Johan Herland
2011-04-26 0:01 ` [PATCH 2/6] Make --dirstat=0 output directories that contribute < 0.1% of changes Johan Herland
2011-04-26 0:01 ` [PATCH 3/6] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file Johan Herland
2011-04-26 16:36 ` Junio C Hamano
2011-04-27 2:02 ` Johan Herland [this message]
2011-04-27 4:53 ` Junio C Hamano
2011-04-27 20:51 ` Junio C Hamano
2011-04-27 21:01 ` Junio C Hamano
2011-04-26 0:01 ` [PATCH 4/6] Add config variable for specifying default --dirstat behavior Johan Herland
2011-04-26 16:43 ` Junio C Hamano
2011-04-27 2:02 ` Johan Herland
2011-04-26 0:01 ` [PATCH 5/6] Use floating point for --dirstat percentages Johan Herland
2011-04-26 16:52 ` Junio C Hamano
2011-04-27 2:02 ` Johan Herland
2011-04-27 4:42 ` Junio C Hamano
2011-04-27 4:53 ` Linus Torvalds
2011-04-27 5:20 ` Junio C Hamano
2011-04-26 0:01 ` [PATCH 6/6] New --dirstat=lines mode, doing dirstat analysis based on diffstat Johan Herland
2011-04-26 16:59 ` Junio C Hamano
2011-04-27 2:02 ` Johan Herland
2011-04-26 0:15 ` [PATCH 0/6] --dirstat fixes, part 2 Linus Torvalds
2011-04-27 2:12 ` [PATCHv2 " Johan Herland
2011-04-27 2:12 ` [PATCHv2 1/6] Add several testcases for --dirstat and friends Johan Herland
2011-04-27 2:12 ` [PATCHv2 2/6] Make --dirstat=0 output directories that contribute < 0.1% of changes Johan Herland
2011-04-27 2:12 ` [PATCHv2 3/6] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file Johan Herland
2011-04-27 2:12 ` [PATCHv2 4/6] Add config variable for specifying default --dirstat behavior Johan Herland
2011-04-27 2:12 ` [PATCHv2 5/6] Use floating point for --dirstat percentages Johan Herland
2011-04-27 2:45 ` Linus Torvalds
2011-04-27 2:12 ` [PATCHv2 6/6] New --dirstat=lines mode, doing dirstat analysis based on diffstat Johan Herland
2011-04-27 8:24 ` [PATCHv3 0/6] --dirstat fixes, part 2 Johan Herland
2011-04-27 8:24 ` [PATCHv3 1/6] Add several testcases for --dirstat and friends Johan Herland
2011-04-27 8:24 ` [PATCHv3 2/6] Make --dirstat=0 output directories that contribute < 0.1% of changes Johan Herland
2011-04-27 8:24 ` [PATCHv3 3/6] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file Johan Herland
2011-04-27 8:24 ` [PATCHv3 4/6] Add config variable for specifying default --dirstat behavior Johan Herland
2011-04-27 8:24 ` [PATCHv3 5/6] Allow specifying --dirstat cut-off percentage as a floating point number Johan Herland
2011-04-27 8:37 ` Linus Torvalds
2011-04-27 10:29 ` [PATCHv4 " Johan Herland
2011-04-27 8:24 ` [PATCHv3 6/6] New --dirstat=lines mode, doing dirstat analysis based on diffstat Johan Herland
2011-04-28 1:17 ` [PATCHv5 0/7] --dirstat fixes, part 2 Johan Herland
2011-04-28 1:17 ` [PATCHv5 1/7] Add several testcases for --dirstat and friends Johan Herland
2011-04-28 1:17 ` [PATCHv5 2/7] Make --dirstat=0 output directories that contribute < 0.1% of changes Johan Herland
2011-04-28 1:17 ` [PATCHv5 3/7] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file Johan Herland
2011-04-28 1:17 ` [PATCHv5 4/7] Add config variable for specifying default --dirstat behavior Johan Herland
2011-04-28 1:17 ` [PATCHv5 5/7] Allow specifying --dirstat cut-off percentage as a floating point number Johan Herland
2011-04-28 1:17 ` [PATCHv5 6/7] New --dirstat=lines mode, doing dirstat analysis based on diffstat Johan Herland
2011-04-28 1:17 ` [PATCHv5 7/7] Improve error handling when parsing dirstat parameters Johan Herland
2011-04-28 18:41 ` Junio C Hamano
2011-04-28 19:20 ` Junio C Hamano
2011-04-28 23:16 ` Johan Herland
2011-04-28 23:13 ` Johan Herland
2011-04-29 4:06 ` Junio C Hamano
2011-04-29 9:36 ` [PATCHv6 0/8] --dirstat fixes, part 2 Johan Herland
2011-04-29 9:36 ` [PATCHv6 1/8] Add several testcases for --dirstat and friends Johan Herland
2011-04-29 9:36 ` [PATCHv6 2/8] Make --dirstat=0 output directories that contribute < 0.1% of changes Johan Herland
2011-04-29 9:36 ` [PATCHv6 3/8] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file Johan Herland
2011-04-29 9:36 ` [PATCHv6 4/8] Add config variable for specifying default --dirstat behavior Johan Herland
2011-04-29 9:36 ` [PATCHv6 5/8] Allow specifying --dirstat cut-off percentage as a floating point number Johan Herland
2011-04-29 9:36 ` [PATCHv6 6/8] New --dirstat=lines mode, doing dirstat analysis based on diffstat Johan Herland
2011-04-29 9:36 ` [PATCHv6 7/8] Improve error handling when parsing dirstat parameters Johan Herland
2011-04-29 9:36 ` [PATCHv6 8/8] Mark dirstat error messages for translation Johan Herland
2011-04-12 18:34 ` [RFC/PATCH 5/3] Alternative --dirstat implementation, based on diffstat analysis Junio C Hamano
2011-04-10 23:17 ` [PATCHv2 0/3] --dirstat fixes Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201104270402.10658.johan@herland.net \
--to=johan@herland.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).