* Re: Bug report: $program_name in error message
From: Stefan Beller @ 2016-12-19 23:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Bleecher Snyder, vascomalmeida, git@vger.kernel.org
In-Reply-To: <xmqqfuljod70.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 19, 2016 at 12:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> -- >8 --
> Subject: rebase -i: fix mistaken i18n
>
> f2d17068fd ("i18n: rebase-interactive: mark comments of squash for
> translation", 2016-06-17) attempted to apply sh-i18n and failed to
> use $(eval_gettext "string with \$variable interpolation").
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Looks sensible.
Thanks,
Stefan
> else
> - commit_message HEAD > "$fixup_msg" || die "$(gettext "Cannot write \$fixup_msg")"
> + commit_message HEAD >"$fixup_msg" ||
> + die "$(eval_gettext "Cannot write \$fixup_msg")"
> count=2
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Brandon Williams @ 2016-12-19 23:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, sandals, David.Turner
In-Reply-To: <20161219232828.5075-4-sbeller@google.com>
On 12/19, Stefan Beller wrote:
> In a later patch we want to report the exact command that we run in the
> error message. Add a convenient way to the run command API that runs the
> given command or reports the exact command as failure.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> run-command.c | 28 ++++++++++++++++++++++++++++
> run-command.h | 4 ++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..a0587db334 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
> return code;
> }
>
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> + int i;
> + struct strbuf err = STRBUF_INIT;
> + if (cmd->git_cmd)
> + strbuf_addstr(&err, "git ");
> + for (i = 0; cmd->argv[i]; )
Missing the increment of i here.
> + strbuf_addf(&err, "'%s'", cmd->argv[i]);
> + die(_("could not %s %s"), action, err.buf);
> +}
> +
> int start_command(struct child_process *cmd)
> {
> int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
> return 0;
> }
>
> +void start_command_or_die(struct child_process *cmd)
> +{
> + if (start_command(cmd))
> + report_and_die(cmd, "start");
> +}
> +
> int finish_command(struct child_process *cmd)
> {
> int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
> @@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)
> return wait_or_whine(cmd->pid, cmd->argv[0], 1);
> }
>
> +void finish_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "finish");
> +}
>
> int run_command(struct child_process *cmd)
> {
> @@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
> return run_command(&cmd);
> }
>
> +void run_command_or_die(struct child_process *cmd)
> +{
> + if (finish_command(cmd))
> + report_and_die(cmd, "run");
> +}
> +
> #ifndef NO_PTHREADS
> static pthread_t main_thread;
> static int main_thread_set;
> diff --git a/run-command.h b/run-command.h
> index dd1c78c28d..e4585885c5 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -56,6 +56,10 @@ int finish_command(struct child_process *);
> int finish_command_in_signal(struct child_process *);
> int run_command(struct child_process *);
>
> +void start_command_or_die(struct child_process *);
> +void finish_command_or_die(struct child_process *);
> +void run_command_or_die(struct child_process *);
> +
> /*
> * Returns the path to the hook file, or NULL if the hook is missing
> * or disabled. Note that this points to static storage that will be
> --
> 2.11.0.rc2.53.gb7b3fba.dirty
>
--
Brandon Williams
^ permalink raw reply
* [PATCHv4 4/5] submodule: add flags to ok_to_remove_submodule
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
In-Reply-To: <20161219232828.5075-1-sbeller@google.com>
In different contexts the question "Is it ok to delete a submodule?"
may be answered differently.
In 293ab15eea (submodule: teach rm to remove submodules unless they
contain a git directory, 2012-09-26) a case was made that we can safely
ignore ignored untracked files for removal as we explicitely ask for the
removal of the submodule.
In a later patch we want to remove submodules even when the user doesn't
explicitly ask for it (e.g. checking out a tree-ish in which the submodule
doesn't exist). In that case we want to be more careful when it comes
to deletion of untracked files. As of this patch it is unclear how this
will be implemented exactly, so we'll offer flags in which the caller
can specify how the different untracked files ought to be handled.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 3 ++-
submodule.c | 19 +++++++++++++------
submodule.h | 5 ++++-
3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..fdd7183f61 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,8 @@ static int check_local_mod(struct object_id *head, int index_only)
*/
if (ce_match_stat(ce, &st, 0) ||
(S_ISGITLINK(ce->ce_mode) &&
- !ok_to_remove_submodule(ce->name)))
+ !ok_to_remove_submodule(ce->name,
+ SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..9aecb8930c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,7 +1019,7 @@ int submodule_uses_gitfile(const char *path)
return 1;
}
-int ok_to_remove_submodule(const char *path)
+int ok_to_remove_submodule(const char *path, unsigned flags)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1032,23 +1032,30 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ argv_array_pushl(&cp.args, "status", "--porcelain",
"--ignore-submodules=none", NULL);
+
+ if (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)
+ argv_array_push(&cp.args, "-uno");
+ else
+ argv_array_push(&cp.args, "-uall");
+
+ if (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
+ argv_array_push(&cp.args, "--ignored");
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
- if (start_command(&cp))
- die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
+ start_command_or_die(&cp);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
ok_to_remove = 0;
close(cp.out);
- if (finish_command(&cp))
- die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
+ finish_command_or_die(&cp);
strbuf_release(&buf);
return ok_to_remove;
diff --git a/submodule.h b/submodule.h
index 61fb610749..3ed3aa479a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,10 @@ extern int fetch_populated_submodules(const struct argv_array *options,
int quiet, int max_parallel_jobs);
extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
extern int submodule_uses_gitfile(const char *path);
-extern int ok_to_remove_submodule(const char *path);
+
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<1)
+extern int ok_to_remove_submodule(const char *path, unsigned flags);
extern int merge_submodule(unsigned char result[20], const char *path,
const unsigned char base[20],
const unsigned char a[20],
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 2/5] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
In-Reply-To: <20161219232828.5075-1-sbeller@google.com>
Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.
While at it, adapt the error messages to reflect the actual invocation.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..9f0b544ebe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {
- "status",
- "--porcelain",
- "-u",
- "--ignore-submodules=none",
- NULL,
- };
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- cp.argv = argv;
+ argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ "--ignore-submodules=none", NULL);
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s", path);
+ die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
close(cp.out);
if (finish_command(&cp))
- die("'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s", path);
+ die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
strbuf_release(&buf);
return ok_to_remove;
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 5/5] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
In-Reply-To: <20161219232828.5075-1-sbeller@google.com>
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>
---
builtin/rm.c | 79 ++++++++++++++---------------------------------------------
t/t3600-rm.sh | 39 ++++++++++++-----------------
2 files changed, 33 insertions(+), 85 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index fdd7183f61..ed758f7144 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);
@@ -218,13 +196,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,
@@ -246,8 +219,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:",
@@ -341,6 +312,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
@@ -357,9 +330,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);
}
/*
@@ -388,32 +358,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;
@@ -422,7 +380,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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
In-Reply-To: <20161219232828.5075-1-sbeller@google.com>
In a later patch we want to report the exact command that we run in the
error message. Add a convenient way to the run command API that runs the
given command or reports the exact command as failure.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
run-command.c | 28 ++++++++++++++++++++++++++++
run-command.h | 4 ++++
2 files changed, 32 insertions(+)
diff --git a/run-command.c b/run-command.c
index ca905a9e80..a0587db334 100644
--- a/run-command.c
+++ b/run-command.c
@@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
return code;
}
+static void report_and_die(struct child_process *cmd, const char *action)
+{
+ int i;
+ struct strbuf err = STRBUF_INIT;
+ if (cmd->git_cmd)
+ strbuf_addstr(&err, "git ");
+ for (i = 0; cmd->argv[i]; )
+ strbuf_addf(&err, "'%s'", cmd->argv[i]);
+ die(_("could not %s %s"), action, err.buf);
+}
+
int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
@@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
return 0;
}
+void start_command_or_die(struct child_process *cmd)
+{
+ if (start_command(cmd))
+ report_and_die(cmd, "start");
+}
+
int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
@@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)
return wait_or_whine(cmd->pid, cmd->argv[0], 1);
}
+void finish_command_or_die(struct child_process *cmd)
+{
+ if (finish_command(cmd))
+ report_and_die(cmd, "finish");
+}
int run_command(struct child_process *cmd)
{
@@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
return run_command(&cmd);
}
+void run_command_or_die(struct child_process *cmd)
+{
+ if (finish_command(cmd))
+ report_and_die(cmd, "run");
+}
+
#ifndef NO_PTHREADS
static pthread_t main_thread;
static int main_thread_set;
diff --git a/run-command.h b/run-command.h
index dd1c78c28d..e4585885c5 100644
--- a/run-command.h
+++ b/run-command.h
@@ -56,6 +56,10 @@ int finish_command(struct child_process *);
int finish_command_in_signal(struct child_process *);
int run_command(struct child_process *);
+void start_command_or_die(struct child_process *);
+void finish_command_or_die(struct child_process *);
+void run_command_or_die(struct child_process *);
+
/*
* Returns the path to the hook file, or NULL if the hook is missing
* or disabled. Note that this points to static storage that will be
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 1/5] submodule.h: add extern keyword to functions
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
In-Reply-To: <20161219232828.5075-1-sbeller@google.com>
As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.
As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line. Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.h | 55 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/submodule.h b/submodule.h
index 6229054b99..61fb610749 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,50 +29,55 @@ struct submodule_update_strategy {
};
#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
-int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
-void stage_updated_gitmodules(void);
-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
+extern int is_staging_gitmodules_ok(void);
+extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
+extern int remove_path_from_gitmodules(const char *path);
+extern void stage_updated_gitmodules(void);
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
const char *path);
-int submodule_config(const char *var, const char *value, void *cb);
-void gitmodules_config(void);
-int parse_submodule_update_strategy(const char *value,
+extern int submodule_config(const char *var, const char *value, void *cb);
+extern void gitmodules_config(void);
+extern int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
-const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
+extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
+extern void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
-void show_submodule_inline_diff(FILE *f, const char *path,
+extern void show_submodule_inline_diff(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
-void set_config_fetch_recurse_submodules(int value);
-void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-int fetch_populated_submodules(const struct argv_array *options,
+extern void set_config_fetch_recurse_submodules(int value);
+extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
int quiet, int max_parallel_jobs);
-unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int submodule_uses_gitfile(const char *path);
-int ok_to_remove_submodule(const char *path);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
- const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
- struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-int parallel_submodules(void);
+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int submodule_uses_gitfile(const char *path);
+extern int ok_to_remove_submodule(const char *path);
+extern int merge_submodule(unsigned char result[20], const char *path,
+ const unsigned char base[20],
+ const unsigned char a[20],
+ const unsigned char b[20], int search);
+extern int find_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_name,
+ struct string_list *needs_pushing);
+extern int push_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_name);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+extern int parallel_submodules(void);
/*
* Prepare the "env_array" parameter of a "struct child_process" for executing
* a submodule by clearing any repo-specific envirionment variables, but
* retaining any config in the environment.
*/
-void prepare_submodule_repo_env(struct argv_array *out);
+extern void prepare_submodule_repo_env(struct argv_array *out);
#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
extern void absorb_git_dir_into_superproject(const char *prefix,
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 0/5] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-19 23:28 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, sandals, David.Turner, Stefan Beller
v4:
* reworded commit messages of the last 2 patches
* introduced a new patch introducing {run,start,finish}_command_or_die
* found an existing function in dir.h to use to remove a directory
which deals gracefully with the cornercases, that Junio pointed out.
v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
if needed
* Removed all the error reporting from git-rm that was related to submodule
git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
with an appropriate error message.
v2:
* new base where to apply the patch:
sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
I got merge conflicts and resolved them this way:
#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
# git commit -m "submodule removal" submod &&
# git checkout HEAD^ &&
# git submodule update &&
#- git checkout -q HEAD^ 2>actual &&
#+ git checkout -q HEAD^ &&
# git checkout -q master 2>actual &&
# - echo "warning: unable to rmdir submod: Directory not empty" >expected &&
# - test_i18ncmp expected actual &&
# + test_i18ngrep "^warning: unable to rmdir submod:" actual &&
# git status -s submod >actual &&
# echo "?? submod/" >expected &&
# test_cmp expected actual &&
#
* improved commit message in "ok_to_remove_submodule: absorb the submodule git dir"
(David Turner offered me some advice on how to write better English off list)
* simplified code in last patch:
-> dropped wrong comment for fallthrough
-> moved redundant code out of both bodies of an if-clause.
* Fixed last patchs commit message to have "or_die" instead of or_dir.
v1:
The "checkout --recurse-submodules" series got too large to comfortably send
it out for review, so I had to break it up into smaller series'; this is the
first subseries, but it makes sense on its own.
This series teaches git-rm to absorb the git directory of a submodule instead
of failing and complaining about the git directory preventing deletion.
It applies on origin/sb/submodule-embed-gitdir.
Any feedback welcome!
Thanks,
Stefan
Stefan Beller (5):
submodule.h: add extern keyword to functions
submodule: modernize ok_to_remove_submodule to use argv_array
run-command: add {run,start,finish}_command_or_die
submodule: add flags to ok_to_remove_submodule
rm: absorb a submodules git dir before deletion
builtin/rm.c | 82 +++++++++++++++--------------------------------------------
run-command.c | 28 ++++++++++++++++++++
run-command.h | 4 +++
submodule.c | 27 ++++++++++----------
submodule.h | 58 ++++++++++++++++++++++++------------------
t/t3600-rm.sh | 39 +++++++++++-----------------
6 files changed, 114 insertions(+), 124 deletions(-)
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply
* Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.
From: Junio C Hamano @ 2016-12-19 23:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: Duy Nguyen, Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kZA1HxsdCAQxKiNMQrH9fLrhGKc8Um7x-R7u92ujVBigQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Dec 19, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> This goes on top of sb/submodule-embed-gitdir.
>>> When the absorbing of a git dir fails, try to recover into a sane state,
>>> i.e. try to undo the move of the git dir.
>>
>> Are these unconditionally good improvements? I ask because the
>> series is still not in 'next' and it is the last chance to fold
>> these into existing patches if we wanted to.
>
> Actually I forgot to mark these as RFC-ish as it is a response to
> https://public-inbox.org/git/20161219053507.GA2335@duynguyen.vn.dektech.internal/
> (which was the only review comment this round)
>
> So I'd say these patches are rather experimental in my mind
> unlike the absorb series, which I assume is ok as is already.
OK. Thanks for clarification.
^ permalink raw reply
* Re: [PATCH v2] mailinfo.c: move side-effects outside of assert
From: Junio C Hamano @ 2016-12-19 23:26 UTC (permalink / raw)
To: Kyle J. McKay
Cc: Jeff King, Johannes Schindelin, Jonathan Tan, Git mailing list
In-Reply-To: <ebaf4c892a78bc3ae614a23d87f9c0f@58437222ff6db9ee7cbe9d1a5a1ad4e>
"Kyle J. McKay" <mackyle@gmail.com> writes:
>> OK. So we do not expect it to fail, but we still do want the side
>> effect of that function (i.e. accmulation into the field).
>>
>> Somebody care to send a final "agreed-upon" version?
>
> Yup, here it is:
Thanks.
> -- 8< --
>
> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
>
> assert(call_a_function(...))
>
> The function in question, check_header, has side effects. This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
>
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
>
> Since the only time that mi->inbody_header_accum is appended to is
> in check_inbody_header, and appending onto a blank
> mi->inbody_header_accum always happens when is_inbody_header is
> true, this guarantees a prefix that causes check_header to always
> return true.
>
> Therefore replace the assert with an if !check_header + DIE
> combination to reflect this.
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Helped-by: Jeff King <peff@peff.net>
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
>
> Notes:
> Please include this PATCH in 2.11.x maint
>
> mailinfo.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 2fb3877e..a489d9d0 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
> {
> if (!mi->inbody_header_accum.len)
> return;
> - assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
> + if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
> + die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
> strbuf_reset(&mi->inbody_header_accum);
> }
>
> ---
^ permalink raw reply
* Re: [PATCH 2/2] dir.c: add retry logic to relocate_gitdir
From: Stefan Beller @ 2016-12-19 23:19 UTC (permalink / raw)
To: Brandon Williams; +Cc: Duy Nguyen, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20161219222551.GA41080@google.com>
On Mon, Dec 19, 2016 at 2:25 PM, Brandon Williams <bmwill@google.com> wrote:
> On 12/19, Stefan Beller wrote:
>> Relocating a git directory consists of 3 steps:
>> 1) Move the directory.
>> 2) Update the gitlink file.
>> 3) Set core.worktree correctly.
>>
>> In an ideal world all three steps would be part of one transaction, such
>> that either all of them happen correctly or none of them.
>> However currently we just execute these three steps sequentially and die
>> in case of an error in any of these 3 steps.
>>
>> Dying is ok in 1) as the transaction hasn't started and the state is
>> recoverable.
>>
>> When dying in 2), this is a problem as the repo state is left in an
>> inconsistent state, e.g. the git link file of a submodule could be
>> empty and hence even the superproject appears to be broken as basic
>> commands such as git-status would die as there is it cannot tell the
>> state of the submodule.
>> So in that case try to undo 1) to be in a less sufferable state.
>
> I now see why an atomic filesystem swap operation would be useful :)
>
>>
>> 3) is less of an issue as experiments with submodules show. When
>> core.worktree is unset or set incorrectly, git-status still works
>> both in the superproject as well as the working tree of the submodule.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> dir.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>> dir.h | 6 ++--
>> submodule.c | 3 +-
>> 3 files changed, 91 insertions(+), 12 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index b2cb23fe88..e4e3f69869 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2749,30 +2749,66 @@ void untracked_cache_add_to_index(struct index_state *istate,
>> untracked_cache_invalidate_path(istate, path);
>> }
>>
>> -static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
>> +/*
>> + * Just like write_file, we try hard to write the full content to the file.
>> + * If there is suspicion the write did not work correctly, make sure the file
>> + * is removed again.
>> + * Return 0 if the write succeeded, -1 if the file was removed,
>> + * -2 if the (partial) file is still there.
>> + */
>> +static int write_file_or_remove(const char *path, const char *buf, size_t len)
>> +{
>> + int retries = 3;
>> + int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
>> + if (write_in_full(fd, buf, len) != len) {
>> + warning_errno(_("could not write '%s'"), path);
>> + goto err;
>> + }
>> + if (close(fd)) {
>> + warning_errno(_("could not close '%s'"), path);
>> + goto err;
>> + }
>> + return 0;
>> +err:
>> + while (retries-- > 0) {
>> + if (file_exists(path))
>> + unlink_or_warn(path);
>> + else
>> + return -1;
>> + }
>> + return -2;
>> +}
>
> Any reason why on attempt 2 or 3 this would succeed if it failed on try
> 1?
>
This code is heavily inspired by refs/files-backend.c which upon
closer inspection only retries directory things within the git directory
(which is assumed to be accessed in parallel by different invocations
of Git)
So I think we could drop the retry logic.
>> {
>> + char *git_dir = xstrdup(real_path(new_git_dir));
>> + char *work_tree = xstrdup(real_path(path));
>> + int c;
>
> I guess in a later patch we could clean up these calls to real_path to
> use real_pathdup from bw/realpath-wo-chdir.
good point.
^ permalink raw reply
* Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.
From: Stefan Beller @ 2016-12-19 23:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqqfuljmswl.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 19, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This goes on top of sb/submodule-embed-gitdir.
>> When the absorbing of a git dir fails, try to recover into a sane state,
>> i.e. try to undo the move of the git dir.
>
> Are these unconditionally good improvements? I ask because the
> series is still not in 'next' and it is the last chance to fold
> these into existing patches if we wanted to.
>
Actually I forgot to mark these as RFC-ish as it is a response to
https://public-inbox.org/git/20161219053507.GA2335@duynguyen.vn.dektech.internal/
(which was the only review comment this round)
So I'd say these patches are rather experimental in my mind
unlike the absorb series, which I assume is ok as is already.
Thanks,
Stefan
^ permalink raw reply
* [PATCH v2] mailinfo.c: move side-effects outside of assert
From: Kyle J. McKay @ 2016-12-19 23:13 UTC (permalink / raw)
To: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Tan
Cc: Git mailing list
In-Reply-To: <xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com>
On Dec 19, 2016, at 13:01, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>>> die("BUG: some explanation of why this can never happen");
>>>>
>>>> which perhaps documents the intended assumptions more clearly. A
>>>> comment
>>>> regarding the side effects might also be helpful.
>>>
>>> I wondered exactly the same thing myself. I was hoping Jonathan
>>> would
>>> pipe in here with some analysis about whether this is:
>>>
>>> a) a super paranoid, just-in-case, can't really ever fail because
>>> by
>>> the time we get to this code we've already effectively validated
>>> everything that could cause check_header to return false in this
>>> case
>>> ...
>> The answer is "a". The only time that mi->inbody_header_accum is
>> appended to is in check_inbody_header, and appending onto a blank
>> mi->inbody_header_accum always happens when is_inbody_header is true
>> (which guarantees a prefix that causes check_header to always return
>> true).
>>
>> Peff's suggestion sounds reasonable to me, maybe with an error
>> message
>> like "BUG: inbody_header_accum, if not empty, must always contain a
>> valid in-body header".
>
> OK. So we do not expect it to fail, but we still do want the side
> effect of that function (i.e. accmulation into the field).
>
> Somebody care to send a final "agreed-upon" version?
Yup, here it is:
-- 8< --
Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:
assert(call_a_function(...))
The function in question, check_header, has side effects. This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.
Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.
Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.
Therefore replace the assert with an if !check_header + DIE
combination to reflect this.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
Notes:
Please include this PATCH in 2.11.x maint
mailinfo.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
{
if (!mi->inbody_header_accum.len)
return;
- assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+ if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
+ die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
strbuf_reset(&mi->inbody_header_accum);
}
---
^ permalink raw reply related
* Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.
From: Junio C Hamano @ 2016-12-19 22:54 UTC (permalink / raw)
To: Stefan Beller; +Cc: pclouds, bmwill, git
In-Reply-To: <20161219215709.24620-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This goes on top of sb/submodule-embed-gitdir.
> When the absorbing of a git dir fails, try to recover into a sane state,
> i.e. try to undo the move of the git dir.
Are these unconditionally good improvements? I ask because the
series is still not in 'next' and it is the last chance to fold
these into existing patches if we wanted to.
>
> Thanks,
> Stefan
>
> Stefan Beller (2):
> dir.c: split up connect_work_tree_and_git_dir
> dir.c: add retry logic to relocate_gitdir
>
> dir.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> dir.h | 6 ++--
> submodule.c | 3 +-
> 3 files changed, 110 insertions(+), 17 deletions(-)
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Philip Oakley @ 2016-12-19 22:53 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: Git List, Michael Haggerty
In-Reply-To: <cover.1482164633.git.mhagger@alum.mit.edu>
From: "Michael Haggerty" <mhagger@alum.mit.edu>
> This patch series changes a bunch of details about how remote-tracking
> references are rendered in the commit list of gitk:
>
[...]
> * Introduce a separate constant to specify the background color used
> for the branch name part of remote-tracking references, to allow it
> to differ from the color used for local branches (which by default
> is bright green).
>
> * Change the default background colors for remote-tracking branches to
> light brown and brown (formerly they were pale orange and bright
> green).
>
> I understand that the colors of pixels on computer screens is an even
> more emotional topic that that of bikesheds, so I implemented the last
> change as a separate commit, the last one in the series. Feel free to
> drop it if you don't want the default color change.
>
Just to say that there was an issue with the bright green (lime) a while
back when 'green' changed its colour.
dscho reported in
(https://github.com/git-for-windows/git/issues/300#issuecomment-133702654
26 Aug 2015)
"[T]his is a change in Tk 8.6 described here (http://wiki.tcl.tk/1424): From
Tcl/Tk 8.6 on, Tk uses Web colours instead of X11 ones, where they
conflict."
In particular the old bright green version of 'green' became a darker green,
with the old colour becoming named lime.
For me, I needed to change my colour scheme (to a lime) as I could not read
the refs against the darker colour.
Anyway, that's the background as I know it.
--
Philip
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Johan Herland @ 2016-12-19 22:05 UTC (permalink / raw)
To: Mike Hommey; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <20161219021212.15978-1-mh@glandium.org>
On Mon, Dec 19, 2016 at 3:12 AM, Mike Hommey <mh@glandium.org> wrote:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
>
> .versions[1].sha1 = $some_sha1
> .tree = <tree structure loaded from $some_sha1>
>
> However, when trees are imported, rather than inherited, that is not the
> case. One can import a tree with a filemodify command, replacing the
> root tree object.
>
> e.g.
> "M 040000 $some_sha1 \n"
>
> In this case, the tree_entry for the branch looks like:
>
> .versions[1].sha1 = $some_sha1
> .tree = NULL
>
> When adding new notes with the notemodify command, do_change_note_fanout
> is called to get a notes count, and to do so, it loops over the
> tree_entry->tree, but doesn't do anything when the tree is NULL.
>
> In the latter case above, it means do_change_note_fanout thinks the tree
> contains no note, and new notes are added with no fanout.
s/note,/notes,/
>
> Interestingly, do_change_note_fanout does check whether subdirectories
> have a NULL .tree, in which case it uses load_tree(). Which means the
> right behaviour happens when using the filemodify command to import
> subdirectories.
>
> This change makes do_change_note_fanount call load_tree() whenever the
> tree_entry it is given has no tree loaded, making all cases handled
> equally.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
Acked-by: Johan Herland <johan@herland.net>
^ permalink raw reply
* Re: [PATCH 2/2] dir.c: add retry logic to relocate_gitdir
From: Brandon Williams @ 2016-12-19 22:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: pclouds, git, gitster
In-Reply-To: <20161219215709.24620-3-sbeller@google.com>
On 12/19, Stefan Beller wrote:
> Relocating a git directory consists of 3 steps:
> 1) Move the directory.
> 2) Update the gitlink file.
> 3) Set core.worktree correctly.
>
> In an ideal world all three steps would be part of one transaction, such
> that either all of them happen correctly or none of them.
> However currently we just execute these three steps sequentially and die
> in case of an error in any of these 3 steps.
>
> Dying is ok in 1) as the transaction hasn't started and the state is
> recoverable.
>
> When dying in 2), this is a problem as the repo state is left in an
> inconsistent state, e.g. the git link file of a submodule could be
> empty and hence even the superproject appears to be broken as basic
> commands such as git-status would die as there is it cannot tell the
> state of the submodule.
> So in that case try to undo 1) to be in a less sufferable state.
I now see why an atomic filesystem swap operation would be useful :)
>
> 3) is less of an issue as experiments with submodules show. When
> core.worktree is unset or set incorrectly, git-status still works
> both in the superproject as well as the working tree of the submodule.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> dir.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> dir.h | 6 ++--
> submodule.c | 3 +-
> 3 files changed, 91 insertions(+), 12 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index b2cb23fe88..e4e3f69869 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2749,30 +2749,66 @@ void untracked_cache_add_to_index(struct index_state *istate,
> untracked_cache_invalidate_path(istate, path);
> }
>
> -static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
> +/*
> + * Just like write_file, we try hard to write the full content to the file.
> + * If there is suspicion the write did not work correctly, make sure the file
> + * is removed again.
> + * Return 0 if the write succeeded, -1 if the file was removed,
> + * -2 if the (partial) file is still there.
> + */
> +static int write_file_or_remove(const char *path, const char *buf, size_t len)
> +{
> + int retries = 3;
> + int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> + if (write_in_full(fd, buf, len) != len) {
> + warning_errno(_("could not write '%s'"), path);
> + goto err;
> + }
> + if (close(fd)) {
> + warning_errno(_("could not close '%s'"), path);
> + goto err;
> + }
> + return 0;
> +err:
> + while (retries-- > 0) {
> + if (file_exists(path))
> + unlink_or_warn(path);
> + else
> + return -1;
> + }
> + return -2;
> +}
Any reason why on attempt 2 or 3 this would succeed if it failed on try
1?
> +
> +static int point_gitlink_file_to(const char *work_tree, const char *git_dir)
> {
> struct strbuf file_name = STRBUF_INIT;
> struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf content = STRBUF_INIT;
> + int ret;
>
> strbuf_addf(&file_name, "%s/.git", work_tree);
> - write_file(file_name.buf, "gitdir: %s",
> - relative_path(git_dir, work_tree, &rel_path));
> + strbuf_addf(&content, "gitdir: %s",
> + relative_path(git_dir, work_tree, &rel_path));
> + ret = write_file_or_remove(file_name.buf, content.buf, content.len);
>
> strbuf_release(&file_name);
> strbuf_release(&rel_path);
> + return ret;
> }
>
> -static void set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
> +static int set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
> {
> struct strbuf file_name = STRBUF_INIT;
> struct strbuf rel_path = STRBUF_INIT;
> + int ret;
>
> strbuf_addf(&file_name, "%s/config", git_dir);
> - git_config_set_in_file(file_name.buf, "core.worktree",
> + ret = git_config_set_in_file_gently(file_name.buf, "core.worktree",
> relative_path(work_tree, git_dir, &rel_path));
>
> strbuf_release(&file_name);
> strbuf_release(&rel_path);
> + return ret;
> }
>
> /* Update gitfile and core.worktree setting to connect work tree and git dir */
> @@ -2790,12 +2826,54 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>
> /*
> * Migrate the git directory of the given path from old_git_dir to new_git_dir.
> + * Return 0 on success and -1 on a minor issue. Die in case the repository is
> + * fatally messed up.
> */
> -void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> +int relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> {
> + char *git_dir = xstrdup(real_path(new_git_dir));
> + char *work_tree = xstrdup(real_path(path));
> + int c;
I guess in a later patch we could clean up these calls to real_path to
use real_pathdup from bw/realpath-wo-chdir.
> +
> if (rename(old_git_dir, new_git_dir) < 0)
> - die_errno(_("could not migrate git directory from '%s' to '%s'"),
> + die_errno(_("could not rename git directory from '%s' to '%s'"),
> old_git_dir, new_git_dir);
>
> - connect_work_tree_and_git_dir(path, new_git_dir);
> + c = point_gitlink_file_to(work_tree, git_dir);
> + if (c < 0) {
> + warning(_("tried to move the git directory from '%s' to '%s'"),
> + old_git_dir, new_git_dir);
> + warning(_("problems with creating a .git file in '%s' to point to '%s'"),
> + work_tree, new_git_dir);
> + if (c == -1) {
> + warning(_("try to undo the move"));
> + if (rename(new_git_dir, old_git_dir) < 0)
> + die_errno(_("could not rename git directory from '%s' to '%s'"),
> + new_git_dir, old_git_dir);
> + return -1;
> + } else if (c == -2) {
> + warning(_("The .git file is still there, "
> + "where the undo operation would move the git "
> + "directory"));
> + die(_("failed to undo the git directory relocation"));
> + }
> + };
> +
> + if (set_core_work_tree_to_connect(work_tree, git_dir) < 0) {
> + /*
> + * At this point the git dir was moved and the gitlink file
> + * was updated correctly, this leaves the repository in a
> + * state that is not totally broken. Setting the core.worktree
> + * correctly would have been the last step to perform a
> + * complete git directory relocation, but this is good enough
> + * to not die.
> + */
> + warning(_("could not set core.worktree in '%s' to point at '%s'"),
> + git_dir, work_tree);
> + return -1;
> + }
> +
> + free(work_tree);
> + free(git_dir);
> + return 0;
> }
> diff --git a/dir.h b/dir.h
> index bf23a470af..86f1cf790f 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -336,7 +336,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
> void add_untracked_cache(struct index_state *istate);
> void remove_untracked_cache(struct index_state *istate);
> extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> -extern void relocate_gitdir(const char *path,
> - const char *old_git_dir,
> - const char *new_git_dir);
> +extern int relocate_gitdir(const char *path,
> + const char *old_git_dir,
> + const char *new_git_dir);
> #endif
> diff --git a/submodule.c b/submodule.c
> index 45ccfb7ab4..fa1f44bb5a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1277,7 +1277,8 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
> prefix ? prefix : "", path,
> real_old_git_dir, real_new_git_dir);
>
> - relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
> + if (relocate_gitdir(path, real_old_git_dir, real_new_git_dir))
> + die(_("could not relocate git directory of '%s'"), path);
>
> free(old_git_dir);
> free(real_old_git_dir);
> --
> 2.11.0.rc2.53.gb7b3fba.dirty
>
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] Tweak help auto-correct phrasing.
From: Junio C Hamano @ 2016-12-19 22:04 UTC (permalink / raw)
To: Marc Branchaud; +Cc: git, Kaartic Sivaraam, Chris Packham, Alex Riesen
In-Reply-To: <20161219170137.5507-1-marcnarc@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> ---
>
> On 2016-12-18 07:48 PM, Chris Packham wrote:
>>
>> This feature already exists (although it's not interactive). See
>> help.autoCorrect in the git-config man page. "git config
>> help.autoCorrect -1" should to the trick.
>
> Awesome, I was unaware of this feature. Thanks!
>
> I found the message it prints a bit awkward, so here's a patch to fix it up.
>
> Instead of:
>
> WARNING: You called a Git command named 'lgo', which does not exist.
> Continuing under the assumption that you meant 'log'
> in 1.5 seconds automatically...
>
> it's now:
>
> WARNING: You called a Git command named 'lgo', which does not exist.
> Continuing in 1.5 seconds under the assumption that you meant 'log'.
>
> M.
Sounds better.
The "Instead of ... we now show ..." description deserves to be in
the log message, not after "---" line.
s/under the assumption/assuming/ would make it even shorter and give
the potentially long corrected command name a chance to still fit on
the line without wrapping, I would think, though.
>
> help.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/help.c b/help.c
> index 53e2a67e00..55350c0673 100644
> --- a/help.c
> +++ b/help.c
> @@ -381,12 +381,18 @@ const char *help_unknown_cmd(const char *cmd)
> clean_cmdnames(&main_cmds);
> fprintf_ln(stderr,
> _("WARNING: You called a Git command named '%s', "
> - "which does not exist.\n"
> - "Continuing under the assumption that you meant '%s'"),
> - cmd, assumed);
> - if (autocorrect > 0) {
> - fprintf_ln(stderr, _("in %0.1f seconds automatically..."),
> - (float)autocorrect/10.0);
> + "which does not exist."),
> + cmd);
> + if (autocorrect < 0)
> + fprintf_ln(stderr,
> + _("Continuing under the assumption that "
> + "you meant '%s'."),
> + assumed);
> + else {
> + fprintf_ln(stderr,
> + _("Continuing in %0.1f seconds under the "
> + "assumption that you meant '%s'."),
> + (float)autocorrect/10.0, assumed);
> sleep_millisec(autocorrect * 100);
> }
> return assumed;
^ permalink raw reply
* [PATCH 1/2] dir.c: split up connect_work_tree_and_git_dir
From: Stefan Beller @ 2016-12-19 21:57 UTC (permalink / raw)
To: pclouds, bmwill; +Cc: git, gitster, Stefan Beller
In-Reply-To: <20161219215709.24620-1-sbeller@google.com>
In a later patch we want to treat the failures of each of the two steps
differently, so split them up first.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
dir.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/dir.c b/dir.c
index d872cc1570..b2cb23fe88 100644
--- a/dir.c
+++ b/dir.c
@@ -2749,27 +2749,41 @@ void untracked_cache_add_to_index(struct index_state *istate,
untracked_cache_invalidate_path(istate, path);
}
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
+static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- char *git_dir = xstrdup(real_path(git_dir_));
- char *work_tree = xstrdup(real_path(work_tree_));
- /* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
relative_path(git_dir, work_tree, &rel_path));
- /* Update core.worktree setting */
- strbuf_reset(&file_name);
+ strbuf_release(&file_name);
+ strbuf_release(&rel_path);
+}
+
+static void set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
+{
+ struct strbuf file_name = STRBUF_INIT;
+ struct strbuf rel_path = STRBUF_INIT;
+
strbuf_addf(&file_name, "%s/config", git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
relative_path(work_tree, git_dir, &rel_path));
strbuf_release(&file_name);
strbuf_release(&rel_path);
+}
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
+{
+ char *git_dir = xstrdup(real_path(git_dir_));
+ char *work_tree = xstrdup(real_path(work_tree_));
+
+ point_gitlink_file_to(work_tree, git_dir);
+ set_core_work_tree_to_connect(work_tree, git_dir);
+
free(work_tree);
free(git_dir);
}
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCH 2/2] dir.c: add retry logic to relocate_gitdir
From: Stefan Beller @ 2016-12-19 21:57 UTC (permalink / raw)
To: pclouds, bmwill; +Cc: git, gitster, Stefan Beller
In-Reply-To: <20161219215709.24620-1-sbeller@google.com>
Relocating a git directory consists of 3 steps:
1) Move the directory.
2) Update the gitlink file.
3) Set core.worktree correctly.
In an ideal world all three steps would be part of one transaction, such
that either all of them happen correctly or none of them.
However currently we just execute these three steps sequentially and die
in case of an error in any of these 3 steps.
Dying is ok in 1) as the transaction hasn't started and the state is
recoverable.
When dying in 2), this is a problem as the repo state is left in an
inconsistent state, e.g. the git link file of a submodule could be
empty and hence even the superproject appears to be broken as basic
commands such as git-status would die as there is it cannot tell the
state of the submodule.
So in that case try to undo 1) to be in a less sufferable state.
3) is less of an issue as experiments with submodules show. When
core.worktree is unset or set incorrectly, git-status still works
both in the superproject as well as the working tree of the submodule.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
dir.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
dir.h | 6 ++--
submodule.c | 3 +-
3 files changed, 91 insertions(+), 12 deletions(-)
diff --git a/dir.c b/dir.c
index b2cb23fe88..e4e3f69869 100644
--- a/dir.c
+++ b/dir.c
@@ -2749,30 +2749,66 @@ void untracked_cache_add_to_index(struct index_state *istate,
untracked_cache_invalidate_path(istate, path);
}
-static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
+/*
+ * Just like write_file, we try hard to write the full content to the file.
+ * If there is suspicion the write did not work correctly, make sure the file
+ * is removed again.
+ * Return 0 if the write succeeded, -1 if the file was removed,
+ * -2 if the (partial) file is still there.
+ */
+static int write_file_or_remove(const char *path, const char *buf, size_t len)
+{
+ int retries = 3;
+ int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+ if (write_in_full(fd, buf, len) != len) {
+ warning_errno(_("could not write '%s'"), path);
+ goto err;
+ }
+ if (close(fd)) {
+ warning_errno(_("could not close '%s'"), path);
+ goto err;
+ }
+ return 0;
+err:
+ while (retries-- > 0) {
+ if (file_exists(path))
+ unlink_or_warn(path);
+ else
+ return -1;
+ }
+ return -2;
+}
+
+static int point_gitlink_file_to(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
+ struct strbuf content = STRBUF_INIT;
+ int ret;
strbuf_addf(&file_name, "%s/.git", work_tree);
- write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, work_tree, &rel_path));
+ strbuf_addf(&content, "gitdir: %s",
+ relative_path(git_dir, work_tree, &rel_path));
+ ret = write_file_or_remove(file_name.buf, content.buf, content.len);
strbuf_release(&file_name);
strbuf_release(&rel_path);
+ return ret;
}
-static void set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
+static int set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
+ int ret;
strbuf_addf(&file_name, "%s/config", git_dir);
- git_config_set_in_file(file_name.buf, "core.worktree",
+ ret = git_config_set_in_file_gently(file_name.buf, "core.worktree",
relative_path(work_tree, git_dir, &rel_path));
strbuf_release(&file_name);
strbuf_release(&rel_path);
+ return ret;
}
/* Update gitfile and core.worktree setting to connect work tree and git dir */
@@ -2790,12 +2826,54 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
/*
* Migrate the git directory of the given path from old_git_dir to new_git_dir.
+ * Return 0 on success and -1 on a minor issue. Die in case the repository is
+ * fatally messed up.
*/
-void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
+int relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
{
+ char *git_dir = xstrdup(real_path(new_git_dir));
+ char *work_tree = xstrdup(real_path(path));
+ int c;
+
if (rename(old_git_dir, new_git_dir) < 0)
- die_errno(_("could not migrate git directory from '%s' to '%s'"),
+ die_errno(_("could not rename git directory from '%s' to '%s'"),
old_git_dir, new_git_dir);
- connect_work_tree_and_git_dir(path, new_git_dir);
+ c = point_gitlink_file_to(work_tree, git_dir);
+ if (c < 0) {
+ warning(_("tried to move the git directory from '%s' to '%s'"),
+ old_git_dir, new_git_dir);
+ warning(_("problems with creating a .git file in '%s' to point to '%s'"),
+ work_tree, new_git_dir);
+ if (c == -1) {
+ warning(_("try to undo the move"));
+ if (rename(new_git_dir, old_git_dir) < 0)
+ die_errno(_("could not rename git directory from '%s' to '%s'"),
+ new_git_dir, old_git_dir);
+ return -1;
+ } else if (c == -2) {
+ warning(_("The .git file is still there, "
+ "where the undo operation would move the git "
+ "directory"));
+ die(_("failed to undo the git directory relocation"));
+ }
+ };
+
+ if (set_core_work_tree_to_connect(work_tree, git_dir) < 0) {
+ /*
+ * At this point the git dir was moved and the gitlink file
+ * was updated correctly, this leaves the repository in a
+ * state that is not totally broken. Setting the core.worktree
+ * correctly would have been the last step to perform a
+ * complete git directory relocation, but this is good enough
+ * to not die.
+ */
+ warning(_("could not set core.worktree in '%s' to point at '%s'"),
+ git_dir, work_tree);
+ return -1;
+ }
+
+ free(work_tree);
+ free(git_dir);
+ return 0;
}
diff --git a/dir.h b/dir.h
index bf23a470af..86f1cf790f 100644
--- a/dir.h
+++ b/dir.h
@@ -336,7 +336,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
-extern void relocate_gitdir(const char *path,
- const char *old_git_dir,
- const char *new_git_dir);
+extern int relocate_gitdir(const char *path,
+ const char *old_git_dir,
+ const char *new_git_dir);
#endif
diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..fa1f44bb5a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1277,7 +1277,8 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
prefix ? prefix : "", path,
real_old_git_dir, real_new_git_dir);
- relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
+ if (relocate_gitdir(path, real_old_git_dir, real_new_git_dir))
+ die(_("could not relocate git directory of '%s'"), path);
free(old_git_dir);
free(real_old_git_dir);
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCH 0/2] improve relocate_git_dir to fail into a sane state.
From: Stefan Beller @ 2016-12-19 21:57 UTC (permalink / raw)
To: pclouds, bmwill; +Cc: git, gitster, Stefan Beller
This goes on top of sb/submodule-embed-gitdir.
When the absorbing of a git dir fails, try to recover into a sane state,
i.e. try to undo the move of the git dir.
Thanks,
Stefan
Stefan Beller (2):
dir.c: split up connect_work_tree_and_git_dir
dir.c: add retry logic to relocate_gitdir
dir.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
dir.h | 6 ++--
submodule.c | 3 +-
3 files changed, 110 insertions(+), 17 deletions(-)
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply
* Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes
From: Junio C Hamano @ 2016-12-19 21:57 UTC (permalink / raw)
To: Max Kirillov; +Cc: Johannes Schindelin, Karsten Blees, git, Johannes Sixt
In-Reply-To: <1482183120-21592-1-git-send-email-max@max630.net>
Max Kirillov <max@max630.net> writes:
> UNICODE_STRING::Length field means size of buffer in bytes[1], despite of buffer
> itself being array of wchar_t. Because of that terminating zero is placed twice
> as far. Fix it.
>
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
Max, I see this is a resend from a few days ago. I suspect that we
are in a slow season, so there may be longer delay until we see
responses to a patch.
I will wait until taking any patch to files specific to MS Windows
platform (compat/win*, compat/mingw*, and compat/vcbuild*) without
first seeing it reviewed and acked by either of the two Johannes's
(well, there might be other people in addition to those two, whose
Acked-by/Reviewed-by I should be trusting; if that is the case, it
only shows how unqualified I would be as the first contact to be on
the To: line of such a patch).
I do not mind "see the patch, ping Johanneses as needed, wait and
then apply it to my tree" flow, but I also wonder if the process can
be made more efficient. Dscho (one of the Johanneses) gets many
patches specific to Windows port via the Git-for-Windows project and
then "upstreams" by forwarding with his sign-off in my direction,
and I do not mind that flow, either. Whichever one is the most
efficient for all three parties involved is fine by me, but if one
is preferred over the other, perhaps I should write it down in the
"notes from the maintainer" or somewhere?
Dscho, J6t, what do you think?
Thanks.
> Access outside of buffer was very unlikely (for that user needed to redirect
> standard fd to a file with path longer than ~250 symbols), it still did not
> seem to do any harm, and otherwise it did not break because only substring is
> checked, but it was still incorrect.
> compat/winansi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 3be60ce..6b4f736 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -553,7 +553,7 @@ static void detect_msys_tty(int fd)
> buffer, sizeof(buffer) - 2, &result)))
> return;
> name = nameinfo->Name.Buffer;
> - name[nameinfo->Name.Length] = 0;
> + name[nameinfo->Name.Length / sizeof(*name)] = 0;
>
> /* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
> if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
^ permalink raw reply
* [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes
From: Max Kirillov @ 2016-12-19 21:32 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin, Karsten Blees; +Cc: Max Kirillov, git
UNICODE_STRING::Length field means size of buffer in bytes[1], despite of buffer
itself being array of wchar_t. Because of that terminating zero is placed twice
as far. Fix it.
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx
Signed-off-by: Max Kirillov <max@max630.net>
---
Access outside of buffer was very unlikely (for that user needed to redirect
standard fd to a file with path longer than ~250 symbols), it still did not
seem to do any harm, and otherwise it did not break because only substring is
checked, but it was still incorrect.
compat/winansi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 3be60ce..6b4f736 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -553,7 +553,7 @@ static void detect_msys_tty(int fd)
buffer, sizeof(buffer) - 2, &result)))
return;
name = nameinfo->Name.Buffer;
- name[nameinfo->Name.Length] = 0;
+ name[nameinfo->Name.Length / sizeof(*name)] = 0;
/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
--
2.3.4.2801.g3d0809b
^ permalink raw reply related
* Re: [PATCH v1] git-p4: add diff/merge properties to .gitattributes for GitLFS files
From: Junio C Hamano @ 2016-12-19 21:29 UTC (permalink / raw)
To: larsxschneider; +Cc: git, luke
In-Reply-To: <20161218190140.3732-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> The `git lfs track` command generates a .gitattributes file with diff
> and merge properties [1]. Set the same .gitattributes format for files
> tracked with GitLFS in git-p4.
>
> [1] https://github.com/git-lfs/git-lfs/blob/v1.5.3/commands/command_track.go#L121
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
Thanks. Luke, does this look ok?
>
> Notes:
> Base Commit: d1271bddd4 (v2.11.0)
> Diff on Web: https://github.com/git/git/compare/d1271bddd4...larsxschneider:e045b3d5c8
> Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-lfs-attributes-v1 && git checkout e045b3d5c8
>
> git-p4.py | 4 ++--
> t/t9824-git-p4-git-lfs.sh | 24 ++++++++++++------------
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52462..87b6932c81 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1098,10 +1098,10 @@ class GitLFS(LargeFileSystem):
> '# Git LFS (see https://git-lfs.github.com/)\n',
> '#\n',
> ] +
> - ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
> + ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
> for f in sorted(gitConfigList('git-p4.largeFileExtensions'))
> ] +
> - ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
> + ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
> for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)
> ]
> )
> diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
> index 110a7e7924..1379db6357 100755
> --- a/t/t9824-git-p4-git-lfs.sh
> +++ b/t/t9824-git-p4-git-lfs.sh
> @@ -81,9 +81,9 @@ test_expect_success 'Store files in LFS based on size (>24 bytes)' '
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - /file2.dat filter=lfs -text
> - /file4.bin filter=lfs -text
> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
> + /file2.dat filter=lfs diff=lfs merge=lfs -text
> + /file4.bin filter=lfs diff=lfs merge=lfs -text
> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -109,7 +109,7 @@ test_expect_success 'Store files in LFS based on size (>25 bytes)' '
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - /file4.bin filter=lfs -text
> + /file4.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -135,7 +135,7 @@ test_expect_success 'Store files in LFS based on extension (dat)' '
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - *.dat filter=lfs -text
> + *.dat filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -163,8 +163,8 @@ test_expect_success 'Store files in LFS based on size (>25 bytes) and extension
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - *.dat filter=lfs -text
> - /file4.bin filter=lfs -text
> + *.dat filter=lfs diff=lfs merge=lfs -text
> + /file4.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -199,8 +199,8 @@ test_expect_success 'Remove file from repo and store files in LFS based on size
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - /file2.dat filter=lfs -text
> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
> + /file2.dat filter=lfs diff=lfs merge=lfs -text
> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -237,8 +237,8 @@ test_expect_success 'Add .gitattributes and store files in LFS based on size (>2
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - /file2.dat filter=lfs -text
> - /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
> + /file2.dat filter=lfs diff=lfs merge=lfs -text
> + /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
> @@ -278,7 +278,7 @@ test_expect_success 'Add big files to repo and store files in LFS based on compr
> #
> # Git LFS (see https://git-lfs.github.com/)
> #
> - /file6.bin filter=lfs -text
> + /file6.bin filter=lfs diff=lfs merge=lfs -text
> EOF
> test_path_is_file .gitattributes &&
> test_cmp expect .gitattributes
^ permalink raw reply
* Re: [PATCH v1] git-p4: fix git-p4.pathEncoding for removed files
From: Junio C Hamano @ 2016-12-19 21:29 UTC (permalink / raw)
To: larsxschneider; +Cc: git, luke
In-Reply-To: <20161218175153.92336-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> In a9e38359e3 we taught git-p4 a way to re-encode path names from what
> was used in Perforce to UTF-8. This path re-encoding worked properly for
> "added" paths. "Removed" paths were not re-encoded and therefore
> different from the "added" paths. Consequently, these files were not
> removed in a git-p4 cloned Git repository because the path names did not
> match.
>
> Fix this by moving the re-encoding to a place that affects "added" and
> "removed" paths. Add a test to demonstrate the issue.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
Thanks.
The above description makes me wonder what happens to "modified"
paths, but presumably they are handled in a separate codepath? Or
does this also cover not just "removed" but also paths with any
change?
Luke, does this look good?
> Notes:
> Base Commit: d1271bddd4 (v2.11.0)
> Diff on Web: https://github.com/git/git/compare/d1271bddd4...larsxschneider:05a82caa69
> Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-path-encoding-v1 && git checkout 05a82caa69
>
> git-p4.py | 19 +++++++++----------
> t/t9822-git-p4-path-encoding.sh | 16 ++++++++++++++++
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52462..8f311cb4e8 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2366,6 +2366,15 @@ class P4Sync(Command, P4UserMap):
> break
>
> path = wildcard_decode(path)
> + try:
> + path.decode('ascii')
> + except:
> + encoding = 'utf8'
> + if gitConfig('git-p4.pathEncoding'):
> + encoding = gitConfig('git-p4.pathEncoding')
> + path = path.decode(encoding, 'replace').encode('utf8', 'replace')
> + if self.verbose:
> + print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path)
> return path
>
> def splitFilesIntoBranches(self, commit):
> @@ -2495,16 +2504,6 @@ class P4Sync(Command, P4UserMap):
> text = regexp.sub(r'$\1$', text)
> contents = [ text ]
>
> - try:
> - relPath.decode('ascii')
> - except:
> - encoding = 'utf8'
> - if gitConfig('git-p4.pathEncoding'):
> - encoding = gitConfig('git-p4.pathEncoding')
> - relPath = relPath.decode(encoding, 'replace').encode('utf8', 'replace')
> - if self.verbose:
> - print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)
> -
> if self.largeFileSystem:
> (git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)
>
> diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
> index 7b83e696a9..c78477c19b 100755
> --- a/t/t9822-git-p4-path-encoding.sh
> +++ b/t/t9822-git-p4-path-encoding.sh
> @@ -51,6 +51,22 @@ test_expect_success 'Clone repo containing iso8859-1 encoded paths with git-p4.p
> )
> '
>
> +test_expect_success 'Delete iso8859-1 encoded paths and clone' '
> + (
> + cd "$cli" &&
> + ISO8859="$(printf "$ISO8859_ESCAPED")" &&
> + p4 delete "$ISO8859" &&
> + p4 submit -d "remove file"
> + ) &&
> + git p4 clone --destination="$git" //depot@all &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git -c core.quotepath=false ls-files >actual &&
> + test_must_be_empty actual
> + )
> +'
> +
> test_expect_success 'kill p4d' '
> kill_p4d
> '
^ 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