* Re: Git bisect does not find commit introducing the bug
From: Christian Couder @ 2017-02-19 13:07 UTC (permalink / raw)
To: Alex Hoffman; +Cc: Johannes Sixt, Stephan Beyer, git
In-Reply-To: <CAMX8fZVeAEJ5tfCO_4Pebnq=rysaJ2xDMjH-9pjmPeF4FziLFw@mail.gmail.com>
On Sun, Feb 19, 2017 at 12:32 PM, Alex Hoffman <spec@gal.ro> wrote:
>> At the end of the git-bisect man page (in the SEE ALSO section) there
>> is a link to https://github.com/git/git/blob/master/Documentation/git-bisect-lk2009.txt
>> which has a lot of details about how bisect works.
>
> Thanks for pointing out the SEE ALSO section. I think it makes sense
> to include/describe the entire algorithm in the man page itself,
> although I am not sure whether the graphs would be always correctly
> visually represented in the man page format.
It would possibly be very long to describe the entire algorithm, as it
can be quite complex in some cases and it is difficult to understand
without graphs. Maybe we could describe it, or some parts of it, in a
separate document and provide links at different places in the man
page.
Anyway feel free to send patches.
>> The goal is to find the first bad commit, which is a commit that has
>> only good parents.
>
> OK, bisect's mission is more exact than I thought, which is good. M
Good that you seem to agree with this goal.
>> As o1 is an ancestor of G, then o1 is considered good by the bisect algorithm.
>> If it was bad, it would means that there is a transition from bad to
>> good between o1 and G.
>> But when a good commit is an ancestor of the bad commit, git bisect
>> makes the assumption that there is no transition from bad to good in
>> the graph.
>
> The assumption that there is no transition from bad to good in the
> graph did not hold in my example and it does not hold when a feature
> was recently introduced and gets broken relative shortly afterwards.
> But I consider it is easy to change the algorithm not to assume, but
> rather to check it.
I don't think the default algorithm will change soon, but there have
been discussions for a long time about adding options to use different
algorithms.
For example people have been discussing a "--first-parent" option for
many years as well as recently. It would bisect only along the first
parents of the involved commits, and it could help find the merge
commit that introduced a bug in the mainline.
>> git bisect makes some assumptions that are true most of the time, so
>> in practice it works well most of the time.
>
> Whatever the definition of "most of the time" everyone might have, I
> think there is room for improvement.
So feel free to send patches that would implement an option with the
improvements you want.
> Below I am trying to make a small
> change to the current algorithm in order to deal with the assumption
> that sometimes does not hold (e.g in my example), by explicitly
> validating the check.
>
>> --o1--o2--o3--G--X1
>> \ \
>> x1--x2--x3--x4--X2--B--
>> \ /
>> y1--y2--y3
>
> Step 1a. (Unchanged) keep only the commits that:
>
> a) are ancestor of the "bad" commit (including the "bad" commit itself),
> b) are not ancestor of a "good" commit (excluding the "good" commits).
>
> The following graph results:
> x1--x2--x3--x4--X2--B--
> \ /
> y1--y2--y3
I would say that the above graph is missing X1.
> Step 1b. (New) Mark all root commits of the resulting graph (i.e
> commits without parents) as unconfirmed (unconfirmed=node that has
> only bad parents). Remove all root commits that user already confirmed
> (e.g if user already marked its parent as good right before starting
> bisect run). For every unconfirmed root commit check if it has any
> good parents. In the example above check whether x1 has good parents.
I think I understand the above...
> If the current root element has any parents and none of them is
> good, we can delete all paths from it until to the next commit that
> has a parent in the ancestors of GOOD. In the example above to delete
> the path x1-x3 and x1-y3. Also add new resulting root commits to the
> list of unconfirmed commits (commit x4).
> Otherwise mark it as confirmed.
... but I don't understand the logic of the above. If the root element
has a bad parent, then it means that the "first bad commit" is either
the bad parent or one of its ancestors, so it is not logical to delete
it. In your example if x1 has one bad parent, this bad parent and its
ancestors should be included in the search of the first bad commit.
Otherwise the goal is not any more to find the first bad commit.
PS: I saw that you have just sent another version of the algorithm,
but I don't want to take a look at it right now. Anyway I am keeping
my above comments as they might still be useful.
> Step2. Continue the existing algorithm.
>
>
> If this improvement works (i.e you do not find any bugs in it and it
> is feasible to implement, which seems to me)
As you describe it, I don't think it is compatible with the goal of
finding the first bad commit.
Also there are many things that are feasible to implement, but it
doesn't mean that someone will soon make the effort to implement them
in a way that looks good enough to be deemed worth merging into the
current code base.
> the following would be
> its advantages:
> 1. An assumption less, as we explicitly check the assumption.
Checking can be costly. If the probability that the check will fail is
very low, while the cost of checking is high, it is less costly on
average to not check.
> 2. It might be quicker, because we delete parts of graph that cannot
> contain transitions.
I don't agree that it's a good idea to delete what you suggest above.
Or if you think that the goal should not be to find the "first bad
commit" in the above case, then you should explain what the goal
should be.
> 3. It returns more exact results.
Yeah, but checking every commit related to the good and bad commits
would also return more exact results. (This can probably be done using
`git rebase --exec ...` by the way.) One could even print a nice graph
with all the good and bad commits. The problem is that it would not be
efficient. If git bisect makes some assumptions, it is because they
have been deemed reasonable and they have worked well in practice.
It's also because the goal of git bisect is to be efficient, otherwise
there would be no point in using a binary search algorithm in the
first place.
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Alex Hoffman @ 2017-02-19 12:43 UTC (permalink / raw)
To: Christian Couder; +Cc: Johannes Sixt, Stephan Beyer, git
In-Reply-To: <CAMX8fZVeAEJ5tfCO_4Pebnq=rysaJ2xDMjH-9pjmPeF4FziLFw@mail.gmail.com>
Below is a correction of the first proposed algorithm:
>--o1--o2--o3--G --X1
> \ \
> x1--x2--x3--x4--X2--B--
> \ /
> y1--y2--y3
>
Step 1a. (Unchanged) keep only the commits that:
a) are ancestor of the "bad" commit (including the "bad" commit itself),
b) are not ancestor of a "good" commit (excluding the "good" commits).
The following graph results:
x1--x2--x3--x4--X2--B--
\ /
y1--y2--y3
Step 1b. (New) Mark all commits of the resulting graph as unconfirmed
(unconfirmed=node without good ancestors).
Mark as confirmed all descendants of commits that user marked
explicitly as good (e.g if user already marked its parent/grand
parent/... as good right before starting bisect run).
Step 1c. From all unconfirmed root commits build a set SET_GOOD of
those with any good parents in the original graph (root commit =
commit without parents in the resulting graph) and one SET_BAD for
commits with only BAD parents. To build a set means to ask explicitly
the user (or the command passed in git bisect run) whether any of its
parents is good. In the example above find out whether x1 has any good
parents or no parent at all and if so add it to SET_GOOD, otherwise to
SET_BAD.
Mark as confirmed each commit in SET_GOOD and all its descendants.
For every commit in SET_BAD delete all paths from it until to the next
confirmed commit. In the example above if x1 is in SET_BAD delete the
path x1-x3 and x1-y3. If any new root commits result (commit x4), redo
step 1c with them.
Step2. Continue the existing algorithm.
If this improvement works (i.e you do not find any bugs in it and it
is feasible to implement, which seems to me) the following would be
its advantages:
1. An assumption less, as we explicitly check the assumption.
2. It might be quicker, because we delete parts of graph that cannot
contain transitions.
3. It returns more exact results.
VG
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Alex Hoffman @ 2017-02-19 11:32 UTC (permalink / raw)
To: Christian Couder; +Cc: Johannes Sixt, Stephan Beyer, git
In-Reply-To: <CAP8UFD3ngMvVy2XLzYNn9OFbS+zQpWTW=pravpHhA-0PcDVhfg@mail.gmail.com>
> At the end of the git-bisect man page (in the SEE ALSO section) there
> is a link to https://github.com/git/git/blob/master/Documentation/git-bisect-lk2009.txt
> which has a lot of details about how bisect works.
>
Thanks for pointing out the SEE ALSO section. I think it makes sense
to include/describe the entire algorithm in the man page itself,
although I am not sure whether the graphs would be always correctly
visually represented in the man page format.
> The goal is to find the first bad commit, which is a commit that has
> only good parents.
OK, bisect's mission is more exact than I thought, which is good. M
> As o1 is an ancestor of G, then o1 is considered good by the bisect algorithm.
> If it was bad, it would means that there is a transition from bad to
> good between o1 and G.
> But when a good commit is an ancestor of the bad commit, git bisect
> makes the assumption that there is no transition from bad to good in
> the graph.
The assumption that there is no transition from bad to good in the
graph did not hold in my example and it does not hold when a feature
was recently introduced and gets broken relative shortly afterwards.
But I consider it is easy to change the algorithm not to assume, but
rather to check it.
> git bisect makes some assumptions that are true most of the time, so
> in practice it works well most of the time.
Whatever the definition of "most of the time" everyone might have, I
think there is room for improvement. Below I am trying to make a small
change to the current algorithm in order to deal with the assumption
that sometimes does not hold (e.g in my example), by explicitly
validating the check.
> --o1--o2--o3--G--X1
> \ \
> x1--x2--x3--x4--X2--B--
> \ /
> y1--y2--y3
Step 1a. (Unchanged) keep only the commits that:
a) are ancestor of the "bad" commit (including the "bad" commit itself),
b) are not ancestor of a "good" commit (excluding the "good" commits).
The following graph results:
x1--x2--x3--x4--X2--B--
\ /
y1--y2--y3
Step 1b. (New) Mark all root commits of the resulting graph (i.e
commits without parents) as unconfirmed (unconfirmed=node that has
only bad parents). Remove all root commits that user already confirmed
(e.g if user already marked its parent as good right before starting
bisect run). For every unconfirmed root commit check if it has any
good parents. In the example above check whether x1 has good parents.
If the current root element has any parents and none of them is
good, we can delete all paths from it until to the next commit that
has a parent in the ancestors of GOOD. In the example above to delete
the path x1-x3 and x1-y3. Also add new resulting root commits to the
list of unconfirmed commits (commit x4).
Otherwise mark it as confirmed.
Step2. Continue the existing algorithm.
If this improvement works (i.e you do not find any bugs in it and it
is feasible to implement, which seems to me) the following would be
its advantages:
1. An assumption less, as we explicitly check the assumption.
2. It might be quicker, because we delete parts of graph that cannot
contain transitions.
3. It returns more exact results.
VG
^ permalink raw reply
* [PATCH v6 6/6] stash: allow pathspecs in the no verb form
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Now that stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well. Always use -- to
disambiguate pathspecs from other non-option arguments.
Also make git stash -p an alias for git stash push -p. This allows
users to use git stash -p <pathspec>.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 11 +++++++----
git-stash.sh | 3 +++
t/t3903-stash.sh | 15 +++++++++++++++
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 3f7fa88ddc..369bfae33d 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -53,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them. The <message> part is optional and gives
- the description along with the stashed state. For quickly making
- a snapshot, you can omit _both_ "save" and <message>, but giving
- only <message> does not trigger this action to prevent a misspelled
- subcommand from making an unwanted stash.
+ the description along with the stashed state.
++
+For quickly making a snapshot, you can omit "push". In this mode,
+non-option arguments are not allowed to prevent a misspelled
+subcommand from making an unwanted stash. The two exceptions to this
+are `stash -p` which acts as alias for `stash push -p` and pathspecs,
+which are allowed after a double hyphen `--` for disambiguation.
+
When pathspec is given to 'git stash push', the new stash records the
modified states only for the files that match the pathspec. The index
diff --git a/git-stash.sh b/git-stash.sh
index 1e55cd5fdd..18aba1346f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -666,12 +666,15 @@ apply_to_branch () {
}
}
+test "$1" = "-p" && set "push" "$@"
+
PARSE_CACHE='--not-parsed'
# The default command is "push" if nothing but options are given
seen_non_option=
for opt
do
case "$opt" in
+ --) break ;;
-*) ;;
*) seen_non_option=t; break ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7f90a247b4..c0ae41e724 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -872,4 +872,19 @@ test_expect_success 'untracked files are left in place when -u is not given' '
test_path_is_file untracked
'
+test_expect_success 'stash without verb with pathspec' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone. Unfortunately
git currently offers no such option. git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.
Allow 'git stash push' to take pathspec to specify which paths to stash.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 9 ++++-
git-stash.sh | 48 +++++++++++++++++++------
t/t3903-stash.sh | 72 ++++++++++++++++++++++++++++++++++++++
t/t3905-stash-include-untracked.sh | 26 ++++++++++++++
4 files changed, 143 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 0f602ea0c8..b7db939a06 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
[-u|--include-untracked] [-a|--all] [<message>]]
'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m|--message <message>]]
+ [--] [<pathspec>...]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -48,7 +49,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
-push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them. The <message> part is optional and gives
@@ -57,6 +58,12 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
only <message> does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
+
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec. The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
If the `--keep-index` option is used, all changes already added to the
index are left intact.
+
diff --git a/git-stash.sh b/git-stash.sh
index ef5d1b45be..b55983d1fd 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list [<options>]
[-u|--include-untracked] [-a|--all] [<message>]]
or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m <message>]
+ [-- <pathspec>...]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -35,15 +36,15 @@ else
fi
no_changes () {
- git diff-index --quiet --cached HEAD --ignore-submodules -- &&
- git diff-files --quiet --ignore-submodules &&
+ git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+ git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
}
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt
+ git ls-files -o -z $excl_opt -- "$@"
}
clear_stash () {
@@ -71,12 +72,16 @@ create_stash () {
shift
untracked=${1?"BUG: create_stash () -u requires an argument"}
;;
+ --)
+ shift
+ break
+ ;;
esac
shift
done
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
exit 0
fi
@@ -108,7 +113,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files | (
+ untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -131,7 +136,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -145,7 +150,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- &&
+ git add--interactive --patch=stash -- "$@" &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -274,7 +279,7 @@ push_stash () {
fi
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
@@ -282,18 +287,39 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash -m "$stash_msg" -u "$untracked"
+ create_stash -m "$stash_msg" -u "$untracked" -- "$@"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
if test -z "$patch_mode"
then
- git reset --hard ${GIT_QUIET:+-q}
+ if test $# != 0
+ then
+ saved_untracked=
+ if test -n "$(git ls-files --others -- "$@")"
+ then
+ saved_untracked=$(
+ git ls-files -z --others -- "$@" |
+ xargs -0 git stash create -u all --)
+ fi
+ git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
+ git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
+ if test -n "$(git ls-files -z --others -- "$@")"
+ then
+ git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
+ fi
+ if test -n "$saved_untracked"
+ then
+ git stash pop -q $saved_untracked
+ fi
+ else
+ git reset --hard ${GIT_QUIET:+-q}
+ fi
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION
+ git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
fi
if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ffe3549ea5..4fb800eec8 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,76 @@ test_expect_success 'create with multiple arguments for the message' '
test_cmp expect actual
'
+test_expect_success 'stash -- <filename> stashes and restores the file' '
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ git stash push -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+ >foo &&
+ >bar &&
+ >extra &&
+ git add foo bar extra &&
+ git stash push -- foo bar &&
+ test_path_is_missing bar &&
+ test_path_is_missing foo &&
+ test_path_is_file extra &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash push -p with pathspec shows no changes only onece' '
+ >file &&
+ git add file &&
+ git stash push -p not-file >actual &&
+ echo "No local changes to save" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec shows no changes when there are none' '
+ >file &&
+ git add file &&
+ git stash push not-file >actual &&
+ echo "No local changes to save" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'untracked file is not removed when using pathspecs' '
+ >untracked &&
+ git stash push untracked &&
+ test_path_is_file untracked
+'
+
+test_expect_success 'untracked files are left in place when -u is not given' '
+ >file &&
+ git add file &&
+ >untracked &&
+ git stash push file &&
+ test_path_is_file untracked
+'
+
test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..193adc7b68 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
test -s .gitignore
'
+test_expect_success 'stash push --include-untracked with pathspec' '
+ >foo &&
+ >bar &&
+ git stash push --include-untracked -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file bar &&
+ test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push --include-untracked -- "foo b*" &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 1/6] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Introduce a new git stash push verb in addition to git stash save. The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.
This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 3 +++
git-stash.sh | 46 ++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++++
3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..0f602ea0c8 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,6 +15,8 @@ SYNOPSIS
'git stash' branch <branchname> [<stash>]
'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -46,6 +48,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>]::
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them. The <message> part is optional and gives
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list [<options>]
or: $dashless branch <branchname> [<stash>]
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
+ or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m <message>]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
return $ret
}
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+ stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
-a|--all)
untracked=all
;;
+ -m|--message)
+ shift
+ test -z ${1+x} && usage
+ stash_msg=$1
+ ;;
--help)
show_help
;;
@@ -251,8 +259,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
- stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +297,36 @@ save_stash () {
fi
}
+save_stash () {
+ push_options=
+ while test $# != 0
+ do
+ case "$1" in
+ --)
+ shift
+ break
+ ;;
+ -*)
+ # pass all options through to push_stash
+ push_options="$push_options $1"
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+
+ stash_msg="$*"
+
+ if test -z "$stash_msg"
+ then
+ push_stash $push_options
+ else
+ push_stash $push_options -m "$stash_msg"
+ fi
+}
+
have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
}
@@ -617,6 +653,10 @@ save)
shift
save_stash "$@"
;;
+push)
+ shift
+ push_stash "$@"
+ ;;
apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' '
test_path_is_missing file
'
+test_expect_success 'push -m shows right message' '
+ >foo &&
+ git add foo &&
+ git stash push -m "test message" &&
+ echo "stash@{0}: On master: test message" >expect &&
+ git stash list -1 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 3/6] stash: refactor stash_create
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Refactor the internal stash_create function to use a -m flag for
specifying the message and -u flag to indicate whether untracked files
should be added to the stash.
This makes it easier to pass a pathspec argument to stash_create in the
next patch.
The user interface for git stash create stays the same.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
git-stash.sh | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..ef5d1b45be 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,22 @@ clear_stash () {
}
create_stash () {
- stash_msg="$1"
- untracked="$2"
+ stash_msg=
+ untracked=
+ while test $# != 0
+ do
+ case "$1" in
+ -m|--message)
+ shift
+ stash_msg=${1?"BUG: create_stash () -m requires an argument"}
+ ;;
+ -u|--include-untracked)
+ shift
+ untracked=${1?"BUG: create_stash () -u requires an argument"}
+ ;;
+ esac
+ shift
+ done
git update-index -q --refresh
if no_changes
@@ -268,7 +282,7 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash "$stash_msg" $untracked
+ create_stash -m "$stash_msg" -u "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
@@ -667,7 +681,7 @@ clear)
;;
create)
shift
- create_stash "$*" && echo "$w_commit"
+ create_stash -m "$*" && echo "$w_commit"
;;
store)
shift
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 2/6] stash: add test for the create command line arguments
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Currently there is no test showing the expected behaviour of git stash
create's command line arguments. Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
t/t3903-stash.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3577115807..ffe3549ea5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
'
+test_expect_success 'create stores correct message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create "create test message") &&
+ echo "On master: create test message" >expect &&
+ git show --pretty=%s -s ${STASH_ID} >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create test untracked) &&
+ echo "On master: test untracked" >expect &&
+ git show --pretty=%s -s ${STASH_ID} >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 0/6] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170217224141.19183-1-t.gummerer@gmail.com>
Thanks Junio and Peff for comments on the last round.
Changes since then:
- removed mention of the "new form" of git stash create from the
Documentation.
- Changed documentation for git stash without a verb, mentioning
stash -p now being an alias for git stash push -p and that -- can be
used as disambiguation for for pathspecs
- Fixed ${1-...} which should have been ${1?...}
- Removed unused new_style variable from create_stash, which was a
leftover from perious rounds.
Interdiff below:
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 97194576ef..369bfae33d 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,8 +20,6 @@ SYNOPSIS
[--] [<pathspec>...]]
'git stash' clear
'git stash' create [<message>]
-'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
- [-- <pathspec>...]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
DESCRIPTION
@@ -55,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them. The <message> part is optional and gives
- the description along with the stashed state. For quickly making
- a snapshot, you can omit _both_ "save" and <message>, but giving
- only <message> does not trigger this action to prevent a misspelled
- subcommand from making an unwanted stash.
+ the description along with the stashed state.
++
+For quickly making a snapshot, you can omit "push". In this mode,
+non-option arguments are not allowed to prevent a misspelled
+subcommand from making an unwanted stash. The two exceptions to this
+are `stash -p` which acts as alias for `stash push -p` and pathspecs,
+which are allowed after a double hyphen `--` for disambiguation.
+
When pathspec is given to 'git stash push', the new stash records the
modified states only for the files that match the pathspec. The index
diff --git a/git-stash.sh b/git-stash.sh
index 1446fbe2e8..18aba1346f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -61,17 +61,16 @@ clear_stash () {
create_stash () {
stash_msg=
untracked=
- new_style=
while test $# != 0
do
case "$1" in
-m|--message)
shift
- stash_msg=${1-"BUG: create_stash () -m requires an argument"}
+ stash_msg=${1?"BUG: create_stash () -m requires an argument"}
;;
-u|--include-untracked)
shift
- untracked=${1-"BUG: create_stash () -u requires an argument"}
+ untracked=${1?"BUG: create_stash () -u requires an argument"}
;;
--)
shift
Thomas Gummerer (6):
stash: introduce push verb
stash: add test for the create command line arguments
stash: refactor stash_create
stash: teach 'push' (and 'create_stash') to honor pathspec
stash: use stash_push for no verb form
stash: allow pathspecs in the no verb form
Documentation/git-stash.txt | 27 ++++++--
git-stash.sh | 127 ++++++++++++++++++++++++++++++-------
t/t3903-stash.sh | 118 +++++++++++++++++++++++++++++++++-
t/t3905-stash-include-untracked.sh | 26 ++++++++
4 files changed, 267 insertions(+), 31 deletions(-)
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* [PATCH v6 5/6] stash: use stash_push for no verb form
From: Thomas Gummerer @ 2017-02-19 11:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski, Matthieu Moy,
Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.
Previously we allowed git stash -- -message, which is no longer allowed
after this patch. Messages starting with a hyphen was allowed since
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options"). However it was never the intent to allow that, but rather it
was allowed accidentally.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 8 ++++----
git-stash.sh | 16 ++++++++--------
t/t3903-stash.sh | 4 +---
3 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index b7db939a06..3f7fa88ddc 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
'git stash' drop [-q|--quiet] [<stash>]
'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
'git stash' branch <branchname> [<stash>]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [<message>]]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [<message>]
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m|--message <message>]]
- [--] [<pathspec>...]
+ [--] [<pathspec>...]]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
diff --git a/git-stash.sh b/git-stash.sh
index b55983d1fd..1e55cd5fdd 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list [<options>]
or: $dashless drop [-q|--quiet] [<stash>]
or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
or: $dashless branch <branchname> [<stash>]
- or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [<message>]]
- or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [-m <message>]
- [-- <pathspec>...]
+ or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [<message>]
+ or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m <message>]
+ [-- <pathspec>...]]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -667,7 +667,7 @@ apply_to_branch () {
}
PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
seen_non_option=
for opt
do
@@ -677,7 +677,7 @@ do
esac
done
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
# Main command set
case "$1" in
@@ -728,7 +728,7 @@ branch)
*)
case $# in
0)
- save_stash &&
+ push_stash &&
say "$(gettext "(To restore them type \"git stash apply\")")"
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4fb800eec8..7f90a247b4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
git add file2 &&
test_must_fail git stash --invalid-option &&
test_must_fail git stash save --invalid-option &&
- test bar5,bar6 = $(cat file),$(cat file2) &&
- git stash -- -message-starting-with-dash &&
- test bar,bar2 = $(cat file),$(cat file2)
+ test bar5,bar6 = $(cat file),$(cat file2)
'
test_expect_success 'stash an added file' '
--
2.12.0.rc2.399.g0ca89a282
^ permalink raw reply related
* Re: [PATCH v5 6/6] stash: allow pathspecs in the no verb form
From: Thomas Gummerer @ 2017-02-19 9:18 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170217234647.bqhzzm533oruhr5e@sigill.intra.peff.net>
On 02/17, Jeff King wrote:
> On Fri, Feb 17, 2017 at 10:41:41PM +0000, Thomas Gummerer wrote:
>
> > Now that stash_push is used in the no verb form of stash, allow
> > specifying the command line for this form as well. Always use -- to
> > disambiguate pathspecs from other non-option arguments.
>
> I think that makes sense.
>
> > Also make git stash -p an alias for git stash push -p. This allows
> > users to use git stash -p <pathspec>.
>
> And I think of all the options we discussed for handling "-p", I think
> this one makes the most sense.
>
> It may be worth calling out in the documentation that this is how it
> works though, so people do not think that:
>
> git stash -k -p <path>
>
> would work ("git stash -k -p" _does_ happen to work due to the old
> options-only rule, but I think we should advertise the general form as
> "-p is an alias for "push -p").
Yeah, I think adding something about this in the documentation would
be good. I'll add a paragraph about this in the re-roll.
> -Peff
^ permalink raw reply
* Re: [PATCH v5 3/6] stash: refactor stash_create
From: Thomas Gummerer @ 2017-02-19 9:17 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170217234805.glvhyfbxab6nwgb7@sigill.intra.peff.net>
On 02/17, Jeff King wrote:
> On Fri, Feb 17, 2017 at 10:41:38PM +0000, Thomas Gummerer wrote:
>
> > Refactor the internal stash_create function to use a -m flag for
> > specifying the message and -u flag to indicate whether untracked files
> > should be added to the stash.
> >
> > This makes it easier to pass a pathspec argument to stash_create in the
> > next patch.
> >
> > The user interface for git stash create stays the same.
>
> Sounds good, but...
>
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index 2e9cef06e6..d93c47446a 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -17,6 +17,7 @@ SYNOPSIS
> > [-u|--include-untracked] [-a|--all] [<message>]]
> > 'git stash' clear
> > 'git stash' create [<message>]
> > +'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
> > 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
>
> Should this hunk be dropped from the manpage, then?
>
> I think there is a similar one in the next patch that adds the
> "pathspec" argument, and should be dropped, too.
Argh yes I should have been more careful. Thanks for catching.
> -Peff
^ permalink raw reply
* Re: difflame improvements
From: Edmundo Carmona Antoranz @ 2017-02-19 6:35 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <CAOc6etZxkspqafifjPTbRDoVBt0XuOLbhiuY9bFTD2Wjtxw-HQ@mail.gmail.com>
On Fri, Feb 17, 2017 at 1:01 AM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 11:17 PM, Jeff King <peff@peff.net> wrote:
>
>> This isn't difflame's fault; that's what "git blame" tells you about
>> that line. But since I already told difflame "v2.6.5..HEAD", it would
>> probably make sense to similarly limit the blame to that range. That
>> turns up a boundary commit for the line. Which is _also_ not helpful,
>> but at least the tool is telling me that the line came from before
>> v2.6.5, and I don't really need to care much about it.
>
>
> I'm running my own tests on difflame and I have a theory about "when"
> it breaks.... at least one of the cases when it breaks:
>
> Analysis for deleted lines is being driven by git blame --reverse.
> What I have noticed is that it "breaks" when blame --reverse drives
> the analysis into revisions where "treeish1" is not part of their
> history (like, bringing analysis "to the sides" of treeish1 instead of
> keeping analysis in revisions in the history of treeish2 that have
> treeish1 as one of their ancestors.... which is definitely a valid
> case for analysis, anyway). In this case, blame --reverse stops being
> helpful.
>
At the cost of being slower, I just pushed to master the best results yet.
The workaround I developed for the case I described on the previous
mail ended up providing much better results overall so I ended up
replacing the whole merge-analysis logic with it.
Thanks for your kind help and comments, Peff. Let me know how it goes.
^ permalink raw reply
* Re: [PATCH] git-check-ref-format: fix typo in man page
From: Jeff King @ 2017-02-19 2:27 UTC (permalink / raw)
To: Philip Oakley; +Cc: Michael Haggerty, Damien Regad, git
In-Reply-To: <2116CBFFB78A482D8FA176BC680B3B9C@PhilipOakley>
On Sun, Feb 19, 2017 at 12:20:33AM -0000, Philip Oakley wrote:
> > Normalize 'refname' by removing any leading slash (`/`)
> > characters and collapsing runs of adjacent slashes between
> > - name components into a single slash. Iff the normalized
> > + name components into a single slash. If the normalized
> > refname is valid then print it to standard output and exit
> > with a status of 0. (`--print` is a deprecated way to spell
> > `--normalize`.)
> > --
>
> Could that be an 'iff' == 'If and only if' (which is common in mathematics)?
> Still could be spelling error though.
When we're not sure what the intent of a change is, a good first step is
to dig up the original commit via `git blame` or similar. In this case,
it comes from a40e6fb67 (Change check_refname_format() to reject
unnormalized refnames, 2011-09-15).
The commit message doesn't mention it (not that I really expected it
to), but it does tell you who the author is. And a good second step is
to cc them on the patch. :)
I suspect it _was_ intended as "iff" here. In my opinion, we probably
don't need to be so rigorous in this instance. However, I note that we
do not describe the "else" half of that "if". So maybe an overall
improvement would be something like:
If the normalized refname is valid then print it to standard output
and exit with a status of 0. Otherwise, exit with a non-zero status.
-Peff
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Junio C Hamano @ 2017-02-19 2:06 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Johannes Schindelin, Pranit Bauva
In-Reply-To: <xmqqinodewdr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> ...
> So, let's give Pranit a concrete "here is what we want to see
> squashed in", while you guys discuss peculiarity with various
> platforms and their system headers, which admittedly is a more
> interesting tangent ;-)
>
> There are early returns with "goto finish" even before _syn
> variables are first assigned to, so they would need to be
> initialized to NULL. The other two get their initial values
> right at the beginning, so they are OK.
>
> builtin/bisect--helper.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
While we are waiting for the topic to be fixed, I've tentatively
applied this on top to update 'pu', so Travis should now be happy
with 'pu' on Mac, too.
^ permalink raw reply
* [PATCH] fetch: print an error when declining to request an unadvertised object
From: Matt McCutchen @ 2017-02-19 1:55 UTC (permalink / raw)
To: git
In-Reply-To: <xmqq60kfezr9.fsf@gitster.mtv.corp.google.com>
Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1. The more common case
of requesting a nonexistent ref normally triggers a die() in
get_fetch_map, so "git fetch" wasn't bothering to check after the fetch
that it got all the refs it sought, like "git fetch-pack" does near the
end of cmd_fetch_pack.
Move the code from cmd_fetch_pack to a new function,
report_unmatched_refs, that is called by fetch_refs_via_pack as part of
"git fetch". Also, change filter_refs (which checks whether a request
for an unadvertised object should be sent to the server) to set a new
match status on the "struct ref" when the request is not allowed, and
have report_unmatched_refs check for this status and print a special
error message, "Server does not allow request for unadvertised object".
Finally, add a simple test case for "git fetch REMOTE SHA1".
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
builtin/fetch-pack.c | 7 +------
fetch-pack.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
fetch-pack.h | 9 +++++++++
remote.h | 9 +++++++--
t/t5516-fetch-push.sh | 3 ++-
transport.c | 14 +++++++++-----
6 files changed, 66 insertions(+), 27 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e44..2a1c1c2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -219,12 +219,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
* remote no-such-ref' would silently succeed without issuing
* an error.
*/
- for (i = 0; i < nr_sought; i++) {
- if (!sought[i] || sought[i]->matched)
- continue;
- error("no such remote ref %s", sought[i]->name);
- ret = 1;
- }
+ ret |= report_unmatched_refs(sought, nr_sought);
while (ref) {
printf("%s %s\n",
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..f12bfcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -578,7 +578,7 @@ static void filter_refs(struct fetch_pack_args *args,
break; /* definitely do not have it */
else if (cmp == 0) {
keep = 1; /* definitely have it */
- sought[i]->matched = 1;
+ sought[i]->match_status = REF_MATCHED;
}
i++;
}
@@ -598,22 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
/* Append unmatched requests to the list */
- if ((allow_unadvertised_object_request &
- (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
- for (i = 0; i < nr_sought; i++) {
- unsigned char sha1[20];
+ for (i = 0; i < nr_sought; i++) {
+ unsigned char sha1[20];
- ref = sought[i];
- if (ref->matched)
- continue;
- if (get_sha1_hex(ref->name, sha1) ||
- ref->name[40] != '\0' ||
- hashcmp(sha1, ref->old_oid.hash))
- continue;
+ ref = sought[i];
+ if (ref->match_status != REF_NOT_MATCHED)
+ continue;
+ if (get_sha1_hex(ref->name, sha1) ||
+ ref->name[40] != '\0' ||
+ hashcmp(sha1, ref->old_oid.hash))
+ continue;
- ref->matched = 1;
+ if ((allow_unadvertised_object_request &
+ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+ ref->match_status = REF_MATCHED;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
+ } else {
+ ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
}
}
*refs = newlist;
@@ -1094,3 +1096,26 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
clear_shallow_info(&si);
return ref_cpy;
}
+
+int report_unmatched_refs(struct ref **sought, int nr_sought)
+{
+ int i, ret = 0;
+
+ for (i = 0; i < nr_sought; i++) {
+ if (!sought[i])
+ continue;
+ switch (sought[i]->match_status) {
+ case REF_MATCHED:
+ continue;
+ case REF_NOT_MATCHED:
+ error(_("no such remote ref %s"), sought[i]->name);
+ break;
+ case REF_UNADVERTISED_NOT_ALLOWED:
+ error(_("Server does not allow request for unadvertised object %s"),
+ sought[i]->name);
+ break;
+ }
+ ret = 1;
+ }
+ return ret;
+}
diff --git a/fetch-pack.h b/fetch-pack.h
index c912e3d..fd4d80e 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
struct sha1_array *shallow,
char **pack_lockfile);
+/*
+ * Print an appropriate error message for each sought ref that wasn't
+ * matched. Return 0 if all sought refs were matched, otherwise 1.
+ *
+ * The type of "sought" should be "const struct ref *const *" but for
+ * http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c .
+ */
+int report_unmatched_refs(struct ref **sought, int nr_sought);
+
#endif
diff --git a/remote.h b/remote.h
index 9248811..0b9d8c4 100644
--- a/remote.h
+++ b/remote.h
@@ -89,8 +89,13 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
- deletion:1,
- matched:1;
+ deletion:1;
+
+ enum {
+ REF_NOT_MATCHED = 0, /* initial value */
+ REF_MATCHED,
+ REF_UNADVERTISED_NOT_ALLOWED
+ } match_status;
/*
* Order is important here, as we write to FETCH_HEAD
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2caf..78f3b8e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
# fetching the hidden object should fail by default
- test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy &&
+ test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+ test_i18ngrep "Server does not allow request for unadvertised object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
# the server side can allow it to succeed
diff --git a/transport.c b/transport.c
index 04e5d66..c377907 100644
--- a/transport.c
+++ b/transport.c
@@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
static int fetch_refs_via_pack(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
+ int ret = 0;
struct git_transport_data *data = transport->data;
struct ref *refs;
char *dest = xstrdup(transport->url);
@@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport *transport,
&transport->pack_lockfile);
close(data->fd[0]);
close(data->fd[1]);
- if (finish_connect(data->conn)) {
- free_refs(refs);
- refs = NULL;
- }
+ if (finish_connect(data->conn))
+ ret = -1;
data->conn = NULL;
data->got_remote_heads = 0;
data->options.self_contained_and_connected =
args.self_contained_and_connected;
+ if (refs == NULL)
+ ret = -1;
+ if (report_unmatched_refs(to_fetch, nr_heads))
+ ret = -1;
+
free_refs(refs_tmp);
free_refs(refs);
free(dest);
- return (refs ? 0 : -1);
+ return ret;
}
static int push_had_errors(struct ref *ref)
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Matt McCutchen @ 2017-02-19 2:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq60kfezr9.fsf@gitster.mtv.corp.google.com>
On Sun, 2017-02-12 at 15:49 -0800, Junio C Hamano wrote:
> The fact that we have the above two choices tells me that a two-step
> approach may be an appropriate approach. [...]
> Even if you did only the first step, as long as the second step can
> be done without reverting what the first step did [*4*] by somebody
> who cares the "specific error" deeply enough, I am OK with that. Of
> course if you did both steps, that is fine by me as well ;-)
I appreciate the flexibility, but now that I've spent the time to
understand all the code involved, it would be a pity not to go for the
complete solution. New patch coming.
Matt
^ permalink raw reply
* Re: [PATCH] git-check-ref-format: fix typo in man page
From: Philip Oakley @ 2017-02-19 0:20 UTC (permalink / raw)
To: Damien Regad, git
In-Reply-To: <c27d7861-b161-a3eb-fcfc-bf766fc7b20b@gmail.com>
From: "Damien Regad" <dregad@mantisbt.org>
> ---
> Documentation/git-check-ref-format.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-check-ref-format.txt
> b/Documentation/git-check-ref-format.txt
> index 8611a99..377c85a 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -100,7 +100,7 @@ OPTIONS
> --normalize::
> Normalize 'refname' by removing any leading slash (`/`)
> characters and collapsing runs of adjacent slashes between
> - name components into a single slash. Iff the normalized
> + name components into a single slash. If the normalized
> refname is valid then print it to standard output and exit
> with a status of 0. (`--print` is a deprecated way to spell
> `--normalize`.)
> --
Could that be an 'iff' == 'If and only if' (which is common in mathematics)?
Still could be spelling error though.
--
Philip
^ permalink raw reply
* RE: [PATCH 3/5] name-hash: precompute hash values during preload-index
From: Jeff Hostetler @ 2017-02-19 0:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Johannes Schindelin, Jeff Hostetler
In-Reply-To: <xmqq37fcnj7v.fsf@gitster.mtv.corp.google.com>
From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C Hamano
>
> The fact that each preload_thread() still walks the index in-order
> makes me wonder if it may allow us to further optimize the "dir"
> part of the hash by passing the previous ce for which we already
> precomputed hash values. While the loop is iterating over the paths
> in the same directory, .dir component from the previous ce can be
> reused and .name component can "continue", no?
>
> It's possible that you already tried such an optimization and
> rejected it after finding that the cost of comparison of pathnames
> to tell if ce and previous ce are still in the same directory is
> more than unconditionally memihash() the directory part, and I am in
> no way saying that I found a missed optimization opportunity you
> must pursue. I am just being curious.
I looked at doing this, but I didn't think the complexity and overhead to
forward search for peers at the current level didn't warrant the limited gains.
(I was just looking at the complexity of clear_ce_flags_1() in unpack-trees.c
and how hard it has to look to find the end of the current directory and the
effect that that has on the recursion and it felt like too much work for the
potential gain.)
Whereas remembering the previous one was basically free. Granted, it only
helps us for adjacent files in the index, so it's not perfect, but gives us the
best bang for the buck.
Jeff
^ permalink raw reply
* Re: [PATCH] git-check-ref-format: fix typo in man page
From: Jacob Keller @ 2017-02-19 0:17 UTC (permalink / raw)
To: Damien Regad; +Cc: Git mailing list
In-Reply-To: <c27d7861-b161-a3eb-fcfc-bf766fc7b20b@gmail.com>
On Sat, Feb 18, 2017 at 2:47 PM, Damien Regad <dregad@mantisbt.org> wrote:
>
> ---
> Documentation/git-check-ref-format.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-check-ref-format.txt
> b/Documentation/git-check-ref-format.txt
> index 8611a99..377c85a 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -100,7 +100,7 @@ OPTIONS
> --normalize::
> Normalize 'refname' by removing any leading slash (`/`)
> characters and collapsing runs of adjacent slashes between
> - name components into a single slash. Iff the normalized
> + name components into a single slash. If the normalized
I think this is a good change, but I do know in some contexts, "Iff"
is used intentionally to mean "If and only if". It's somewhat unlikely
that's what was going on here, and I don't think we need to be that
pedantic in our help documentation anyway.
Thanks,
Jake
> refname is valid then print it to standard output and exit
> with a status of 0. (`--print` is a deprecated way to spell
> `--normalize`.)
> --
> 2.7.4
>
>
^ permalink raw reply
* RE: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
From: Jeff Hostetler @ 2017-02-19 0:02 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Jeff Hostetler, Johannes Schindelin, git@vger.kernel.org,
Jeff Hostetler
In-Reply-To: <xmqqvas8m499.fsf@gitster.mtv.corp.google.com>
From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C Hamano
> Jeff King <peff@peff.net> writes:
>> On Wed, Feb 15, 2017 at 09:27:53AM -0500, Jeff Hostetler wrote:
>>
>>> I have some informal numbers in a spreadsheet. I was seeing
>>> a 8-9% speed up on a status on my gigantic repo.
>>>
>>> I'll try to put together a before/after perf-test to better
>>> demonstrate this.
>>
>> Thanks. What I'm mostly curious about is how much each individual step
>> buys. Sometimes when doing a long optimization series, I find that some
>> of the optimizations make other ones somewhat redundant (e.g., if patch
>> 2 causes us to call the optimized code from patch 3 less often).
>
> I am curious too.
>
> To me 1/5 (reduction of redundant calls), 4/5 (correctly size the
> hash that would grow to a known size anyway) and 5/5 (take advantage
> of the fact that adjacent cache entries are often in the same
> directory) look like no brainers to take, regardless of the others
> (including themselves).
agreed.
> It is not clear to me if 3/5 (preload-index uses available cores to
> compute hashes) is an unconditional win (an operation that is
> pathspec limited may need hashes for only a small fraction of the
> index---would it still be a win to compute the hash for all entries
> upon loading the index, even if we are using otherwise-idel cores?).
I'm not sure about pathspec cases. What I was seeing was that during
the call to lazy_name_init_hash() was taking 30% of the time in
"git status" and 40% in "git add <one_file>". (Again this was on my
giant repo with a 450MB index).
> Of course 2/5 is a prerequisite step for 3/5 and 5/5, so if we want
> either of the latter two, we cannot avoid it.
jeff
^ permalink raw reply
* RE: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area
From: Jeff Hostetler @ 2017-02-18 23:52 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Jeff Hostetler, Johannes Schindelin, git@vger.kernel.org,
Jeff Hostetler
In-Reply-To: <xmqqk28nmdi4.fsf@gitster.mtv.corp.google.com>
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 17, 2017 at 09:58:21PM -0800, Junio C Hamano wrote:
>>
>>> Jeff Hostetler <git@jeffhostetler.com> writes:
>>>
>>> > I'll try to put together a before/after perf-test to better
>>> > demonstrate this.
>>>
>>> I didn't pick up the series while watching these exchanges, as I
>>> didn't know how quick your turnaround would be, but now a few days
>>> have passed. Just to make sure we won't forget this topic, I'll
>>> pick up these 5 patches in the meantime.
>>
>> Yeah, to be clear my question was not an objection, but mostly
>> curiosity and interest.
>
> Yes, it was very clear that it wasn't an objection.
>
> But the other Jeff sounded like a follow-up was to follow shortly if
> not imminent so I decided to allocate my time on other topics still
> only on the list first while waiting to see what happens.
Sorry, I was out of the office for a family emergency on Thursday
and Friday. Add to that the long weekend, and I won't get back around
to this until Tuesday or Wednesday at the earliest.
Jeff
^ permalink raw reply
* [PATCH] git-check-ref-format: fix typo in man page
From: Damien Regad @ 2017-02-18 22:47 UTC (permalink / raw)
To: git
---
Documentation/git-check-ref-format.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-check-ref-format.txt
b/Documentation/git-check-ref-format.txt
index 8611a99..377c85a 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -100,7 +100,7 @@ OPTIONS
--normalize::
Normalize 'refname' by removing any leading slash (`/`)
characters and collapsing runs of adjacent slashes between
- name components into a single slash. Iff the normalized
+ name components into a single slash. If the normalized
refname is valid then print it to standard output and exit
with a status of 0. (`--print` is a deprecated way to spell
`--normalize`.)
--
2.7.4
^ permalink raw reply related
* Re: Git bisect does not find commit introducing the bug
From: Johannes Sixt @ 2017-02-18 22:37 UTC (permalink / raw)
To: Alex Hoffman; +Cc: Stephan Beyer, git
In-Reply-To: <CAMX8fZW2y+iPRfSbXVcHufbM+CsqgekS_0WnCEJ++=njy_TvKg@mail.gmail.com>
Am 18.02.2017 um 19:36 schrieb Alex Hoffman:
> Let's consider your example with distinct names for nodes:
>
> --o1--o2--o3--G--X1
> \ \
> x1--x2--x3--x4--X2--B--
>
> It makes sense that git bisect is expecting a single transition, as
> this is a precondition for a binary search to work. My definition of
> "the transition" is a commit with at least one of its parents as a
> good version, but the commit itself a bad version.
But that is not the definition of transition that lets you pin-point the
breaking commit precisly. Assume x3 is the commit introducing the
breakage in the graph above. Even though you only know initially that G
is good and B is bad, would you not prefer to find x3 instead of X2? As
Christian said, a transition is when a commit is bad and all its parents
are good, and this definition lets you find x3.
> I hope we agree
> that git bisect's mission is to search for this transition (as I
> suppose that most of people need such a functionality from git, if not
> exactly from git bisect). How could be x1 or x3 be the transition, if
> chances are that o1 is not a good version?
By declaring G as good, it is implied that o1 is also good. If it is in
fact bad, the assumptions of git bisect are violated (because there
would be a transition from bad to good at o2, o3, or G), and anything
can happen.
> If you consider that git bisect's mission is different from finding
> the transition, could you please explain what exact commit git bisect
> is supposed to return (ideally with terms from the graph theory) and
> when it makes sense to return that? Because I do not see any sense in
> looking in the path x1-x3 without knowing that those commits may be a
> transition.
And I do not see a reason why git bisect should not look at those
commits. If x3 is the commit that broke my program, I do prefer to be
pointed at x3 rather than X2.
-- Hannes
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Hilco Wijbenga @ 2017-02-18 22:36 UTC (permalink / raw)
To: Alex Hoffman; +Cc: Johannes Sixt, Stephan Beyer, Git Users
In-Reply-To: <CAMX8fZW2y+iPRfSbXVcHufbM+CsqgekS_0WnCEJ++=njy_TvKg@mail.gmail.com>
On 18 February 2017 at 10:36, Alex Hoffman <spec@gal.ro> wrote:
> You definitely convinced me that git MUST search more than only in the
> paths between good and bad commits, as the good commit G does not have
> to be the first good commit (thank you for that). My problem/confusion
> is that it returns something that does not make sense to me, because
> it does not make sure it returns a transition.
If multiple transitions from GOOD to BAD happen, then I don't see how
binary search is useful/possible. The same is true for a simple list
of numbers, say, 1 5 6 2 3 4. You can't use binary search here because
you can't "throw away" all numbers to the left (or right) of your
pivot. Or am I missing your point?
^ 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