* 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
* 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: 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: [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: [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: [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 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 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: 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 11/23] log_ref_setup(): separate code for create vs non-create
From: Junio C Hamano @ 2017-01-01 3:28 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Jeff King, David Turner
In-Reply-To: <d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> + 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 {
It may be valid C, but an
{
/*
* an empty block without any statement,
* not even a null statement.
*/
}
always makes me a bit nervous. Have a line with a semicolon without
anything else (other than the indent) at the end and it will read
nicer, at least to me.
^ permalink raw reply
* Test failures when Git is built with libpcre and grep is built without it
From: A. Wilcox @ 2017-01-01 4:59 UTC (permalink / raw)
To: git
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hello!
I'm attempting to package Git for our new Linux distribution and I
have run in to a failure on our PowerPC builder while running the test
suite.
The PowerPC builder runs a tiny version of grep(1) that was not built
with PCRE. As such, grep -P returns 2 and prints:
grep: support for the -P option is not compiled into this
- --disable-perl-regexp binary
However, our Git build *does* link against libpcre. This causes a
tests numbered 142 and 143 to fail in t7810-grep.sh.
I am not sure the best way to handle this but I felt it would be
prudent to inform you of this issue. I will be happy to provide any
other information you may require.
Best,
arw
- --
A. Wilcox (awilfox)
Project Lead, Adélie Linux
http://adelielinux.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBCAAGBQJYaIycAAoJEMspy1GSK50U/g4P/AtPRfeOv8pENfI/PnlhXqSD
C28CR0Hzv4qBv/Ay8vpBfmkM177bBnra5GTcfnAZ0wvxJUiqnkluu+EJ2UWfPNse
r5ZbDkXCp42oek+vIi7jYi7UsyNqwyT1DziYLR5PDopT3L8YK7i9pZCQZ+mDSmVT
Iu3gYmLvyyEL7r104o6ACRw11P5CDL9bKFmSVacD9UvPL3EjOM9LrSdRDzvJmk0u
nqAOH6yw4wknxCWGPA3JT4DM9GD5DB/Y+UmVQrrpA+Txl8gbPjgzb1PX4qogHjNq
dNDk1jeKJd/CmwJdAU6eKQ3bgfnIXzLbTDVmaMTMlUUsHzlqE/7QYnrae76ECmmn
drmezgKRwTtVgQjQ8l/F/c+EQhux9c0zM9Lz+0Hrd7v1WwohLQDA0xYOE8cCaLw1
8WmXQWjNG1Ih2GpAZH7CRreWyiDAiaAZI2d/zOhsgG9edNEmNn+LkesajVdMywUJ
jKlxmKk6qK2uPDPVrD261ODtMxVdptG6/+m24iz56WRERWkkFBpZz15OtrtlLa2E
0LcFiu2f5lki12XDZsSJwkLeGfb6KyxfCPKtGP7TAVAYO43ORQHKhks8pheOMwJa
dpL4+72CzkI57LyCtl0NiSMZH2zKUvxp8OqKgE/gpDwbhwh8spAHdUaiaS6emiPj
MM6jb7ru9eo4gD6seRCp
=R9eQ
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Jeff King @ 2017-01-01 5:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, David Turner
In-Reply-To: <xmqqvatz4imu.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:
> 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.
Hmm, yeah. This is called when we are deleting a ref, and I thought we
explicitly _didn't_ do check_refname_format() when deleting, so that
funny-named refs could be deleted. It's only is_refname_safe() that we
must pass.
So I have no idea if that's a problem in the code or not, but it is at
least not immediately obvious who is responsible for calling
check_refname_format() here.
-Peff
^ permalink raw reply
* Re: [PATCH v3 10/23] log_ref_write(): inline function
From: Michael Haggerty @ 2017-01-01 8:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner
In-Reply-To: <xmqqzijb4jky.fsf@gitster.mtv.corp.google.com>
On 01/01/2017 03:09 AM, Junio C Hamano wrote:
> 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.
Thanks; will fix.
Michael
^ permalink raw reply
* Re: Rebasing multiple branches at once
From: Johannes Sixt @ 2017-01-01 8:42 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, Johannes Schindelin
In-Reply-To: <20161231081433.3zo6lrsjsu2qho4u@glandium.org>
Am 31.12.2016 um 09:14 schrieb Mike Hommey:
> 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.
Git garden shears, perhaps?
https://public-inbox.org/git/alpine.DEB.2.20.1609111027330.129229@virtualbox/
-- Hannes
^ permalink raw reply
* Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create
From: Michael Haggerty @ 2017-01-01 8:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner
In-Reply-To: <xmqqful34fww.fsf@gitster.mtv.corp.google.com>
On 01/01/2017 04:28 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> + 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 {
>
> It may be valid C, but an
>
> {
> /*
> * an empty block without any statement,
> * not even a null statement.
> */
> }
>
> always makes me a bit nervous. Have a line with a semicolon without
> anything else (other than the indent) at the end and it will read
> nicer, at least to me.
That's a form of superstition that I haven't encountered before ;-) but
I'll change it.
Michael
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Jacob Keller @ 2017-01-01 9:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Michael Haggerty, Git mailing list, David Turner
In-Reply-To: <xmqqr34n4ii8.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
I do have one comment regarding this series. Is it ever possible for
an older version of git to be running a process while a new version of
git which cleans up dirs runs? Is this expected? I just want to make
sure we don't need to worry about that scenario since otherwise it
makes it much more challenge.
My thought as far as I understand it is that it is possible, because a
user COULD choose to run both this and an older version, but that it
is unlikely in practice outside of a few developer boxes who
periodically switch between versions of git, and are unlikely to
actually run multiple versions at exactly the same time.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Jacob Keller @ 2017-01-01 9:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Michael Haggerty, Git mailing list, David Turner
In-Reply-To: <CA+P7+xqqVFvDKSCTrGVVdpZB_VHwGdZ3gFQzo+RQqCu0FpOsAQ@mail.gmail.com>
On Sun, Jan 1, 2017 at 1:24 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> I do have one comment regarding this series. Is it ever possible for
> an older version of git to be running a process while a new version of
> git which cleans up dirs runs? Is this expected? I just want to make
> sure we don't need to worry about that scenario since otherwise it
> makes it much more challenge.
>
> My thought as far as I understand it is that it is possible, because a
> user COULD choose to run both this and an older version, but that it
> is unlikely in practice outside of a few developer boxes who
> periodically switch between versions of git, and are unlikely to
> actually run multiple versions at exactly the same time.
>
> Thanks,
> Jake
To add to this, if it is possible, it might be worth merging the "make
ourselves safer against a race" first, and then waiting some time
before merging the "we are now safe to delete directories". I am not
yet convinced that it is necessary, but wanted to point it out so that
someone more knowledgeable could explain why it is safe to do so.
Regards,
Jake
^ permalink raw reply
* Re: Test failures when Git is built with libpcre and grep is built without it
From: Torsten Bögershausen @ 2017-01-01 10:19 UTC (permalink / raw)
To: A. Wilcox, git
In-Reply-To: <58688C9F.4000605@adelielinux.org>
On 01.01.17 05:59, A. Wilcox wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Hello!
>
> I'm attempting to package Git for our new Linux distribution and I
> have run in to a failure on our PowerPC builder while running the test
> suite.
>
> The PowerPC builder runs a tiny version of grep(1) that was not built
> with PCRE. As such, grep -P returns 2 and prints:
>
> grep: support for the -P option is not compiled into this
> - --disable-perl-regexp binary
>
> However, our Git build *does* link against libpcre. This causes a
> tests numbered 142 and 143 to fail in t7810-grep.sh.
>
> I am not sure the best way to handle this but I felt it would be
> prudent to inform you of this issue. I will be happy to provide any
> other information you may require.
The first thing you can do is to run the test with debug and verbose:
debug=t verbose=t ./t7810-grep.sh
and post the output of these 2 test cases here:
(mine looks like this)
expecting success:
echo "ab:a+bc" >expected &&
git \
-c grep.patterntype=extended \
-c grep.patterntype=fixed \
-c grep.patterntype=basic \
grep "a+b*c" ab >actual &&
test_cmp expected actual
ok 142 - grep pattern with grep.patternType=extended, =fixed, =basic
expecting success:
echo "ab:abc" >expected &&
git grep -F -G -E "a+b*c" ab >actual &&
test_cmp expected actual
ok 143 - grep -F -G -E pattern
^ permalink raw reply
* Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Junio C Hamano @ 2017-01-01 10:36 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, git, David Turner
In-Reply-To: <20161231175808.cvm54nmk3x7zoipo@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
FWIW, in my mental model, 2. is not a failure ("we returned without
creating new log because that is what was asked by the user"). A
true failure case is "we wanted to open but couldn't".
The caller needs to be able to differentiate between these two cases
because we get no usable fd out of the function in either case.
I agree that we could cram the error status and the file descriptor
into a single return value as you two discussed, but I think what
the patch chose to do is easier to use from the callers' point of
view. The caller can switch between the codepath to give an error
message and the non-error codepath based on the return value, and in
the non-error codepath can choose what to do based on the value of
logfd.
I do not mind "all negative values mean there is no fd" plus "some
negative values are more special than others" convention, and if a
patch did that from the beginning, I certainly would not suggest to
rewrite it to use the "error status comes as a return value, file
descriptor is an out parameter" convention; i.e. I personally do not
see much difference either way, so...
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Philip Oakley @ 2017-01-01 12:43 UTC (permalink / raw)
To: Jacob Keller, Junio C Hamano
Cc: Jeff King, Michael Haggerty, Git mailing list, David Turner
In-Reply-To: <CA+P7+xqqVFvDKSCTrGVVdpZB_VHwGdZ3gFQzo+RQqCu0FpOsAQ@mail.gmail.com>
From: "Jacob Keller" <jacob.keller@gmail.com>
Sent: Sunday, January 01, 2017 9:24 AM
> On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> I do have one comment regarding this series. Is it ever possible for
> an older version of git to be running a process while a new version of
> git which cleans up dirs runs? Is this expected? I just want to make
> sure we don't need to worry about that scenario since otherwise it
> makes it much more challenge.
It is easily possible in the Windows environment where the install philosphy
is different, and some external vendor tools may even bring in their own
copy of Git as well. There is also the Portable Git version, so the
possibility of multiple versions running concurrently is there, though it is
on Windows...
I certainly have a Git-for-Windows published version, and a recent SDK
version on my home machines.
>
> My thought as far as I understand it is that it is possible, because a
> user COULD choose to run both this and an older version, but that it
> is unlikely in practice outside of a few developer boxes who
> periodically switch between versions of git, and are unlikely to
> actually run multiple versions at exactly the same time.
>
> Thanks,
> Jake
>
Philip
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Luke Diamand @ 2017-01-01 14:23 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git Users, Stefan Beller
In-Reply-To: <20161231114412.23439-1-pranit.bauva@gmail.com>
On 31 December 2016 at 11:44, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> 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" &&
Would it be clearer to use this?
git p4 commit |\
grep -q -v "git author.*does not match" &&
With your original change, I think that if "git p4 commit" fails, then
that expression will be treated as a pass. What we want is for "git p4
commit" to pass, but the string to be missing.
(I would have used "--invert-match" rather than "-v", but it seems
that's not supported on Solaris).
Luke
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Johannes Sixt @ 2017-01-01 14:50 UTC (permalink / raw)
To: Luke Diamand; +Cc: Pranit Bauva, Git Users, Stefan Beller
In-Reply-To: <CAE5ih7-7e+ZLUbE7iquWV2=qP4ofzAHUC2ZPg3b-ivSpCo4eRw@mail.gmail.com>
Am 01.01.2017 um 15:23 schrieb Luke Diamand:
> On 31 December 2016 at 11:44, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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" &&
>
> Would it be clearer to use this?
>
> git p4 commit |\
> grep -q -v "git author.*does not match" &&
>
> With your original change, I think that if "git p4 commit" fails, then
> that expression will be treated as a pass.
No. The exit code of the upstream in a pipe is ignored. For this reason,
having a git invocation as the upstream of a pipe *anywhere* in the test
suite is frowned upon. Hence, a better rewrite would be
git p4 commit >actual &&
! grep "git author.*does not match" actual &&
which makes me wonder: Is the message that we do expect not to occur
actually printed on stdout? It sounds much more like an error message,
i.e., text that is printed on stderr. Wouldn't we need this?
git p4 commit >actual 2>&1 &&
! grep "git author.*does not match" actual &&
-- Hannes
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Luke Diamand @ 2017-01-01 15:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Pranit Bauva, Git Users, Stefan Beller
In-Reply-To: <285ed013-5c59-0b98-7dc0-8f729587a313@kdbg.org>
On 1 January 2017 at 14:50, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 01.01.2017 um 15:23 schrieb Luke Diamand:
>>
>> On 31 December 2016 at 11:44, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>
>>> 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" &&
>>
>>
>> Would it be clearer to use this?
>>
>> git p4 commit |\
>> grep -q -v "git author.*does not match" &&
>>
>> With your original change, I think that if "git p4 commit" fails, then
>> that expression will be treated as a pass.
>
>
> No. The exit code of the upstream in a pipe is ignored. For this reason,
> having a git invocation as the upstream of a pipe *anywhere* in the test
> suite is frowned upon. Hence, a better rewrite would be
>
> git p4 commit >actual &&
> ! grep "git author.*does not match" actual &&
>
> which makes me wonder: Is the message that we do expect not to occur
> actually printed on stdout? It sounds much more like an error message, i.e.,
> text that is printed on stderr. Wouldn't we need this?
>
> git p4 commit >actual 2>&1 &&
> ! grep "git author.*does not match" actual &&
The message is actually part of a template presented to the user via
their chosen editor. For this test, we set the editor to be "cat", so
it comes out on stdout.
Your first suggestion would therefore be fine (and similarly for the
other cases).
^ permalink raw reply
* Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Stephan Beyer @ 2017-01-01 16:27 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPtF5P5nGp+=btHtwNm1unTJ7qo1khJHCqLvNn1=RYAUQ@mail.gmail.com>
Hi Pranit,
On 12/31/2016 11:43 AM, Pranit Bauva wrote:
>>> +
>>> +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.
The invoked bisect_next shell code called "exit", right... you had to
replace this by passing return values. I get it. Thank you!
>>> 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".
"checkout the next bisection commit" maybe?
Thanks,
Stephan
^ permalink raw reply
* [PATCH] read-cache: mark a file-local symbol with static
From: Ramsay Jones @ 2017-01-01 17:10 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Christian,
If you need to re-roll your 'cc/split-index-config' branch, could
you please squash this into the relevant patch (commit 8a7e3ef9a6,
"read-cache: touch shared index files when used", 26-12-2016).
Thanks!
ATB,
Ramsay Jones
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 119701bf0..17b9f2461 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1682,7 +1682,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
* for some time. It's ok to fail to update the mtime if we are on a
* read only file system.
*/
-void freshen_shared_index(char *base_sha1_hex)
+static void freshen_shared_index(char *base_sha1_hex)
{
const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
check_and_freshen_file(shared_index, 1);
--
2.11.0
^ 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