* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Christian Couder @ 2016-11-22 13:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CACsJy8BazeR=4tz3q2f35x=fCfp-Ld9LJz0mQh_CZoR_iXKEEQ@mail.gmail.com>
On Tue, Nov 22, 2016 at 11:35 AM, Duy Nguyen <pclouds@gmail.com> wrote:
[...]
>>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>>> do the right thing, just re-split the index when it makes sense", "no"
>>> is disabled of course. If the user wants to be specific, just write
>>> "10" or some other percentage.(and either 0 or 100 would mean enable
>>> split-index but do not re-split automatically, let _me_ do it when I
>>> want it)
>>
>> The meaning of a future "auto" option for "core.splitIndex" could be
>> "use the split-index feature only if the number of entries in whole
>> index is greater than 10000 (by default)".
>
> Well.. with the "just re-split the index when it makes sense" part,
> the user entrusts git to do something sensible in all cases,
That's an interpretation of what "core.splitIndex=true" could mean,
but there could be users who trust Git to re-split when it makes
sense, but who do want to use the split-index on all theirs repos even
the small ones or who just don't trust Git to choose when it might be
better to use it or not.
Yeah, a typical git user would most of the time just trust Git for all
those things, but on the other hand there are companies out there that
are willing to tweak many configuration options to get the better
possible behavior for them.
In fact I am working on this for Booking.com, and if we find out later
that we would gain something significant, like performance
improvements or configuration simplification, by adding "auto" and/or
other configuration variables to tweak more split-index related
things, we might very well post patch series to do that.
So if we now mix things up just to avoid one more configuration
option, we could very well make things harder to develop, to
configure, to parse and to understand later, so it is not a trade off
worth making.
> and going
> with absolute numbers might not be the best way, I think. It's big
> responsibility :)
About going with absolute number, yeah I am not sure at all it is the
best way, but this was just part of an example to try to explain what
I am saying above.
^ permalink raw reply
* [PATCH] merge-recursive.c: use QSORT macro
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 12:30 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Nguyễn Thái Ngọc Duy
This is the follow up of rs/qsort series, merged in b8688ad (Merge
branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
automatic transformation.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
coccinelle missed this place, understandably, because it can't know
that
sizeof(*entries->items)
is the same as
sizeof(*df_name_compare.items)
without some semantic analysis.
merge-recursive.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f..2d4dca9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -451,7 +451,7 @@ static void record_df_conflict_files(struct merge_options *o,
string_list_append(&df_sorted_entries, next->string)->util =
next->util;
}
- qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
+ QSORT(df_sorted_entries.items, entries->nr,
string_list_df_name_compare);
string_list_clear(&o->df_conflict_file_set, 1);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-11-22 10:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <20161110203428.30512-32-sbeller@google.com>
On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@google.com> wrote:
> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
Hmm.. common_prefix_len() has always been a bit relaxing and can cover
more than needed. It's for early pruning. Exact pathspec matching
_will_ be done later anyway.
Is that obvious? I'm wondering if we need to add a line or two in the
big comment code before this statement. I'm thinking it is and we
probably don't need more comments...
--
Duy
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Duy Nguyen @ 2016-11-22 10:35 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CAP8UFD1mun7wz2WqV8GCj6MODVjP2mPEBRGbJTf_ypiGCgtb9Q@mail.gmail.com>
On Fri, Nov 18, 2016 at 9:34 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> (sorry I got sick in the last few weeks and could not respond to this earlier)
>
> (Yeah, I have also been sick during the last few weeks.)
>
>> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>>>
>>>> Christian Couder <christian.couder@gmail.com> writes:
>>>>
>>>> > I think it is easier for user to be able to just set core.splitIndex
>>>> > to true to enable split-index.
>>>>
>>>> You can have that exact benefit by making core.splitIndex to
>>>> bool-or-more. If your default is 20%, take 'true' as if the user
>>>> specified 20% and take 'false' as if the user specified 100% (or is
>>>> it 0%? I do not care about the details but you get the point).
>>
>>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>>> default 20%, we will have to make it accept things like "auto,10".
>
> (Sorry for writing the above on my phone which added HTML, so that it
> didn't reach the list.)
>
>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>> do the right thing, just re-split the index when it makes sense", "no"
>> is disabled of course. If the user wants to be specific, just write
>> "10" or some other percentage.(and either 0 or 100 would mean enable
>> split-index but do not re-split automatically, let _me_ do it when I
>> want it)
>
> The meaning of a future "auto" option for "core.splitIndex" could be
> "use the split-index feature only if the number of entries in whole
> index is greater than 10000 (by default)".
Well.. with the "just re-split the index when it makes sense" part,
the user entrusts git to do something sensible in all cases, and going
with absolute numbers might not be the best way, I think. It's big
responsibility :)
> If there is no difference between "true" and "auto" then, when users
> who have "core.splitIndex=true" will migrate to the git version that
> adds the "auto" feature, their repos with under 10000 entires will not
> use the split-index feature anymore. These users may then be annoyed
> that the behavior has been switched under them, and that the
> split-index feature is not always used despite having
> "core.splitIndex=true" in their config.
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Duy Nguyen @ 2016-11-22 10:31 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Ralf Thielow, Taufiq Hoven
In-Reply-To: <9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de>
On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When 84c9dc2 (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17) extended the core.commentChar functionality to
> allow for the value 'auto', it forgot that rebase -i was already taught to
> handle core.commentChar,
I think this is 180bad3 (rebase -i: respect core.commentchar -
2013-02-11) and a couple followup fixes. My bad.
> and in turn forgot to let rebase -i handle that
> new value gracefully.
>
> Reported by Taufiq Hoven.
Another report in the same area: a merge conflict always writes the
"Conflicts:" section in the commit message with '#' as comment char
when core.commentChar is auto. I've known this for a while, but it has
not bitten me hard enough to do something about it,
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> git-rebase--interactive.sh | 13 +++++++++++--
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ca994c5..6bb740c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -93,8 +93,17 @@ eval '
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +''|auto)
> + comment_char=#
My first thought was "this kinda defeats the purpose of auto, doesn't
it". But 'auto' here is irrelevant because we control the leading
character of all lines in the todo file. 'auto' makes little sense in
this context, falling back to any comment char would do.
So, ack (after you fix the $(comment_char other people have mentioned,
of course).
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Duy Nguyen @ 2016-11-22 10:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Ralf Thielow, Taufiq Hoven
In-Reply-To: <5567548295222401fab10d3f2901c1787afbfd07.1479737858.git.johannes.schindelin@gmx.de>
On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> `stripspace` command to respect the config setting `core.commentChar`,
> it forgot that this variable may be defined in .git/config.
>
> So when rebasing interactively with a commentChar defined in the current
> repository's config, the help text at the bottom of the edit script
> potentially used an incorrect comment character. This was not only
> funny-looking, but also resulted in tons of warnings like this one:
>
> Warning: the command isn't recognized in the following line
> - #
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/stripspace.c | 4 +++-
> t/t0030-stripspace.sh | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716e..1e62a00 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> if (argc)
> usage_with_options(stripspace_usage, options);
>
> - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> + setup_git_directory_gently(NULL);
> git_config(git_default_config, NULL);
> + }
This conditional config file reading is a trap for similar bugs to
happen again. Is there any reason we should not just mark the command
RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?
>
> if (strbuf_read(&buf, 0, 1024) < 0)
> die_errno("could not read the input");
--
Duy
^ permalink raw reply
* [PATCH 3/3] worktree list: keep the list sorted
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index b835b91..80ccc51 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -434,6 +434,13 @@ static void measure_widths(struct worktree **wt, int *abbrev, int *maxlen)
}
}
+static int compare_worktree(const void *a_, const void *b_)
+{
+ const struct worktree *const *a = a_;
+ const struct worktree *const *b = b_;
+ return fspathcmp((*a)->path, (*b)->path);
+}
+
static int list(int ac, const char **av, const char *prefix)
{
int porcelain = 0;
@@ -448,11 +455,20 @@ static int list(int ac, const char **av, const char *prefix)
usage_with_options(worktree_usage, options);
else {
struct worktree **worktrees = get_worktrees();
- int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
+ int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i, nr;
if (!porcelain)
measure_widths(worktrees, &abbrev, &path_maxlen);
+ for (i = nr = 0; worktrees[i]; i++)
+ nr++;
+
+ /*
+ * don't sort the first item (main worktree), which will
+ * always be the first
+ */
+ QSORT(worktrees + 1, nr - 1, compare_worktree);
+
for (i = 0; worktrees[i]; i++) {
if (porcelain)
show_worktree_porcelain(worktrees[i]);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/3] get_worktrees() must return main worktree as first item even on error
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
This is required by git-worktree.txt, stating that the main worktree is
the first line (especially in --porcelain mode when we can't just change
behavior at will).
There's only one case when get_worktrees() may skip main worktree, when
parse_ref() fails. Update the code so that we keep first item as main
worktree and return something sensible in this case:
- In user-friendly mode, since we're not constraint by anything,
returning "(error)" should do the job (we already show "(detached
HEAD)" which is not machine-friendly). Actually errors should be
printed on stderr by parse_ref() (*)
- In plumbing mode, we do not show neither 'bare', 'detached' or
'branch ...', which is possible by the format description if I read
it right.
Careful readers may realize that when the local variable "head_ref" in
get_main_worktree() is emptied, add_head_info() will do nothing to
wt->head_sha1. But that's ok because head_sha1 is zero-ized in the
previous patch.
(*) Well, it does not. But it's supposed to be a stop gap
implementation until refs we can reuse code, to parse "ref: " stuff
in HEAD, from resolve_refs_unsafe(). Now may be the time since
refs refactoring is mostly done.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 8 +++++---
worktree.c | 6 ++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..b835b91 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
if (wt->is_detached)
printf("detached\n");
- else
+ else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
printf("\n");
@@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
else {
strbuf_addf(&sb, "%-*s ", abbrev_len,
find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
- if (!wt->is_detached)
+ if (wt->is_detached)
+ strbuf_addstr(&sb, "(detached HEAD)");
+ else if (wt->head_ref)
strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
else
- strbuf_addstr(&sb, "(detached HEAD)");
+ strbuf_addstr(&sb, "(error)");
}
printf("%s\n", sb.buf);
diff --git a/worktree.c b/worktree.c
index f7c1b5e..a674efa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
- goto done;
+ strbuf_reset(&head_ref);
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
worktree->is_detached = is_detached;
add_head_info(&head_ref, worktree);
-done:
strbuf_release(&path);
strbuf_release(&worktree_path);
strbuf_release(&head_ref);
@@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
list = xmalloc(alloc * sizeof(struct worktree *));
- if ((list[counter] = get_main_worktree()))
- counter++;
+ list[counter++] = get_main_worktree();
strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
This keeps things a bit simpler when we add more fields, knowing that
default values are always zero.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/worktree.c b/worktree.c
index f7869f8..f7c1b5e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
- worktree = xmalloc(sizeof(struct worktree));
+ worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
- worktree->id = NULL;
worktree->is_bare = is_bare;
- worktree->head_ref = NULL;
worktree->is_detached = is_detached;
- worktree->is_current = 0;
add_head_info(&head_ref, worktree);
- worktree->lock_reason = NULL;
- worktree->lock_reason_valid = 0;
done:
strbuf_release(&path);
@@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
- worktree = xmalloc(sizeof(struct worktree));
+ worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->id = xstrdup(id);
- worktree->is_bare = 0;
- worktree->head_ref = NULL;
worktree->is_detached = is_detached;
- worktree->is_current = 0;
add_head_info(&head_ref, worktree);
- worktree->lock_reason = NULL;
- worktree->lock_reason_valid = 0;
done:
strbuf_release(&path);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 0/3] Minor fixes on 'git worktree'
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
This fixes two things:
- make sure the first item is always the main worktree even if we
fail to retrieve some info
- keep 'worktree list' order stable (which in turn fixes the random
failure on my 'worktree-move' series
Nguyễn Thái Ngọc Duy (3):
worktree.c: zero new 'struct worktree' on allocation
get_worktrees() must return main worktree as first item even on error
worktree list: keep the list sorted
builtin/worktree.c | 26 ++++++++++++++++++++++----
worktree.c | 20 ++++----------------
2 files changed, 26 insertions(+), 20 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Junio C Hamano @ 2016-11-22 7:07 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So I guess we should test a bit more extensively, maybe
>
> git status >expect
> git submodule embedgitdirs
> git status >actual
> test_cmp expect actual
> # further testing via
> test -f ..
> test -d ..
Something along that line. "status should succeed" does not tell
the readers what kind of breakage the test is expecting to protect
us from. If we are expecting a breakage in embed-git-dirs would
somehow corrupt an existing submodule, which would lead to "status"
that is run in the superproject report the submodule differently,
then comparing output before and after the operation may be a
reasonable test. Going there to the submodule working tree and
checking the health of the repository (of the submodule) may be
another sensible test.
>> In the
>> extreme, if the failed "git submodule" command did
>>
>> rm -fr .git ?* && git init
>>
>> wouldn't "git status" still succeed?
>
> In that particular case you'd get
> $ git status
> fatal: Not a git repository (or any parent up to mount point ....)
Even with "&& git init"? Or you forgot that part?
^ permalink raw reply
* Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting
From: Junio C Hamano @ 2016-11-22 7:02 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79ka60_D8xfQyBegkXxkTGW5YDMuagB7kjhiCR6NriLR8+A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Can the effect of this change demonstrated in a new test? There
>>> must be a scenario where the current behaviour is broken and this
>>> change fixes an incorrect computation of relative path, no?
>
> I do not think the current usage exposes this bug in
> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
> which fills the second parameter `git_dir` via a call to read_gitfile,
> which itself produces an absolute path.
OK. Fixing a potential bug as a preparatory step is good.
> The current caller of connect_work_tree_and_git_dir passes
> an absolute path for the `git_dir` parameter. In the future patch
> we will also pass in relative path for `git_dir`. Extend the functionality
> of connect_work_tree_and_git_dir to take relative paths for parameters.
>
> We could work around this in the future patch by computing the absolute
> path for the git_dir in the calling site, however accepting relative
> paths for either parameter makes the API for this function easier
> to use.
Yup, sounds sensible. Thanks.
^ permalink raw reply
* Re: [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element
From: Junio C Hamano @ 2016-11-22 3:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, jacob.keller
In-Reply-To: <20161122013550.1800-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +Whenever a submodule configuration is parsed in `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
It will overwrite? It will be overwritten? I guess it is the latter?
> +So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
> +with the repository configuration, the null_sha1 entry contains the local
> +configuration of a submodule (e.g. consolidated values from local git
> configuration and the .gitmodules file in the worktree).
>
> For an example usage see test-submodule-config.c.
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Junio C Hamano @ 2016-11-22 3:40 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Looks good and I agree with Peff's analysis. Care to wrap it in a
patch with a log message?
Thanks.
>
> Thanks,
> Jonathan
>
> transport-helper.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
>
> code = start_command(helper);
> if (code < 0 && errno == ENOENT)
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Junio C Hamano @ 2016-11-22 3:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> It is also possible to pass in a tree hash to lookup a submodule config.
> Make it clear by naming the variables accrodingly. Looking up a submodule
> config by tree hash will come in handy in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Yeah, I noticed that while reading the previous round, too, but...
> -`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
> +`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
>
> The same as above but lookup by name.
Doesn't (the) "above", aka submodule_from_path() want the same
treatment?
Also the explanation of "above" has room for improvement. Namely it
says:
Lookup values for one submodule by its commit_sha1 and path.
I do not think the commit-sha1 (or commit-or-tree-object-name) is
"ITS" object name for the submodule. The name belongs to the
containing superproject commit (or tree), no?
Given a tree-ish in the superproject and a path, return the
submodule that is bound at the path in the named tree.
is what I would write for that one. Thinking about it a bit
further, "treeish_name" would be a much better name for the variable
than "commit_or_tree", as "treeish" is an established short and
sweet word that means "commit_or_tree", and having a "name"
somewhere in the variable name makes it clear that we are not
passing the pointer to an in-core object (e.g. "struct commit *").
> +test_expect_success 'using tree sha1 works' '
> + (
> + cd super &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + commit=$(git rev-parse HEAD^{commit}) &&
> + test-submodule-config $commit b >expect &&
> + test-submodule-config $tree b >actual &&
> + test_cmp expect actual
> + )
> +'
Perhaps also test a tag that points at the commit?
^ permalink raw reply
* Re: [PATCHv2 1/3] submodule config: inline config_from_{name, path}
From: Junio C Hamano @ 2016-11-22 3:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> There is no other user of config_from_{name, path}, such that there is no
> reason for the existence of these one liner functions. Just inline these
> to increase readability.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Makes sense.
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-11-22 2:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > - if (!git_dir)
> > + if (!git_dir) {
> > + if (!startup_info->have_repository)
> > + die("BUG: setup_git_env called without repository");
> > git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> > + }
>
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Yeah, I think making it conditional makes the most sense. Just trying to
think of cases that might not be covered by your patch:
- if we are not in a repository, we shouldn't ever need to override an
existing $GIT_DIR from the environment. Because if $GIT_DIR is
present, then we _would_ be in a repo if it's valid, and die if it
isn't.
- not setting $GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So worst case it may expend a little
effort to try to find a repository and fail (I think remote-curl
would do this to try to find repo-level config).
Both of those points assume that we've already called
setup_git_directory_gently(), but I think that's a reasonable
assumption. And certainly is true for ls-remote, and should be for any
git command that invokes the transport code.
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
So yeah, I think this is the extent of the change needed.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Stefan Beller @ 2016-11-22 2:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them. See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.
Will fix and test in a resend.
>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> + git init sub2 &&
>> + test_commit -C sub2 first &&
>> + git add sub2 &&
>> + git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> + test_must_fail git submodule interngitdirs &&
>> + # check that we did not break the repository:
>> + git status
>> +'
>
> It is not clear what the last "git status" wants to test.
Any errors that I ran into when manually truing to embed a submodules
git dir, were fatal with `git status` already, e.g. missing or wrong
call of connect_work_tree_and_git_dir were my main failure points.
So I guess we should test a bit more extensively, maybe
git status >expect
git submodule embedgitdirs
git status >actual
test_cmp expect actual
# further testing via
test -f ..
test -d ..
> In the
> extreme, if the failed "git submodule" command did
>
> rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?
In that particular case you'd get
$ git status
fatal: Not a git repository (or any parent up to mount point ....)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ echo $?
128
but I get the idea, which is why I propose the double check via status.
That would detect any logical change for the repository, e.g. a
change to the .gitmodules file.
>
> What are the minimum things that we expect from "did not break" to
> see? sub2/.git is still a directory and is a valid repository? The
> contents of the .git/modules/* before and after the "git submodule"
> does not change? Some other things?
I thought about making up a name for such a repo and creating that engry
in .gitmodules. But I refrained from doing so, because it seems too much
for this command.
I dunno, but I would suspect the double status is fine here, too?
^ permalink raw reply
* [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/api-submodule-config.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 768458580f..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -55,8 +55,11 @@ Functions
The same as above but lookup by name.
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
For an example usage see test-submodule-config.c.
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 1/3] submodule config: inline config_from_{name, path}
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule-config.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return submodule;
}
-static const struct submodule *config_from_path(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *path)
-{
- return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *name)
-{
- return config_from(cache, commit_sha1, name, lookup_name);
-}
-
static void ensure_cache_init(void)
{
if (is_cache_init)
@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
const char *name)
{
ensure_cache_init();
- return config_from_name(&the_submodule_cache, commit_sha1, name);
+ return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
}
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
{
ensure_cache_init();
- return config_from_path(&the_submodule_cache, commit_sha1, path);
+ return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
}
void submodule_free(void)
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
In-Reply-To: <20161122013550.1800-1-sbeller@google.com>
It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/technical/api-submodule-config.txt | 8 ++---
submodule-config.c | 46 ++++++++++++------------
submodule-config.h | 4 +--
t/t7411-submodule-config.sh | 11 ++++++
4 files changed, 40 insertions(+), 29 deletions(-)
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,15 @@ Functions
Can be passed to the config parsing infrastructure to parse
local (worktree) submodule configurations.
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
The same as above but lookup by name.
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
submodule will be returned (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, const char *arg)
return parse_push_recurse(opt, arg, 1);
}
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
const char *name, const char *option)
{
const char *commit_string = "WORKTREE";
- if (commit_sha1)
- commit_string = sha1_to_hex(commit_sha1);
+ if (commit_or_tree)
+ commit_string = sha1_to_hex(commit_or_tree);
warning("%s:.gitmodules, multiple configurations found for "
"'submodule.%s.%s'. Skipping second one!",
commit_string, name, option);
@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,
struct parse_config_parameter {
struct submodule_cache *cache;
- const unsigned char *commit_sha1;
+ const unsigned char *commit_or_tree;
const unsigned char *gitmodules_sha1;
int overwrite;
};
@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"path");
else {
if (submodule->path)
@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, void *data)
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"fetchrecursesubmodules");
else
submodule->fetch_recurse = parse_fetch_recurse(
@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
strcmp(value, "dirty") &&
@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, void *data)
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url) {
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"url");
} else {
free((void *) submodule->url);
@@ -351,21 +351,21 @@ static int parse_config(const char *var, const char *value, void *data)
ret = config_error_nonbool(var);
else if (!me->overwrite &&
submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"update");
else if (parse_submodule_update_strategy(value,
&submodule->update_strategy) < 0)
die(_("invalid value for %s"), var);
} else if (!strcmp(item.buf, "shallow")) {
if (!me->overwrite && submodule->recommend_shallow != -1)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"shallow");
else
submodule->recommend_shallow =
git_config_bool(var, value);
} else if (!strcmp(item.buf, "branch")) {
if (!me->overwrite && submodule->branch)
- warn_multiple_config(me->commit_sha1, submodule->name,
+ warn_multiple_config(me->commit_or_tree, submodule->name,
"branch");
else {
free((void *)submodule->branch);
@@ -379,18 +379,18 @@ static int parse_config(const char *var, const char *value, void *data)
return ret;
}
-static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+static int gitmodule_sha1_from_commit(const unsigned char *commit_or_tree,
unsigned char *gitmodules_sha1,
struct strbuf *rev)
{
int ret = 0;
- if (is_null_sha1(commit_sha1)) {
+ if (is_null_sha1(commit_or_tree)) {
hashclr(gitmodules_sha1);
return 1;
}
- strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+ strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_or_tree));
if (get_sha1(rev->buf, gitmodules_sha1) >= 0)
ret = 1;
@@ -402,7 +402,7 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
* revisions.
*/
static const struct submodule *config_from(struct submodule_cache *cache,
- const unsigned char *commit_sha1, const char *key,
+ const unsigned char *commit_or_tree, const char *key,
enum lookup_type lookup_type)
{
struct strbuf rev = STRBUF_INIT;
@@ -418,7 +418,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
* return the first submodule. Can be used to check whether
* there are any submodules parsed.
*/
- if (!commit_sha1 || !key) {
+ if (!commit_or_tree || !key) {
struct hashmap_iter iter;
struct submodule_entry *entry;
@@ -428,7 +428,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
return entry->config;
}
- if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))
+ if (!gitmodule_sha1_from_commit(commit_or_tree, sha1, &rev))
goto out;
switch (lookup_type) {
@@ -448,7 +448,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
/* fill the submodule config into the cache */
parameter.cache = cache;
- parameter.commit_sha1 = commit_sha1;
+ parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;
git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
@@ -484,7 +484,7 @@ int parse_submodule_config_option(const char *var, const char *value)
{
struct parse_config_parameter parameter;
parameter.cache = &the_submodule_cache;
- parameter.commit_sha1 = NULL;
+ parameter.commit_or_tree = NULL;
parameter.gitmodules_sha1 = null_sha1;
parameter.overwrite = 1;
@@ -492,18 +492,18 @@ int parse_submodule_config_option(const char *var, const char *value)
return parse_config(var, value, ¶meter);
}
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
+ return config_from(&the_submodule_cache, commit_or_tree, name, lookup_name);
}
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path)
{
ensure_cache_init();
- return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
+ return config_from(&the_submodule_cache, commit_or_tree, path, lookup_path);
}
void submodule_free(void)
diff --git a/submodule-config.h b/submodule-config.h
index d05c542d2c..99df8e593c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -25,9 +25,9 @@ struct submodule {
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
const char *path);
void submodule_free(void);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..301ed5e48f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -93,6 +93,17 @@ test_expect_success 'error message contains blob reference' '
)
'
+test_expect_success 'using tree sha1 works' '
+ (
+ cd super &&
+ tree=$(git rev-parse HEAD^{tree}) &&
+ commit=$(git rev-parse HEAD^{commit}) &&
+ test-submodule-config $commit b >expect &&
+ test-submodule-config $tree b >actual &&
+ test_cmp expect actual
+ )
+'
+
cat >super/expect_url <<EOF
Submodule url: 'git@somewhere.else.net:a.git' for path 'b'
Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* [PATCHv3 0/3] submodule-config: clarify/cleanup docs and header
From: Stefan Beller @ 2016-11-22 1:35 UTC (permalink / raw)
To: bmwill, gitster; +Cc: git, jacob.keller, Stefan Beller
replacing sb/submodule-config-cleanup
v3:
diff to current origin/sb/submodule-config-cleanup:
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,17 +49,17 @@ Functions
`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
The same as above but lookup by name.
Whenever a submodule configuration is parsed in `parse_submodule_config_option`
-via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
-zeroed out. So in the normal case, when HEAD:.gitmodules is parsed first and
-then overlayed with the repository configuration, the null_sha1 entry contains
-the local configuration of a submodule (e.g. consolidated values from local git
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
configuration and the .gitmodules file in the worktree).
For an example usage see test-submodule-config.c.
diff --git a/submodule-config.c b/submodule-config.c
index 4c5f5d074b..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -448,7 +448,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
/* fill the submodule config into the cache */
parameter.cache = cache;
- // todo: get the actual tree here:
parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;
v2:
addressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.
Thanks,
Stefan
interdiff to v1:
diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,7 +49,7 @@ Functions
`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
- Lookup values for one submodule by its commit_sha1 and path.
+ Lookup values for one submodule by its commit_or_tree and path.
`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
v1:
A small series that would have helped me understand the submodule config
once again.
Thanks,
Stefan
Stefan Beller (3):
submodule config: inline config_from_{name, path}
submodule-config: rename commit_sha1 to commit_or_tree
submodule-config: clarify parsing of null_sha1 element
Documentation/technical/api-submodule-config.txt | 13 ++++--
submodule-config.c | 58 ++++++++++--------------
submodule-config.h | 4 +-
t/t7411-submodule-config.sh | 11 +++++
4 files changed, 44 insertions(+), 42 deletions(-)
--
2.11.0.rc2.18.g0126045.dirty
^ permalink raw reply related
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Stefan Beller @ 2016-11-22 1:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Jacob Keller
In-Reply-To: <20161122001658.GE149321@google.com>
On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/21, Stefan Beller wrote:
>> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams <bmwill@google.com> wrote:
>> > On 11/21, Stefan Beller wrote:
>> >>
>> >> switch (lookup_type) {
>> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
>> >>
>> >> /* fill the submodule config into the cache */
>> >> parameter.cache = cache;
>> >> - parameter.commit_sha1 = commit_sha1;
>> >> + // todo: get the actual tree here:
>> >
>> > s/todo/TODO
>> >
>> > Makes it more clear that this is a TODO
>> >
>>
>> The // is more annoying here. I'll review these changes closely
>> before sending out v3.
>
> Well I prefer // to /* */ but that's not the style we use :)
background: Initially I assume we would need to do work here as that part
is exposed to the user in error messages, such that we maybe want to
go the reverse direction and not state a tree but instead the commit containing
it. Since then I decided that it is not worth to optimize for some
hypothetical future
and hence I did not go with the internal todo note I put there. Then I
forgot about
it and it just showed up in the patch here.
Having looked through the patch again, the rest looks clean to me.
Stefan
>
> --
> Brandon Williams
^ permalink raw reply
* Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C
From: Stephan Beyer @ 2016-11-22 0:49 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b29-65f79716-42c6-4327-acda-8c8d0fe05471-000000@eu-west-1.amazonses.com>
Okay Pranit,
this is the last patch for me to review in this series.
Now that I have a coarse overview of what you did, I have the general
remark that imho the "terms" variable should simply be global instead of
being passed around all the time.
I also had some other remarks but I forgot them... maybe they come to my
mind again when I see patch series v16.
I also want to remark again that I am not a Git developer and only
reviewed this because of my interest in git-bisect. So some of my
suggestions might conflict with other beliefs here. For example, I
consider it very bad style to leak memory... but Git is rather written
as a scripting tool than a genuine library, so perhaps many people here
do not care about it as long as it works...
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c18ca07..b367d8d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> terms->term_good = arg;
> } else if (!strcmp(arg, "--term-bad") ||
> !strcmp(arg, "--term-new")) {
> - const char *next_arg;
This should already have been removed in patch 15/27, not here.
> @@ -875,6 +875,117 @@ static int bisect_log(void)
> return status;
> }
>
> +static int get_next_word(const char *line, int pos, struct strbuf *word)
> +{
> + int i, len = strlen(line), begin = 0;
> + strbuf_reset(word);
> + for (i = pos; i < len; i++) {
> + if (line[i] == ' ' && begin)
> + return i + 1;
> +
> + if (!begin)
> + begin = 1;
> + strbuf_addch(word, line[i]);
> + }
> +
> + return i;
> +}
> +
> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
> +{
> + struct strbuf line = STRBUF_INIT;
> + struct strbuf word = STRBUF_INIT;
> + FILE *fp = NULL;
(The initialization is not necessary here.)
> + int res = 0;
> +
> + if (is_empty_or_missing_file(filename)) {
> + error(_("no such file with name '%s' exists"), filename);
The error message is misleading if the file exists but is empty.
Maybe something like "cannot read file '%s' for replaying"?
> + res = -1;
> + goto finish;
goto fail;
:D
> + }
> +
> + if (bisect_reset(NULL)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + fp = fopen(filename, "r");
> + if (!fp) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + while (strbuf_getline(&line, fp) != EOF) {
> + int pos = 0;
> + while (pos < line.len) {
> + pos = get_next_word(line.buf, pos, &word);
> +
> + if (!strcmp(word.buf, "git")) {
> + continue;
> + } else if (!strcmp(word.buf, "git-bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "#")) {
> + break;
Maybe it is more robust to check whether word.buf begins with #
> + }
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, word.buf)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> +
> + if (!strcmp(word.buf, "start")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + sq_dequote_to_argv_array(line.buf+pos, &argv);
> + if (bisect_start(terms, 0, argv.argv, argv.argc)) {
> + argv_array_clear(&argv);
> + res = -1;
> + goto finish;
goto fail;
> + }
> + argv_array_clear(&argv);
> + break;
> + }
> +
> + if (one_of(word.buf, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
> + res = -1;
> + goto finish;
goto fail;
> + }
> + break;
> + }
> +
> + if (!strcmp(word.buf, "terms")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + sq_dequote_to_argv_array(line.buf+pos, &argv);
> + if (bisect_terms(terms, argv.argv, argv.argc)) {
> + argv_array_clear(&argv);
> + res = -1;
> + goto finish;
goto fail;
> + }
> + argv_array_clear(&argv);
> + break;
> + }
> +
> + error(_("?? what are you talking about?"));
I know this string is taken from the original source. However, even
something like error(_("Replay file contains rubbish (\"%s\")"),
word.buf) is more informative.
> + res = -1;
> + goto finish;
goto fail;
> + }
> + }
> + goto finish;
+fail:
+ res = -1;
I just wanted to make finally clear what I was referring to by the
"goto fail" trick. :D
Also I think the readability could be improved by extracting the body of
the outer while loop into an extra function replay_line(). Then most of
my suggested "goto fail;" lines simply become "return -1;" :)
> @@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
> die(_("--bisect-log requires 0 arguments"));
> res = bisect_log();
> break;
> + case BISECT_REPLAY:
> + if (argc != 1)
> + die(_("--bisect-replay requires 1 argument"));
I'd keep the (already translated) string from the original source:
"No logfile given"
Cheers,
Stephan
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jonathan Nieder @ 2016-11-22 0:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen
In-Reply-To: <20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite. So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> environment.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
> const char *replace_ref_base;
>
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir)
> + if (!git_dir) {
> + if (!startup_info->have_repository)
> + die("BUG: setup_git_env called without repository");
> git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> + }
This trips reproducibly for
git ls-remote https://kernel.googlesource.com/pub/scm/git/git
when run outside of a git repository.
Backtrace:
#0 setup_git_env () at environment.c:172
#1 get_git_dir () at environment.c:214
#2 get_helper at transport-helper.c:127
#3 get_refs_list (for_push=0) at transport-helper.c:1038
#4 transport_get_remote_refs at transport.c:1076
#5 cmd_ls_remote at builtin/ls-remote.c:97
transport-helper.c:127 is
argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
get_git_dir());
That code is pretty clearly wrong. Should it be made conditional
on have_git_dir(), like this?
Thanks,
Jonathan
transport-helper.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..e4fd982 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
- argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
- get_git_dir());
+ if (have_git_dir())
+ argv_array_pushf(&helper->env_array, "%s=%s",
+ GIT_DIR_ENVIRONMENT, get_git_dir());
code = start_command(helper);
if (code < 0 && errno == ENOENT)
--
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox