* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Jacob Keller @ 2016-12-07 23:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqqa8c7wfxu.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> ... Suppose that you version all
>> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
>> Now, you also have other tags which represent -rc releases and other
>> such tags. If you want to find the first major release that contains
>> a given commit you might try
>>
>> git describe --contains --match="v?.?" <commit>
>>
>> This will work as long as you have only single digits. But if you start
>> adding multiple digits, the pattern becomes not enough to match all the
>> tags you wanted while excluding the ones you didn't.
>
> Isn't what you really want for the use case a negative pattern,
> i.e. "I want ones that match v* but not the ones that match *-rc*",
> though?
That's another way of implementing it. I just went with straight
forward patterns that I was already using in sequence.
Basically, this started as a script to try each pattern in sequence,
but this is slow, cumbersome and easy to mess up.
You're suggesting just add a single second pattern that we will do
matches and discard any tag that matches that first?
I think I can implement that pretty easily, and it should have simpler
semantics. We can discard first, and then match what remains easily.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv5 0/5] submodule embedgitdirs
From: Stefan Beller @ 2016-12-07 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqeg1juxd7.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> v5:
>>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>>> moving a git dir of one repository. The submodule specific stuff (e.g.
>>> recursion into nested submodules) is in submodule.{c,h}
>>>
>>> This was motivated by reviews on the series of checkout aware of submodules
>>> building on top of this series, as we want to directly call the embed-git-dirs
>>> function without the overhead of spawning a child process.
>>
>> OK. Comparing the last steps between this round and the previous
>> one, I do think the separation of the responsibility among helpers
>> is much more reasonable in this version, where:
>>
>> - submodule_embed_git_dir() is given a single path and is
>> responsible for that submodule itself, which is done by calling
>> submodule_embed_git_dir_for_path() on itself, and its
>> sub-submodules, which is done by spawning the helper recursively
>> with appropriate super-prefix;
>>
>> - submodule_embed_git_dir_for_path() computes where the given path
>> needs to be moved to using the knowledge specific to the
>> submodule subsystem, and asks relocate_gitdir() to perform the
>> actual relocation;
>>
>> - relocate_gitdir() used to do quite a lot more, but now it is only
>> about moving an existing .git directory elsewhere and pointing to
>> the new location with .git file placed in the old location.
>>
>> I would have called the second helper submodule_embed_one_git_dir(),
>> but that is a minor detail.
>>
>> Very nicely done.
>
> One thing that is not so nice from the code organization point of
> view is that "dir.c" now needs to include "submodule.h" because it
> wants to call connect_work_tree_and_git_dir().
>
> I wonder if that function belongs to dir.c or worktree.c not
> submodule.c; I do not see anything that is submodule specific about
> what the function does.
Right, it just happened historically for that function to live
in submodule area. I'll move the connect_work_tree_and_git_dir
to dir.c as well.
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Junio C Hamano @ 2016-12-07 23:42 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Git Mailing List, Rasmus Villemoes
In-Reply-To: <CACsJy8A=KeGsXAt6ZR-eOkTurSsnYPkt3yTfkYT9aZ86rV1rYg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Dec 6, 2016 at 8:42 PM, Jeff King <peff@peff.net> wrote:
>> The final one _seems_ reasonable after reading your explanation, but I
>> lack enough context to know whether or not there might be a corner case
>> that you're missing. I'm inclined to trust your assessment on it.
>
> Yeah I basically just wrote down my thoughts so somebody could maybe
> spot something wrong. I'm going to think about it some more in the
> next few days.
In the meantime let me queue them as-is.
Thanks.
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Stefan Beller @ 2016-12-07 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqlgvruyt2.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 3:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
>> {"resolve-relative-url", resolve_relative_url, 0},
>> {"resolve-relative-url-test", resolve_relative_url_test, 0},
>> {"init", module_init, 0},
>> - {"remote-branch", resolve_remote_submodule_branch, 0}
>> + {"remote-branch", resolve_remote_submodule_branch, 0},
>> + {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
>> };
>
> If you want to avoid patch noise like this, your 2/5 can add a
> trailing comma after the entry for remote-branch. It is OK to end
> elements in an array literal with trailing comma, even though we
> avoid doing similar in enum definition (which is only allowed in
> newer C standards).
Ok, thanks for pointing out!
>
>> diff --git a/dir.c b/dir.c
>> index bfa8c8a9a5..e023b04407 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,
>> {
>> untracked_cache_invalidate_path(istate, path);
>> }
>> +
>> +/*
>> + * Migrate the git directory of the given `path` from `old_git_dir` to
>> + * `new_git_dir`. If an error occurs, append it to `err` and return the
>> + * error code.
>> + */
>> +int relocate_gitdir(const char *path, const char *old_git_dir,
>> + const char *new_git_dir, const char *displaypath,
>> + struct strbuf *err)
>> +{
>> + int ret = 0;
>> +
>> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
>> + displaypath, old_git_dir, new_git_dir);
>
> By using "strbuf err", it looks like that the calling convention of
> this function wants to cater to callers who want to have tight
> control over their output and an unconditional printing to the
> standard output looks somewhat out of place.
Before sending it out to the list I had a version with another flag
that controlled whether we die on error or just print a warning.
That was not fully true however as the connect_work_tree_and_git_dir
would die nevertheless, we'd only warn for the failed rename().
It turns out we do not need a fully functional non-die()ing version
for the checkout series, so I ripped that out again and the version sent out
just dies in case of failure in relocate_gitdir.
That said, I think the printing of the migration message ought to go
into the caller and to stderr as you note.
>
> Besides, does it belong to the standard output? It looks more like
> a progress-bar eye candy that we send out to the standard error
> stream (and only when we are talking to a tty).
>
>> +/* Embeds a single submodule, non recursively. */
>> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)
>> +{
>> + struct worktree **worktrees;
>> + struct strbuf pathbuf = STRBUF_INIT;
>> + struct strbuf errbuf = STRBUF_INIT;
>> + char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
>> + const char *new_git_dir;
>> + const struct submodule *sub;
>> + int code;
>> +
>> + worktrees = get_submodule_worktrees(path, 0);
>> + if (worktrees) {
>> + int i;
>> + for (i = 0; worktrees[i]; i++)
>> + ;
>> + free_worktrees(worktrees);
>> + if (i > 1)
>> + die(_("relocate_gitdir for submodule '%s' with "
>> + "more than one worktree not supported"), path);
>> + }
>
> We may benefit from "does this have secondary worktrees?" boolean
> helper function, perhaps?
ok, I'll add that helper as its own patch to the worktree code earlier
in the series.
>
>> + old_git_dir = xstrfmt("%s/.git", path);
>> + if (read_gitfile(old_git_dir))
>> + /* If it is an actual gitfile, it doesn't need migration. */
>> + return;
>
> Isn't this "no-op return" a valid thing to do, even when the
> submodule has secondary worktrees that borrow from it?
If we have 2 worktrees, all bets are off (in my non-understanding).
The git file here *could* point to git directory living inside the
other working tree, which then would need migration from that other
working tree to some embedded place.
I don't think that is how worktrees work (or have ever worked?)
but I am unfamiliar with worktrees, so I erred on the safe side.
> I am
> wondering if the "ah, we don't do a repository that has secondary
> worktrees" check should come after this one.
Maybe Duy can answer that? (Where are git directories
located in a worktree setup, even historically? Were they
ever inside one of the submodules? The other working tree may
not be a submodule, but a standalone thing as well?)
>
>> + real_old_git_dir = xstrdup(real_path(old_git_dir));
>> +...
>> +}
>
>> +/*
>> + * Migrate the git directory of the submodule given by path from
>> + * having its git directory within the working tree to the git dir nested
>> + * in its superprojects git dir under modules/.
>> + */
>
> I think that this operation removes the git directories that are
> embedded in the working tree of the superproject and storing them
> away to safer place, i.e. unembedding.
Oh right we had that discussion what the embedding actually means.
I asked around for English language help here:
What about "absorbGitDir" ? (absorb as in eat/consume) for the external
UI and internally I can call it relocate_submodule_git_dir_into_superproject
(or embed_git_dir_into_superproject)
>> +
>> +
>
> Lose the extra blank line here?
ok
>> + if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
>
> Yeah, checking for NULL-ness with !skip_prefix() helps ;-)
I think you are referring to the interdiff to the previous patches..
Yes we should just use it as it is here.
>
>> + submodule_embed_git_dir_for_path(prefix, path);
>> +
>> + if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
>> + die("BUG: we don't know how to pass the flags down?");
>> +
>> + if (get_super_prefix())
>> + strbuf_addstr(&sb, get_super_prefix());
>> + strbuf_addstr(&sb, path);
>> + strbuf_addch(&sb, '/');
>> +
>> + cp.dir = path;
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
>> + "submodule--helper",
>> + "embed-git-dirs", NULL);
>> + prepare_submodule_repo_env(&cp.env_array);
>> + if (run_command(&cp))
>> + die(_("could not recurse into submodule '%s'"), path);
>> + strbuf_release(&sb);
>> + }
>
> Hmph. We cannot use run_processes_parallel() thing here? Is its
> API too hard to use to be worth it?
The problem here is that we do not know about the names of
the nested submodules. We could do a "git -C path submodule--helper list"
and then use the run_processes_parallel for
git -C <submodule> embed-git-dir <nested-sub1>
git -C <submodule> embed-git-dir <nested-sub2>
git -C <submodule> embed-git-dir <nested-sub3>
etc. As a first approach I considered
git -C <submodule> embed-git-dir <no-pathspec>
to be fast enough as the functionality is not implemented is only
a filesystem-local rename() (fast regardless of directory size).
So if we want to make that relocate git dir aware of
non-atomic-cross-filesystem moves, we want to revisit the decision
to run it in parallel?
>
>> +test_expect_success 'embedding does not fail for deinitalized submodules' '
>> + test_when_finished "git submodule update --init" &&
>> + git submodule deinit --all &&
>> + git submodule embedgitdirs &&
>> + test -d .git/modules/sub1 &&
>> + ! test -f sub1/.git &&
>
> Does this expect "sub1/.git is not a regular file (we want directory
> instead)"? Or "there is no filesystem entity at sub1/.git"?
This mainly ought to test that the new call doesn't crash or segfault
but accepts this as a valid state.
>
> If the former, write "test -d sub1/.git"; if the latter, you
> probably want "! test -e sub1/.git" instead.
However the -e is the correct thing to do.
^ permalink raw reply
* Re: [PATCHv5 0/5] submodule embedgitdirs
From: Junio C Hamano @ 2016-12-07 23:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, pclouds
In-Reply-To: <xmqqzik7v04b.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> v5:
>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>> moving a git dir of one repository. The submodule specific stuff (e.g.
>> recursion into nested submodules) is in submodule.{c,h}
>>
>> This was motivated by reviews on the series of checkout aware of submodules
>> building on top of this series, as we want to directly call the embed-git-dirs
>> function without the overhead of spawning a child process.
>
> OK. Comparing the last steps between this round and the previous
> one, I do think the separation of the responsibility among helpers
> is much more reasonable in this version, where:
>
> - submodule_embed_git_dir() is given a single path and is
> responsible for that submodule itself, which is done by calling
> submodule_embed_git_dir_for_path() on itself, and its
> sub-submodules, which is done by spawning the helper recursively
> with appropriate super-prefix;
>
> - submodule_embed_git_dir_for_path() computes where the given path
> needs to be moved to using the knowledge specific to the
> submodule subsystem, and asks relocate_gitdir() to perform the
> actual relocation;
>
> - relocate_gitdir() used to do quite a lot more, but now it is only
> about moving an existing .git directory elsewhere and pointing to
> the new location with .git file placed in the old location.
>
> I would have called the second helper submodule_embed_one_git_dir(),
> but that is a minor detail.
>
> Very nicely done.
One thing that is not so nice from the code organization point of
view is that "dir.c" now needs to include "submodule.h" because it
wants to call connect_work_tree_and_git_dir().
I wonder if that function belongs to dir.c or worktree.c not
submodule.c; I do not see anything that is submodule specific about
what the function does.
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Brandon Williams @ 2016-12-07 23:27 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8ChJ_H3gDOuKVYGAKYumG0u2WkBVpNr_3ePyAJ9NojvEg@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > A few small changes to improve readability. This is done by grouping related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > pathspec.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
> > die(_("%s: 'literal' and 'glob' are incompatible"), elt);
> >
> > + /* Create match string which will be used for pathspec matching */
> > if (pathspec_prefix >= 0) {
> > match = xstrdup(copyfrom);
> > prefixlen = pathspec_prefix;
> > @@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > match = xstrdup(copyfrom);
> > prefixlen = 0;
> > } else {
> > - match = prefix_path_gently(prefix, prefixlen, &prefixlen, copyfrom);
> > + match = prefix_path_gently(prefix, prefixlen,
> > + &prefixlen, copyfrom);
> > if (!match)
> > die(_("%s: '%s' is outside repository"), elt, copyfrom);
> > }
> > +
> > item->match = match;
> > + item->len = strlen(item->match);
> > + item->prefix = prefixlen;
> > +
> > /*
> > * Prefix the pathspec (keep all magic) and assign to
> > * original. Useful for passing to another command.
> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > } else {
> > item->original = xstrdup(elt);
> > }
> > - item->len = strlen(item->match);
> > - item->prefix = prefixlen;
> >
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> > strip_submodule_slash_cheap(item);
> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> > strip_submodule_slash_expensive(item);
> >
> > - if (magic & PATHSPEC_LITERAL)
> > + if (magic & PATHSPEC_LITERAL) {
> > item->nowildcard_len = item->len;
> > - else {
> > + } else {
> > item->nowildcard_len = simple_length(item->match);
> > if (item->nowildcard_len < prefixlen)
> > item->nowildcard_len = prefixlen;
> > }
> > +
> > item->flags = 0;
>
> You probably can move this line up with the others too.
I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags. I made more
sense to keep them grouped.
--
Brandon Williams
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Duy Nguyen @ 2016-12-07 23:21 UTC (permalink / raw)
To: Stephan Beyer
Cc: Junio C Hamano, Git Mailing List, Christian Couder,
SZEDER Gábor
In-Reply-To: <8d32c645-e850-5e7c-b01c-6e4d81e2d672@gmx.net>
On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 12/07/2016 09:04 PM, Junio C Hamano wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
Sorry I didn't know about --quit (and it has been there since 2011, I
guess I'm just not big sequencer user).
> Oh. ;) I am not sure. I personally think that --forget is a better name
Yeah, I was stuck with the name --destroy for many months and was very
happy the day I found --forget, which does not imply any destructive
side effects and is distinct enough from --abort to not confuse
people.
> than --quit because when I hear --quit I tend to look into the manual
> page first to check if there are weird side effects (and then the manual
> page says that it "forgets" ;D).
> So I'd rather favor adding --forget to cherry-pick/revert instead... or
> this:
--
Duy
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Junio C Hamano @ 2016-12-07 23:03 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, pclouds
In-Reply-To: <20161207210157.18932-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
> {"resolve-relative-url", resolve_relative_url, 0},
> {"resolve-relative-url-test", resolve_relative_url_test, 0},
> {"init", module_init, 0},
> - {"remote-branch", resolve_remote_submodule_branch, 0}
> + {"remote-branch", resolve_remote_submodule_branch, 0},
> + {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
> };
If you want to avoid patch noise like this, your 2/5 can add a
trailing comma after the entry for remote-branch. It is OK to end
elements in an array literal with trailing comma, even though we
avoid doing similar in enum definition (which is only allowed in
newer C standards).
> diff --git a/dir.c b/dir.c
> index bfa8c8a9a5..e023b04407 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,
> {
> untracked_cache_invalidate_path(istate, path);
> }
> +
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> + const char *new_git_dir, const char *displaypath,
> + struct strbuf *err)
> +{
> + int ret = 0;
> +
> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> + displaypath, old_git_dir, new_git_dir);
By using "strbuf err", it looks like that the calling convention of
this function wants to cater to callers who want to have tight
control over their output and an unconditional printing to the
standard output looks somewhat out of place.
Besides, does it belong to the standard output? It looks more like
a progress-bar eye candy that we send out to the standard error
stream (and only when we are talking to a tty).
> +/* Embeds a single submodule, non recursively. */
> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)
> +{
> + struct worktree **worktrees;
> + struct strbuf pathbuf = STRBUF_INIT;
> + struct strbuf errbuf = STRBUF_INIT;
> + char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
> + const char *new_git_dir;
> + const struct submodule *sub;
> + int code;
> +
> + worktrees = get_submodule_worktrees(path, 0);
> + if (worktrees) {
> + int i;
> + for (i = 0; worktrees[i]; i++)
> + ;
> + free_worktrees(worktrees);
> + if (i > 1)
> + die(_("relocate_gitdir for submodule '%s' with "
> + "more than one worktree not supported"), path);
> + }
We may benefit from "does this have secondary worktrees?" boolean
helper function, perhaps?
> + old_git_dir = xstrfmt("%s/.git", path);
> + if (read_gitfile(old_git_dir))
> + /* If it is an actual gitfile, it doesn't need migration. */
> + return;
Isn't this "no-op return" a valid thing to do, even when the
submodule has secondary worktrees that borrow from it? I am
wondering if the "ah, we don't do a repository that has secondary
worktrees" check should come after this one.
> + real_old_git_dir = xstrdup(real_path(old_git_dir));
> +...
> +}
> +/*
> + * Migrate the git directory of the submodule given by path from
> + * having its git directory within the working tree to the git dir nested
> + * in its superprojects git dir under modules/.
> + */
I think that this operation removes the git directories that are
embedded in the working tree of the superproject and storing them
away to safer place, i.e. unembedding.
> +int submodule_embed_git_dir(const char *prefix,
> + const char *path,
> + unsigned flags)
> +{
> + const char *sub_git_dir, *v;
> + char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
> + struct strbuf gitdir = STRBUF_INIT;
> +
> +
Lose the extra blank line here?
> + strbuf_addf(&gitdir, "%s/.git", path);
> + sub_git_dir = resolve_gitdir(gitdir.buf);
> +
> + /* Not populated? */
> + if (!sub_git_dir)
> + goto out;
> +
> + /* Is it already embedded? */
> + real_sub_git_dir = xstrdup(real_path(sub_git_dir));
> + real_common_git_dir = xstrdup(real_path(get_git_common_dir()));
> + if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
Yeah, checking for NULL-ness with !skip_prefix() helps ;-)
> + submodule_embed_git_dir_for_path(prefix, path);
> +
> + if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
> + die("BUG: we don't know how to pass the flags down?");
> +
> + if (get_super_prefix())
> + strbuf_addstr(&sb, get_super_prefix());
> + strbuf_addstr(&sb, path);
> + strbuf_addch(&sb, '/');
> +
> + cp.dir = path;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
> + "submodule--helper",
> + "embed-git-dirs", NULL);
> + prepare_submodule_repo_env(&cp.env_array);
> + if (run_command(&cp))
> + die(_("could not recurse into submodule '%s'"), path);
> + strbuf_release(&sb);
> + }
Hmph. We cannot use run_processes_parallel() thing here? Is its
API too hard to use to be worth it?
> +test_expect_success 'embedding does not fail for deinitalized submodules' '
> + test_when_finished "git submodule update --init" &&
> + git submodule deinit --all &&
> + git submodule embedgitdirs &&
> + test -d .git/modules/sub1 &&
> + ! test -f sub1/.git &&
Does this expect "sub1/.git is not a regular file (we want directory
instead)"? Or "there is no filesystem entity at sub1/.git"?
If the former, write "test -d sub1/.git"; if the latter, you
probably want "! test -e sub1/.git" instead.
^ permalink raw reply
* Re: [PATCHv5 4/5] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-07 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqvauvuzna.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> + submodule_common_dir = strbuf_detach(&sb, NULL);
>> + ret = get_worktrees_internal(submodule_common_dir, flags);
>> +
>> + free(submodule_gitdir);
>
> This sequence felt somewhat unusual. I would have written this
> without an extra variable, i.e.
>
> ret = get_worktrees_internal(sb.buf, flags);
> strbuf_release(&sb);
Yours is cleaner; I don't remember what I was thinking.
Feel free to squash it in; in case a resend is needed I will do that.
Thanks,
Stefan
^ permalink raw reply
* [PATCH 2/2] mailmap: Update my e-mail address
From: vi0oss @ 2016-12-07 22:49 UTC (permalink / raw)
To: git; +Cc: stefanbeller, Vitaly "_Vi" Shukela
In-Reply-To: <20161207224948.7957-1-vi0oss@gmail.com>
From: "Vitaly \"_Vi\" Shukela" <vi0oss@gmail.com>
Signed-off-by: Vitaly "_Vi" Shukela <vi0oss@gmail.com>
---
.mailmap | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.mailmap b/.mailmap
index 9cc33e9..b7ae81a 100644
--- a/.mailmap
+++ b/.mailmap
@@ -246,7 +246,7 @@ Uwe Kleine-König <u.kleine-koenig@pengutronix.de> <ukleinek@informatik.uni-frei
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> <uzeisberger@io.fsforth.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> <zeisberg@informatik.uni-freiburg.de>
Ville Skyttä <ville.skytta@iki.fi> <scop@xemacs.org>
-Vitaly "_Vi" Shukela <public_vi@tut.by>
+Vitaly "_Vi" Shukela <vi0oss@gmail.com> <public_vi@tut.by>
W. Trevor King <wking@tremily.us> <wking@drexel.edu>
William Pursell <bill.pursell@gmail.com>
YONETANI Tomokazu <y0n3t4n1@gmail.com> <qhwt+git@les.ath.cx>
--
2.10.2
^ permalink raw reply related
* [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-07 22:49 UTC (permalink / raw)
To: git; +Cc: stefanbeller, Vitaly "_Vi" Shukela
From: "Vitaly \"_Vi\" Shukela" <vi0oss@gmail.com>
In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced. It did not address _nested_
submodules, however.
This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.
As submodule's alternate target does not end in .git/objects
(rather .git/modules/qqqqqq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".
New tests have been added to t7408-submodule-reference.
Signed-off-by: Vitaly _Vi Shukela <vi0oss@gmail.com>
---
Notes:
Resolved issues pointed by Stefan Beller except of
the one about loosened path check, which he aggreed
to be relaxed for this case.
builtin/submodule--helper.c | 21 ++++++++++--
t/t7408-submodule-reference.sh | 72 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..7b7633d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
/*
* If the alternate object store is another repository, try the
- * standard layout with .git/modules/<name>/objects
+ * standard layout with .git/(modules/<name>)+/objects
*/
- if (ends_with(alt->path, ".git/objects")) {
+ if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+ char *sm_alternate = NULL, *error_strategy = NULL;
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", &prefix,
@@ -672,6 +673,22 @@ static int module_clone(int argc, const char **argv, const char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
relative_path(path, sm_gitdir, &rel_path));
+
+ /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+ git_config_get_string("submodule.alternateLocation", &sm_alternate);
+ if (sm_alternate) {
+ git_config_set_in_file(p, "submodule.alternateLocation",
+ sm_alternate);
+ }
+ git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+ if (error_strategy) {
+ git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+ error_strategy);
+ }
+
+ free(sm_alternate);
+ free(error_strategy);
+
strbuf_release(&sb);
strbuf_release(&rel_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..ef7771b 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
)
'
+test_expect_success 'preparing second superproject with a nested submodule' '
+ test_create_repo supersuper &&
+ (
+ cd supersuper &&
+ echo "I am super super." >file &&
+ git add file &&
+ git commit -m B-super-super-initial
+ git submodule add "file://$base_dir/super" subwithsub &&
+ git commit -m B-super-super-added &&
+ git submodule update --init --recursive &&
+ git repack -ad
+ )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference supersuper supersuper supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ # test superproject has alternates setup correctly
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
+ # nested submodule also has alternate:
+ test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ )
+'
+
+test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
+ test_when_finished "rm -rf supersuper-clone supersuper2" &&
+ git clone supersuper supersuper2 &&
+ (
+ cd supersuper2 &&
+ git submodule update --init
+ ) &&
+ test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ # test superproject has alternates setup correctly
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # update of the submodule fails
+ test_must_fail git submodule update --init --recursive &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
+ # but nested submodule has no alternate:
+ test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ )
+'
+
+test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
+ test_when_finished "rm -rf supersuper-clone supersuper2" &&
+ git clone supersuper supersuper2 &&
+ (
+ cd supersuper2 &&
+ git submodule update --init
+ ) &&
+ git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ # test superproject has alternates setup correctly
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # update of the submodule fails
+ test_must_fail git submodule update --init --recursive &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
+ # but nested submodule has no alternate:
+ test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ )
+'
+
test_done
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 03/17] dir: convert fill_directory to use the pathspec struct interface
From: Brandon Williams @ 2016-12-07 22:46 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8BsGVrUoFFFEqdLS-h4XYCkFg-gbx+BeWmGd8srupNWqw@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > 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 | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/dir.c b/dir.c
> > index 7df292b..8730a4f 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
> > len = common_prefix_len(pathspec);
> >
> > /* Read the directory and prune it */
> > - read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, pathspec);
> > + read_directory(dir, pathspec->nr ? pathspec->items[0].match : "",
> > + len, pathspec);
>
> Or even better, use common_prefix()'s return value here. I took me a
> while to realize this code was not buggy. It is fine to just pick the
> first item because the first <len> characters of _all_ pathspec items
> must be the same. Something like this
>
> prefix = common_prefix(..)
> read_directory(..., prefix, strlen(prefix), pathspec);
>
> expresses it much better. Yeah one extra mem allocation, no big deal
> since fill_directory() is not called very often.
I didn't even notice that. Now looking at this you're right that its
not immediately obvious that what's there is correct. I'll change this.
>
> > return len;
> > }
> >
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> --
> Duy
--
Brandon Williams
^ permalink raw reply
* Re: [PATCHv5 4/5] worktree: get worktrees from submodules
From: Junio C Hamano @ 2016-12-07 22:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, pclouds
In-Reply-To: <20161207210157.18932-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> + submodule_common_dir = strbuf_detach(&sb, NULL);
> + ret = get_worktrees_internal(submodule_common_dir, flags);
> +
> + free(submodule_gitdir);
This sequence felt somewhat unusual. I would have written this
without an extra variable, i.e.
ret = get_worktrees_internal(sb.buf, flags);
strbuf_release(&sb);
^ permalink raw reply
* Re: [PATCH 04/17] ls-tree: convert show_recursive to use the pathspec struct interface
From: Brandon Williams @ 2016-12-07 22:43 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8DELy5JsJmcyDtwT-O9qGa9+hR1UfcKWRY1cmCnTALixA@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > Convert 'show_recursive()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec struct.
>
> Slightly off-topic (sorry, but you made me look at this code! :D),
> could you update the magic_mask argument of parse_pathspec() in this
> file to PATHSPEC_ALL_MAGIC & ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL)?
> It makes sure all future magic will be caught as unsupported (and I
> think Stefan is adding one, but understandably he did not find this
> code).
>
> I think it's in the spirit of renaming _raw to match too. By limiting
> magic to fromtop and literal, we are sure match can only be path and
> nothing else, which is good because this show_recursive can't handle
> anything else either.
Can do.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2016-12-07 22:41 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8CKY5wb+0KzpincHLiYwC0za9Wexo9QvNedcQTGc+ZRDw@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > @@ -413,10 +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,
> > + item[i].magic = prefix_pathspec(item + i,
> > flags,
> > prefix, prefixlen, entry);
>
> The final output looks a bit ...um.. strangely tall, with the first
> two lines that have one argument each, then the last line comes with
> three arguments. Maybe put 'flags' in the same line as 'item + i'?
Yep you're right, it does look a bit funny.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic
From: Brandon Williams @ 2016-12-07 22:41 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Du5XUzcKpWfSKy8iknF01C-RWCgPeh73W3E0VkPJ9sog@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > @@ -426,8 +423,7 @@ void parse_pathspec(struct pathspec *pathspec,
> > nr_exclude++;
> > if (item[i].magic & magic_mask)
> > unsupported_magic(entry,
> > - item[i].magic & magic_mask,
> > - short_magic);
> > + item[i].magic & magic_mask);
>
> Same here. Maybe put both arguments in the same line. It looks a bit
> better. (sorry for two mails on the same patch, I'm reading the final
> output first before going through individual patches that breaks this
> function down)
All good. Sometimes its easier to parse comments if they are in
multiple small emails. I don't mind getting lots of mail :)
>
> >
> > if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
> > has_symlink_leading_path(item[i].match, item[i].len)) {
> --
> Duy
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 11/17] pathspec: factor global magic into its own function
From: Brandon Williams @ 2016-12-07 22:39 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8D9SAOYtzPTuGnst3J7qCUjMuGMrZ=KNH0MLSxMrq4krw@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > Create helper functions to read the global magic environment variables
> > in additon to factoring out the global magic gathering logic into its
> > own function.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > pathspec.c | 120 +++++++++++++++++++++++++++++++++++++------------------------
> > 1 file changed, 74 insertions(+), 46 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 5afebd3..08e76f6 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -87,6 +87,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> > strbuf_addf(sb, ",prefix:%d)", prefixlen);
> > }
> >
> > +static inline int get_literal_global(void)
> > +{
> > + static int literal_global = -1;
> > +
> > + if (literal_global < 0)
> > + literal_global = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,
> > + 0);
>
> These zeros look so lonely. I know it would exceed 80 columns if we
> put it on the previous line. But I think it's ok for occasional
> exceptions. Or you could rename noglob_global to noglob.
I was thinking the same thing but was so torn between the char limit. I
think it's probably ok to rename these vars by drooping the global since
the function name themselves indicate they are global.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Brandon Williams @ 2016-12-07 22:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8B6Mj-L1t-CETY5DWRyABHZsYZszwXD3dgUqChfXRB6FA@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > A few small changes to improve readability. This is done by grouping related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > pathspec.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>
> btw, since this function has stopped being "just prefix pathspec" for
> a long time, perhaps rename it to parse_pathspec_item, or something.
I was thinking about doing that after I sent this out. Glad you also
pointed that out.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCHv5 0/5] submodule embedgitdirs
From: Junio C Hamano @ 2016-12-07 22:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, pclouds
In-Reply-To: <20161207210157.18932-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> v5:
> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
> moving a git dir of one repository. The submodule specific stuff (e.g.
> recursion into nested submodules) is in submodule.{c,h}
>
> This was motivated by reviews on the series of checkout aware of submodules
> building on top of this series, as we want to directly call the embed-git-dirs
> function without the overhead of spawning a child process.
OK. Comparing the last steps between this round and the previous
one, I do think the separation of the responsibility among helpers
is much more reasonable in this version, where:
- submodule_embed_git_dir() is given a single path and is
responsible for that submodule itself, which is done by calling
submodule_embed_git_dir_for_path() on itself, and its
sub-submodules, which is done by spawning the helper recursively
with appropriate super-prefix;
- submodule_embed_git_dir_for_path() computes where the given path
needs to be moved to using the knowledge specific to the
submodule subsystem, and asks relocate_gitdir() to perform the
actual relocation;
- relocate_gitdir() used to do quite a lot more, but now it is only
about moving an existing .git directory elsewhere and pointing to
the new location with .git file placed in the old location.
I would have called the second helper submodule_embed_one_git_dir(),
but that is a minor detail.
Very nicely done.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-07 22:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, sbeller, peff, jacob.keller
In-Reply-To: <7d968fd8-a92d-efd3-ce67-7de6049b6d56@kdbg.org>
On 12/07, Johannes Sixt wrote:
> Am 07.12.2016 um 01:10 schrieb Brandon Williams:
> >This function should accept both absolute and relative paths, which
> >means it should probably accept "C:\My Files". I wasn't thinking about
> >windows 100% of the time while writing this so I'm hoping that a windows
> >expert will point things like this out to me :).
>
> ;)
>
> With this patch, the test suite fails at the very first git init call:
>
> D:\Src\mingw-git\t>sh t0000-basic.sh -v -i -x
> fatal: Invalid path '/:': No such file or directory
> error: cannot run git init -- have you built things yet?
> FATAL: Unexpected exit with code 1
>
> I haven't dug further, yet.
>
> -- Hannes
>
Thanks for providing me with the error. Instead of assuming root is "/"
I'll need to extract what root is from an absolute path. Aside from
what root looks like, do most other path constructs behave similarly in
unix and windows? (like ".." and "." as examples)
Since I don't really have a windows machine to test things it might be
slightly difficult to get everything correct quickly but hopefully we can
get this working :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-07 22:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Ramsay Jones, git, sbeller, peff,
jacob.keller
In-Reply-To: <xmqqtwafwkdt.fsf@gitster.mtv.corp.google.com>
On 12/07, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > But in any case it seems that e.g.
> > //SEFVER/SHARE/DIR1/DIR2/..
> > must be converted into
> > //SEFVER/SHARE/DIR1
> >
> > and
> > \\SEFVER\SHARE\DIR1\DIR2\..
> > must be converted into
> > \\SEFVER\SHARE\DIR1
>
> Additional questions that may be interesting are:
>
> //A/B/../C is it //A/C? is it an error?
> //A/B/../../C/D is it //C/D? is it an error?
>
Also is //.. the same as //? I would assume so since /.. is /
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-07 22:09 UTC (permalink / raw)
To: vi0oss; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <9493435c-16e5-d8af-9455-ec3f3cba390d@gmail.com>
On Wed, Dec 7, 2016 at 1:24 PM, vi0oss <vi0oss@gmail.com> wrote:
> On 12/07/2016 11:09 PM, Stefan Beller wrote:
>>>
>>> As submodule's alternate target does not end in .git/objects
>>> (rather .git/modules/qqqqqq/objects), this alternate target
>>> path restriction for in add_possible_reference_from_superproject
>>> relates from "*.git/objects" to just */objects".
>>
>> I wonder if this check is too weak and we actually have to check for
>> either .git/objects or modules/<name/possibly/having/slashes>/objects.
>> When writing the referenced commit I assumed we'd need a stronger check
>> to be safer and not add some random location as a possible alternate.
>>
> 1. Do we really need to check that it is named ".git"? Although
> "git clone --mirror --recursive" is not (directly) supported
> now, user may create one bare repository with [remnants of]
> submodules by converting reqular repository into bare one.
> Why not take advantage of additional information available locally
> in this case?
Oh, great point.
>
> 2. Is the check need to be strict because of we need to traverse
> one directory level up? Normally this "/objects" part is added by
> Git, so just one "../" seems to be OK. User can't specify "--reference
> somerepo/.git/objects", a strange reference can appear only if user
> manually creates alternates. Maybe better to document this case
> instead of restricting the feature?
Not sure I understand what needs better documentation here?
>
> 3. If nonetheless check for ".git/*/objects", then
> a. What functions should be used in Git codebase for such checks?
> b. Should we handle tricks like "smth/.git/../../objects" and so on?
I see we're getting into problems here.
>
> 4. Should we print (or print in verbose mode) each used alternate,
> to inform operator what his or her new clone will depend on?
>
> P.S. Actually I discovered the --recursive --reference feature when tried to
> put reference to a mega-repo with all possible submodules added as remotes.
> I expected --reference to just get though across all submodules, but it
> complained
> to missing "/modules/..." instead (the check went though becase the
> repository
> was named like "megarepo.git", so it did ended in ".git/objects").
Oh :(
With that said, I think the original patch is a sensible approach.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Junio C Hamano @ 2016-12-07 22:08 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller
In-Reply-To: <20161207023259.29355-2-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> ... Suppose that you version all
> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
> Now, you also have other tags which represent -rc releases and other
> such tags. If you want to find the first major release that contains
> a given commit you might try
>
> git describe --contains --match="v?.?" <commit>
>
> This will work as long as you have only single digits. But if you start
> adding multiple digits, the pattern becomes not enough to match all the
> tags you wanted while excluding the ones you didn't.
Isn't what you really want for the use case a negative pattern,
i.e. "I want ones that match v* but not the ones that match *-rc*",
though?
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Stefan Beller @ 2016-12-07 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Paul Tan
In-Reply-To: <xmqqmvg7wips.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 1:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So my first question I had to answer was if we do the right thing here,
>> i.e. if we could just fail instead. But we want to continue and just
>> not write back the index, which is fine.
>>
>> So we do not have to guard refresh_cache, but just call
>> update_index_if_able conditionally.
>
> An explanation with stepping back a little bit may help.
>
> You may be asked to visit a repository of a friend, to which you do
> not have write access to but you can still read. You may want to do
> "diff", "status" or "describe" there.
>
> In order to avoid getting fooled into thinking some paths are dirty
> only because the cached stat information does not match, these need
> to refresh the in-core index before doing their "comparison" to
> report which paths are different (in "diff"), what are the modified
> but not staged paths (in "status"), and if there is a need to add
> the "-dirty" suffix (in "describe").
>
> Since we are doing the expensive "bunch of lstat()" anyway, if we
> could write it back to the index, it would help future operations in
> the same repository--that is the reasoning behind the opportunistic
> updates. It is perfectly OK if we do not have write access to the
> repository and cannot write update the index.
>
Thanks for that explanation!
So I agree that we should not call it _if_needed, but the _if_able
part is still confusing, as we check for different parts here.
The case of not being able to write (read only) sounds similar to
the case of not being able to write (a concurrent lock),
I'll think about it more.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 00/17] pathspec cleanup
From: Junio C Hamano @ 2016-12-07 22:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <1481061106-117775-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> The intent of this series is to cleanup some of the pathspec initialization
> code as well as finally migrating the remaining users of the _raw field or
> get_pathspec() to the pathspec struct interface. This way both the _raw field
> and get_pathspec() can be removed from the codebase. This also removes the
> functionality where parse_pathspec() modified the const char * argv array that
> was passed in (which felt kind of odd to me as I wouldn't have expected the
> passed in array to be modified).
>
> I also noticed that there are memory leaks associated with the 'original' and
> 'match' strings. To fix this the pathspec struct needed to take ownership of
> the memory for these fields so that they can be cleaned up when clearing the
> pathspec struct.
Both good goals. Thanks for working on this.
^ permalink raw reply
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