* [PATCH v4 8/8] rebase: Implement --merge via the interactive machinery
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181211161139.31686-1-newren@gmail.com>
As part of an ongoing effort to make rebase have more uniform behavior,
modify the merge backend to behave like the interactive one, by
re-implementing it on top of the latter.
Interactive rebases are implemented in terms of cherry-pick rather than
the merge-recursive builtin, but cherry-pick also calls into the
recursive merge machinery by default and can accept special merge
strategies and/or special strategy options. As such, there really is
not any need for having both git-rebase--merge and
git-rebase--interactive anymore. Delete git-rebase--merge.sh and
instead implement it in builtin/rebase.c.
This results in a few deliberate but small user-visible changes:
* The progress output is modified (see t3406 and t3420 for examples)
* A few known test failures are now fixed (see t3421)
* bash-prompt during a rebase --merge is now REBASE-i instead of
REBASE-m. Reason: The prompt is a reflection of the backend in use;
this allows users to report an issue to the git mailing list with
the appropriate backend information, and allows advanced users to
know where to search for relevant control files. (see t9903)
testcase modification notes:
t3406: --interactive and --merge had slightly different progress output
while running; adjust a test to match the new expectation
t3420: these test precise output while running, but rebase--am,
rebase--merge, and rebase--interactive all were built on very
different commands (am, merge-recursive, cherry-pick), so the
tests expected different output for each type. Now we expect
--merge and --interactive to have the same output.
t3421: --interactive fixes some bugs in --merge! Wahoo!
t9903: --merge uses the interactive backend so the prompt expected is
now REBASE-i.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
.gitignore | 1 -
Documentation/git-rebase.txt | 17 +--
Makefile | 1 -
builtin/rebase.c | 15 ++-
git-legacy-rebase.sh | 43 ++++----
git-rebase--merge.sh | 166 ------------------------------
t/t3406-rebase-message.sh | 7 +-
t/t3420-rebase-autostash.sh | 78 ++------------
t/t3421-rebase-topology-linear.sh | 10 +-
t/t9903-bash-prompt.sh | 2 +-
10 files changed, 43 insertions(+), 297 deletions(-)
delete mode 100644 git-rebase--merge.sh
diff --git a/.gitignore b/.gitignore
index 0d77ea5894..910b1d2d2f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -124,7 +124,6 @@
/git-rebase--am
/git-rebase--common
/git-rebase--interactive
-/git-rebase--merge
/git-rebase--preserve-merges
/git-receive-pack
/git-reflog
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index dff17b3178..8bfa36a185 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
INCOMPATIBLE OPTIONS
--------------------
-git-rebase has many flags that are incompatible with each other,
-predominantly due to the fact that it has three different underlying
-implementations:
-
- * one based on linkgit:git-am[1] (the default)
- * one based on git-merge-recursive (merge backend)
- * one based on linkgit:git-cherry-pick[1] (interactive backend)
-
-Flags only understood by the am backend:
+The following options:
* --committer-date-is-author-date
* --ignore-date
@@ -520,15 +512,12 @@ Flags only understood by the am backend:
* --ignore-whitespace
* -C
-Flags understood by both merge and interactive backends:
+are incompatible with the following options:
* --merge
* --strategy
* --strategy-option
* --allow-empty-message
-
-Flags only understood by the interactive backend:
-
* --[no-]autosquash
* --rebase-merges
* --preserve-merges
@@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
* --edit-todo
* --root when used in combination with --onto
-Other incompatible flag pairs:
+In addition, the following pairs of options are incompatible:
* --preserve-merges and --interactive
* --preserve-merges and --signoff
diff --git a/Makefile b/Makefile
index 1a44c811aa..82e1eb1a4a 100644
--- a/Makefile
+++ b/Makefile
@@ -628,7 +628,6 @@ SCRIPT_LIB += git-parse-remote
SCRIPT_LIB += git-rebase--am
SCRIPT_LIB += git-rebase--common
SCRIPT_LIB += git-rebase--preserve-merges
-SCRIPT_LIB += git-rebase--merge
SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ec2e5fbf23..d95843a8d4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -122,7 +122,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
case REBASE_PRESERVE_MERGES:
break;
case REBASE_MERGE:
- /* we silently *upgrade* --merge to --interactive if needed */
+ /* we now implement --merge via --interactive */
default:
opts->type = REBASE_INTERACTIVE; /* implied */
break;
@@ -481,10 +481,6 @@ static int run_specific_rebase(struct rebase_options *opts)
backend = "git-rebase--am";
backend_func = "git_rebase__am";
break;
- case REBASE_MERGE:
- backend = "git-rebase--merge";
- backend_func = "git_rebase__merge";
- break;
case REBASE_PRESERVE_MERGES:
backend = "git-rebase--preserve-merges";
backend_func = "git_rebase__preserve_merges";
@@ -1191,6 +1187,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
+ if (options.type == REBASE_MERGE)
+ imply_interactive(&options, "--merge");
+
if (options.root && !options.onto_name)
imply_interactive(&options, "--root without --onto");
@@ -1220,10 +1219,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (is_interactive(&options) && i >= 0)
- die(_("cannot combine am options "
- "with interactive options"));
- if (options.type == REBASE_MERGE && i >= 0)
- die(_("cannot combine am options with merge options "));
+ die(_("cannot combine am options with either "
+ "interactive or merge options"));
}
if (options.signoff) {
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 6baf10192d..0c9c19bc60 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -218,6 +218,7 @@ then
state_dir="$apply_dir"
elif test -d "$merge_dir"
then
+ type=interactive
if test -d "$merge_dir"/rewritten
then
type=preserve-merges
@@ -225,10 +226,7 @@ then
preserve_merges=t
elif test -f "$merge_dir"/interactive
then
- type=interactive
interactive_rebase=explicit
- else
- type=merge
fi
state_dir="$merge_dir"
fi
@@ -477,6 +475,7 @@ then
test -z "$interactive_rebase" && interactive_rebase=implied
fi
+actually_interactive=
if test -n "$interactive_rebase"
then
if test -z "$preserve_merges"
@@ -485,11 +484,12 @@ then
else
type=preserve-merges
fi
-
+ actually_interactive=t
state_dir="$merge_dir"
elif test -n "$do_merge"
then
- type=merge
+ interactive_rebase=implied
+ type=interactive
state_dir="$merge_dir"
else
type=am
@@ -505,13 +505,9 @@ incompatible_opts=$(echo " $git_am_opt " | \
sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
if test -n "$incompatible_opts"
then
- if test -n "$interactive_rebase"
- then
- die "$(gettext "fatal: cannot combine am options with interactive options")"
- fi
- if test -n "$do_merge"
+ if test -n "$actually_interactive" || test "$do_merge"
then
- die "$(gettext "fatal: cannot combine am options with merge options")"
+ die "$(gettext "fatal: cannot combine am options with either interactive or merge options")"
fi
fi
@@ -676,7 +672,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
# but this should be done only when upstream and onto are the same
# and if this is not an interactive rebase.
mb=$(git merge-base "$onto" "$orig_head")
-if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
+if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
test "$mb" = "$onto" && test -z "$restrict_revision" &&
# linear history?
! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
@@ -726,6 +722,19 @@ then
GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
fi
+if test -z "$actually_interactive" && test "$mb" = "$orig_head"
+then
+ say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
+ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
+ git checkout -q "$onto^0" || die "could not detach HEAD"
+ # If the $onto is a proper descendant of the tip of the branch, then
+ # we just fast-forwarded.
+ git update-ref ORIG_HEAD $orig_head
+ move_to_original_branch
+ finish_rebase
+ exit 0
+fi
+
test -n "$interactive_rebase" && run_specific_rebase
# Detach HEAD and reset the tree
@@ -735,16 +744,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
git checkout -q "$onto^0" || die "could not detach HEAD"
git update-ref ORIG_HEAD $orig_head
-# If the $onto is a proper descendant of the tip of the branch, then
-# we just fast-forwarded.
-if test "$mb" = "$orig_head"
-then
- say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
- move_to_original_branch
- finish_rebase
- exit 0
-fi
-
if test -n "$rebase_root"
then
revisions="$onto..$orig_head"
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
deleted file mode 100644
index ced38bb3a6..0000000000
--- a/git-rebase--merge.sh
+++ /dev/null
@@ -1,166 +0,0 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its merge-based non-interactive mode that copes well with renamed
-# files.
-#
-# Copyright (c) 2010 Junio C Hamano.
-#
-
-prec=4
-
-read_state () {
- onto_name=$(cat "$state_dir"/onto_name) &&
- end=$(cat "$state_dir"/end) &&
- msgnum=$(cat "$state_dir"/msgnum)
-}
-
-continue_merge () {
- test -d "$state_dir" || die "$state_dir directory does not exist"
-
- unmerged=$(git ls-files -u)
- if test -n "$unmerged"
- then
- echo "You still have unmerged paths in your index"
- echo "did you forget to use git add?"
- die "$resolvemsg"
- fi
-
- cmt=$(cat "$state_dir/current")
- if ! git diff-index --quiet --ignore-submodules HEAD --
- then
- if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} $signoff $allow_empty_message \
- --no-verify -C "$cmt"
- then
- echo "Commit failed, please do not call \"git commit\""
- echo "directly, but instead do one of the following: "
- die "$resolvemsg"
- fi
- if test -z "$GIT_QUIET"
- then
- printf "Committed: %0${prec}d " $msgnum
- fi
- echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
- else
- if test -z "$GIT_QUIET"
- then
- printf "Already applied: %0${prec}d " $msgnum
- fi
- fi
- test -z "$GIT_QUIET" &&
- GIT_PAGER='' git log --format=%s -1 "$cmt"
-
- # onto the next patch:
- msgnum=$(($msgnum + 1))
- echo "$msgnum" >"$state_dir/msgnum"
-}
-
-call_merge () {
- msgnum="$1"
- echo "$msgnum" >"$state_dir/msgnum"
- cmt="$(cat "$state_dir/cmt.$msgnum")"
- echo "$cmt" > "$state_dir/current"
- git update-ref REBASE_HEAD "$cmt"
- hd=$(git rev-parse --verify HEAD)
- cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
- eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
- eval GITHEAD_$hd='$onto_name'
- export GITHEAD_$cmt GITHEAD_$hd
- if test -n "$GIT_QUIET"
- then
- GIT_MERGE_VERBOSITY=1 && export GIT_MERGE_VERBOSITY
- fi
- test -z "$strategy" && strategy=recursive
- # If cmt doesn't have a parent, don't include it as a base
- base=$(git rev-parse --verify --quiet $cmt^)
- eval 'git merge-$strategy' $strategy_opts $base ' -- "$hd" "$cmt"'
- rv=$?
- case "$rv" in
- 0)
- unset GITHEAD_$cmt GITHEAD_$hd
- return
- ;;
- 1)
- git rerere $allow_rerere_autoupdate
- die "$resolvemsg"
- ;;
- 2)
- echo "Strategy: $strategy failed, try another" 1>&2
- die "$resolvemsg"
- ;;
- *)
- die "Unknown exit code ($rv) from command:" \
- "git merge-$strategy $cmt^ -- HEAD $cmt"
- ;;
- esac
-}
-
-finish_rb_merge () {
- move_to_original_branch
- if test -s "$state_dir"/rewritten
- then
- git notes copy --for-rewrite=rebase <"$state_dir"/rewritten
- hook="$(git rev-parse --git-path hooks/post-rewrite)"
- test -x "$hook" && "$hook" rebase <"$state_dir"/rewritten
- fi
- say All done.
-}
-
-git_rebase__merge () {
-
-case "$action" in
-continue)
- read_state
- continue_merge
- while test "$msgnum" -le "$end"
- do
- call_merge "$msgnum"
- continue_merge
- done
- finish_rb_merge
- return
- ;;
-skip)
- read_state
- git rerere clear
- cmt="$(cat "$state_dir/cmt.$msgnum")"
- echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
- msgnum=$(($msgnum + 1))
- while test "$msgnum" -le "$end"
- do
- call_merge "$msgnum"
- continue_merge
- done
- finish_rb_merge
- return
- ;;
-show-current-patch)
- exec git show REBASE_HEAD --
- ;;
-esac
-
-mkdir -p "$state_dir"
-echo "$onto_name" > "$state_dir/onto_name"
-write_basic_state
-rm -f "$(git rev-parse --git-path REBASE_HEAD)"
-
-msgnum=0
-for cmt in $(git rev-list --topo-order --reverse --no-merges "$revisions")
-do
- msgnum=$(($msgnum + 1))
- echo "$cmt" > "$state_dir/cmt.$msgnum"
-done
-
-echo 1 >"$state_dir/msgnum"
-echo $msgnum >"$state_dir/end"
-
-end=$msgnum
-msgnum=1
-
-while test "$msgnum" -le "$end"
-do
- call_merge "$msgnum"
- continue_merge
-done
-
-finish_rb_merge
-
-}
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index f64b130cb8..b393e1e9fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,14 +17,9 @@ test_expect_success 'setup' '
git tag start
'
-cat >expect <<\EOF
-Already applied: 0001 A
-Already applied: 0002 B
-Committed: 0003 Z
-EOF
-
test_expect_success 'rebase -m' '
git rebase -m master >report &&
+ >expect &&
sed -n -e "/^Already applied: /p" \
-e "/^Committed: /p" report >actual &&
test_cmp expect actual
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 4c7494cc8f..2d1094e483 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -53,41 +53,6 @@ create_expected_success_interactive () {
EOF
}
-create_expected_success_merge () {
- cat >expected <<-EOF
- $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
- First, rewinding head to replay your work on top of it...
- Merging unrelated-onto-branch with HEAD~1
- Merging:
- $(git rev-parse --short unrelated-onto-branch) unrelated commit
- $(git rev-parse --short feature-branch^) second commit
- found 1 common ancestor:
- $(git rev-parse --short feature-branch~2) initial commit
- [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
- Author: A U Thor <author@example.com>
- Date: Thu Apr 7 15:14:13 2005 -0700
- 2 files changed, 2 insertions(+)
- create mode 100644 file1
- create mode 100644 file2
- Committed: 0001 second commit
- Merging unrelated-onto-branch with HEAD~0
- Merging:
- $(git rev-parse --short rebased-feature-branch~1) second commit
- $(git rev-parse --short feature-branch) third commit
- found 1 common ancestor:
- $(git rev-parse --short feature-branch~1) second commit
- [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
- Author: A U Thor <author@example.com>
- Date: Thu Apr 7 15:15:13 2005 -0700
- 1 file changed, 1 insertion(+)
- create mode 100644 file3
- Committed: 0002 third commit
- All done.
- Applied autostash.
- EOF
-}
-
create_expected_failure_am () {
cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
@@ -112,43 +77,6 @@ create_expected_failure_interactive () {
EOF
}
-create_expected_failure_merge () {
- cat >expected <<-EOF
- $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
- HEAD is now at $(git rev-parse --short feature-branch) third commit
- First, rewinding head to replay your work on top of it...
- Merging unrelated-onto-branch with HEAD~1
- Merging:
- $(git rev-parse --short unrelated-onto-branch) unrelated commit
- $(git rev-parse --short feature-branch^) second commit
- found 1 common ancestor:
- $(git rev-parse --short feature-branch~2) initial commit
- [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
- Author: A U Thor <author@example.com>
- Date: Thu Apr 7 15:14:13 2005 -0700
- 2 files changed, 2 insertions(+)
- create mode 100644 file1
- create mode 100644 file2
- Committed: 0001 second commit
- Merging unrelated-onto-branch with HEAD~0
- Merging:
- $(git rev-parse --short rebased-feature-branch~1) second commit
- $(git rev-parse --short feature-branch) third commit
- found 1 common ancestor:
- $(git rev-parse --short feature-branch~1) second commit
- [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
- Author: A U Thor <author@example.com>
- Date: Thu Apr 7 15:15:13 2005 -0700
- 1 file changed, 1 insertion(+)
- create mode 100644 file3
- Committed: 0002 third commit
- All done.
- Applying autostash resulted in conflicts.
- Your changes are safe in the stash.
- You can run "git stash pop" or "git stash drop" at any time.
- EOF
-}
-
testrebase () {
type=$1
dotest=$2
@@ -177,6 +105,9 @@ testrebase () {
test_expect_success "rebase$type --autostash: check output" '
test_when_finished git branch -D rebased-feature-branch &&
suffix=${type#\ --} && suffix=${suffix:-am} &&
+ if test ${suffix} = "merge"; then
+ suffix=interactive
+ fi &&
create_expected_success_$suffix &&
test_i18ncmp expected actual
'
@@ -274,6 +205,9 @@ testrebase () {
test_expect_success "rebase$type: check output with conflicting stash" '
test_when_finished git branch -D rebased-feature-branch &&
suffix=${type#\ --} && suffix=${suffix:-am} &&
+ if test ${suffix} = "merge"; then
+ suffix=interactive
+ fi &&
create_expected_failure_$suffix &&
test_i18ncmp expected actual
'
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index 23ad4cff35..7274dca40b 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -111,7 +111,7 @@ test_run_rebase () {
"
}
test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
test_run_rebase success -i
test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -126,7 +126,7 @@ test_run_rebase () {
"
}
test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
test_run_rebase success -i
test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -141,7 +141,7 @@ test_run_rebase () {
"
}
test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
test_run_rebase success -i
test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -284,7 +284,7 @@ test_run_rebase () {
"
}
test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
test_run_rebase success -i
test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -315,7 +315,7 @@ test_run_rebase () {
"
}
test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
test_run_rebase success -i
test_have_prereq !REBASE_P || test_run_rebase failure -p
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 81a5179e28..5cadedb2a9 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
'
test_expect_success 'prompt - rebase merge' '
- printf " (b2|REBASE-m 1/3)" >expected &&
+ printf " (b2|REBASE-i 1/3)" >expected &&
git checkout b2 &&
test_when_finished "git checkout master" &&
test_must_fail git rebase --merge b1 b2 &&
--
2.20.0.8.g5de428d695
^ permalink raw reply related
* [PATCH v4 6/8] git-legacy-rebase: simplify unnecessary triply-nested if
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181211161139.31686-1-newren@gmail.com>
The git-legacy-rebase.sh script previously had code of the form:
if git_am_opt:
if interactive:
if incompatible_opts:
show_error_about_interactive_and_am_incompatibilities
if rebase-merge:
if incompatible_opts
show_error_about_merge_and_am_incompatibilities
which was a triply nested if. However, the first conditional
(git_am_opt) and third (incompatible_opts) were somewhat redundant: the
latter condition was a strict subset of the former. Simplify this by
moving the innermost conditional to the outside, allowing us to remove
the test on git_am_opt entirely and giving us the following form:
if incompatible_opts:
if interactive:
show_error_about_interactive_and_am_incompatibilities
if rebase-merge:
show_error_about_merge_and_am_incompatibilities
Signed-off-by: Elijah Newren <newren@gmail.com>
---
git-legacy-rebase.sh | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index f4088b7bda..6baf10192d 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -501,21 +501,17 @@ then
git_format_patch_opt="$git_format_patch_opt --progress"
fi
-if test -n "$git_am_opt"; then
- incompatible_opts=$(echo " $git_am_opt " | \
- sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
+incompatible_opts=$(echo " $git_am_opt " | \
+ sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
+if test -n "$incompatible_opts"
+then
if test -n "$interactive_rebase"
then
- if test -n "$incompatible_opts"
- then
- die "$(gettext "fatal: cannot combine am options with interactive options")"
- fi
+ die "$(gettext "fatal: cannot combine am options with interactive options")"
fi
- if test -n "$do_merge"; then
- if test -n "$incompatible_opts"
- then
- die "$(gettext "fatal: cannot combine am options with merge options")"
- fi
+ if test -n "$do_merge"
+ then
+ die "$(gettext "fatal: cannot combine am options with merge options")"
fi
fi
--
2.20.0.8.g5de428d695
^ permalink raw reply related
* [PATCH v4 2/8] rebase: fix incompatible options error message
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181211161139.31686-1-newren@gmail.com>
In commit f57696802c30 ("rebase: really just passthru the `git am`
options", 2018-11-14), the handling of `git am` options was simplified
dramatically (and an option parsing bug was fixed), but it introduced
a small regression in the error message shown when options only
understood by separate backends were used:
$ git rebase --keep --ignore-whitespace
fatal: cannot combine interactive options (--interactive, --exec,
--rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
am options (.git/rebase-apply/applying)
$ git rebase --merge --ignore-whitespace
fatal: cannot combine merge options (--merge, --strategy,
--strategy-option) with am options (.git/rebase-apply/applying)
Note that in both cases, the list of "am options" is
".git/rebase-apply/applying", which makes no sense. Since the lists of
backend-specific options is documented pretty thoroughly in the rebase
man page (in the "Incompatible Options" section, with multiple links
throughout the document), and since I expect this list to change over
time, just simplify the error message.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/rebase.c | 10 +++-------
git-legacy-rebase.sh | 4 ++--
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 85f980bdce..78e982298f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1223,14 +1223,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (is_interactive(&options) && i >= 0)
- die(_("cannot combine interactive options "
- "(--interactive, --exec, --rebase-merges, "
- "--preserve-merges, --keep-empty, --root + "
- "--onto) with am options (%s)"), buf.buf);
+ die(_("cannot combine am options "
+ "with interactive options"));
if (options.type == REBASE_MERGE && i >= 0)
- die(_("cannot combine merge options (--merge, "
- "--strategy, --strategy-option) with am options "
- "(%s)"), buf.buf);
+ die(_("cannot combine am options with merge options "));
}
if (options.signoff) {
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 11548e927c..fccb33b959 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -508,13 +508,13 @@ if test -n "$git_am_opt"; then
then
if test -n "$incompatible_opts"
then
- die "$(gettext "fatal: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
+ die "$(gettext "fatal: cannot combine am options with interactive options")"
fi
fi
if test -n "$do_merge"; then
if test -n "$incompatible_opts"
then
- die "$(gettext "fatal: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
+ die "$(gettext "fatal: cannot combine am options with merge options")"
fi
fi
fi
--
2.20.0.8.g5de428d695
^ permalink raw reply related
* [PATCH v4 0/8] Reimplement rebase --merge via interactive machinery
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181122044841.20993-1-newren@gmail.com>
This series continues the work of making rebase more self-consistent
by removing inconsistencies between different backends. In
particular, this series focuses on making the merge machinery behave
like the interactive machinery (though a few differences between the am
and interactive backends are also fixed along the way), and ultimately
removes the merge backend in favor of reimplementing the relevant
options on top of the interactive machinery.
Differences since v3 (full range-diff below):
- Fixed the redundant "fatal: error:" error message prefixes, as pointed
out by Duy
- Rebased on 2.20.0
Elijah Newren (8):
rebase: make builtin and legacy script error messages the same
rebase: fix incompatible options error message
t5407: add a test demonstrating how interactive handles --skip
differently
am, rebase--merge: do not overlook --skip'ed commits with post-rewrite
git-rebase, sequencer: extend --quiet option for the interactive
machinery
git-legacy-rebase: simplify unnecessary triply-nested if
rebase: define linearization ordering and enforce it
rebase: Implement --merge via the interactive machinery
.gitignore | 1 -
Documentation/git-rebase.txt | 17 +---
Makefile | 1 -
builtin/am.c | 9 ++
builtin/rebase.c | 30 ++----
git-legacy-rebase.sh | 65 ++++++------
git-rebase--am.sh | 2 +-
git-rebase--common.sh | 2 +-
git-rebase--merge.sh | 164 ------------------------------
sequencer.c | 23 +++--
sequencer.h | 1 +
t/t3406-rebase-message.sh | 7 +-
t/t3420-rebase-autostash.sh | 78 ++------------
t/t3421-rebase-topology-linear.sh | 10 +-
t/t3425-rebase-topology-merges.sh | 15 ++-
t/t5407-post-rewrite-hook.sh | 34 +++++++
t/t9903-bash-prompt.sh | 2 +-
17 files changed, 121 insertions(+), 340 deletions(-)
delete mode 100644 git-rebase--merge.sh
Range-diff:
-: ---------- > 1: 2e8b1bcb8b rebase: make builtin and legacy script error messages the same
1: 2f4bdd1980 ! 2: eba87828c6 rebase: fix incompatible options error message
@@ -9,12 +9,12 @@
understood by separate backends were used:
$ git rebase --keep --ignore-whitespace
- fatal: error: cannot combine interactive options (--interactive, --exec,
+ fatal: cannot combine interactive options (--interactive, --exec,
--rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
am options (.git/rebase-apply/applying)
$ git rebase --merge --ignore-whitespace
- fatal: error: cannot combine merge options (--merge, --strategy,
+ fatal: cannot combine merge options (--merge, --strategy,
--strategy-option) with am options (.git/rebase-apply/applying)
Note that in both cases, the list of "am options" is
@@ -33,18 +33,17 @@
break;
if (is_interactive(&options) && i >= 0)
-- die(_("error: cannot combine interactive options "
+- die(_("cannot combine interactive options "
- "(--interactive, --exec, --rebase-merges, "
- "--preserve-merges, --keep-empty, --root + "
- "--onto) with am options (%s)"), buf.buf);
-+ die(_("error: cannot combine am options "
++ die(_("cannot combine am options "
+ "with interactive options"));
if (options.type == REBASE_MERGE && i >= 0)
-- die(_("error: cannot combine merge options (--merge, "
+- die(_("cannot combine merge options (--merge, "
- "--strategy, --strategy-option) with am options "
- "(%s)"), buf.buf);
-+ die(_("error: cannot combine am options "
-+ "with merge options "));
++ die(_("cannot combine am options with merge options "));
}
if (options.signoff) {
@@ -56,15 +55,15 @@
then
if test -n "$incompatible_opts"
then
-- die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
-+ die "$(gettext "error: cannot combine am options with interactive options")"
+- die "$(gettext "fatal: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
++ die "$(gettext "fatal: cannot combine am options with interactive options")"
fi
fi
if test -n "$do_merge"; then
if test -n "$incompatible_opts"
then
-- die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
-+ die "$(gettext "error: cannot combine am options with merge options")"
+- die "$(gettext "fatal: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
++ die "$(gettext "fatal: cannot combine am options with merge options")"
fi
fi
fi
2: cc33a8ccc1 = 3: 15d929edb2 t5407: add a test demonstrating how interactive handles --skip differently
3: f5838ef763 = 4: c9d6d5141e am, rebase--merge: do not overlook --skip'ed commits with post-rewrite
4: 50dc863d9f = 5: 0b19ad8e2d git-rebase, sequencer: extend --quiet option for the interactive machinery
5: 35cf552f27 ! 6: 5ded8654ec git-legacy-rebase: simplify unnecessary triply-nested if
@@ -18,7 +18,7 @@
moving the innermost conditional to the outside, allowing us to remove
the test on git_am_opt entirely and giving us the following form:
- if incomptable_opts:
+ if incompatible_opts:
if interactive:
show_error_about_interactive_and_am_incompatibilities
if rebase-merge:
@@ -44,18 +44,18 @@
then
- if test -n "$incompatible_opts"
- then
-- die "$(gettext "error: cannot combine am options with interactive options")"
+- die "$(gettext "fatal: cannot combine am options with interactive options")"
- fi
-+ die "$(gettext "error: cannot combine am options with interactive options")"
++ die "$(gettext "fatal: cannot combine am options with interactive options")"
fi
- if test -n "$do_merge"; then
- if test -n "$incompatible_opts"
- then
-- die "$(gettext "error: cannot combine am options with merge options")"
+- die "$(gettext "fatal: cannot combine am options with merge options")"
- fi
+ if test -n "$do_merge"
+ then
-+ die "$(gettext "error: cannot combine am options with merge options")"
++ die "$(gettext "fatal: cannot combine am options with merge options")"
fi
fi
6: 2a3d8ff1c1 = 7: bb8e5a4527 rebase: define linearization ordering and enforce it
7: 58371d377a ! 8: 5de428d695 rebase: Implement --merge via the interactive machinery
@@ -142,14 +142,15 @@
imply_interactive(&options, "--root without --onto");
@@
+ break;
if (is_interactive(&options) && i >= 0)
- die(_("error: cannot combine am options "
+- die(_("cannot combine am options "
- "with interactive options"));
- if (options.type == REBASE_MERGE && i >= 0)
-- die(_("error: cannot combine am options "
-- "with merge options "));
-+ "with either interactive or merge options"));
+- die(_("cannot combine am options with merge options "));
++ die(_("cannot combine am options with either "
++ "interactive or merge options"));
}
if (options.signoff) {
@@ -205,13 +206,13 @@
then
- if test -n "$interactive_rebase"
- then
-- die "$(gettext "error: cannot combine am options with interactive options")"
+- die "$(gettext "fatal: cannot combine am options with interactive options")"
- fi
- if test -n "$do_merge"
+ if test -n "$actually_interactive" || test "$do_merge"
then
-- die "$(gettext "error: cannot combine am options with merge options")"
-+ die "$(gettext "error: cannot combine am options with either interactive or merge options")"
+- die "$(gettext "fatal: cannot combine am options with merge options")"
++ die "$(gettext "fatal: cannot combine am options with either interactive or merge options")"
fi
fi
@@ -225,7 +226,7 @@
# linear history?
! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
@@
- GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
+ GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
fi
+if test -z "$actually_interactive" && test "$mb" = "$orig_head"
--
2.20.0.8.g5de428d695
^ permalink raw reply
* [PATCH v4 4/8] am, rebase--merge: do not overlook --skip'ed commits with post-rewrite
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181211161139.31686-1-newren@gmail.com>
The post-rewrite hook is supposed to be invoked for each rewritten
commit. The fact that a commit was selected and processed by the rebase
operation (even though when we hit an error a user said it had no more
useful changes), suggests we should write an entry for it. In
particular, let's treat it as an empty commit trivially squashed into
its parent.
This brings the rebase--am and rebase--merge backends in sync with the
behavior of the interactive rebase backend.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/am.c | 9 +++++++++
git-rebase--merge.sh | 2 ++
t/t5407-post-rewrite-hook.sh | 3 +++
3 files changed, 14 insertions(+)
diff --git a/builtin/am.c b/builtin/am.c
index 8f27f3375b..af9d034838 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2000,6 +2000,15 @@ static void am_skip(struct am_state *state)
if (clean_index(&head, &head))
die(_("failed to clean index"));
+ if (state->rebasing) {
+ FILE *fp = xfopen(am_path(state, "rewritten"), "a");
+
+ assert(!is_null_oid(&state->orig_commit));
+ fprintf(fp, "%s ", oid_to_hex(&state->orig_commit));
+ fprintf(fp, "%s\n", oid_to_hex(&head));
+ fclose(fp);
+ }
+
am_next(state);
am_load(state);
am_run(state, 0);
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index aa2f2f0872..91250cbaed 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -121,6 +121,8 @@ continue)
skip)
read_state
git rerere clear
+ cmt="$(cat "$state_dir/cmt.$msgnum")"
+ echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
msgnum=$(($msgnum + 1))
while test "$msgnum" -le "$end"
do
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 6426ec8991..a4a5903cba 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -78,6 +78,7 @@ test_expect_success 'git rebase --skip' '
git rebase --continue &&
echo rebase >expected.args &&
cat >expected.data <<-EOF &&
+ $(git rev-parse C) $(git rev-parse HEAD^)
$(git rev-parse D) $(git rev-parse HEAD)
EOF
verify_hook_input
@@ -91,6 +92,7 @@ test_expect_success 'git rebase --skip the last one' '
echo rebase >expected.args &&
cat >expected.data <<-EOF &&
$(git rev-parse E) $(git rev-parse HEAD)
+ $(git rev-parse F) $(git rev-parse HEAD)
EOF
verify_hook_input
'
@@ -120,6 +122,7 @@ test_expect_success 'git rebase -m --skip' '
git rebase --continue &&
echo rebase >expected.args &&
cat >expected.data <<-EOF &&
+ $(git rev-parse C) $(git rev-parse HEAD^)
$(git rev-parse D) $(git rev-parse HEAD)
EOF
verify_hook_input
--
2.20.0.8.g5de428d695
^ permalink raw reply related
* [PATCH v4 1/8] rebase: make builtin and legacy script error messages the same
From: Elijah Newren @ 2018-12-11 16:11 UTC (permalink / raw)
To: git
Cc: gitster, Johannes.Schindelin, predatoramigo, phillip.wood,
Elijah Newren
In-Reply-To: <20181211161139.31686-1-newren@gmail.com>
The conversion of the script version of rebase took messages that were
prefixed with "error:" and passed them along to die(), which adds a
"fatal:" prefix, thus resulting in messages of the form:
fatal: error: cannot combine...
which seems redundant. Remove the "error:" prefix from the builtin
version of rebase, and change the prefix from "error:" to "fatal:" in
the legacy script to match.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/rebase.c | 10 +++++-----
git-legacy-rebase.sh | 12 ++++++------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b5c99ec10c..85f980bdce 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1223,12 +1223,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (is_interactive(&options) && i >= 0)
- die(_("error: cannot combine interactive options "
+ die(_("cannot combine interactive options "
"(--interactive, --exec, --rebase-merges, "
"--preserve-merges, --keep-empty, --root + "
"--onto) with am options (%s)"), buf.buf);
if (options.type == REBASE_MERGE && i >= 0)
- die(_("error: cannot combine merge options (--merge, "
+ die(_("cannot combine merge options (--merge, "
"--strategy, --strategy-option) with am options "
"(%s)"), buf.buf);
}
@@ -1248,15 +1248,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
* git-rebase.txt caveats with "unless you know what you are doing"
*/
if (options.rebase_merges)
- die(_("error: cannot combine '--preserve-merges' with "
+ die(_("cannot combine '--preserve-merges' with "
"'--rebase-merges'"));
if (options.rebase_merges) {
if (strategy_options.nr)
- die(_("error: cannot combine '--rebase-merges' with "
+ die(_("cannot combine '--rebase-merges' with "
"'--strategy-option'"));
if (options.strategy)
- die(_("error: cannot combine '--rebase-merges' with "
+ die(_("cannot combine '--rebase-merges' with "
"'--strategy'"));
}
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b4c7dbfa57..11548e927c 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -508,13 +508,13 @@ if test -n "$git_am_opt"; then
then
if test -n "$incompatible_opts"
then
- die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
+ die "$(gettext "fatal: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
fi
fi
if test -n "$do_merge"; then
if test -n "$incompatible_opts"
then
- die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
+ die "$(gettext "fatal: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
fi
fi
fi
@@ -522,7 +522,7 @@ fi
if test -n "$signoff"
then
test -n "$preserve_merges" &&
- die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
+ die "$(gettext "fatal: cannot combine '--signoff' with '--preserve-merges'")"
git_am_opt="$git_am_opt $signoff"
force_rebase=t
fi
@@ -533,15 +533,15 @@ then
# Note: incompatibility with --interactive is just a strong warning;
# 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'")"
+ die "$(gettext "fatal: cannot combine '--preserve-merges' with '--rebase-merges'")"
fi
if test -n "$rebase_merges"
then
test -n "$strategy_opts" &&
- die "$(gettext "error: cannot combine '--rebase-merges' with '--strategy-option'")"
+ die "$(gettext "fatal: cannot combine '--rebase-merges' with '--strategy-option'")"
test -n "$strategy" &&
- die "$(gettext "error: cannot combine '--rebase-merges' with '--strategy'")"
+ die "$(gettext "fatal: cannot combine '--rebase-merges' with '--strategy'")"
fi
if test -z "$rebase_root"
--
2.20.0.8.g5de428d695
^ permalink raw reply related
* Re: [PATCH] parse-options: fix SunCC compiler warning
From: Duy Nguyen @ 2018-12-11 15:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Git Mailing List,
Eric Sunshine, SZEDER Gábor
In-Reply-To: <xmqqk1kgn73c.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 11, 2018 at 3:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > The reason it's in parse_options_step() is because -h is also handled
> > in there. Although -h does not bury exit() deep in the call chain. So
> > how about this as a replacement?
>
> So just like -h returns PARSE_OPT_HELP and causes the calling
> parse_options() to exit(129), the new _COMPLETE can be used to
> trigger an early & successful return?
>
> We'd also have to cover builtin/{blame,shortlog,update-index}.c
> where they switch() on the return value from parse_options_step(),
> but other than that, it probably is a better approach. The users of
> parse_options_step() that avoid parse_options() want to handle the
> various "we stopped by hitting a non-option" cases in their own
> ways, so treating this special action the same way as -h is treated
> would be sensible.
Yeah, I saw that shortly after sending the patch.
> We _might_ want to standardize some of the case
> arms in these custom switch statements that appear in these three
> built-in command implementations, but that can be done more easily
> if this step is done like you showed below, I think.
I have a better plan: stop exposing parse-options loop to outside.
What all these commands need is the ability to deal with unknown
options (or non-options in update-index case). They could register
callbacks to deal with those and keep calling parse_options() instead.
I'm converting rev-list to use parse_options() and as an intermediate
step, this callback idea should work well. The end game though is a
single parse_options() call that covers everything in, say, git-log.
But we will see.
--
Duy
^ permalink raw reply
* [PATCH v2] parse-options: fix SunCC compiler warning
From: Nguyễn Thái Ngọc Duy @ 2018-12-11 15:35 UTC (permalink / raw)
To: pclouds; +Cc: avarab, git, gitster, sunshine, szeder.dev
In-Reply-To: <20181209102724.8707-1-pclouds@gmail.com>
The compiler reports this because show_gitcomp() never actually
returns a value:
"parse-options.c", line 520: warning: Function has no return
statement : show_gitcomp
We could shut the compiler up. But instead let's not bury exit() too
deep. Do the same as internal -h handling, return a special error code
and handle the exit() in parse_options() (and other
parse_options_step() callers) instead.
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/blame.c | 2 ++
builtin/shortlog.c | 2 ++
builtin/update-index.c | 2 ++
parse-options.c | 4 +++-
parse-options.h | 1 +
5 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 06a7163ffe..6d798f9939 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -850,6 +850,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
case PARSE_OPT_HELP:
case PARSE_OPT_ERROR:
exit(129);
+ case PARSE_OPT_COMPLETE:
+ exit(0);
case PARSE_OPT_DONE:
if (ctx.argv[0])
dashdash_pos = ctx.cpidx;
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 88f88e97b2..65cd41392c 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -287,6 +287,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
case PARSE_OPT_HELP:
case PARSE_OPT_ERROR:
exit(129);
+ case PARSE_OPT_COMPLETE:
+ exit(0);
case PARSE_OPT_DONE:
goto parse_done;
}
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 31e7cce301..e19da77edc 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1086,6 +1086,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
case PARSE_OPT_HELP:
case PARSE_OPT_ERROR:
exit(129);
+ case PARSE_OPT_COMPLETE:
+ exit(0);
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_DONE:
{
diff --git a/parse-options.c b/parse-options.c
index 3b874a83a0..6932eaff61 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -516,7 +516,7 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
show_negated_gitcomp(original_opts, -1);
show_negated_gitcomp(original_opts, nr_noopts);
fputc('\n', stdout);
- exit(0);
+ return PARSE_OPT_COMPLETE;
}
static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -638,6 +638,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
case PARSE_OPT_HELP:
case PARSE_OPT_ERROR:
exit(129);
+ case PARSE_OPT_COMPLETE:
+ exit(0);
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_DONE:
break;
diff --git a/parse-options.h b/parse-options.h
index 6c4fe2016d..a650a7d220 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -208,6 +208,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags);
/*----- incremental advanced APIs -----*/
enum {
+ PARSE_OPT_COMPLETE = -2,
PARSE_OPT_HELP = -1,
PARSE_OPT_DONE,
PARSE_OPT_NON_OPTION,
--
2.20.0.481.gca8ed29094
^ permalink raw reply related
* Re: Announcing Pro Git Second Edition Reedited
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 15:13 UTC (permalink / raw)
To: nobozo; +Cc: Jeff King, Git Mailing List
In-Reply-To: <a1941151-9453-5830-7175-7c8e27425274@gmail.com>
On Tue, Dec 11, 2018 at 4:02 PM Jon Forrest <nobozo@gmail.com> wrote:
> On 12/11/2018 2:50 AM, Jeff King wrote:
>
> > The content at https://git-scm.com/book is pulled regularly from
> > https://github.com/progit/progit2, which has collected a number of fixes
> > (as well as translations) since the 2nd edition was released.
> >
> > Have you considered sending some of your edits there? It sounds like
> > they may be too large to just dump as a big PR, but it might be possible
> > to grow together over time.
>
> Fair question. I had tried doing this for the first edition of Pro Git,
> but the person who was in charge of accepting changes wasn't a
> native speaker of English. As a result I had a hard time
> convincing him that my changes were necessary. Many of my changes
> were very subjective, and not technical, so this was hard to overcome.
> Things might have been different if I were correcting technical errors
> or adding significant sections to the book. But, since I'm not a Git
> expert, that's not what I was attempting to do.
>
> Things have changed for the better for the second edition of Pro Git.
> Its management seems much more willing to accept the kind of changes
> I make, as shown by their reaction to the excellent work by Robert
> Day. Even so, given the amount of changes I've made, it's unlikely
> that my changes would be accepted.
>
> I do track the changes to the second edition of Pro Git and
> incorporate that ones that still apply into my version.
>
> But I hear what you're saying. Maybe if and when the third edition
> of Pro Git comes out I'll try what you suggest.
As someone who's read neither your edit or the original edition, but I
did read your version of the intro, it would be very helpful to me /
others if there was some diff between the two so we could make up our
own mind about which one to read, and to get an idea of what sorts of
wording changes etc. these are.
^ permalink raw reply
* [PATCH 2/2] help -a: handle aliases with long names gracefully
From: Johannes Schindelin via GitGitGadget @ 2018-12-11 14:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.97.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We take pains to determine the longest command beforehand, so that we
can align the category column after printing the command names.
However, then we re-use that value when printing the aliases. If any
alias name is longer than the longest command name, we consequently try
to add a negative number of spaces (but `mput_char()` does not expect
any negative values and simply decrements until the value is 0, i.e.
it tries to add close to 2**31 spaces).
Let's fix this by adjusting the `longest` variable before printing the
aliases.
This fixes https://github.com/git-for-windows/git/issues/1975.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
help.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/help.c b/help.c
index 4745b32299..ff05fd22df 100644
--- a/help.c
+++ b/help.c
@@ -83,8 +83,9 @@ static void print_command_list(const struct cmdname_help *cmds,
for (i = 0; cmds[i].name; i++) {
if (cmds[i].category & mask) {
+ size_t len = strlen(cmds[i].name);
printf(" %s ", cmds[i].name);
- mput_char(' ', longest - strlen(cmds[i].name));
+ mput_char(' ', longest > len ? longest - len : 1);
puts(_(cmds[i].help));
}
}
@@ -526,6 +527,13 @@ void list_all_cmds_help(void)
git_config(get_alias, &alias_list);
string_list_sort(&alias_list);
+
+ for (i = 0; i < alias_list.nr; i++) {
+ size_t len = strlen(alias_list.items[i].string);
+ if (longest < len)
+ longest = len;
+ }
+
if (alias_list.nr) {
printf("\n%s\n", _("Command aliases"));
ALLOC_ARRAY(aliases, alias_list.nr + 1);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] help.h: fix coding style
From: Johannes Schindelin via GitGitGadget @ 2018-12-11 14:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <pull.97.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We want a space after the `while` keyword.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
help.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/help.h b/help.h
index 9eab6a3f89..a141e209ae 100644
--- a/help.h
+++ b/help.h
@@ -15,7 +15,7 @@ struct cmdnames {
static inline void mput_char(char c, unsigned int num)
{
- while(num--)
+ while (num--)
putchar(c);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] Fix git help -a with long alias names
From: Johannes Schindelin via GitGitGadget @ 2018-12-11 14:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
The code added in 26c7d0678324 (help -a: improve and make --verbose default,
2018-09-29) that intends to print out aliases in addition to commands failed
to adjust for the length of the aliases. As a consequence, if there was any
alias whose name is longer than 18 characters, git help -a tried to print an
insanely large number of spaces, one at a time, causing what appeared to be
a "hang".
Let's fix this, and while at it fix a style issue that I saw on the way as
well.
Original report at https://github.com/git-for-windows/git/issues/1975
Johannes Schindelin (2):
help.h: fix coding style
help -a: handle aliases with long names gracefully
help.c | 10 +++++++++-
help.h | 2 +-
2 files changed, 10 insertions(+), 2 deletions(-)
base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-97%2Fdscho%2Ffix-help-a-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-97/dscho/fix-help-a-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/97
--
gitgitgadget
^ permalink raw reply
* Re: pw/add-p-select, was Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Phillip Wood @ 2018-12-11 14:48 UTC (permalink / raw)
To: Johannes Schindelin, phillip.wood; +Cc: Slavica Djukic, Junio C Hamano, git
In-Reply-To: <nycvar.QRO.7.76.6.1812111049560.43@tvgsbejvaqbjf.bet>
Hi Dscho and Slavica
On 11/12/2018 09:56, Johannes Schindelin wrote:
> Hi Phillip,
>
> [Cc:ing Slavica, the Outreachy intern working on converting `add -i` to a
> built-in]
>
> On Mon, 10 Dec 2018, Phillip Wood wrote:
>
>> On 09/12/2018 20:31, Johannes Schindelin wrote:
>>>
<snip/>
>>> (which I take as being inspired by Git GUI's "Stage Selected Line"),
>>
>> not particularly, I don't use git gui I just wanted a way to easily
>> stage a subset of lines without editing the hunk.
>
> Okay. I used to use Git GUI quite a bit to stage individual lines, but
> recently I tried to stay more in the terminal and used the `split` and
> `edit` commands of `add -p` quite extensively. Wishing for an quicker way
> to stage individual lines between all of my debug print statements.
Yes that's one of my main uses for selective staging as well
>>> and thought that it would be useful.
>>>
>>> I could imagine, however, that it would make sense for `git add -p` to
>>> imitate that feature more closely: by allowing to stage a single line
>>> and then presenting the current hunk (re-computed) again.
>>
>> that sounds like it would be quite tedious to stage more than a couple
>> of lines,
>
> It would be. But then, if you want to do anything slightly more
> complicated than staging a hunk or a line, I personally prefer the `edit`
> command *a lot*, as it lets me even split unrelated changes in the same
> line into two commits.
I was hoping for something simpler than editing patches just to stage a
subset of lines that do not need to be edited.
>> and it could be awkward to get it to stage modified lines correctly
>> (While I was writing the feature I tried git gui, I think it is supposed
>> to be able to stage modified lines correctly but I never persuaded it to
>> do it for me. I also tried gitg, tig and hg commit -i but I couldn't get
>> them to do modified lines either)
>
> Git GUI works very reliably for me, but then, I have Git for Windows'
> patched Git GUI at my finger tips (oh how I wish we had a Git GUI
> maintainer again).
>
> It should not be awkward to stage a single modified line at all.
> Essentially, you take the hunk, strip out all `-` and `+` lines except the
> one you want to stage, then stage that with `git apply --cached
> --recount`, and then you simply re-generate that hunk.
But that involves editing the hunk or have I misunderstood? The aim of
my series was that you'd select the '-' & '+' lines for the modification
and it would stage them properly as a modification so given
1 -a
2 -b
3 +c
selecting 1,3 would stage
-a
+c
b
not
-a
b
+c
(see https://public-inbox.org/git/878ta8vyqe.fsf@evledraar.gmail.com/
for the background)
>> I'll try and re-roll in the New Year, when does the outreachy project
>> for converting add -i start? - it would be good to try and get this
>> pinned down before then.
>
> Too late. Slavica started on December 4th, and you can even read about it
> on their blog: https://slavicadj.github.io/blog/
Thanks for the link and hello Slavica! - I look forward to reading your
patches when you post them here.
Best Wishes
Phillip
^ permalink raw reply
* Re: Difficulty with parsing colorized diff output
From: George King @ 2018-12-11 14:47 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20181211101742.GE31588@sigill.intra.peff.net>
Peff & Stefan, thank you for the feedback. For my purposes, I am content to rely on gitconfig to reduce the colors to something that I can parse without losing information. Since my first email I have found that `wsErrorHighlight = none` gets rid of the problematic extra green highlights in the `+` lines.
I still think that a config to differentiate log coloring from diff coloring would be worthwhile, as it would guarantee that highlighters are getting unadulterated lines from git.
I also think that bracketing every colored line with a color code before the space/plus/minus and a reset just before the newline would be smart, because then a parser can just parse line by line: if there is an SGR sequence before the space/plus/minus, then it would know to strip off the final reset. This is in contrast to how it stands now, where a context line (with no leading color) is ambiguous by itself; I have to remember from previous lines in the hunk that we have been seeing colors in order to know wether I should strip off the reset.
I agree that a machine-readable format would be nice, but regardless it would be useful to make the regular output more parser-friendly.
> On 2018-12-11, at 5:17 AM, Jeff King <peff@peff.net> wrote:
>
> On Mon, Dec 10, 2018 at 07:26:46PM -0800, Stefan Beller wrote:
>
>>> Context lines do have both. It's just that the default color for context
>>> lines is empty. ;)
>>
>> The content itself can contain color codes.
>>
>> Instead of unconditionally resetting each line, we could parse each
>> content line to determine if we actually have to reset the colors.
>
> Good point. I don't recall that being the motivation back when this
> behavior started, but it's a nice side effect (and the more recent line
> you mentioned in emit_line_0 certainly is doing it intentionally).
>
> That doesn't cover _other_ terminal codes, which could also make for
> confusing output, but I do think color codes are somewhat special. We
> generally send patches through "less -R", which will pass through the
> colors but show escaped versions of other codes.
>
>> Another idea would be to allow Git to output its output
>> as if it was run through test_decode_color, slightly related:
>> https://public-inbox.org/git/20180804015317.182683-8-sbeller@google.com/
>> i.e. we'd markup the output instead of coloring it.
>
> Yeah, I think in the most general form, the problem is that colorizing
> (including whitespace highlighting) loses information within a single
> line. It would be nice to have a machine-readable format that represents
> all the various annotations (like whitespace and coloring moved bits)
> that Git computes.
>
> -Peff
^ permalink raw reply
* Re: Announcing Pro Git Second Edition Reedited
From: Jon Forrest @ 2018-12-11 14:39 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20181211105007.GD7233@sigill.intra.peff.net>
On 12/11/2018 2:50 AM, Jeff King wrote:
> The content at https://git-scm.com/book is pulled regularly from
> https://github.com/progit/progit2, which has collected a number of fixes
> (as well as translations) since the 2nd edition was released.
>
> Have you considered sending some of your edits there? It sounds like
> they may be too large to just dump as a big PR, but it might be possible
> to grow together over time.
Fair question. I had tried doing this for the first edition of Pro Git,
but the person who was in charge of accepting changes wasn't a
native speaker of English. As a result I had a hard time
convincing him that my changes were necessary. Many of my changes
were very subjective, and not technical, so this was hard to overcome.
Things might have been different if I were correcting technical errors
or adding significant sections to the book. But, since I'm not a Git
expert, that's not what I was attempting to do.
Things have changed for the better for the second edition of Pro Git.
Its management seems much more willing to accept the kind of changes
I make, as shown by their reaction to the excellent work by Robert
Day. Even so, given the amount of changes I've made, it's unlikely
that my changes would be accepted.
I do track the changes to the second edition of Pro Git and
incorporate that ones that still apply into my version.
But I hear what you're saying. Maybe if and when the third edition
of Pro Git comes out I'll try what you suggest.
Cordially,
Jon Forrest
^ permalink raw reply
* Re: [PATCH] rebase -i: introduce the 'test' command
From: Jeff King @ 2018-12-11 14:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Eric Sunshine, Johannes Schindelin, paul.morelle, Git List
In-Reply-To: <8736r4nsna.fsf@evledraar.gmail.com>
On Tue, Dec 11, 2018 at 01:40:25PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> Are you thinking of the "break" command (not "pause") which Dscho
> >> already added[1]?
> >>
> >> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
> >
> > Yes, thanks (as you can see, I haven't actually used it yet ;) ).
>
> Related: I got poked about a bug where someone was doing "exec bash" to
> do the same, and would then CD to other repos, and git operations would
> still be executed on the original repo because we set GIT_DIR=* in the
> envioronment (but not for "break").
>
> Not a big deal, but I wondered if that was a bug in itself, i.e. if we
> need to set GIT_DIR et al in the environment for "exec". Isn't having
> the current directory set to the checkout sufficient?
This behavior is discussed in this sub-thread:
https://public-inbox.org/git/xmqqsh4jt5d0.fsf@gitster-ct.c.googlers.com/
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] protocol v2 and hidden refs
From: Jeff King @ 2018-12-11 13:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Brandon Williams, Jonathan Tan
In-Reply-To: <875zw0nv77.fsf@evledraar.gmail.com>
On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I don't know if there's a good solution. I tried running the whole
> > test suite with v2 as the default. It does find this bug, but it has
> > a bunch of other problems (notably fetch-pack won't run as v2, but
> > some other tests I think also depend on v0's reachability rules,
> > which v2 is documented not to enforce).
>
> I think a global test mode for it would be a very good idea.
Yeah, but somebody needs to pick through the dozens of false positives
for it to be useful.
> > The patches are:
> >
> > [1/3]: serve: pass "config context" through to individual commands
> > [2/3]: parse_hide_refs_config: handle NULL section
> > [3/3]: upload-pack: support hidden refs with protocol v2
>
> Does this issue rise to the level of needing a security point-release
> (which I'm discussing here as the details are already public). The
> transfer.hideRefs docs have said:
>
> Even if you hide refs, a client may still be able to steal the
> target objects via the techniques described in the "SECURITY"
> section of the gitnamespaces(7) man page; it’s best to keep private
> data in a separate repository.
>
> So we never promised to hide the objects, but definitely promised to
> hide the ref names. I don't know if anyone uses this in practice for
> secret ref names, but if they do they have a data leak if they enable
> protocol v2.
Yes, that was my line of thinking. You can't really consider such items
secure, though it is unfortunate that this leak makes it way easier to
access them (you can just fetch them, rather than playing
oracle-guessing games with deltas).
At GitHub we keep some internal book-keeping refs, but exposing them to
the user is mostly an annoyance.
One thing to note is that there's no "enable protocol v2". If you're
running a recent enough Git (v2.18+?) on the server, anybody can ask for
these refs.
> More importantly, the docs for receive.hideRefs say. "An attempt to
> update or delete a hidden ref by git push is rejected.". It seems this
> bit was enforced, i.e. this passes before and after your 3/3, but I have
> not dug enough to be 100% satisfied with that.
This part is OK. There is no v2 push protocol yet, so you end up running
the regular v0 receive-pack.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/3] git clone <url> C:\cygwin\home\USER\repo' is working (again)
From: Johannes Schindelin @ 2018-12-11 13:39 UTC (permalink / raw)
To: Steven Penny; +Cc: git
In-Reply-To: <CAAXzdLUKhCfvqdvsPryeMGJ2ttJxof4sUcyTx-xd5p2BaoryiQ@mail.gmail.com>
Hi Steven,
On Mon, 10 Dec 2018, Steven Penny wrote:
> On Mon, Dec 10, 2018 at 2:46 AM Johannes Schindelin wrote:
> > please stop dropping me from the Cc: list. Thanks.
>
> i dropped you specifically because i knew you were going to flame like
> you just did below. oh well, i guess you cant avoid the inevitable.
I have no intention of flaming anybody. That is simply a
misrepresentation.
> > > - pc-windows
> > > - pc-win
> > > - win
> >
> > I find all of those horrible.
>
> they arent great, but "win32" simply isnt valid.
It is a long established convention to talk about the Win32 API as the set
of functions developed for Windows NT and backported to Windows 95.
There is no benefit in abandoning that convention just to please you.
> > ... except if you take into account that this has been our convention
> > for, what, almost 9 years (since 44626dc7d5 (MSVC: Windows-native
> > implementation for subset of Pthreads API, 2010-01-15), to be
> > precise)? In that case, it makes a ton of sense, and one might be
> > tempted to ask who the person wanting to change that thinks they
> > are...
>
> "That's the way it's always been done" is not a good reason to keep
> doing something. I would say the justification goes the other way, as to
> why we should keep using an old moniker when its past making sense.
If you want to change something that has been in use for a long time, you
have to have good reasons. None of your arguments convinces me so far that
you have any good reason to change these.
Let's hear some good argument in a well-prepared patch, or alternatively
let's just not discuss these hypotheticals anymore.
> > You may disagree all you want, but given that Torsten has been a lot
> > more active on this code than you have been so far, I'll go with
> > Torsten's taste. Which incidentally happens to match my tastes, so
> > that's an added bonus.
>
> in the end i dont really care what your taste is, or Torsten for that
> matter. I care that the issue be fixed.
If anyone truly cares about an issue to be fixed, I would expect more
assisting, and less distracting, to do wonders.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 1/1] git clone <url> C:\cygwin\home\USER\repo' is working (again)
From: Johannes Schindelin @ 2018-12-11 13:28 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git, svnpenn
In-Reply-To: <20181211061204.GA1130@tor.lan>
[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]
Hi Torsten,
On Tue, 11 Dec 2018, Torsten Bögershausen wrote:
> On Mon, Dec 10, 2018 at 09:32:03AM +0100, Johannes Schindelin wrote:
> >
> > On Sat, 8 Dec 2018, tboegi@web.de wrote:
> >
> > > And, before any cleanup is done, I sould like to ask if anybody
> > > can build the code with VS and confirm that it works, please ?
> >
> > Can you give me an easy-to-fetch branch?
> >
> > Thanks,
> > Dscho
>
> @Dscho: The branch should be here:
> https://github.com/tboegi/git/tree/tb.181208_0844_cygwin-dos-drive
> (or fetch it from Junio, please see below:)
I fetched it from you, as Junio frequently applies patches anywhere except
where they were developed. I'd rather see what you see. For the record,
this is the commit I tested: cc1e08eeb83b.
It builds fine here, and some cursory tests reveal that it works as
advertised (I ran t0001, t0060 and t5580).
However.
Can you please replace the rather unnecessary, very, very long
`win_path_utils_` function name prefix by the much better prefix `win32_`,
to keep in line with the current, already existing, surrounding files'
convention? Thanks a bunch.
Ciao,
Dscho
> @Junio:
> Please keep tb/use-common-win32-pathfuncs-on-cygwin
> in pu for a while. I need to send a V4 to fix t5601 for cygwin.
>
>
^ permalink raw reply
* Re: [PATCH 5/5] midx: implement midx_repack()
From: Derrick Stolee @ 2018-12-11 13:00 UTC (permalink / raw)
To: Stefan Beller, gitgitgadget
Cc: git, Jeff King, Jonathan Nieder,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee
In-Reply-To: <CAGZ79kbPcy2U9XJA+Je0zRxFsQJGA9u8nfYZe_s75V8c97+dNw@mail.gmail.com>
On 12/10/2018 9:32 PM, Stefan Beller wrote:
> On Mon, Dec 10, 2018 at 10:06 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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'.
> Makes sense.
>
> With this operation we only coalesce some packfiles into a new
> pack file. So to perform the "complete" repack this command
> has to be run repeatedly until there is at most one packfile
> left that is smaller than batch size.
Well, the batch size essentially means "If a pack-file is larger than
<size>, then leave it be. I'm happy with packs that large." This assumes
that the reason the pack is that large is because it was already
combined with other packs or contains a lot of objects.
>
> Imagine the following scenario:
>
> There are 5 packfiles A, B, C, D, E,
> created last Monday thru Friday (A is oldest, E youngest).
> The sizes are [A=4, B=6, C=5, D=5, E=4]
>
> You'd issue a repack with batch size=10, such that
> A and B would be repacked into F, which is
> created today, size is less or equal than 10.
>
> You issue another repack tomorrow, which then would
> coalesce C and D to G, which is
> dated tomorrow, size is less or equal to 10 as well.
>
> You issue a third repack, which then takes E
> (as it is the oldest) and would probably find F as the
> next oldest (assuming it is less than 10), to repack
> into H.
>
> H is then compromised of A, B and E, and G is C+D.
>
> In a way these repacks, always picking up the oldest,
> sound like you "roll forward" objects into new packs.
> As the new packs are newest (we have no packs from
> the future), we'd cycle through different packs to look at
> for packing on each repacking.
>
> It is however more likely that content is more similar
> on a temporal basis. (e.g. I am boldly claiming that
> [ABC, DE] would take less space than [ABE, CD]
> as produced above).
>
> (The obvious solution to this hypothetical would be
> to backdate the resulting pack to the youngest pack
> that is input to the new pack, but I dislike fudging with
> the time a file is created/touched, so let's not go there)
This raises a good point about what happens when we "roll over" into the
"repacked" packs.
I'm not claiming that this is an optimal way to save space, but is a way
to incrementally collect small packs into slightly larger packs, all
while not interrupting concurrent Git commands. Reducing pack count
improves data locality, which is my goal here. In our environment, we do
see reduced space as a benefit, even if it is not optimal.
>
> Would the object count make sense as input instead of
> the pack date?
>
>
>> 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
> referenced
>
>> 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.
> Ah, that also explains why the above idea is toast.
>
> Would it make sense to extend or annotate the midx file
> to give hints at which packs are easy to combine?
>
> I guess such an "annotation worker" could run in a separate
> thread / pool with the lowest priority as this seems like a
> decent fallback for the lack of any better information how
> to pick the packfiles.
One idea I had earlier (and is in
Documentation/technical/multi-pack-index.txt) is to have the midx track
metadata about pack-files. We could avoid this "rollover" problem by
tracking which packs were repacked using this mechanism. This could
create a "pack generation" value, and we could collect a batch of packs
that have the same generation. This does seem a bit overcomplicated for
the potential benefit, and could waste better use of that metadata
concept. For instance, we could use the metadata to track the
information given by ".keep" and ".promisor" files.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH] run-command: report exec failure
From: Junio C Hamano @ 2018-12-11 12:50 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, John Passaro
In-Reply-To: <nycvar.QRO.7.76.6.1812111327460.43@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> This breaks on Windows (on Windows, the error message says "cannot spawn", see
Thanks for a quick feedback. Let's update to look for the pathname
of the command, as Peff suggested earlier.
^ permalink raw reply
* Re: [PATCH 4/5] multi-pack-index: prepare 'repack' verb
From: Derrick Stolee @ 2018-12-11 12:45 UTC (permalink / raw)
To: Stefan Beller, gitgitgadget
Cc: git, Jeff King, Jonathan Nieder,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee
In-Reply-To: <CAGZ79kaNJ8=iCunkeeHnMcOwFik9D-V4g9o7SuvLnnM0vvfBnA@mail.gmail.com>
On 12/10/2018 8:54 PM, Stefan Beller wrote:
> On Mon, Dec 10, 2018 at 10:06 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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.
> This highlights an optimization problem: How do we pick the
> batches optimally?
> Ideally we'd pick packs that have an overlap of many
> same objects to dedup them completely, next best would
> be to have objects that are very similar, such that they delta
> very well.
> (Assuming that the sum of the resulting pack sizes is a metric
> we'd want to optimize for eventually)
>
> For now it seems we just take a random cut of "small" packs.
>
>> 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.
> This is another clever trick that makes the test correct
> despite no implementation yet. :-)
>
>> +repack::
>> + When given as the verb, collect a batch of pack-files whose
>> + size are all at most the size given by --batch-size,
> okay.
>
>> but
>> + whose sizes sum to larger than --batch-size.
> or less if there are not enough packs.
If there are not enough packs to reach the batch size, then we do nothing.
> Now it would be interesting if we can specify an upper bound
> (e.g. my laptop has 8GB of ram, can I use this incremental
> repacking optimally by telling git to make batches of at most
> 7.5G), the answer seems to follow...
Well, this gets us to the "unsigned long" problem and how it pervades
the OPT_MAGNITUDE() code and things that use that kind of parameter.
This means that Windows users can specify a maximum of (1 << 32) - 1.
This size is intended to be large enough to make a reasonable change in
the pack organization without rewriting the entire repo (e.g. 1GB). If
there is another word than "repack" that means "collect objects from a
subset of pack-files and place them into a new pack-file, doing a small
amount of redeltification in the process" then I am open to suggestions.
I tried (briefly) to fix this dependence, but it got too large for me to
handle at the same time as this change. I'll consider revisiting it
again later.
>> 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
> those
>
>> + contain that pack-file. A later run of 'git multi-pack-index
>> + expire' will delete the pack-files that were part of this
>> + batch.
> ... but the optimization seems to be rather about getting rid
> of the oldest packs first instead of getting as close to the batch
> size. (e.g. another way to look at this is to "find the permutation
> of all packs that (each are smaller than batch size), but in sum
> are the smallest threshold above the batch size).
You are describing the subset-sum problem, with an additional
optimization component. While there are dynamic programming approaches
that are usually effective (if the sum is small), this problem is
NP-complete, and hence could lead to complications.
> I guess that the strategy of picking the oldest is just easiest
> to implement and should be sufficient for now, but memory
> bounds might be interesting to keep in mind, just as the
> optimal packing from above.
It is easy to implement, and fast. Further, we do have a heuristic that
the pack modified time correlates with the time the objects were
introduced to the repository and hence may compress well when placed
together.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
From: SZEDER Gábor @ 2018-12-11 12:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20181211110919.GC8452@sigill.intra.peff.net>
On Tue, Dec 11, 2018 at 06:09:19AM -0500, Jeff King wrote:
> On Sun, Dec 09, 2018 at 11:56:23PM +0100, SZEDER Gábor wrote:
>
> > 'test-lib.sh' looks for the presence of certain options like '--tee'
> > and '--verbose-log', so it can execute the test script again to save
> > its standard output and error. This happens way before the actual
> > option parsing loop, and the condition looking for these options looks
> > a bit odd, too. This patch series will add two more options to look
> > out for, and, in addition, will have to extract these options' stuck
> > arguments (i.e. '--opt=arg') as well.
> >
> > Add a proper option parsing loop to check these select options early
> > in 'test-lib.sh', making this early option checking more readable and
> > keeping those later changes in this series simpler. Use a 'for opt in
> > "$@"' loop to iterate over the options to preserve "$@" intact, so
> > options like '--verbose-log' can execute the test script again with
> > all the original options.
> >
> > As an alternative, we could parse all options early, but there are
> > options that do require an _unstuck_ argument, which is tricky to
> > handle properly in such a for loop, and the resulting complexity is,
> > in my opinion, worse than having this extra, partial option parsing
> > loop.
>
> In general, I'm not wild about having multiple option-parsing loops that
> skip the normal left-to-right parsing, since it introduces funny corner
> cases (like "-foo --bar" which should be the same as "--foo=--bar"
> instead thinking that "--bar" was passed as an option).
Yeah, that's already an "issue" in the current implementation as well,
though there are no such options that require options as argument.
> But looking at what this is replacing:
>
> > -case "$GIT_TEST_TEE_STARTED, $* " in
> > -done,*)
> > - # do not redirect again
> > - ;;
> > -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
Anyway, I had another crack at turning the current option parsing loop
into a for loop keeping $@ intact, and the results don't look all that
bad this time. Note that this diff below only does the while -> for
conversion, but leaves the loop where it is, so the changes are easily
visible. The important bits are the conditions at the beginning of
the loop and after the loop, and the handling of '-r'; the rest is
mostly s/shift// and sort-of s/$1/$opt/.
Thoughts? Is it better than two loops? I think it's better.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a3f7930a3..efdb6be3c8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,58 +264,65 @@ test "x$TERM" != "xdumb" && (
) &&
color=t
-while test "$#" -ne 0
+store_arg_to=
+prev_opt=
+for opt
do
- case "$1" in
+ if test -n "$store_arg_to"
+ then
+ eval $store_arg_to=\$opt
+ store_arg_to=
+ prev_opt=
+ continue
+ fi
+ case "$opt" in
-d|--d|--de|--deb|--debu|--debug)
- debug=t; shift ;;
+ debug=t ;;
-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
- immediate=t; shift ;;
+ immediate=t ;;
-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
- GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;;
+ GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
-r)
- shift; test "$#" -ne 0 || {
- echo 'error: -r requires an argument' >&2;
- exit 1;
- }
- run_list=$1; shift ;;
+ store_arg_to=run_list
+ prev_opt=-r
+ ;;
--run=*)
- run_list=${1#--*=}; shift ;;
+ run_list=${opt#--*=} ;;
-h|--h|--he|--hel|--help)
- help=t; shift ;;
+ help=t ;;
-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
- verbose=t; shift ;;
+ verbose=t ;;
--verbose-only=*)
- verbose_only=${1#--*=}
- shift ;;
+ verbose_only=${opt#--*=}
+ ;;
-q|--q|--qu|--qui|--quie|--quiet)
# Ignore --quiet under a TAP::Harness. Saying how many tests
# passed without the ok/not ok details is always an error.
- test -z "$HARNESS_ACTIVE" && quiet=t; shift ;;
+ test -z "$HARNESS_ACTIVE" && quiet=t ;;
--with-dashes)
- with_dashes=t; shift ;;
+ with_dashes=t ;;
--no-color)
- color=; shift ;;
+ color= ;;
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
valgrind=memcheck
- shift ;;
+ ;;
--valgrind=*)
- valgrind=${1#--*=}
- shift ;;
+ valgrind=${opt#--*=}
+ ;;
--valgrind-only=*)
- valgrind_only=${1#--*=}
- shift ;;
+ valgrind_only=${opt#--*=}
+ ;;
--tee)
- shift ;; # was handled already
+ ;; # was handled already
--root=*)
- root=${1#--*=}
- shift ;;
+ root=${opt#--*=}
+ ;;
--chain-lint)
GIT_TEST_CHAIN_LINT=1
- shift ;;
+ ;;
--no-chain-lint)
GIT_TEST_CHAIN_LINT=0
- shift ;;
+ ;;
-x)
# Some test scripts can't be reliably traced with '-x',
# unless the test is run with a Bash version supporting
@@ -335,15 +342,21 @@ do
else
echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
fi
- shift ;;
+ ;;
-V|--verbose-log)
verbose_log=t
- shift ;;
+ ;;
*)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
done
+if test -n "$store_arg_to"
+then
+ echo "error: $prev_opt requires an argument" >&2
+ exit 1
+fi
+
if test -n "$valgrind_only"
then
test -z "$valgrind" && valgrind=memcheck
^ permalink raw reply related
* Re: [PATCH] rebase -i: introduce the 'test' command
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 12:40 UTC (permalink / raw)
To: Jeff King; +Cc: Eric Sunshine, Johannes Schindelin, paul.morelle, Git List
In-Reply-To: <20181202033110.GA30004@sigill.intra.peff.net>
On Sun, Dec 02 2018, Jeff King wrote:
> On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote:
>
>> On Sat, Dec 1, 2018 at 3:02 PM Jeff King <peff@peff.net> wrote:
>> > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote:
>> > > In reality, I think that it would even make sense to change the default to
>> > > reschedule failed `exec` commands. Which is why I suggested to also add a
>> > > config option.
>> >
>> > I sometimes add "x false" to the top of the todo list to stop and create
>> > new commits before the first one. That would be awkward if I could never
>> > get past that line. However, I think elsewhere a "pause" line has been
>> > discussed, which would serve the same purpose.
>>
>> Are you thinking of the "break" command (not "pause") which Dscho
>> already added[1]?
>>
>> [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
>
> Yes, thanks (as you can see, I haven't actually used it yet ;) ).
Related: I got poked about a bug where someone was doing "exec bash" to
do the same, and would then CD to other repos, and git operations would
still be executed on the original repo because we set GIT_DIR=* in the
envioronment (but not for "break").
Not a big deal, but I wondered if that was a bug in itself, i.e. if we
need to set GIT_DIR et al in the environment for "exec". Isn't having
the current directory set to the checkout sufficient?
^ permalink raw reply
* Re: [PATCH] http: add http.version option to select http protocol version
From: Johannes Schindelin @ 2018-12-11 12:40 UTC (permalink / raw)
To: Eric Sunshine; +Cc: silvio.fricke, Git List
In-Reply-To: <CAPig+cQW5_9fH-P8X50Mx5kGJRwEOskw2L49Ajk+3D4xWpcOHg@mail.gmail.com>
Hi Eric,
On Mon, 10 Dec 2018, Eric Sunshine wrote:
> On Mon, Dec 10, 2018 at 5:50 PM Silvio Fricke <silvio.fricke@gmail.com> wrote:
> > HTTP has several protocol versions. By default, libcurl is using HTTP/2
> > today and check if the remote can use this protocol variant and fall
> > back to a previous version if not.
> >
> > Under rare conditions it is needed to switch the used protocol version
> > to fight again wrongly implemented authorization mechanism like ntlm
> > with gssapi on remote side.
Please note that this has been addressed for NTLM in
https://github.com/curl/curl/pull/3345 and the gssapi problem is probably
worked around by https://github.com/curl/curl/pull/3349.
Both patches were backported to the cURL version included in Git for
Windows v2.20.0.
> > Signed-off-by: Silvio Fricke <silvio.fricke@gmail.com>
>
> This looks very similar to [1] which is already in Junio's "next"
> branch (although not yet in a released version of Git).
Small correction: it is in Git *for Windows* v2.20.0, so in a manner of
speaking it *is* in a released version of Git.
The reason: even if we included the NTLM/Kerberos patches in Git for
Windows, there might be other scenarios where neither of those patches
catch.
Ciao,
Johannes
> [1]: https://public-inbox.org/git/71f8b71b346f132d0dc9a23c9a7f2ca2cb91966f.1541735051.git.gitgitgadget@gmail.com/
>
^ 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