* Re: [PATCH] contrib: remove gitview
From: Junio C Hamano @ 2017-01-01 0:55 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git, jvoss, Aneesh Kumar K.V
In-Reply-To: <20161229015918.jyiqd42z4htjibul@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Dec 28, 2016 at 09:28:37AM -0800, Stefan Beller wrote:
> ...
>> 2 files changed, 1362 deletions(-)
>> delete mode 100755 contrib/gitview/gitview
>> delete mode 100644 contrib/gitview/gitview.txt
>
> Thanks for assembling the patch. This seems reasonable to me, though I'd
> like to get an Ack from Aneesh if we can.
Likewise.
^ permalink raw reply
* Counter-intuitive result from diff -C --stat
From: Mike Hommey @ 2016-12-28 8:09 UTC (permalink / raw)
To: git
Hi,
So I was checking out differences between two branches, accounting for
file moves with -C, and was surprised by the number of insertions and
deletions that it indicated, because it was telling me I had removed
more than I added, which I really don't think is true.
I took a closer look, and what happens is that I had a lot of stuff in
a __init__.py file that I moved to another file, while keeping a now
new, empty, __init__.py file.
Which means while diff counts the deletions from __init__.py, it doesn't
count the additions from the move because it is a move, leading to a
counter-intuitive result.
Here's how to reproduce in case code makes more sense than prose:
/tmp$ git init g
Initialized empty Git repository in /tmp/g/.git/
/tmp$ cd g
/tmp/g$ echo foo > foo
/tmp/g$ git add foo
/tmp/g$ git commit -m foo
[master (root-commit) 14749a7] foo
1 file changed, 1 insertion(+)
create mode 100644 foo
/tmp/g$ git mv foo bar
/tmp/g$ touch foo
/tmp/g$ git add foo
/tmp/g$ git commit -m bar
[master 9fbf50e] bar
2 files changed, 1 insertion(+), 1 deletion(-)
create mode 100644 bar
/tmp/g$ git diff HEAD~ -C --stat
foo => bar | 0
foo | 1 -
2 files changed, 1 deletion(-)
I'm actually not sure what the right thing would be. I guess this is a
case where -B should help, but it doesn't.
Any thoughts?
Mike
^ permalink raw reply
* Rebasing multiple branches at once
From: Mike Hommey @ 2016-12-31 8:14 UTC (permalink / raw)
To: git
Hi,
I've had this kind of things to do more than once, and had to do it a
lot today, so I figured it would be worth discussing whether git-rebase
should be enhanced to support this, or if this should go in a separate
tool or whatever.
So here is what I'm trying to do in a not-too painful way:
I'm starting with something like this:
A---B---C---D---E
\---F
where A is master, and E and F are two local topics with a common set of
things on top of master.
The next thing that happens is that master is updated, and I want to
rebase both topics on top of the new master.
So I now have:
A---G
\---B---C---D---E
\---F
If I do the dumb thing, which is to do `git rebase master E` and `git
rebase master F`, I end up with:
A---G---B'---C'---D'---E'
\---B"---C"---D"---F'
That is, I just lost the fast that E and F had common history.
I could go with `git rebase master E` followed by `git rebase --onto D'
D F` but that's cumbersome, especially when you have more than 2 topics,
not necessarily diverging at the same point (e.g. I might have another
topic that diverges at C)
So, what I end up doing is something like:
- git co -b merge E
- git merge --strategy ours F (and any other topic I might want to
rebase)
- git rebase master --preserve-merges
If everything goes fine, then I can `git update-ref` the topics to each
parent of the merge branch.
But, usually, since rebase --preserve-merges redoes merges with the
default strategy, I end up with conflicts, and instead of trying to
figure stuff out, I just pick the rewritten sha1s from
.git/rebase-merge/rewritten/* to update the refs.
It is my understanding that the --strategy option to git-rebase is used
for the rebase itself, so I'm not sure there's a way to tell rebase to
use a specific strategy for the preserved merges only.
Anyways, it /seems/ like just allowing multiple branches on the git
rebase command line and make this work would improve things
significantly. The question then, is how would that interact with other
options (I'm thinking e.g. -i, but -i already has a problem with
--preserve-merges). But it does seem like it would be a worthwhile
improvement.
What do you think?
Mike
^ permalink raw reply
* Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Jeff King @ 2016-12-31 17:58 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <f5ced16d-61dc-ba14-7f29-88f20d4a65d2@alum.mit.edu>
On Sat, Dec 31, 2016 at 08:58:43AM +0100, Michael Haggerty wrote:
> > The return value is always "0" or "-1". It seems like it would be
> > simpler to just return the descriptor instead of 0.
> >
> > I guess that makes it hard to identify the case when we chose not to
> > create a descriptor. I wonder if more "normal" semantics would be:
> >
> > 1. ret >= 0: file existed or was created, and ret is the descriptor
> >
> > 2. ret < 0, err is empty: we chose not to create
> >
> > 3. ret < 0, err is non-empty: a real error
>
> I don't like making callers read err to find out whether the call was
> successful, and I think we've been able to avoid that pattern so far.
I guess my mental model is that case 2 _is_ a failure, because we didn't
open the reflog. It's just one that callers may want to distinguish from
case 3, because it's probably a silent failure, not one we want to
complain to the user about.
But whether that's accurate would depend on the callers. Looking at the
callers, I think the immediate callers would be happier with this, but
you probably would want to end up converting case 3 back to "return 0"
out of files_log_ref_write().
> > I dunno. This may just be bikeshedding, and I can live with it either
> > way (especially because you documented it!).
>
> Let's see if anybody has a strong opinion about it; otherwise I'd rather
> leave it as is.
Sounds good.
-Peff
^ permalink raw reply
* [PATCH] don't use test_must_fail with grep
From: Pranit Bauva @ 2016-12-31 11:44 UTC (permalink / raw)
To: git; +Cc: sbeller, Pranit Bauva
test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.
Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 6 +++---
t/t5504-fetch-receive-strict.sh | 2 +-
t/t5516-fetch-push.sh | 2 +-
t/t5601-clone.sh | 2 +-
t/t6030-bisect-porcelain.sh | 2 +-
t/t7610-mergetool.sh | 2 +-
t/t9001-send-email.sh | 2 +-
t/t9117-git-svn-init-clone.sh | 12 ++++++------
t/t9813-git-p4-preserve-users.sh | 8 ++++----
t/t9814-git-p4-rename.sh | 6 +++---
10 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "cherry picked from" initial_msg &&
+ ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "Signed-off-by:" initial_msg &&
+ ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
- test_must_fail grep "Signed-off-by:" picked_msg &&
+ ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
'
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
- test_must_fail grep "missingEmail" act
+ ! grep "missingEmail" act
'
test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd refs/heads/master:refs/remotes/origin/master &&
- test_must_fail grep -q Done .git/bar
+ ! grep -q Done .git/bar
'
test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
git show-ref 2> clone.err > clone.out) &&
- test_must_fail grep Duplicate mirror2/clone.err &&
+ ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
'
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
- test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+ ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
'
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
- test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+ ! grep ^\./both_LOCAL_ actual >/dev/null &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
'
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {
--smtp-server="$(pwd)/fake.sendmail" \
$@ \
$patches >stdout &&
- test_must_fail grep "Send this email" stdout &&
+ ! grep "Send this email" stdout &&
>no_confirm_okay
}
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 69a675052..044f65e91 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -55,7 +55,7 @@ test_expect_success 'clone to target directory with --stdlayout' '
test_expect_success 'init without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn init "$svnrepo"/project/trunk trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -63,7 +63,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
test_expect_success 'clone without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -86,7 +86,7 @@ EOF
test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn init -s "$svnrepo"/project project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -95,7 +95,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn clone -s "$svnrepo"/project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -104,7 +104,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
@@ -113,7 +113,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 0fe231280..2384535a7 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed authorship' '
grep "git author charlie@example.com does not match" &&
make_change_by_user usernamefile3 alice alice@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ ! git p4 commit |\
+ grep "git author.*does not match" &&
git config git-p4.skipUserNameCheck true &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ ! git p4 commit |\
+ grep "git author.*does not match" &&
p4_check_commit_author usernamefile3 alice
)
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index c89992cf9..e7e0268e9 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -141,7 +141,7 @@ test_expect_success 'detect copies' '
git diff-tree -r -C HEAD &&
git p4 submit &&
p4 filelog //depot/file8 &&
- p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file8 | grep -q "branch from" &&
echo "file9" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -154,7 +154,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies true &&
git p4 submit &&
p4 filelog //depot/file9 &&
- p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file9 | grep -q "branch from" &&
echo "file10" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -202,7 +202,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies $(($level + 2)) &&
git p4 submit &&
p4 filelog //depot/file12 &&
- p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file12 | grep -q "branch from" &&
echo "file13" >>file2 &&
git commit -a -m "Differentiate file2" &&
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 2/6] Add ability to follow a remote branch with a dialog
From: Paul Mackerras @ 2016-12-31 8:53 UTC (permalink / raw)
To: Pierre Dumuid; +Cc: git
In-Reply-To: <20161215112847.14719-2-pmdumuid@gmail.com>
On Thu, Dec 15, 2016 at 09:58:43PM +1030, Pierre Dumuid wrote:
> A suggested name is provided when creating a new "following" branch.
>
> Signed-off-by: Pierre Dumuid <pmdumuid@gmail.com>
> ---
> gitk | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/gitk b/gitk
> index 50d1ef4..36cba49 100755
> --- a/gitk
> +++ b/gitk
> @@ -2673,6 +2673,7 @@ proc makewindow {} {
> {mc "Rename this branch" command mvbranch}
> {mc "Remove this branch" command rmbranch}
> {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
> + {mc "Follow this branch" command follow_remote_branch_dialog}
> }
> $headctxmenu configure -tearoff 0
>
> @@ -9947,23 +9948,100 @@ proc headmenu {x y id head} {
> stopfinding
> set headmenuid $id
> set headmenuhead $head
> - array set state {0 normal 1 normal 2 normal}
> + array set state {0 normal 1 normal 2 normal 3 normal}
> if {[string match "remotes/*" $head]} {
> set localhead [string range $head [expr [string last / $head] + 1] end]
> if {[info exists headids($localhead)]} {
> set state(0) disabled
> }
> - array set state {1 disabled 2 disabled}
> + array set state {1 disabled 2 disabled 3 normal}
You set array(3) to "normal" just above, no need to do it again.
> }
> if {$head eq $mainhead} {
> - array set state {0 disabled 2 disabled}
> + array set state {0 disabled 2 disabled 3 disabled}
> + } else {
> + set state(3) disabled
> }
As far as I can see, this will always end up with state(3) set to
"disabled", won't it? Is that really what you want?
Paul.
^ permalink raw reply
* Re: [PATCH 3/6] Add a tree view to the local branches, remote branches and tags, where / is treated as a directory seperator.
From: Paul Mackerras @ 2016-12-31 9:08 UTC (permalink / raw)
To: Pierre Dumuid; +Cc: git
In-Reply-To: <20161215112847.14719-3-pmdumuid@gmail.com>
On Thu, Dec 15, 2016 at 09:58:44PM +1030, Pierre Dumuid wrote:
> Signed-off-by: Pierre Dumuid <pmdumuid@gmail.com>
> ---
> gitk | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 117 insertions(+)
Nice idea in general... a few comments below. Also, please don't put
the entire commit message in the subject line. :)
> diff --git a/gitk b/gitk
> index 36cba49..a894f1d 100755
> --- a/gitk
> +++ b/gitk
> @@ -2089,6 +2089,10 @@ proc makewindow {} {
> {mc "Reread re&ferences" command rereadrefs}
> {mc "&List references" command showrefs -accelerator F2}
> {xx "" separator}
> + {mc "List Local Branches" command {show_tree_of_references_dialog "localBranches"} -accelerator F6}
> + {mc "List Remote Branches" command {show_tree_of_references_dialog "remoteBranches"} -accelerator F7}
> + {mc "List Tags" command {show_tree_of_references_dialog "tags"} -accelerator F8}
> + {xx "" separator}
> {mc "Start git &gui" command {exec git gui &}}
> {xx "" separator}
> {mc "&Quit" command doquit -accelerator Meta1-Q}
> @@ -2601,6 +2605,9 @@ proc makewindow {} {
> bind . <F5> updatecommits
> bindmodfunctionkey Shift 5 reloadcommits
> bind . <F2> showrefs
> + bind . <F6> {show_tree_of_references_dialog "localBranches"}
> + bind . <F7> {show_tree_of_references_dialog "remoteBranches"}
> + bind . <F8> {show_tree_of_references_dialog "tags"}
> bindmodfunctionkey Shift 4 {newview 0}
> bind . <F4> edit_or_newview
> bind . <$M1B-q> doquit
> @@ -10146,6 +10153,116 @@ proc rmbranch {} {
> run refill_reflist
> }
>
> +# Display a tree view of local branches, remote branches, and tags according to view_type.
> +#
> +# @param string view_type
> +# Must be one of "localBranches", "remoteBranches", or "tags".
> +#
> +proc show_tree_of_references_dialog {view_type} {
> + global NS
> + global treefilelist
> + global headids tagids
> +
> + switch -- $view_type {
> + "localBranches" {
> + set dialogName "Local Branches"
> + set top .show_tree_of_local_branches
> + set listOfReferences [lsort [array names headids -regexp {^(?!remotes/)} ]]
> + set truncateFrom 0
> + }
> + "remoteBranches" {
> + set dialogName "Remote Branches"
> + set top .show_tree_of_remote_branches
> + set listOfReferences [lsort [array names headids -regexp {^remotes/} ]]
> + set truncateFrom 8
> + }
> + "tags" {
> + set dialogName "Tags"
> + set top .show_tree_of_tags
> + set listOfReferences [lsort [array names tagids]]
> + set truncateFrom 0
> + }
> + }
> +
> + if {[winfo exists $top]} {
> + raise $top
> + return
> + }
> +
> + ttk_toplevel $top
> + wm title $top [mc "$dialogName: %s" [file tail [pwd]]]
> + wm geometry $top "600x900"
Do you really need to do this? A fixed size like this is inevitably
going to be too big for some users and too small for others.
> +
> + make_transient $top .
> +
> + ## See http://www.tkdocs.com/tutorial/tree.html
> + ttk::treeview $top.referenceList -xscrollcommand "$top.horizontalScrollBar set" -yscrollcommand "$top.verticalScrollBar set"
We still have the option for people to run without ttk, in case
someone is still using an old Tcl/Tk version or just doesn't like the
ttk widgets. However, there isn't an equivalent of ttk::treeview in
the older Tk widget set. It would be OK to omit the new menu entries
or to disable them if $use_ttk is false, but I don't want to have menu
entries that will always cause gitk to blow up when $use_ttk is false.
We possibly should consider converting the file list view to use a
ttk::treeview when $use_ttk is true.
Paul.
^ permalink raw reply
* Re: [PATCH 1/6] Enable ability to visualise the results of git cherry C1 C2
From: Paul Mackerras @ 2016-12-31 8:30 UTC (permalink / raw)
To: Pierre Dumuid; +Cc: git
In-Reply-To: <20161215112847.14719-1-pmdumuid@gmail.com>
On Thu, Dec 15, 2016 at 09:58:42PM +1030, Pierre Dumuid wrote:
> It's a bit clunky but it works!!
>
> Usage:
> - mark commit one (e.g. v45)
> - Select commit two.
> - Switch the gdttype to the option, "git-cherry between marked commit and:"
This needs a better description. "Git-cherry between marked commit
and" is a description of an implementation not a description of what's
being achieved. Having read the git cherry man page, it seems like
it's (Find commit) included in marked commit but not in this commit
(or the other way around?). We would need a terser description that
that, though.
[...]
> +proc update_gitcherrylist {} {
> + global gitcherryids
> + global markedid
> + global findstring
> + global fstring
> + global currentid
> + global iddrawn
> +
> + unset -nocomplain gitcherryids
> + set fs $findstring
> +
> + if {$findstring eq {}} {
> + $fstring delete 0 end
> + $fstring insert 0 $currentid
> + }
> +
> + if {![info exists markedid]} {
> + error_popup [mc "Please mark a git commit before using this find method!"]
> + return
> + }
> +
> + #puts [join [list "Running cherry between: `" $markedid "` and `" $findstring "`"] ""]
> +
> + if {[catch {set cherryOutput [exec git cherry $markedid $findstring]}]} {
How long could the git cherry take to run? If it's more than a
fraction of a second, then we would need to handle its output
asynchronously like we do in [do_file_hl].
Paul.
^ permalink raw reply
* Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Pranit Bauva @ 2016-12-31 10:43 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <06402c8a-14a4-3d70-8d98-659cfe9f1aa2@gmx.net>
Hey Stephan,
On Tue, Nov 22, 2016 at 3:05 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi Pranit,
>
> in this mail I review the "second part" of your patch: the transition of
> bisect_next and bisect_auto_next to C.
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1d3e17f..fcd7574 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -408,6 +411,136 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
>> return 0;
>> }
>>
>> +static int register_good_ref(const char *refname,
>> + const struct object_id *oid, int flags,
>> + void *cb_data)
>> +{
>> + struct string_list *good_refs = cb_data;
>> + string_list_append(good_refs, oid_to_hex(oid));
>> + return 0;
>> +}
>> +
>> +static int bisect_next(struct bisect_terms *terms, const char *prefix)
>> +{
>> + int res, no_checkout;
>> +
>> + /*
>> + * In case of mistaken revs or checkout error, or signals received,
>> + * "bisect_auto_next" below may exit or misbehave.
>> + * We have to trap this to be able to clean up using
>> + * "bisect_clean_state".
>> + */
>
> The comment above makes no sense here, or does it?
No it doesn't right now. I had this initally because I was trying to
keep bisect_clean_state() call inside this function but then I
realized it should be placed elsewhere. I will remove this.
>> + if (bisect_next_check(terms, terms->term_good))
>> + return -1;
>> +
>> + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
>> +
>> + /* Perform all bisection computation, display and checkout */
>> + res = bisect_next_all(prefix , no_checkout);
>
> Style: there is a space left of the comma.
Nice catch! ;) I have gone through this patch so many times, yet I
have forgot to notice this.
>> +
>> + if (res == 10) {
>> + FILE *fp = NULL;
>> + unsigned char sha1[20];
>> + struct commit *commit;
>> + struct pretty_print_context pp = {0};
>> + struct strbuf commit_name = STRBUF_INIT;
>> + char *bad_ref = xstrfmt("refs/bisect/%s",
>> + terms->term_bad);
>> + int retval = 0;
>> +
>> + read_ref(bad_ref, sha1);
>> + commit = lookup_commit_reference(sha1);
>> + format_commit_message(commit, "%s", &commit_name, &pp);
>> + fp = fopen(git_path_bisect_log(), "a");
>> + if (!fp) {
>> + retval = -1;
>> + goto finish_10;
>> + }
>> + if (fprintf(fp, "# first %s commit: [%s] %s\n",
>> + terms->term_bad, sha1_to_hex(sha1),
>> + commit_name.buf) < 1){
>> + retval = -1;
>> + goto finish_10;
>> + }
>> + goto finish_10;
>> + finish_10:
>> + if (fp)
>> + fclose(fp);
>> + strbuf_release(&commit_name);
>> + free(bad_ref);
>> + return retval;
>> + }
>> + else if (res == 2) {
>> + FILE *fp = NULL;
>> + struct rev_info revs;
>> + struct argv_array rev_argv = ARGV_ARRAY_INIT;
>> + struct string_list good_revs = STRING_LIST_INIT_DUP;
>> + struct pretty_print_context pp = {0};
>> + struct commit *commit;
>> + char *term_good = xstrfmt("%s-*", terms->term_good);
>> + int i, retval = 0;
>> +
>> + fp = fopen(git_path_bisect_log(), "a");
>> + if (!fp) {
>> + retval = -1;
>> + goto finish_2;
>> + }
>> + if (fprintf(fp, "# only skipped commits left to test\n") < 1) {
>> + retval = -1;
>> + goto finish_2;
>> + }
>> + for_each_glob_ref_in(register_good_ref, term_good,
>> + "refs/bisect/", (void *) &good_revs);
>> +
>> + argv_array_pushl(&rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
>> + for (i = 0; i < good_revs.nr; i++)
>> + argv_array_push(&rev_argv, good_revs.items[i].string);
>> +
>> + /* It is important to reset the flags used by revision walks
>> + * as the previous call to bisect_next_all() in turn
>> + * setups a revision walk.
>> + */
>> + reset_revision_walk();
>> + init_revisions(&revs, NULL);
>> + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
>> + argv_array_clear(&rev_argv);
>> + string_list_clear(&good_revs, 0);
>> + if (prepare_revision_walk(&revs))
>> + die(_("revision walk setup failed\n"));
>> +
>> + while ((commit = get_revision(&revs)) != NULL) {
>> + struct strbuf commit_name = STRBUF_INIT;
>> + format_commit_message(commit, "%s",
>> + &commit_name, &pp);
>> + fprintf(fp, "# possible first %s commit: "
>> + "[%s] %s\n", terms->term_bad,
>> + oid_to_hex(&commit->object.oid),
>> + commit_name.buf);
>> + strbuf_release(&commit_name);
>> + }
>> + goto finish_2;
>> + finish_2:
>> + if (fp)
>> + fclose(fp);
>> + string_list_clear(&good_revs, 0);
>> + argv_array_clear(&rev_argv);
>> + free(term_good);
>> + if (retval)
>> + return retval;
>> + else
>> + return res;
>> + }
>> + return res;
>> +}
>
> It would be much nicer if you put the (res == 10) branch and the
> (res == 2) branch into separate functions and just call them.
> Then you also won't need ugly label naming like finish_10 or finish_2.
> I'd also (again) recommend to use goto fail instead of setting retval to
> -1 separately each time.
Yes, that seems a better way to go! Thanks! :)
> I'd also recommend to use a separate function to append to the bisect
> log file. There is a lot of duplicated opening, checking, closing code;
> IIRC such a function would also already be handy for some of the
> previous patches.
True. I have made that function. Will reuse it here. Thanks! :)
>> +
>> +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)
>> +{
>> + if (!bisect_next_check(terms, NULL))
>> + return bisect_next(terms, prefix);
>> +
>> + return 0;
>> +}
>
> Hmm, the handling of the return values is a little confusing. However,
> if I understand the sh source correctly, it always returns success, no
> matter if bisect_next failed or not. I do not know if you had something
> special in mind here.
Umm. Shell code used to die() and thus exit with an error code. Thus
it is important to return the exit code. Handling return values was
*very confusing* for me too while writing this commit ;) A lot of
behaviour is changed in this commit regarding return values.
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> @@ -643,6 +794,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("print out the bisect terms"), BISECT_TERMS),
>> OPT_CMDMODE(0, "bisect-start", &cmdmode,
>> N_("start the bisect session"), BISECT_START),
>> + OPT_CMDMODE(0, "bisect-next", &cmdmode,
>> + N_("find the next bisection commit"), BISECT_NEXT),
>> + OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
>> + N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
>
> The next bisection *state* is found?
checkout is more appropriate. I don't remember why I used "find".
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index f0896b3..d574c44 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -139,45 +119,7 @@ bisect_state() {
>> *)
>> usage ;;
>> esac
>> - bisect_auto_next
> [...deleted lines...]
>> + git bisect--helper --bisect-auto-next || exit
>
> Why is the "|| exit" necessary?
Since it returning exit code, we need to quit, see earlier comment.
Earlier in shell script we used die() but now we are using return
error() thus we need to make it exit here too.
>> @@ -319,14 +260,15 @@ case "$#" in
>> help)
>> git bisect -h ;;
>> start)
>> - bisect_start "$@" ;;
>> + git bisect--helper --bisect-start "$@" ;;
>> bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
>> bisect_state "$cmd" "$@" ;;
>> skip)
>> bisect_skip "$@" ;;
>> next)
>> # Not sure we want "next" at the UI level anymore.
>> - bisect_next "$@" ;;
>> + get_terms
>> + git bisect--helper --bisect-next "$@" || exit ;;
>
> Why is the "|| exit" necessary? ;)
Same as before.
>
> Furthermore:
> Where is the bisect_autostart call from bisect_next() sh source gone?
> Was it not necessary?
It is necessary, but I couldn't just incorporate it in one commit. The
functions are called in a circular fashion. Thus I skipped
bisect_autostart() as for now but then when I managed to port
bisect_autostart(), I called that function from this.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Pranit Bauva @ 2016-12-31 10:23 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <8bd0682f-e35e-a50e-24a9-fd3a53454ed4@gmx.net>
Hey Stephan,
Extremely sorry I just forgot to reply to this email before. I was
preparing from the next iteration when I saw this.
On Mon, Nov 21, 2016 at 1:31 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi Pranit,
>
> this one is hard to review because you do two or three commits in one here.
> I think the first commit should be the exit()->return conversion, the
> second commit is next and autonext, and the third commit is the pretty
> trivial bisect_start commit ;) However, you did it this way and it's
> always a hassle to split commit, so I don't really care...
I had confusion about how to split the commits, but then I then
decided to dump it all together so that it compiles (I was finding it
difficult to split into meaningful parts which also compiled).
> However, I was reviewing this superficially, to be honest. This mail
> skips the next and autonext part.
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/bisect.c b/bisect.c
>> index 45d598d..7c97e85 100644
>> --- a/bisect.c
>> +++ b/bisect.c
>> @@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix)
>> *
>> * If that's not the case, we need to check the merge bases.
>> * If a merge base must be tested by the user, its source code will be
>> - * checked out to be tested by the user and we will exit.
>> + * checked out to be tested by the user and we will return.
>> */
>> -static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
>> +static int check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
>> {
>> char *filename = git_pathdup("BISECT_ANCESTORS_OK");
>> struct stat st;
>> - int fd;
>> + int fd, res = 0;
>>
>> + /*
>> + * We don't want to clean the bisection state
>> + * as we need to get back to where we started
>> + * by using `git bisect reset`.
>> + */
>> if (!current_bad_oid)
>> - die(_("a %s revision is needed"), term_bad);
>> + error(_("a %s revision is needed"), term_bad);
>
> Only error() or return error()?
It should be return error(). Thanks for pointing it out! :)
>> @@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
>> filename);
>> else
>> close(fd);
>> +
>> + goto done;
>> done:
>
> I never understand why you do this. In case of adding a "fail" label
> (and fail code like "res = -1;") between "goto done" and "done:", it's
> fine... but without one this is just a nop.
I will just remove that line.
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1d3e17f..fcd7574 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> no_checkout = 1;
>>
>> for (i = 0; i < argc; i++) {
>> - if (!strcmp(argv[i], "--")) {
>> + const char *arg;
>> + if (starts_with(argv[i], "'"))
>> + arg = sq_dequote(xstrdup(argv[i]));
>> + else
>> + arg = argv[i];
>
> One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's
> an inconsistent leak... I guess it's a bad idea to do it this way ;)
> (Also below.)
Yes. I will use xstrdup() and it does leak.
>> @@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> no_checkout = 1;
>> } else if (!strcmp(arg, "--term-good") ||
>> !strcmp(arg, "--term-old")) {
>> + if (starts_with(argv[++i], "'"))
>> + terms->term_good = sq_dequote(xstrdup(argv[i]));
>> + else
>> + terms->term_good = xstrdup(argv[i]);
>> must_write_terms = 1;
>> - terms->term_good = xstrdup(argv[++i]);
>> } else if (skip_prefix(arg, "--term-good=", &arg)) {
>> must_write_terms = 1;
>> - terms->term_good = xstrdup(arg);
>> + terms->term_good = arg;
>
> No ;) (See my other comments (to other patches) for the "terms" leaks.)
Yes I have addressed this issue.
> [This repeats several times below.]
>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index f0896b3..d574c44 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -109,6 +88,7 @@ bisect_skip() {
>> bisect_state() {
>> bisect_autostart
>> state=$1
>> + get_terms
>> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
>> get_terms
>> case "$#,$state" in
>
> I can't say if this change is right or wrong. It looks right, but: How
> does this relate to the other changes? Is this the right patch for it?
This line is because of the following:
* TERM_BAD and TERM_GOOD are global but in the coming patch they
would be removed as global variables.
* To compensate for that, I will write out the state of TERM_BAD and
TERM_GOOD every time it is updated in the file BISECT_TERMS.
* So we will be reading it from there.
* It is quite possible that this is completely redundant as for now
but I really don't care to check for each case because I have removed
the shell function bisect_state() afterwards and then this line won't
create a problem there because we are using `struct bisect_terms`
there.
^ permalink raw reply
* Re: [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument
From: Michael Haggerty @ 2016-12-31 8:01 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231063523.fncqqpr3m42jjvbs@sigill.intra.peff.net>
On 12/31/2016 07:35 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:12:54AM +0100, Michael Haggerty wrote:
>
>> It's unnecessary to pass a strbuf holding the reflog path up and down
>> the call stack now that it is hardly needed by the callers. Remove the
>> places where log_ref_write_1() uses it, in preparation for making it
>> internal to log_ref_setup().
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> refs/files-backend.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 7f26cf8..5a96424 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
>> result = log_ref_write_fd(logfd, old_sha1, new_sha1,
>> git_committer_info(0), msg);
>> if (result) {
>> - strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
>> - strerror(errno));
>> + int save_errno = errno;
>> +
>> + strbuf_addf(err, "unable to append to '%s': %s",
>> + git_path("logs/%s", refname), strerror(save_errno));
>
> Hmm. This means the logic of "the path for a reflog is
> git_path(logs/%s)" is now replicated in several places. Which feels kind
> of like a backwards step. But I guess it is pretty well cemented in the
> concept of files-backend.c, and I do like the later cleanups that this
> allows.
This might end up in a helper function in the not-too-distant future for
other reasons, but given that such code already appears multiple times,
I didn't feel too guilty about it.
Michael
^ permalink raw reply
* Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Michael Haggerty @ 2016-12-31 7:58 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231063211.tqsiafg3iahcuotz@sigill.intra.peff.net>
On 12/31/2016 07:32 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote:
>
>> This function will most often be called by log_ref_write_1(), which
>> wants to append to the reflog file. In that case, it is silly to close
>> the file only for the caller to reopen it immediately. So, in the case
>> that the file was opened, pass the open file descriptor back to the
>> caller.
>
> Sounds like a much saner interface.
>
>> /*
>> - * Create a reflog for a ref. If force_create = 0, the reflog will
>> - * only be created for certain refs (those for which
>> - * should_autocreate_reflog returns non-zero. Otherwise, create it
>> - * regardless of the ref name. Fill in *err and return -1 on failure.
>> + * Create a reflog for a ref. Store its path to *logfile. If
>> + * force_create = 0, only create the reflog for certain refs (those
>> + * for which should_autocreate_reflog returns non-zero). Otherwise,
>> + * create it regardless of the reference name. If the logfile already
>> + * existed or was created, return 0 and set *logfd to the file
>> + * descriptor opened for appending to the file. If no logfile exists
>> + * and we decided not to create one, return 0 and set *logfd to -1. On
>> + * failure, fill in *err, set *logfd to -1, and return -1.
>> */
>> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
>> +static int log_ref_setup(const char *refname,
>> + struct strbuf *logfile, int *logfd,
>> + struct strbuf *err, int force_create)
>
> The return value is always "0" or "-1". It seems like it would be
> simpler to just return the descriptor instead of 0.
>
> I guess that makes it hard to identify the case when we chose not to
> create a descriptor. I wonder if more "normal" semantics would be:
>
> 1. ret >= 0: file existed or was created, and ret is the descriptor
>
> 2. ret < 0, err is empty: we chose not to create
>
> 3. ret < 0, err is non-empty: a real error
I don't like making callers read err to find out whether the call was
successful, and I think we've been able to avoid that pattern so far.
Another alternative would be to make ret == -1 mean a real error and ret
== -2 mean that we chose not to create the file. But that would be
straying from the usual "-1 means error" interface of open(), so I
wasn't fond of that idea either.
> I dunno. This may just be bikeshedding, and I can live with it either
> way (especially because you documented it!).
Let's see if anybody has a strong opinion about it; otherwise I'd rather
leave it as is.
Michael
^ permalink raw reply
* Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create
From: Michael Haggerty @ 2016-12-31 7:52 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231062607.uxftwujophv37ymb@sigill.intra.peff.net>
On 12/31/2016 07:26 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:
>> [...]
>> - adjust_shared_perm(logfile->buf);
>> - close(logfd);
>> + if (logfd >= 0) {
>> + adjust_shared_perm(logfile->buf);
>> + close(logfd);
>> + }
>> +
>
> Hmm. I would have thought in the existing-logfile case that we would not
> need to adjust_shared_perm(). But maybe we just do it anyway to pick up
> potentially-changed config.
I didn't change this aspect of the code's behavior (though I also found
it a bit surprising).
Another thing I considered was changing adjust_shared_perm() to adjust
the file's permissions through the open file descriptor using fchmod().
But that function has a bunch of callers, and I didn't want to have to
duplicate the code, nor did I have the energy to change all of its
callers (if that would even make sense for all callers, which I doubt).
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v3 05/23] raceproof_create_file(): new function
From: Michael Haggerty @ 2016-12-31 7:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231061146.gxlbma6w7odho4c7@sigill.intra.peff.net>
On 12/31/2016 07:11 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote:
>> [...]
>> +/*
>> + * Callback function for raceproof_create_file(). This function is
>> + * expected to do something that makes dirname(path) permanent despite
>> + * the fact that other processes might be cleaning up empty
>> + * directories at the same time. Usually it will create a file named
>> + * path, but alternatively it could create another file in that
>> + * directory, or even chdir() into that directory. The function should
>> + * return 0 if the action was completed successfully. On error, it
>> + * should return a nonzero result and set errno.
>> + * raceproof_create_file() treats two errno values specially:
>> + *
>> + * - ENOENT -- dirname(path) does not exist. In this case,
>> + * raceproof_create_file() tries creating dirname(path)
>> + * (and any parent directories, if necessary) and calls
>> + * the function again.
>> + *
>> + * - EISDIR -- the file already exists and is a directory. In this
>> + * case, raceproof_create_file() deletes the directory
>> + * (recursively) if it is empty and calls the function
>> + * again.
>
> It took me a minute to figure out why EISDIR is recursive.
>
> If we are trying to create "foo/bar/baz", we can only get EISDIR when
> "baz" exists and is a directory. I at first took your recursive to me
> that we delete it and "foo/bar" and "foo". Which is just silly and
> counterproductive.
>
> But presumably you mean that we delete "foo/bar/baz/xyzzy", etc, up to
> "foo/bar/baz", provided they are all empty directories. I think your
> comment is probably OK and I was just being thick, but maybe stating it
> like:
>
> ...removes the directory if it is empty (and recursively any empty
> directories it contains) and calls the function again.
>
> would be more clear. That is still leaving the definition of "empty"
> implied, but it's hopefully obvious from the context.
Yes, that's clearer. I'll change it. Thanks!
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Jeff King @ 2016-12-31 6:47 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:
> This is a re-roll of an old patch series. v1 [1] got some feedback,
> which I think was all addressed in v2 [2]. But it seems that v2 fell
> on the floor, and I didn't bother following up because it was in the
> same area of code that was undergoing heavy changes due to the
> pluggable reference backend work. Sorry for the long delay before
> getting back to it.
I've read through the whole thing, and aside from a few very minor nits
(that I am not even sure are worth a re-roll), I didn't see anything
wrong. And the overall goal and approach seem obviously sound.
> Michael Haggerty (23):
I'll admit to being daunted by the number of patches, but it was quite a
pleasant and easy read. Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Jeff King @ 2016-12-31 6:40 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <6164af2d1f9eeb5bd339b3913e8046c1ea0b02be.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:
> It's bad manners and surprising and therefore error-prone.
Agreed.
I wondered while reading your earlier patch whether the
safe_create_leading_directories() function had the same problem, but it
carefully replaces the NUL it inserts.
> -static void try_remove_empty_parents(char *refname)
> +static void try_remove_empty_parents(const char *refname)
> {
> + struct strbuf buf = STRBUF_INIT;
> char *p, *q;
> int i;
> - p = refname;
> +
> + strbuf_addstr(&buf, refname);
I see here you just make a copy. I think it would be enough to do:
> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
> q--;
> if (q == p)
> break;
> - *q = '\0';
> - if (rmdir(git_path("%s", refname)))
> + strbuf_setlen(&buf, q - buf.buf);
> + if (rmdir(git_path("%s", buf.buf)))
> break;
*q = '\0';
r = rmdir(git_path("%s", refname));
*q = '/';
if (r)
break;
or something. But I doubt the single allocation is breaking the bank,
and it has the nice side effect that callers can pass in a const string
(I didn't check yet whether that enables further cleanups).
-Peff
^ permalink raw reply
* Re: [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument
From: Jeff King @ 2016-12-31 6:35 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <1e1295aff09039fc49188b085bda6ee5166d313e.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:12:54AM +0100, Michael Haggerty wrote:
> It's unnecessary to pass a strbuf holding the reflog path up and down
> the call stack now that it is hardly needed by the callers. Remove the
> places where log_ref_write_1() uses it, in preparation for making it
> internal to log_ref_setup().
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs/files-backend.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 7f26cf8..5a96424 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
> result = log_ref_write_fd(logfd, old_sha1, new_sha1,
> git_committer_info(0), msg);
> if (result) {
> - strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
> - strerror(errno));
> + int save_errno = errno;
> +
> + strbuf_addf(err, "unable to append to '%s': %s",
> + git_path("logs/%s", refname), strerror(save_errno));
Hmm. This means the logic of "the path for a reflog is
git_path(logs/%s)" is now replicated in several places. Which feels kind
of like a backwards step. But I guess it is pretty well cemented in the
concept of files-backend.c, and I do like the later cleanups that this
allows.
-Peff
^ permalink raw reply
* Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Jeff King @ 2016-12-31 6:32 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <ef2355e9d5ccaa53928c821530bae59f2b118013.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote:
> This function will most often be called by log_ref_write_1(), which
> wants to append to the reflog file. In that case, it is silly to close
> the file only for the caller to reopen it immediately. So, in the case
> that the file was opened, pass the open file descriptor back to the
> caller.
Sounds like a much saner interface.
> /*
> - * Create a reflog for a ref. If force_create = 0, the reflog will
> - * only be created for certain refs (those for which
> - * should_autocreate_reflog returns non-zero. Otherwise, create it
> - * regardless of the ref name. Fill in *err and return -1 on failure.
> + * Create a reflog for a ref. Store its path to *logfile. If
> + * force_create = 0, only create the reflog for certain refs (those
> + * for which should_autocreate_reflog returns non-zero). Otherwise,
> + * create it regardless of the reference name. If the logfile already
> + * existed or was created, return 0 and set *logfd to the file
> + * descriptor opened for appending to the file. If no logfile exists
> + * and we decided not to create one, return 0 and set *logfd to -1. On
> + * failure, fill in *err, set *logfd to -1, and return -1.
> */
> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
> +static int log_ref_setup(const char *refname,
> + struct strbuf *logfile, int *logfd,
> + struct strbuf *err, int force_create)
The return value is always "0" or "-1". It seems like it would be
simpler to just return the descriptor instead of 0.
I guess that makes it hard to identify the case when we chose not to
create a descriptor. I wonder if more "normal" semantics would be:
1. ret >= 0: file existed or was created, and ret is the descriptor
2. ret < 0, err is empty: we chose not to create
3. ret < 0, err is non-empty: a real error
I dunno. This may just be bikeshedding, and I can live with it either
way (especially because you documented it!).
-Peff
^ permalink raw reply
* Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create
From: Jeff King @ 2016-12-31 6:26 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:
> + } else {
> + logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
> if (logfd < 0) {
> - strbuf_addf(err, "unable to append to '%s': %s",
> - logfile->buf, strerror(errno));
> - return -1;
> + if (errno == ENOENT || errno == EISDIR) {
> + /*
> + * The logfile doesn't already exist,
> + * but that is not an error; it only
> + * means that we won't write log
> + * entries to it.
> + */
> + } else {
> + strbuf_addf(err, "unable to append to '%s': %s",
> + logfile->buf, strerror(errno));
> + return -1;
> + }
> }
> }
>
> - adjust_shared_perm(logfile->buf);
> - close(logfd);
> + if (logfd >= 0) {
> + adjust_shared_perm(logfile->buf);
> + close(logfd);
> + }
> +
Hmm. I would have thought in the existing-logfile case that we would not
need to adjust_shared_perm(). But maybe we just do it anyway to pick up
potentially-changed config.
I also had to double-take at this close(). Aren't we calling this
function so we can actually write to the log? But I skipped ahead in
your series and see you address that confusion. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 05/23] raceproof_create_file(): new function
From: Jeff King @ 2016-12-31 6:11 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <f933f9d3c4c53b42ecc75b7a743ed4bfd390b4c5.1483153436.git.mhagger@alum.mit.edu>
On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote:
> Add a function that tries to create a file and any containing
> directories in a way that is robust against races with other processes
> that might be cleaning up empty directories at the same time.
>
> The actual file creation is done by a callback function, which, if it
> fails, should set errno to EISDIR or ENOENT according to the convention
> of open(). raceproof_create_file() detects such failures, and
> respectively either tries to delete empty directories that might be in
> the way of the file or tries to create the containing directories. Then
> it retries the callback function.
This seems like a nice primitive, and the resulting change in patch 7 is
very pleasant.
At first I was surprised that the callback did not take the more usual
open(2) flags, which might make it easy to reuse a few basic callbacks.
But I see that in most cases the actual opening is deep inside a higher
level construct like the lockfile code, and anything beyond the "void *"
callback parameter that you have would make that really awkward.
> +/*
> + * Callback function for raceproof_create_file(). This function is
> + * expected to do something that makes dirname(path) permanent despite
> + * the fact that other processes might be cleaning up empty
> + * directories at the same time. Usually it will create a file named
> + * path, but alternatively it could create another file in that
> + * directory, or even chdir() into that directory. The function should
> + * return 0 if the action was completed successfully. On error, it
> + * should return a nonzero result and set errno.
> + * raceproof_create_file() treats two errno values specially:
> + *
> + * - ENOENT -- dirname(path) does not exist. In this case,
> + * raceproof_create_file() tries creating dirname(path)
> + * (and any parent directories, if necessary) and calls
> + * the function again.
> + *
> + * - EISDIR -- the file already exists and is a directory. In this
> + * case, raceproof_create_file() deletes the directory
> + * (recursively) if it is empty and calls the function
> + * again.
It took me a minute to figure out why EISDIR is recursive.
If we are trying to create "foo/bar/baz", we can only get EISDIR when
"baz" exists and is a directory. I at first took your recursive to me
that we delete it and "foo/bar" and "foo". Which is just silly and
counterproductive.
But presumably you mean that we delete "foo/bar/baz/xyzzy", etc, up to
"foo/bar/baz", provided they are all empty directories. I think your
comment is probably OK and I was just being thick, but maybe stating it
like:
...removes the directory if it is empty (and recursively any empty
directories it contains) and calls the function again.
would be more clear. That is still leaving the definition of "empty"
implied, but it's hopefully obvious from the context.
> +int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
> +{
> + /*
> + * The number of times we will try to remove empty directories
> + * in the way of path. This is only 1 because if another
> + * process is racily creating directories that conflict with
> + * us, we don't want to fight against them.
> + */
> + int remove_directories_remaining = 1;
> +
> + /*
> + * The number of times that we will try to create the
> + * directories containing path. We are willing to attempt this
> + * more than once, because another process could be trying to
> + * clean up empty directories at the same time as we are
> + * trying to create them.
> + */
> + int create_directories_remaining = 3;
We know that 3 is higher than 1, so we would not fight forever between
writing "foo" and "foo/bar". That made me wonder if we could fight with
other code. The obvious one would be try_remove_empty_parents() in
files-backend.c, but it makes only a single attempt at each directory.
So we should "win" against it short of weird cases (like somebody
running "git pack-refs --prune" in a tight loop).
> [...the actual function logic...]
Nice. This looks very straightforward.
-Peff
^ permalink raw reply
* [PATCH v3 10/23] log_ref_write(): inline function
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
This function doesn't do anything beyond call files_log_ref_write(), so
replace it with the latter at its call sites.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 49a119c..fd8a751 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2832,14 +2832,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
return 0;
}
-static int log_ref_write(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
-{
- return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
- err);
-}
-
int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg,
int flags, struct strbuf *err)
@@ -2903,7 +2895,8 @@ static int commit_ref_update(struct files_ref_store *refs,
assert_main_repository(&refs->base, "commit_ref_update");
clear_loose_ref_cache(refs);
- if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
+ if (files_log_ref_write(lock->ref_name, lock->old_oid.hash, sha1,
+ logmsg, 0, err)) {
char *old_msg = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot update the ref '%s': %s",
lock->ref_name, old_msg);
@@ -2934,7 +2927,7 @@ static int commit_ref_update(struct files_ref_store *refs,
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name)) {
struct strbuf log_err = STRBUF_INIT;
- if (log_ref_write("HEAD", lock->old_oid.hash, sha1,
+ if (files_log_ref_write("HEAD", lock->old_oid.hash, sha1,
logmsg, 0, &log_err)) {
error("%s", log_err.buf);
strbuf_release(&log_err);
@@ -2973,7 +2966,8 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,
struct strbuf err = STRBUF_INIT;
unsigned char new_sha1[20];
if (logmsg && !read_ref(target, new_sha1) &&
- log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
+ files_log_ref_write(refname, lock->old_oid.hash, new_sha1,
+ logmsg, 0, &err)) {
error("%s", err.buf);
strbuf_release(&err);
}
@@ -3748,9 +3742,11 @@ static int files_transaction_commit(struct ref_store *ref_store,
if (update->flags & REF_NEEDS_COMMIT ||
update->flags & REF_LOG_ONLY) {
- if (log_ref_write(lock->ref_name, lock->old_oid.hash,
- update->new_sha1,
- update->msg, update->flags, err)) {
+ if (files_log_ref_write(lock->ref_name,
+ lock->old_oid.hash,
+ update->new_sha1,
+ update->msg, update->flags,
+ err)) {
char *old_msg = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot update the ref '%s': %s",
--
2.9.3
^ permalink raw reply related
* [PATCH v3 09/23] rename_tmp_log(): improve error reporting
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
* Don't capitalize error strings
* Report true paths of affected files
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3f18a01..49a119c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2518,10 +2518,11 @@ static int rename_tmp_log(const char *newrefname)
ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
if (ret) {
if (errno == EISDIR)
- error("Directory not empty: %s", path);
+ error("directory not empty: %s", path);
else
- error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
- newrefname, strerror(true_errno));
+ error("unable to move logfile %s to %s: %s",
+ git_path(TMP_RENAMED_LOG), path,
+ strerror(true_errno));
}
free(path);
--
2.9.3
^ permalink raw reply related
* [PATCH v3 04/23] safe_create_leading_directories(): set errno on SCLD_EXISTS
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
The exit path for SCLD_EXISTS wasn't setting errno, which some callers
use to generate error messages for the user. Fix the problem and
document that the function sets errno correctly to help avoid similar
regressions in the future.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
cache.h | 5 +++--
sha1_file.c | 4 +++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index a50a61a..8177c3a 100644
--- a/cache.h
+++ b/cache.h
@@ -1031,8 +1031,9 @@ int adjust_shared_perm(const char *path);
/*
* Create the directory containing the named path, using care to be
- * somewhat safe against races. Return one of the scld_error values
- * to indicate success/failure.
+ * somewhat safe against races. Return one of the scld_error values to
+ * indicate success/failure. On error, set errno to describe the
+ * problem.
*
* SCLD_VANISHED indicates that one of the ancestor directories of the
* path existed at one point during the function call and then
diff --git a/sha1_file.c b/sha1_file.c
index 10395e7..ae8f0b4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -137,8 +137,10 @@ enum scld_error safe_create_leading_directories(char *path)
*slash = '\0';
if (!stat(path, &st)) {
/* path exists */
- if (!S_ISDIR(st.st_mode))
+ if (!S_ISDIR(st.st_mode)) {
+ errno = ENOTDIR;
ret = SCLD_EXISTS;
+ }
} else if (mkdir(path, 0777)) {
if (errno == EEXIST &&
!stat(path, &st) && S_ISDIR(st.st_mode))
--
2.9.3
^ permalink raw reply related
* [PATCH v3 08/23] rename_tmp_log(): use raceproof_create_file()
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
Besides shortening the code, this saves an unnecessary call to
safe_create_leading_directories_const() in almost all cases.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 73 +++++++++++++++++++++-------------------------------
1 file changed, 30 insertions(+), 43 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 74de289..3f18a01 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,55 +2489,42 @@ static int files_delete_refs(struct ref_store *ref_store,
*/
#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
-static int rename_tmp_log(const char *newrefname)
+static int rename_tmp_log_callback(const char *path, void *cb)
{
- int attempts_remaining = 4;
- struct strbuf path = STRBUF_INIT;
- int ret = -1;
+ int *true_errno = cb;
- retry:
- strbuf_reset(&path);
- strbuf_git_path(&path, "logs/%s", newrefname);
- switch (safe_create_leading_directories_const(path.buf)) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- goto out;
+ if (rename(git_path(TMP_RENAMED_LOG), path)) {
+ /*
+ * rename(a, b) when b is an existing directory ought
+ * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
+ * Sheesh. Record the true errno for error reporting,
+ * but report EISDIR to raceproof_create_file() so
+ * that it knows to retry.
+ */
+ *true_errno = errno;
+ if (errno == ENOTDIR)
+ errno = EISDIR;
+ return -1;
+ } else {
+ return 0;
}
+}
- if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
- /*
- * rename(a, b) when b is an existing
- * directory ought to result in ISDIR, but
- * Solaris 5.8 gives ENOTDIR. Sheesh.
- */
- if (remove_empty_directories(&path)) {
- error("Directory not empty: logs/%s", newrefname);
- goto out;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
- * Maybe another process just deleted one of
- * the directories in the path to newrefname.
- * Try again from the beginning.
- */
- goto retry;
- } else {
+static int rename_tmp_log(const char *newrefname)
+{
+ char *path = git_pathdup("logs/%s", newrefname);
+ int ret, true_errno;
+
+ ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
+ if (ret) {
+ if (errno == EISDIR)
+ error("Directory not empty: %s", path);
+ else
error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
- newrefname, strerror(errno));
- goto out;
- }
+ newrefname, strerror(true_errno));
}
- ret = 0;
-out:
- strbuf_release(&path);
+
+ free(path);
return ret;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
It's unnecessary to pass a strbuf holding the reflog path up and down
the call stack now that it is hardly needed by the callers. Remove the
places where log_ref_write_1() uses it, in preparation for making it
internal to log_ref_setup().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7f26cf8..5a96424 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
git_committer_info(0), msg);
if (result) {
- strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
- strerror(errno));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
close(logfd);
return -1;
}
if (close(logfd)) {
- strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
- strerror(errno));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
return -1;
}
return 0;
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox