* Re: [PATCH] real_path: make real_path thread-safe
From: Junio C Hamano @ 2016-12-06 23:44 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, peff, jacob.keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> + if (path->len > 1) {
> + char *last_slash = find_last_dir_sep(path->buf);
> + strbuf_setlen(path, last_slash - path->buf);
> + }
> +}
You use find_last_dir_sep() which takes care of "Windows uses
backslash" issue. Is this function expected to be fed something
like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or
is that handled by a lot higher level up in the callchain? I am
reacting the comparison of path->len and 1 here.
Also is the input expected to be normalized? Are we expected to be
fed something like "/a//b/./c/../d/e" and react sensibly, or is that
handled by a lot higher level up in the callchain?
> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
> + char *start = NULL;
> + char *end = NULL;
> +
> + strbuf_reset(next);
> +
> + /* look for the next component */
> + /* Skip sequences of multiple path-separators */
> + for (start = remaining->buf; is_dir_sep(*start); start++)
> + /* nothing */;
Style:
; /* nothing */
> + /* Find end of the path component */
> + for (end = start; *end && !is_dir_sep(*end); end++)
> + /* nothing */;
> +
> + strbuf_add(next, start, end - start);
OK, so this was given "///foo/bar" in "remaining" and appended
'foo/' to "next". I.e. deduping of slashes is handled here.
POSIX cares about treating "//" at the very beginning of the path
specially. Is that supposed to be handled here, or by a lot higher
level up in the callchain?
> + /* remove the component from 'remaining' */
> + strbuf_remove(remaining, 0, end - remaining->buf);
> +}
> +
> /* We allow "recursive" symbolic links. Only within reason, though. */
> -#define MAXDEPTH 5
> +#define MAXSYMLINKS 5
>
> /*
> * Return the real path (i.e., absolute path, with symlinks resolved
> @@ -21,7 +51,6 @@ int is_directory(const char *path)
> * absolute_path().) The return value is a pointer to a static
> * buffer.
> *
> * The directory part of path (i.e., everything up to the last
> * dir_sep) must denote a valid, existing directory, but the last
> * component need not exist. If die_on_error is set, then die with an
> @@ -33,22 +62,16 @@ int is_directory(const char *path)
> */
> static const char *real_path_internal(const char *path, int die_on_error)
> {
> + static struct strbuf resolved = STRBUF_INIT;
This being 'static' would probably mean that this is not reentrant,
which goes against the title of the patch.
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-12-06 23:40 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List, Christian Couder, Lars Schneider
In-Reply-To: <CAFZEwPNmB7rYvUTPy6dvfqfbUsjDeEcteLBBH5Wk-G_suE+YTw@mail.gmail.com>
Hi Pranit and Christian and Lars ;)
On 12/07/2016 12:02 AM, Pranit Bauva wrote:
> On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>> Okay Pranit,
>>
>> this is the last patch for me to review in this series.
>>
>> Now that I have a coarse overview of what you did, I have the general
>> remark that imho the "terms" variable should simply be global instead of
>> being passed around all the time.
>
> In a personal conversation with my mentors, we thought it is the best
> fit to keep it in a struct and pass it around so that it is easier in
> libification.
I guess you had that conversation at the beginning of the project and I
think that at that stage I would have recommended it that way, too.
However, looking at the v15 bisect--helper.c, it looks like it is rather
a pain to do it that way. I mean, you use the terms like they were
global variables: you initialize them in cmd_bisect__helper() and then
you always pass them around; you update them "globally", never using
restricted scope, never ever use a second instantiation besides the one
of cmd_bisect__helper(). These are all signs that *in the current code*
using (static) global variables is the better way (making the code simpler).
The libification argument may be worth considering, though. I am not
sure if there is really a use-case where you need two different
instantiations of the terms, *except* if the lib allows bisecting with
different terms at the same time (in different repos, of course). Is the
lib "aware" of such use-cases like doing stuff in different repos at the
same time? If that's the case, not using global variables is the right
thing to do. In any other case I can imagine, I'd suggest to use global
variables.
~Stephan
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-12-06 23:20 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPNmB7rYvUTPy6dvfqfbUsjDeEcteLBBH5Wk-G_suE+YTw@mail.gmail.com>
Hey Pranit,
On 12/07/2016 12:02 AM, Pranit Bauva wrote:
>>> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
>>> +{
>>> + struct strbuf line = STRBUF_INIT;
>>> + struct strbuf word = STRBUF_INIT;
>>> + FILE *fp = NULL;
>>
>> (The initialization is not necessary here.)
>
> Um. I think it is. Otherwise if it goes to the finish block before you
> try to operate on fp, it will cause a seg fault.
You are right, thanks!
>>> + while (strbuf_getline(&line, fp) != EOF) {
>>> + int pos = 0;
>>> + while (pos < line.len) {
>>> + pos = get_next_word(line.buf, pos, &word);
>>> +
>>> + if (!strcmp(word.buf, "git")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "git-bisect")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "bisect")) {
>>> + continue;
>>> + } else if (!strcmp(word.buf, "#")) {
>>> + break;
>>
>> Maybe it is more robust to check whether word.buf begins with #
>
> Assuming that you meant "# ", yes.
No, if I get it right "# " can never occur because the word.buf never
contains a space.
What I meant was that you are currently ignoring everything after a
"# ", so comments like
# foo
are ignored.
However, imagine a user changes the file by hand (he probably should not
do it but, hey, it's git: unixy, hacky ... and he thinks he knows what
he does) and then we have in the file something like
#foo
which makes perfectly sense when you are used to programming languages
with # as comment-till-eol marker. The problem is that your current code
does expect "#" as a single word and would hence not recognize #foo as a
comment.
I hope I made it clear why I suggested to test if the word *begins* with
"#" (not "# ").
~Stephan
^ permalink raw reply
* Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
From: Stephan Beyer @ 2016-12-06 23:05 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPNZCSGzeabHMH6wihz-51OUMMtnBMsffwJJVm9G8Fn=tw@mail.gmail.com>
Hey Pranit,
On 12/06/2016 10:14 PM, Pranit Bauva wrote:
>>> +
>>> + if (argc == 0) {
>>> + printf(_("Your current terms are %s for the old state\nand "
>>> + "%s for the new state.\n"), terms->term_good,
>>> + terms->term_bad);
>>
>> Very minor: It improves the readability if you'd split the string after
>> the \n and put the "and "in the next line.
>
> Ah. This is because of the message. If I do the other way, then it
> won't match the output in one of the tests in t/t6030 thus, I am
> keeping it that way in order to avoid modifying the file t/t6030.
I think I was unclear here. I was referring to the coding/layouting
style, not to the string. I mean like writing:
printf(_("Your current terms are %s for the old state\n"
"and "%s for the new state.\n"),
terms->term_good, terms->term_bad);
The string fed to _() is the same, but it is split in a different (imho
more readable) way: after the "\n", not after the "and ".
>>> + die(_("invalid argument %s for 'git bisect "
>>> + "terms'.\nSupported options are: "
>>> + "--term-good|--term-old and "
>>> + "--term-bad|--term-new."), argv[i]);
>>
>> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
>> this case. Because I am always looking from a library perspective, I'd
>> prefer "return error(...)".
>
> I should use return error()
When you reroll your patches, please also check if you always put _()
around your error()s ;) (Hmmm... On the other hand, it might be arguable
if translations are useful for errors that only occur when people hack
git-bisect or use the bisect--helper directly... This makes me feel like
all those errors should be prefixed by some "BUG: " marker since the
ordinary user only sees them when there is a bug. But I don't feel in
the position to decide or recommend such a thing, so it's just a thought.)
~Stephan
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Pranit Bauva @ 2016-12-06 23:02 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <1816d5b4-a4c1-7c97-09ff-b11001501423@gmx.net>
Hey Stephan,
On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Okay Pranit,
>
> this is the last patch for me to review in this series.
>
> Now that I have a coarse overview of what you did, I have the general
> remark that imho the "terms" variable should simply be global instead of
> being passed around all the time.
In a personal conversation with my mentors, we thought it is the best
fit to keep it in a struct and pass it around so that it is easier in
libification.
> I also had some other remarks but I forgot them... maybe they come to my
> mind again when I see patch series v16.
>
> I also want to remark again that I am not a Git developer and only
> reviewed this because of my interest in git-bisect. So some of my
> suggestions might conflict with other beliefs here. For example, I
> consider it very bad style to leak memory... but Git is rather written
> as a scripting tool than a genuine library, so perhaps many people here
> do not care about it as long as it works...
Thanks for taking out your time to review my series extremely
carefully. I will try to post a v16 next week probably.
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index c18ca07..b367d8d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> terms->term_good = arg;
>> } else if (!strcmp(arg, "--term-bad") ||
>> !strcmp(arg, "--term-new")) {
>> - const char *next_arg;
>
> This should already have been removed in patch 15/27, not here.
>
>> @@ -875,6 +875,117 @@ static int bisect_log(void)
>> return status;
>> }
>>
>> +static int get_next_word(const char *line, int pos, struct strbuf *word)
>> +{
>> + int i, len = strlen(line), begin = 0;
>> + strbuf_reset(word);
>> + for (i = pos; i < len; i++) {
>> + if (line[i] == ' ' && begin)
>> + return i + 1;
>> +
>> + if (!begin)
>> + begin = 1;
>> + strbuf_addch(word, line[i]);
>> + }
>> +
>> + return i;
>> +}
>> +
>> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
>> +{
>> + struct strbuf line = STRBUF_INIT;
>> + struct strbuf word = STRBUF_INIT;
>> + FILE *fp = NULL;
>
> (The initialization is not necessary here.)
Um. I think it is. Otherwise if it goes to the finish block before you
try to operate on fp, it will cause a seg fault.
>> + int res = 0;
>> +
>> + if (is_empty_or_missing_file(filename)) {
>> + error(_("no such file with name '%s' exists"), filename);
>
> The error message is misleading if the file exists but is empty.
> Maybe something like "cannot read file '%s' for replaying"?
Okay will change.
>> + res = -1;
>> + goto finish;
>
> goto fail;
> :D
>
>> + }
>> +
>> + if (bisect_reset(NULL)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + fp = fopen(filename, "r");
>> + if (!fp) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + while (strbuf_getline(&line, fp) != EOF) {
>> + int pos = 0;
>> + while (pos < line.len) {
>> + pos = get_next_word(line.buf, pos, &word);
>> +
>> + if (!strcmp(word.buf, "git")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "git-bisect")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "bisect")) {
>> + continue;
>> + } else if (!strcmp(word.buf, "#")) {
>> + break;
>
> Maybe it is more robust to check whether word.buf begins with #
Assuming that you meant "# ", yes.
>> + }
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, word.buf)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> +
>> + if (!strcmp(word.buf, "start")) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + sq_dequote_to_argv_array(line.buf+pos, &argv);
>> + if (bisect_start(terms, 0, argv.argv, argv.argc)) {
>> + argv_array_clear(&argv);
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + argv_array_clear(&argv);
>> + break;
>> + }
>> +
>> + if (one_of(word.buf, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + break;
>> + }
>> +
>> + if (!strcmp(word.buf, "terms")) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + sq_dequote_to_argv_array(line.buf+pos, &argv);
>> + if (bisect_terms(terms, argv.argv, argv.argc)) {
>> + argv_array_clear(&argv);
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + argv_array_clear(&argv);
>> + break;
>> + }
>> +
>> + error(_("?? what are you talking about?"));
>
> I know this string is taken from the original source. However, even
> something like error(_("Replay file contains rubbish (\"%s\")"),
> word.buf) is more informative.
Okay will change.
>> + res = -1;
>> + goto finish;
>
> goto fail;
>
>> + }
>> + }
>> + goto finish;
>
> +fail:
> + res = -1;
>
> I just wanted to make finally clear what I was referring to by the
> "goto fail" trick. :D
I got it. Will update accordingly.
> Also I think the readability could be improved by extracting the body of
> the outer while loop into an extra function replay_line(). Then most of
> my suggested "goto fail;" lines simply become "return -1;" :)
>
>> @@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
>> die(_("--bisect-log requires 0 arguments"));
>> res = bisect_log();
>> break;
>> + case BISECT_REPLAY:
>> + if (argc != 1)
>> + die(_("--bisect-replay requires 1 argument"));
>
> I'd keep the (already translated) string from the original source:
> "No logfile given"
Sure
Regards.
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
From: Pranit Bauva @ 2016-12-06 22:43 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <b7691fac-9642-f87d-f23f-5175b5ead05b@gmx.net>
Hey Stephan,
On Fri, Nov 18, 2016 at 1:55 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi Pranit,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3f19b68..c6c11e3 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
>> N_("git bisect--helper --bisect-clean-state"),
>> N_("git bisect--helper --bisect-reset [<commit>]"),
>> N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
>> + N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),
>
> Here's the same as in the previous patch... I'd not use
> TERM_GOOD/TERM_BAD in capitals.
Sure.
>> NULL
>> };
>>
>> @@ -212,6 +213,38 @@ static int bisect_write(const char *state, const char *rev,
>> return retval;
>> }
>>
>> +static int set_terms(struct bisect_terms *terms, const char *bad,
>> + const char *good)
>> +{
>> + terms->term_good = xstrdup(good);
>> + terms->term_bad = xstrdup(bad);
>> + return write_terms(terms->term_bad, terms->term_good);
>
> At this stage of the patch series I am wondering why you are setting
> "terms" here, but I guess you'll need it later.
>
> However, you are leaking memory here. Something like
>
> free(terms->term_good);
> free(terms->term_bad);
> terms->term_good = xstrdup(good);
> terms->term_bad = xstrdup(bad);
>
> should be safe (because you've always used xstrdup() for the terms
> members before). Or am I overseeing something?
Yeah it is safe.
>> @@ -278,6 +314,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> terms.term_bad = xstrdup(argv[3]);
>> res = bisect_write(argv[0], argv[1], &terms, nolog);
>> break;
>> + case CHECK_AND_SET_TERMS:
>> + if (argc != 3)
>> + die(_("--check-and-set-terms requires 3 arguments"));
>> + terms.term_good = xstrdup(argv[1]);
>> + terms.term_bad = xstrdup(argv[2]);
>> + res = check_and_set_terms(&terms, argv[0]);
>> + break;
>
> Ha! When I reviewed the last patch, I asked you why you changed the code
> from returning directly from each subcommand to setting res; break; and
> then return res at the bottom of the function.
>
> Now I see why this was useful. The two members of "terms" are again
> leaking memory: you are allocating memory by using xstrdup() but you are
> not freeing it.
I will take care about freeing the memory.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C
From: Pranit Bauva @ 2016-12-06 22:42 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <c49a6151-0827-cae5-0569-8f05515be0f9@gmx.net>
Hey Stephan,
On Fri, Nov 18, 2016 at 3:17 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 493034c..c18ca07 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -858,6 +858,23 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,
>> return -1;
>> }
>>
>> +static int bisect_log(void)
>> +{
>> + int fd, status;
>> + fd = open(git_path_bisect_log(), O_RDONLY);
>> + if (fd < 0)
>> + return -1;
>> +
>> + status = copy_fd(fd, 1);
>
> Perhaps
>
> status = copy_fd(fd, STDOUT_FILENO);
Sure!
>> + if (status) {
>> + close(fd);
>> + return -1;
>> + }
>> +
>> + close(fd);
>> + return status;
>> +}
>
> That's weird.
> Either get rid of the if() and actually use status:
>
> status = copy_fd(fd, STDOUT_FILENO);
>
> close(fd);
> return status ? -1 : 0;
This one seems better!
^ permalink raw reply
* Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C
From: Pranit Bauva @ 2016-12-06 22:40 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <0cc22155-2b3f-b69c-9ed4-2b1c55e40eee@gmx.net>
Hey Stephan,
On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> Reimplement the `bisect_state` shell function in C and also add a
>> subcommand `--bisect-state` to `git-bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-state` subcommand is a temporary measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other methods.
>>
>> `bisect_head` is called from `bisect_state` thus its not required to
>> introduce another subcommand.
>
> Missing comma before "thus", and "it is" (or "it's") instead of "its" :)
Sure, will fix.
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1767916..1481c6d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>> return 0;
>> }
>>
>> +static char *bisect_head(void)
>> +{
>> + if (is_empty_or_missing_file(git_path_bisect_head()))
>> + return "HEAD";
>> + else
>> + return "BISECT_HEAD";
>> +}
>
> This is very shellish.
> In C I'd expect something like
>
> static int bisect_head_sha1(unsigned char *sha)
> {
> int res;
> if (is_empty_or_missing_file(git_path_bisect_head()))
> res = get_sha1("HEAD", sha);
> else
> res = get_sha1("BISECT_HEAD", sha);
>
> if (res)
> return error(_("Could not find BISECT_HEAD or HEAD."));
>
> return 0;
> }
>
>> +
>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>> + int argc)
>> +{
>> + const char *state = argv[0];
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, state))
>> + return -1;
>> +
>> + if (!argc)
>> + die(_("Please call `--bisect-state` with at least one argument"));
>
> I think this check should move to cmd_bisect__helper. There are also the
> other argument number checks.
Not really. After the whole conversion, the cmdmode will cease to
exists while bisect_state will be called directly, thus it is
important to check it here.
>> +
>> + if (argc == 1 && one_of(state, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + const char *bisected_head = xstrdup(bisect_head());
>> + const char *hex[1];
>
> Maybe:
> const char *hex;
>
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(bisected_head, sha1))
>> + die(_("Bad rev input: %s"), bisected_head);
>
> And instead of...
>
>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>> + return -1;
>> +
>> + *hex = xstrdup(sha1_to_hex(sha1));
>> + if (check_expected_revs(hex, 1))
>> + return -1;
>
> ... simply:
>
> hex = xstrdup(sha1_to_hex(sha1));
> if (set_state(terms, state, hex)) {
> free(hex);
> return -1;
> }
> free(hex);
>
> where:
Yes I am planning to convert all places with hex rather than the sha1
but not yet, maybe in an another patch series because currently a lot
of things revolve around sha1 and changing its behaviour wouldn't
really be a part of a porting patch series.
> static int set_state(struct bisect_terms *terms, const char *state,
> const char *hex)
> {
> if (bisect_write(state, hex, terms, 0))
> return -1;
> if (check_expected_revs(&hex, 1))
> return -1;
> return 0;
> }
>
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
>> + one_of(state, terms->term_good, "skip", NULL)) {
>> + int i;
>> + struct string_list hex = STRING_LIST_INIT_DUP;
>> +
>> + for (i = 1; i < argc; i++) {
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(argv[i], sha1)) {
>> + string_list_clear(&hex, 0);
>> + die(_("Bad rev input: %s"), argv[i]);
>> + }
>> + string_list_append(&hex, sha1_to_hex(sha1));
>> + }
>> + for (i = 0; i < hex.nr; i++) {
>
> ... And replace this:
>
>> + const char **hex_string = (const char **) &hex.items[i].string;
>> + if(bisect_write(state, *hex_string, terms, 0)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>> + if (check_expected_revs(hex_string, 1)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>
> by:
>
> const char *hex_str = hex.items[i].string;
> if (set_state(terms, state, hex_string)) {
> string_list_clear(&hex, 0);
> return -1;
> }
>
>> + }
>> + string_list_clear(&hex, 0);
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if (!strcmp(state, terms->term_bad))
>> + die(_("'git bisect %s' can take only one argument."),
>> + terms->term_bad);
>> +
>> + return -1;
>> +}
>> +
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> {
>> enum {
>> @@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
>> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
>> N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
>> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
>> + N_("mark the state of ref (or refs)"), BISECT_STATE),
>
> "mark the state of the given revs"
>
> Note that rev != ref
Might have been a typo. Will fix.
>> @@ -902,6 +980,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> terms.term_bad = "bad";
>> res = bisect_autostart(&terms);
>> break;
>> + case BISECT_STATE:
>> + if (argc == 0)
>> + die(_("--bisect-state requires at least 1 argument"));
>
> "at least one revision"
Okay, that would make it more specific.
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index cd56551..a9eebbb 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -61,44 +51,7 @@ bisect_skip() {
>> esac
>> all="$all $revs"
>> done
>> - eval bisect_state 'skip' $all
> [...deleted lines...]
>> + eval git bisect--helper --bisect-state 'skip' $all
>
> I think you don't need "eval" here any longer.
Yes, I wouldn't
>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>> state="$TERM_GOOD"
>> fi
>>
>> - # We have to use a subshell because "bisect_state" can exit.
>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>> + # We have to use a subshell because "--bisect-state" can exit.
>> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
True, but right now I didn't want to modify that part of the source
code to remove the comment. I will remove the comment all together
when I port bisect_run()
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH 10/17] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2016-12-06 22:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <CAGZ79kausxZMinZymG8mE4jQ1pi4yJ80WRBUGFhUK7mmfOBCvg@mail.gmail.com>
On 12/06, Stefan Beller wrote:
> On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
>
> > struct strbuf sb = STRBUF_INIT;
> > - if (prefixlen && !literal_global) {
> > - /* Preserve the actual prefix length of each pattern */
> > - if (short_magic)
> > - prefix_short_magic(&sb, prefixlen, short_magic);
> > - else if (long_magic_end) {
> > - strbuf_add(&sb, elt, long_magic_end - elt);
> > - strbuf_addf(&sb, ",prefix:%d)", prefixlen);
> > - } else
> > - strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
>
> This fixes the issue with add -p . mentioned somewhere else on the mailing list.
>
> > - }
> > +
> > + /* Preserve the actual prefix length of each pattern */
> > + prefix_magic(&sb, prefixlen, element_magic);
> > +
>
> Did you find a reason why we passed magic literally, i.e. short magic
> was passed as short magic and long magic as long magic before?
>
> I cannot think of any reason why that would have been the case,
> but I assume there had to be a reason for that.
nope, perhaps it was because we technically already have the long magic
string and the short magic needs to be converted to long magic (as you
can't mix short and long magic).
> Another note: This collides with the attr system refactoring, which I
> postpone redoing until the submodule checkout is done, so maybe
> you want to pickup this patch:
> https://public-inbox.org/git/20161110203428.30512-31-sbeller@google.com/
> which only relies on one patch prior
> https://public-inbox.org/git/20161110203428.30512-30-sbeller@google.com/
After looking at those patches I think I do something extremely similar
in a future patch in this series, the parse_long_magic patch.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Stefan Beller @ 2016-12-06 22:30 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481061106-117775-17-git-send-email-bmwill@google.com>
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
> A few small changes to improve readability. This is done by grouping related
> assignments, adding blank lines, ensuring lines are <80 characters, etc.
The 'etc' sounds a bit sloppy in the commit message.
Maybe s/etc/and adding proper comments/ ?
Code looks good.
^ permalink raw reply
* Re: [PATCH 10/17] pathspec: simpler logic to prefix original pathspec elements
From: Stefan Beller @ 2016-12-06 22:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481061106-117775-11-git-send-email-bmwill@google.com>
On Tue, Dec 6, 2016 at 1:51 PM, Brandon Williams <bmwill@google.com> wrote:
> struct strbuf sb = STRBUF_INIT;
> - if (prefixlen && !literal_global) {
> - /* Preserve the actual prefix length of each pattern */
> - if (short_magic)
> - prefix_short_magic(&sb, prefixlen, short_magic);
> - else if (long_magic_end) {
> - strbuf_add(&sb, elt, long_magic_end - elt);
> - strbuf_addf(&sb, ",prefix:%d)", prefixlen);
> - } else
> - strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
This fixes the issue with add -p . mentioned somewhere else on the mailing list.
> - }
> +
> + /* Preserve the actual prefix length of each pattern */
> + prefix_magic(&sb, prefixlen, element_magic);
> +
Did you find a reason why we passed magic literally, i.e. short magic
was passed as short magic and long magic as long magic before?
I cannot think of any reason why that would have been the case,
but I assume there had to be a reason for that.
Another note: This collides with the attr system refactoring, which I
postpone redoing until the submodule checkout is done, so maybe
you want to pickup this patch:
https://public-inbox.org/git/20161110203428.30512-31-sbeller@google.com/
which only relies on one patch prior
https://public-inbox.org/git/20161110203428.30512-30-sbeller@google.com/
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-06 22:24 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206135113.i7nlr45vg7uzgfcn@sigill.intra.peff.net>
On 12/06, Jeff King wrote:
> On Mon, Dec 05, 2016 at 12:04:52PM -0800, Junio C Hamano wrote:
>
> > > I'm sending out another reroll of this series so that in Jeff's he can
> > > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > > option, which should make this test stop barfing.
> >
> > I was hoping to eventually merge Peff's series to older maintenance
> > tracks. How bad would it be if we rebased the v8 of this series
> > together with Peff's series to say v2.9 (or even older if it does
> > not look too bad)?
>
> My series actually fixes existing security problems, so I'd consider it
> a bug-fix. I _think_ Brandon's series is purely about allowing more
> expressiveness in the whitelist policy, and so could be considered more
> of a feature.
Yes this was really the main intent on my series.
> So one option is to apply my series for older 'maint', and then just
> rebase Brandon's on top of that for 'master'.
>
> I don't know if that makes things any easier. I feel funny saying "no,
> no, mine preempts yours because it is more maint-worthy", but I think
> that order does make sense.
>
> I think it would be OK to put Brandon's on maint, too, though. It is a
> refactor of an existing security feature to make it more featureful, but
> the way it is implemented could not cause security regressions unless
> you use the new feature (IOW, we still respect the whitelist environment
> exactly as before).
Either way let me know if there is something I need to do.
--
Brandon Williams
^ permalink raw reply
* Re* [BUG] Index.lock error message regression in git 2.11.0
From: Junio C Hamano @ 2016-12-06 22:15 UTC (permalink / raw)
To: Robbie Iannucci; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqbmwozppx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Perhaps the attached would fix it (not even compile tested, though)?
>
> I would prefer to make 0 to mean "show error but return -1", 1 to
> mean "die on error", and 2 to mean "be silent and return -1 on
> error", though. Asking to be silent should be the exception for
> this error from usability and safety's point of view, and requiring
> such exceptional callers to pass LOCK_SILENT_ON_ERROR would be
> easier to "git grep" for them.
So here is the "You have to ask explicitly, if you know that it is
safe to be silent" version with a proper log message.
-- >8 --
Subject: [PATCH] lockfile: LOCK_SILENT_ON_ERROR
Recent "libify merge machinery" stopped from passing die_on_error
bit to hold_locked_index(), and lost an error message when there are
competing update in progress with "git merge --ff-only $commit", for
example. The command still exits with a non-zero status, but that
is not of much help for an interactive user. The last thing the
command says is "Updating $from..$to". We used to follow it with a
big error message that makes it clear that "merge --ff-only" did not
succeed.
Introduce a new bit "LOCK_SILENT_ON_ERROR" that can be passed by
callers that do want to silence the message (because they either
make it a non-error by doing something else, or they show their own
error message to explain the situation), and show the error message
we used to give for everybody else, including the caller that was
touched by the libification in question.
I would not be surprised if some existing calls to hold_lock*()
functions that pass die_on_error=0 need to be updated to pass
LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look
like a regression, but we are better off starting louder and squelch
the ones that we find safe to make silent than the other way around.
Reported-by: Robbie Iannucci <iannucci@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
lockfile.c | 11 +++++++++--
lockfile.h | 8 +++++++-
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f7e8104449 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
int flags, long timeout_ms)
{
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
- if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_die(path, errno);
+ if (fd < 0) {
+ if (flags & LOCK_DIE_ON_ERROR)
+ unable_to_lock_die(path, errno);
+ else if (!(flags & LOCK_SILENT_ON_ERROR)) {
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(path, errno, &buf);
+ error("%s", buf.buf);
+ }
+ }
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..98b4862254 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,9 +129,15 @@ struct lock_file {
/*
* If a lock is already taken for the file, `die()` with an error
* message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked gives the same error message and returns -1 to
+ * the caller.
*/
#define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to be totally silent and return
+ * -1 to the caller upon error with this flag
+ */
+#define LOCK_SILENT_ON_ERROR 2
/*
* Usually symbolic links in the destination path are resolved. This
--
2.11.0-270-g0b6beed61f
^ permalink raw reply related
* Re: [BUG] Index.lock error message regression in git 2.11.0
From: Junio C Hamano @ 2016-12-06 21:56 UTC (permalink / raw)
To: Robbie Iannucci; +Cc: git, Johannes Schindelin
In-Reply-To: <CA+q_oBdHytoeSD-hmLx_N473M8XinjqckvE35Re3eNpQRWYjHQ@mail.gmail.com>
Robbie Iannucci <iannucci@google.com> writes:
> I just upgraded to 2.11.0 from 2.10.2, and I noticed that some
> commands no longer print an error message when the `index.lock` file
> exists (such as `git merge --ff-only`).
>
> It appears this bug was introduced in
> 55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify
> checkout_fast_forward()). I determined this by running the attached
> bisect script (on OS X, but I don't think that matters; I've also seen
> the error message missing on Linux):
Yes, I can see that this is not limited to OSX. The command does
exit with non-zero status, but that is not of much help for an
interactive user.
$ git checkout v2.11.0^0
$ >.git/index.lock
$ git merge --ff-only master; echo $?
Updating 454cb6bd52..8d7a455ed5
1
$ exit
We can see that it attempted to update, but because there is no
indication that it failed, the user can easily be misled to think
that we are now at the tip of the master branch.
We used to give "fatal: Unable to create ..." followed by "Another
git process seems to be running..." advice, that would have helped
the user from the confusion.
I do not think it is the right solution to call hold_locked_index()
with die_on_error=1 from the codepath changed by your 55f5704da6
("sequencer: lib'ify checkout_fast_forward()", 2016-09-09). Perhaps
the caller of checkout_fast_forward() needs to learn how/why the
function returned error and diagnose this case and a message? Or
perhaps turn that die_on_error parameter from boolean to tricolor
(i.e. the caller does not want you to die, but the caller knows that
you have more information to give better error message than it does,
so please show an error message instead of silently returning -1)?
Perhaps the attached would fix it (not even compile tested, though)?
I would prefer to make 0 to mean "show error but return -1", 1 to
mean "die on error", and 2 to mean "be silent and return -1 on
error", though. Asking to be silent should be the exception for
this error from usability and safety's point of view, and requiring
such exceptional callers to pass LOCK_SILENT_ON_ERROR would be
easier to "git grep" for them.
Dscho, what do you think?
lockfile.c | 11 +++++++++--
lockfile.h | 5 +++++
merge.c | 2 +-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f190caddb0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
int flags, long timeout_ms)
{
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
- if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_die(path, errno);
+ if (fd < 0) {
+ if (flags & LOCK_DIE_ON_ERROR)
+ unable_to_lock_die(path, errno);
+ else if (flags & LOCK_ERROR_ON_ERROR) {
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(path, errno, &buf);
+ error("%s", buf.buf);
+ }
+ }
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..6cba9c8142 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -132,6 +132,11 @@ struct lock_file {
* is already locked returns -1 to the caller.
*/
#define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to show the usual error message
+ * and still return -1 to the caller with this flag
+ */
+#define LOCK_ERROR_ON_ERROR 2
/*
* Usually symbolic links in the destination path are resolved. This
diff --git a/merge.c b/merge.c
index 23866c9165..9e2e4f1a22 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- if (hold_locked_index(lock_file, 0) < 0)
+ if (hold_locked_index(lock_file, LOCK_ERROR_ON_ERROR) < 0)
return -1;
memset(&trees, 0, sizeof(trees));
^ permalink raw reply related
* [PATCH 15/17] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 793caf1..41aa213 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -257,6 +257,44 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
return parse_short_magic(magic, elem);
}
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+ int i;
+
+ if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
+ (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
+ S_ISGITLINK(active_cache[i]->ce_mode)) {
+ item->len--;
+ item->match[item->len] = '\0';
+ }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len <= ce_len || item->match[ce_len] != '/' ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ if (item->len == ce_len + 1) {
+ /* strip trailing slash */
+ item->len--;
+ item->match[item->len] = '\0';
+ } else {
+ die (_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+ }
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
- int i, pathspec_prefix = -1;
+ int pathspec_prefix = -1;
/* PATHSPEC_LITERAL_PATH ignores magic */
if (!(flags & PATHSPEC_LITERAL_PATH)) {
@@ -327,33 +365,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
item->len = strlen(item->match);
item->prefix = prefixlen;
- if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
- (item->len >= 1 && item->match[item->len - 1] == '/') &&
- (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
- S_ISGITLINK(active_cache[i]->ce_mode)) {
- item->len--;
- match[item->len] = '\0';
- }
+ if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+ strip_submodule_slash_cheap(item);
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- int ce_len = ce_namelen(ce);
-
- if (!S_ISGITLINK(ce->ce_mode))
- continue;
-
- if (item->len <= ce_len || match[ce_len] != '/' ||
- memcmp(ce->name, match, ce_len))
- continue;
- if (item->len == ce_len + 1) {
- /* strip trailing slash */
- item->len--;
- match[item->len] = '\0';
- } else
- die (_("Pathspec '%s' is in submodule '%.*s'"),
- elt, ce_len, ce->name);
- }
+ strip_submodule_slash_expensive(item);
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 17/17] pathspec: remove outdated comment
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Remove part of the function header comment to prefix_pathspec as it is
no longer relevant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 8a07b02..66db257 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -298,15 +298,6 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
- *
- * For now, we only parse the syntax and throw out anything other than
- * "top" magic.
- *
- * NEEDSWORK: This needs to be rewritten when we start migrating
- * get_pathspec() users to use the "struct pathspec" interface. For
- * example, a pathspec element may be marked as case-insensitive, but
- * the prefix part must always match literally, and a single stupid
- * string cannot express such a case.
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 05/17] pathspec: remove the deprecated get_pathspec function
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c | 42 +++--------------------------------
pathspec.h | 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
index 540e455..eb1fa98 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:
- prefix and args come from cmd_* functions
-get_pathspec() is obsolete and should never be used in new code.
-
parse_pathspec() helps catch unsupported features and reject them
politely. At a lower level, different pathspec-related functions may
not support the same set of features. Such pathspec-sensitive
diff --git a/cache.h b/cache.h
index a50a61a..0f80e01 100644
--- a/cache.h
+++ b/cache.h
@@ -514,7 +514,6 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
extern void setup_work_tree(void);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
diff --git a/pathspec.c b/pathspec.c
index 22ca74a..1f918cb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -103,7 +103,7 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
- const char **raw, unsigned flags,
+ unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -240,7 +240,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
- *raw = item->match = match;
+ item->match = match;
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -381,8 +381,6 @@ void parse_pathspec(struct pathspec *pathspec,
/* No arguments with prefix -> prefix pathspec */
if (!entry) {
- static const char *raw[2];
-
if (flags & PATHSPEC_PREFER_FULL)
return;
@@ -394,10 +392,7 @@ void parse_pathspec(struct pathspec *pathspec,
item->original = prefix;
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
- raw[0] = prefix;
- raw[1] = NULL;
pathspec->nr = 1;
- pathspec->_raw = raw;
return;
}
@@ -415,7 +410,6 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
item = pathspec->items;
- pathspec->_raw = argv;
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
@@ -423,7 +417,7 @@ void parse_pathspec(struct pathspec *pathspec,
entry = argv[i];
item[i].magic = prefix_pathspec(item + i, &short_magic,
- argv + i, flags,
+ flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -457,36 +451,6 @@ void parse_pathspec(struct pathspec *pathspec,
}
}
-/*
- * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
- * based interface - see pathspec.c:parse_pathspec().
- *
- * Arguments:
- * - prefix - a path relative to the root of the working tree
- * - pathspec - a list of paths underneath the prefix path
- *
- * Iterates over pathspec, prepending each path with prefix,
- * and return the resulting list.
- *
- * If pathspec is empty, return a singleton list containing prefix.
- *
- * If pathspec and prefix are both empty, return an empty list.
- *
- * This is typically used by built-in commands such as add.c, in order
- * to normalize argv arguments provided to the built-in into a list of
- * paths to process, all relative to the root of the working tree.
- */
-const char **get_pathspec(const char *prefix, const char **pathspec)
-{
- struct pathspec ps;
- parse_pathspec(&ps,
- PATHSPEC_ALL_MAGIC &
- ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
- PATHSPEC_PREFER_CWD,
- prefix, pathspec);
- return ps._raw;
-}
-
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
*dst = *src;
diff --git a/pathspec.h b/pathspec.h
index 59809e4..70a592e 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -19,7 +19,6 @@
#define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR */
struct pathspec {
- const char **_raw; /* get_pathspec() result, not freed by clear_pathspec() */
int nr;
unsigned int has_wildcard:1;
unsigned int recursive:1;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 04/17] ls-tree: convert show_recursive to use the pathspec struct interface
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-tree.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 0e30d86..e0f4307 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -31,21 +31,18 @@ static const char * const ls_tree_usage[] = {
static int show_recursive(const char *base, int baselen, const char *pathname)
{
- const char **s;
+ int i;
if (ls_options & LS_RECURSIVE)
return 1;
- s = pathspec._raw;
- if (!s)
+ if (!pathspec.nr)
return 0;
- for (;;) {
- const char *spec = *s++;
+ for (i = 0; i < pathspec.nr; i++) {
+ const char *spec = pathspec.items[i].match;
int len, speclen;
- if (!spec)
- return 0;
if (strncmp(base, spec, baselen))
continue;
len = strlen(pathname);
@@ -59,6 +56,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
continue;
return 1;
}
+ return 0;
}
static int show_tree(const unsigned char *sha1, struct strbuf *base,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 06/17] pathspec: copy and free owned memory
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 22 ++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1f918cb..8f367f0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
- } else
- item->original = elt;
+ } else {
+ item->original = xstrdup(elt);
+ }
item->len = strlen(item->match);
item->prefix = prefixlen;
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
pathspec->items = item = xcalloc(1, sizeof(*item));
- item->match = prefix;
- item->original = prefix;
+ item->match = xstrdup(prefix);
+ item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,26 @@ void parse_pathspec(struct pathspec *pathspec,
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
+ int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+ for (i = 0; i < dst->nr; i++) {
+ dst->items[i].match = xstrdup(src->items[i].match);
+ dst->items[i].original = xstrdup(src->items[i].original);
+ }
}
void clear_pathspec(struct pathspec *pathspec)
{
+ int i;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ free(pathspec->items[i].match);
+ free(pathspec->items[i].original);
+ }
free(pathspec->items);
pathspec->items = NULL;
}
diff --git a/pathspec.h b/pathspec.h
index 70a592e..49fd823 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
- const char *match;
- const char *original;
+ char *match;
+ char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 07/17] mv: small code cleanup
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Now that the call to 'parse_pathspec()' doesn't modify the passed in
const char **array there isn't a need to duplicate the pathspec element
prior to freeing the intermediate strings. This small cleanup just
makes the code a bit easier to read.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/mv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 4df4a12..b7cceb6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -56,9 +56,8 @@ static const char **internal_copy_pathspec(const char *prefix,
/* Copy the pathspec and free the old intermediate strings */
for (i = 0; i < count; i++) {
- const char *match = xstrdup(ps.items[i].match);
free((char *) result[i]);
- result[i] = match;
+ result[i] = xstrdup(ps.items[i].match);
}
clear_pathspec(&ps);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 12/17] pathspec: create parse_short_magic function
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 54 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 08e76f6..d4d4839 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -156,6 +156,41 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for short magic
+ *
+ * saves all magic in 'magic'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_short_magic(unsigned *magic, const char *elem)
+{
+ const char *pos;
+
+ for (pos = elem + 1; *pos && *pos != ':'; pos++) {
+ char ch = *pos;
+ int i;
+
+ if (!is_pathspec_magic(ch))
+ break;
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (pathspec_magic[i].mnemonic == ch) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Unimplemented pathspec magic '%c' in '%s'"),
+ ch, elem);
+ }
+
+ if (*pos == ':')
+ pos++;
+
+ return pos;
+}
+
+/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
*
@@ -220,24 +255,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
copyfrom++;
} else {
/* shorthand */
- for (copyfrom = elt + 1;
- *copyfrom && *copyfrom != ':';
- copyfrom++) {
- char ch = *copyfrom;
-
- if (!is_pathspec_magic(ch))
- break;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (pathspec_magic[i].mnemonic == ch) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Unimplemented pathspec magic '%c' in '%s'"),
- ch, elt);
- }
- if (*copyfrom == ':')
- copyfrom++;
+ copyfrom = parse_short_magic(&element_magic, elt);
}
magic |= element_magic;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 11/17] pathspec: factor global magic into its own function
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 74 insertions(+), 46 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 5afebd3..08e76f6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -87,6 +87,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
+static inline int get_literal_global(void)
+{
+ static int literal_global = -1;
+
+ if (literal_global < 0)
+ literal_global = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,
+ 0);
+ return literal_global;
+}
+
+static inline int get_glob_global(void)
+{
+ static int glob_global = -1;
+
+ if (glob_global < 0)
+ glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
+ return glob_global;
+}
+
+static inline int get_noglob_global(void)
+{
+ static int noglob_global = -1;
+
+ if (noglob_global < 0)
+ noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,
+ 0);
+ return noglob_global;
+}
+
+static inline int get_icase_global(void)
+{
+ static int icase_global = -1;
+
+ if (icase_global < 0)
+ icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+
+ return icase_global;
+}
+
+static int get_global_magic(int element_magic)
+{
+ int global_magic = 0;
+
+ if (get_literal_global())
+ global_magic |= PATHSPEC_LITERAL;
+
+ /* --glob-pathspec is overridden by :(literal) */
+ if (get_glob_global() && !(element_magic & PATHSPEC_LITERAL))
+ global_magic |= PATHSPEC_GLOB;
+
+ if (get_glob_global() && get_noglob_global())
+ die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));
+
+ if (get_icase_global())
+ global_magic |= PATHSPEC_ICASE;
+
+ if ((global_magic & PATHSPEC_LITERAL) &&
+ (global_magic & ~PATHSPEC_LITERAL))
+ die(_("global 'literal' pathspec setting is incompatible "
+ "with all other global pathspec settings"));
+
+ /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
+ if (get_noglob_global() && !(element_magic & PATHSPEC_GLOB))
+ global_magic |= PATHSPEC_LITERAL;
+
+ return global_magic;
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -105,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
const char *prefix, int prefixlen,
const char *elt)
{
- static int literal_global = -1;
- static int glob_global = -1;
- static int noglob_global = -1;
- static int icase_global = -1;
- unsigned magic = 0, element_magic = 0, global_magic = 0;
+ unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
- if (literal_global < 0)
- literal_global = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
- if (literal_global)
- global_magic |= PATHSPEC_LITERAL;
-
- if (glob_global < 0)
- glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
- if (glob_global)
- global_magic |= PATHSPEC_GLOB;
-
- if (noglob_global < 0)
- noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
-
- if (glob_global && noglob_global)
- die(_("global 'glob' and 'noglob' pathspec settings are incompatible"));
-
-
- if (icase_global < 0)
- icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
- if (icase_global)
- global_magic |= PATHSPEC_ICASE;
-
- if ((global_magic & PATHSPEC_LITERAL) &&
- (global_magic & ~PATHSPEC_LITERAL))
- die(_("global 'literal' pathspec setting is incompatible "
- "with all other global pathspec settings"));
-
- if (flags & PATHSPEC_LITERAL_PATH)
- global_magic = 0;
-
- if (elt[0] != ':' || literal_global ||
+ if (elt[0] != ':' || get_literal_global() ||
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
@@ -208,15 +242,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
magic |= element_magic;
- /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
- if (noglob_global && !(magic & PATHSPEC_GLOB))
- global_magic |= PATHSPEC_LITERAL;
-
- /* --glob-pathspec is overridden by :(literal) */
- if ((global_magic & PATHSPEC_GLOB) && (magic & PATHSPEC_LITERAL))
- global_magic &= ~PATHSPEC_GLOB;
-
- magic |= global_magic;
+ /* PATHSPEC_LITERAL_PATH ignores magic */
+ if (!(flags & PATHSPEC_LITERAL_PATH))
+ magic |= get_global_magic(element_magic);
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
@@ -242,7 +270,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
* original. Useful for passing to another command.
*/
if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
- prefixlen && !literal_global) {
+ prefixlen && !get_literal_global()) {
struct strbuf sb = STRBUF_INIT;
/* Preserve the actual prefix length of each pattern */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 14/17] pathspec: create parse_element_magic helper
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Factor out the logic responsible for the magic in a pathspec element
into its own function.
Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1d28679..793caf1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -244,6 +244,19 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
return pos;
}
+static const char *parse_element_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ if (elem[0] != ':' || get_literal_global())
+ return elem; /* nothing to do */
+ else if (elem[1] == '(')
+ /* longhand */
+ return parse_long_magic(magic, prefix_len, elem);
+ else
+ /* shorthand */
+ return parse_short_magic(magic, elem);
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -267,24 +280,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
char *match;
int i, pathspec_prefix = -1;
- if (elt[0] != ':' || get_literal_global() ||
- (flags & PATHSPEC_LITERAL_PATH)) {
- ; /* nothing to do */
- } else if (elt[1] == '(') {
- /* longhand */
- copyfrom = parse_long_magic(&element_magic,
- &pathspec_prefix,
- elt);
- } else {
- /* shorthand */
- copyfrom = parse_short_magic(&element_magic, elt);
- }
-
- magic |= element_magic;
-
/* PATHSPEC_LITERAL_PATH ignores magic */
- if (!(flags & PATHSPEC_LITERAL_PATH))
+ if (!(flags & PATHSPEC_LITERAL_PATH)) {
+ copyfrom = parse_element_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
+ magic |= element_magic;
magic |= get_global_magic(element_magic);
+ }
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 13/17] pathspec: create parse_long_magic function
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Factor out the logic responsible for parsing long magic into its own
function. As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 92 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 57 insertions(+), 35 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index d4d4839..1d28679 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -156,6 +156,60 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for long magic
+ *
+ * saves all magic in 'magic'
+ * if prefix magic is used, save the prefix length in 'prefix_len'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_long_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ const char *pos;
+ const char *nextat;
+
+ for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
+ size_t len = strcspn(pos, ",)");
+ int i;
+
+ if (pos[len] == ',')
+ nextat = pos + len + 1; /* handle ',' */
+ else
+ nextat = pos + len; /* handle ')' and '\0' */
+
+ if (!len)
+ continue;
+
+ if (starts_with(pos, "prefix:")) {
+ char *endptr;
+ *prefix_len = strtol(pos + 7, &endptr, 10);
+ if (endptr - pos != len)
+ die(_("invalid parameter for pathspec magic 'prefix'"));
+ continue;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (strlen(pathspec_magic[i].name) == len &&
+ !strncmp(pathspec_magic[i].name, pos, len)) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Invalid pathspec magic '%.*s' in '%s'"),
+ (int) len, pos, elem);
+ }
+
+ if (*pos != ')')
+ die(_("Missing ')' at the end of pathspec magic in '%s'"),
+ elem);
+ pos++;
+
+ return pos;
+}
+
+/*
* Parse the pathspec element looking for short magic
*
* saves all magic in 'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
; /* nothing to do */
} else if (elt[1] == '(') {
/* longhand */
- const char *nextat;
- for (copyfrom = elt + 2;
- *copyfrom && *copyfrom != ')';
- copyfrom = nextat) {
- size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == ',')
- nextat = copyfrom + len + 1;
- else
- /* handle ')' and '\0' */
- nextat = copyfrom + len;
- if (!len)
- continue;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
- if (strlen(pathspec_magic[i].name) == len &&
- !strncmp(pathspec_magic[i].name, copyfrom, len)) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (starts_with(copyfrom, "prefix:")) {
- char *endptr;
- pathspec_prefix = strtol(copyfrom + 7,
- &endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for pathspec magic 'prefix'"));
- /* "i" would be wrong, but it does not matter */
- break;
- }
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Invalid pathspec magic '%.*s' in '%s'"),
- (int) len, copyfrom, elt);
- }
- if (*copyfrom != ')')
- die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
- copyfrom++;
+ copyfrom = parse_long_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
} else {
/* shorthand */
copyfrom = parse_short_magic(&element_magic, elt);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 16/17] pathspec: small readability changes
From: Brandon Williams @ 2016-12-06 21:51 UTC (permalink / raw)
To: git; +Cc: sbeller, pclouds, gitster, Brandon Williams
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, etc.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 41aa213..8a07b02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
+ /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
- match = prefix_path_gently(prefix, prefixlen, &prefixlen, copyfrom);
+ match = prefix_path_gently(prefix, prefixlen,
+ &prefixlen, copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+ item->len = strlen(item->match);
+ item->prefix = prefixlen;
+
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
} else {
item->original = xstrdup(elt);
}
- item->len = strlen(item->match);
- item->prefix = prefixlen;
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
- if (magic & PATHSPEC_LITERAL)
+ if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
- else {
+ } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox