* Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 18:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <xmqqmvfich2e.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> exit(0);
>> }
>>
>> + submodules_absorb_gitdir_if_needed(prefix);
>> +
>> /*
>> * If not forced, the file, the index and the HEAD (if exists)
>> * must match; but the file can already been removed, since
>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> oidclr(&oid);
>> if (check_local_mod(&oid, index_only))
>> exit(1);
>> - } else if (!index_only) {
>> - if (check_submodules_use_gitfiles())
>> - exit(1);
>> }
>>
>
> Hmph. It may be a bit strange to see an "index-only" remove to
> touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is
> unconditionally called above, which feels somewhat unexpected.
I agree. In a reroll I'll protect the call with
if (!index_only)
submodules_absorb_gitdir_if_needed(prefix);
>
>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> */
>> if (!index_only) {
>> int removed = 0, gitmodules_modified = 0;
>> for (i = 0; i < list.nr; i++) {
>> const char *path = list.entry[i].name;
>> if (list.entry[i].is_submodule) {
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + strbuf_addstr(&buf, path);
>> + if (remove_dir_recursively(&buf, 0))
>> + die(_("could not remove '%s'"), path);
>> + strbuf_release(&buf);
>> +
>> + removed = 1;
>> + if (!remove_path_from_gitmodules(path))
>> + gitmodules_modified = 1;
>> + continue;
>> }
>
> I do not see any behaviour change from the original (not quoted
> here), but it is somewhat surprising that "git rm ./submodule" does
> not really check if the submodule has local modifications and files
> that are not even added before remove_dir_recursively() is called.
>
> Or perhaps I am reading the code incorrectly and such a check is
> done elsewhere?
When determining if the entry is a submodule (i.e. setting
the is_submodule bit) we have
list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
if (list.entry[list.nr++].is_submodule &&
!is_staging_gitmodules_ok())
die (_("Please stage ..."));
I think for clarity we could drop the is_submodule bit and only
and directly do
if (S_ISGITLINK(ce->ce_mode) &&
!is_staging_gitmodules_ok())
die (_("Please stage ..."));
and below (the code that you quoted) also just check via
S_ISGITLINK(ce->ce_mode)
Thanks,
Stefan
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)
From: Michael Haggerty @ 2016-12-27 18:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1gdcmwn.fsf@gitster.mtv.corp.google.com>
On 12/27/2016 06:16 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
>> constant introduced by
>>
>> 3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
>>
>> sets that constant to the value 2,...
>
> Sorry I didn't notice this earlier, either. Thanks for spotting.
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Tue, 27 Dec 2016 09:12:09 -0800
> Subject: [PATCH] lockfile: move REPORT_ON_ERROR bit elsewhere
>
> There was LOCK_NO_DEREF defined as 2 = 1<<1 with the same value,
> which was missed due to a huge comment block. Deconflict by moving
> the new one to 4 = 1<<2 for now.
The fix is obviously correct. I'm not sure I like it that comments are
being blamed for mistakes :-P
Perhaps defining these constants within an `enum` would make it clearer
that they are a group and help prevent problems like this in the future
(though that could be done later rather than as part of this hot fix).
Michael
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> lockfile.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lockfile.h b/lockfile.h
> index 16775a7d79..7b715f9e77 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -137,7 +137,7 @@ struct lock_file {
> * ... this flag can be passed instead to return -1 and give the usual
> * error message upon an error.
> */
> -#define LOCK_REPORT_ON_ERROR 2
> +#define LOCK_REPORT_ON_ERROR 4
>
> /*
> * Usually symbolic links in the destination path are resolved. This
>
^ permalink raw reply
* Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <CAGZ79kaRY0x+31-UiiBU1iBXGKAgeTRSSjvN0isd7jdg-Y7_rQ@mail.gmail.com>
On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> exit(0);
>>> }
>>>
>>> + submodules_absorb_gitdir_if_needed(prefix);
>>> +
>>> /*
>>> * If not forced, the file, the index and the HEAD (if exists)
>>> * must match; but the file can already been removed, since
>>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> oidclr(&oid);
>>> if (check_local_mod(&oid, index_only))
>>> exit(1);
>>> - } else if (!index_only) {
>>> - if (check_submodules_use_gitfiles())
>>> - exit(1);
>>> }
>>>
>>
>> Hmph. It may be a bit strange to see an "index-only" remove to
>> touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is
>> unconditionally called above, which feels somewhat unexpected.
>
> I agree. In a reroll I'll protect the call with
>
> if (!index_only)
> submodules_absorb_gitdir_if_needed(prefix);
>
>>
>>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> */
>>> if (!index_only) {
>>> int removed = 0, gitmodules_modified = 0;
>>> for (i = 0; i < list.nr; i++) {
>>> const char *path = list.entry[i].name;
>>> if (list.entry[i].is_submodule) {
>>> + struct strbuf buf = STRBUF_INIT;
>>> +
>>> + strbuf_addstr(&buf, path);
>>> + if (remove_dir_recursively(&buf, 0))
>>> + die(_("could not remove '%s'"), path);
>>> + strbuf_release(&buf);
>>> +
>>> + removed = 1;
>>> + if (!remove_path_from_gitmodules(path))
>>> + gitmodules_modified = 1;
>>> + continue;
>>> }
>>
>> I do not see any behaviour change from the original (not quoted
>> here), but it is somewhat surprising that "git rm ./submodule" does
>> not really check if the submodule has local modifications and files
>> that are not even added before remove_dir_recursively() is called.
>>
>> Or perhaps I am reading the code incorrectly and such a check is
>> done elsewhere?
>
> When determining if the entry is a submodule (i.e. setting
> the is_submodule bit) we have
>
> list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
> if (list.entry[list.nr++].is_submodule &&
> !is_staging_gitmodules_ok())
> die (_("Please stage ..."));
>
Well scratch that.
is_staging_gitmodules_ok only checks for the .gitmodules file and not
for the submodule itself (the submodule is not an argument to that function)
The actual answer is found in check_local_mod called via
if (!force) {
struct object_id oid;
if (get_oid("HEAD", &oid))
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
}
check_local_mod was touched in the previous patch as it has:
/*
* Is the index different from the file in the work tree?
* If it's a submodule, is its work tree modified?
*/
if (ce_match_stat(ce, &st, 0) ||
(S_ISGITLINK(ce->ce_mode) &&
bad_to_remove_submodule(ce->name,
SUBMODULE_REMOVAL_DIE_ON_ERROR |
SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
^ permalink raw reply
* Re: Gitview Shell Injection Vulnerability
From: Stefan Beller @ 2016-12-27 18:45 UTC (permalink / raw)
To: Javantea, Junio C Hamano; +Cc: git@vger.kernel.org, aneesh.kumar
In-Reply-To: <20161227082922.8B7A813893D@mail.altsci.com>
+cc the author of gitview
On Tue, Dec 27, 2016 at 12:29 AM, Javantea <jvoss@altsci.com> wrote:
> I have found a shell injection vulnerability in contrib/gitview/gitview.
>
> Gitview Shell Injection Vulnerability
>
> Versions affected: 8cb711c8a5-1d1bdafd64 (<=2.11.0)
>
> Gitview executes shell commands using string concatenation with user supplied data, filenames and branch names. Running Gitview and interacting with the user interface with a malicious filename or branch name in the current repository results in malicious commands being executed as the current user.
>
> AnnotateWindow.add_file_data(self, filename, commit_sha1, line_num):
> fp = os.popen("git cat-file blob " + commit_sha1 +":"+filename)
>
> AnnotateWindow.annotate(self, filename, commit_sha1, line_num):
> fp = os.popen("git ls-tree "+ commit_sha1 + " -- " + filename)
> fp = os.popen("git blame --incremental -C -C -- " + filename + " " + commit_sha1)
>
> GitView.set_branch(self, args):
> fp = os.popen("git rev-parse --sq --default HEAD " + list_to_string(args, 1))
> fp = os.popen("git rev-list --header --topo-order --parents " + git_rev_list_cmd)
>
> The program also has other uses of os.popen but none use values that the user can manipulate. However, the fix should definitely replace these instances so that the code might one day pass pylint and manual code review easier.
>
> The function os.popen has been replaced by safer functions in the subprocess module. The code can be improved easily because it requires very little change to convert the code to work with arrays of strings instead of strings.
>
> If you have any questions or would like a patch, please let me know.
>
I guess you could send a patch to fix it. It is unclear to me
how the patch submission process for these work, though.
Please see contrib/README to see why it is unclear to me.
> I expect that things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature, either by becoming
> projects on their own, or moving to the toplevel directory. On
> the other hand, I expect I'll be proposing removal of disused
> and inactive ones from time to time.
Maybe it's time for a spring cleanup and remove some old (dead?)
projects from contrib?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 12/21] Documentation/config: add splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-12-27 19:09 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-13-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/config.txt | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 221c5982c0..e0f5a77980 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2773,6 +2773,19 @@ showbranch.default::
> The default set of branches for linkgit:git-show-branch[1].
> See linkgit:git-show-branch[1].
>
> +splitIndex.maxPercentChange::
> + When the split index feature is used, this specifies the
> + percent of entries the split index can contain compared to the
> + whole number of entries in both the split index and the shared
s/whole/total/ to match the last sentence of this section, perhaps?
"The number of all entries" would also be OK, but "the whole number
of entries" sounds a bit strange.
^ permalink raw reply
* Re: [PATCH v3 13/21] sha1_file: make check_and_freshen_file() non static
From: Junio C Hamano @ 2016-12-27 19:09 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-14-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> This function will be used in a commit soon, so let's make
> it available globally.
See comment on 14/21; I am not convinced that this is the function
you would want to borrow.
^ permalink raw reply
* [PATCHv6] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 19:03 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, 6t, Stefan Beller
When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.
Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.
An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future. But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.
The solution proposed here defers all these checks to the caller.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This replaces the last patch of the series
"git-rm absorbs submodule git directory before deletion".
named "rm: absorb a submodules git dir before deletion".
The only change is a guard (!index_only) for
submodules_absorb_gitdir_if_needed(prefix);
Junio, the other concern you raised about not checking
for local modifications in the submodule has been answered
as a reply there; we do check for local modifications there,
though it is not obvious.
Thanks,
Stefan
builtin/rm.c | 80 ++++++++++++++---------------------------------------------
t/t3600-rm.sh | 39 +++++++++++------------------
2 files changed, 34 insertions(+), 85 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..20635dca94 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
}
}
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
- print_error_files(files,
- Q_("the following submodule (or one of its nested "
- "submodules)\n"
- "uses a .git directory:",
- "the following submodules (or one of their nested "
- "submodules)\n"
- "use a .git directory:", files->nr),
- _("\n(use 'rm -rf' if you really want to remove "
- "it including all of its history)"),
- errs);
- string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
{
int i;
- int errs = 0;
- struct string_list files = STRING_LIST_INIT_NODUP;
-
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
- string_list_append(&files, name);
+ absorb_git_dir_into_superproject(prefix, name,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
}
-
- error_removing_concrete_submodules(&files, &errs);
-
- return errs;
}
static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
int errs = 0;
struct string_list files_staged = STRING_LIST_INIT_NODUP;
struct string_list files_cached = STRING_LIST_INIT_NODUP;
- struct string_list files_submodule = STRING_LIST_INIT_NODUP;
struct string_list files_local = STRING_LIST_INIT_NODUP;
no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)
else if (!index_only) {
if (staged_changes)
string_list_append(&files_cached, name);
- if (local_changes) {
- if (S_ISGITLINK(ce->ce_mode) &&
- !submodule_uses_gitfile(name))
- string_list_append(&files_submodule, name);
- else
- string_list_append(&files_local, name);
- }
+ if (local_changes)
+ string_list_append(&files_local, name);
}
}
print_error_files(&files_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)
&errs);
string_list_clear(&files_cached, 0);
- error_removing_concrete_submodules(&files_submodule, &errs);
-
print_error_files(&files_local,
Q_("the following file has local modifications:",
"the following files have local modifications:",
@@ -342,6 +313,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
exit(0);
}
+ if (!index_only)
+ submodules_absorb_gitdir_if_needed(prefix);
+
/*
* If not forced, the file, the index and the HEAD (if exists)
* must match; but the file can already been removed, since
@@ -358,9 +332,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
- } else if (!index_only) {
- if (check_submodules_use_gitfiles())
- exit(1);
}
/*
@@ -389,32 +360,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
*/
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
- struct strbuf buf = STRBUF_INIT;
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
- if (is_empty_dir(path)) {
- if (!rmdir(path)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- continue;
- }
- } else {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, path);
- if (!remove_dir_recursively(&buf, 0)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- strbuf_release(&buf);
- continue;
- } else if (!file_exists(path))
- /* Submodule was removed by user */
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- /* Fallthrough and let remove_path() fail. */
- }
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, path);
+ if (remove_dir_recursively(&buf, 0))
+ die(_("could not remove '%s'"), path);
+ strbuf_release(&buf);
+
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
@@ -423,7 +382,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
- strbuf_release(&buf);
if (gitmodules_modified)
stage_updated_gitmodules();
}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index bcbb680651..5aa6db584c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
test_cmp expect actual
'
-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '
+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
git checkout -f master &&
git reset --hard &&
git submodule update &&
(cd submod &&
rm .git &&
cp -R ../.git/modules/sub .git &&
- GIT_WORK_TREE=. git config --unset core.worktree
+ GIT_WORK_TREE=. git config --unset core.worktree &&
+ rm -r ../.git/modules/sub
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
cat >expect.deepmodified <<EOF
@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc
git submodule update --recursive &&
(cd submod/subsubmod &&
rm .git &&
- cp -R ../../.git/modules/sub/modules/sub .git &&
+ mv ../../.git/modules/sub/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/subsubmod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
test_expect_success 'checking out a commit after submodule removal needs manual updates' '
- git commit -m "submodule removal" submod &&
+ git commit -m "submodule removal" submod .gitmodules &&
git checkout HEAD^ &&
git submodule update &&
git checkout -q HEAD^ &&
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply related
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2016-12-27 19:10 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-15-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> +/*
> + * Signal that the shared index is used by updating its mtime.
> + *
> + * This way, shared index can be removed if they have not been used
> + * for some time. It's ok to fail to update the mtime if we are on a
> + * read only file system.
> + */
> +void freshen_shared_index(char *base_sha1_hex)
> +{
> + const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
> + check_and_freshen_file(shared_index, 1);
What happens when this call fails? The function returns 0 if the
file did not even exist. It also returns 0 if you cannot update its
timestamp.
Shouldn't the series be exposing freshen_file() instead _and_ taking
its error return value seriously?
> +}
> +
> int read_index_from(struct index_state *istate, const char *path)
> {
> struct split_index *split_index;
> @@ -2273,6 +2286,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> + } else {
> + freshen_shared_index(sha1_to_hex(si->base_sha1));
> }
>
> return write_split_index(istate, lock, flags);
^ permalink raw reply
* Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex
From: Junio C Hamano @ 2016-12-27 19:04 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-7-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> +test_expect_success 'set core.splitIndex config variable to true' '
> + git config core.splitIndex true &&
> + : >three &&
> + git update-index --add three &&
> + git ls-files --stage >ls-files.actual &&
> + cat >ls-files.expect <<EOF &&
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 one
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 three
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 two
> +EOF
> + test_cmp ls-files.expect ls-files.actual &&
It does not add much value to follow the "existing" outdated style
like this when you are only adding new tests. Write these like
cat >ls-files.expect <<-\EOF &&
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 one
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 three
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 two
EOF
which would give incentive to others (or yourself) to update the
style of the existing mess ;-).
^ permalink raw reply
* Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var
From: Junio C Hamano @ 2016-12-27 19:07 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-9-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/git-update-index.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index 7386c93162..e091b2a409 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -171,6 +171,12 @@ may not support it yet.
> given again, all changes in $GIT_DIR/index are pushed back to
> the shared index file. This mode is designed for very large
> indexes that take a significant amount of time to read or write.
> ++
> +These options take effect whatever the value of the `core.splitIndex`
> +configuration variable (see linkgit:git-config[1]).
Doesn't the "whatever..." clause in this sentence lack its verb
(presumably, "is", right after "variable")?
> +emitted when the change goes against the configured value, as the
> +configured value will take effect next time the index is read and this
> +will remove the intended effect of the option.
It feels somewhat strange to see a warning in this case.
If the user configures the system to usually do X, and issues a
command that explicitly does Y (or "not-X"), we do not warn for the
command invocation if it is a single-shot operation. That's the
usual "command line overrides configuration" an end-user expects.
I think you made this deviate from the usual end-user expectation
because it is unpredictable when the configuration kicks in the next
time to undo the effect of this command line request. And I agree
that it is a very un-nice thing to do to the users if we did not
warn here, if we are to keep that unpredictableness.
But stepping back a bit, we know the user's intention is that with
"--split-index" the user wants to use the split index, even though
the user may have configuration not to use the split index. Don't
we want to honor that intention? In order to honor that intention
without getting confused by the configured variable to tell us not
to split, another way is to destroy that unpredictableness. For
that, shouldn't we automatically remove the configuration that says
"do not split" (by perhaps set it to "do split")? Doing so still
may need a warning that says "we disabled your previously configured
setting while at it", but it would be much better than a warning
message that essentially says "we do it once, but we will disobey
you at an unpredictable time", I would think.
^ permalink raw reply
* Re: [PATCH v3 10/21] read-cache: regenerate shared index if necessary
From: Junio C Hamano @ 2016-12-27 19:08 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-11-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> + case 0:
> + return 1; /* 0% means always write a new shared index */
> + case 100:
> + return 0; /* 100% means never write a new shared index */
> + default:
> + ; /* do nothing: just use the configured value */
> + }
Just like you did in 04/21, write "break" to avoid mistakes made in
the future, i.e.
default:
break; /* just use the configured value */
> +
> + /* Count not shared entries */
> + for (i = 0; i < istate->cache_nr; i++) {
> + struct cache_entry *ce = istate->cache[i];
> + if (!ce->index)
> + not_shared++;
> + }
> +
> + return istate->cache_nr * max_split < not_shared * 100;
On a 32-bit arch with 2G int and more than 20 million paths in the
index, multiplying by max_split that can come close to 100 can
theoretically cause integer overflow, but in practice it probably
does not matter. Or does it?
^ permalink raw reply
* Re: [PATCH v3 20/21] Documentation/config: add splitIndex.sharedIndexExpire
From: Junio C Hamano @ 2016-12-27 19:11 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-21-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/config.txt | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e0f5a77980..24e771d22e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2786,6 +2786,17 @@ splitIndex.maxPercentChange::
> than 20 percent of the total number of entries.
> See linkgit:git-update-index[1].
>
> +splitIndex.sharedIndexExpire::
> + When the split index feature is used, shared index files with
> + a mtime older than this time will be removed when a new shared
As end-user facing documentation, it would be much better if we can
rephrase it for those who do not know what a 'mtime' is, and it
would be even better if we can do so without losing precision.
I think "shared index files that were not modified since the time
this variable specifies will be removed" would be understandable and
correct enough?
> + index file is created. The value "now" expires all entries
> + immediately, and "never" suppresses expiration altogether.
> + The default value is "one.week.ago".
> + Note that each time a new split-index file is created, the
> + mtime of the related shared index file is updated to the
> + current time.
To match the above suggestion, "Note that a shared index file is
considered modified (for the purpose of expiration) each time a new
split-index file is created based on it."?
> + See linkgit:git-update-index[1].
> +
> status.relativePaths::
> By default, linkgit:git-status[1] shows paths relative to the
> current directory. Setting this variable to `false` shows paths
^ permalink raw reply
* Re: [PATCH v3 21/21] Documentation/git-update-index: explain splitIndex.*
From: Junio C Hamano @ 2016-12-27 19:13 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-22-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> --split-index::
> --no-split-index::
> - Enable or disable split index mode. If enabled, the index is
> - split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex.<SHA-1>.
> - Changes are accumulated in $GIT_DIR/index while the shared
> - index file contains all index entries stays unchanged. If
> - split-index mode is already enabled and `--split-index` is
> - given again, all changes in $GIT_DIR/index are pushed back to
> - the shared index file. This mode is designed for very large
> - indexes that take a significant amount of time to read or write.
> + Enable or disable split index mode. If split-index mode is
> + already enabled and `--split-index` is given again, all
> + changes in $GIT_DIR/index are pushed back to the shared index
> + file.
In the world after this series that introduced the percentage-based
auto consolidation, it smells strange, or even illogical, that index
is un-split after doing this:
$ git update-index --split-index
$ git update-index --split-index
Before this series, it may have been a quick and dirty way to force
consolidating the split index without totally disabling the feature,
i.e. it would have looked more like
$ git update-index --split-index
... work work work to accumulate more modifications
... consolidate into one --- there was no other way than
... disabling it temporarily
$ git update-index --no-split-index
... but the user likes the feature so re-enable it.
$ git update-index --split-index
which I guess is where this strange behaviour comes from.
It may be something we need to fix to unconfuse the end-users after
this series lands. Even though "first disable and then re-enable"
takes two commands (as opposed to one), it is far more logical. And
the percentage-based auto consolidation feature is meant to reduce
the need for manual consolidation, so it probably makes sense to
remove this illogical feature.
> @@ -394,6 +390,31 @@ Although this bit looks similar to assume-unchanged bit, its goal is
> different from assume-unchanged bit's. Skip-worktree also takes
> precedence over assume-unchanged bit when both are set.
>
> +Split index
> +-----------
> +
> +This mode is designed for very large indexes that take a significant
> +amount of time to read or write.
This is not a new problem, but it probably is incorrect to say "to
read or write". It saves time by not rewriting the whole thing but
instead write out only the updated bits. You'd still read the whole
thing while populating the in-core index from the disk, and if
anything, you'd probably spend _more_ cycles because you'd essentially
be reading the base and then reading the delta to apply on top.
> +To avoid deleting a shared index file that is still used, its mtime is
> +updated to the current time everytime a new split index based on the
> +shared index file is either created or read from.
The same comment on the mention of "mtime" in another patch applies
here as well.
^ permalink raw reply
* Intermittent failure of t0021
From: Michael Haggerty @ 2016-12-27 19:21 UTC (permalink / raw)
To: git discussion list, Lars Schneider
Hi,
I'm seeing intermittent failures of t0021. If I run it as follows (using
a ramdisk as temporary space, and `EXPENSIVE` turned off), it fails on
average about every 40 attempts:
$ make -j8 O=0 && (cd t && for i in $(seq 200); do if ./t0021-*.sh ;
then echo "passed attempt $i"; else echo "failed on attempt $i"; break;
fi ; done )
(I've also seen it fail while running the whole test suite, so it is not
the tight test loop that triggers the problem.)
The output when it fails is
Initialized empty Git repository in
/home/mhagger/self/proj/git/git/t/trash
directory.t0021-conversion/repo/.git/
[master (root-commit) 56d459b] test commit 1
Author: A U Thor <author@example.com>
1 file changed, 1 insertion(+)
create mode 100644 .gitattributes
[master c30c7fc] test commit 2
Author: A U Thor <author@example.com>
4 files changed, 5 insertions(+)
create mode 100644 test.r
create mode 100644 test2.r
create mode 100644 test4-empty.r
create mode 100644 testsubdir/test3 'sq',$x.r
sort: cannot read: rot13-filter.log: No such file or directory
--- expected.log 2016-12-27 17:58:18.017505012 +0000
+++ rot13-filter.log 2016-12-27 17:58:18.017505012 +0000
@@ -1,7 +0,0 @@
-x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
-x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
-x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
-x IN: clean testsubdir/test3 'sq',$x.r 49 [OK] -- OUT: 49 . [OK]
- 1 START
- 1 STOP
- 1 init handshake complete
not ok 15 - required process filter should filter data
The failure bisects to
9c48b4f "t0021: minor filter process test cleanup", 2016-12-04
If I test the two parts of that commit separately, it is the removal of
the `.` arguments from `git commit` that triggers the failures (i.e.,
not the removal of the creation of `.gitignore`).
It's possible that the test was unreliable or racy even before this
change, but I have run the same test 1000+ times against the parent
commit (b18f6a00662524443cfb82f5fed7d3b54524c8ab) under the same
conditions without seeing a single failure. So *something* seems to have
been changed for the worse by this commit.
I don't plan to look into it any further, but if you have trouble
reproducing the problem don't hesitate to contact me.
Michael
^ permalink raw reply
* Re: Intermittent failure of t0021
From: Michael Haggerty @ 2016-12-27 19:30 UTC (permalink / raw)
To: git discussion list, Lars Schneider
In-Reply-To: <ec454def-88a7-5562-1bfe-9d52de42dc55@alum.mit.edu>
On 12/27/2016 08:21 PM, Michael Haggerty wrote:
> I'm seeing intermittent failures of t0021. [...]
Just after submitting this, I saw that the problem has already been
reported and a fix proposed:
http://public-inbox.org/git/dd8decbc-f856-4f68-6d77-7ea9d5f9d126@ramsayjones.plus.com/
Sorry for the noise.
Michael
^ permalink raw reply
* [PATCH 0/2] submodule config test cleanup
From: Stefan Beller @ 2016-12-27 19:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
A test cleanup and an additional test for the submodule configuration.
Stefan Beller (2):
t7411: quote URLs
t7411: test lookup of uninitialized submodules
t/t7411-submodule-config.sh | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply
* [PATCH 1/2] t7411: quote URLs
From: Stefan Beller @ 2016-12-27 19:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161227193605.12413-1-sbeller@google.com>
The variables may contain white spaces, so we need to quote them.
By not quoting the variables we'd end up passing multiple arguments to
git config, which doesn't fail for two arguments as value.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7411-submodule-config.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..3646f28b40 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -120,8 +120,8 @@ test_expect_success 'reading of local configuration' '
"" submodule \
>actual &&
test_cmp expect_local_path actual &&
- git config submodule.a.url $old_a &&
- git config submodule.submodule.url $old_submodule &&
+ git config submodule.a.url "$old_a" &&
+ git config submodule.submodule.url "$old_submodule" &&
git config --unset submodule.a.path c
)
'
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply related
* [PATCH 2/2] t7411: test lookup of uninitialized submodules
From: Stefan Beller @ 2016-12-27 19:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161227193605.12413-1-sbeller@google.com>
Sometimes we need to lookup information of uninitialized submodules. Make
sure that works.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7411-submodule-config.sh | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 3646f28b40..b40df6a4c1 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -126,6 +126,27 @@ test_expect_success 'reading of local configuration' '
)
'
+cat >super/expect_url <<EOF
+Submodule url: '../submodule' for path 'b'
+Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
+EOF
+
+test_expect_success 'reading of local configuration for uninitialized submodules' '
+ (
+ cd super &&
+ git submodule deinit -f b &&
+ old_submodule=$(git config submodule.submodule.url) &&
+ git config submodule.submodule.url git@somewhere.else.net:submodule.git &&
+ test-submodule-config --url \
+ "" b \
+ "" submodule \
+ >actual &&
+ test_cmp expect_url actual &&
+ git config submodule.submodule.url "$old_submodule" &&
+ git submodule init b
+ )
+'
+
cat >super/expect_fetchrecurse_die.err <<EOF
fatal: bad submodule.submodule.fetchrecursesubmodules argument: blabla
EOF
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply related
* Re: [PATCH v9 01/20] ref-filter: implement %(if), %(then), and %(else) atoms
From: Junio C Hamano @ 2016-12-27 20:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-2-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>
> +Some atoms like %(align) and %(if) always require a matching %(end).
> +We call them "opening atoms" and sometimes denote them as %($open).
> +
> +When a scripting language specific quoting is in effect, everything
> +between a top-level opening atom and its matching %(end) is evaluated
> +according to the semantics of the opening atom and its result is
> +quoted.
What is unsaid in the last paragraph is that you assume "is
evaluated according to the semantics of the opening atom" does not
involve quoting and only the result from the top-level is quoted. I
am not sure if that is clear to the first-time readers.
>
> EXAMPLES
> --------
> @@ -273,6 +291,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
> eval "$eval"
> ------------
> ...
> +------------
> +git for-each-ref --format="%(refname)%(if)%(authorname)%(then) %(color:red)Authored by: %(authorname)%(end)"
> +------------
This makes readers wonder how "red"ness is reset, but that is not
something this example is interested in demonstrating. Let's drop
the %(color:red) bit to avoid distracting readers.
^ permalink raw reply
* Re: [PATCH v9 02/20] ref-filter: include reference to 'used_atom' within 'atom_value'
From: Junio C Hamano @ 2016-12-27 20:59 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-3-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Ensure that each 'atom_value' has a reference to its corresponding
> 'used_atom'. This let's us use values within 'used_atom' in the
s/let's us use/lets us use/;
> 'handler' function.
^ permalink raw reply
* Re: [PATCH v9 03/20] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Junio C Hamano @ 2016-12-27 21:00 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-4-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
>
> Add tests and Documentation for the same.
s/Documentation/documentation/
^ permalink raw reply
* Re: [PATCH v9 19/20] branch: use ref-filter printing APIs
From: Junio C Hamano @ 2016-12-27 21:17 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-20-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> static char branch_colors[][COLOR_MAXLEN] = {
> - GIT_COLOR_RESET,
> - GIT_COLOR_NORMAL, /* PLAIN */
> - GIT_COLOR_RED, /* REMOTE */
> - GIT_COLOR_NORMAL, /* LOCAL */
> - GIT_COLOR_GREEN, /* CURRENT */
> - GIT_COLOR_BLUE, /* UPSTREAM */
> + "%(color:reset)",
> + "%(color:reset)", /* PLAIN */
> + "%(color:red)", /* REMOTE */
> + "%(color:reset)", /* LOCAL */
> + "%(color:green)", /* CURRENT */
> + "%(color:blue)", /* UPSTREAM */
> };
The contents of this table is no longer tied to COLOR_MAXLEN. The
above entries used by default happen to be shorter, but it is
misleading.
static const char *branch_colors[] = {
"%(color:reset)",
...
};
perhaps?
More importantly, does this re-definition of the branch_colors[]
work with custom colors filled in git_branch_config() by calling
e.g. color_parse("red", branch_colors[BRANCH_COLOR_REMOTE]), where
"red" and "remote" come from an existing configuration file?
[color "branch"]
remote = red
It obviously would not work if you changed the type of branch_colors[]
because the color_parse() wants the caller to have allocated space
of COLOR_MAXLEN.
But if filling these ANSI escape sequence into the format works OK,
then doesn't it tell us that we do not need to have this change to
use "%(color:reset)" etc. as the new default values?
^ permalink raw reply
* Re: [PATCH v9 11/20] ref-filter: introduce refname_atom_parser()
From: Junio C Hamano @ 2016-12-27 21:04 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-12-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> +symref::
> + The ref which the given symbolic ref refers to. If not a
> + symbolic ref, nothing is printed. Respects the `:short` and
> + `:strip` options in the same way as `refname` above.
> +
I am slightly unhappy with this name. If we had an atom that lets
you ask "Is this a symref?" and yields "" or "->", it could also be
called symref, and we would name it "is_symref" or something to
disambiguate it. Then it is only fair to give this one that lets
you ask "What does this symref point at?" a bit more specific name,
like "symref_target" or something.
But probably I am worried too much. "is_symref", if necessary, can
be written as "%(if:notequals=)%(symref)%(then)...%(else)...%(end)"
and it is not likely that it would be used often, so let's keep it
as-is.
^ permalink raw reply
* Re: [PATCH v9 15/20] ref-filter: modify the 'lstrip=<N>' option to work with negative '<N>'
From: Junio C Hamano @ 2016-12-27 21:11 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, ramsay
In-Reply-To: <20161227162357.28212-16-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Currently the 'lstrip=<N>' option only takes a positive value '<N>'
> and strips '<N>' slash-separated path components from the left. Modify
> the 'lstrip' option to also take a negative number '<N>' which would
> only _leave_ behind 'N' slash-separated path components from the left.
"would only leave behind N components from the left" sounds as if
the result is A/B, when you are given A/B/C/D/E and asked to
lstrip:-2. Given these two tests added by the patch ...
> +test_atom head refname:lstrip=-1 master
> +test_atom head refname:lstrip=-2 heads/master
... I somehow think that is not what you wanted to say. Instead,
you strip from the left as many as necessary and leave -N
components that appear at the right-most end, no?
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -98,7 +98,8 @@ refname::
> abbreviation mode. If `lstrip=<N>` is appended, strips `<N>`
> slash-separated path components from the front of the refname
> (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
> - `<N>` must be a positive integer.
> + if `<N>` is a negative number, then only `<N>` path components
> + are left behind.
I think positive <N> is so obvious not to require an example but it
is good that you have one. The negative <N> case needs illustration
more than the positive case. Perhaps something like:
(e.g. %(refname:lstrip=-1) strips components of refs/tags/frotz
from the left to leave only one component, i.e. 'frotz').
Would %(refname:lstrip=-4) attempt to strip components of
refs/tags/frotz from the left to leave only four components, and
because the original does not have that many components, it ends
with refs/tags/frotz?
I am debating myself if we need something like "When the ref does
not have enough components, the result becomes an empty string if
stripping with positive <N>, or it becomes the full refname if
stripping with negative <N>. Neither is an error." is necessary
here. Or is it too obvious?
^ permalink raw reply
* Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Junio C Hamano @ 2016-12-27 21:55 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <CAGZ79kZLk2bNj2Z_PFfo9KzODn8nSihDLdLuKvpSPodR9Eg-4w@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>>>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>>> oidclr(&oid);
>>>> if (check_local_mod(&oid, index_only))
>>>> exit(1);
>>>> - } else if (!index_only) {
>>>> - if (check_submodules_use_gitfiles())
>>>> - exit(1);
>>>> }
>>>>
>>>
>>> Hmph. It may be a bit strange to see an "index-only" remove to
>>> touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is
>>> unconditionally called above, which feels somewhat unexpected.
>> ...
> Well scratch that.
> is_staging_gitmodules_ok only checks for the .gitmodules file and not
> for the submodule itself (the submodule is not an argument to that function)
>
> The actual answer is found in check_local_mod called via
>
> if (!force) {
> struct object_id oid;
> if (get_oid("HEAD", &oid))
> oidclr(&oid);
> if (check_local_mod(&oid, index_only))
> exit(1);
> }
OK, that happens way before this loop starts finding submodules and
removing them, so we can say that the latter is well protected.
Thanks.
^ 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