* Re: Rebasing multiple branches at once
From: Junio C Hamano @ 2017-01-01 2:40 UTC (permalink / raw)
To: Mike Hommey; +Cc: git
In-Reply-To: <20161231081433.3zo6lrsjsu2qho4u@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> 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'
>
What people seem to do is to teach the branch that ends with F that
its upstream is the local branch that ends with E, so that they can
be lazy when rebasing a branch that knows its upstream. I suspect
that you would end up with
A---G---B'--C'--D'--E'--F'
instead if it is done naively, but if you really care that the
branch that ends with F does not have E, you presumably want to have
the branch that ends at D its own identity, so
(1) 'master' or whatever that used to end at A and now its tip is
at G;
(2) the branch that ends at D whose upstream is 'master';
(3) the branch that ends at E whose upstream is (2); and
(4) the branch that ends at F whose upstream is (2).
I personally do not do that, though, because you'd need to remember
the order in which these three branches must be rebased (i.e. (2)
must be done first before rebasing (3) and (4) in any order).
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Junio C Hamano @ 2017-01-01 2:32 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, git, David Turner
In-Reply-To: <20161231064746.6bvis76p5x5ubc2b@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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
Thanks, both. These patches indeed were pleasant.
^ permalink raw reply
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Junio C Hamano @ 2017-01-01 2:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Jeff King, David Turner
In-Reply-To: <c7a89febcbf7bdffb44f8fdf63a43f11339a0289.1483153436.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> "refname" has already been checked by check_refname_format(), so it
> cannot have consecutive slashes.
In the endgame state, this has two callers. Both use what came in
the transaction->updates[] array. Presumably "has already been
checked by check_refname_format()" says that whoever created entries
in that array must have called the function, but it would be helpful
to be more explicit here.
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs/files-backend.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index af5a0e2..397488e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2294,15 +2294,14 @@ static void try_remove_empty_parents(const char *refname)
> for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
> while (*p && *p != '/')
> p++;
> - /* tolerate duplicate slashes; see check_refname_format() */
> - while (*p == '/')
> + if (*p == '/')
> p++;
> }
> q = buf.buf + buf.len;
> while (1) {
> while (q > p && *q != '/')
> q--;
> - while (q > p && *(q-1) == '/')
> + if (q > p && *(q-1) == '/')
> q--;
> if (q == p)
> break;
^ permalink raw reply
* Re: [PATCH v3 10/23] log_ref_write(): inline function
From: Junio C Hamano @ 2017-01-01 2:09 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Jeff King, David Turner
In-Reply-To: <dba3d081c32854593d8113f9cd604a9891748bcd.1483153436.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This function doesn't do anything beyond call files_log_ref_write(), so
s/call/&ing/; I think.
> 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",
^ permalink raw reply
* Re: [PATCH v3 05/23] raceproof_create_file(): new function
From: Junio C Hamano @ 2017-01-01 2:07 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Jeff King, git, David Turner
In-Reply-To: <241de54c-63ee-d13c-c9fe-8b420871ac51@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 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!
Thanks. Will tweak it in while queuing.
^ permalink raw reply
* Re: [PATCHv2] unpack-trees: move checkout state into check_updates
From: Junio C Hamano @ 2017-01-01 1:26 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, l.s.r
In-Reply-To: <20161229194309.2373-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The checkout state was introduced via 16da134b1f9
> (read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
> refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
> checkout state explicitly to check_updates(), 2016-09-13), but we can
> go even further.
>
> The `struct checkout state` is not used in unpack_trees apart from
> initializing it, so move it into the function that makes use of it,
> which is `check_updates`.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
I'd add René's Reviewed-by: here.
> -static int check_updates(struct unpack_trees_options *o,
> - const struct checkout *state)
> +static int check_updates(struct unpack_trees_options *o)
> {
> unsigned cnt = 0, total = 0;
> struct progress *progress = NULL;
> struct index_state *index = &o->result;
> - int i;
> - int errs = 0;
> + struct checkout state = CHECKOUT_INIT;
> + int i, errs = 0;
> +
> + state.force = 1;
> + state.quiet = 1;
> + state.refresh_cache = 1;
> + state.istate = index;
I think moving heavier and initialized variables earlier and more
lightweight and ephemeral ones like "i" later does make it easier to
follow. "errs" has the significance and the lifetime similar to
cnt/total, and logically should be higher, though. It is not a big
enough deal to reroll (but as your futzing of the variable definition
order was not a big enough deal to do in this patch, either, so...).
Queued. Thanks.
^ permalink raw reply
* Re: Counter-intuitive result from diff -C --stat
From: Junio C Hamano @ 2017-01-01 1:15 UTC (permalink / raw)
To: Mike Hommey; +Cc: git
In-Reply-To: <20161228080916.72id4hzqxfbygtth@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> 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.
Intuition is in the eyes of observer.
A pairing of the original and the result you saw might be not very
useful (which I have no opinion on), but in the context of the
chosen pairing of the original and the result, in order to produce
the final result, you started from a copy of the original and
removed quite a lot while adding just a bit, so what you saw was an
outcome that was deliberately designed.
^ permalink raw reply
* Re: [PATCH] pathspec: give better message for submodule related pathspec error
From: Junio C Hamano @ 2017-01-01 1:11 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20161229192908.32633-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1].
>
> The usual response from the mailing list is link to old discussions[2],
> and acknowledging the problem stating it is known.
>
> For now just improve the user visible error message.
Thans. judging from the date: header I take this is meant as v3 that
supersedes v2 done on Wednesday.
It is not clear in the above that what this thing is. Given that we
are de-asserting it, is the early part of the new code diagnosing an
end-user error (i.e. you gave me a pathspec but that extends into a
submodule which is a no-no)? The "was a submodule issue" comment
added is "this is an end-user mistake, there is nothing to fix in
the code"?
I take that the new "BUG" thing tells the Git developers that no
sane codepath should throw an pathspec_item that satisfies the
condition of the if() statement for non-submodules?
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..b446d79615 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> }
>
> /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> - item->prefix <= item->len);
> + if (item->nowildcard_len > item->len ||
> + item->prefix > item->len) {
> + /* Historically this always was a submodule issue */
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> + int ce_len = ce_namelen(ce);
> + int len = ce_len < item->len ? ce_len : item->len;
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
Computation of ce_len and len are better done after this check, no?
> + if (!memcmp(ce->name, item->match, len))
> + die (_("Pathspec '%s' is in submodule '%.*s'"),
> + item->original, ce_len, ce->name);
> + }
> + /* The error is a new unknown bug */
> + die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
> + }
> +
> return magic;
> }
>
> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..e62dbb7327
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> + test_commit 1 &&
> + git submodule add ./ sub &&
Is this adding our own project as its submodule?
It MIGHT be a handy hack when writing a test, but let's stop doing
that insanity. No sane project does that in real life, doesn't it?
Create a subdirectory, make it a repository, have a commit there and
bind that as our own submodule. That would be a more normal way to
start your own superproject and its submodule pair if they originate
together at the same place.
Better yet create a separate repository, have a commit there, and
then pull it in with "git submodule add && git submodule init" into
our repository. That would be the normal way to borrow somebody
else's project as a part of your own superproject.
> + git commit -a -m "add submodule" &&
> + git submodule deinit --all
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec 'sub/a' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule' '
> + echo a >sub/a &&
> + test_must_fail git add sub/a 2>actual &&
> + test_cmp expect actual
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec '.' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule from within submodule' '
> + test_must_fail git -C sub add . 2>actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
^ permalink raw reply
* 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
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