* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Jeff King @ 2017-01-07 1:31 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Johannes Schindelin, git
In-Reply-To: <87bmvjll8m.fsf@kyleam.com>
On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote:
> > The other option is just "git checkout --detach", which is also used in
> > the test suite. I tend to prefer it because it's a little more obvious
> > to a reader.
>
> True, that does seem clearer. Seems I should've waited a bit before
> sending out v2.
I think it's OK either way. Junio can also mark it up while applying,
too, if he has a preference.
-Peff
^ permalink raw reply
* [PATCH 3/3] execv_dashed_external: wait for child on signal death
From: Jeff King @ 2017-01-07 1:22 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170107011445.3e4fv6vdtimrwhgv@sigill.intra.peff.net>
When you hit ^C to interrupt a git command going to a pager,
this usually leaves the pager running. But when a dashed
external is in use, the pager ends up in a funny state and
quits (but only after eating one more character from the
terminal!). This fixes it.
Explaining the reason will require a little background.
When git runs a pager, it's important for the git process to
hang around and wait for the pager to finish, even though it
has no more data to feed it. This is because git spawns the
pager as a child, and thus the git process is the session
leader on the terminal. After it dies, the pager will finish
its current read from the terminal (eating the one
character), and then get EIO trying to read again.
When you hit ^C, that sends SIGINT to git and to the pager,
and it's a similar situation. The pager ignores it, but the
git process needs to hang around until the pager is done. We
addressed that long ago in a3da882120 (pager: do
wait_for_pager on signal death, 2009-01-22).
But when you have a dashed external (or an alias pointing to
a builtin, which will re-exec git for the builtin), there's
an extra process in the mix. For instance, running:
git -c alias.l=log log
will end up with a process tree like:
git (parent)
\
git-log (child)
\
less (pager)
If you hit ^C, SIGINT goes to all of them. The pager ignores
it, and the child git process will end up in wait_for_pager().
But the parent git process will die, and the usual EIO
trouble happens.
So we really want the parent git process to wait_for_pager(),
but of course it doesn't know anything about the pager at
all, since it was started by the child. However, we can
have it wait on the git-log child, which in turn is waiting
on the pager. And that's what this patch does.
There are a few design decisions here worth explaining:
1. The new feature is attached to run-command's
clean_on_exit feature. Partly this is convenience,
since that feature already has a signal handler that
deals with child cleanup.
But it's also a meaningful connection. The main reason
that dashed externals use clean_on_exit is to bind the
two processes together. If somebody kills the parent
with a signal, we propagate that to the child (in this
instance with SIGINT, we do propagate but it doesn't
matter because the original signal went to the whole
process group). Likewise, we do not want the parent
to go away until the child has done so.
In a traditional Unix world, we'd probably accomplish
this binding by just having the parent execve() the
child directly. But since that doesn't work on Windows,
everything goes through run_command's more spawn-like
interface.
2. We do _not_ automatically waitpid() on any
clean_on_exit children. For dashed externals this makes
sense; we know that the parent is doing nothing but
waiting for the child to exit anyway. But with other
children, it's possible that the child, after getting
the signal, could be waiting on the parent to do
something (like closing a descriptor). If we were to
wait on such a child, we'd end up in a deadlock. So
this errs on the side of caution, and lets callers
enable the feature explicitly.
3. When we send children the cleanup signal, we send all
the signals first, before waiting on any children. This
is to avoid the case where one child might be waiting
on another one to exit, causing a deadlock. We inform
all of them that it's time to die before reaping any.
In practice, there is only ever one dashed external run
from a given process, so this doesn't matter much now.
But it future-proofs us if other callers start using
the wait_after_clean mechanism.
There's no automated test here, because it would end up racy
and unportable. But it's easy to reproduce the situation by
running the log command given above and hitting ^C.
Signed-off-by: Jeff King <peff@peff.net>
---
git.c | 1 +
run-command.c | 19 +++++++++++++++++++
run-command.h | 1 +
3 files changed, 21 insertions(+)
diff --git a/git.c b/git.c
index bc2f2a7ec9..c8fe6637df 100644
--- a/git.c
+++ b/git.c
@@ -588,6 +588,7 @@ static void execv_dashed_external(const char **argv)
argv_array_pushf(&cmd.args, "git-%s", argv[0]);
argv_array_pushv(&cmd.args, argv + 1);
cmd.clean_on_exit = 1;
+ cmd.wait_after_clean = 1;
cmd.silent_exec_failure = 1;
trace_argv_printf(cmd.args.argv, "trace: exec:");
diff --git a/run-command.c b/run-command.c
index ca905a9e80..73bfba7ef9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
static void cleanup_children(int sig, int in_signal)
{
+ struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,23 @@ static void cleanup_children(int sig, int in_signal)
}
kill(p->pid, sig);
+
+ if (p->process->wait_after_clean) {
+ p->next = children_to_wait_for;
+ children_to_wait_for = p;
+ } else {
+ if (!in_signal)
+ free(p);
+ }
+ }
+
+ while (children_to_wait_for) {
+ struct child_to_clean *p = children_to_wait_for;
+ children_to_wait_for = p->next;
+
+ while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+ ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}
diff --git a/run-command.h b/run-command.h
index dd1c78c28d..4fa8f65adb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,7 @@ struct child_process {
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
unsigned clean_on_exit:1;
+ unsigned wait_after_clean:1;
void (*clean_on_exit_handler)(struct child_process *process);
void *clean_on_exit_handler_cbdata;
};
--
2.11.0.527.gfef230ca76
^ permalink raw reply related
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Kyle Meyer @ 2017-01-07 1:19 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170107011138.uuy6ob234kyy3y4e@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:
>
>> > $ git grep -c HEAD^{} junio/pu -- t/
>> > junio/pu:t/t3200-branch.sh:3
>> >
>> > Maybe use HEAD^0 just for consistency?
>>
>> Yes, thanks for pointing that out.
>
> The other option is just "git checkout --detach", which is also used in
> the test suite. I tend to prefer it because it's a little more obvious
> to a reader.
True, that does seem clearer. Seems I should've waited a bit before
sending out v2.
--
Kyle
^ permalink raw reply
* [PATCH 2/3] execv_dashed_external: stop exiting with negative code
From: Jeff King @ 2017-01-07 1:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170107011445.3e4fv6vdtimrwhgv@sigill.intra.peff.net>
When we try to exec a git sub-command, we pass along the
status code from run_command(). But that may return -1 if we
ran into an error with pipe() or execve(). This tends to
work (and end up as 255 due to twos-complement wraparound
and truncation), but in general it's probably a good idea to
avoid negative exit codes for portability.
We can easily translate to the normal generic "128" code we
get when syscalls cause us to die.
Signed-off-by: Jeff King <peff@peff.net>
---
I know that negative exit codes were a problem once upon a time on
Windows, but I think that is fine since 47e3de0e79 (MinGW: truncate
exit()'s argument to lowest 8 bits, 2009-07-05). Still, I think it's a
weird portability thing that we are better off avoiding (and certainly I
wouldn't be surprised if some callers assume everything >128 is a
signal).
git.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/git.c b/git.c
index d0e04d5c97..bc2f2a7ec9 100644
--- a/git.c
+++ b/git.c
@@ -593,12 +593,16 @@ static void execv_dashed_external(const char **argv)
trace_argv_printf(cmd.args.argv, "trace: exec:");
/*
- * if we fail because the command is not found, it is
- * OK to return. Otherwise, we just pass along the status code.
+ * If we fail because the command is not found, it is
+ * OK to return. Otherwise, we just pass along the status code,
+ * or our usual generic code if we were not even able to exec
+ * the program.
*/
status = run_command(&cmd);
- if (status >= 0 || errno != ENOENT)
+ if (status >= 0)
exit(status);
+ else if (errno != ENOENT)
+ exit(128);
}
static int run_argv(int *argcp, const char ***argv)
--
2.11.0.527.gfef230ca76
^ permalink raw reply related
* [PATCH 1/3] execv_dashed_external: use child_process struct
From: Jeff King @ 2017-01-07 1:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170107011445.3e4fv6vdtimrwhgv@sigill.intra.peff.net>
When we run a dashed external, we use the one-liner
run_command_v_opt() to do so. Let's switch to using a
child_process struct, which has two advantages:
1. We can drop all of the allocation and cleanup code for
building our custom argv array, and just rely on the
builtin argv_array (at the minor cost of doing a few
extra mallocs).
2. We have access to the complete range of child_process
options, not just the ones that the "_opt()" form can
forward.
Signed-off-by: Jeff King <peff@peff.net>
---
It's possible people may disagree with reason (2), and we should add a
new RUN_WAIT_AFTER_CLEAN flag in the final patch. IMHO the whole
run_command_v_opt() thing is a good sign that you should be switching to
a child_process struct. :)
git.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/git.c b/git.c
index dce529fcbf..d0e04d5c97 100644
--- a/git.c
+++ b/git.c
@@ -575,8 +575,7 @@ static void handle_builtin(int argc, const char **argv)
static void execv_dashed_external(const char **argv)
{
- struct strbuf cmd = STRBUF_INIT;
- const char *tmp;
+ struct child_process cmd = CHILD_PROCESS_INIT;
int status;
if (get_super_prefix())
@@ -586,30 +585,20 @@ static void execv_dashed_external(const char **argv)
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
- strbuf_addf(&cmd, "git-%s", argv[0]);
+ argv_array_pushf(&cmd.args, "git-%s", argv[0]);
+ argv_array_pushv(&cmd.args, argv + 1);
+ cmd.clean_on_exit = 1;
+ cmd.silent_exec_failure = 1;
- /*
- * argv[0] must be the git command, but the argv array
- * belongs to the caller, and may be reused in
- * subsequent loop iterations. Save argv[0] and
- * restore it on error.
- */
- tmp = argv[0];
- argv[0] = cmd.buf;
-
- trace_argv_printf(argv, "trace: exec:");
+ trace_argv_printf(cmd.args.argv, "trace: exec:");
/*
* if we fail because the command is not found, it is
* OK to return. Otherwise, we just pass along the status code.
*/
- status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
+ status = run_command(&cmd);
if (status >= 0 || errno != ENOENT)
exit(status);
-
- argv[0] = tmp;
-
- strbuf_release(&cmd);
}
static int run_argv(int *argcp, const char ***argv)
--
2.11.0.527.gfef230ca76
^ permalink raw reply related
* [PATCH 0/3] fix ^C killing pager when running alias
From: Jeff King @ 2017-01-07 1:14 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106232042.ptn6grtll5wpxhc4@sigill.intra.peff.net>
On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
> > In general, I think it is wrong to wait for child processes when a signal
> > was received. After all, it is the purpose of a (deadly) signal to have the
> > process go away. There may be programs that know it better, like less, but
> > git should not attempt to know better in general.
> >
> > We do apply some special behavior for certain cases like we do for the
> > pager. And now the case with aliases is another special situation. The
> > parent git process only delegates to the child, and as such it is reasonable
> > that it binds its life time to the first child, which executes the expanded
> > alias.
>
> Yeah, I think I agree. That binding is something you want in many cases,
> but not necessarily all. The original purpose of clean_on_exit was to
> create a binding like that, but of course it can be (and with the
> smudge-filter stuff, arguably has been) used for other cases, too.
>
> I'll work up a patch that makes it a separate option, which should be
> pretty easy.
Yeah, this did turn out to be really easy. I spent most of the time
trying to explain the issue in the commit message in a sane way.
Hopefully it didn't end up _too_ long. :)
The interesting bit is in the third one. The first is a necessary
preparatory step, and the second is a cleanup I noticed in the
neighborhood.
[1/3]: execv_dashed_external: use child_process struct
[2/3]: execv_dashed_external: stop exiting with negative code
[3/3]: execv_dashed_external: wait for child on signal death
git.c | 36 +++++++++++++++---------------------
run-command.c | 19 +++++++++++++++++++
run-command.h | 1 +
3 files changed, 35 insertions(+), 21 deletions(-)
-Peff
^ permalink raw reply
* [PATCH v2] branch_get_push: do not segfault when HEAD is detached
From: Kyle Meyer @ 2017-01-07 1:12 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Kyle Meyer
In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com>
Move the detached HEAD check from branch_get_push_1() to
branch_get_push() to avoid setting branch->push_tracking_ref when
branch is NULL.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
remote.c | 6 +++---
t/t1514-rev-parse-push.sh | 6 ++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index ad6c5424e..d5eaec737 100644
--- a/remote.c
+++ b/remote.c
@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
{
struct remote *remote;
- if (!branch)
- return error_buf(err, _("HEAD does not point to a branch"));
-
remote = remote_get(pushremote_for_branch(branch, NULL));
if (!remote)
return error_buf(err,
@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
const char *branch_get_push(struct branch *branch, struct strbuf *err)
{
+ if (!branch)
+ return error_buf(err, _("HEAD does not point to a branch"));
+
if (!branch->push_tracking_ref)
branch->push_tracking_ref = branch_get_push_1(branch, err);
return branch->push_tracking_ref;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 7214f5b33..623a32aa6 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
resolve topic@{push} refs/remotes/origin/magic/topic
'
+test_expect_success 'resolving @{push} fails with a detached HEAD' '
+ git checkout HEAD^0 &&
+ test_when_finished "git checkout -" &&
+ test_must_fail git rev-parse @{push}
+'
+
test_done
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Jeff King @ 2017-01-07 1:11 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Johannes Schindelin, git
In-Reply-To: <87inprllpv.fsf@kyleam.com>
On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:
> > $ git grep -c HEAD^{} junio/pu -- t/
> > junio/pu:t/t3200-branch.sh:3
> >
> > Maybe use HEAD^0 just for consistency?
>
> Yes, thanks for pointing that out.
The other option is just "git checkout --detach", which is also used in
the test suite. I tend to prefer it because it's a little more obvious
to a reader.
-Peff
^ permalink raw reply
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Kyle Meyer @ 2017-01-07 1:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1701061438300.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
[...]
>> +test_expect_success 'resolving @{push} fails with a detached HEAD' '
>> + git checkout HEAD^{} &&
>
> I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing
> scripts seem to agree with me:
>
> $ git grep -c HEAD^0 junio/pu -- t/
> junio/pu:t/t1450-fsck.sh:1
> junio/pu:t/t1507-rev-parse-upstream.sh:2
> junio/pu:t/t2020-checkout-detach.sh:5
> junio/pu:t/t3200-branch.sh:1
> junio/pu:t/t3203-branch-output.sh:3
> junio/pu:t/t3400-rebase.sh:1
> junio/pu:t/t3404-rebase-interactive.sh:1
> junio/pu:t/t5407-post-rewrite-hook.sh:2
> junio/pu:t/t5505-remote.sh:1
> junio/pu:t/t5510-fetch.sh:1
> junio/pu:t/t5533-push-cas.sh:3
> junio/pu:t/t6035-merge-dir-to-symlink.sh:3
> junio/pu:t/t7201-co.sh:2
> junio/pu:t/t7402-submodule-rebase.sh:1
> junio/pu:t/t9105-git-svn-commit-diff.sh:1
> junio/pu:t/t9107-git-svn-migrate.sh:1
>
> $ git grep -c HEAD^{} junio/pu -- t/
> junio/pu:t/t3200-branch.sh:3
>
> Maybe use HEAD^0 just for consistency?
Yes, thanks for pointing that out.
--
Kyle
^ permalink raw reply
* Re: [PATCH] submodule update --init: displays correct path from submodule
From: Stefan Beller @ 2017-01-07 1:01 UTC (permalink / raw)
To: David Turner; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <1483750501.31165.9.camel@frank>
On Fri, Jan 6, 2017 at 4:55 PM, David Turner <novalis@novalis.org> wrote:
> (for the test)
> Signed-off-by: David Turner <dturner@twosigma.com>
>
> TIL: super-prefix!
yeah that was introduced recently for prefixes from "outside the repository"
e.g. a superproject, its first user is grep --recurse-submodules.
Also I realize I have set diff.context to 15 as I was experimenting
with it earlier,
and forgot to turn it off. So a lot of context in the diff for the patch.
Thanks for the bug report,
Stefan
^ permalink raw reply
* Re: [PATCH] submodule update --init: displays correct path from submodule
From: David Turner @ 2017-01-07 0:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git
In-Reply-To: <20170107001953.3196-1-sbeller@google.com>
(for the test)
Signed-off-by: David Turner <dturner@twosigma.com>
TIL: super-prefix!
On Fri, 2017-01-06 at 16:19 -0800, Stefan Beller wrote:
> In the submodule helper we did not correctly handled the display path
> for initializing submodules when both the submodule is inside a
> subdirectory as well as the command being invoked from a subdirectory
> (as viewed from the superproject).
>
> This was broken in 3604242f080, which was written at a time where
> there was no super-prefix available, so we abused the --prefix option
> for the same purpose and could get only one case right (the call from
> within a subdirectory, not the submodule being in a subdirectory).
>
> Test-provided-by: David Turner <novalis@novalis.org>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> applies on sb/submodule-embed-gitdir as that contains 89c862655
> (submodule helper: support super prefix)
>
> builtin/submodule--helper.c | 13 +++++++------
> git-submodule.sh | 2 +-
> t/t7406-submodule-update.sh | 17 +++++++++++++++++
> 3 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 242d9911a6..7b3f9fc293 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -305,32 +305,36 @@ static int module_list(int argc, const char **argv, const char *prefix)
>
> utf8_fprintf(stdout, "%s\n", ce->name);
> }
> return 0;
> }
>
> static void init_submodule(const char *path, const char *prefix, int quiet)
> {
> const struct submodule *sub;
> struct strbuf sb = STRBUF_INIT;
> char *upd = NULL, *url = NULL, *displaypath;
>
> /* Only loads from .gitmodules, no overlay with .git/config */
> gitmodules_config();
>
> - if (prefix) {
> - strbuf_addf(&sb, "%s%s", prefix, path);
> + if (prefix && get_super_prefix())
> + die("BUG: cannot have prefix and superprefix");
> + else if (prefix)
> + displaypath = xstrdup(relative_path(path, prefix, &sb));
> + else if (get_super_prefix()) {
> + strbuf_addf(&sb, "%s%s", get_super_prefix(), path);
> displaypath = strbuf_detach(&sb, NULL);
> } else
> displaypath = xstrdup(path);
>
> sub = submodule_from_path(null_sha1, path);
>
> if (!sub)
> die(_("No url found for submodule path '%s' in .gitmodules"),
> displaypath);
>
> /*
> * Copy url setting when it is not set yet.
> * To look up the url in .git/config, we must not fall back to
> * .gitmodules, so look it up directly.
> */
> @@ -391,33 +395,30 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> }
> strbuf_release(&sb);
> free(displaypath);
> free(url);
> free(upd);
> }
>
> static int module_init(int argc, const char **argv, const char *prefix)
> {
> struct pathspec pathspec;
> struct module_list list = MODULE_LIST_INIT;
> int quiet = 0;
> int i;
>
> struct option module_init_options[] = {
> - OPT_STRING(0, "prefix", &prefix,
> - N_("path"),
> - N_("alternative anchor for relative paths")),
> OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> OPT_END()
> };
>
> const char *const git_submodule_helper_usage[] = {
> N_("git submodule--helper init [<path>]"),
> NULL
> };
>
> argc = parse_options(argc, argv, prefix, module_init_options,
> git_submodule_helper_usage, 0);
>
> if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> return 1;
>
> @@ -1117,31 +1118,31 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>
> struct cmd_struct {
> const char *cmd;
> int (*fn)(int, const char **, const char *);
> unsigned option;
> };
>
> static struct cmd_struct commands[] = {
> {"list", module_list, 0},
> {"name", module_name, 0},
> {"clone", module_clone, 0},
> {"update-clone", update_clone, 0},
> {"relative-path", resolve_relative_path, 0},
> {"resolve-relative-url", resolve_relative_url, 0},
> {"resolve-relative-url-test", resolve_relative_url_test, 0},
> - {"init", module_init, 0},
> + {"init", module_init, SUPPORT_SUPER_PREFIX},
> {"remote-branch", resolve_remote_submodule_branch, 0},
> {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> };
>
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> {
> int i;
> if (argc < 2)
> die(_("submodule--helper subcommand must be "
> "called with a subcommand"));
>
> for (i = 0; i < ARRAY_SIZE(commands); i++) {
> if (!strcmp(argv[1], commands[i].cmd)) {
> if (get_super_prefix() &&
> !(commands[i].option & SUPPORT_SUPER_PREFIX))
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9285b5c43d..4e47ff8ad8 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -362,31 +362,31 @@ cmd_init()
> ;;
> --)
> shift
> break
> ;;
> -*)
> usage
> ;;
> *)
> break
> ;;
> esac
> shift
> done
>
> - git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} "$@"
> + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} "$@"
> }
>
> #
> # Unregister submodules from .git/config and remove their work tree
> #
> cmd_deinit()
> {
> # parse $args after "submodule ... deinit".
> deinit_all=
> while test $# -ne 0
> do
> case "$1" in
> -f|--force)
> force=$1
> ;;
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 64f322c4cc..725bbed1f8 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -128,30 +128,47 @@ done.
> Cloning into '$pwd/recursivesuper/super/submodule'...
> done.
> EOF
>
> test_expect_success 'submodule update --init --recursive from subdirectory' '
> git -C recursivesuper/super reset --hard HEAD^ &&
> (cd recursivesuper &&
> mkdir tmp &&
> cd tmp &&
> git submodule update --init --recursive ../super >../../actual 2>../../actual2
> ) &&
> test_i18ncmp expect actual &&
> test_i18ncmp expect2 actual2
> '
>
> +cat <<EOF >expect2
> +Submodule 'foo/sub' ($pwd/withsubs/../rebasing) registered for path 'sub'
> +EOF
> +
> +test_expect_success 'submodule update --init from and of subdirectory' '
> + git init withsubs &&
> + (cd withsubs &&
> + mkdir foo &&
> + git submodule add "$(pwd)/../rebasing" foo/sub &&
> + (cd foo &&
> + git submodule deinit -f sub &&
> + git submodule update --init sub 2>../../actual2
> + )
> + ) &&
> + test_i18ncmp expect2 actual2
> +'
> +
> apos="'";
> test_expect_success 'submodule update does not fetch already present commits' '
> (cd submodule &&
> echo line3 >> file &&
> git add file &&
> test_tick &&
> git commit -m "upstream line3"
> ) &&
> (cd super/submodule &&
> head=$(git rev-parse --verify HEAD) &&
> echo "Submodule path ${apos}submodule$apos: checked out $apos$head$apos" > ../../expected &&
> git reset --hard HEAD~1
> ) &&
> (cd super &&
> git submodule update > ../actual 2> ../actual.err
^ permalink raw reply
* [PATCH] submodule update --init: displays correct path from submodule
From: Stefan Beller @ 2017-01-07 0:19 UTC (permalink / raw)
To: gitster; +Cc: novalis, git, Stefan Beller
In the submodule helper we did not correctly handled the display path
for initializing submodules when both the submodule is inside a
subdirectory as well as the command being invoked from a subdirectory
(as viewed from the superproject).
This was broken in 3604242f080, which was written at a time where
there was no super-prefix available, so we abused the --prefix option
for the same purpose and could get only one case right (the call from
within a subdirectory, not the submodule being in a subdirectory).
Test-provided-by: David Turner <novalis@novalis.org>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
applies on sb/submodule-embed-gitdir as that contains 89c862655
(submodule helper: support super prefix)
builtin/submodule--helper.c | 13 +++++++------
git-submodule.sh | 2 +-
t/t7406-submodule-update.sh | 17 +++++++++++++++++
3 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 242d9911a6..7b3f9fc293 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -305,32 +305,36 @@ static int module_list(int argc, const char **argv, const char *prefix)
utf8_fprintf(stdout, "%s\n", ce->name);
}
return 0;
}
static void init_submodule(const char *path, const char *prefix, int quiet)
{
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
- if (prefix) {
- strbuf_addf(&sb, "%s%s", prefix, path);
+ if (prefix && get_super_prefix())
+ die("BUG: cannot have prefix and superprefix");
+ else if (prefix)
+ displaypath = xstrdup(relative_path(path, prefix, &sb));
+ else if (get_super_prefix()) {
+ strbuf_addf(&sb, "%s%s", get_super_prefix(), path);
displaypath = strbuf_detach(&sb, NULL);
} else
displaypath = xstrdup(path);
sub = submodule_from_path(null_sha1, path);
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
displaypath);
/*
* Copy url setting when it is not set yet.
* To look up the url in .git/config, we must not fall back to
* .gitmodules, so look it up directly.
*/
@@ -391,33 +395,30 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
}
strbuf_release(&sb);
free(displaypath);
free(url);
free(upd);
}
static int module_init(int argc, const char **argv, const char *prefix)
{
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
int i;
struct option module_init_options[] = {
- OPT_STRING(0, "prefix", &prefix,
- N_("path"),
- N_("alternative anchor for relative paths")),
OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
OPT_END()
};
const char *const git_submodule_helper_usage[] = {
N_("git submodule--helper init [<path>]"),
NULL
};
argc = parse_options(argc, argv, prefix, module_init_options,
git_submodule_helper_usage, 0);
if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
return 1;
@@ -1117,31 +1118,31 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
unsigned option;
};
static struct cmd_struct commands[] = {
{"list", module_list, 0},
{"name", module_name, 0},
{"clone", module_clone, 0},
{"update-clone", update_clone, 0},
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
- {"init", module_init, 0},
+ {"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
{
int i;
if (argc < 2)
die(_("submodule--helper subcommand must be "
"called with a subcommand"));
for (i = 0; i < ARRAY_SIZE(commands); i++) {
if (!strcmp(argv[1], commands[i].cmd)) {
if (get_super_prefix() &&
!(commands[i].option & SUPPORT_SUPER_PREFIX))
diff --git a/git-submodule.sh b/git-submodule.sh
index 9285b5c43d..4e47ff8ad8 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -362,31 +362,31 @@ cmd_init()
;;
--)
shift
break
;;
-*)
usage
;;
*)
break
;;
esac
shift
done
- git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} "$@"
+ git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} "$@"
}
#
# Unregister submodules from .git/config and remove their work tree
#
cmd_deinit()
{
# parse $args after "submodule ... deinit".
deinit_all=
while test $# -ne 0
do
case "$1" in
-f|--force)
force=$1
;;
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c4cc..725bbed1f8 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -128,30 +128,47 @@ done.
Cloning into '$pwd/recursivesuper/super/submodule'...
done.
EOF
test_expect_success 'submodule update --init --recursive from subdirectory' '
git -C recursivesuper/super reset --hard HEAD^ &&
(cd recursivesuper &&
mkdir tmp &&
cd tmp &&
git submodule update --init --recursive ../super >../../actual 2>../../actual2
) &&
test_i18ncmp expect actual &&
test_i18ncmp expect2 actual2
'
+cat <<EOF >expect2
+Submodule 'foo/sub' ($pwd/withsubs/../rebasing) registered for path 'sub'
+EOF
+
+test_expect_success 'submodule update --init from and of subdirectory' '
+ git init withsubs &&
+ (cd withsubs &&
+ mkdir foo &&
+ git submodule add "$(pwd)/../rebasing" foo/sub &&
+ (cd foo &&
+ git submodule deinit -f sub &&
+ git submodule update --init sub 2>../../actual2
+ )
+ ) &&
+ test_i18ncmp expect2 actual2
+'
+
apos="'";
test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
echo line3 >> file &&
git add file &&
test_tick &&
git commit -m "upstream line3"
) &&
(cd super/submodule &&
head=$(git rev-parse --verify HEAD) &&
echo "Submodule path ${apos}submodule$apos: checked out $apos$head$apos" > ../../expected &&
git reset --hard HEAD~1
) &&
(cd super &&
git submodule update > ../actual 2> ../actual.err
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 23:20 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <2ed6f78b-7704-c724-c99b-e310c383c4e8@kdbg.org>
On Fri, Jan 06, 2017 at 11:42:25PM +0100, Johannes Sixt wrote:
> > So I dunno. Maybe this waiting should be restricted only to certain
> > cases like executing git sub-commands.
>
> If given it some thought.
>
> In general, I think it is wrong to wait for child processes when a signal
> was received. After all, it is the purpose of a (deadly) signal to have the
> process go away. There may be programs that know it better, like less, but
> git should not attempt to know better in general.
>
> We do apply some special behavior for certain cases like we do for the
> pager. And now the case with aliases is another special situation. The
> parent git process only delegates to the child, and as such it is reasonable
> that it binds its life time to the first child, which executes the expanded
> alias.
Yeah, I think I agree. That binding is something you want in many cases,
but not necessarily all. The original purpose of clean_on_exit was to
create a binding like that, but of course it can be (and with the
smudge-filter stuff, arguably has been) used for other cases, too.
I'll work up a patch that makes it a separate option, which should be
pretty easy.
-Peff
^ permalink raw reply
* Git filesystem case-insensitive to case-sensitive system broken
From: Steven Robertson @ 2017-01-06 21:56 UTC (permalink / raw)
To: git
Hello,
I was doing development on a linux box on AWS, when we found a code
bug that had me switching to running the code on a Mac instead. We
discovered that we had accidentally named two files the same when
looked at case-insensitively, which made git commands afterwards
display the wrong thing. It looked like this (ignoring some things
that aren't relevant):
$ git status
modified: tests/test_system/show_19_L.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ git checkout tests/test_system/show_19_L.txt
$ git status
modified: tests/test_system/show_19_l.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ git checkout tests/test_system/show_19_l.txt
$ git status
modified: tests/test_system/show_19_L.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ diff tests/test_system/show_19_L.txt tests/test_system/show_19_l.txt
$
Those two files are different in our repo, and as such git thinks that
we modified one of them when we try and pull it down from github
again.
Thanks for looking at this!
-- Steven
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Johannes Sixt @ 2017-01-06 22:42 UTC (permalink / raw)
To: Jeff King; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106194115.k5u5esv7t63mryvk@sigill.intra.peff.net>
Am 06.01.2017 um 20:41 schrieb Jeff King:
> On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:
>
>>> diff --git a/run-command.c b/run-command.c
>>> index ca905a9e80..db47c429b7 100644
>>> --- a/run-command.c
>>> +++ b/run-command.c
>>> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>>>
>>> static void cleanup_children(int sig, int in_signal)
>>> {
>>> + struct child_to_clean *children_to_wait_for = NULL;
>>> +
>>> while (children_to_clean) {
>>> struct child_to_clean *p = children_to_clean;
>>> children_to_clean = p->next;
>>> @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
>>> }
>>>
>>> kill(p->pid, sig);
>>> + p->next = children_to_wait_for;
>>> + children_to_wait_for = p;
>>> + }
>>> +
>>> + while (children_to_wait_for) {
>>> + struct child_to_clean *p = children_to_wait_for;
>>> + children_to_wait_for = p->next;
>>> +
>>> + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
>>> + ; /* spin waiting for process exit or error */
>>> +
>>> if (!in_signal)
>>> free(p);
>>> }
>>>
>>
>> This looks like the minimal change necessary. I wonder, though, whether the
>> new local variable is really required. Wouldn't it be sufficient to walk the
>> children_to_clean chain twice?
>
> Yeah, I considered that. The fact that we disassemble the list in the
> first loop has two side effects:
>
> 1. It lets us free the list as we go (for the !in_signal case).
>
> 2. If we were to get another signal, it makes us sort-of reentrant. We
> will only kill and wait for each pid once.
>
> Obviously (1) moves down to the lower loop, but I was trying to preserve
> (2). I'm not sure if it is worth bothering, though.
Makes sense.
> The way we pull
> items off of the list is certainly not atomic (it does shorten the race
> to a few instructions, though, versus potentially waiting on waitpid()
> to return).
>
> My bigger concern with the whole thing is whether we could hit some sort
> of deadlock if the child doesn't die when we send it a signal. E.g.,
> imagine we have a pipe open to the child and somebody sends SIGTERM to
> us. We propagate SIGTERM to the child, and then waitpid() for it. The
> child decides to ignore our SIGTERM for some reason and keep reading
> until EOF on the pipe. It won't ever get it, and the two processes will
> hang forever.
>
> You can argue perhaps that the child is broken in that case. And I doubt
> this could trigger when running a git sub-command. But we may add more
> children in the future. Right now we use it for the new multi-file
> clean/smudge filters. They use the hook feature to close the
> descriptors, but note that that won't run in the in_signal case.
>
> So I dunno. Maybe this waiting should be restricted only to certain
> cases like executing git sub-commands.
If given it some thought.
In general, I think it is wrong to wait for child processes when a
signal was received. After all, it is the purpose of a (deadly) signal
to have the process go away. There may be programs that know it better,
like less, but git should not attempt to know better in general.
We do apply some special behavior for certain cases like we do for the
pager. And now the case with aliases is another special situation. The
parent git process only delegates to the child, and as such it is
reasonable that it binds its life time to the first child, which
executes the expanded alias.
-- Hannes
^ permalink raw reply
* minor bug: git submodule update --init from a subdir shows wrong path
From: David Turner @ 2017-01-06 21:31 UTC (permalink / raw)
To: git, sbeller
[-- Attachment #1: Type: text/plain, Size: 411 bytes --]
I believe (from bisection) that this was introduced in the transition to
C at 3604242f080.
I've attached a repro (against master). At the time the bug was
introduced, the output in question went to stdout instead of stderr, so
the attached test case will not quite work on the older version. But if
you run under -v, you'll be able to see the bad ("foo/foo/sub" instead
of "foo/sub" or just "sub") output.
[-- Attachment #2: wrong-submodule-path.patch --]
[-- Type: text/x-patch, Size: 917 bytes --]
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..e1deb17 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -140,6 +140,23 @@ test_expect_success 'submodule update --init --recursive from subdirectory' '
test_i18ncmp expect2 actual2
'
+cat <<EOF >expect2
+Submodule 'foo/sub' (/home/novalis/twosigma/git/t/trash directory.t7406-submodule-update/withsubs/../rebasing) registered for path 'foo/sub'
+EOF
+
+test_expect_success 'submodule update --init from and of subdirectory' '
+ git init withsubs &&
+ (cd withsubs &&
+ mkdir foo &&
+ git submodule add "$(pwd)/../rebasing" foo/sub &&
+ (cd foo &&
+ git submodule deinit -f sub &&
+ git submodule update --init sub 2>../../actual2
+ )
+ ) &&
+ test_i18ncmp expect2 actual2
+'
+
apos="'";
test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
^ permalink raw reply related
* Re: Rebasing a branch with merges
From: Philip Oakley @ 2017-01-06 21:28 UTC (permalink / raw)
To: Robert Dailey, Git; +Cc: Johannes Schindelin
In-Reply-To: <CAHd499BREpaHHyN89a1HchyJiQzPpdo3NSfoLLGVONEmX1m19g@mail.gmail.com>
From: "Robert Dailey" <rcdailey.lists@gmail.com>
> Here's the scenario:
>
> I create a topic branch so one other developer and myself can work on
> a feature that takes 2 weeks to complete. During that 2 week period,
> changes are occurring on master that I need in my topic branch. Since
> I have a collaborator on the branch, I opt for merges instead of
> rebase.
>
> Each day I merge from master to the topic branch, which changes code
> I'm actively working in and requires semantic changes (functions
> renamed, moved, etc).
>
> Once I'm ready to merge the topic branch back into master, I have two
> options (bearing in mind the goal is to keep history as clean as
> possible. Furthermore this implies that the constant merging into
> topic from master has made the topic branch look unwieldy and
> difficult to audit):
a broader question zero;
0. Is the merge always clean? Do you always do a preparatory fixup! to
ensure that the merge will be clean?
Ensuring that the merge will be clean should greatly simplify your decision
about process.
>
> 1. Do a squash merge, which keeps history clean but we lose context
> for the important bits (the commits representing units of work that
> contribute to the topic itself).
>
> 2. Do a final rebase prior to merging.
>
> #2 doesn't seem to be possible due to patch ordering. For example, if
> I have real commits after merge commits that depend on those changes
> from master being present as a base at that point in time, the rebase
> will cause the patch before it to no longer include those changes from
> master.
How much of the historic fixups to cover changes on master do you want to
keep visible? i.e. how many fork-points are truly needed (a. by you, b. by
the project - personal knowledge vs corporate knowledge).?
>
> Is there a mechanism to rebase in this situation to both achieve a
> clean, linear history for the topic branch and allow fast forward
> merging if desired, while still not causing superfluous conflicts due
> to the merges being omitted during the rebase?
>
> Thanks in advance for any advice in this scenario.
>
Have you looked at @dscho's garden-shears scripts that he uses on
Git-for-Windows as he has to continuously rebase the Windows specific
patches on top of the progressing Git master? Very similar issues ;-)
https://github.com/git-for-windows/build-extra/blob/master/shears.sh
https://blogs.msdn.microsoft.com/visualstudioalm/2016/09/03/whats-new-in-git-for-windows-2-10/
(#Fun Facts)
--
Philip
^ permalink raw reply
* [PATCH 2/5] unpack-trees: remove unneeded continue
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
The continue is the last statement in the loop, so not needed.
This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
read-tree -u remove index entries outside sparse area) when statements
after the continue were removed.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 55c75b4d6a..f16ef14294 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -272,31 +272,30 @@ static int check_updates(struct unpack_trees_options *o)
progress = start_progress_delay(_("Checking out files"),
total, 50, 1);
cnt = 0;
}
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
- continue;
}
}
remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 3/5] unpack-trees: factor progress setup out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f16ef14294..971d091fd0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,55 +237,63 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ struct index_state *index = &o->result;
+
+ if (!o->update || !o->verbose_update)
+ return NULL;
+
+ for (; cnt < index->cache_nr; cnt++) {
+ const struct cache_entry *ce = index->cache[cnt];
+ if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+ total++;
+ }
+
+ return start_progress_delay(_("Checking out files"),
+ total, 50, 1);
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ unsigned cnt = 0;
int i, errs = 0;
struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
- if (o->update && o->verbose_update) {
- for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
- const struct cache_entry *ce = index->cache[cnt];
- if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
- total++;
- }
-
- progress = start_progress_delay(_("Checking out files"),
- total, 50, 1);
- cnt = 0;
- }
+ progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
}
}
remove_marked_cache_entries(index);
remove_scheduled_dirs();
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 4/5] unpack-trees: factor file removal out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 971d091fd0..b954ec1233 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,30 +237,50 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+static int remove_workingtree_files(struct unpack_trees_options *o,
+ struct progress *progress)
+{
+ int i;
+ unsigned cnt = 0;
+ struct index_state *index = &o->result;
+
+ for (i = 0; i < index->cache_nr; i++) {
+ const struct cache_entry *ce = index->cache[i];
+
+ if (ce->ce_flags & CE_WT_REMOVE) {
+ display_progress(progress, ++cnt);
+ if (o->update && !o->dry_run)
+ unlink_entry(ce);
+ }
+ }
+
+ return cnt;
+}
+
static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
struct index_state *index = &o->result;
if (!o->update || !o->verbose_update)
return NULL;
for (; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
return start_progress_delay(_("Checking out files"),
@@ -273,39 +293,32 @@ static int check_updates(struct unpack_trees_options *o)
int i, errs = 0;
struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
- for (i = 0; i < index->cache_nr; i++) {
- const struct cache_entry *ce = index->cache[i];
- if (ce->ce_flags & CE_WT_REMOVE) {
- display_progress(progress, ++cnt);
- if (o->update && !o->dry_run)
- unlink_entry(ce);
- }
- }
+ cnt = remove_workingtree_files(o, progress);
remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
errs |= checkout_entry(ce, &state, NULL);
}
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 5/5] unpack-trees: factor working tree update out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index b954ec1233..b40c069b1b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -275,67 +275,79 @@ static struct progress *get_progress(struct unpack_trees_options *o)
struct index_state *index = &o->result;
if (!o->update || !o->verbose_update)
return NULL;
for (; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
return start_progress_delay(_("Checking out files"),
total, 50, 1);
}
-static int check_updates(struct unpack_trees_options *o)
+static int update_working_tree_files(struct unpack_trees_options *o,
+ struct progress *progress,
+ unsigned start_cnt)
{
- unsigned cnt = 0;
+ unsigned cnt = start_cnt;
int i, errs = 0;
- struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
- progress = get_progress(o);
-
- if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
-
- cnt = remove_workingtree_files(o, progress);
- remove_marked_cache_entries(index);
- remove_scheduled_dirs();
-
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
- if (o->update && !o->dry_run) {
+ if (o->update && !o->dry_run)
errs |= checkout_entry(ce, &state, NULL);
- }
}
}
+
+ return errs;
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ struct progress *progress = NULL;
+ struct index_state *index = &o->result;
+ int errs;
+ unsigned total_removed;
+
+ progress = get_progress(o);
+
+ if (o->update)
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+ total_removed = remove_workingtree_files(o, progress);
+ remove_marked_cache_entries(index);
+ remove_scheduled_dirs();
+ errs = update_working_tree_files(o, progress, total_removed);
+
stop_progress(&progress);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
return errs != 0;
}
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o);
static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types,
struct unpack_trees_options *o);
static int apply_sparse_checkout(struct index_state *istate,
struct cache_entry *ce,
struct unpack_trees_options *o)
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* Re: [PATCHv2] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, René Scharfe
In-Reply-To: <xmqq8tqv6051.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 31, 2016 at 5:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I'd add René's Reviewed-by: here.
done
>
> I think moving heavier and initialized variables earlier and more
> lightweight and ephemeral ones like "i" later does make it easier to
> follow. "errs" has the significance and the lifetime similar to
> cnt/total, and logically should be higher, though. It is not a big
> enough deal to reroll (but as your futzing of the variable definition
> order was not a big enough deal to do in this patch, either, so...).
I will send out a series, that is based on this patch shortly;
as I fuzzed again with the small variables, that series doesn't
apply on this version but the version to be sent out shortly.
>
> Queued. Thanks.
please replace with the following series.
^ permalink raw reply
* [PATCHv3 1/5] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.
The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.
Reviewed-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index bc56195e27..55c75b4d6a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,77 +237,82 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o,
- const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ int i, errs = 0;
+
struct progress *progress = NULL;
struct index_state *index = &o->result;
- int i;
- int errs = 0;
+ struct checkout state = CHECKOUT_INIT;
+
+ state.force = 1;
+ state.quiet = 1;
+ state.refresh_cache = 1;
+ state.istate = index;
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
progress = start_progress_delay(_("Checking out files"),
total, 50, 1);
cnt = 0;
}
if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
continue;
}
}
- remove_marked_cache_entries(&o->result);
+ remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
- errs |= checkout_entry(ce, state, NULL);
+ errs |= checkout_entry(ce, &state, NULL);
}
}
}
stop_progress(&progress);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
return errs != 0;
}
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o);
static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types,
struct unpack_trees_options *o);
@@ -1113,38 +1118,33 @@ static void mark_new_skip_worktree(struct exclude_list *el,
static int verify_absent(const struct cache_entry *,
enum unpack_trees_error_types,
struct unpack_trees_options *);
/*
* N-way merge "len" trees. Returns 0 on success, -1 on failure to manipulate the
* resulting index, -2 on failure to reflect the changes to the work tree.
*
* CE_ADDED, CE_UNPACKED and CE_NEW_SKIP_WORKTREE are used internally
*/
int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
{
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
- struct checkout state = CHECKOUT_INIT;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
- state.force = 1;
- state.quiet = 1;
- state.refresh_cache = 1;
- state.istate = &o->result;
memset(&el, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
char *sparse = git_pathdup("info/sparse-checkout");
if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0)
o->skip_sparse_checkout = 1;
else
o->el = ⪙
free(sparse);
}
memset(&o->result, 0, sizeof(o->result));
o->result.initialized = 1;
@@ -1257,31 +1257,31 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (ret < 0)
goto return_failed;
/*
* Sparse checkout is meant to narrow down checkout area
* but it does not make sense to narrow down to empty working
* tree. This is usually a mistake in sparse checkout rules.
* Do not allow users to do that.
*/
if (o->result.cache_nr && empty_worktree) {
ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
goto done;
}
}
o->src_index = NULL;
- ret = check_updates(o, &state) ? (-2) : 0;
+ ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
o->result.cache_tree = cache_tree();
if (!cache_tree_fully_valid(o->result.cache_tree))
cache_tree_update(&o->result,
WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
}
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
discard_index(&o->result);
}
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 0/5] refactor unpack-trees
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <CAGZ79kaM=Uosm7KAvAP-8w59Tyfc7LZiV3ZOr=PZnBgMCzr2AA@mail.gmail.com>
unpack-trees is a central file needed for the understanding
of working tree manipulation. To help with the understanding
refactor the code to be more readable.
The first patch was a standalone patch 8 days ago;
now incorporated into this series as a v3,
reducing the scope of the checkout state.
The second patch removes a single continue statement;
it needed some digging to explain, but looks trivial.
The last 3 patches shorten the check_updates function by adding more
functions. If we ever want to parallelize file IO then these smaller
functions would be the scope to do it, keeping the check_updates as
a high level function guiding through the steps what is happening during
a working tree update.
Thanks,
Stefan
Stefan Beller (5):
unpack-trees: move checkout state into check_updates
unpack-trees: remove unneeded continue
unpack-trees: factor progress setup out of check_updates
unpack-trees: factor file removal out of check_updates
unpack-trees: factor working tree update out of check_updates
unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 64 insertions(+), 32 deletions(-)
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Jeff King @ 2017-01-06 20:10 UTC (permalink / raw)
To: Stefan Beller; +Cc: Ralf Thielow, Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAGZ79kZOfHWP_pQGN1QcmR71Ft6ib0aPwNKX80YMT7KcK0_Stg@mail.gmail.com>
On Fri, Jan 06, 2017 at 11:43:52AM -0800, Stefan Beller wrote:
> >>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
> >>
> >> That seems strange. Is it possible that your config is not writable?
> >> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
> >> is configured already via git config --global/--system/--local --list)
> >>
> >
> > Have you actually tried to reproduce this issue? I'm on current next
> > and can reproduce the problem.
>
> eh, I was on $random_version that I currently have installed
> (with messed up submodule code, but otherwise close to master).
Hmm. I can reproduce, but only in one situation: when the new
description is empty. In which case we try to delete the variable. In
other words:
[this breaks; the file remains empty and we try to delete the
nonexistent config]
$ EDITOR=true git branch --edit-description master
fatal: could not unset 'branch.master.description'
[this works; we actually set the variable]
$ EDITOR='echo foo >' git branch --edit-description master
$ git config branch.master.description
foo
[and now the unsetting works; note we have to truncate here, since
the file will be prepopulated with "foo" from the existing desc]
$ EDITOR='>' git branch --edit-description master
$ git config branch.master.description
[no output]
The history of this behavior is a bit funny.
In old versions of git, we would return a failing exit code of "1" from
git-branch, with no message.
Then in bd25f89014 (branch: die on config error when editing branch
description, 2016-02-22), we actually started returning "0"! This was
because the config code did not propagate errors from its helper
functions in all cases.
That was fixed by 9c14bb08a4 (git_config_set_multivar_in_file: all
non-zero returns are errors, 2016-04-09), giving the behavior we see
today.
So between v2.7.3 and v2.8.3, we did return 0, but I think that was a
bug (we also returned 0 for a lot of other bogus cases, too).
I could see either behavior as reasonable, but I think the right
solution would be for the branch code from bd25f89014 to use the
"gently" function set the variable, and then decide which cases should
be silently ignored, and which propagated as errors.
-Peff
^ 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