* Re: [PATCH v4 0/5] road to reentrant real_path
From: Torsten Bögershausen @ 2017-01-04 6:56 UTC (permalink / raw)
To: Jeff King, Brandon Williams
Cc: git, sbeller, jacob.keller, gitster, ramsay, j6t, pclouds,
larsxschneider
In-Reply-To: <20170104004825.3s27dsircdp5lqte@sigill.intra.peff.net>
On 04.01.17 01:48, Jeff King wrote:
> On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
>
>> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
>
> Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> what all other similar functions will be using.
>
> The only problem was that we were redefining the macro. So maybe:
>
> #ifndef MAXSYMLINKS
> #define MAXSYMLINKS 5
> #endif
>
> would be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32
Would the same value value for all Git installations on all platforms make sense?
#define GITMAXSYMLINKS 20
>
> It looks like the "usual" value is more like 20 or 30 on most systems,
> though. We should probably also set errno to ELOOP when we hit the
> limit, which is what other symlink-resolving functions would do.
>
> I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too.
> We should be picking it up from <sys/param.h>.
>
> -Peff
>
^ permalink raw reply
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Jeff King @ 2017-01-04 7:01 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Brandon Williams, git, sbeller, jacob.keller, gitster, ramsay,
j6t, pclouds, larsxschneider
In-Reply-To: <3f9a530c-402f-f276-4721-fa6a8a6fef41@web.de>
On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:
> On 04.01.17 01:48, Jeff King wrote:
> > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> >
> >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
> >
> > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> > what all other similar functions will be using.
> >
> > The only problem was that we were redefining the macro. So maybe:
> >
> > #ifndef MAXSYMLINKS
> > #define MAXSYMLINKS 5
> > #endif
> >
> > would be a good solution?
> Why 5 ? (looking at the 20..30 below)
> And why 5 on one system and e.g. on my Mac OS
> #define MAXSYMLINKS 32
I mentioned "5" because that is the current value of MAXDEPTH. I do
think it would be reasonable to bump it to something higher.
> Would the same value value for all Git installations on all platforms make sense?
> #define GITMAXSYMLINKS 20
I think it's probably more important to match the rest of the OS, so
that open("foo") and realpath("foo") behave similarly on the same
system. Though I think even that isn't always possible, as the limit is
dynamic on some systems.
I think the idea of the 20-30 range is that it's small enough to catch
an infinite loop quickly, and large enough that nobody will ever hit it
in practice. :)
-Peff
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Jeff King @ 2017-01-04 7:05 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <cover.1483354746.git.git@drmicha.warpmail.net>
On Mon, Jan 02, 2017 at 12:14:49PM +0100, Michael J Gruber wrote:
> Currently, the headers "error: ", "warning: " etc. - generated by die(),
> warning() etc. - are not localized, but we feed many localized error messages
> into these functions so that we produce error messages with mixed localisation.
>
> This series introduces variants of die() etc. that use localised variants of
> the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
> instead of die(_("not workee")), which would produce a mixed localisation (such
> as "error: geht ned"), one should use die_(_("not workee")) (resulting in
> "Fehler: geht ned").
I can't say I'm excited about having matching "_" variants for each
function. Are we sure that they are necessary? I.e., would it be
acceptable to just translate them always?
> 1/5 prepares the error machinery
> 2/5 provides new variants error_() etc.
> 3/5 has coccinelli rules error(_(E)) -> error_(_(E)) etc.
> 4/5 applies the coccinelli patches
>
> 5/5 is not to be applied to the main tree, but helps you try out the feature:
> it has changes to de.po and git.pot so that e.g. "git branch" has fully localised
> error messages (see the recipe in the commit message).
Your patches 4 and 5 don't seem to have made it to the list. Judging
from the diffstat, I'd guess they broke the 100K limit.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Jeff King @ 2017-01-04 7:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill
In-Reply-To: <20170104014835.22377-3-sbeller@google.com>
On Tue, Jan 03, 2017 at 05:48:35PM -0800, Stefan Beller wrote:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
>
> This patch accomplishes two things:
>
> 1. Switch assert() to die("BUG") to give a more readable message.
>
> 2. Take one of the cases where we hit a BUG and turn it into a normal
> "there was something wrong with the input" message.
As this last bit is quoted from me, I won't deny that it's brilliant as
usual.
But as this commit message needs to stand on its own, rather than as part of a
larger discussion thread, it might be worth expanding "one of the cases"
here. And talking about what's happening to the other cases.
Like:
This assertion triggered for cases where there wasn't a programming
bug, but just bogus input. In particular, if the user asks for a
pathspec that is inside a submodule, we shouldn't assert() or
die("BUG"); we should tell the user their request is bogus.
We'll retain the assertion for non-submodule cases, though. We don't
know of any cases that would trigger this, but it _would_ be
indicative of a programming error, and we should catch it here.
or something. Writing the first paragraph made me wonder if a better
solution, though, would be to catch and complain about this case
earlier. IOW, this _is_ a programming bug, because we're violating some
assumption of the pathspec code. And whatever is putting that item into
the pathspec list is what should be fixed.
I haven't looked closely enough to have a real opinion on that, though.
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..574a0bb158 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,28 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> }
>
> /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> - item->prefix <= item->len);
> + if (item->nowildcard_len > item->len ||
> + item->prefix > item->len) {
> + /* Historically this always was a submodule issue */
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> + int len;
Given the discussion, this comment seems funny now. Who cares about
"historically"? It should probably be something like:
/*
* This case can be triggered by the user pointing us to a pathspec
* inside a submodule, which is an input error. Detect that here
* and complain, but fallback in the non-submodule case to a BUG,
* as we have no idea what would trigger that.
*/
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Jeff King @ 2017-01-04 8:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, 마누엘, Junio C Hamano
In-Reply-To: <3c160f81a88cf8697f2459bb7f2a3e27fb3e469c.1483373021.git.johannes.schindelin@gmx.de>
On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de>
>
> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
> to build it as an `article`. This seems to be a problem when building
> the documentation with `asciidoctor`. Furthermore the parts *Git
> Glossary* and *Appendix B* had no subsections which is not allowed when
> building with `asciidoctor`. So lets add a *dummy* section.
The git-scm.com site uses asciidoctor, too, and I think I have seen some
oddness with the rendering though. So in general I am in favor of making
things work under both asciidoc and asciidoctor.
I diffed the results of "make user-manual.html" before and after this
patch. A lot of "h3" chapter titles get bumped to "h2", which is OK. The
chapter titles now say "Chapter 1. Repositories and Branches" rather
than just "Repositories and Branches". Likewise, references now say
Chapter 1, _Repositories and Branches_
rather than:
the section called "Repositories and Branches".
which is probably OK, though the whole thing is short enough that
calling the sections chapters feels a bit over-important. Chapters now
get their own table of contents, too. This is especially silly in
one-section chapters like "Git Glossary".
So I dunno. I really do think "article" is conceptually the most
appropriate style, but I agree that there are some book-like things
(like appendices).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] giteveryday: unbreak rendering with AsciiDoctor
From: Jeff King @ 2017-01-04 8:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <c3b21bbec96408c4d6698129fd336c24c9e2f0f0.1483373021.git.johannes.schindelin@gmx.de>
On Mon, Jan 02, 2017 at 05:04:05PM +0100, Johannes Schindelin wrote:
> The "giteveryday" document has a callout list that contains a code
> block. This is not a problem for AsciiDoc, but AsciiDoctor sadly was
> explicitly designed *not* to render this correctly [*1*]. The symptom is
> an unhelpful
>
> line 322: callout list item index: expected 1 got 12
> line 325: no callouts refer to list item 1
> line 325: callout list item index: expected 2 got 13
> line 327: no callouts refer to list item 2
>
> In Git for Windows, we rely on the speed improvement of AsciiDoctor (on
> this developer's machine, `make -j15 html` takes roughly 30 seconds with
> AsciiDoctor, 70 seconds with AsciiDoc), therefore we need a way to
> render this correctly.
>
> The easiest way out is to simplify the callout list, as suggested by
> AsciiDoctor's author, even while one may very well disagree with him
> that a code block hath no place in a callout list.
This looks like a good re-write to avoid the issue while maintaining the
meaning and flow of the original.
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/2] t9813: avoid using pipes
From: Luke Diamand @ 2017-01-04 9:11 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git Users, Stefan Beller, Johannes Sixt
In-Reply-To: <20170103195708.15157-2-pranit.bauva@gmail.com>
On 3 January 2017 at 19:57, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
Do we also need to fix t9814-git-p4-rename.sh ?
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> t/t9813-git-p4-preserve-users.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
> index 798bf2b67..2133b21ae 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
> make_change_by_user usernamefile3 Derek derek@example.com &&
> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
> export P4EDITOR P4USER P4PASSWD &&
> - git p4 commit |\
> - grep "git author derek@example.com does not match" &&
> + git p4 commit >actual &&
> + grep "git author derek@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 Charlie charlie@example.com &&
> - git p4 commit |\
> - grep "git author charlie@example.com does not match" &&
> + git p4 commit >actual &&
> + grep "git author charlie@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 alice alice@example.com &&
> git p4 commit >actual 2>&1 &&
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands
From: Luke Diamand @ 2017-01-04 9:09 UTC (permalink / raw)
To: Ori Rawlings; +Cc: Igor Kushnir, Git Users, Lars Schneider
In-Reply-To: <CAPv0x+PoQ+3ERAc_0gviYP5j1-Zg=X+B1OSC6vDKatqUhFtAag@mail.gmail.com>
On 3 January 2017 at 20:02, Ori Rawlings <orirawlings@gmail.com> wrote:
> Looks good to me.
And me.
>
>
> Ori Rawlings
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Johannes Schindelin @ 2017-01-03 21:33 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, David Aguilar,
Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <CAGZ79kZ--jp08pK+xwn1N2VQQr8bA5+DveE2HsoY90R1gR6c_A@mail.gmail.com>
Hi Stefan,
On Tue, 3 Jan 2017, Stefan Beller wrote:
> On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Technically, it is correct that git_exec_path() returns a possibly
> > malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> > let's just use a static variable to make it a singleton. That'll shut
> > Coverity up, hopefully.
>
> I picked up this patch and applied it to the coverity branch
> that I maintain at github/stefanbeller/git.
>
> I'd love to see this patch upstream as it reduces my maintenance
> burden of the coverity branch by a patch.
>
> Early on when Git was new to coverity, some arguments were made
> that patches like these only clutter the main code base which is read
> by a lot of people, hence we want these quirks for coverity not upstream.
> And I think that still holds.
>
> If this patch is only to appease coverity (as the commit message eludes
> to) I think this may be a bad idea for upstream. If this patch fixes an
> actual problem, then the commit message needs to spell that out.
This patch was originally only to appease Coverity, but it actually *does*
plug a very real memory leak: previously, *every* call to git_exec_path()
*possibly* returned a newly-malloc()ed buffer. Now, the first call will
store that pointer in a static variable and reuse it later.
Could you maybe help me with improving the commit message?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 2/2] t9813: avoid using pipes
From: Pranit Bauva @ 2017-01-04 11:49 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Stefan Beller, Johannes Sixt
In-Reply-To: <CAE5ih78vLwDubesnAxD=g3TzsbN0sQZae3McdFcwDAZfYYhXSg@mail.gmail.com>
Hey Luke,
On Wed, Jan 4, 2017 at 2:41 PM, Luke Diamand <luke@diamand.org> wrote:
> On 3 January 2017 at 19:57, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> The exit code of the upstream in a pipe is ignored thus we should avoid
>> using it. By writing out the output of the git command to a file, we can
>> test the exit codes of both the commands.
>
> Do we also need to fix t9814-git-p4-rename.sh ?
I don't think so. As Johannes[1] and Stefan[2] pointed out, we should
avoid upstream pipes for git. p4 can be treated as an "external
command" just like grep/sed.
[1]: http://public-inbox.org/git/285ed013-5c59-0b98-7dc0-8f729587a313@kdbg.org/
[2]: http://public-inbox.org/git/CAGZ79kZRFLzD7wcAnFvke9vBxxTAgE7=Ud7F_O95EfkWqz=LJw@mail.gmail.com/
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Duy Nguyen @ 2017-01-04 13:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: Michael J Gruber, git@vger.kernel.org
In-Reply-To: <CAGZ79ka-FXfFN8ZqE6+v06o3nKa7ad0iWttn99Y2wf5m6wfs8A@mail.gmail.com>
On Wed, Jan 4, 2017 at 2:45 AM, Stefan Beller <sbeller@google.com> wrote:
>> In this implementation, the gettext call for the header and the body are done
>> in different places (error function vs. caller) but this call pattern seems to
>> be the easiest variant for the caller, because the message body has to be marked
>> for localisation in any case, and N_() requires more letters than _(), an extra
>> argument to die() etc. even more than the extra "_" in the function name.
>
> I see. We have to markup the strings to be translatable such that the .po files
> are complete. It would be really handy if there was a way to say "anything that
> is fed to this function (die_) needs to be marked for translation.
>
> Looking through
> https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
> such a thing doesn't seem to exist.
I think --keyword is exactly for that purpose: marking more text for
translations besides standard markers like _() or N_(). Yes we need to
call gettext() explicitly in die_() later on. We already do that for
parse-options. We just need to N_() the strings, without actually
spelling it out.
>
> So in that case die_(_(...)) seems to be the easiest way forward.
I still prefer changing the die_routine though since die() in many
cases could be used in both plumbing and porcelain contexts. And we
have tried to keep plumbing output (and behavior) as stable as
possible. The approach has some similarity to unpack_trees() which
shares the same porcelain/plumbing problem.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/2] dir.c: add retry logic to relocate_gitdir
From: Duy Nguyen @ 2017-01-04 13:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kZoiFfi9+-yqHZmF3Ofp3X-CV6s0qg1bm2CRpN-ETbDLw@mail.gmail.com>
On Tue, Dec 20, 2016 at 6:19 AM, Stefan Beller <sbeller@google.com> wrote:
>> On 12/19, Stefan Beller wrote:
> This code is heavily inspired by refs/files-backend.c which upon
> closer inspection only retries directory things within the git directory
> (which is assumed to be accessed in parallel by different invocations
> of Git)
I take inspiration from lock/temp files on the other hand. Could we
keep some sort of "undo journal" as we move along and clear it when
the "transaction" is completed? The good thing about lock files is,
even if you die() or SIGTERM'd, rolling back can still take place (but
it has to be something very simple, like removing or renaming because
you don't want to do big things in a signal handler).
If things turn out to be complicated and risky to be executed in an
unknown context, we could still print a helpful message like "yeah
you're in trouble, maybe try this and this, or consult git mailing
list. We apologise for the inconvenience,"
--
Duy
^ permalink raw reply
* Re: [PATCH v4 00/16] pathspec cleanup
From: Duy Nguyen @ 2017-01-04 13:56 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20170103184241.128409-1-bmwill@google.com>
On Wed, Jan 4, 2017 at 1:42 AM, Brandon Williams <bmwill@google.com> wrote:
> diff --git a/dir.c b/dir.c
> index 15f7c9993..e8ddd7f8a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1353,6 +1353,15 @@ static int simplify_away(const char *path, int pathlen,
> {
> int i;
>
> + if (pathspec)
> + guard_pathspec(pathspec,
> + pathspec_fromtop |
> + pathspec_maxdepth |
> + pathspec_literal |
> + pathspec_glob |
> + pathspec_icase |
> + pathspec_exclude);
You have done some magic (or your MTA/editor did) to lower case
GUARD_PATHSPEC and all the flags. The real patch looks good though, so
no problem.
> +
> if (!pathspec || !pathspec->nr)
> return 0;
Super tiny nit, if GUARD_PATHSPEC is placed after this line, then we
don't have to check if pathspec is non-NULL. Probably not worth a
re-roll unless somebody else finds something else.
> if (m->mnemonic)
> - strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
> + strbuf_addf(&sb, "'%s' (mnemonic: '%c')",
> + m->name, m->mnemonic);
.. and that somebody might be me :) we need to mark "mnemonic" for
translation. Putting _() around the string would do.
Ideally I would give the translators the whole sentence so they can
have good context (and good translation as a result). But since we
potentially concatenate multiple unsupported magic in the string,
there's no way to provide one (or a few) fixed string(s) at compile
time. So let's just _() it and leave it at that.
--
Duy
^ permalink raw reply
* Re: Preserve/Prune Old Pack Files
From: Martin Fick @ 2017-01-04 16:11 UTC (permalink / raw)
To: repo-discuss; +Cc: jmelvin, jgit-dev, git
In-Reply-To: <24abd0ed58c25ce832014f9bd5bb2090@codeaurora.org>
I am replying to this email across lists because I wanted to
highlight to the git community this jgit change to repacking
that we have up for review
https://git.eclipse.org/r/#/c/87969/
This change introduces a new convention for how to preserve
old pack files in a staging area
(.git/objects/packs/preserved) before deleting them. I
wanted to ensure that the new proposed convention would be
done in a way that would be satisfactory to the git
community as a whole so that it would be more easy to
provide the same behavior in git eventually. The preserved
pack files (and accompanying index and bitmap files), are not
only moved, but they are also renamed so that they no longer
will match recursive finds looking for pack files.
I look forward to any review (it need not happen on the
change, replies to this email would be fine also), in
particular with respect to the approach and naming
conventions.
Thanks,
-Martin
On Tuesday, January 03, 2017 02:46:12 PM
jmelvin@codeaurora.org wrote:
> We’ve noticed cases where Stale File Handle Exceptions
> occur during git operations, which can happen on users of
> NFS repos when repacking is done on them.
>
> To address this issue, we’ve added two new options to the
> JGit GC command:
>
> --preserve-oldpacks: moves old pack files into the
> preserved subdirectory instead of deleting them after
> repacking
>
> --prune-preserved: prunes old pack files from the
> preserved subdirectory after repacking, but before
> potentially moving the latest old pack files to this
> subdirectory
>
> The strategy is to preserve old pack files around until
> the next repack with the hopes that they will become
> unreferenced by then and not cause any exceptions to
> running processes when they are finally deleted (pruned).
>
> Change is uploaded for review here:
> https://git.eclipse.org/r/#/c/87969/
>
> Thanks,
> James
--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v4 00/16] pathspec cleanup
From: Brandon Williams @ 2017-01-04 17:53 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8C=SC+gE1HQeGdE5z3+S8RHwiM-4ZXBzuwvN=+COUGNzg@mail.gmail.com>
On 01/04, Duy Nguyen wrote:
> On Wed, Jan 4, 2017 at 1:42 AM, Brandon Williams <bmwill@google.com> wrote:
> > diff --git a/dir.c b/dir.c
> > index 15f7c9993..e8ddd7f8a 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -1353,6 +1353,15 @@ static int simplify_away(const char *path, int pathlen,
> > {
> > int i;
> >
> > + if (pathspec)
> > + guard_pathspec(pathspec,
> > + pathspec_fromtop |
> > + pathspec_maxdepth |
> > + pathspec_literal |
> > + pathspec_glob |
> > + pathspec_icase |
> > + pathspec_exclude);
>
> You have done some magic (or your MTA/editor did) to lower case
> GUARD_PATHSPEC and all the flags. The real patch looks good though, so
> no problem.
That's really odd, I was sure I just did a cut and paste. I'll fix that
:)
>
> > +
> > if (!pathspec || !pathspec->nr)
> > return 0;
>
>
> Super tiny nit, if GUARD_PATHSPEC is placed after this line, then we
> don't have to check if pathspec is non-NULL. Probably not worth a
> re-roll unless somebody else finds something else.
I saw this after I sent out the series again, and I agree. I'll move
the guard to be after this check.
>
> > if (m->mnemonic)
> > - strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
> > + strbuf_addf(&sb, "'%s' (mnemonic: '%c')",
> > + m->name, m->mnemonic);
>
> .. and that somebody might be me :) we need to mark "mnemonic" for
> translation. Putting _() around the string would do.
>
> Ideally I would give the translators the whole sentence so they can
> have good context (and good translation as a result). But since we
> potentially concatenate multiple unsupported magic in the string,
> there's no way to provide one (or a few) fixed string(s) at compile
> time. So let's just _() it and leave it at that.
Will do!
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 00/16] pathspec cleanup
From: Brandon Williams @ 2017-01-04 17:56 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20170104175347.GA69227@google.com>
On 01/04, Brandon Williams wrote:
> On 01/04, Duy Nguyen wrote:
> > On Wed, Jan 4, 2017 at 1:42 AM, Brandon Williams <bmwill@google.com> wrote:
> > > diff --git a/dir.c b/dir.c
> > > index 15f7c9993..e8ddd7f8a 100644
> > > --- a/dir.c
> > > +++ b/dir.c
> > > @@ -1353,6 +1353,15 @@ static int simplify_away(const char *path, int pathlen,
> > > {
> > > int i;
> > >
> > > + if (pathspec)
> > > + guard_pathspec(pathspec,
> > > + pathspec_fromtop |
> > > + pathspec_maxdepth |
> > > + pathspec_literal |
> > > + pathspec_glob |
> > > + pathspec_icase |
> > > + pathspec_exclude);
> >
> > You have done some magic (or your MTA/editor did) to lower case
> > GUARD_PATHSPEC and all the flags. The real patch looks good though, so
> > no problem.
>
> That's really odd, I was sure I just did a cut and paste. I'll fix that
> :)
So it looks like what ever I did to insert this into the cover letter
made everything lower case. The actual patch itself looks fine.
--
Brandon Williams
^ permalink raw reply
* [PATCH v5 15/16] pathspec: small readability changes
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-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, and
adding additional comments.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index f3a7a1d33..e53530e7a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -67,11 +67,11 @@ static struct pathspec_magic {
char mnemonic; /* this cannot be ':'! */
const char *name;
} pathspec_magic[] = {
- { PATHSPEC_FROMTOP, '/', "top" },
- { PATHSPEC_LITERAL, 0, "literal" },
- { PATHSPEC_GLOB, '\0', "glob" },
- { PATHSPEC_ICASE, '\0', "icase" },
- { PATHSPEC_EXCLUDE, '!', "exclude" },
+ { PATHSPEC_FROMTOP, '/', "top" },
+ { PATHSPEC_LITERAL, '\0', "literal" },
+ { PATHSPEC_GLOB, '\0', "glob" },
+ { PATHSPEC_ICASE, '\0', "icase" },
+ { PATHSPEC_EXCLUDE, '!', "exclude" },
};
static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
@@ -336,6 +336,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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;
@@ -343,11 +344,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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.
@@ -364,8 +370,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
} 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);
@@ -373,13 +377,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 13/16] pathspec: create parse_element_magic helper
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-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 | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index f6356bde1..00fcae4e1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -245,6 +245,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,26 +280,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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) {
magic = PATHSPEC_LITERAL;
- else
+ } else {
+ 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.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 12/16] pathspec: create parse_long_magic function
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-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 1b0901848..f6356bde1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,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, unsigned flags,
; /* 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.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 10/16] pathspec: factor global magic into its own function
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-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 | 127 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 78 insertions(+), 49 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index af7f2d01d..77df55da6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -87,6 +87,75 @@ 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 = -1;
+
+ if (literal < 0)
+ literal = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
+
+ return literal;
+}
+
+static inline int get_glob_global(void)
+{
+ static int glob = -1;
+
+ if (glob < 0)
+ glob = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return glob;
+}
+
+static inline int get_noglob_global(void)
+{
+ static int noglob = -1;
+
+ if (noglob < 0)
+ noglob = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return noglob;
+}
+
+static inline int get_icase_global(void)
+{
+ static int icase = -1;
+
+ if (icase < 0)
+ icase = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+
+ return icase;
+}
+
+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.
@@ -104,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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] == '(') {
@@ -207,15 +242,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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 = PATHSPEC_LITERAL;
+ else
+ magic |= get_global_magic(element_magic);
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
@@ -241,7 +272,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
* 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 */
@@ -408,9 +439,7 @@ void parse_pathspec(struct pathspec *pathspec,
item[i].magic = prefix_pathspec(item + i, flags,
prefix, prefixlen, entry);
- if ((flags & PATHSPEC_LITERAL_PATH) &&
- !(magic_mask & PATHSPEC_LITERAL))
- item[i].magic |= PATHSPEC_LITERAL;
+
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 08/16] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index b9a3819d6..5df364bc6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -101,9 +101,7 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
* 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 *p_short_magic,
- unsigned flags,
+static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -210,7 +208,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
magic |= short_magic;
- *p_short_magic = short_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -329,8 +326,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
}
static void NORETURN unsupported_magic(const char *pattern,
- unsigned magic,
- unsigned short_magic)
+ unsigned magic)
{
struct strbuf sb = STRBUF_INIT;
int i;
@@ -339,9 +335,11 @@ static void NORETURN unsupported_magic(const char *pattern,
if (!(magic & m->bit))
continue;
if (sb.len)
- strbuf_addch(&sb, ' ');
- if (short_magic & m->bit)
- strbuf_addf(&sb, "'%c'", m->mnemonic);
+ strbuf_addstr(&sb, ", ");
+
+ if (m->mnemonic)
+ strbuf_addf(&sb, _("'%s' (mnemonic: '%c')"),
+ m->name, m->mnemonic);
else
strbuf_addf(&sb, "'%s'", m->name);
}
@@ -413,11 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
- unsigned short_magic;
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, &short_magic,
- flags,
+ item[i].magic = prefix_pathspec(item + i, flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -425,9 +421,7 @@ void parse_pathspec(struct pathspec *pathspec,
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
- unsupported_magic(entry,
- item[i].magic & magic_mask,
- short_magic);
+ unsupported_magic(entry, item[i].magic & magic_mask);
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 09/16] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 5df364bc6..af7f2d01d 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -74,13 +74,12 @@ static struct pathspec_magic {
{ PATHSPEC_EXCLUDE, '!', "exclude" },
};
-static void prefix_short_magic(struct strbuf *sb, int prefixlen,
- unsigned short_magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
{
int i;
strbuf_addstr(sb, ":(");
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (short_magic & pathspec_magic[i].bit) {
+ if (magic & pathspec_magic[i].bit) {
if (sb->buf[sb->len - 1] != '(')
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name);
@@ -109,8 +108,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
static int glob_global = -1;
static int noglob_global = -1;
static int icase_global = -1;
- unsigned magic = 0, short_magic = 0, global_magic = 0;
- const char *copyfrom = elt, *long_magic_end = NULL;
+ unsigned magic = 0, element_magic = 0, global_magic = 0;
+ const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
@@ -164,7 +163,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom, len)) {
- magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (starts_with(copyfrom, "prefix:")) {
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
}
if (*copyfrom != ')')
die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
- long_magic_end = copyfrom;
copyfrom++;
} else {
/* shorthand */
@@ -196,7 +194,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
break;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
if (pathspec_magic[i].mnemonic == ch) {
- short_magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (ARRAY_SIZE(pathspec_magic) <= i)
@@ -207,7 +205,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
copyfrom++;
}
- magic |= short_magic;
+ magic |= element_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
*/
- if (flags & PATHSPEC_PREFIX_ORIGIN) {
+ if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
+ prefixlen && !literal_global) {
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);
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
} else {
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 16/16] pathspec: rename prefix_pathspec to init_pathspec_item
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index e53530e7a..ff2509ddd 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -297,21 +297,11 @@ 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.
+ * Perform the initialization of a pathspec_item based on a pathspec element.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
- const char *prefix, int prefixlen,
- const char *elt)
+static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+ const char *prefix, int prefixlen,
+ const char *elt)
{
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
magic |= get_global_magic(element_magic);
}
+ item->magic = magic;
+
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
die("BUG: 'prefix' magic is supposed to be used at worktree's root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
assert(item->nowildcard_len <= item->len &&
item->prefix <= item->len);
- return magic;
}
static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -501,8 +492,7 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, flags,
- prefix, prefixlen, entry);
+ init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 14/16] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-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 00fcae4e1..f3a7a1d33 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,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)
+{
+ if (item->len >= 1 && item->match[item->len - 1] == '/') {
+ int i = cache_name_pos(item->match, item->len - 1);
+
+ if (i >= 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 flags,
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) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
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.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v5 07/16] pathspec: remove unused variable from unsupported_magic
From: Brandon Williams @ 2017-01-04 18:04 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
Removed unused variable 'n' from the 'unsupported_magic()' function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index b8faa8f46..b9a3819d6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -333,8 +333,8 @@ static void NORETURN unsupported_magic(const char *pattern,
unsigned short_magic)
{
struct strbuf sb = STRBUF_INIT;
- int i, n;
- for (n = i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
const struct pathspec_magic *m = pathspec_magic + i;
if (!(magic & m->bit))
continue;
@@ -344,7 +344,6 @@ static void NORETURN unsupported_magic(const char *pattern,
strbuf_addf(&sb, "'%c'", m->mnemonic);
else
strbuf_addf(&sb, "'%s'", m->name);
- n++;
}
/*
* We may want to substitute "this command" with a command
--
2.11.0.390.gc69c2f50cf-goog
^ 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