* 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
* Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
From: Stephan Beyer @ 2016-12-19 21:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612191515000.54750@virtualbox>
Hi Dscho,
>> However, maintaining more than one directory (like "sequencer" for
>> sequencer state and "rebase-merge" for rebase todo and log) can cause
>> the necessity to be even more careful when hacking on sequencer... For
>> example, the cleanup code must delete both of them, not only one of them.
>
> That is incorrect. It depends on the options which directory is used. And
> it is that directory (and not both) that should be cleaned up in the end.
>
> Otherwise you run into a ton of pain e.g. when running a rebase -i with an
> `exec git cherry-pick ...` line: all of a sudden, that little innocuous
> line would simply destroy the state directory of the current rebase -i.
>
> That's a rather big no-no.
Ahh, I see, there seems to be a misunderstanding on my side about how
you did it (I did not look into the other patches (obviously)).
Thanks for clarifying!
Best
Stephan
^ permalink raw reply
* Re: [PATCH v2 00/21] Add configuration options for split-index
From: Junio C Hamano @ 2016-12-19 21:03 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161219120246.GE24125@ash>
Duy Nguyen <pclouds@gmail.com> writes:
> I've read through the series (*) and I think it looks good, just a few
> minor comments here and there.
>
> (*) guiltily admit that I only skimmed through tests, not giving them
> as much attention as I should have
OK. I'd still want to see them get reviewed, though. Perhaps I'll
do so myself once I run out of things to do, but hopefully somebody
else gets there first ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Junio C Hamano @ 2016-12-19 21:01 UTC (permalink / raw)
To: Jonathan Tan
Cc: Kyle J. McKay, Jeff King, Johannes Schindelin, Git mailing list
In-Reply-To: <d5690ac7-ff62-99b9-7e7e-929bd7f0433b@google.com>
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?
^ permalink raw reply
* Re: [PATCH v2 10/11] worktree move: refuse to move worktrees with submodules
From: Stefan Beller @ 2016-12-19 20:57 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20161128094319.16176-11-pclouds@gmail.com>
On Mon, Nov 28, 2016 at 1:43 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> +
> + if (S_ISGITLINK(ce->ce_mode)) {
> + found_submodules = 1;
> + break;
> + }
While I applaud being careful with submodules, I think this may be a bit
overeager, because empty (both not initialized as well as not populated)
submodules are fine.
In origin/bw/grep-recurse-submodules^6 you find
int is_submodule_populated(const char *path)
that could be useful here, i.e. I'd imagine you'd change the condition to
if (S_ISGITLINK(ce->ce_mode)
&& !is_submodule_populated(ce->name)) {
...
I guess that can come as a fixup later though.
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Jonathan Tan @ 2016-12-19 20:54 UTC (permalink / raw)
To: Kyle J. McKay, Jeff King, Johannes Schindelin
Cc: Junio C Hamano, Git mailing list
In-Reply-To: <A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com>
On 12/19/2016 12:38 PM, Kyle J. McKay wrote:
> On Dec 19, 2016, at 12:03, Jeff King wrote:
>
>> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
>>
>>> 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(...))
Thanks for spotting this - I'm not sure how I missed that.
>> 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
>
> -or-
>
> b) Yeah, it could fail in the real world and it should "die" (and
> probably have a test added that triggers such death)
>
> -or-
>
> c) Actually, if check_header does return false we can keep going
> without problem
>
> -or-
>
> d) Actually, if check_header does return false we can keep going by
> making a minor change that should be in the patch
>
> I assume that since Jonathan added the code he will just know the answer
> as to which one it is and I won't have to rely on the results of my
> imaginary analysis. ;)
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".
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Junio C Hamano @ 2016-12-19 20:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: Josh Bleecher Snyder, vascomalmeida, git@vger.kernel.org
In-Reply-To: <xmqqk2avodi1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Comparing these changes that involve "\$variable" ...
>
> dashless=$(basename -- "$0" | sed -e 's/-/ /')
> usage() {
> - die "usage: $dashless $USAGE"
> + die "$(eval_gettext "usage: \$dashless \$USAGE")"
> ... and these that appear in the same patch ...
>
> @@ -190,13 +193,16 @@ cd_to_toplevel () {
> require_work_tree_exists () {
> if test "z$(git rev-parse --is-bare-repository)" != zfalse
> then
> - die "fatal: $0 cannot be used without a working tree."
> + program_name=$0
> + die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
> ...
>
> tells me that the latter needs to be eval_gettext?
Just for fun:
$ git grep -n '[^_]gettext .*\\\$'
shows three hits. Two of them are from that one.
The other is at git-rebase--interactive.sh:440
Perhaps like this to fix.
-- >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>
---
git-rebase--interactive.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 41fd374c72..96865b2375 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -437,7 +437,8 @@ update_squash_messages () {
}' <"$squash_msg".bak
} >"$squash_msg"
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
{
printf '%s\n' "$comment_char $(gettext "This is a combination of 2 commits.")"
^ permalink raw reply related
* Re: Bug report: $program_name in error message
From: Junio C Hamano @ 2016-12-19 20:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: Josh Bleecher Snyder, vascomalmeida, git@vger.kernel.org
In-Reply-To: <CAGZ79ka=RzAjrb=7u7p5xnveo=kcNCoGn=TC=0j-CBp8Oby7OA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> + Vasco Almeida, who authored d323c6b641,
> (i18n: git-sh-setup.sh: mark strings for translation, 2016-06-17)
Comparing these changes that involve "\$variable" ...
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c48139a494..2eda134800 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -2,6 +2,9 @@
# to set up some variables pointing at the normal git directories and
# a few helper shell functions.
+# Source git-sh-i18n for gettext support.
+. git-sh-i18n
+
# Having this variable in your environment would break scripts because
# you would cause "cd" to be taken to unexpected places. If you
# like CDPATH, define it for your interactive shell sessions without
@@ -83,16 +86,16 @@ if test -n "$OPTIONS_SPEC"; then
else
dashless=$(basename -- "$0" | sed -e 's/-/ /')
usage() {
- die "usage: $dashless $USAGE"
+ die "$(eval_gettext "usage: \$dashless \$USAGE")"
}
if [ -z "$LONG_USAGE" ]
then
- LONG_USAGE="usage: $dashless $USAGE"
+ LONG_USAGE="$(eval_gettext "usage: \$dashless \$USAGE")"
else
- LONG_USAGE="usage: $dashless $USAGE
+ LONG_USAGE="$(eval_gettext "usage: \$dashless \$USAGE
... and these that appear in the same patch ...
@@ -190,13 +193,16 @@ cd_to_toplevel () {
require_work_tree_exists () {
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
- die "fatal: $0 cannot be used without a working tree."
+ program_name=$0
+ die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
fi
}
require_work_tree () {
- test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
- die "fatal: $0 cannot be used without a working tree."
+ test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true || {
+ program_name=$0
+ die "$(gettext "fatal: \$program_name cannot be used without a working tree.")"
+ }
}
tells me that the latter needs to be eval_gettext?
^ permalink raw reply related
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Kyle J. McKay @ 2016-12-19 20:38 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin
Cc: Jonathan Tan, Junio C Hamano, Git mailing list
In-Reply-To: <20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net>
On Dec 19, 2016, at 12:03, Jeff King wrote:
> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
>
>> 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.
>>
>> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
>> ---
>>
>> Notes:
>> Please include this PATCH in 2.11.x maint
>
> 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
-or-
b) Yeah, it could fail in the real world and it should "die" (and
probably have a test added that triggers such death)
-or-
c) Actually, if check_header does return false we can keep going
without problem
-or-
d) Actually, if check_header does return false we can keep going by
making a minor change that should be in the patch
I assume that since Jonathan added the code he will just know the
answer as to which one it is and I won't have to rely on the results
of my imaginary analysis. ;)
On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
> ACK. I noticed this problem (and fixed it independently as a part of a
> huge patch series I did not get around to submit yet) while trying
> to get
> Git to build correctly with Visual C.
Does this mean that Dscho and I are the only ones who add -DNDEBUG for
release builds? Or are we just the only ones who actually run the
test suite on such builds?
--Kyle
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Jeff King @ 2016-12-19 20:03 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Jonathan Tan, Junio C Hamano, Git mailing list
In-Reply-To: <900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0>
On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
> 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.
>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
>
> Notes:
> Please include this PATCH in 2.11.x maint
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.
-Peff
^ permalink raw reply
* Re: [PATCH v1] t0021: fix flaky test
From: Jeff King @ 2016-12-19 20:00 UTC (permalink / raw)
To: Ramsay Jones; +Cc: larsxschneider, git, gitster
In-Reply-To: <6e0ea8b0-181a-2ab6-5bab-4f8dfa1d76fa@ramsayjones.plus.com>
On Mon, Dec 19, 2016 at 05:24:32PM +0000, Ramsay Jones wrote:
> > t0021.15 creates files, adds them to the index, and commits them. All
> > this usually happens in a test run within the same second and Git cannot
> > know if the files have been changed between `add` and `commit`. Thus,
> > Git has to run the clean filter in both operations. Sometimes these
> > invocations spread over two different seconds and Git can infer that the
> > files were not changed between `add` and `commit` based on their
> > modification timestamp. The test would fail as it expects the filter
> > invocation. Remove this expectation to make the test stable.
> [...]
> I applied this to the pu branch and ran the test by hand
> 48 times in a row without failure. (the most trials without
> error beforehand was 24).
The original also fails nearly-instantly under my stress script[1], and
it runs for several minutes with this patch.
It might be instructive to try all of the tests under that script, but
it would require a fair bit of patience (and to some degree, people
running "make -j32 test" accomplishes the same thing over time).
-Peff
[1] https://github.com/peff/git/blob/meta/stress
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Sixt @ 2016-12-19 19:34 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Pranit Bauva
In-Reply-To: <d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org>
Am 18.12.2016 um 16:37 schrieb Johannes Sixt:
> winansi.c is all about overriding MSVCRT's console handling. If we are
> connected to a console, then by the time isatty() is called (from
> outside the emulation layer), all handling of file descriptors 1 and 2
> is already outside MSVCRT's control. In particular, we have determined
> unambiguously whether a terminal is connected (see is_console()). I
> suggest to have the implementation below (on top of the patch I'm
> responding to).
>
> What do you think?
I thought a bit more about this approach, and I retract it. I think it
does not work when Git is connected to an MSYS TTY, i.e., when the
"console" is in reality the pipe that is detected in detect_msys_tty().
At the same time I wonder how your original winansi_isatty() could have
worked: In this case, MSVCRT's isatty() would return 1 (because
detect_msys_tty() has set things up that this happens), but then
winansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a
real Windows console. But it is not: it is a pipe. Am I missing something?
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index ba360be69b..1748d17777 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
>
> int winansi_isatty(int fd)
> {
> - int res = isatty(fd);
> -
> - if (res) {
> + switch (fd) {
> + case 0:
> /*
> * Make sure that /dev/null is not fooling Git into believing
> * that we are connected to a terminal, as "_isatty() returns a
> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)
> *
> * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
> */
> - HANDLE handle = winansi_get_osfhandle(fd);
> - if (fd == STDIN_FILENO) {
> + {
> + HANDLE handle = (HANDLE)_get_osfhandle(fd);
> DWORD dummy;
>
> - if (!GetConsoleMode(handle, &dummy))
> - res = 0;
> - } else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
> - CONSOLE_SCREEN_BUFFER_INFO dummy;
> -
> - if (!GetConsoleScreenBufferInfo(handle, &dummy))
> - res = 0;
> + return !!GetConsoleMode(handle, &dummy);
> }
> + case 1:
> + return !!hconsole1;
> + case 2:
> + return !!hconsole2;
> }
>
> - return res;
> + return isatty(fd);
> }
>
> void winansi_init(void)
>
^ permalink raw reply
* Re: Suggestion for the "Did you mean this?" feature
From: Kaartic Sivaraam @ 2016-12-19 19:24 UTC (permalink / raw)
To: Chris Packham; +Cc: GIT
In-Reply-To: <CAFOYHZDnpzdYq9j4-xGSdKZQX9deLBpZZhz209qV7cCtq537SA@mail.gmail.com>
Hello all,
On Sun, 18 December 2016 at 20:59, Alexei Lozovsky wrote,
> It's definitely a good thing for human users. For example, I am
> annoyed
> from time to time when I type in some long spell, mistype one minor
> thing,
> and the whole command fails. Then I need to press <up>, correct the
> obvious typo, and run the command again.
>
> Though, there is one aspect which may be the reason why git does not
> have
> this feature: it requires interactive input. For example, it won't
> work
> if some script tries to run an invalid git command. And git cannot
> really
> tell whether it is running interactively or in a batch mode. If it is
> running in batch mode then the whole script may hang indefinitely
> waiting
> for nonexistent input. This also may apply to using git with pipes.
>
> Maybe a configuration option or some GIT_NO_PROMPT environment
> variable
> may be used to force disable this, but it still will be a hassle for
> the
> scripts.
This is a good point that I didn't think of, sir. Thanks for bringing
it up. It seems that in some other form git does have the feature I was
suggesting.
On Mon, 2016-12-19 at 13:48 +1300, 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.
Thanks for bringing this to notice, sir. I wasn't aware of it before.
It's in essence the same feature.
On Mon, 2016-12-19 at 12:01 -0500, Marc Branchaud wrote:
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> ---
>
> 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'.
Happy that my mail introduced a little change to git by revealing a not
often used feature.
--
Regards,
Kaartic
^ permalink raw reply
* Re: [PATCH v2 32/34] sequencer (rebase -i): show the progress
From: Junio C Hamano @ 2016-12-19 19:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <9e48dffa8e58183debb79f29413afa81af174475.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The interactive rebase keeps the user informed about its progress.
> If the sequencer wants to do the grunt work of the interactive
> rebase, it also needs to show that progress.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
Sensible. This further adds two comparisons with TODO_COMMENT and
makes need for is_counted() or something like that felt more.
$ git grep TODO_COMMENT pu sequencer.c
shows that some places "x < TODO_COMMENT" is used while some other
places "y != TODO_COMMENT" is used, both for the same purpose, and
"z >= TODO_COMMENT" is also seen for the negation of the same.
I think all of them except for one can become is_counted() or
!is_counted() to convey what they want to check better while the one
used in the loop control:
for (i = 0; i < TODO_COMMENT; i++)
should probably become:
for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
as the parsing loop wants to check the input against all known
commands and there is no strong reason for that layer to know how
the insns are numbered. is_xxx() implementation can take advantage
of the way the insns are numbered, but there is no point spreading
the knowledge to higher layer in the callchain.
Other than that, all 34 patches looked sensible steps explained as a
coherent story that unfolds bit by bit, which was mostly a pleasant
read.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
From: Junio C Hamano @ 2016-12-19 19:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <b82347c627fd3b8a74827bc773a5df2d16c6dded.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> In the upcoming patch, we will support rebase -i's progress
> reporting. The progress skips comments but counts 'noop's.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
> diff --git a/sequencer.c b/sequencer.c
> index 1f314b2743..63f6f25ced 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -770,7 +770,9 @@ enum todo_command {
> TODO_EXEC,
> /* commands that do nothing but are counted for reporting progress */
> TODO_NOOP,
> - TODO_DROP
> + TODO_DROP,
> + /* comments (not counted for reporting progress) */
> + TODO_COMMENT
> };
>
> static struct {
Makes sense. I would have done this immediately after introducing
NOOP if I were doing this series, if only because by having the
unchanging last element early in enum {} definition, we can avoid
having to deal with the "last element cannot have comma", but that
is not a big issue.
> @@ -785,12 +787,13 @@ static struct {
> { 's', "squash" },
> { 'x', "exec" },
> { 0, "noop" },
> - { 'd', "drop" }
> + { 'd', "drop" },
> + { 0, NULL }
> };
>
> static const char *command_to_string(const enum todo_command command)
> {
> - if ((size_t)command < ARRAY_SIZE(todo_command_info))
> + if (command < TODO_COMMENT)
> return todo_command_info[command].str;
> die("Unknown command: %d", command);
> }
The same comment as "instead of comparing with TODO_NOOP, you would
want is_noop()" applies to three instances of comparing with
TODO_COMMENT we can see in this patch, I think.
"is_counted()" perhaps?
^ permalink raw reply
* Re: [PATCH v2 24/34] sequencer (rebase -i): respect strategy/strategy_opts settings
From: Junio C Hamano @ 2016-12-19 18:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <a21233f368f066408051e6bdc9a2b6ec513e9e11.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The sequencer already has an idea about using different merge
> strategies. We just piggy-back on top of that, using rebase -i's
> own settings, when running the sequencer in interactive rebase mode.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> sequencer.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
A handful of steps before and including this one look quite faithful
port to C from the scripted one.
Looking good.
^ 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