* Re: "IMAP IDLE"-like long-polling "git fetch"
From: Eric Wong @ 2018-12-29 6:13 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: git, meta
In-Reply-To: <20181229043858.GA28509@pure.paranoia.local>
Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Sat, Dec 29, 2018 at 03:56:21AM +0000, Eric Wong wrote:
> > Hey all, I just added this to the TODO file for public-inbox[1] but
> > obviously it's intended for git.git (meta@public-inbox cc-ed):
> >
> > > +* Contribute something like IMAP IDLE for "git fetch".
> > > + Inboxes (and any git repos) can be kept up-to-date without
> > > + relying on polling.
> >
> > I would've thought somebody had done this by now, but I guess
> > it's dependent on a bunch of things (TLS layer nowadays, maybe
> > HTTP/2), so git-daemon support alone wouldn't cut it...
>
> Polling is not all bad, especially for large repository collections. I'm
> not sure you want to "idle" individual repositories when there's
> thousands of them. We ended up writing grokmirror for replicating
> repo collections using manifest files.
I wasn't intending it for giant sites like korg, but for
individual hackers on their workstations tracking a handful of
projects they follow.
The cost for a hackers' machine would be the same as the current
situation where developers idle on IRC channels for the projects
they're involved in.
> > Anyways, until this is implemented, feel free to continue
> > hammering a way on https://public-inbox.org/git/ with frequent
> > "git fetch". I write C10K servers in my sleep -_-
>
> The archive is also mirrored at
> https://git.kernel.org/pub/scm/public-inbox/vger.kernel.org/git.git, and
> also on kernel.googlesource.com.
Now, I'm wondering if you can make a v2 public-inbox mirror of
git@vger and run it on lore. Converting public-inbox.org/git to
v2 would break things for everybody fetching, right now :<
^ permalink raw reply
* [PATCH v3] sha1-name.c: Fix handling of revisions that contain paths with brackets
From: Stan Hu @ 2018-12-29 5:43 UTC (permalink / raw)
To: git, stanhu
In-Reply-To: <20181224080651.GA12708@duynguyen.home>
Previously, calling ls-tree with a revision such as
`master^{tree}:foo/{{path}}` would show the root tree instead of the
correct tree pointed by foo/{{path}}. We improve this by ensuring
peel_onion() consumes all "len" characters or none.
Note that it's also possible to have directories named ^{tree} and other
patterns that look like type restrictions. To handle those cases, scan
from the beginning of the name instead of the end.
Signed-off-by: Stan Hu <stanhu@gmail.com>
---
sha1-name.c | 35 ++++++++++--------
t/t3104-ls-tree-braces.sh | 75 +++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+), 14 deletions(-)
create mode 100755 t/t3104-ls-tree-braces.sh
diff --git a/sha1-name.c b/sha1-name.c
index faa60f69e3..ab372a90be 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -989,7 +989,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
unsigned lookup_flags)
{
struct object_id outer;
- const char *sp;
+ const char *sp, *end;
unsigned int expected_type = 0;
struct object *o;
@@ -1001,33 +1001,40 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
* "ref^{commit}". "commit^{tree}" could be used to find the
* top-level tree of the given commit.
*/
- if (len < 4 || name[len-1] != '}')
+ if (len < 4)
return -1;
- for (sp = name + len - 1; name <= sp; sp--) {
+ for (sp = name; sp < name + len; sp++) {
int ch = *sp;
- if (ch == '{' && name < sp && sp[-1] == '^')
+ if (ch == '^' && sp < name + len && sp[1] == '{')
break;
}
- if (sp <= name)
+
+ if (sp == name + len)
return -1;
- sp++; /* beginning of type name, or closing brace for empty */
- if (starts_with(sp, "commit}"))
+ sp += 2; /* beginning of type name, or closing brace for empty */
+
+ if (skip_prefix(sp, "commit}", &end))
expected_type = OBJ_COMMIT;
- else if (starts_with(sp, "tag}"))
+ else if (skip_prefix(sp, "tag}", &end))
expected_type = OBJ_TAG;
- else if (starts_with(sp, "tree}"))
+ else if (skip_prefix(sp, "tree}", &end))
expected_type = OBJ_TREE;
- else if (starts_with(sp, "blob}"))
+ else if (skip_prefix(sp, "blob}", &end))
expected_type = OBJ_BLOB;
- else if (starts_with(sp, "object}"))
+ else if (skip_prefix(sp, "object}", &end))
expected_type = OBJ_ANY;
- else if (sp[0] == '}')
+ else if (sp[0] == '}') {
expected_type = OBJ_NONE;
- else if (sp[0] == '/')
+ end = sp + 1;
+ } else if (sp[0] == '/') {
expected_type = OBJ_COMMIT;
- else
+ end = name + len;
+ } else
+ return -1;
+
+ if (end != name + len)
return -1;
lookup_flags &= ~GET_OID_DISAMBIGUATORS;
diff --git a/t/t3104-ls-tree-braces.sh b/t/t3104-ls-tree-braces.sh
new file mode 100755
index 0000000000..adeed5bb49
--- /dev/null
+++ b/t/t3104-ls-tree-braces.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+
+test_description='ls-tree with folders with brackets'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ mkdir -p "newdir/{{curly}}" &&
+ mkdir -p "newdir/^{tree}" &&
+ touch "newdir/{{curly}}/one" &&
+ touch "newdir/^{tree}/two" &&
+ git add "newdir/{{curly}}/one" &&
+ git add "newdir/^{tree}/two" &&
+ git commit -m "Test message: search me"
+'
+
+test_expect_success 'ls-tree with curly brace folder' '
+ cat >expect <<-EOF &&
+ 100644 blob $EMPTY_BLOB one
+ EOF
+ git ls-tree -r "HEAD:newdir/{{curly}}" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-tree with type restriction and curly brace folder' '
+ cat >expect <<-EOF &&
+ 100644 blob $EMPTY_BLOB one
+ EOF
+ git ls-tree "HEAD^{tree}:newdir/{{curly}}" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-tree with type restriction and folder named ^{tree}' '
+ cat >expect <<-EOF &&
+ 100644 blob $EMPTY_BLOB two
+ EOF
+ git ls-tree "HEAD^{tree}:newdir/^{tree}" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-tree with unknown type restriction' '
+ (git ls-tree HEAD^{foobar} >actual || true) &&
+ test_must_be_empty actual
+'
+
+test_output () {
+ sed -e "s/ $OID_REGEX / X /" <actual >check
+ test_cmp expect check
+}
+
+test_expect_success 'ls-tree with regex' '
+ cat >expect <<-EOF &&
+ 040000 tree X newdir
+ EOF
+ git ls-tree "HEAD^{/message}" >actual &&
+ test_output
+'
+
+test_expect_success 'ls-tree with regex with a colon' '
+ cat >expect <<-EOF &&
+ 040000 tree X newdir
+ EOF
+ git ls-tree "HEAD^{/message: search}" >actual &&
+ test_output
+'
+
+test_expect_success 'ls-tree with regex and curly brace folder' '
+ cat >expect <<-EOF &&
+ 100644 blob $EMPTY_BLOB one
+ EOF
+ git ls-tree "HEAD^{/message: search}:newdir/{{curly}}" >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 1/2] t5403: Refactor
From: Ramsay Jones @ 2018-12-29 3:06 UTC (permalink / raw)
To: Junio C Hamano, orgads; +Cc: git
In-Reply-To: <xmqqmuopl1qz.fsf@gitster-ct.c.googlers.com>
On 28/12/2018 22:34, Junio C Hamano wrote:
> orgads@gmail.com writes:
>
>> Subject: Re: [PATCH 1/2] t5403: Refactor
>
[snip]
>> if test "$(git config --bool core.filemode)" = true; then
>
> This is a tangent but this conditional came from an ancient d42ec126
> ("disable post-checkout test on Cygwin", 2009-03-17) that says
>
> disable post-checkout test on Cygwin
>
> It is broken because of the tricks we have to play with
> lstat to get the bearable perfomance out of the call.
> Sadly, it disables access to Cygwin's executable attribute,
> which Windows filesystems do not have at all.
>
> I wonder if this is still relevant these days (Cc'ed Ramsay for
> input).
Ah, no, the 'tricks we have to play with lstat' mentioned in that
commit message are long gone! ;-) If you remove that conditional,
then the test passes just fine.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: "IMAP IDLE"-like long-polling "git fetch"
From: Konstantin Ryabitsev @ 2018-12-29 4:38 UTC (permalink / raw)
To: Eric Wong; +Cc: git, meta
In-Reply-To: <20181229035621.cwjpknctq3rjnlhs@dcvr>
On Sat, Dec 29, 2018 at 03:56:21AM +0000, Eric Wong wrote:
> Hey all, I just added this to the TODO file for public-inbox[1] but
> obviously it's intended for git.git (meta@public-inbox cc-ed):
>
> > +* Contribute something like IMAP IDLE for "git fetch".
> > + Inboxes (and any git repos) can be kept up-to-date without
> > + relying on polling.
>
> I would've thought somebody had done this by now, but I guess
> it's dependent on a bunch of things (TLS layer nowadays, maybe
> HTTP/2), so git-daemon support alone wouldn't cut it...
Polling is not all bad, especially for large repository collections. I'm
not sure you want to "idle" individual repositories when there's
thousands of them. We ended up writing grokmirror for replicating
repo collections using manifest files.
> Anyways, until this is implemented, feel free to continue
> hammering a way on https://public-inbox.org/git/ with frequent
> "git fetch". I write C10K servers in my sleep -_-
The archive is also mirrored at
https://git.kernel.org/pub/scm/public-inbox/vger.kernel.org/git.git, and
also on kernel.googlesource.com.
-K
^ permalink raw reply
* "IMAP IDLE"-like long-polling "git fetch"
From: Eric Wong @ 2018-12-29 3:56 UTC (permalink / raw)
To: git; +Cc: meta
In-Reply-To: <20181229034342.11543-1-e@80x24.org>
Hey all, I just added this to the TODO file for public-inbox[1] but
obviously it's intended for git.git (meta@public-inbox cc-ed):
> +* Contribute something like IMAP IDLE for "git fetch".
> + Inboxes (and any git repos) can be kept up-to-date without
> + relying on polling.
I would've thought somebody had done this by now, but I guess
it's dependent on a bunch of things (TLS layer nowadays, maybe
HTTP/2), so git-daemon support alone wouldn't cut it...
Anyways, until this is implemented, feel free to continue
hammering a way on https://public-inbox.org/git/ with frequent
"git fetch". I write C10K servers in my sleep -_-
[1] https://public-inbox.org/meta/20181229034342.11543-1-e@80x24.org/
^ permalink raw reply
* Re: a git svn bug
From: Eric Wong @ 2018-12-29 4:16 UTC (permalink / raw)
To: 肖建晶; +Cc: git
In-Reply-To: <650358a3.8395.167f09e21b6.Coremail.06271023@bjtu.edu.cn>
肖建晶 <06271023@bjtu.edu.cn> wrote:
> hi,
> git developers. I found a bug when i want to convert webkit to git
> there are some branch named safari... in webkit svn repo.
> when i want to convert them to branch in git. a problem happen.
>
> if you want to reproduce the problem, just follow the guide below.
> 1. git svn clone -s https://svn.webkit.org/......
Do you mean https://svn.webkit.org/repository/webkit ?
> 2. when it runs to about r13800, stop it and rerun the above command
So running "clone" again? Normally, I'd run "git svn fetch" if
I stopped and want to resume (or my Internet connection drops,
which happens a lot).
> 3. the git client will check the refs it already found, and
> if it met a ~ in the branch name. it will failed to
> proceed. error is git thinks it an invalid ref name
>j
> i digged into it, and found a solution in
> git/perl/git/svn.pm. I mod this file and bypass the
> problem.
Can you show us what you did to perl/Git/SVN.pm?
the "refname" sub in perl/Git/SVN.pm already escapes "~",
it seems.
> plean be kind to fix the problem and remind me the fix
> commit, and i will update to a new version.
We'll try, but I think we need more information. Thanks.
^ permalink raw reply
* [PATCH v4 1/4] transport-helper: use xread instead of read
From: randall.s.becker @ 2018-12-28 23:35 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
From: "Randall S. Becker" <rsbecker@nexbridge.com>
This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than
BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
was the only place outside of wrapper.c where it is used instead of xread.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index bf225c698f..a290695a12 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
return 0; /* No space for more. */
transfer_debug("%s is readable", t->src_name);
- bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
+ bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
- if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
- errno != EINTR) {
+ if (bytes < 0 && errno != EINTR) {
error_errno(_("read(%s) failed"), t->src_name);
--
2.17.0.10.gb132f7033
^ permalink raw reply related
* Re: [PATCH] completion: escape metacharacters when completing paths
From: Junio C Hamano @ 2018-12-28 23:25 UTC (permalink / raw)
To: Chayoung You; +Cc: git, SZEDER Gábor
In-Reply-To: <20181226160835.66342-1-yousbe@gmail.com>
Chayoung You <yousbe@gmail.com> writes:
> Subject: Re: [PATCH] completion: escape metacharacters when completing paths
I am not a zsh user, but from the patch text I am getting the
impression that this patch is only about zsh. If so, please help
readers of "git shortlog" by saying so in the title.
Subject: zsh: complete unquoted paths with spaces correctly
or something like that, perhaps?
> Let's say there is a file named 'foo bar.txt' in repository, but it's
> not yet added to the repository. Then the following command triggers a
> completion:
>
> git add fo<Tab>
> git add 'fo<Tab>
> git add "fo<Tab>
>
> The completion results in bash:
>
> git add foo\ bar.txt
> git add 'foo bar.txt'
> git add "foo bar.txt"
>
> While them in zsh:
>
> git add foo bar.txt
> git add 'foo bar.txt'
> git add "foo bar.txt"
You leave it unsaid what you think is wrong, and what you think
should be the right output, before saying "cause of this is...".
I do not think it is given that "git add fo<TAB>" should be
completed to "git add foo\ bar.txt". It would also be OK if the
completion produced "git add 'foo bar.txt'", for example. So what
your ideal output is is rather important, and by doing that you end
up with saying what problem you have with the current output is.
So, say something here, perhaps like...
The first one, where the pathname is not enclosed in quotes,
should escape the SP with a backslash, just like bash
completion does.
^ permalink raw reply
* Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout
From: Junio C Hamano @ 2018-12-28 22:53 UTC (permalink / raw)
To: orgads; +Cc: git
In-Reply-To: <20181224212425.16596-3-orgads@gmail.com>
orgads@gmail.com writes:
> From: Orgad Shaneh <orgads@gmail.com>
> Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout
There is no explanation here?
Is this a regression fix (i.e. scripted version of "rebase" used to
run the hook)? Or a new feature (i.e. no earlier version of
"rebase" run the hook but you think it ought to run it)?
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
> builtin/rebase.c | 11 +++++++++--
> t/t5403-post-checkout-hook.sh | 20 ++++++++++++++++++++
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b5c99ec10c..7f7a2c738e 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)
>
> #define RESET_HEAD_DETACH (1<<0)
> #define RESET_HEAD_HARD (1<<1)
> +#define RESET_HEAD_RUN_HOOK (1<<2)
Would it be plausible that the only possible hook that can be run by
reset_head() function will always be post-checkout and nothing else,
I wonder? Shouldn't this bit be called *_RUN_POST_CHECKOUT to make
sure it is specific enough?
> @@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,
> {
> unsigned detach_head = flags & RESET_HEAD_DETACH;
> unsigned reset_hard = flags & RESET_HEAD_HARD;
> + unsigned run_hook = flags & RESET_HEAD_RUN_HOOK;
> struct object_id head_oid;
> struct tree_desc desc[2] = { { NULL }, { NULL } };
> struct lock_file lock = LOCK_INIT;
> @@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,
> ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
> UPDATE_REFS_MSG_ON_ERR);
> }
> + if (run_hook)
> + run_hook_le(NULL, "post-checkout",
> + oid_to_hex(orig ? orig : &null_oid),
> + oid_to_hex(oid), "1", NULL);
>
This function is never about checking out paths from tree-ish/index
and is always about checking out a branch, so hardcoded "1" is
justified here. Good.
> @@ -1465,7 +1471,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> options.switch_to);
> if (reset_head(&oid, "checkout",
> - options.head_name, 0,
> + options.head_name,
> + RESET_HEAD_RUN_HOOK,
> NULL, buf.buf) < 0) {
> ret = !!error(_("could not switch to "
> "%s"),
> @@ -1539,7 +1546,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> strbuf_addf(&msg, "%s: checkout %s",
> getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
> if (reset_head(&options.onto->object.oid, "checkout", NULL,
> - RESET_HEAD_DETACH, NULL, msg.buf))
> + RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))
> die(_("Could not detach HEAD"));
> strbuf_release(&msg);
So... among 6 callers of reset_head(), these two whose *action is
"checkout" gets the RUN_HOOK bit. Makes sense.
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 9f9a5163c5..5b4e582caa 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -13,6 +13,8 @@ test_expect_success setup '
> EOF
> test_commit one &&
> test_commit two &&
> + test_commit rebase-on-me &&
> + git reset --hard HEAD^ &&
> test_commit three three
> '
>
> @@ -51,6 +53,24 @@ test_expect_success 'post-checkout receives the right args when not switching br
> rm -f .git/post-checkout.args
> '
>
> +test_expect_success 'post-checkout is triggered on rebase' '
> + git checkout -b rebase-test master &&
> + rm -f .git/post-checkout.args &&
Read the title of this whole test script file; it should verify what
is in the file before removing it.
> + git rebase rebase-on-me &&
> + read old new flag < .git/post-checkout.args &&
No SP between "<" and ".git/post-checkout.args".
> + test $old != $new && test $flag = 1 &&
> + rm -f .git/post-checkout.args
> +'
Regarding the clean-up of this file, see my review on the previous
one.
> +test_expect_success 'post-checkout is triggered on rebase with fast-forward' '
The same comment as above applies to this.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] t5403: Refactor
From: Junio C Hamano @ 2018-12-28 22:34 UTC (permalink / raw)
To: orgads; +Cc: git, Ramsay Jones
In-Reply-To: <20181224212425.16596-2-orgads@gmail.com>
orgads@gmail.com writes:
> Subject: Re: [PATCH 1/2] t5403: Refactor
Hmph. "Refactor" alone leaves readers wondering "refactor what?",
"refactor for what?" and "refactor how?". Given that the overfall
goal this change seeks seems to be to simplify it by losing about 20
lines, how about justifying it like so?
Subject: t5403: simplify by using a single repository
There is no strong reason to use separate clones to run
these tests; just use a single test repository prepared
with more modern test_commit shell helper function.
While at it, replace three "awk '{print $N}'" on the same
file with shell built-in "read" into three variables.
> From: Orgad Shaneh <orgads@gmail.com>
>
> * Replace multiple clones and commits by test_commits.
> * Replace 3 invocations of awk by read.
>
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
> t/t5403-post-checkout-hook.sh | 80 +++++++++++++----------------------
> 1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index fc898c9eac..9f9a5163c5 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -7,77 +7,55 @@ test_description='Test the post-checkout hook.'
> . ./test-lib.sh
>
> test_expect_success setup '
> - echo Data for commit0. >a &&
> - echo Data for commit0. >b &&
> - git update-index --add a &&
> - git update-index --add b &&
> - tree0=$(git write-tree) &&
> - commit0=$(echo setup | git commit-tree $tree0) &&
> - git update-ref refs/heads/master $commit0 &&
> - git clone ./. clone1 &&
> - git clone ./. clone2 &&
> - GIT_DIR=clone2/.git git branch new2 &&
> - echo Data for commit1. >clone2/b &&
> - GIT_DIR=clone2/.git git add clone2/b &&
> - GIT_DIR=clone2/.git git commit -m new2
> -'
> -
> -for clone in 1 2; do
> - cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
> -#!/bin/sh
> -echo $@ > $GIT_DIR/post-checkout.args
> -EOF
> - chmod u+x clone${clone}/.git/hooks/post-checkout
> -done
> -
> -test_expect_success 'post-checkout runs as expected ' '
> - GIT_DIR=clone1/.git git checkout master &&
> - test -e clone1/.git/post-checkout.args
> + mv .git/hooks-disabled .git/hooks &&
I am not sure why you want to do this---it sends a wrong signal to
readers saying that you want to use whatever hook that are in the
moved-away .git/hooks-disabled/ directory. I am guessing that the
only reason why you do this is because there must be .git/hooks
directory in order for write_script below to work, so a more
readable approach would be to "mkdir .git/hooks" instead, no?
> + write_script .git/hooks/post-checkout <<-\EOF &&
> + echo $@ >.git/post-checkout.args
A dollar-at inside a shell script that is not in a pair of dq always
makes readers wonder if the author forgot dq around it or omitted eq
around it deliberately; avoid it.
In this case, use "$@" (i.e. within dq) would be more friendly to
readers. The second best is to write it $*, as in this context we
know that $1, $2 and $3 will not contain any $IFS, that echo will
take these three separate arguments and write them on one line
separated by a space in between, which will allow "read A B C" read
them. Or "$*" to feed such a single line with three arguments
separated by a space in between as a single string to echo for the
same effect.
> + EOF
> + test_commit one &&
> + test_commit two &&
> + test_commit three three
Makes readers wonder why the last one duplicates. Is this because
you somehow do not want to use three.t as the pathname in a later
test? "test_commit X" that creates test file X.t is a quite well
established convention (see "git grep '\.t\>' t/"), by the way.
> '
>
> test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
> - old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> - new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
> - flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> - test $old = $new && test $flag = 1
> + git checkout master &&
> + test -e .git/post-checkout.args &&
Use "test -f", as you do know you'd be creating a file ("-e"
succeeds as long as it _exists_, and does not care if it is a file
or directory or whatever).
> + read old new flag <.git/post-checkout.args &&
This indeed is much nicer.
> + test $old = $new && test $flag = 1 &&
> + rm -f .git/post-checkout.args
The last one is not a test but a clean-up. If any of the earlier
step failed (e.g. $old and $new were different), the output file
would be left behind, resulting in confusing the next test.
Instead, do it like so:
test_expect_success 'title of the test' '
test_when_finished "rm -f .git/post-checkout.args" &&
git checkout master &&
test -f .git/post-checkout.args &&
read old new flag <.git/post-checkout.args &&
test $old = $new &&
test $flag = 1
'
That is, use test_when_finished() before the step that creates the
file that may be left behind to arrange that it will be cleaned at
the end.
This comment on clean-up applies to _all_ tests in this patch that
has "rm -f .git/post-checkout.args" at the end.
> '
>
> test_expect_success 'post-checkout runs as expected ' '
> - GIT_DIR=clone1/.git git checkout master &&
> - test -e clone1/.git/post-checkout.args
> + git checkout master &&
> + test -e .git/post-checkout.args &&
> + rm -f .git/post-checkout.args
> '
Now that the script got so simplified, this one looks even more
redundant, given that the previous one already checked the same
"checkout 'master' when already at the tip of 'master'" situation.
Do we still need this one, in other words?
> test_expect_success 'post-checkout args are correct with git checkout -b ' '
> - GIT_DIR=clone1/.git git checkout -b new1 &&
> - old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> - new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
> - flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> - test $old = $new && test $flag = 1
> + git checkout -b new1 &&
> + read old new flag <.git/post-checkout.args &&
> + test $old = $new && test $flag = 1 &&
> + rm -f .git/post-checkout.args
> '
This one forgets "did the hook run and create the file" before
"read", unlike the previous tests. It is not strictly necessary as
"read" will fail if the file is not there, but it'd be better to be
consistent.
> if test "$(git config --bool core.filemode)" = true; then
This is a tangent but this conditional came from an ancient d42ec126
("disable post-checkout test on Cygwin", 2009-03-17) that says
disable post-checkout test on Cygwin
It is broken because of the tricks we have to play with
lstat to get the bearable perfomance out of the call.
Sadly, it disables access to Cygwin's executable attribute,
which Windows filesystems do not have at all.
I wonder if this is still relevant these days (Cc'ed Ramsay for
input). Windows port should be running enabled hooks (and X_OK is
made pretty much no-op in compat/mingw.c IIUC), so the above
conditional is overly broad anyway, even if Cygwin still has issues
with the executable bit.
> mkdir -p templates/hooks
> -cat >templates/hooks/post-checkout <<'EOF'
> -#!/bin/sh
> -echo $@ > $GIT_DIR/post-checkout.args
> +write_script templates/hooks/post-checkout <<-\EOF
> +echo $@ >$GIT_DIR/post-checkout.args
> EOF
> -chmod +x templates/hooks/post-checkout
>
> test_expect_success 'post-checkout hook is triggered by clone' '
> git clone --template=templates . clone3 &&
^ permalink raw reply
* Re: [PATCH 0/2] Improve documentation on UTF-16
From: Philip Oakley @ 2018-12-28 20:31 UTC (permalink / raw)
To: Johannes Sixt, brian m. carlson
Cc: git, Lars Schneider, Torsten Bögershausen
In-Reply-To: <34d4f912-2ec3-9dd1-f5fb-aad6a26e1464@kdbg.org>
On 28/12/2018 08:59, Johannes Sixt wrote:
> Am 28.12.18 um 00:45 schrieb brian m. carlson:
>> On Thu, Dec 27, 2018 at 08:55:27PM +0100, Johannes Sixt wrote:
>>> But why do you add another U+FEFF on the way to UTF-8? There is one
>>> in the
>>> incoming UTF-16 data, and only *that* one must be converted. If
>>> there is no
>>> U+FEFF in the UTF-16 data, the should not be one in UTF-8, either.
>>> Puzzled...
>>
>> So for UTF-16, there must be a BOM. For UTF-16LE and UTF-16BE, there
>> must not be a BOM. So if we do this:
>>
>> $ printf '\xfe\xff\x00\x0a' | iconv -f UTF-16BE -t UTF-16 | xxd -g1
>> 00000000: ff fe ff fe 0a 00 ......
>
> What sort of braindamage is this? Fix iconv.
>
> But as I said, I'm not an expert. I just vented my worries that
> widespread existing practice would be ignored under the excuse "you
> are the outlier".
>
> -- Hannes
For ref, I dug out a Microsoft document [1] on its view of BOMs which
can be compared to the ref [0] Brian gave
[1]
https://docs.microsoft.com/en-us/windows/desktop/intl/using-byte-order-marks
[0] https://unicode.org/faq/utf_bom.html#bom9
Maybe the documentation patch ([PATCH 1/2] Documentation: document
UTF-16-related behavior) should include the line ", because we encode
into UTF-8 internally,", and a link to ref [0], and maybe [1]
Whether the various Windows programs actually follow the Microsoft
convention is another matter altogether .
--
Philip
^ permalink raw reply
* RE: [PATCH v1 1/4] transport-helper: use xread instead of read
From: Randall S. Becker @ 2018-12-28 20:38 UTC (permalink / raw)
To: 'Junio C Hamano', randall.s.becker; +Cc: git
In-Reply-To: <xmqqk1jto1jb.fsf@gitster-ct.c.googlers.com>
On December 28, 2018 15:11, Junio C Hamano wrote:
> randall.s.becker@rogers.com writes:
>
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less
> > than BUFFERSIZE resulting in EINVAL. The call to read in
> > transport-helper.c was the only place outside of wrapper.c where it is
used
> instead of xread.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > transport-helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/transport-helper.c b/transport-helper.c index
> > bf225c698f..a290695a12 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -1225,7 +1225,7 @@ static int udt_do_read(struct
> unidirectional_transfer *t)
> > return 0; /* No space for more. */
> >
> > transfer_debug("%s is readable", t->src_name);
> > - bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> > + bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> > if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> > errno != EINTR) {
> > error_errno(_("read(%s) failed"), t->src_name);
>
> As Peff pointed out in the earlier round of the same patch, replacing
read()
> with xread() here will affect what errno's can be possible after the
function
> returns. The checks affected by this change must also be updated, either
in
> the same patch, or a follow-up patch in the same series. Otherwise we
> _will_ forget to clean them up.
If I read the xread code correctly, the change would be to leave EINTR and
remove the other two conditions. Correct?
^ permalink raw reply
* Re: [PATCH 0/2] Improve documentation on UTF-16
From: Philip Oakley @ 2018-12-28 20:35 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, brian m. carlson
Cc: git, Lars Schneider, Torsten Bögershausen
In-Reply-To: <87lg4adoo5.fsf@evledraar.gmail.com>
On 28/12/2018 08:46, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Dec 27 2018, brian m. carlson wrote:
>
>> We've recently fielded several reports from unhappy Windows users about
>> our handling of UTF-16, UTF-16LE, and UTF-16BE, none of which seem to be
>> suitable for certain Windows programs.
> Just for context, is "we" here $DAYJOB or a reference to some previous
> ML thread(s) on this list, or something else?
I think
https://public-inbox.org/git/CADN+U_PUfnYWb-wW6drRANv-ZaYBEk3gWHc7oJtxohA5Vc3NEg@mail.gmail.com/
was the most recent on the Git list.
--
Philip
^ permalink raw reply
* RE: [PATCH v1 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Randall S. Becker @ 2018-12-28 20:33 UTC (permalink / raw)
To: 'Junio C Hamano', 'Randall S. Becker'
Cc: 'Eric Sunshine', 'Git List'
In-Reply-To: <xmqqa7kpmmtj.fsf@gitster-ct.c.googlers.com>
On December 28, 2018 15:07, Junio C Hamano
> "Randall S. Becker" <randall.s.becker@rogers.com> writes:
>
> > On December 27, 2018 12:03, Eric Sunshine wrote:
> >> On Wed, Dec 26, 2018 at 6:05 PM <randall.s.becker@rogers.com> wrote:
> >> > A number of configuration options are not automatically detected by
> >> > configure mechanisms, including the location of Perl and Python.
> >> > [...]
> >> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> >> > ---
> >> > diff --git a/config.mak.uname b/config.mak.uname @@ -441,26
> +441,45
> >> @@
> >> > ifeq ($(uname_S),NONSTOP_KERNEL)
> >> > # Our's are in ${prefix}/bin (perl might also be in
/usr/bin/perl).
> >> > - PERL_PATH = ${prefix}/bin/perl
> >> > - PYTHON_PATH = ${prefix}/bin/python
> >> > + PERL_PATH = /usr/bin/perl
> >> > + PYTHON_PATH = /usr/bin/python
> >>
> >> Is the in-code comment about ${prefix} still applicable after this
change?
> >
> > The ${prefix} is not applicable on this platform for perl and python.
> > Those locations must be in /usr/bin and are managed by the Operating
> > System vendor not by customers. The change is wrapped in an IF so is
> > only applicable to NonStop.
>
> So the answer is "Our's are in ${prefix}/bin..." is no longer correct? If
so, this
> patch must remove that stale comment at the same time, no?
Yessir. Fixed at v3 (now at v4).
^ permalink raw reply
* Re: ag/sequencer-reduce-rewriting-todo, was Re: What's cooking in git.git (Dec 2018, #02; Fri, 28)
From: Junio C Hamano @ 2018-12-28 20:28 UTC (permalink / raw)
To: Alban Gruin; +Cc: git
In-Reply-To: <202be141-cc57-cf2a-dc15-59647e7bca09@gmail.com>
Alban Gruin <alban.gruin@gmail.com> writes:
>> With too many topics in-flight that touch sequencer and rebaser,
>> this need to wait giving precedence to other topics that fix bugs.
>>
>
> Most of these topics have reached master and have been released in git
> 2.20. Currently, there is four topics actually touching rebase,
> interactive rebase and/or the sequencer (js/rebase-i-redo-exec,
> nd/backup-log, en/rebase-merge-on-sequencer and nd/the-index). Among
> these, only nd/the-index conflicts with my series.
OK, so now it's turn for this topic ;-) Thanks.
^ permalink raw reply
* Re: [PATCH v12 04/26] ident: add the ability to provide a "fallback identity"
From: Junio C Hamano @ 2018-12-28 19:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Paul-Sebastian Ungureanu, git, t.gummerer
In-Reply-To: <nycvar.QRO.7.76.6.1812272221380.45@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Wed, 26 Dec 2018, Junio C Hamano wrote:
>
>> Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
>>
>> > +static void set_env_if(const char *key, const char *value, int *given, int bit)
>> > +{
>> > + if ((*given & bit) || getenv(key))
>> > + return; /* nothing to do */
>> > + setenv(key, value, 0);
>> > + *given |= bit;
>> > +}
>>
>> We call setenv(3) with overwrite=0 but we protect the call with a
>> check for existing value with getenv(3), which feels a bit like an
>> anti-pattern. Wouldn't the following be simpler to follow, I wonder?
>>
>> if (!(*given & bit)) {
>> setenv(key, value, 1);
>> *given |= bit;
>> }
>>
>> The only case these two may behave differently is when '*given' does
>> not have the 'bit' set but the environment 'key' already exists.
>
> Indeed, this is the case where your version would actually do the wrong
> thing. Imagine that GIT_AUTHOR_NAME is set already. Your code would
> *override* it. But that is not what we want to do here. We want to *fall
> back* if there is no already-configured value.
>
> And of course we won't set the `given` bit if we don't fall back here;
> that should be done somewhere else, where that environment variable (that
> we *refuse* to overwrite) is *actually* used.
OK, so the designed calling sequence of the new "prepare fallback
ident" is that any process that wants to use fallback ident must
call the "prepare" function _before_ making a call to any other
functions (IOW, it is a BUG() if things like git_committer_info() is
called before prepare_fallback_ident() gets called). Under that
condition, you are absolutely right that the two implementation
behaves differently.
That indeed indicates that this is unfriendly to future callers,
which was the main issue I had with the patch. A comment before the
prepare_fallback_ident() function to explain that would have helped.
Also the first condition checking bit in *given does not help---it
is quite misleading. It would have helped if it were
static void set_env_if( ... )
{
if (*given & bit)
BUG("%s was checked before prepare_fallback got called", key);
if (getenv(key))
return; /* nothing to do */
setenv(key, value, 1);
*given |= bit;
}
Because (author|committer)_ident_explicitly_given would never say
"yes, already" if the setting of fallback MUST be done before using
other API functions in ident.c, it we were to have a check for that
condition, it would be testing for a BUG(). And I wouldn't have
been confused by the code while reviewing.
>> > +void prepare_fallback_ident(const char *name, const char *email)
>> > +{
>> > + set_env_if("GIT_AUTHOR_NAME", name,
>> > + &author_ident_explicitly_given, IDENT_NAME_GIVEN);
>> > + set_env_if("GIT_AUTHOR_EMAIL", email,
>> > + &author_ident_explicitly_given, IDENT_MAIL_GIVEN);
>> > + set_env_if("GIT_COMMITTER_NAME", name,
>> > + &committer_ident_explicitly_given, IDENT_NAME_GIVEN);
>> > + set_env_if("GIT_COMMITTER_EMAIL", email,
>> > + &committer_ident_explicitly_given, IDENT_MAIL_GIVEN);
>> > +}
>>
>> Introducing this function alone without a caller and without
>> function doc is a bit unfriendly to future callers, who must be
>> careful when to call it, I think. For example, they must know that
>> it will be a disaster if they call this before they call
>> git_ident_config(), right?
As you can see from this "For example,...", the review comment (and
the "simplified but does the wrong thing" version) was written under
an assumption different from the expected calling sequence the
posted version makes. What future writers of callers that use the
fallback ident feature must know is (unlike the above) that they
must call the "prepare" before they call git_ident_config() or
anything in ident.c. A comment before this function that explain
how and when in the program flow this is designed to be used is
needed.
Thanks for a clarification.
^ permalink raw reply
* Re: [PATCH v1 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Junio C Hamano @ 2018-12-28 20:07 UTC (permalink / raw)
To: Randall S. Becker
Cc: 'Eric Sunshine', 'Git List',
'Randall S. Becker'
In-Reply-To: <000601d49e0b$e11d7520$a3585f60$@rogers.com>
"Randall S. Becker" <randall.s.becker@rogers.com> writes:
> On December 27, 2018 12:03, Eric Sunshine wrote:
>> On Wed, Dec 26, 2018 at 6:05 PM <randall.s.becker@rogers.com> wrote:
>> > A number of configuration options are not automatically detected by
>> > configure mechanisms, including the location of Perl and Python.
>> > [...]
>> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> > ---
>> > diff --git a/config.mak.uname b/config.mak.uname @@ -441,26 +441,45
>> @@
>> > ifeq ($(uname_S),NONSTOP_KERNEL)
>> > # Our's are in ${prefix}/bin (perl might also be in /usr/bin/perl).
>> > - PERL_PATH = ${prefix}/bin/perl
>> > - PYTHON_PATH = ${prefix}/bin/python
>> > + PERL_PATH = /usr/bin/perl
>> > + PYTHON_PATH = /usr/bin/python
>>
>> Is the in-code comment about ${prefix} still applicable after this change?
>
> The ${prefix} is not applicable on this platform for perl and
> python. Those locations must be in /usr/bin and are managed by the
> Operating System vendor not by customers. The change is wrapped in
> an IF so is only applicable to NonStop.
So the answer is "Our's are in ${prefix}/bin..." is no longer
correct? If so, this patch must remove that stale comment at the
same time, no?
^ permalink raw reply
* Re: [PATCH] imap-send: Fix compilation without deprecated OpenSSL APIs
From: Junio C Hamano @ 2018-12-28 19:20 UTC (permalink / raw)
To: Rosen Penev; +Cc: git
In-Reply-To: <CAKxU2N9egn6MbJeWUWFsyYpnwOCj4=mckmkJJtVJGhmQUt36aw@mail.gmail.com>
Rosen Penev <rosenp@gmail.com> writes:
> On Wed, Dec 26, 2018 at 10:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Rosen Penev <rosenp@gmail.com> writes:
>>
>> > Initialization in OpenSSL has been deprecated in version 1.1.
>>
>> https://www.openssl.org/docs/man1.0.2/ssl/SSL_library_init.html says
>>
>> SSL_library_init() must be called before any other action takes
>> place.
>>
>> https://www.openssl.org/docs/man1.1.0/ssl/SSL_library_init.html says
>> the same.
> Later on in the document it mentions that it is deprecated.
>>
>> Which makes it necessary for us to defend the following claim
>>
>> > This makes
>> > compilation fail when deprecated APIs for OpenSSL are compile-time
>> > disabled.
>>
>> as a valid problem description more rigorously. To me, the cursory
>> web-serfing I did above makes me suspect that an OpenSSL
>> implementation with such a compile-time disabling _is_ buggy, as it
>> forbids the API users to call an API function they are told to call
>> before doing anything else.
> I agree the man page is misleading. The changelog for 1.1.0 is very
> clear though:
>
> Added support for auto-initialisation and de-initialisation of the library.
> OpenSSL no longer requires explicit init or deinit routines to be called,
> except in certain circumstances. See the OPENSSL_init_crypto() and
> OPENSSL_init_ssl() man pages for further information.
> [Matt Caswell]
All it says is explicit calls to SSL_library_init() is now optional.
It does not say it is a compile-time-error worthy offense to make
explicit init/deinit calls. Which still means that an OpenSSL
implementation with such a compile-time disabling _is_ buggy, as it
forbids the API users to call an API function they are told to call
before doing anything else.
>> > diff --git a/imap-send.c b/imap-send.c
>> > index b4eb886e2..21f741c8c 100644
>> > --- a/imap-send.c
>> > +++ b/imap-send.c
>> > @@ -284,8 +284,10 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
>> > int ret;
>> > X509 *cert;
>> >
>> > +#if (OPENSSL_VERSION_NUMBER < 0x10000000L)
>>
>> https://www.openssl.org/docs/man1.1.0/crypto/OPENSSL_VERSION_NUMBER.html
>>
>> says that OPENSSL_VERSION_NUMBER is of form 0xMNNFFPPS where M is
>> major, NN is minor, FF is fix, PP is patch and S is status, and
>> gives an example that 0x00906023 stands for 0.9.6.b beta 3 (M=0,
>> NN=09, FF=06, PP=02 and S=3). So "< 0x10000000L" means "anything
>> with M smaller than 1". IOW, we would no longer call _init() for
>> e.g. "version 1.0.0 beta 0". That contradicts with the first claim
>> of the proposed log message ("deprecated in 1.1" implying that it is
>> not yet deprecated in say 1.0.2).
> This is a mistake. I will send a v2 to fix.
>
> Oh I see what I did wrong. I mistakenly copied the above
> OPENSSL_VERSION_NUMBER check without looking carefully at the number.
Yup, please be careful next time.
>>
>>
>>
>> > SSL_library_init();
>> > SSL_load_error_strings();
>> > +#endif
>> >
>> > meth = SSLv23_method();
>> > if (!meth) {
^ permalink raw reply
* Re: [PATCH v3] log -G: ignore binary files
From: Junio C Hamano @ 2018-12-26 23:24 UTC (permalink / raw)
To: Thomas Braun; +Cc: git, peff, sbeller, avarab
In-Reply-To: <f7cb34c6268f556772fa4ee374e2541ba518e2b8.1544811828.git.thomas.braun@virtuell-zuhause.de>
Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> - The internally used algorithm for generating patch text is based on
> xdiff and its states in [1]
>
> > The output format of the binary patch file is proprietary
> > (and binary) and it is basically a collection of copy and insert
> > commands [..]
>
> which means that the current format could change once the internal
> algorithm is changed as the format is not standardized. In addition
> the git binary patch format used for preparing patches for git apply
> is *different* from the xdiff format as can be seen by comparing
This particular argument sounds like a red herring. After all, when
the --text option is given
>
> git log -p -a
>
> commit 6e95bf4bafccf14650d02ab57f3affe669be10cf
> Author: A U Thor <author@example.com>
> Date: Thu Apr 7 15:14:13 2005 -0700
>
> modify binary file
>
> diff --git a/data.bin b/data.bin
> index f414c84..edfeb6f 100644
> --- a/data.bin
> +++ b/data.bin
> @@ -1,2 +1,4 @@
> a
> a^@a
> +a
> +a^@a
we will see 'a' in the output no matter how xdiff internally works;
there is no way to express the above change textually without
showing "+a" somewhere in the patch output.
The rest of the log message looks good, and ...
> Changes since v2:
> - Introduce a setup step for the new tests
> - Really start with a clean history in the tests
> - Added more complex commit history for the tests
> - Use test_when_finished for cleanup instead of doing nothing
> - Enhanced commit message to motivate the change better
> - Added some more documentation
... the tests are certainly a lot easier to follow.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits
From: Junio C Hamano @ 2018-12-26 22:52 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87tvj1ok4k.fsf@javad.com>
Sergey Organov <sorganov@gmail.com>, Sergey Organov
<sorganov@gmail.com> writes:
> Hi Junio,
>
> What's the status of these patches?
The status of these patches is "Just updated on the list", as far as
I am concerned, and its cover letter would have described what's
improved relative to the previous round better than whatever answer
I could give here ;-)
I checked the overall diff between the previous round and the result
of applying all four patches to find out that the updates are only
to the tests. The changes in the code (still) looked correct.
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b1602718f8..3259bd59eb 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
test_expect_code 129 git cherry-pick -m 0 b
'
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
git reset --hard &&
git checkout a^0 &&
- test_expect_code 128 git cherry-pick -m 1 b &&
- git diff --exit-code a --
+ git cherry-pick -m 1 b &&
+ git diff --exit-code c --
'
@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
'
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
git reset --hard &&
git checkout c^0 &&
- test_must_fail git revert -m 1 b &&
- git diff --exit-code c
+ git revert -m 1 b &&
+ git diff --exit-code a
'
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac6f0..127dd0082f 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
git checkout -b new A
'
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
git reset --hard A -- &&
- test_must_fail git cherry-pick --ff -m 1 B &&
- git diff --exit-code A --
+ git cherry-pick --ff -m 1 B &&
+ git diff --exit-code C --
'
test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeefdc9..941d5026da 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
test_expect_success 'cherry-pick persists opts correctly' '
pristine_detach initial &&
- test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+ # to make sure that the session to cherry-pick a sequence
+ # gets interrupted, use a high-enough number that is larger
+ # than the number of parents of any commit we have created
+ mainline=4 &&
+ test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
test_path_is_file .git/sequencer/todo &&
@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
echo "true" >expect &&
git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
test_cmp expect actual &&
- echo "1" >expect &&
+ echo "$mainline" >expect &&
git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
test_cmp expect actual &&
echo "recursive" >expect &&
^ permalink raw reply related
* Re: [PATCH v2 1/6] ref-filter: add objectsize:disk option
From: Junio C Hamano @ 2018-12-26 20:44 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
In-Reply-To: <01020167e063687c-37a43a09-0a5f-4335-8c21-ec15a0a67882-000000@eu-west-1.amazonses.com>
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> @@ -880,7 +886,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
> name++;
> if (!strcmp(name, "objecttype"))
> v->s = xstrdup(type_name(oi->type));
> - else if (!strcmp(name, "objectsize")) {
> + else if (!strcmp(name, "objectsize:disk")) {
> + v->value = oi->disk_size;
> + v->s = xstrfmt("%"PRIuMAX, (intmax_t)oi->disk_size);
Shouldn't this cast the field to (uintmax_t) type, as we'd format
with %PRIuMAX and we know the size on-disk is not negative?
Other than that, looks good.
Let me rewind the tip of 'next' and replace the previous round with
this iteration.
Thanks.
> + } else if (!strcmp(name, "objectsize")) {
> v->value = oi->size;
> v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
> }
>
> --
> https://github.com/git/git/pull/552
^ permalink raw reply
* Re: [PATCH 00/23] sb/more-repo-in-api
From: Junio C Hamano @ 2018-12-26 18:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20181215000942.46033-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> I realized next has not been rewound, so I can resend sb/more-repo-in-api,
> which I hereby do. The changes are minimal and address the only comment
> by Jonathan so far.
Yeah, the only change I see matches what is in your range-diff after
applying them to my tree.
Will rewind 'next' by the end of the year and replace the topic. Thanks.
> 1: 99017ffac8 ! 1: f24b120287 submodule: use submodule repos for object lookup
> @@ -40,12 +40,13 @@
> - * attempt to lookup both the left and right commits and put them into the
> - * left and right pointers.
> +/*
> -+ * Initialize 'out' based on the provided submodule path.
> ++ * Initialize a repository struct for a submodule based on the provided 'path'.
> + *
> + * Unlike repo_submodule_init, this tolerates submodules not present
> + * in .gitmodules. This function exists only to preserve historical behavior,
> + *
> -+ * Returns 0 on success, -1 when the submodule is not present.
> ++ * Returns the repository struct on success,
> ++ * NULL when the submodule is not present.
> */
> -static void show_submodule_header(struct diff_options *o, const char *path,
> +static struct repository *open_submodule(const char *path)
> @@ -59,6 +60,7 @@
> + return NULL;
> + }
> +
> ++ /* Mark it as a submodule */
> + out->submodule_prefix = xstrdup(path);
> +
> + strbuf_release(&sb);
> 2: 809765861c = 2: 25190d6174 submodule: don't add submodule as odb for push
> 3: 4a7735da72 = 3: 965421aab2 commit-graph: convert remaining functions to handle any repo
> 4: aeeb1ba49e = 4: bf31f32723 commit: prepare free_commit_buffer and release_commit_memory for any repo
> 5: 5ffebe9463 = 5: c4e54e6b0d path.h: make REPO_GIT_PATH_FUNC repository agnostic
> 6: 9c89920c46 = 6: a7ed0c57ba t/helper/test-repository: celebrate independence from the_repository
^ permalink raw reply
* Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present
From: Junio C Hamano @ 2018-12-26 18:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20181214235945.41191-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> 2018-09-07). The revert was needed as the nearby commit e98317508c
> (submodule: ensure core.worktree is set after update, 2018-06-18) is
> faulty and at the time of 7e25437d35 (Merge branch
> 'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty
> commit only, as they were depending on each other: If core.worktree is
> unset, we have to have ways to ensure that it is set again once
> the working tree reappears again.
>
> Now that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),
> specifically 74d4731da1 (submodule--helper: replace
> connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is
> present, we already check and ensure core.worktree is set when
> populating a new work tree, such that we can re-introduce the commits
> that unset core.worktree when removing the worktree.
Cleanly explained. Will queue. Thanks.
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 14 ++++++++++++++
> submodule.h | 2 ++
> t/lib-submodule-update.sh | 3 ++-
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6415cc5580..d393e947e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
> return ret;
> }
>
> +void submodule_unset_core_worktree(const struct submodule *sub)
> +{
> + char *config_path = xstrfmt("%s/modules/%s/config",
> + get_git_common_dir(), sub->name);
> +
> + if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
> + warning(_("Could not unset core.worktree setting in submodule '%s'"),
> + sub->path);
> +
> + free(config_path);
> +}
> +
> static const char *get_super_prefix_or_empty(void)
> {
> const char *s = get_super_prefix();
> @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,
>
> if (is_empty_dir(path))
> rmdir_or_warn(path);
> +
> + submodule_unset_core_worktree(sub);
> }
> }
> out:
> diff --git a/submodule.h b/submodule.h
> index a680214c01..9e18e9b807 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -131,6 +131,8 @@ int submodule_move_head(const char *path,
> const char *new_head,
> unsigned flags);
>
> +void submodule_unset_core_worktree(const struct submodule *sub);
> +
> /*
> * Prepare the "env_array" parameter of a "struct child_process" for executing
> * a submodule by clearing any repo-specific environment variables, but
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 016391723c..51d4555549 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
> git branch -t remove_sub1 origin/remove_sub1 &&
> $command remove_sub1 &&
> test_superproject_content origin/remove_sub1 &&
> - ! test -e sub1
> + ! test -e sub1 &&
> + test_must_fail git config -f .git/modules/sub1/config core.worktree
> )
> '
> # ... absorbing a .git directory along the way.
^ permalink raw reply
* Re: [PATCH 1/4] submodule update: add regression test with old style setups
From: Junio C Hamano @ 2018-12-26 18:21 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20181214235945.41191-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The place to add such a regression test may look odd in t7412, but
> that is the best place as there we setup old style submodule setups
> explicitly.
Makes sense; thanks.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index ce74c12da2..1cfa150768 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
> GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
> core.worktree "../../../nested" &&
> # make sure this re-setup is correct
> - git status --ignore-submodules=none
> + git status --ignore-submodules=none &&
> +
> + # also make sure this old setup does not regress
> + git submodule update --init --recursive >out 2>err &&
> + test_must_be_empty out &&
> + test_must_be_empty err
> '
>
> test_expect_success 'absorb the git dir in a nested submodule' '
^ permalink raw reply
* Re: [PATCH v1 1/4] transport-helper: use xread instead of read
From: Junio C Hamano @ 2018-12-28 20:10 UTC (permalink / raw)
To: randall.s.becker; +Cc: git, Randall S. Becker
In-Reply-To: <20181226230523.16572-2-randall.s.becker@rogers.com>
randall.s.becker@rogers.com writes:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c where it is used instead of xread.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> transport-helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index bf225c698f..a290695a12 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
> return 0; /* No space for more. */
>
> transfer_debug("%s is readable", t->src_name);
> - bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> + bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
> if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> errno != EINTR) {
> error_errno(_("read(%s) failed"), t->src_name);
As Peff pointed out in the earlier round of the same patch,
replacing read() with xread() here will affect what errno's can be
possible after the function returns. The checks affected by this
change must also be updated, either in the same patch, or a
follow-up patch in the same series. Otherwise we _will_ forget to
clean them up.
^ 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