* Re: [PATCH v3 06/16] pathspec: copy and free owned memory
From: Duy Nguyen @ 2017-01-03 12:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <1481670870-66754-7-git-send-email-bmwill@google.com>
On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams <bmwill@google.com> wrote:
> 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;
We should set pathspec->nr to zero so that calling this function again
won't cause any harm.
> }
--
Duy
^ permalink raw reply
* Re: [PATCH v3 08/16] pathspec: always show mnemonic and name in unsupported_magic
From: Duy Nguyen @ 2017-01-03 12:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <1481670870-66754-9-git-send-email-bmwill@google.com>
On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams <bmwill@google.com> wrote:
> @@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char *pattern,
> continue;
> if (sb.len)
> strbuf_addch(&sb, ' ');
> - if (short_magic & m->bit)
> - strbuf_addf(&sb, "'%c'", m->mnemonic);
> +
> + if (m->mnemonic)
> + strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
> else
> strbuf_addf(&sb, "'%s'", m->name);
> }
The die() call is out of diff context, but it'll print
pathspec magic not supported by this command: (!)top
which looks too much like :(<name>)<mnemonic> pathspec syntax too me
and threw me off a bit. And it's a bit cryptic, isn't it? Since this
is meant for human, maybe we can just write
pathspec magic not supported by this command: top (mnemonic: '!')
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)
From: Duy Nguyen @ 2017-01-03 12:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8CmKBpWa=yi44vGUe56CmeT-Swh_M_XxMeA+xkLLUhQ3Q@mail.gmail.com>
On Thu, Dec 29, 2016 at 5:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams <bmwill@google.com> wrote:
>> On 12/27, Junio C Hamano wrote:
>>> * bw/pathspec-cleanup (2016-12-14) 16 commits
>>> - pathspec: rename prefix_pathspec to init_pathspec_item
>>> - pathspec: small readability changes
>>> - pathspec: create strip submodule slash helpers
>>> - pathspec: create parse_element_magic helper
>>> - pathspec: create parse_long_magic function
>>> - pathspec: create parse_short_magic function
>>> - pathspec: factor global magic into its own function
>>> - pathspec: simpler logic to prefix original pathspec elements
>>> - pathspec: always show mnemonic and name in unsupported_magic
>>> - pathspec: remove unused variable from unsupported_magic
>>> - pathspec: copy and free owned memory
>>> - pathspec: remove the deprecated get_pathspec function
>>> - ls-tree: convert show_recursive to use the pathspec struct interface
>>> - dir: convert fill_directory to use the pathspec struct interface
>>> - dir: remove struct path_simplify
>>> - mv: remove use of deprecated 'get_pathspec()'
>>>
>>> Code clean-up in the pathspec API.
>>>
>>> Waiting for the (hopefully) final round of review before 'next'.
>>
>> What more needs to be reviewed for this series?
>
> I wanted to have a look at it but I successfully managed to put if off
> so far. Will do soonish.
I have just sent a couple of minor comments. The rest looks good!
--
Duy
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Duy Nguyen @ 2017-01-03 12:26 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List
In-Reply-To: <cover.1483354746.git.git@drmicha.warpmail.net>
On Mon, Jan 2, 2017 at 6:14 PM, Michael J Gruber
<git@drmicha.warpmail.net> 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").
Another option, not as thorough, but less effort, is changing
die/err/warn default routines to the "porcelain" versions where we do
_("fatal:") internally _with_ die(), not die_(). We can set this for
porcelain commands that we know can be fully i18n-ized. Then maybe
die_() will fill in the gap if there's still need for it.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex
From: Junio C Hamano @ 2017-01-03 12:58 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD1TwVvsvuffyHuzse_9afbNvSEJtyQyWzn6Rc4KwJNwHQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> Ok, I will add a patch to update the style of the existing tests at
> the beginning of the series and then use the same new style in the
> tests I add in later patches.
That's not what I meant---I was expecting and was willing to accept
a corrected patch that leaves existing old-fashioned ones as they
are while making sure that added ones are modern, to reduce the cost
of finishing this series, leaving the style fixes of existing ones
for future clean-up task that can be done by anybody after the dust
from this series settles.
A preparatory clean-up patch before the series like you plan is fine
(eh, rather, "extra nice"), so... thanks.
^ permalink raw reply
* [PATCH 1/2] stash: fix USAGE text
From: Marc Strapetz @ 2017-01-03 15:53 UTC (permalink / raw)
To: git; +Cc: Marc Strapetz
Add missing usage description for stash subcommands
'create' and 'store'.
Signed-off-by: Marc Strapetz <marc.strapetz@syntevo.com>
---
git-stash.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1a..c6b9db694 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,7 +9,9 @@ USAGE="list [<options>]
or: $dashless branch <branchname> [<stash>]
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
- or: $dashless clear"
+ or: $dashless clear
+ or: $dashless create [<message>]
+ or: $dashless store [-m|--message <message>] [-q|--quiet] <commit>"
SUBDIRECTORY_OK=Yes
OPTIONS_SPEC=
--
2.11.0
^ permalink raw reply related
* [PATCH 2/2] stash: --[no-]include-untracked option for create
From: Marc Strapetz @ 2017-01-03 15:53 UTC (permalink / raw)
To: git; +Cc: Marc Strapetz
In-Reply-To: <20170103155356.11213-1-marc.strapetz@syntevo.com>
Expose internal option to include untracked files
for the stash 'create' subcommand.
Signed-off-by: Marc Strapetz <marc.strapetz@syntevo.com>
---
Documentation/git-stash.txt | 2 +-
git-stash.sh | 14 ++++++++++++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e..cc7944e59 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -16,7 +16,7 @@ SYNOPSIS
'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
'git stash' clear
-'git stash' create [<message>]
+'git stash' create [-u|--[no-]include-untracked] [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index c6b9db694..16f5fe93e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -10,7 +10,7 @@ USAGE="list [<options>]
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
or: $dashless clear
- or: $dashless create [<message>]
+ or: $dashless create [-u|--[no-]include-untracked] [<message>]
or: $dashless store [-m|--message <message>] [-q|--quiet] <commit>"
SUBDIRECTORY_OK=Yes
@@ -629,7 +629,17 @@ clear)
;;
create)
shift
- create_stash "$*" && echo "$w_commit"
+ case "$1" in
+ -u|--include-untracked)
+ untracked=untracked
+ shift
+ ;;
+ --no-include-untracked)
+ untracked=
+ shift
+ ;;
+ esac
+ create_stash "$*" "$untracked" && echo "$w_commit"
;;
store)
shift
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] archive-zip: load userdiff config
From: René Scharfe @ 2017-01-03 17:24 UTC (permalink / raw)
To: Jeff King, git
In-Reply-To: <20170102222509.ho7motscnffrtnfh@sigill.intra.peff.net>
Am 02.01.2017 um 23:25 schrieb Jeff King:
> Since 4aff646d17 (archive-zip: mark text files in archives,
> 2015-03-05), the zip archiver will look at the userdiff
> driver to decide whether a file is text or binary. This
> usually doesn't need to look any further than the attributes
> themselves (e.g., "-diff", etc). But if the user defines a
> custom driver like "diff=foo", we need to look at
> "diff.foo.binary" in the config. Prior to this patch, we
> didn't actually load it.
Ah, didn't think of that, obviously.
Would it make sense for userdiff_find_by_path() to die if
userdiff_config() hasn't been called, yet?
> I also happened to notice that zipfiles are created using the local
> timezone (because they have no notion of the timezone, so we have to
> pick _something_). That's probably the least-terrible option, but it was
> certainly surprising to me when I tried to bit-for-bit reproduce a
> zipfile from GitHub on my local machine.
That reminds me of an old request to allow users better control over the
meta-data written into archives. Being able to specify a time zone
offset could be a start.
> archive-zip.c | 7 +++++++
> t/t5003-archive-zip.sh | 22 ++++++++++++++++++----
> 2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/archive-zip.c b/archive-zip.c
> index 9db47357b0..b429a8d974 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
> *dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
> }
>
> +static int archive_zip_config(const char *var, const char *value, void *data)
> +{
> + return userdiff_config(var, value);
> +}
> +
> static int write_zip_archive(const struct archiver *ar,
> struct archiver_args *args)
> {
> int err;
>
> + git_config(archive_zip_config, NULL);
> +
I briefly thought about moving this call to archive.c with the rest of
the config-related stuff, but I agree it's better kept here.
> dos_time(&args->time, &zip_date, &zip_time);
>
> zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 14744b2a4b..55c7870997 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -64,6 +64,12 @@ check_zip() {
> test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
> test_cmp_bin $original/nodiff.lf $extracted/nodiff.lf
> "
> +
> + test_expect_success UNZIP " validate that custom diff is unchanged " "
> + test_cmp_bin $original/custom.cr $extracted/custom.cr &&
> + test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
> + test_cmp_bin $original/custom.lf $extracted/custom.lf
> + "
> }
>
> test_expect_success \
> @@ -78,6 +84,9 @@ test_expect_success \
> printf "text\r" >a/nodiff.cr &&
> printf "text\r\n" >a/nodiff.crlf &&
> printf "text\n" >a/nodiff.lf &&
> + printf "text\r" >a/custom.cr &&
> + printf "text\r\n" >a/custom.crlf &&
> + printf "text\n" >a/custom.lf &&
> printf "\0\r" >a/binary.cr &&
> printf "\0\r\n" >a/binary.crlf &&
> printf "\0\n" >a/binary.lf &&
> @@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
> test_expect_success 'setup export-subst and diff attributes' '
> echo "a/nodiff.* -diff" >>.git/info/attributes &&
> echo "a/diff.* diff" >>.git/info/attributes &&
> + echo "a/custom.* diff=custom" >>.git/info/attributes &&
> + git config diff.custom.binary true &&
> echo "substfile?" export-subst >>.git/info/attributes &&
> git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
> >a/substfile1
> '
>
> -test_expect_success \
> - 'create bare clone' \
> - 'git clone --bare . bare.git &&
> - cp .git/info/attributes bare.git/info/attributes'
> +test_expect_success 'create bare clone' '
> + git clone --bare . bare.git &&
> + cp .git/info/attributes bare.git/info/attributes &&
> + # Recreate our changes to .git/config rather than just copying it, as
> + # we do not want to clobber core.bare or other settings.
> + git -C bare.git config diff.custom.binary true
> +'
>
> test_expect_success \
> 'remove ignored file' \
>
Looks good, thanks!
René
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Stefan Beller @ 2017-01-03 17:52 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git@vger.kernel.org
In-Reply-To: <20161231114412.23439-1-pranit.bauva@gmail.com>
On Sat, Dec 31, 2016 at 3:44 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> test_must_fail should only be used for testing git commands. To test the
> failure of other commands use `!`.
>
> Reported-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Thanks for writing up such a patch!
I had put it on my todo list, but you
were faster on actually going through.
Thanks,
Stefan
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)
From: Brandon Williams @ 2017-01-03 17:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8BxsEtU0q4VBpRpELTnubmL624n35Hw3HPhBVx4=6b5DQ@mail.gmail.com>
On 01/03, Duy Nguyen wrote:
> On Thu, Dec 29, 2016 at 5:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams <bmwill@google.com> wrote:
> >> On 12/27, Junio C Hamano wrote:
> >>> * bw/pathspec-cleanup (2016-12-14) 16 commits
> >>> - pathspec: rename prefix_pathspec to init_pathspec_item
> >>> - pathspec: small readability changes
> >>> - pathspec: create strip submodule slash helpers
> >>> - pathspec: create parse_element_magic helper
> >>> - pathspec: create parse_long_magic function
> >>> - pathspec: create parse_short_magic function
> >>> - pathspec: factor global magic into its own function
> >>> - pathspec: simpler logic to prefix original pathspec elements
> >>> - pathspec: always show mnemonic and name in unsupported_magic
> >>> - pathspec: remove unused variable from unsupported_magic
> >>> - pathspec: copy and free owned memory
> >>> - pathspec: remove the deprecated get_pathspec function
> >>> - ls-tree: convert show_recursive to use the pathspec struct interface
> >>> - dir: convert fill_directory to use the pathspec struct interface
> >>> - dir: remove struct path_simplify
> >>> - mv: remove use of deprecated 'get_pathspec()'
> >>>
> >>> Code clean-up in the pathspec API.
> >>>
> >>> Waiting for the (hopefully) final round of review before 'next'.
> >>
> >> What more needs to be reviewed for this series?
> >
> > I wanted to have a look at it but I successfully managed to put if off
> > so far. Will do soonish.
>
> I have just sent a couple of minor comments. The rest looks good!
Thanks! I'll go take a look.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 2/2] t9813: avoid using pipes
From: Stefan Beller @ 2017-01-03 17:58 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git@vger.kernel.org, luke, Johannes Sixt
In-Reply-To: <20170102184536.10488-2-pranit.bauva@gmail.com>
On Mon, Jan 2, 2017 at 10:45 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it.
for commands under test, i.e. git things. Other parts can be piped if that makes
the test easier. Though I guess that can be guessed by the reader as well,
as you only convert git commands on upstream pipes.
> By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Thanks for taking ownership of this issue as well. :)
> ---
> 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..9d7550ff3 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 2>&1 &&
Why do we need to pipe 2>&1 here?
Originally the piping only fed the stdout to grep, so this patch changes the
test? Maybe
2>actual.err &&
test_must_be_empty actual.err
instead?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 08/16] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2017-01-03 18:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Cja1um2oFDWufyk_7xaZbf9+=kyuWFx==Vb0nHiMqiwA@mail.gmail.com>
On 01/03, Duy Nguyen wrote:
> On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams <bmwill@google.com> wrote:
> > @@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char *pattern,
> > continue;
> > if (sb.len)
> > strbuf_addch(&sb, ' ');
> > - if (short_magic & m->bit)
> > - strbuf_addf(&sb, "'%c'", m->mnemonic);
> > +
> > + if (m->mnemonic)
> > + strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
> > else
> > strbuf_addf(&sb, "'%s'", m->name);
> > }
>
> The die() call is out of diff context, but it'll print
>
> pathspec magic not supported by this command: (!)top
>
> which looks too much like :(<name>)<mnemonic> pathspec syntax too me
> and threw me off a bit. And it's a bit cryptic, isn't it? Since this
> is meant for human, maybe we can just write
>
> pathspec magic not supported by this command: top (mnemonic: '!')
I was trying to keep it short and sweet, turns out that ends up being
more difficult to understand. I like your suggestion, it definitely
makes things much clearer.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-03 18:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Jeff King
In-Reply-To: <xmqqh95j60uh.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>>
>> For now just improve the user visible error message.
>
> Thans. judging from the date: header I take this is meant as v3 that
> supersedes v2 done on Wednesday.
Yes, that is correct. Sorry for being sloppy not numbering the
patches correctly.
>
> It is not clear in the above that what this thing is. Given that we
> are de-asserting it, is the early part of the new code diagnosing an
> end-user error (i.e. you gave me a pathspec but that extends into a
> submodule which is a no-no)? The "was a submodule issue" comment
> added is "this is an end-user mistake, there is nothing to fix in
> the code"?
This is not a fix in the code, but purely improving an error message.
So far anytime someone run into this assert, it was related to submodules.
I do not know the pathspec code well enough to claim this condition
can be produced via submodules *only*, though.
So I proposed a more defensive patch, which diagnoses if it is the
"no-no, pathspec extends into a submodule" first and then throws
a generic error afterwards in case it is not the submodule issue.
> I take that the new "BUG" thing tells the Git developers that no
> sane codepath should throw an pathspec_item that satisfies the
> condition of the if() statement for non-submodules?
If we want to keep the semantics of the assert around, then we
have to have a blank statement if the submodule error message
is not triggered.
I assume if we print this BUG, then there is an actual bug.
>
>> diff --git a/pathspec.c b/pathspec.c
>> index 22ca74a126..b446d79615 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -313,8 +313,23 @@ 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 ce_len = ce_namelen(ce);
>> + int len = ce_len < item->len ? ce_len : item->len;
>> + if (!S_ISGITLINK(ce->ce_mode))
>> + continue;
>
> Computation of ce_len and len are better done after this check, no?
Yes, though I trusted the modern-day-compilers to get it right. Will
fix in a reroll.
>> +test_expect_success 'setup a submodule' '
>> + test_commit 1 &&
>> + git submodule add ./ sub &&
>
> Is this adding our own project as its submodule?
Yes it is.
>
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.
I agree that this is not a good idea.
> No sane project does that in real life, doesn't it?
If such a project was cloned with submodules, it would recurse endlessly. :)
> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule. That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.
I wonder if we want to have a helper function in test-lib.sh to be used
for that. This use case (have a repository and a submodule) happens in
a lot of tests, so we could make life easier by providing a function
in the library so it is even easier than this HACK.
> Better yet create a separate repository, have a commit there, and
> then pull it in with "git submodule add && git submodule init" into
> our repository. That would be the normal way to borrow somebody
> else's project as a part of your own superproject.
The library function could do that, yes.
Thanks,
Stefan
^ permalink raw reply
* [PATCHv2] submodule.c: use GIT_DIR_ENVIRONMENT consistently
From: Stefan Beller @ 2017-01-03 18:30 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <6dd0a31e-d877-5311-37ef-313ed9ab9716@web.de>
In C code we have the luxury of having constants for all the important
things that are hard coded. This is the only place in C, that hard codes
the git directory environment variable, so fix it.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
v2:
argv_array_pushf and realigned.
v1:
Signed-off-by-the-format-patch-config ;)
This is the only occurrence for "GIT_DIR=" in C, but what about ".git"
git grep "\.git\"" *.c finds some places, which we may want to convert
to DEFAULT_GIT_DIR_ENVIRONMENT?
(mainly things that are newer if I can judge the places correctly
lots of submodules, worktrees and the no data in ".git" bug AFAICT)
Thanks,
Stefan
submodule.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
- argv_array_push(out, "GIT_DIR=.git");
+ argv_array_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+ DEFAULT_GIT_DIR_ENVIRONMENT);
}
--
2.11.0.259.ga95e92af08.dirty
^ permalink raw reply related
* Re: builtin difftool parsing issue
From: Stefan Beller @ 2017-01-03 18:47 UTC (permalink / raw)
To: Paul Sbarra, Jacob Keller
Cc: Johannes Schindelin, David Aguilar, Dennis Kaarsemaker,
git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGf+dSjM9nuroeSM9mkQmO3ho4XcZhLo1CR76q-jbeQ-WNGG+Q@mail.gmail.com>
On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> wrote:
>> I would have expected `git difftool --submodule=diff ...` to work... What
>> are the problems?
>
> The docs for difftool state...
> "git difftool is a frontend to git diff and accepts the same options
> and arguments."
I think such a sentence in the man page is dangerous, as nobody
was caught this issue until now. There have been numerous authors
and reviewers that touched "diff --submodule=<format>, but as there
is no back-reference, hinting that the patch is only half done, and the
difftool also needs to implement such an option.
We should reword the man page either as
"git difftool is a frontend to git diff and accepts the most(?) options
and arguments."
or even be explicit and list the arguments instead. There we could also
describe differences if any (e.g. the formats available might be different
for --submodule=<format>)
^ permalink raw reply
* [PATCH v4 03/16] dir: convert fill_directory to use the pathspec struct interface
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-1-bmwill@google.com>
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
dir.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 1df61f10f..e8ddd7f8a 100644
--- a/dir.c
+++ b/dir.c
@@ -174,17 +174,21 @@ char *common_prefix(const struct pathspec *pathspec)
int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
{
- size_t len;
+ char *prefix;
+ size_t prefix_len;
/*
* Calculate common prefix for the pathspec, and
* use that to optimize the directory walk
*/
- len = common_prefix_len(pathspec);
+ prefix = common_prefix(pathspec);
+ prefix_len = prefix ? strlen(prefix) : 0;
/* Read the directory and prune it */
- read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, pathspec);
- return len;
+ read_directory(dir, prefix, prefix_len, pathspec);
+
+ free(prefix);
+ return prefix_len;
}
int within_depth(const char *name, int namelen,
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v4 10/16] pathspec: factor global magic into its own function
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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 032436bc1..f760f44f9 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 v4 04/16] ls-tree: convert show_recursive to use the pathspec struct interface
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-1-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 | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 0e30d8623..d7ebeb4ce 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,
@@ -175,8 +173,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
* cannot be lifted until it is converted to use
* match_pathspec() or tree_entry_interesting()
*/
- parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE,
+ parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
PATHSPEC_PREFER_CWD,
prefix, argv + 1);
for (i = 0; i < pathspec.nr; i++)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v4 01/16] mv: remove use of deprecated 'get_pathspec()'
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-1-bmwill@google.com>
Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface. Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/mv.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877bc..4e86dc523 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -4,6 +4,7 @@
* Copyright (C) 2006 Johannes Schindelin
*/
#include "builtin.h"
+#include "pathspec.h"
#include "lockfile.h"
#include "dir.h"
#include "cache-tree.h"
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {
#define DUP_BASENAME 1
#define KEEP_TRAILING_SLASH 2
-static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
- int count, unsigned flags)
+static const char **internal_prefix_pathspec(const char *prefix,
+ const char **pathspec,
+ int count, unsigned flags)
{
int i;
const char **result;
+ int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
- COPY_ARRAY(result, pathspec, count);
- result[count] = NULL;
+
+ /* Create an intermediate copy of the pathspec based on the flags */
for (i = 0; i < count; i++) {
- int length = strlen(result[i]);
+ int length = strlen(pathspec[i]);
int to_copy = length;
+ char *it;
while (!(flags & KEEP_TRAILING_SLASH) &&
- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
to_copy--;
- if (to_copy != length || flags & DUP_BASENAME) {
- char *it = xmemdupz(result[i], to_copy);
- if (flags & DUP_BASENAME) {
- result[i] = xstrdup(basename(it));
- free(it);
- } else
- result[i] = it;
+
+ it = xmemdupz(pathspec[i], to_copy);
+ if (flags & DUP_BASENAME) {
+ result[i] = xstrdup(basename(it));
+ free(it);
+ } else {
+ result[i] = it;
}
}
- return get_pathspec(prefix, result);
+ result[count] = NULL;
+
+ /* Prefix the pathspec and free the old intermediate strings */
+ for (i = 0; i < count; i++) {
+ const char *match = prefix_path(prefix, prefixlen, result[i]);
+ free((char *) result[i]);
+ result[i] = match;
+ }
+
+ return result;
}
static const char *add_slash(const char *path)
@@ -130,7 +142,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
- source = internal_copy_pathspec(prefix, argv, argc, 0);
+ source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
/*
* Keep trailing slash, needed to let
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
flags = KEEP_TRAILING_SLASH;
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
flags = 0;
- dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
+ dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
submodule_gitfile = xcalloc(argc, sizeof(char *));
if (dest_path[0][0] == '\0')
/* special case: "." was normalized to "" */
- destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
else if (!lstat(dest_path[0], &st) &&
S_ISDIR(st.st_mode)) {
dest_path[0] = add_slash(dest_path[0]);
- destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
} else {
if (argc != 1)
die(_("destination '%s' is not a directory"), dest_path[0]);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v4 09/16] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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 ee87494c7..032436bc1 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 v4 14/16] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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 fe811a0a4..4a1f8ea44 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 v4 08/16] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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..ee87494c7 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 v4 07/16] pathspec: remove unused variable from unsupported_magic
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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
* [PATCH v4 05/16] pathspec: remove the deprecated get_pathspec function
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-1-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 540e45568..eb1fa9853 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 a50a61a19..0f80e01bd 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 22ca74a12..1f918cbae 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 59809e479..70a592e91 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.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH v4 13/16] pathspec: create parse_element_magic helper
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-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 f1439f6f9..fe811a0a4 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
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