* [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-08 0:39 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:
Another round of fixups:
* Corrected code style
* Refactored and fixed the test
Previously test contained errorneous
test_must_fail, which was masked by
missing &&.
Mailmap patch omitted this time.
builtin/submodule--helper.c | 19 ++++++++++--
t/t7408-submodule-reference.sh | 66 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..92fd676 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,20 @@ 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..9325297 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
)
'
+test_expect_success 'preparing second superproject with a nested submodule plus partial clone' '
+ 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
+ ) &&
+ git clone supersuper supersuper2 &&
+ (
+ cd supersuper2 &&
+ git submodule update --init
+ )
+'
+
+# 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
+ )
+'
+
+check_that_two_of_three_alternates_are_used() {
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # 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 fails clone and submodule update' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule fails
+ test_must_fail git submodule update --init --recursive
+ )
+'
+
+test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule succeeds
+ git submodule update --init --recursive
+ )
+'
+
test_done
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Brandon Williams @ 2016-12-08 0:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Bm+Nif_PL1=BzTYLKGrnz94rjKOB=_PXz6OB47Js6v2A@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 the 'internal_copy_pathspec()' function to use the pathspec
> > struct interface from using the deprecated 'get_pathspec()' interface.
> >
> > 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 prior to duplicating the result of parse_pathspec (which contains
> > each of the elements with the prefix prepended).
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 32 insertions(+), 13 deletions(-)
>
> Stefan did something similar last year [1] but I couldn't find why it
> did not get merged. Not sure if the reasons are still relevant or
> not...
>
> [1] http://public-inbox.org/git/%3C1438885632-26470-1-git-send-email-sbeller@google.com%3E/
>
> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 2f43877..4df4a12 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"
> > @@ -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.
>
> [1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)
>
> > }
> > - return get_pathspec(prefix, result);
> > +
> > + clear_pathspec(&ps);
> > + return result;
> > }
> >
> > static const char *add_slash(const char *path)
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>
>
>
> --
> Duy
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). I agree that this code should probably be rewritten and made a
bit cleaner, I don't know if that fits in the scope of this series or
should be done as a followup patch. If you think it fits here then I
can try and find some time to do the rework.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Junio C Hamano @ 2016-12-08 0:20 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CA+P7+xrPivwMzGhzKxu30jns+YvSQGXBKUc4JDmfbenTy27tZg@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> 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 am not suggesting anything. I was just trying to see how well what
was designed and implemented supports the use case that motivated
the feature. Think of it as a sanity check and review of the design.
> I think I can implement that pretty easily, and it should have simpler
> semantics. We can discard first, and then match what remains easily.
I actually think "multiple" and "negative" are orthogonal and both
are good things. If we are enhancing the filtering by refname
patterns to allow multiple patterns (i.e. your patch), that is good,
and it would be ideal if we can also have support for negative ones.
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 0:03 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8AX09pxkyUkLU905v1MpXocLzV5bK0APuNmMUNb50Lavg@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 '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?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 0:02 UTC (permalink / raw)
To: vi0oss; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161207224948.7957-1-vi0oss@gmail.com>
On Wed, Dec 7, 2016 at 2:49 PM, <vi0oss@gmail.com> wrote:
> Notes:
> Resolved issues pointed by Stefan Beller except of
> the one about loosened path check, which he aggreed
> to be relaxed for this case.
I am sorry to have given an incomplete review at the first time. :/
More below.
> + /* 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);
> + }
For a single command, we usually omit the braces for
an if clause, i.e.
if (foo)
bar(...);
/* code goes on */
> + 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);
> +
The indentation seems a bit off for the free here?
(The main nit that motivated me writing the email)
> 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
> + )
Both the first and the last part are the same in the last two tests,
the only difference is the line with git clone --reference ...
Maybe we can use a function somehow to make this a bit more
obvious?
Otherwise the tests look good to me.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Jacob Keller @ 2016-12-08 0:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161207153627.1468-1-Karthik.188@gmail.com>
On Wed, Dec 7, 2016 at 7:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
> v7 can be found here: http://marc.info/?l=git&m=147863593317362&w=2
>
> Changes in this version:
>
> 1. use an enum for holding the comparision type in
> %(if:[equals/notequals=...]) options.
> 2. rename the 'strip' option to 'lstrip' and introduce an 'rstrip'
> option. Also modify them to take negative values. This drops the
> ':dri' and ':base' options.
> 3. Drop unecessary code.
> 4. Cleanup code and fix spacing.
> 5. Add more comments wherever required.
> 6. Add quote_literal_for_format(const char *s) for safer string
> insertions in branch.c:build_format().
>
> Thanks to Jacob, Jackub, Junio and Matthieu for their inputs on the
> previous version.
>
> Interdiff below.
>
> Karthik Nayak (19):
> ref-filter: implement %(if), %(then), and %(else) atoms
> ref-filter: include reference to 'used_atom' within 'atom_value'
> ref-filter: implement %(if:equals=<string>) and
> %(if:notequals=<string>)
> ref-filter: modify "%(objectname:short)" to take length
> ref-filter: move get_head_description() from branch.c
> ref-filter: introduce format_ref_array_item()
> ref-filter: make %(upstream:track) prints "[gone]" for invalid
> upstreams
> ref-filter: add support for %(upstream:track,nobracket)
> ref-filter: make "%(symref)" atom work with the ':short' modifier
> ref-filter: introduce refname_atom_parser_internal()
> ref-filter: introduce refname_atom_parser()
> ref-filter: make remote_ref_atom_parser() use
> refname_atom_parser_internal()
> ref-filter: rename the 'strip' option to 'lstrip'
> ref-filter: modify the 'lstrip=<N>' option to work with negative '<N>'
> ref-filter: add an 'rstrip=<N>' option to atoms which deal with
> refnames
> ref-filter: allow porcelain to translate messages in the output
> branch, tag: use porcelain output
> branch: use ref-filter printing APIs
> branch: implement '--format' option
>
> Documentation/git-branch.txt | 7 +-
> Documentation/git-for-each-ref.txt | 86 +++++--
> builtin/branch.c | 290 +++++++---------------
> builtin/tag.c | 6 +-
> ref-filter.c | 488 +++++++++++++++++++++++++++++++------
> ref-filter.h | 7 +
> t/t3203-branch-output.sh | 16 +-
> t/t6040-tracking-info.sh | 2 +-
> t/t6300-for-each-ref.sh | 88 ++++++-
> t/t6302-for-each-ref-filter.sh | 94 +++++++
> 10 files changed, 784 insertions(+), 300 deletions(-)
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f4ad297..c72baeb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,13 +92,14 @@ refname::
> The name of the ref (the part after $GIT_DIR/).
> For a non-ambiguous short name of the ref append `:short`.
> The option core.warnAmbiguousRefs is used to select the strict
> - abbreviation mode. If `strip=<N>` is appended, strips `<N>`
> - slash-separated path components from the front of the refname
> - (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> - `<N>` must be a positive integer. If a displayed ref has fewer
> - components than `<N>`, the command aborts with an error. For the base
> - directory of the ref (i.e. foo in refs/foo/bar/boz) append
> - `:base`. For the entire directory path append `:dir`.
> + abbreviation mode. If `lstrip=<N>` or `rstrip=<N>` option can
Grammar here, drop the If before `lstrip since you're referring to
multiples and you say "x can be appended to y" rather than "if x is
added, do y"
> + be appended to strip `<N>` slash-separated path components
> + from or end of the refname respectively (e.g.,
> + `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`). if
> + `<N>` is a negative number, then only `<N>` path components
> + 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?
Thanks,
Jake
^ permalink raw reply
* 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
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