* Re: [wishlist] git submodule update --reset-hard
From: Yaroslav Halchenko @ 2018-12-10 20:14 UTC (permalink / raw)
Cc: git
In-Reply-To: <CAGZ79kYDa27EFk4A9uEzCnoW7scjb1U8fKwCo0P7rUZESto+Qg@mail.gmail.com>
>
>> Took me some time to figure out
>> why I was getting
>>
>> fatal: bad value for update parameter
>>
>> after all my changes to the git-submodule.sh script after looking at
>an
>> example commit 42b491786260eb17d97ea9fb1c4b70075bca9523 which
>introduced
>> --merge to the update ;-)
>
>Yeah I saw you also updated the submodule related C code, was that
>fatal message related to that?
Yes
--
Yaroslav O. Halchenko (mobile version)
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, NH, USA
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Elijah Newren @ 2018-12-10 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqq8t0z3xcc.fsf@gitster-ct.c.googlers.com>
On Sun, Dec 9, 2018 at 12:42 AM Junio C Hamano <gitster@pobox.com> wrote:
> Git 2.20 has been tagged. I'd expect that we would slow down to see
> how stable it is and queue only the brown-paper-bag fixes for a week
> or so, before opening the tree for the next cycle, rewinding the tip
> of 'next', etc.
Does this mean you'd prefer we continue to wait a little longer before
sending in new series and re-rolls, or just that you are managing
expectations about when they might be queued?
(Just curious whether I should submit my rebase-merge-on-sequencer
re-roll or continue waiting. I'm happy to do whatever is more
convenient for others.)
^ permalink raw reply
* [PATCH 3/3] rebase: introduce a shortcut for --reschedule-failed-exec
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 19:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.90.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
It is a bit cumbersome to write out the `--reschedule-failed-exec`
option before `-x <cmd>` all the time; let's introduce a convenient
option to do both at the same time: `-y <cmd>`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-rebase.txt | 6 ++++++
builtin/rebase.c | 21 +++++++++++++++++++++
git-legacy-rebase.sh | 6 ++++++
t/t3418-rebase-continue.sh | 3 +++
4 files changed, 36 insertions(+)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9dd68f77f6..99ca589ffc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,6 +462,12 @@ without an explicit `--interactive`.
+
See also INCOMPATIBLE OPTIONS below.
+-y <cmd>::
+ This is the same as passing `--reschedule-failed-exec` before
+ `-x <cmd>`, i.e. it appends the specified `exec` command and
+ turns on the mode where failed `exec` commands are automatically
+ rescheduled.
+
--root::
Rebase all commits reachable from <branch>, instead of
limiting them with an <upstream>. This allows you to rebase
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06e450b537..b707ccf00f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -754,6 +754,23 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
return 0;
}
+struct opt_y {
+ struct string_list *list;
+ struct rebase_options *options;
+};
+
+static int parse_opt_y(const struct option *opt, const char *arg, int unset)
+{
+ struct opt_y *o = opt->value;
+
+ if (unset || !arg)
+ return -1;
+
+ o->options->reschedule_failed_exec = 1;
+ string_list_append(o->list, arg);
+ return 0;
+}
+
static void NORETURN error_on_missing_default_upstream(void)
{
struct branch *current_branch = branch_get(NULL);
@@ -817,6 +834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
+ struct opt_y opt_y = { .list = &exec, .options = &options };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
N_("revision"),
@@ -894,6 +912,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
N_("add exec lines after each commit of the "
"editable list")),
+ { OPTION_CALLBACK, 'y', NULL, &opt_y, N_("<cmd>"),
+ N_("same as --reschedule-failed-exec -x <cmd>"),
+ PARSE_OPT_NONEG, parse_opt_y },
OPT_BOOL(0, "allow-empty-message",
&options.allow_empty_message,
N_("allow rebasing commits with empty messages")),
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 1b268a5fcc..8048891fed 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -26,6 +26,7 @@ f,force-rebase! cherry-pick all commits, even if unchanged
m,merge! use merging strategies to rebase
i,interactive! let the user edit the list of commits to rebase
x,exec=! add exec lines after each commit of the editable list
+y=! same as --reschedule-failed-exec -x
k,keep-empty preserve empty commits during rebase
allow-empty-message allow rebasing commits with empty messages
stat! display a diffstat of what changed upstream
@@ -262,6 +263,11 @@ do
cmd="${cmd}exec ${1#--exec=}${LF}"
test -z "$interactive_rebase" && interactive_rebase=implied
;;
+ -y*)
+ reschedule_failed_exec=--reschedule-failed-exec
+ cmd="${cmd}exec ${1#-y}${LF}"
+ test -z "$interactive_rebase" && interactive_rebase=implied
+ ;;
--interactive)
interactive_rebase=explicit
;;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index bdaa511bb0..25aaacacfc 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -262,6 +262,9 @@ test_expect_success '--reschedule-failed-exec' '
test_must_fail git -c rebase.rescheduleFailedExec=true \
rebase -x false HEAD^ 2>err &&
grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+ test_i18ngrep "has been rescheduled" err &&
+ git rebase --abort &&
+ test_must_fail git rebase -y false HEAD^ 2>err &&
test_i18ngrep "has been rescheduled" err
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/3] rebase: introduce --reschedule-failed-exec
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 19:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.90.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
A common use case for the `--exec` option is to verify that each commit
in a topic branch compiles cleanly, via `git rebase -x make <base>`.
However, when an `exec` in such a rebase fails, it is not re-scheduled,
which in this instance is not particularly helpful.
Let's offer a flag to reschedule failed `exec` commands.
Based on an idea by Paul Morelle.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-rebase.txt | 5 +++++
builtin/rebase--interactive.c | 2 ++
builtin/rebase.c | 16 +++++++++++++++-
git-legacy-rebase.sh | 16 +++++++++++++++-
git-rebase--common.sh | 1 +
sequencer.c | 13 ++++++++++---
sequencer.h | 1 +
t/t3418-rebase-continue.sh | 6 ++++++
8 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..9dd68f77f6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -501,6 +501,11 @@ See also INCOMPATIBLE OPTIONS below.
with care: the final stash application after a successful
rebase might result in non-trivial conflicts.
+--reschedule-failed-exec::
+--no-reschedule-failed-exec::
+ Automatically reschedule `exec` commands that failed. This only makes
+ sense in interactive mode (or when an `--exec` option was provided).
+
INCOMPATIBLE OPTIONS
--------------------
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a2ab68ed06..a9ab009fdb 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
OPT_STRING(0, "onto-name", &onto_name, N_("onto-name"), N_("onto name")),
OPT_STRING(0, "cmd", &cmd, N_("cmd"), N_("the command to run")),
OPT_RERERE_AUTOUPDATE(&opts.allow_rerere_auto),
+ OPT_BOOL(0, "reschedule-failed-exec", &opts.reschedule_failed_exec,
+ N_("automatically re-schedule any `exec` that fails")),
OPT_END()
};
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..6d556fc6c8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -104,6 +104,7 @@ struct rebase_options {
int rebase_merges, rebase_cousins;
char *strategy, *strategy_opts;
struct strbuf git_format_patch_opt;
+ int reschedule_failed_exec;
};
static int is_interactive(struct rebase_options *opts)
@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
argv_array_push(&child.args, opts->gpg_sign_opt);
if (opts->signoff)
argv_array_push(&child.args, "--signoff");
+ if (opts->reschedule_failed_exec)
+ argv_array_push(&child.args, "--reschedule-failed-exec");
status = run_command(&child);
goto finished_rebase;
@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
"strategy")),
OPT_BOOL(0, "root", &options.root,
N_("rebase all reachable commits up to the root(s)")),
+ OPT_BOOL(0, "reschedule-failed-exec",
+ &options.reschedule_failed_exec,
+ N_("automatically re-schedule any `exec` that fails")),
OPT_END(),
};
int i;
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
}
+ if (options.reschedule_failed_exec && !is_interactive(&options))
+ die(_("--reschedule-failed-exec requires an interactive rebase"));
+
if (options.git_am_opts.argc) {
/* all am options except -q are compatible only with --am */
for (i = options.git_am_opts.argc - 1; i >= 0; i--)
@@ -1220,7 +1229,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.flags |= REBASE_FORCE;
}
- if (options.type == REBASE_PRESERVE_MERGES)
+ if (options.type == REBASE_PRESERVE_MERGES) {
/*
* Note: incompatibility with --signoff handled in signoff block above
* Note: incompatibility with --interactive is just a strong warning;
@@ -1230,6 +1239,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
die(_("error: cannot combine '--preserve-merges' with "
"'--rebase-merges'"));
+ if (options.reschedule_failed_exec)
+ die(_("error: cannot combine '--preserve-merges' with "
+ "'--reschedule-failed-exec'"));
+ }
+
if (options.rebase_merges) {
if (strategy_options.nr)
die(_("error: cannot combine '--rebase-merges' with "
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..5f0f0c5ab8 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -48,6 +48,7 @@ skip! skip current patch and continue
edit-todo! edit the todo list during an interactive rebase
quit! abort but keep HEAD where it is
show-current-patch! show the patch file being applied or merged
+reschedule-failed-exec automatically reschedule failed exec commands
"
. git-sh-setup
set_reflog_action rebase
@@ -92,6 +93,7 @@ autosquash=
keep_empty=
allow_empty_message=--allow-empty-message
signoff=
+reschedule_failed_exec=
test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
case "$(git config --bool commit.gpgsign)" in
true) gpg_sign_opt=-S ;;
@@ -126,6 +128,8 @@ read_basic_state () {
signoff="$(cat "$state_dir"/signoff)"
force_rebase=t
}
+ test -f "$state_dir"/reschedule-failed-exec &&
+ reschedule_failed_exec=t
}
finish_rebase () {
@@ -163,7 +167,8 @@ run_interactive () {
"$allow_empty_message" "$autosquash" "$verbose" \
"$force_rebase" "$onto_name" "$head_name" "$strategy" \
"$strategy_opts" "$cmd" "$switch_to" \
- "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff"
+ "$allow_rerere_autoupdate" "$gpg_sign_opt" "$signoff" \
+ "$reschedule_failed_exec"
}
run_specific_rebase () {
@@ -378,6 +383,12 @@ do
--gpg-sign=*)
gpg_sign_opt="-S${1#--gpg-sign=}"
;;
+ --reschedule-failed-exec)
+ reschedule_failed_exec=--reschedule-failed-exec
+ ;;
+ --no-reschedule-failed-exec)
+ reschedule_failed_exec=
+ ;;
--)
shift
break
@@ -534,6 +545,9 @@ then
# git-rebase.txt caveats with "unless you know what you are doing"
test -n "$rebase_merges" &&
die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
+
+ test -n "$reschedule_failed_exec" &&
+ die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
fi
if test -n "$rebase_merges"
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
index 7e39d22871..a8a44608e0 100644
--- a/git-rebase--common.sh
+++ b/git-rebase--common.sh
@@ -19,6 +19,7 @@ write_basic_state () {
"$state_dir"/allow_rerere_autoupdate
test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+ test -n "$reschedule_failed_exec" && : > "$state_dir"/reschedule-failed-exec
}
apply_autostash () {
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1..69bee63e11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
static int git_sequencer_config(const char *k, const char *v, void *cb)
{
@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
opts->signoff = 1;
}
+ if (file_exists(rebase_path_reschedule_failed_exec()))
+ opts->reschedule_failed_exec = 1;
+
read_strategy_opts(opts, &buf);
strbuf_release(&buf);
@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
if (opts->signoff)
write_file(rebase_path_signoff(), "--signoff\n");
+ if (opts->reschedule_failed_exec)
+ write_file(rebase_path_reschedule_failed_exec(), "%s", "");
return 0;
}
@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
*end_of_arg = saved;
/* Reread the todo file if it has changed. */
- if (res)
- ; /* fall through */
- else if (stat(get_todo_path(opts), &st))
+ if (res) {
+ if (opts->reschedule_failed_exec)
+ reschedule = 1;
+ } else if (stat(get_todo_path(opts), &st))
res = error_errno(_("could not stat '%s'"),
get_todo_path(opts));
else if (match_stat_data(&todo_list->stat, &st)) {
diff --git a/sequencer.h b/sequencer.h
index 5071a73563..1f865dae26 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -39,6 +39,7 @@ struct replay_opts {
int allow_empty_message;
int keep_redundant_commits;
int verbose;
+ int reschedule_failed_exec;
int mainline;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 0210b2ac6f..54b26a9284 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -254,4 +254,10 @@ test_expect_success 'the todo command "break" works' '
test_path_is_file execed
'
+test_expect_success '--reschedule-failed-exec' '
+ test_when_finished "git rebase --abort" &&
+ test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
+ grep "^exec false" .git/rebase-merge/git-rebase-todo
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] rebase: add a config option to default to --reschedule-failed-exec
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 19:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.90.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
It would be cumbersome to type out that option all the time, so let's
offer the convenience of a config setting: rebase.rescheduleFailedExec.
Besides, this opens the door to changing the default in a future version
of Git: it does make some sense to reschedule failed `exec` commands by
default (and if we could go back in time when the `exec` command was
invented, we probably would change that default right from the start).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/config/rebase.txt | 5 +++++
builtin/rebase.c | 5 +++++
git-legacy-rebase.sh | 2 ++
t/t3418-rebase-continue.sh | 7 ++++++-
4 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..331d250e04 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -64,3 +64,8 @@ instead of:
-------------------------------------------
+
Defaults to false.
+
+rebase.rescheduleFailedExec::
+ Automatically reschedule `exec` commands that failed. This only makes
+ sense in interactive mode (or when an `--exec` option was provided).
+ This is the same as specifying the `--reschedule-failed-exec` option.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6d556fc6c8..06e450b537 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -677,6 +677,11 @@ static int rebase_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "rebase.reschedulefailedexec")) {
+ opts->reschedule_failed_exec = git_config_bool(var, value);
+ return 0;
+ }
+
return git_default_config(var, value, data);
}
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 5f0f0c5ab8..1b268a5fcc 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -99,6 +99,8 @@ case "$(git config --bool commit.gpgsign)" in
true) gpg_sign_opt=-S ;;
*) gpg_sign_opt= ;;
esac
+test "$(git config --bool rebase.reschedulefailedexec)" = "true" &&
+reschedule_failed_exec=--reschedule-failed-exec
. git-rebase--common
read_basic_state () {
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 54b26a9284..bdaa511bb0 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -257,7 +257,12 @@ test_expect_success 'the todo command "break" works' '
test_expect_success '--reschedule-failed-exec' '
test_when_finished "git rebase --abort" &&
test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
- grep "^exec false" .git/rebase-merge/git-rebase-todo
+ grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+ git rebase --abort &&
+ test_must_fail git -c rebase.rescheduleFailedExec=true \
+ rebase -x false HEAD^ 2>err &&
+ grep "^exec false" .git/rebase-merge/git-rebase-todo &&
+ test_i18ngrep "has been rescheduled" err
'
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 19:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
The idea was brought up by Paul Morelle.
To be honest, this idea of rescheduling a failed exec makes so much sense
that I wish we had done this from the get-go.
So let's do the next best thing: implement a command-line option and a
config setting to make it so.
The obvious intent behind that config setting is to not only give users a
way to opt into that behavior already, but also to make it possible to flip
the default at a later date, after the feature has been battle-tested and
after deprecating the non-rescheduling behavior for a couple of Git
versions.
If the team agrees with my reasoning, then the 3rd patch (introducing -y
<cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
much sense, as it would introduce a short option that would become obsolete
soon.
Johannes Schindelin (3):
rebase: introduce --reschedule-failed-exec
rebase: add a config option to default to --reschedule-failed-exec
rebase: introduce a shortcut for --reschedule-failed-exec
Documentation/config/rebase.txt | 5 ++++
Documentation/git-rebase.txt | 11 +++++++++
builtin/rebase--interactive.c | 2 ++
builtin/rebase.c | 42 ++++++++++++++++++++++++++++++++-
git-legacy-rebase.sh | 24 ++++++++++++++++++-
git-rebase--common.sh | 1 +
sequencer.c | 13 +++++++---
sequencer.h | 1 +
t/t3418-rebase-continue.sh | 14 +++++++++++
9 files changed, 108 insertions(+), 5 deletions(-)
base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/90
--
gitgitgadget
^ permalink raw reply
* Re: [wishlist] git submodule update --reset-hard
From: Stefan Beller @ 2018-12-10 18:58 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git
In-Reply-To: <20181208042139.GA4827@hopa.kiewit.dartmouth.edu>
On Fri, Dec 7, 2018 at 8:21 PM Yaroslav Halchenko <yoh@onerussian.com> wrote:
>
>
> On Fri, 07 Dec 2018, Yaroslav Halchenko wrote:
>
>
> > On Fri, 07 Dec 2018, Stefan Beller wrote:
> > > > the initial "git submodule update --reset-hard" is pretty much a
> > > > crude workaround for some of those cases, so I would just go earlier in
> > > > the history, and redo some things, whenever I could just drop or revert
> > > > some selected set of commits.
>
> > > That makes sense.
> > > Do you want to give the implementation a try for the --reset-hard switch?
>
> > ok, will do, thanks for the blessing ;-)
>
> The patch is attached (please advise if should be done
> differently) and also submitted as PR
> https://github.com/git/git/pull/563
Yes, usually we send patches inline
(Random example:
https://public-inbox.org/git/244bdf2a6fc300f2b535ac8edfc2fbdaf5260266.1544465177.git.gitgitgadget@gmail.com/T/#u
compared to https://public-inbox.org/git/20181208042139.GA4827@hopa.kiewit.dartmouth.edu/
(which I am replying to))
See Documentation/SubmittingPatches.
There are some tools that provide a GithubPR -> emailPatch workflow at
https://github.com/gitgitgadget/git
I think if you'd open your pull request there, then it would be automatically
mailed to the list correctly.
I left some comments on the PR.
>
> I guess it would need more tests.
Writing tests is hard, as we don't know what we expect to break. ;-)
> Took me some time to figure out
> why I was getting
>
> fatal: bad value for update parameter
>
> after all my changes to the git-submodule.sh script after looking at an
> example commit 42b491786260eb17d97ea9fb1c4b70075bca9523 which introduced
> --merge to the update ;-)
Yeah I saw you also updated the submodule related C code, was that
fatal message related to that?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] terminology tweak: prune -> path limiting
From: Matthew DeVore @ 2018-12-10 18:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, matvore, dstolee, pclouds, Jeff King
In-Reply-To: <xmqqo99v5vnc.fsf@gitster-ct.c.googlers.com>
On Sat, Dec 8, 2018 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matthew DeVore <matvore@google.com> writes:
>
> > In the codebase, "prune" is a highly overloaded term, and it caused me a
> > lot of trouble to figure out what it meant when it was used in the
> > context of path limiting. Stop using the word "prune" when we really
> > mean "path limiting."
>
> path limiting is also used for two purposes. "pruning", which is to
> cull the side branches that do not contribute the changes made to
> the paths we are interested in, and showing only the changes to the
> paths that match pathspec.
Thank you for the clarification re: side branches.
>
> AFAIK, "prune" is also used to describe unreachable loose objects,
> but that use is fairly isolated and have little risk of being
> confusing too much. Are there other uses to make you consider it
> "highly overloaded"?
This is what I found:
git prune - cull unreachable loose objects
git fetch --prune - remove remote-tracking refs that no longer exist
at source. Also note "--prune-tags" option
git notes prune - remove notes for non-existing/unreachable objects
git worktree prune - prunes "administrative" files
git prune-packed - removes loose objects that are also in pack files
git filter-branch --prune-empty - removes commits that become empty as
a result of rewriting.
It seems there are three general categories of the use of the term -
- to remove things from a view (e.g. in path limiting)
- to remove loose objects for efficiency (git prune, git prune-packed)
- to remove other things for either efficiency or to reduce cognitive
overhead (git worktree prune, git fetch --prune)
... and each of these categories has 2+ subcategories.
When I tried to figure out what "prune" and "prune_data" ("data" is
quite vague, so these two fields read like "prune_1" and "prune_2")
referred to in "revision.h", I basically did "grep -rn prune" and
looked through the results, but there were too many uses of the term
"prune" to pinpoint its meaning. If I used an IDE I might have been
able to "find usages" of struct revs's prune field, and I probably
*should have* done that, but I didn't have an IDE prepared and I
figured it wasn't important. Then an hour or so later I realized I was
still being confused by this term, and set out to figuring out what it
actually meant.
>
> My gut feeling is that the result is not reducing "overloading" in a
> meaningful way, and this change is not worth the churn, but it
> depends on the answer to the above question.
>
> Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Josh Steadmon @ 2018-12-10 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8t0z3xcc.fsf@gitster-ct.c.googlers.com>
On 2018.12.09 17:42, Junio C Hamano wrote:
> * js/protocol-advertise-multi (2018-11-17) 1 commit
> - protocol: advertise multiple supported versions
>
> The transport layer has been updated so that the protocol version
> used can be negotiated between the parties, by the initiator
> listing the protocol versions it is willing to talk, and the other
> side choosing from one of them.
>
> How's the doneness of this one?
I'm not sure if you're asking me specifically or the list in general,
but I haven't heard any complaints about this, and there are no
outstanding requests on the review thread.
> * js/smart-http-detect-remote-error (2018-11-17) 3 commits
> (merged to 'next' on 2018-11-18 at 5c6edfcb85)
> + remote-curl: die on server-side errors
> + remote-curl: tighten "version 2" check for smart-http
> + remote-curl: refactor smart-http discovery
>
> Some errors from the other side coming over smart HTTP transport
> were not noticed, which has been corrected.
>
> Will cook in 'next'.
Masaya had a series that addresses this as well:
https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/
To combine these, we'd want to drop the diff to remote-curl.c in the
final patch of this series.
^ permalink raw reply
* Re: [PATCH] style: the opening '{' of a function is in a separate line
From: Duy Nguyen @ 2018-12-10 18:51 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git Mailing List
In-Reply-To: <CAGZ79kZpcgRFs=2NgaYHUAfRQ16u-LG7CjEJJg4+=oxUQ6Bbnw@mail.gmail.com>
On Mon, Dec 10, 2018 at 7:47 PM Stefan Beller <sbeller@google.com> wrote:
>
> On Sun, Dec 9, 2018 at 2:25 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> >
> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > ---
> > bisect.c | 3 ++-
> > builtin/config.c | 3 ++-
> > builtin/push.c | 4 +++-
> > convert.c | 3 ++-
> > credential-cache--daemon.c | 3 ++-
> > diff.c | 6 ++++--
> > git.c | 3 ++-
> > imap-send.c | 3 ++-
> > remote-curl.c | 3 ++-
> > sequencer.c | 6 ++++--
> > string-list.c | 3 ++-
> > t/helper/test-sigchain.c | 3 ++-
> > transport-helper.c | 3 ++-
> > url.c | 3 ++-
> > userdiff.c | 3 ++-
> > 15 files changed, 35 insertions(+), 17 deletions(-)
>
> Can you say more about how you found these?
I was updating diff.c (65 patches coming soon!) and found one function
that triggered me. "git grep '^static.*) {$'" caught most of them. A
bit more regex got some more but I don't even know if I have caught
them all.
> (Are these all of them, or did you look through some subset of files?)
I left compat and xdiff directories out if I remember correctly.
> Or did you use a formatting tool?
Unfortunately no. Perhaps clang-format can do it? I didn't try.
--
Duy
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Elijah Newren @ 2018-12-10 18:47 UTC (permalink / raw)
To: Nguyễn Thái Ngọc
Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8CR2p4784PxX2o+t4QcgyiU=QoiqLeB5o86cdyd5_fHEA@mail.gmail.com>
On Mon, Dec 10, 2018 at 10:34 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, Dec 10, 2018 at 7:25 PM Elijah Newren <newren@gmail.com> wrote:
> > > I'm not the unpack-trees man (I think that would still be Junio). And
> > > I'm not saying it's sane either. I think it's just some leftover
> > > things since Linus split "the index" in unpack-tree operation to
> > > 'src', 'result' and 'dst' many years ago and nobody was brave enough
> > > to clean it up (then I piled on with untracked cache and split index,
> > > but I did not see it clearly either). That person could be you ;-)
> >
> > Hmm, might make a good New Year's resolution: Enter the abyss, find
> > out if one can return from it... or maybe I could just sanely run
> > away screaming. We'll see.
>
> I'm getting off topic. But my new years resolution would be optimize
> for the case where src_index == dst_index, which is somewhat ironic
> because we used to do everything in the same index, but it was a messy
> mess and had to be split up.
Ooh, that sounds cool too. I look forward to seeing it.
^ permalink raw reply
* [PATCH 1/2] mingw: demonstrate a problem with certain absolute paths
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.96.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, there are several categories of absolute paths. One such
category starts with a backslash and is implicitly relative to the
drive associated with the current working directory. Example:
c:
git clone https://github.com/git-for-windows/git \G4W
should clone into C:\G4W.
There is currently a problem with that, in that mingw_mktemp() does not
expect the _wmktemp() function to prefix the absolute path with the
drive prefix, and as a consequence, the resulting path does not fit into
the originally-passed string buffer. The symptom is a "Result too large"
error.
Reported by Juan Carlos Arevalo Baeza.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t5580-clone-push-unc.sh | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index ba548df4a9..c2b0082296 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -17,14 +17,11 @@ fi
UNCPATH="$(winpwd)"
case "$UNCPATH" in
[A-Z]:*)
+ WITHOUTDRIVE="${UNCPATH#?:}"
# Use administrative share e.g. \\localhost\C$\git-sdk-64\usr\src\git
# (we use forward slashes here because MSYS2 and Git accept them, and
# they are easier on the eyes)
- UNCPATH="//localhost/${UNCPATH%%:*}\$/${UNCPATH#?:}"
- test -d "$UNCPATH" || {
- skip_all='could not access administrative share; skipping'
- test_done
- }
+ UNCPATH="//localhost/${UNCPATH%%:*}\$$WITHOUTDRIVE"
;;
*)
skip_all='skipping UNC path tests, cannot determine current path as UNC'
@@ -32,6 +29,18 @@ case "$UNCPATH" in
;;
esac
+test_expect_failure 'clone into absolute path lacking a drive prefix' '
+ USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix |
+ tr / \\\\)" &&
+ git clone . "$USINGBACKSLASHES" &&
+ test -f without-drive-prefix/.git/HEAD
+'
+
+test -d "$UNCPATH" || {
+ skip_all='could not access administrative share; skipping'
+ test_done
+}
+
test_expect_success setup '
test_commit initial
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/2] mingw: allow absolute paths without drive prefix
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.96.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When specifying an absolute path without a drive prefix, we convert that
path internally. Let's make sure that we handle that case properly, too
;-)
This fixes the command
git clone https://github.com/git-for-windows/git \G4W
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 10 +++++++++-
t/t5580-clone-push-unc.sh | 2 +-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 34b3880b29..4d009901d8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)
char *mingw_mktemp(char *template)
{
wchar_t wtemplate[MAX_PATH];
+ int offset = 0;
+
if (xutftowcs_path(wtemplate, template) < 0)
return NULL;
+
+ if (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&
+ iswalpha(wtemplate[0]) && wtemplate[1] == L':') {
+ /* We have an absolute path missing the drive prefix */
+ offset = 2;
+ }
if (!_wmktemp(wtemplate))
return NULL;
- if (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)
+ if (xwcstoutf(template, wtemplate + offset, strlen(template) + 1) < 0)
return NULL;
return template;
}
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index c2b0082296..17c38c33a5 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -29,7 +29,7 @@ case "$UNCPATH" in
;;
esac
-test_expect_failure 'clone into absolute path lacking a drive prefix' '
+test_expect_success 'clone into absolute path lacking a drive prefix' '
USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix |
tr / \\\\)" &&
git clone . "$USINGBACKSLASHES" &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] mingw: support absolute paths without a drive prefix
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
On Windows, when using a path like \hello\world, it is silently understood
to be absolute, but relative to the current directory's drive.
Let's support that, too.
Johannes Schindelin (2):
mingw: demonstrate a problem with certain absolute paths
mingw: allow absolute paths without drive prefix
compat/mingw.c | 10 +++++++++-
t/t5580-clone-push-unc.sh | 19 ++++++++++++++-----
2 files changed, 23 insertions(+), 6 deletions(-)
base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-96%2Fdscho%2Fdrive-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-96/dscho/drive-prefix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/96
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] style: the opening '{' of a function is in a separate line
From: Stefan Beller @ 2018-12-10 18:46 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <20181209102521.5301-1-pclouds@gmail.com>
On Sun, Dec 9, 2018 at 2:25 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> bisect.c | 3 ++-
> builtin/config.c | 3 ++-
> builtin/push.c | 4 +++-
> convert.c | 3 ++-
> credential-cache--daemon.c | 3 ++-
> diff.c | 6 ++++--
> git.c | 3 ++-
> imap-send.c | 3 ++-
> remote-curl.c | 3 ++-
> sequencer.c | 6 ++++--
> string-list.c | 3 ++-
> t/helper/test-sigchain.c | 3 ++-
> transport-helper.c | 3 ++-
> url.c | 3 ++-
> userdiff.c | 3 ++-
> 15 files changed, 35 insertions(+), 17 deletions(-)
Can you say more about how you found these?
(Are these all of them, or did you look through some subset of files?)
Or did you use a formatting tool?
^ permalink raw reply
* Re: [PATCH 6/8] checkout: add --cached option
From: Elijah Newren @ 2018-12-10 18:42 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <20181209200449.16342-7-t.gummerer@gmail.com>
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Add a new --cached option to git checkout, which works only on the
> index, but not the working tree, similar to what 'git reset <tree-ish>
> -- <pathspec>... does. Indeed the tests are adapted from the 'git
> reset' tests.
>
> In the longer term the idea is to potentially deprecate 'git reset
> <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
> about re-pointing the HEAD, and not also about copying entries from
> <tree-ish> to the index.
>
> Note that 'git checkout' by default works in overlay mode, meaning
> files that match the pathspec that don't exist in <tree-ish>, but
> exist in the index would not be removed. 'git checkout --no-overlay
> --cached' can be used to get the same behaviour as 'git reset
> <tree-ish> -- <pathspec>'.
I think this argues _even more_ that --no-overlay should be the
default. Your series is valuable even if we don't push on that, I'm
just being noisy about what I think would be an even better world.
Also, I don't think I've mentioned it yet, but I'm really excited
about this series and what you're doing. It's super cool. (Which I
expected when I saw the description of the desired behavior, but I'm
also liking and contemplating re-using some code...)
> One thing this patch doesn't currently deal with is conflicts.
> Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
> doesn't do anything with the index, so the --cached option just
> mirrors that behaviour. But given it doesn't even deal with
> conflicts, the '--cached' option doesn't make much sense when no
> <tree-ish> is given. As it operates only on the index, it's always a
> no-op if no tree-ish is given.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>
> Maybe we can just disallow --cached without <tree-ish> given for now,
> and possibly later allow it with some different behaviour for
> conflicts, not sure what the best way forward here is. We can also
> just make it update the index as appropriate, and have it behave
> different than 'git checkout' curerntly does when handling conflicts?
Huh?
"git checkout -- <path>"
means update <path> from the index, meaning the index is left alone
(it's the source) and only the working tree is touched.
When you add a flag named --cached to only update the index and not
the working tree, then the index becomes the sole destination.
Now we combine: no tree is specified means the index is the source of
the writing, and --cached being specified means the index is the sole
destination of the writing. Thus, you have a no-op. If the user
specifies --cached and no tree, you should immediately exit with a
message along the lines of "Nothing to do; no tree given and --cached
specified." The presence of conflicts seems completely irrelevant to
me here.
> builtin/checkout.c | 26 ++++++++--
> t/t2016-checkout-patch.sh | 8 +++
> t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
> t/t9902-completion.sh | 1 +
> 4 files changed, 135 insertions(+), 3 deletions(-)
> create mode 100755 t/t2026-checkout-cached.sh
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 0aef35bbc4..6ba85e9de5 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -45,6 +45,7 @@ struct checkout_opts {
> int ignore_other_worktrees;
> int show_progress;
> int overlay_mode;
> + int cached;
> /*
> * If new checkout options are added, skip_merge_working_tree
> * should be updated accordingly.
> @@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts,
> die(_("Cannot update paths and switch to branch '%s' at the same time."),
> opts->new_branch);
>
> + if (opts->patch_mode && opts->cached)
> + return run_add_interactive(revision, "--patch=reset",
> + &opts->pathspec);
> +
> if (opts->patch_mode)
> return run_add_interactive(revision, "--patch=checkout",
> &opts->pathspec);
> @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> * the current index, which means that it should
> * be removed.
> */
> - ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> + ce->ce_flags |= CE_MATCHED | CE_REMOVE;
> + if (!opts->cached)
> + ce->ce_flags |= CE_WT_REMOVE;
> continue;
> } else {
> /*
> @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> for (pos = 0; pos < active_nr; pos++) {
> struct cache_entry *ce = active_cache[pos];
> if (ce->ce_flags & CE_MATCHED) {
> + if (opts->cached) {
> + continue;
> + }
> if (!ce_stage(ce)) {
> errs |= checkout_entry(ce, &state, NULL);
> continue;
> @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
> * not tested here
> */
>
> + /*
> + * opts->cached cannot be used with switching branches so is
> + * not tested here
> + */
> +
> /*
> * If we aren't creating a new branch any changes or updates will
> * happen in the existing branch. Since that could only be updating
> @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts,
> die(_("'%s' cannot be used with switching branches"),
> "--patch");
>
> - if (!opts->overlay_mode)
> + if (opts->overlay_mode != -1)
> + die(_("'%s' cannot be used with switching branches"),
> + "--overlay/--no-overlay");
> +
> + if (opts->cached)
> die(_("'%s' cannot be used with switching branches"),
> - "--no-overlay");
> + "--cached");
>
> if (opts->writeout_stage)
> die(_("'%s' cannot be used with switching branches"),
> @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> + OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
> OPT_END(),
> };
>
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 47aeb0b167..e8774046e0 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
> verify_state dir/foo head head
> '
>
> +test_expect_success PERL 'git checkout --cached -p' '
> + set_and_save_state dir/foo work work &&
> + test_write_lines n y | git checkout --cached -p >output &&
> + verify_state dir/foo work head &&
> + verify_saved_state bar &&
> + test_i18ngrep "Unstage" output
> +'
> +
> test_expect_success PERL 'none of this moved HEAD' '
> verify_saved_head
> '
> diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
> new file mode 100755
> index 0000000000..1b66192727
> --- /dev/null
> +++ b/t/t2026-checkout-cached.sh
> @@ -0,0 +1,103 @@
> +#!/bin/sh
> +
> +test_description='checkout --cached <pathspec>'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'checkout --cached <pathspec>' '
> + echo 1 >file1 &&
> + echo 2 >file2 &&
> + git add file1 file2 &&
> + test_tick &&
> + git commit -m files &&
> + git rm file2 &&
> + echo 3 >file3 &&
> + echo 4 >file1 &&
> + git add file1 file3 &&
> + git checkout --cached HEAD -- file1 file2 &&
> + test_must_fail git diff --quiet &&
> +
> + cat >expect <<-\EOF &&
> + diff --git a/file1 b/file1
> + index d00491f..b8626c4 100644
> + --- a/file1
> + +++ b/file1
> + @@ -1 +1 @@
> + -1
> + +4
> + diff --git a/file2 b/file2
> + deleted file mode 100644
> + index 0cfbf08..0000000
> + --- a/file2
> + +++ /dev/null
> + @@ -1 +0,0 @@
> + -2
> + EOF
> + git diff >actual &&
> + test_cmp expect actual &&
> +
> + cat >expect <<-\EOF &&
> + diff --git a/file3 b/file3
> + new file mode 100644
> + index 0000000..00750ed
> + --- /dev/null
> + +++ b/file3
> + @@ -0,0 +1 @@
> + +3
> + EOF
> + git diff --cached >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'checking out an unmodified path is a no-op' '
> + git reset --hard &&
> + git checkout --cached HEAD -- file1 &&
> + git diff-files --exit-code &&
> + git diff-index --cached --exit-code HEAD
> +'
> +
> +test_expect_success 'checking out specific path that is unmerged' '
> + test_commit file3 file3 &&
> + git rm --cached file2 &&
> + echo 1234 >file2 &&
> + F1=$(git rev-parse HEAD:file1) &&
> + F2=$(git rev-parse HEAD:file2) &&
> + F3=$(git rev-parse HEAD:file3) &&
> + {
> + echo "100644 $F1 1 file2" &&
> + echo "100644 $F2 2 file2" &&
> + echo "100644 $F3 3 file2"
> + } | git update-index --index-info &&
> + git ls-files -u &&
> + git checkout --cached HEAD file2 &&
> + test_must_fail git diff --quiet &&
> + git diff-index --exit-code --cached HEAD
> +'
> +
> +test_expect_success '--cached without --no-overlay does not remove entry from index' '
> + test_must_fail git checkout --cached HEAD^ file3 &&
> + git ls-files --error-unmatch -- file3
> +'
> +
> +test_expect_success 'file is removed from the index with --no-overlay' '
> + git checkout --cached --no-overlay HEAD^ file3 &&
> + test_path_is_file file3 &&
> + test_must_fail git ls-files --error-unmatch -- file3
> +'
> +
> +test_expect_success 'test checkout --cached --no-overlay at given paths' '
> + mkdir sub &&
> + >sub/file1 &&
> + >sub/file2 &&
> + git update-index --add sub/file1 sub/file2 &&
> + T=$(git write-tree) &&
> + git checkout --cached --no-overlay HEAD sub/file2 &&
> + test_must_fail git diff --quiet &&
> + U=$(git write-tree) &&
Do we need to worry at all about losing the exit status of write-tree
in either invocation? In particular, if the second one for U fails
somehow, we'd end up with $U being a blank string and we'd still
probably get "$T" != "$U" below.
You also had some rev-parse invocations hidden in a sub-shell in both
this patch and patch 5, but subsequent commands relied on non-empty
output out of those, so I figured those were fine. This one might be
too, but I thought I'd at least mention it.
> + echo "$T" &&
> + echo "$U" &&
> + test_must_fail git diff-index --cached --exit-code "$T" &&
> + test "$T" != "$U"
> +'
> +
> +test_done
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a3fd9a9630..cbc304ace8 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' '
> --no-quiet Z
> --no-... Z
> --overlay Z
> + --cached Z
> EOF
> '
>
> --
> 2.20.0.405.gbc1bbc6f85
^ permalink raw reply
* Re: [PATCH 0/8] introduce no-overlay and cached mode in git checkout
From: Duy Nguyen @ 2018-12-10 18:37 UTC (permalink / raw)
To: Elijah Newren; +Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CABPp-BFaWtDiBPuGQVU_+VGQtDkemDKnvjHhz+h1sbUGssffmQ@mail.gmail.com>
On Mon, Dec 10, 2018 at 6:18 PM Elijah Newren <newren@gmail.com> wrote:
> > The final step in the series is to actually make use of this in 'git
> > stash', which simplifies the code there a bit. I am however happy to
> > hold off on this step until the stash-in-C series is merged, so we
> > don't delay that further.
> >
> > In addition to the no-overlay mode, we also add a --cached mode, which
> > works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'.
>
> If you're adding a --cached mode to make it only work on the index,
> should there be a similar mode to allow it to only work on the working
> tree? (I'm not as concerned with that here, but I really think the
> new restore-files command by default should only operate on the
> working tree, and then have options to affect the index either in
> addition or instead of the working tree.)
In the context of restore-files, --target=<worktree|index|both> is a
very good candidate because "restore-files --from=foo --target=index"
is almost like saying "restore files in the index from "foo"". For
checkout, probably not as good. But then we can have different option
names for the two commands. So if "git checkout" is going to never
have "update worktree only" mode, then --cached is a still good way to
go.
--
Duy
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Duy Nguyen @ 2018-12-10 18:33 UTC (permalink / raw)
To: Elijah Newren; +Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CABPp-BH7CuNy-6wg1jnxytuwEF9Vw=8Y0N8dUhdCJJbyps2kyw@mail.gmail.com>
On Mon, Dec 10, 2018 at 7:25 PM Elijah Newren <newren@gmail.com> wrote:
> > I'm not the unpack-trees man (I think that would still be Junio). And
> > I'm not saying it's sane either. I think it's just some leftover
> > things since Linus split "the index" in unpack-tree operation to
> > 'src', 'result' and 'dst' many years ago and nobody was brave enough
> > to clean it up (then I piled on with untracked cache and split index,
> > but I did not see it clearly either). That person could be you ;-)
>
> Hmm, might make a good New Year's resolution: Enter the abyss, find
> out if one can return from it... or maybe I could just sanely run
> away screaming. We'll see.
I'm getting off topic. But my new years resolution would be optimize
for the case where src_index == dst_index, which is somewhat ironic
because we used to do everything in the same index, but it was a messy
mess and had to be split up.
--
Duy
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Elijah Newren @ 2018-12-10 18:25 UTC (permalink / raw)
To: Nguyễn Thái Ngọc
Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8BG5ri=UMeOPqLTqxcOYqPsc9BdY4pxgQA8pfb+rE1MyA@mail.gmail.com>
On Mon, Dec 10, 2018 at 10:19 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, Dec 10, 2018 at 7:09 PM Elijah Newren <newren@gmail.com> wrote:
> > > > For the two current callsites, unpack-trees seems to do this
> > > > invalidation itself internally.
> > >
> > > I'm still a bit scared of this invalidation business in unpack-trees.
> > > The thing is, we handle two separate index_state there, src_index and
> > > result and invalidation has to be done on the right one (because index
> > > extensions are on src_index until the very end of unpack-trees;
> > > invalidating on 'result' would be no-op and wrong).
> > > remove_marked_cache_entries() seems to be called on 'result' while
> > > invalidate_ce_path() is on src_index, hm....
> >
> > Is Thomas avoiding problems here simply because merge is the only
> > caller of unpack_trees with src_index != dst_index? Or does src_index
> > == dst_index for checkout not actually help?
>
> I think it would not help. 'result' is a temporary index where we copy
> things to (and it does not have anything from the beginning). If you
> invalidate stuff in there, you invalidate nothing, regardless whether
> dst_index == src_index.
>
> > If that does help with the checkout case, then allow me to find a
> > different way to muddy the waters... I think I might want to make use
> > of this function in the merge machinery at some point, so I either
> > need to figure out how to convince you to verify if all this cache
> > tree invalidation stuff is sane, or somehow figure out all the
> > cache_tree stuff stuff myself so I can figure out what is right here.
> > :-)
>
> I'm not the unpack-trees man (I think that would still be Junio). And
> I'm not saying it's sane either. I think it's just some leftover
> things since Linus split "the index" in unpack-tree operation to
> 'src', 'result' and 'dst' many years ago and nobody was brave enough
> to clean it up (then I piled on with untracked cache and split index,
> but I did not see it clearly either). That person could be you ;-)
Hmm, might make a good New Year's resolution: Enter the abyss, find
out if one can return from it... or maybe I could just sanely run
away screaming. We'll see.
^ permalink raw reply
* Re: [PATCH 5/8] checkout: introduce --{,no-}overlay option
From: Elijah Newren @ 2018-12-10 18:19 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <20181209200449.16342-6-t.gummerer@gmail.com>
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Currently 'git checkout' is defined as an overlay operation, which
> means that if in 'git checkout <tree-ish> -- [<pathspec>]' we have an
> entry in the index that matches <pathspec>, but that doesn't exist in
> <tree-ish>, that entry will not be removed from the index or the
> working tree.
>
> Introduce a new --{,no-}overlay option, which allows using 'git
> checkout' in non-overlay mode, thus removing files from the working
> tree if they do not exist in <tree-ish> but match <pathspec>.
>
> Note that 'git checkout -p <tree-ish> -- [<pathspec>]' already works
> this way, so no changes are needed for the patch mode. We disallow
> 'git checkout --overlay -p' to avoid confusing users who would expect
> to be able to force overlay mode in 'git checkout -p' this way.
Whoa...that's interesting. To me, that argues even further that the
traditional checkout behavior was wrong all along and the choice of
--overlay vs. --no-overlay in the original implementation was a total
oversight. I'm really tempted to say that --no-overlay should just be
the default in checkout too...but maybe that's too high a hill to
climb, at least for now.
Making --overlap and -p incompatible is a reasonable first step. But
you should probably add a comment to the -p option documentation that
it implies --no-overlay.
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> builtin/checkout.c | 64 +++++++++++++++++++++++++++-------
> t/t2025-checkout-no-overlay.sh | 47 +++++++++++++++++++++++++
> t/t9902-completion.sh | 1 +
> 3 files changed, 99 insertions(+), 13 deletions(-)
> create mode 100755 t/t2025-checkout-no-overlay.sh
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index acdafc6e4c..0aef35bbc4 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -44,6 +44,7 @@ struct checkout_opts {
> int ignore_skipworktree;
> int ignore_other_worktrees;
> int show_progress;
> + int overlay_mode;
> /*
> * If new checkout options are added, skip_merge_working_tree
> * should be updated accordingly.
> @@ -132,7 +133,8 @@ static int skip_same_name(const struct cache_entry *ce, int pos)
> return pos;
> }
>
> -static int check_stage(int stage, const struct cache_entry *ce, int pos)
> +static int check_stage(int stage, const struct cache_entry *ce, int pos,
> + int overlay_mode)
> {
> while (pos < active_nr &&
> !strcmp(active_cache[pos]->name, ce->name)) {
> @@ -140,6 +142,8 @@ static int check_stage(int stage, const struct cache_entry *ce, int pos)
> return 0;
> pos++;
> }
> + if (!overlay_mode)
> + return 0;
> if (stage == 2)
> return error(_("path '%s' does not have our version"), ce->name);
> else
> @@ -165,7 +169,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)
> }
>
> static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
> - const struct checkout *state)
> + const struct checkout *state, int overlay_mode)
> {
> while (pos < active_nr &&
> !strcmp(active_cache[pos]->name, ce->name)) {
> @@ -173,6 +177,10 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
> return checkout_entry(active_cache[pos], state, NULL);
> pos++;
> }
> + if (!overlay_mode) {
> + unlink_entry(ce);
> + return 0;
> + }
> if (stage == 2)
> return error(_("path '%s' does not have our version"), ce->name);
> else
> @@ -302,15 +310,29 @@ static int checkout_paths(const struct checkout_opts *opts,
> ce->ce_flags &= ~CE_MATCHED;
> if (!opts->ignore_skipworktree && ce_skip_worktree(ce))
> continue;
> - if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
> - /*
> - * "git checkout tree-ish -- path", but this entry
> - * is in the original index; it will not be checked
> - * out to the working tree and it does not matter
> - * if pathspec matched this entry. We will not do
> - * anything to this entry at all.
> - */
> - continue;
> + if (opts->source_tree && !(ce->ce_flags & CE_UPDATE)) {
> + if (!opts->overlay_mode &&
> + ce_path_match(&the_index, ce, &opts->pathspec, ps_matched)) {
> + /*
> + * "git checkout --no-overlay <tree-ish> -- path",
> + * and the path is not in tree-ish, but is in
> + * the current index, which means that it should
> + * be removed.
> + */
> + ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> + continue;
> + } else {
> + /*
> + * "git checkout tree-ish -- path", but this
> + * entry is in the original index; it will not
> + * be checked out to the working tree and it
> + * does not matter if pathspec matched this
> + * entry. We will not do anything to this entry
> + * at all.
> + */
> + continue;
> + }
> + }
> /*
> * Either this entry came from the tree-ish we are
> * checking the paths out of, or we are checking out
> @@ -348,7 +370,7 @@ static int checkout_paths(const struct checkout_opts *opts,
> if (opts->force) {
> warning(_("path '%s' is unmerged"), ce->name);
> } else if (opts->writeout_stage) {
> - errs |= check_stage(opts->writeout_stage, ce, pos);
> + errs |= check_stage(opts->writeout_stage, ce, pos, opts->overlay_mode);
> } else if (opts->merge) {
> errs |= check_stages((1<<2) | (1<<3), ce, pos);
> } else {
> @@ -375,12 +397,14 @@ static int checkout_paths(const struct checkout_opts *opts,
> continue;
> }
> if (opts->writeout_stage)
> - errs |= checkout_stage(opts->writeout_stage, ce, pos, &state);
> + errs |= checkout_stage(opts->writeout_stage, ce, pos, &state, opts->overlay_mode);
> else if (opts->merge)
> errs |= checkout_merged(pos, &state);
> pos = skip_same_name(ce, pos) - 1;
> }
> }
> + remove_marked_cache_entries(&the_index, 1);
> + remove_scheduled_dirs();
> errs |= finish_delayed_checkout(&state);
>
> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> @@ -542,6 +566,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
> * opts->show_progress only impacts output so doesn't require a merge
> */
>
> + /*
> + * opts->overlay_mode cannot be used with switching branches so is
> + * not tested here
> + */
> +
> /*
> * If we aren't creating a new branch any changes or updates will
> * happen in the existing branch. Since that could only be updating
> @@ -1178,6 +1207,10 @@ static int checkout_branch(struct checkout_opts *opts,
> die(_("'%s' cannot be used with switching branches"),
> "--patch");
>
> + if (!opts->overlay_mode)
> + die(_("'%s' cannot be used with switching branches"),
> + "--no-overlay");
> +
> if (opts->writeout_stage)
> die(_("'%s' cannot be used with switching branches"),
> "--ours/--theirs");
> @@ -1266,6 +1299,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> "checkout", "control recursive updating of submodules",
> PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> + OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> OPT_END(),
> };
>
> @@ -1274,6 +1308,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> opts.overwrite_ignore = 1;
> opts.prefix = prefix;
> opts.show_progress = -1;
> + opts.overlay_mode = -1;
>
> git_config(git_checkout_config, &opts);
>
> @@ -1297,6 +1332,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
> die(_("-b, -B and --orphan are mutually exclusive"));
>
> + if (opts.overlay_mode == 1 && opts.patch_mode)
> + die(_("-p and --overlay are mutually exclusive"));
> +
> /*
> * From here on, new_branch will contain the branch to be checked out,
> * and new_branch_force and new_orphan_branch will tell us which one of
> diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
> new file mode 100755
> index 0000000000..3575321382
> --- /dev/null
> +++ b/t/t2025-checkout-no-overlay.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +test_description='checkout --no-overlay <tree-ish> -- <pathspec>'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + git commit --allow-empty -m "initial"
> +'
> +
> +test_expect_success 'checkout --no-overlay deletes files not in <tree>' '
> + >file &&
> + mkdir dir &&
> + >dir/file1 &&
> + git add file dir/file1 &&
> + git checkout --no-overlay HEAD -- file &&
> + test_path_is_missing file &&
> + test_path_is_file dir/file1
> +'
> +
> +test_expect_success 'checkout --no-overlay removing last file from directory' '
> + git checkout --no-overlay HEAD -- dir/file1 &&
> + test_path_is_missing dir
> +'
> +
> +test_expect_success 'checkout -p --overlay is disallowed' '
> + test_must_fail git checkout -p --overlay HEAD 2>actual &&
> + test_i18ngrep "fatal: -p and --overlay are mutually exclusive" actual
> +'
> +
> +test_expect_success '--no-overlay --theirs with M/D conflict deletes file' '
> + test_commit file1 file1 &&
> + test_commit file2 file2 &&
> + git rm --cached file1 &&
> + echo 1234 >file1 &&
> + F1=$(git rev-parse HEAD:file1) &&
> + F2=$(git rev-parse HEAD:file2) &&
> + {
> + echo "100644 $F1 1 file1" &&
> + echo "100644 $F2 2 file1"
> + } | git update-index --index-info &&
> + test_path_is_file file1 &&
> + git checkout --theirs --no-overlay -- file1 &&
> + test_path_is_missing file1
> +'
> +
> +test_done
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 175f83d704..a3fd9a9630 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1436,6 +1436,7 @@ test_expect_success 'double dash "git checkout"' '
> --progress Z
> --no-quiet Z
> --no-... Z
> + --overlay Z
> EOF
> '
>
> --
> 2.20.0.405.gbc1bbc6f85
>
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Duy Nguyen @ 2018-12-10 18:19 UTC (permalink / raw)
To: Elijah Newren; +Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CABPp-BEkeRa7jOkDcNNpZMY9J9JmNGtMKjZeNv8i_u7jUFihcw@mail.gmail.com>
On Mon, Dec 10, 2018 at 7:09 PM Elijah Newren <newren@gmail.com> wrote:
> > > For the two current callsites, unpack-trees seems to do this
> > > invalidation itself internally.
> >
> > I'm still a bit scared of this invalidation business in unpack-trees.
> > The thing is, we handle two separate index_state there, src_index and
> > result and invalidation has to be done on the right one (because index
> > extensions are on src_index until the very end of unpack-trees;
> > invalidating on 'result' would be no-op and wrong).
> > remove_marked_cache_entries() seems to be called on 'result' while
> > invalidate_ce_path() is on src_index, hm....
>
> Is Thomas avoiding problems here simply because merge is the only
> caller of unpack_trees with src_index != dst_index? Or does src_index
> == dst_index for checkout not actually help?
I think it would not help. 'result' is a temporary index where we copy
things to (and it does not have anything from the beginning). If you
invalidate stuff in there, you invalidate nothing, regardless whether
dst_index == src_index.
> If that does help with the checkout case, then allow me to find a
> different way to muddy the waters... I think I might want to make use
> of this function in the merge machinery at some point, so I either
> need to figure out how to convince you to verify if all this cache
> tree invalidation stuff is sane, or somehow figure out all the
> cache_tree stuff stuff myself so I can figure out what is right here.
> :-)
I'm not the unpack-trees man (I think that would still be Junio). And
I'm not saying it's sane either. I think it's just some leftover
things since Linus split "the index" in unpack-tree operation to
'src', 'result' and 'dst' many years ago and nobody was brave enough
to clean it up (then I piled on with untracked cache and split index,
but I did not see it clearly either). That person could be you ;-)
--
Duy
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Elijah Newren @ 2018-12-10 18:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc
Cc: Thomas Gummerer, Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8AiQvu8W4=2HLKMdg+n2HiDrcLvKPRurKvziXaJdqefRg@mail.gmail.com>
On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > When marking cache entries for removal, and later removing them all at
> > once using 'remove_marked_cache_entries()', cache entries currently
> > have to be invalidated manually in the cache tree and in the untracked
> > cache.
> >
> > Add an invalidate flag to the function. With the flag set, the
> > function will take care of invalidating the path in the cache tree and
> > in the untracked cache.
> >
> > This will be useful in a subsequent commit.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >
> > For the two current callsites, unpack-trees seems to do this
> > invalidation itself internally.
>
> I'm still a bit scared of this invalidation business in unpack-trees.
> The thing is, we handle two separate index_state there, src_index and
> result and invalidation has to be done on the right one (because index
> extensions are on src_index until the very end of unpack-trees;
> invalidating on 'result' would be no-op and wrong).
> remove_marked_cache_entries() seems to be called on 'result' while
> invalidate_ce_path() is on src_index, hm....
Is Thomas avoiding problems here simply because merge is the only
caller of unpack_trees with src_index != dst_index? Or does src_index
== dst_index for checkout not actually help?
If that does help with the checkout case, then allow me to find a
different way to muddy the waters... I think I might want to make use
of this function in the merge machinery at some point, so I either
need to figure out how to convince you to verify if all this cache
tree invalidation stuff is sane, or somehow figure out all the
cache_tree stuff stuff myself so I can figure out what is right here.
:-)
> > I don't quite understand why we don't
> > need it in split-index mode though. I assume it's because the cache
> > tree in the main index would already have been invalidated? I didn't
> > have much time to dig, but couldn't produce any failures with it
> > either, so I assume not invalidating paths is the right thing to do
> > here.
>
> Yeah I think it's because cache-tree and untracked cache are already
> properly invalidated. This merge base thingy is done when we load the
> index files up, not when we write them down. The "front" index may
> record that a few paths in the base index are no longer valid and need
> to be deleted. But untracked cache and cache-tree both should have
> recorded that same info when these paths are marked for delete at
> index write time.
^ permalink raw reply
* [PATCH 5/5] midx: implement midx_repack()
From: Derrick Stolee via GitGitGadget @ 2018-12-10 18:06 UTC (permalink / raw)
To: git; +Cc: sbeller, peff, jrnieder, avarab, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.92.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
To repack using a multi-pack-index, first sort all pack-files by
their modified time. Second, walk those pack-files from oldest
to newest, adding the packs to a list if they are smaller than the
given pack-size. Finally, collect the objects from the multi-pack-
index that are in those packs and send them to 'git pack-objects'.
While first designing a 'git multi-pack-index repack' operation, I
started by collecting the batches based on the size of the objects
instead of the size of the pack-files. This allows repacking a
large pack-file that has very few referencd objects. However, this
came at a significant cost of parsing pack-files instead of simply
reading the multi-pack-index and getting the file information for
the pack-files. This object-size idea could be a direction for
future expansion in this area.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
midx.c | 109 +++++++++++++++++++++++++++++++++++-
t/t5319-multi-pack-index.sh | 25 +++++++++
2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/midx.c b/midx.c
index 4caf148464..3718e78132 100644
--- a/midx.c
+++ b/midx.c
@@ -8,6 +8,7 @@
#include "sha1-lookup.h"
#include "midx.h"
#include "progress.h"
+#include "run-command.h"
#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
#define MIDX_VERSION 1
@@ -1116,7 +1117,113 @@ int expire_midx_packs(const char *object_dir)
return result;
}
-int midx_repack(const char *object_dir, size_t batch_size)
+struct time_and_id {
+ timestamp_t mtime;
+ uint32_t pack_int_id;
+};
+
+static int compare_by_mtime(const void *a_, const void *b_)
{
+ const struct time_and_id *a, *b;
+
+ a = (const struct time_and_id *)a_;
+ b = (const struct time_and_id *)b_;
+
+ if (a->mtime < b->mtime)
+ return -1;
+ if (a->mtime > b->mtime)
+ return 1;
return 0;
}
+
+int midx_repack(const char *object_dir, size_t batch_size)
+{
+ int result = 0;
+ uint32_t i, packs_to_repack;
+ size_t total_size;
+ struct time_and_id *pack_ti;
+ unsigned char *include_pack;
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf base_name = STRBUF_INIT;
+ struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
+
+ if (!m)
+ return 0;
+
+ include_pack = xcalloc(m->num_packs, sizeof(unsigned char));
+ pack_ti = xcalloc(m->num_packs, sizeof(struct time_and_id));
+
+ for (i = 0; i < m->num_packs; i++) {
+ pack_ti[i].pack_int_id = i;
+
+ if (prepare_midx_pack(m, i))
+ continue;
+
+ pack_ti[i].mtime = m->packs[i]->mtime;
+ }
+ QSORT(pack_ti, m->num_packs, compare_by_mtime);
+
+ total_size = 0;
+ packs_to_repack = 0;
+ for (i = 0; total_size < batch_size && i < m->num_packs; i++) {
+ int pack_int_id = pack_ti[i].pack_int_id;
+ struct packed_git *p = m->packs[pack_int_id];
+
+ if (!p)
+ continue;
+ if (p->pack_size >= batch_size)
+ continue;
+
+ packs_to_repack++;
+ total_size += p->pack_size;
+ include_pack[pack_int_id] = 1;
+ }
+
+ if (total_size < batch_size || packs_to_repack < 2)
+ goto cleanup;
+
+ argv_array_push(&cmd.args, "pack-objects");
+
+ strbuf_addstr(&base_name, object_dir);
+ strbuf_addstr(&base_name, "/pack/pack");
+ argv_array_push(&cmd.args, base_name.buf);
+ strbuf_release(&base_name);
+
+ cmd.git_cmd = 1;
+ cmd.in = cmd.out = -1;
+
+ if (start_command(&cmd)) {
+ error(_("could not start pack-objects"));
+ result = 1;
+ goto cleanup;
+ }
+
+ for (i = 0; i < m->num_objects; i++) {
+ struct object_id oid;
+ uint32_t pack_int_id = nth_midxed_pack_int_id(m, i);
+
+ if (!include_pack[pack_int_id])
+ continue;
+
+ nth_midxed_object_oid(&oid, m, i);
+ xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
+ xwrite(cmd.in, "\n", 1);
+ }
+ close(cmd.in);
+
+ if (finish_command(&cmd)) {
+ error(_("could not finish pack-objects"));
+ result = 1;
+ goto cleanup;
+ }
+
+ result = write_midx_internal(object_dir, m, NULL);
+ m = NULL;
+
+cleanup:
+ if (m)
+ close_midx(m);
+ free(include_pack);
+ free(pack_ti);
+ return result;
+}
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index c23e930a5d..3cc9c918d5 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -421,4 +421,29 @@ test_expect_success 'repack does not create any packs' '
)
'
+test_expect_success 'repack creates a new pack' '
+ (
+ cd dup &&
+ SECOND_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 2 | tail -n 1) &&
+ BATCH_SIZE=$(($SECOND_SMALLEST_SIZE + 1)) &&
+ git multi-pack-index repack --batch-size=$BATCH_SIZE &&
+ ls .git/objects/pack/*idx >idx-list &&
+ test_line_count = 5 idx-list &&
+ test-tool read-midx .git/objects | grep idx >midx-list &&
+ test_line_count = 5 midx-list
+ )
+'
+
+test_expect_success 'expire removes repacked packs' '
+ (
+ cd dup &&
+ ls -S .git/objects/pack/*pack | head -n 3 >expect &&
+ git multi-pack-index expire &&
+ ls -S .git/objects/pack/*pack >actual &&
+ test_cmp expect actual &&
+ test-tool read-midx .git/objects | grep idx >midx-list &&
+ test_line_count = 3 midx-list
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/5] multi-pack-index: prepare 'repack' verb
From: Derrick Stolee via GitGitGadget @ 2018-12-10 18:06 UTC (permalink / raw)
To: git; +Cc: sbeller, peff, jrnieder, avarab, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.92.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
In an environment where the multi-pack-index is useful, it is due
to many pack-files and an inability to repack the object store
into a single pack-file. However, it is likely that many of these
pack-files are rather small, and could be repacked into a slightly
larger pack-file without too much effort. It may also be important
to ensure the object store is highly available and the repack
operation does not interrupt concurrent git commands.
Introduce a 'repack' verb to 'git multi-pack-index' that takes a
'--batch-size' option. The verb will inspect the multi-pack-index
for referenced pack-files whose size is smaller than the batch
size, until collecting a list of pack-files whose sizes sum to
larger than the batch size. Then, a new pack-file will be created
containing the objects from those pack-files that are referenced
by the multi-pack-index. The resulting pack is likely to actually
be smaller than the batch size due to compression and the fact
that there may be objects in the pack-files that have duplicate
copies in other pack-files.
The current change introduces the command-line arguments, and we
add a test that ensures we parse these options properly. Since
we specify a small batch size, we will guarantee that future
implementations do not change the list of pack-files.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-multi-pack-index.txt | 12 ++++++++++++
builtin/multi-pack-index.c | 10 +++++++++-
midx.c | 5 +++++
midx.h | 1 +
t/t5319-multi-pack-index.sh | 11 +++++++++++
5 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
index 822d83c845..e43e7da71e 100644
--- a/Documentation/git-multi-pack-index.txt
+++ b/Documentation/git-multi-pack-index.txt
@@ -39,6 +39,18 @@ expire::
Rewrite the MIDX file afterward to remove all references to
these pack-files.
+repack::
+ When given as the verb, collect a batch of pack-files whose
+ size are all at most the size given by --batch-size, but
+ whose sizes sum to larger than --batch-size. The batch is
+ selected by greedily adding small pack-files starting with
+ the oldest pack-files that fit the size. Create a new pack-
+ file containing the objects the multi-pack-index indexes
+ into thos pack-files, and rewrite the multi-pack-index to
+ contain that pack-file. A later run of 'git multi-pack-index
+ expire' will delete the pack-files that were part of this
+ batch.
+
EXAMPLES
--------
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 145de3a46c..d87a2235e3 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -5,12 +5,13 @@
#include "midx.h"
static char const * const builtin_multi_pack_index_usage[] = {
- N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire)"),
+ N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)"),
NULL
};
static struct opts_multi_pack_index {
const char *object_dir;
+ unsigned long batch_size;
} opts;
int cmd_multi_pack_index(int argc, const char **argv,
@@ -19,6 +20,8 @@ int cmd_multi_pack_index(int argc, const char **argv,
static struct option builtin_multi_pack_index_options[] = {
OPT_FILENAME(0, "object-dir", &opts.object_dir,
N_("object directory containing set of packfile and pack-index pairs")),
+ OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
+ N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
OPT_END(),
};
@@ -40,6 +43,11 @@ int cmd_multi_pack_index(int argc, const char **argv,
return 1;
}
+ if (!strcmp(argv[0], "repack"))
+ return midx_repack(opts.object_dir, (size_t)opts.batch_size);
+ if (opts.batch_size)
+ die(_("--batch-size option is only for 'repack' verb"));
+
if (!strcmp(argv[0], "write"))
return write_midx_file(opts.object_dir);
if (!strcmp(argv[0], "verify"))
diff --git a/midx.c b/midx.c
index 50e4cd7270..4caf148464 100644
--- a/midx.c
+++ b/midx.c
@@ -1115,3 +1115,8 @@ int expire_midx_packs(const char *object_dir)
string_list_clear(&packs_to_drop, 0);
return result;
}
+
+int midx_repack(const char *object_dir, size_t batch_size)
+{
+ return 0;
+}
diff --git a/midx.h b/midx.h
index e3a2b740b5..394a21ee96 100644
--- a/midx.h
+++ b/midx.h
@@ -50,6 +50,7 @@ int write_midx_file(const char *object_dir);
void clear_midx_file(struct repository *r);
int verify_midx_file(const char *object_dir);
int expire_midx_packs(const char *object_dir);
+int midx_repack(const char *object_dir, size_t batch_size);
void close_midx(struct multi_pack_index *m);
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 210279a3cf..c23e930a5d 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -410,4 +410,15 @@ test_expect_success 'expire removes unreferenced packs' '
)
'
+test_expect_success 'repack does not create any packs' '
+ (
+ cd dup &&
+ ls .git/objects/pack >expect &&
+ MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
+ git multi-pack-index repack --batch-size=$MINSIZE &&
+ ls .git/objects/pack >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/5] midx: refactor permutation logic
From: Derrick Stolee via GitGitGadget @ 2018-12-10 18:06 UTC (permalink / raw)
To: git; +Cc: sbeller, peff, jrnieder, avarab, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.92.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When writing a multi-pack-index, we keep track of an integer
permutation, tracking the list of pack-files that we know about
(both from the existing multi-pack-index and the new pack-files
being introduced) and converting them into a sorted order for
the new multi-pack-index.
In anticipation of dropping pack-files from the existing multi-
pack-index, refactor the logic around how we track this permutation.
First, insert the permutation into the pack_list structure. This
allows us to grow the permutation dynamically as we add packs.
Second, fill the permutation with values corresponding to their
position in the list of pack-files, sorted as follows:
1. The pack-files in the existing multi-pack-index,
sorted lexicographically.
2. The pack-files not in the existing multi-pack-index,
sorted as discovered from the filesystem.
There is a subtle thing in how we initialize this permutation,
specifically how we use 'i' for the initial value. This will
matter more when we implement the logic for dropping existing
packs, as we will create holes in the ordering.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
midx.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/midx.c b/midx.c
index bb825ef816..3bd7183a53 100644
--- a/midx.c
+++ b/midx.c
@@ -380,9 +380,11 @@ static size_t write_midx_header(struct hashfile *f,
struct pack_list {
struct packed_git **list;
char **names;
+ uint32_t *perm;
uint32_t nr;
uint32_t alloc_list;
uint32_t alloc_names;
+ uint32_t alloc_perm;
size_t pack_name_concat_len;
struct multi_pack_index *m;
};
@@ -398,6 +400,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,
ALLOC_GROW(packs->list, packs->nr + 1, packs->alloc_list);
ALLOC_GROW(packs->names, packs->nr + 1, packs->alloc_names);
+ ALLOC_GROW(packs->perm, packs->nr + 1, packs->alloc_perm);
packs->list[packs->nr] = add_packed_git(full_path,
full_path_len,
@@ -417,6 +420,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,
return;
}
+ packs->perm[packs->nr] = packs->nr;
packs->names[packs->nr] = xstrdup(file_name);
packs->pack_name_concat_len += strlen(file_name) + 1;
packs->nr++;
@@ -443,7 +447,7 @@ static void sort_packs_by_name(char **pack_names, uint32_t nr_packs, uint32_t *p
ALLOC_ARRAY(pairs, nr_packs);
for (i = 0; i < nr_packs; i++) {
- pairs[i].pack_int_id = i;
+ pairs[i].pack_int_id = perm[i];
pairs[i].pack_name = pack_names[i];
}
@@ -755,7 +759,6 @@ int write_midx_file(const char *object_dir)
struct hashfile *f = NULL;
struct lock_file lk;
struct pack_list packs;
- uint32_t *pack_perm = NULL;
uint64_t written = 0;
uint32_t chunk_ids[MIDX_MAX_CHUNKS + 1];
uint64_t chunk_offsets[MIDX_MAX_CHUNKS + 1];
@@ -774,18 +777,22 @@ int write_midx_file(const char *object_dir)
packs.nr = 0;
packs.alloc_list = packs.m ? packs.m->num_packs : 16;
- packs.alloc_names = packs.alloc_list;
+ packs.alloc_perm = packs.alloc_names = packs.alloc_list;
packs.list = NULL;
packs.names = NULL;
+ packs.perm = NULL;
packs.pack_name_concat_len = 0;
ALLOC_ARRAY(packs.list, packs.alloc_list);
ALLOC_ARRAY(packs.names, packs.alloc_names);
+ ALLOC_ARRAY(packs.perm, packs.alloc_perm);
if (packs.m) {
for (i = 0; i < packs.m->num_packs; i++) {
ALLOC_GROW(packs.list, packs.nr + 1, packs.alloc_list);
ALLOC_GROW(packs.names, packs.nr + 1, packs.alloc_names);
+ ALLOC_GROW(packs.perm, packs.nr + 1, packs.alloc_perm);
+ packs.perm[packs.nr] = i;
packs.list[packs.nr] = NULL;
packs.names[packs.nr] = xstrdup(packs.m->pack_names[i]);
packs.pack_name_concat_len += strlen(packs.names[packs.nr]) + 1;
@@ -802,10 +809,9 @@ int write_midx_file(const char *object_dir)
packs.pack_name_concat_len += MIDX_CHUNK_ALIGNMENT -
(packs.pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
- ALLOC_ARRAY(pack_perm, packs.nr);
- sort_packs_by_name(packs.names, packs.nr, pack_perm);
+ sort_packs_by_name(packs.names, packs.nr, packs.perm);
- entries = get_sorted_entries(packs.m, packs.list, pack_perm, packs.nr, &nr_entries);
+ entries = get_sorted_entries(packs.m, packs.list, packs.perm, packs.nr, &nr_entries);
for (i = 0; i < nr_entries; i++) {
if (entries[i].offset > 0x7fffffff)
@@ -923,8 +929,8 @@ cleanup:
free(packs.list);
free(packs.names);
+ free(packs.perm);
free(entries);
- free(pack_perm);
free(midx_name);
return 0;
}
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox