* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Jeff King @ 2016-12-08 17:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote:
> The idea here is to discover the .git/ directory gently (i.e. without
> changing the current working directory), and to use it to read the
> .git/config file early, before we actually called setup_git_directory()
> (if we ever do that).
Great. Thanks for taking a stab at this.
> Notes:
>
> - I find the diff pretty ugly: I wish there was a more elegant way to
> *disable* discovery of .git/ *just* for `init` and `clone`. I
> considered a function `about_to_create_git_dir()` that is called in a
> hard-coded manner *only* for `init` and `clone`, but that would
> introduce another magic side effect, when all I want is to reduce those.
It looks like a lot of that ugliness comes from passing around the "are
we OK to peek at git-dir config" flag through the various pager-related
calls. I don't think it would be bad to use a global for "we do not
want a repo". After all, it's just modifying the _existing_ global for
"are we in a repo". And I do not see that global going away anytime
soon. Sometimes it's good to make incremental steps towards an end goal,
but in this case it seems like we just pay the cost of the step without
any real plan for reaping the benefit in the long term.
As an alternative, I also think it would be OK to just always have the
pager config read from local repo, even for init/clone. In other words,
to loosen the idea that git-init can _never_ look in the current
git-dir, and declare that there is a stage before the command is
initiated, and during which git may read local-repo config. Aliases
would fall into this, too, so:
git config --local alias.foo init
git foo /some/other/dir
would work (as it must, because we cannot know that "foo" is "init"
until we read the config!).
I have a feeling you may declare that too magical, but I think it's
consistent and practical.
> - For the moment, I do not handle dashed invocations of `init` and
> `clone` correctly. The real problem is the continued existence of
> the dashed git-init and git-clone, of course.
I assume you mean setting the CREATES_GIT_DIR flag here? I think it
would apply to the dashed invocations, too. They strip off the "git-"
and end up in handle_builtin() just like "git clone" does. I didn't test
it, though.
> - There is still duplicated code. For the sake of this RFC, I did not
> address that yet.
Yeah, I did not read your discover function very carefully. Because I
think the one thing we really don't want to do here is introduce a
separate lookup process that is not shared by setup_git_directory(). The
only sane path, I think, is to refactor setup_git_directory() to build
on top of discover_git_directory(), which implies that the latter
handles all of the cases.
> - The read_early_config() function is called multiple times, re-reading
> all the config files and re-discovering the .git/ directory multiple
> times, which is quite wasteful. For the sake of this RFC, I did not
> address that yet.
We already have a config-caching system. If we went with a global
"config_discover_refs", then I think the sequence for something like
git-init would become:
1. When git.c starts, config_discover_refs is set to "true". Pager and
alias lookup may look in .git/config if it's available, even if
they go through the configset cache.
2. As soon as git-init starts, it disables config_discover_refs, and
it flushes the config cache. Any configset lookups will now examine
the reduced config.
3. When git-init has set up the real repo it is operating on, it can
reenable config_discover_refs (though it may not even need to; that
flag probably wouldn't have any effect once we've entered the
repository and have_git_dir() returns true).
> - t7006 fails and the error message is a bit cryptic (not to mention the
> involved function trace, which is mind-boggling for what is supposed
> to be a simply, shell script-based test suite). I did not have time to
> look into that yet.
Running t7006 I see a lot of old failures turned into successes, which
is good (because running from a subdirectory now actually respects local
pager config). The one failure looks like it is testing the wrong thing.
It is checking that we _don't_ respect a local core.pager in some cases,
as a proxy for whether or not we are breaking things by doing setup too
early. But the whole point of your series is to fix that, and respect
the config without causing the setup breakage. After your patches, the
proxy behavior and the failure case are disconnected. The test should be
flipped, and ideally another one added that confirms we didn't actually
run setup_git_directory(), but I'm not sure how to test that directly.
> - after discover_git_directory_gently() did its work, the code happily
> uses its result *only* for the current read_early_config() run, and
> lets setup_git_dir_gently() do the whole work *again*. For the sake of
> this RFC, I did not address that yet.
If caching happens at the config layer, then we'd probably only call
this once anyway (or if we did call it again after a config flush, it
would be a good sign that we should compute its value again).
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] Make sequencer abort safer
From: Junio C Hamano @ 2016-12-08 17:27 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Stephan Beyer, git, Christian Couder, SZEDER Gábor
In-Reply-To: <alpine.DEB.2.20.1612081627290.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 30b10ba14..c9b560ac1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
>> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>
> Is it required by law to have a four-letter infix, or can we have a nicer
> variable name (e.g. git_path_current_file)?
I agree with you that, as other git_path_*_file variables match the
actual name on the filesystem, this one should too, together with
the update_curr_file() function.
By the way, this step seems to be a fix to an existing problem, and
the new test added in 3/5 seems to be a demonstration of the issue.
If that is the case, shouldn't the new test initially expect failure
and updated by this step to expect success?
I'll queue this on top of step 4/5 as "SQUASH???" as usual. The
other SQUASH??? that must come after 3/5 for t3510 should be trivial
(the reverse of what appears here).
Thanks.
sequencer.c | 22 +++++++++++-----------
t/t3510-cherry-pick-sequence.sh | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c9b560ac15..ce04377f8e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
-static void update_curr_file()
+static void update_current_file(void)
{
struct object_id head;
@@ -320,9 +320,9 @@ static void update_curr_file()
return;
if (!get_oid("HEAD", &head))
- write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+ write_file(git_path_current_file(), "%s", oid_to_hex(&head));
else
- write_file(git_path_curr_file(), "%s", "");
+ write_file(git_path_current_file(), "%s", "");
}
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
- update_curr_file();
+ update_current_file();
return 0;
}
@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
leave:
free_message(commit, &msg);
- update_curr_file();
+ update_current_file();
return res;
}
@@ -1149,23 +1149,23 @@ static int save_head(const char *head)
return 0;
}
-static int rollback_is_safe()
+static int rollback_is_safe(void)
{
struct strbuf sb = STRBUF_INIT;
struct object_id expected_head, actual_head;
- if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+ if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
strbuf_trim(&sb);
if (get_oid_hex(sb.buf, &expected_head)) {
strbuf_release(&sb);
- die(_("could not parse %s"), git_path_curr_file());
+ die(_("could not parse %s"), git_path_current_file());
}
strbuf_release(&sb);
}
else if (errno == ENOENT)
oidclr(&expected_head);
else
- die_errno(_("could not read '%s'"), git_path_curr_file());
+ die_errno(_("could not read '%s'"), git_path_current_file());
if (get_oid("HEAD", &actual_head))
oidclr(&actual_head);
@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
- update_curr_file();
+ update_current_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc485..372307c21b 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
'
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked anotherpick &&
git reset --hard base &&
^ permalink raw reply related
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Jeff King @ 2016-12-08 17:46 UTC (permalink / raw)
To: Stefan Beller; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <CAGZ79kZU401JRp4EtwTGHzk3Zq+snQhX3GArDfF6SpKxsSwtWg@mail.gmail.com>
On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>
> >
> > Previously test contained errorneous
> > test_must_fail, which was masked by
> > missing &&.
>
> I wonder if we could make either
> the test_must_fail intelligent to detect such a broken && call chain
> or the test_expect_success macro to see for those broken chains.
I don't think test_must_fail is relevant for &&-chains. Even something
like:
test_must_fail foo
bar
or:
bar
test_must_fail foo
will both trigger on the &&-chain linter, because it uses a magic exit
code to detect the breakage. I think the problem is just that the
&&-chain linter cannot peek inside subshells, and that's where the bug
was in this case.
I wish we could improve that, but I spend a lot of brain cycles on it at
one point and couldn't come up with a workable solution.
-Peff
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-08 17:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Stefan Beller, git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CACsJy8BdUsGD-Bx=-qiP3cWt2AjwD1P3NTbqEnjnKa1v0TvySQ@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/05, Stefan Beller wrote:
> >> > static const char *real_path_internal(const char *path, int die_on_error)
> >> > {
> >> > - static struct strbuf sb = STRBUF_INIT;
> >> > + static struct strbuf resolved = STRBUF_INIT;
> >>
> >> Also by having this static here, it is not quite thread safe, yet.
> >>
> >> By removing the static here we cannot do the early cheap check as:
> >>
> >> > /* We've already done it */
> >> > - if (path == sb.buf)
> >> > + if (path == resolved.buf)
> >> > return path;
> >>
> >> I wonder how often we run into this case; are there some callers explicitly
> >> relying on real_path_internal being cheap for repeated calls?
> >> (Maybe run the test suite with this early return instrumented? Not sure how
> >> to assess the impact of removing the cheap out return optimization)
> >>
> >> The long tail (i.e. the actual functionality) should actually be
> >> faster, I'd imagine
> >> as we do less than with using chdir.
> >
> > Depends on how expensive the chdir calls were. And I'm working to get
> > rid of the static buffer. Just need have the callers own the memory
> > first.
>
> I suggest you turn this real_path_internal into strbuf_real_path. In
> other words, it takes a strbuf and writes the result there, allocating
> memory if needed.
>
> This function can replace the two strbuf_addstr(..., real_path(..));
> we have in setup.c and sha1_file.c. real_path() can own a static
> strbuf buffer to retain current behavior. We could also have a new
> wrapper real_pathdup() around strbuf_real_path(), which can replace 9
> instances of xstrdup(real_path(...)) (and Stefan is adding a few more;
> that's what led me back to these mails)
Sounds like a plan, thanks for the advice.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Junio C Hamano @ 2016-12-08 18:01 UTC (permalink / raw)
To: Paul Tan; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com>
Paul Tan <pyokagan@gmail.com> writes:
> Hmm, to add on, looking at the three other call sites of this
> function, two of them (builtin/commit.c and builtin/describe.c)
> basically do:
>
> if (0 <= fd)
> update_index_if_able(...)
>
> with that 0 <= fd conditional. With this patch it becomes three out of
> four.
The other one is diff.c::refresh_index_quietly() that you are not
counting, I think, but if you look at it again, it also is not
called after hold_locked_index() fails to acquire the lock, so with
this fix everybody refrains from calling it when it does not hold
the lock.
> Perhaps the repeated use of this conditional is a sign that the
> 0 <= fd check could be built into update_index_if_able()?
That condition is "do we have the lock? Otherwise we are not even
allowed to update it", so in that sense it may make sense.
> I think there is precedent for building in these kind of checks --
> rollback_lock_file() also does not fail if the lock file was not
> successfully opened.
>
> That said, the number of call sites is quite low so it's probably not
> worth doing this.
Yeah, I can go either way. At least with the change things are
consistent.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net>
On Thu, Dec 8, 2016 at 9:46 AM, Jeff King <peff@peff.net> wrote:
>
> will both trigger on the &&-chain linter, because it uses a magic exit
> code to detect the breakage. I think the problem is just that the
> &&-chain linter cannot peek inside subshells, and that's where the bug
> was in this case.
Uh, yeah in the subshell, but the patch v2 did have it not in
subshells, I'll take another look.
>
> I wish we could improve that, but I spend a lot of brain cycles on it at
> one point and couldn't come up with a workable solution.
Is it possible to overwrite what happens when you open a subshell with ( ) ?
i.e. I imagine in the global test-setup we'd setup that ( ) not just
opens /bin/sh
but a shell with benefits such that we can execute just one && chain.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-08 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net>
On 12/08/2016 08:46 PM, Jeff King wrote:
> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
>
>> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>>
>>> Previously test contained errorneous
>>> test_must_fail, which was masked by
>>> missing &&.
>> I wonder if we could make either
>> the test_must_fail intelligent to detect such a broken && call chain
>> or the test_expect_success macro to see for those broken chains.
>>
>>
>> I wish we could improve that, but I spend a lot of brain cycles on it at
>> one point and couldn't come up with a workable solution.
>>
>> -Peff
>>
Why Git test use &&-chains instead of proper "set -e"?
^ permalink raw reply
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:06 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8DVuOdRsu4x9iFTwweSVn=RRJYzBTm43ufBevTWMf4Qmg@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
> >> > {
> >> > int i;
> >> > const char **result;
> >> > + struct pathspec ps;
> >> > 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;
> >> > + }
> >> > + result[count] = NULL;
> >> > +
> >> > + parse_pathspec(&ps,
> >> > + PATHSPEC_ALL_MAGIC &
> >> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> >> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> >> > + prefix, result);
> >> > + assert(count == ps.nr);
> >> > +
> >> > + /* Copy the pathspec and free the old intermediate strings */
> >> > + for (i = 0; i < count; i++) {
> >> > + const char *match = xstrdup(ps.items[i].match);
> >> > + free((char *) result[i]);
> >> > + result[i] = match;
> >>
> >> Sigh.. it looks so weird that we do all the parsing (in a _copy_
> >> pathspec function) then remove struct pathspec and return the plain
> >> string. I guess we can't do anything more until we rework cmd_mv code
> >> to handle pathspec natively.
> >>
> >> At the least I think we should rename this function to something else.
> >> But if you have time I really wish we could kill this function. I
> >> haven't stared at cmd_mv() long and hard, but it looks to me that we
> >> combining two separate functionalities in the same function here.
> >>
> >> If "mv" takes n arguments, then the first <n-1> arguments may be
> >> pathspec, the last one is always a plain path. The "dest_path =
> >> internal_copy_pathspec..." could be as simple as "dest_path =
> >> prefix_path(argv[argc - 1])". the special treatment for this last
> >> argument [1] can live here. Then, we can do parse_pathspec for the
> >> <n-1> arguments in cmd_mv(). It's still far from perfect, because
> >> cmd_mv can't handle pathspec properly, but it reduces the messy mess
> >> in internal_copy_pathspec a bit, I hope.
> >>
> >
> > Actually, after looking at this a bit more it seems like we could
> > technically use prefix_path for both source and dest (based on how the
> > current code is structured) since the source's provied must all exist (as
> > in no wildcards are allowed). We could drop using the pathspec struct
> > completely in addition to renaming the function (to what I'm still
> > unsure).
>
> Yeah that sounds good too (with a caveat: I'm not a heavy user of
> git-mv nor touching this code a lot, I might be missing something).
> It'll take some looong time before somebody starts converting it to
> use pathspec properly, I guess. prefix_path() would keep the code
> clean meanwhile.
K for now I'll switch to using prefix_path() and rename the function
`internal_prefix_pathspec()` as that is a bit more descriptive.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Robbie Iannucci @ 2016-12-08 18:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.2.20.1612081252490.23160@virtualbox>
Thanks all for taking a look at this, I didn't expect such a quick response :).
No hard feelings re: breakage; I know how gnarly these sorts of
refactors can be (and more libification is always better :)).
Robbie
On Thu, Dec 8, 2016 at 3:53 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Wed, 7 Dec 2016, Junio C Hamano wrote:
>
>> The "libify sequencer" topic stopped passing the die_on_error option
>> to hold_locked_index(), and this lost an error message from "git
>> merge --ff-only $commit" when there are competing updates in
>> progress.
>
> Sorry for the breakage.
>
> When libifying the code, I tried to be careful to retain the error
> messages when not dying, and mistakenly assumed that hold_locked_index()
> would do the same.
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:19 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8DtUwnOjBV49navkfgqPzEsNuX2LVaeVU=Ap2PWLpGFdA@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/07, Duy Nguyen wrote:
> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> > using the '_raw' entry in the pathspec.
> >>
> >> It would be even better to kill this create_simplify() and let
> >> simplify_away() handle struct pathspec directly.
> >>
> >> There is a bug in this code, that might have been found if we
> >> simpify_away() handled pathspec directly: the memcmp() in
> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> ignore exclude patterns there too (although not excluding is not a
> >> bug).
> >
> > So are you implying that the simplify struct needs to be killed? That
> > way the pathspec struct itself is being passed around instead?
>
> Yes. simplify struct was a thing when pathspec was an array of char *.
> At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 18:21 UTC (permalink / raw)
To: vi0oss; +Cc: Jeff King, git@vger.kernel.org, Stefan Beller
In-Reply-To: <d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com>
On Thu, Dec 8, 2016 at 10:04 AM, vi0oss <vi0oss@gmail.com> wrote:
> On 12/08/2016 08:46 PM, Jeff King wrote:
>>
>> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
>>
>>> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>>>
>>>> Previously test contained errorneous
>>>> test_must_fail, which was masked by
>>>> missing &&.
>>>
>>> I wonder if we could make either
>>> the test_must_fail intelligent to detect such a broken && call chain
>>> or the test_expect_success macro to see for those broken chains.
>>>
>>>
>>> I wish we could improve that, but I spend a lot of brain cycles on it at
>>> one point and couldn't come up with a workable solution.
>>>
>>> -Peff
>>>
> Why Git test use &&-chains instead of proper "set -e"?
>
"Because set -e kills the shell and we would want to keep going
until the test suite is finished and display a summary what failed"
would be my first reaction, but let's dig into history:
bb79af9d09 might be helpful on that, but it doesn't explain why
we use && chains.
I could not find any commit explaining the use of && chains.
e1970ce43abf might be interesting (the introduction of the test
suite), as that did not contain && chains.
I guess it would be hard(er) to implement e.g. test_must_fail
in an environment where -e is set.
^ permalink raw reply
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Junio C Hamano @ 2016-12-08 18:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Robbie Iannucci
In-Reply-To: <alpine.DEB.2.20.1612081252490.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Sorry for the breakage.
Apologies from me, too. Once a topic is merged, the credit still
remains with the contributor, but the blame is shared by the project
as a whole, with those who missed breakages during their reviews,
and those who didn't review or test and let breakages pass.
> When libifying the code, I tried to be careful to retain the error
> messages when not dying,...
Quite honestly, I do not think either of us cared about preserving
the exact error message the end-user was getting from each failure
sites that the series changed a call with die-on-error=1 to a call
with die-on-error=0 that is followed by a negative return while
reviewing this series. As I wrote in the proposed log message for
3/3, this one was noticed as a end-user breaking change because it
was the only one that has become totally silent. For example, this
bit from sequencer.c::write_message() we can see in the output from
"git show --first-parent 2a4062a4a8" does not preserve the message
at all:
diff --git a/sequencer.c b/sequencer.c
index 3804fa931d..eec8a60d6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@
...
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
{
static struct lock_file msg_file;
- int msg_fd = hold_lock_file_for_update(&msg_file, filename,
- LOCK_DIE_ON_ERROR);
+ int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+ if (msg_fd < 0)
+ return error_errno(_("Could not lock '%s'"), filename);
And I do not think it is necessarily bad that the error message
changed with this conversion. In other words, I do not think it
should have been the goal to preserve the exact error message.
hold_lock*() can afford to give a detailed message that strongly
sounds as being the final decision when called with die-on-error=1
because it knows it is dying. However, the message from the updated
write_message(), "could not lock", cannot be final---the caller may
want to add something else after it to describe what failed in a
larger picture.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Johannes Sixt @ 2016-12-08 18:41 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Brandon Williams, Junio C Hamano, Ramsay Jones, git, sbeller,
peff, jacob.keller
In-Reply-To: <20161208075555.GA23595@tb-raspi>
Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
> Some conversion may be done in mingw.c:
> https://github.com/github/git-msysgit/blob/master/compat/mingw.c
> So what I understand, '/' in Git are already converted into '\' if needed ?
Only if needed, and there are not many places where this is the case.
(Actually, I can't find one place where we do.) In particular, typical
file accesses does not require the conversion.
> It seams that we may wnat a function get_start_of_path(uncpath),
> which returns:
>
> get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
We have offset_1st_component().
-- Hannes
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Jeff King @ 2016-12-08 18:53 UTC (permalink / raw)
To: vi0oss; +Cc: Stefan Beller, git@vger.kernel.org, Stefan Beller
In-Reply-To: <d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com>
On Thu, Dec 08, 2016 at 09:04:46PM +0300, vi0oss wrote:
> Why Git test use &&-chains instead of proper "set -e"?
Because "set -e" comes with all kinds of confusing corner cases. Using
&& chains is annoying, but rarely surprising.
One of my favorite examples is:
set -e
(
false
echo 1
) || {
echo outcome=$?
false
}
echo 2
which prints both "1" and "2".
Inside the subshell, "set -e" has no effect, and you cannot re-enable it
by setting "-e" (it's suppressed entirely because we are on the
left-hand side of an || conditional).
So you could write a function like this:
foo() {
do_one
do_two
}
that relies on catching the failure from do_one. And it works here:
set -e
foo
but not here:
set -e
if foo then
do_something
fi
And there's no way to make it work without adding back in the
&&-chaining.
-Peff
^ permalink raw reply
* Re: [PATCHv6 4/7] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-08 18:55 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Brandon Williams, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8Bs78ywGq5p6yAFGi1KACAXFEeroyQSJye5-RL5gqOS+Q@mail.gmail.com>
On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
>>
>> worktree = xcalloc(1, sizeof(*worktree));
>> worktree->path = strbuf_detach(&worktree_path, NULL);
>> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>
> All the good stuff is outside context lines again.. Somewhere between
> here we call add_head_info() which calls resolve_ref_unsafe(), which
> always uses data from current repo, not the submodule we want it to
> look at.
Unrelated side question: What would you think of "variable context line
configuration" ? e.g. you could configure it to include anything from
up that line
that is currently shown after the @@ which is the function signature line.
As to the add_head_info/resolve_ref_unsafe what impact does that have?
It produces a wrong head info but AFAICT it will never die(), such that for the
purposes of this series (which only wants to know if a submodule uses the
worktree feature) it should be fine.
It is highly misleading though for others to build upon this.
So maybe I'll only add the functionality internally in worktree.c
and document why the values are wrong, and only expose the
"int submodule_uses_worktrees(const char *path)" ?
>> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>> return list;
>
> Right before this line is mark_current_worktree(), which in turn calls
> get_git_dir() and not suitable for peeking into another repository the
> way submodule code does. get_worktree_git_dir() called within that
> function shares the same problem.
It actually works correctly: "No submodule is the current worktree".
>
>> }
>>
>> +struct worktree **get_worktrees(unsigned flags)
>> +{
>> + return get_worktrees_internal(get_git_common_dir(), flags);
>> +}
>> +
>> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
>> +{
>> + char *submodule_gitdir;
>> + struct strbuf sb = STRBUF_INIT;
>> + struct worktree **ret;
>> +
>> + submodule_gitdir = git_pathdup_submodule(path, "%s", "");
>> + if (!submodule_gitdir)
>> + return NULL;
>> +
>> + /* the env would be set for the superproject */
>> + get_common_dir_noenv(&sb, submodule_gitdir);
>
> Technically we need to read submodule_gitdir/.config and see if we can
> understand core.repositoryformatversion, or find any unrecognized
> extensions. But the problem is not specific to this code. And fixing
> it is no small task. But perhaps we could call a dummy
> validate_submodule_gitdir() here? Then when we implement that function
> for real, we don't have to search the entire code base to see where to
> put it.
>
> Kinda off-topic though. Feel free to ignore the above comment.
ok I'll add a TODO/emptyfunction for that.
Thanks for the review!
Stefan
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Junio C Hamano @ 2016-12-08 18:58 UTC (permalink / raw)
To: Jacob Keller; +Cc: Karthik Nayak, Git mailing list, Jakub Narębski
In-Reply-To: <CA+P7+xquordVY19dypqNcAuQqoRbFmHhzb0w+HXCaJmm_Ex7zQ@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> + are left behind. If a displayed ref has fewer components than
>> + `<N>`, the command aborts with an error.
>>
>
> Would it make more sense to not die and instead just return the empty
> string? On the one hand, if we die() it's obvious that you tried to
> strip too many components. But on the other hand, it's also somewhat
> annoying to have the whole command fail because we happen upon a
> single ref that has fewer components?
>
> So, for positive numbers, we simply strip what we can, which may
> result in the empty string, and for negative numbers, we keep up to
> what we said, while potentially keeping the entire string. I feel
> that's a better alternative than a die() in the middle of a ref
> filter..
>
> What are other people's thoughts on this?
There probably are three ways to handle a formatting request that
cannot be satisfied for some refs but not others [*1*]. I agree
with you that dying the whole thing is probably the least useful
one.
We already format "%(taggername)" into an empty string when the ref
points at an object that is not an annotated tag, and substituting
an unsatisifiable request "%(refname:lstrip=N)" with an empty string
for a ref whose name does not have enough number of components is in
line with that existing practice.
The other possibility is to omit refs that cannot be formatted
according to the format specifier. We do not currently have
provision for doing so, but it may not be bad if we can say:
$ git for-each-ref \
--require="%(taggername)" \
--require="%(refname:lstrip=4)" \
--format="%(refname:short)" \
refs/tags/
to list _only_ annotated tags that has at least 4 components from
refs/tags/ hierarchy.
The "--require" thing obviously is an orthogonal feature, that
nobody has asked for, and does not have to exist. For the purpose
of this review thread, I think it is OK to just conclude:
"--format" should replace "%(any unsatisifiable atom)" with an
empty string.
Thanks.
[Footnote]
*1* A malformed formatting request (e.g. %(if) that is not closed)
cannot be satisified but that is true for all refs and is
outside of the scope of this discussion. The command should die
and I think it already does.
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string
From: SZEDER Gábor @ 2016-12-08 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
In-Reply-To: <20161207160923.7028-3-szeder.dev@gmail.com>
On Wed, Dec 7, 2016 at 5:09 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> ref-filter's parse_ref_filter_atom() function parses an atom between
> the start and end pointers it gets as arguments. This is fine for two
> of its callers, which process '%(atom)' format specifiers and the end
> pointer comes directly from strchr() looking for the closing ')'.
> However, it's not quite so straightforward for its other two callers,
> which process sort specifiers given as plain nul-terminated strings.
> Especially not for ref_default_sorting(), which has the default
> hard-coded as a string literal, but can't use it directly, because a
> pointer to the end of that string literal is needed as well.
> The next patch will add yet another caller using a string literal.
Oops, that last sentence should be deleted, there is no third patch, sorry.
Gábor
^ permalink raw reply
* [PATCH v2 00/16] pathspec cleanup
From: Brandon Williams @ 2016-12-08 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
v2 of this series addresses the comments brought up in v1, most of which were
small cosmetic changes (since this is mostly a cosmetic series to begin with).
Brandon Williams (16):
mv: remove use of deprecated 'get_pathspec()'
dir: convert create_simplify to use the pathspec struct interface
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: rename prefix_pathspec to init_pathspec_item
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 16 +-
builtin/mv.c | 50 ++--
cache.h | 1 -
dir.c | 37 +--
pathspec.c | 468 +++++++++++++++++++---------------
pathspec.h | 5 +-
7 files changed, 317 insertions(+), 262 deletions(-)
--- interdiff from v1
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index e0f4307..d7ebeb4 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -173,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++)
diff --git a/builtin/mv.c b/builtin/mv.c
index b7cceb6..4e86dc5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -20,13 +20,13 @@ 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;
- struct pathspec ps;
+ int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
/* Create an intermediate copy of the pathspec based on the flags */
@@ -42,25 +42,19 @@ static const char **internal_copy_pathspec(const char *prefix,
if (flags & DUP_BASENAME) {
result[i] = xstrdup(basename(it));
free(it);
- } else
+ } else {
result[i] = it;
+ }
}
result[count] = NULL;
- parse_pathspec(&ps,
- PATHSPEC_ALL_MAGIC &
- ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
- PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
- prefix, result);
- assert(count == ps.nr);
-
- /* Copy the pathspec and free the old intermediate strings */
+ /* 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] = xstrdup(ps.items[i].match);
+ result[i] = match;
}
- clear_pathspec(&ps);
return result;
}
@@ -148,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
@@ -158,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]);
diff --git a/dir.c b/dir.c
index 8730a4f..a50b6f0 100644
--- a/dir.c
+++ b/dir.c
@@ -179,18 +179,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->items[0].match : "",
- len, pathspec);
- return len;
+ read_directory(dir, prefix, prefix_len, pathspec);
+
+ free(prefix);
+ return prefix_len;
}
int within_depth(const char *name, int namelen,
diff --git a/pathspec.c b/pathspec.c
index 66db257..08abdd3 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -89,41 +89,42 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
static inline int get_literal_global(void)
{
- static int literal_global = -1;
+ static int literal = -1;
- if (literal_global < 0)
- literal_global = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,
- 0);
- return literal_global;
+ if (literal < 0)
+ literal = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
+
+ return literal;
}
static inline int get_glob_global(void)
{
- static int glob_global = -1;
+ static int glob = -1;
+
+ if (glob < 0)
+ glob = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
- if (glob_global < 0)
- glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
- return glob_global;
+ return glob;
}
static inline int get_noglob_global(void)
{
- static int noglob_global = -1;
+ static int noglob = -1;
+
+ if (noglob < 0)
+ noglob = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
- if (noglob_global < 0)
- noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,
- 0);
- return noglob_global;
+ return noglob;
}
static inline int get_icase_global(void)
{
- static int icase_global = -1;
+ static int icase = -1;
- if (icase_global < 0)
- icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+ if (icase < 0)
+ icase = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
- return icase_global;
+ return icase;
}
static int get_global_magic(int element_magic)
@@ -296,13 +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.
+ * 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;
@@ -310,7 +309,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
int pathspec_prefix = -1;
/* PATHSPEC_LITERAL_PATH ignores magic */
- if (!(flags & PATHSPEC_LITERAL_PATH)) {
+ if (flags & PATHSPEC_LITERAL_PATH) {
+ magic = PATHSPEC_LITERAL;
+ } else {
copyfrom = parse_element_magic(&element_magic,
&pathspec_prefix,
elt);
@@ -318,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
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");
@@ -390,7 +393,6 @@ 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);
- return magic;
}
static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -489,17 +491,12 @@ 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);
- if ((flags & PATHSPEC_LITERAL_PATH) &&
- !(magic_mask & PATHSPEC_LITERAL))
- item[i].magic |= PATHSPEC_LITERAL;
+ init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
+
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
- unsupported_magic(entry,
- item[i].magic & magic_mask);
+ 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.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 01/16] mv: remove use of deprecated 'get_pathspec()'
From: Brandon Williams @ 2016-12-08 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-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 2f43877..4e86dc5 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.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 02/16] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-bmwill@google.com>
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
dir.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a..7df292b 100644
--- a/dir.c
+++ b/dir.c
@@ -1787,25 +1787,24 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
}
-static struct path_simplify *create_simplify(const char **pathspec)
+static struct path_simplify *create_simplify(const struct pathspec *pathspec)
{
- int nr, alloc = 0;
+ int i;
struct path_simplify *simplify = NULL;
- if (!pathspec)
+ if (!pathspec || !pathspec->nr)
return NULL;
- for (nr = 0 ; ; nr++) {
+ ALLOC_ARRAY(simplify, pathspec->nr + 1);
+ for (i = 0; i < pathspec->nr; i++) {
const char *match;
- ALLOC_GROW(simplify, nr + 1, alloc);
- match = *pathspec++;
- if (!match)
- break;
- simplify[nr].path = match;
- simplify[nr].len = simple_length(match);
+ match = pathspec->items[i].match;
+ simplify[i].path = match;
+ simplify[i].len = pathspec->items[i].nowildcard_len;
}
- simplify[nr].path = NULL;
- simplify[nr].len = 0;
+ simplify[i].path = NULL;
+ simplify[i].len = 0;
+
return simplify;
}
@@ -2036,7 +2035,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const stru
* subset of positive ones, which has no impacts on
* create_simplify().
*/
- simplify = create_simplify(pathspec ? pathspec->_raw : NULL);
+ simplify = create_simplify(pathspec);
untracked = validate_untracked_cache(dir, len, pathspec);
if (!untracked)
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 03/16] dir: convert fill_directory to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-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 7df292b..a50b6f0 100644
--- a/dir.c
+++ b/dir.c
@@ -179,17 +179,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.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 06/16] pathspec: copy and free owned memory
From: Brandon Williams @ 2016-12-08 18:59 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-bmwill@google.com>
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 22 ++++++++++++++++++----
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1f918cb..8f367f0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
- } else
- item->original = elt;
+ } else {
+ item->original = xstrdup(elt);
+ }
item->len = strlen(item->match);
item->prefix = prefixlen;
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
pathspec->items = item = xcalloc(1, sizeof(*item));
- item->match = prefix;
- item->original = prefix;
+ item->match = xstrdup(prefix);
+ item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,26 @@ void parse_pathspec(struct pathspec *pathspec,
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
+ int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+ for (i = 0; i < dst->nr; i++) {
+ dst->items[i].match = xstrdup(src->items[i].match);
+ dst->items[i].original = xstrdup(src->items[i].original);
+ }
}
void clear_pathspec(struct pathspec *pathspec)
{
+ int i;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ free(pathspec->items[i].match);
+ free(pathspec->items[i].original);
+ }
free(pathspec->items);
pathspec->items = NULL;
}
diff --git a/pathspec.h b/pathspec.h
index 70a592e..49fd823 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
- const char *match;
- const char *original;
+ char *match;
+ char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 11/16] pathspec: create parse_short_magic function
From: Brandon Williams @ 2016-12-08 18:59 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-bmwill@google.com>
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 54 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 523d7bf..29054d2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,41 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for short magic
+ *
+ * saves all magic in 'magic'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_short_magic(unsigned *magic, const char *elem)
+{
+ const char *pos;
+
+ for (pos = elem + 1; *pos && *pos != ':'; pos++) {
+ char ch = *pos;
+ int i;
+
+ if (!is_pathspec_magic(ch))
+ break;
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (pathspec_magic[i].mnemonic == ch) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Unimplemented pathspec magic '%c' in '%s'"),
+ ch, elem);
+ }
+
+ if (*pos == ':')
+ pos++;
+
+ return pos;
+}
+
+/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
*
@@ -220,24 +255,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
copyfrom++;
} else {
/* shorthand */
- for (copyfrom = elt + 1;
- *copyfrom && *copyfrom != ':';
- copyfrom++) {
- char ch = *copyfrom;
-
- if (!is_pathspec_magic(ch))
- break;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (pathspec_magic[i].mnemonic == ch) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Unimplemented pathspec magic '%c' in '%s'"),
- ch, elt);
- }
- if (*copyfrom == ':')
- copyfrom++;
+ copyfrom = parse_short_magic(&element_magic, elt);
}
magic |= element_magic;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 10/16] pathspec: factor global magic into its own function
From: Brandon Williams @ 2016-12-08 18:59 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-bmwill@google.com>
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 127 +++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 78 insertions(+), 49 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 49adea4..523d7bf 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 */
@@ -407,9 +438,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.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 09/16] pathspec: simpler logic to prefix original pathspec elements
From: Brandon Williams @ 2016-12-08 18:59 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <1481223550-65277-1-git-send-email-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 a360193..49adea4 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.8.0.rc3.226.g39d4020
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox