* Re: [PATCH] t*: avoid using pipes
From: Prathamesh Chavan @ 2017-03-09 5:26 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, Johannes Sixt, git@vger.kernel.org, Pranit Bauva
In-Reply-To: <CAME+mvUzR6--AeUff6yGZ69GN-hE6AyDP-CkdFxnFAwccpn2yg@mail.gmail.com>
I have created the required changes and submitted a single file patch.
Also I tried my best to include each of the suggestions in that patch.
https://public-inbox.org/git/0102015aae7b8536-00c57d0a-1d48-4153-a202-87c4ea9e0e19-000000@eu-west-1.amazonses.com/
On Wed, Mar 8, 2017 at 7:02 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> On Wed, Mar 8, 2017 at 11:33 AM, Jeff King <peff@peff.net> wrote:
>> On Tue, Mar 07, 2017 at 12:52:49PM -0800, Stefan Beller wrote:
>>
>>> On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>>
>>> > Welcome to the Git community!
>>>
>>> >
>>> > Actually, being a *micro* project, it should stay so. Not doing all of the
>>> > changes would leave some tasks for other apprentices to get warm with our
>>> > review process.
>>>
>>> right, so just pick one file.
>>
>> I also wonder if we really want all invocations of git to be marked up
>> in this way. If the primary goal of the test is checking that a certain
>> git command runs successfully and generates the expected output, then I
>> think it is a good candidate for conversion.
>>
>> So in a hunk like this:
>>
>> test_expect_success 'git commit-tree records the correct tree in a commit' '
>> commit0=$(echo NO | git commit-tree $P) &&
>> - tree=$(git show --pretty=raw $commit0 |
>> - sed -n -e "s/^tree //p" -e "/^author /q") &&
>> + tree=$(git show --pretty=raw $commit0 >out &&
>> + sed -n -e "s/^tree //p" -e "/^author /q" <out) &&
>> test "z$tree" = "z$P"
>>
>> we are interested in testing commit-tree, not "git show". Is it worth
>> avoiding pipes there? I admit the cost to using the intermediate file is
>> not huge there, but it feels more awkward and un-shell-like to me as a
>> reader.
>>
>> -Peff
>
> Thank you everyone, for reviewing my changes. And as said in the
> reviews, I'll send a single patch file as my microproject, leaving the other
> files as low hanging fruit for the others to look at. Also, I try to include as
> many suggested improvements as possible and will also remember them for
> my future patches.
^ permalink raw reply
* Re: [PATCH 01/10] pack-objects: eat CR in addition to LF after fgets.
From: Jeff King @ 2017-03-09 7:01 UTC (permalink / raw)
To: Jeff Hostetler
Cc: git, gitster, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-2-git-send-email-jeffhost@microsoft.com>
On Wed, Mar 08, 2017 at 05:37:56PM +0000, Jeff Hostetler wrote:
> From: Jeff Hostetler <git@jeffhostetler.com>
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> builtin/pack-objects.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index f294dcf..7e052bb 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2764,6 +2764,8 @@ static void get_object_list(int ac, const char **av)
> int len = strlen(line);
> if (len && line[len - 1] == '\n')
> line[--len] = 0;
> + if (len && line[len - 1] == '\r')
> + line[--len] = 0;
Rather than add features to this bespoke line-reader, can we switch this
to use strbuf_getline()? That handles line endings, and avoids the
awkward corner case where fgets "breaks" a long line across two calls.
Something like the patch below. I suspect read_object_list_from_stdin()
should get the same treatment.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 76b1919ca..6b9fffe9c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2765,7 +2765,7 @@ static void record_recent_commit(struct commit *commit, void *data)
static void get_object_list(int ac, const char **av)
{
struct rev_info revs;
- char line[1000];
+ struct strbuf buf = STRBUF_INIT;
int flags = 0;
init_revisions(&revs, NULL);
@@ -2775,12 +2775,12 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow();
- while (fgets(line, sizeof(line), stdin) != NULL) {
- int len = strlen(line);
- if (len && line[len - 1] == '\n')
- line[--len] = 0;
- if (!len)
+ while (strbuf_getline(&buf, stdin) != EOF) {
+ const char *line = buf.buf;
+
+ if (!buf.len)
break;
+
if (*line == '-') {
if (!strcmp(line, "--not")) {
flags ^= UNINTERESTING;
@@ -2800,6 +2800,7 @@ static void get_object_list(int ac, const char **av)
if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", line);
}
+ strbuf_release(&buf);
if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
return;
^ permalink raw reply related
* Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Jeff King @ 2017-03-09 7:04 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Junio C Hamano, Jeff Hostetler, git, markbt, benpeart,
jonathantanmy
In-Reply-To: <1c38ffbc-e4a2-32e5-d324-506c2111043e@jeffhostetler.com>
On Wed, Mar 08, 2017 at 03:21:11PM -0500, Jeff Hostetler wrote:
> > And not ."gitmodules"?
> >
> > What happens when we later add ".gitsomethingelse"?
> >
> > Do we have to worry about the case where the set of git "special
> > files" (can we have a better name for them please, by the way?)
> > understood by the sending side and the receiving end is different?
> >
> > I have a feeling that a mode that makes anything whose name begins
> > with ".git" excempt from the size based cutoff may generally be
> > easier to handle.
>
> I forgot about ".gitmodules". The more I think about it, maybe
> we should always include them (or anything starting with ".git*")
> and ignore the size, since they are important for correct behavior.
I'm also in favor of staking out ".git*" as "this is special and belongs
to Git".
A while back when we discussed whether to allow symlinks for
.gitattributes, etc, I think the consensus was to treat the whole
".git*" namespace consistently. I haven't followed up with patches yet,
but my plan was to go that route.
-Peff
^ permalink raw reply
* Re: [PATCH 03/10] pack-objects: test for --partial-by-size --partial-special
From: Jeff King @ 2017-03-09 7:35 UTC (permalink / raw)
To: Jeff Hostetler
Cc: git, gitster, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-4-git-send-email-jeffhost@microsoft.com>
On Wed, Mar 08, 2017 at 05:37:58PM +0000, Jeff Hostetler wrote:
> diff --git a/t/5316-pack-objects-partial.sh b/t/5316-pack-objects-partial.sh
> [...]
> +test_expect_success 'setup' '
> + perl -e "print \"a\" x 11;" > a &&
> + perl -e "print \"a\" x 1100;" > b &&
> + perl -e "print \"a\" x 1100000;" > c &&
> + echo "ignored" > .gitignore &&
> + git add a b c .gitignore &&
> + git commit -m test
> + '
A few minor style nits. We usually prefer ">a" with no space, and the
closing single-quote isn't indented.
> +test_expect_success 'special' '
> + git pack-objects --revs --thin --stdout --partial-special >spec.pack <<-EOF &&
> + master
> +
> + EOF
> + git index-pack spec.pack &&
> + test 1 = $(git verify-pack -v spec.pack | grep blob | wc -l)
> + '
All of the tests make sense to me except this one. I see from the code
in pack-objects why this returns only the .gitattributes file. I'm just
not clear on whether that would ever be useful. I guess it lets you ask
"give me only the special files", but again, that seems kind of weird if
you are not otherwise limiting.
-Peff
^ permalink raw reply
* Re: [PATCH][GSoc] Changed signed flags to unsigned type
From: Christian Couder @ 2017-03-09 7:49 UTC (permalink / raw)
To: Vedant Bassi; +Cc: git
In-Reply-To: <CACczA6V6t4f6TTT=CJfqsuCtbYuM1QNh8AgtOwqRt7pz4VMeRA@mail.gmail.com>
On Thu, Mar 9, 2017 at 5:24 AM, Vedant Bassi <sharababy.dev@gmail.com> wrote:
> As part of my microproject :
>
> Use unsigned integral type for collection of bits:
> Pick one field of a structure that (1) is of signed integral type and
> (2) is used as a collection of multiple bits. Discuss if there is a
> good reason why it has to be a signed integral field and change it to
> an unsigned type otherwise.
>
> More ref: https://public-inbox.org/git/xmqqsiebrlez.fsf@gitster.dls.corp.google.com
> http://stackoverflow.com/questions/29795170/usage-of-signed-vs-unsigned-variables-for-flags-in-c
>
> I have found several structures where a signed int was used on flags
> for bitwise & to check various cases.
This email has a title that starts with "[PATCH]" but it doesn't
contain a real patch that can be applied using `git am` for example.
Please look at https://git.github.io/SoC-2017-Microprojects/ and at
Documentation/SubmittingPatches about how patches should be created.
> diff --git a/bisect.h b/bisect.h
>
> index a979a7f..4b562a8 100644
>
> --- a/bisect.h
>
> +++ b/bisect.h
>
> @@ -16,6 +16,8 @@ extern struct commit_list *filter_skipped(struct
> commit_list *list,
>
>
>
> struct rev_list_info {
>
> struct rev_info *revs;
>
> +
>
> + // int flags changed to unsigned int
You don't need to add such comments as "what has been done" will be
obvious when looking at the commit. It could be interesting to explain
"why the change is made" in the commit message though.
If there is something subtle in the code or something that could save
a reader some time if it was documented, then a comment might be
useful, but anyway comments should use "/* ... */" markers, not "//".
> unsigned int flags;
>
> int show_timestamp;
>
> int hdr_termination;
[...]
> result : the changes were made in bisect.h , parse-options.h and builtin/add.c
>
> I have not yet tested these changes.
I think sending just one patch for bisect.h is ok. If you really want
you could send another patch for parse-options.h and yet another one
for builtin/add.c, all in the same patch series.
Anyway please test that the patches can be applied (using git am) and
that they look good (compared with other commits) when applied before
sending them to the list. And yeah it is also a good idea to also
check that the test suite still passes after each patch before sending
them.
^ permalink raw reply
* Re: [PATCH 04/10] upload-pack: add partial (sparse) fetch
From: Jeff King @ 2017-03-09 7:48 UTC (permalink / raw)
To: Jeff Hostetler
Cc: git, gitster, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-5-git-send-email-jeffhost@microsoft.com>
On Wed, Mar 08, 2017 at 05:37:59PM +0000, Jeff Hostetler wrote:
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index c59ac99..0032729 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -212,6 +212,7 @@ out of what the server said it could do with the first 'want' line.
> upload-request = want-list
> *shallow-line
> *1depth-request
> + *partial
> flush-pkt
>
> want-list = first-want
> @@ -223,10 +224,15 @@ out of what the server said it could do with the first 'want' line.
> PKT-LINE("deepen-since" SP timestamp) /
> PKT-LINE("deepen-not" SP ref)
>
> + partial = PKT-LINE("partial-by-size" SP magnitude) /
> + PKT-LINE("partial-special)
> +
I probably would have added this as a capability coming back from the
client, since it only makes sense to send once (the same way we ask for
other features like include-tag or ofs-delta). I guess it's six of one,
half a dozen of the other, though.
I notice that you require the client to request the "partial" capability
_and_ to send these commands. I'm not sure what the client capability
response is helping. The server has said "I can do this" and the client
either asks for it or not.
> + if (skip_prefix(line, "partial-by-size ", &arg)) {
> + unsigned long s;
> + if (!client_requested_partial_capability)
> + die("git upload-pack: 'partial-by-size' option requires 'partial' capability");
> + if (!git_parse_ulong(arg, &s))
> + die("git upload-pack: invalid partial-by-size value: %s", line);
> + strbuf_addstr(&partial_by_size, "--partial-by-size=");
> + strbuf_addstr(&partial_by_size, arg);
> + have_partial_by_size = 1;
> + continue;
So we parse it here for validation, but then pass the original string on
to be parsed again by pack-objects. I think I'd rather see us use the
result of our parse here, just to avoid any bugs where the parsing isn't
identical (and there is such a bug currently due to the signed/unsigned
thing I mentioned).
I also wonder whether the magnitude suffixes are worth exposing across
the wire. Anybody touching the list of units in git_parse_ulong() would
probably be surprised that the protocol is dependent on them (not that I
expect us to really take any away, but it just seems like an unnecessary
protocol complication).
-Peff
^ permalink raw reply
* Re: [PATCH 06/10] rev-list: add --allow-partial option to relax connectivity checks
From: Jeff King @ 2017-03-09 7:56 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Junio C Hamano, Jeff Hostetler, git, markbt, benpeart,
jonathantanmy
In-Reply-To: <2cc503b4-f6cf-336a-bc3f-f44b1d187eaf@jeffhostetler.com>
On Wed, Mar 08, 2017 at 03:10:54PM -0500, Jeff Hostetler wrote:
> > Even though I do very much like the basic "high level" premise to
> > omit often useless large blobs that are buried deep in the history
> > we would not necessarily need from the initial cloning and
> > subsequent fetches, I find it somewhat disturbing that the code
> > "Assume"s that any missing blob is due to an previous partial clone.
> > Adding this option smells like telling the users that they are not
> > supposed to run "git fsck" because a partially cloned repository is
> > inherently a corrupt repository.
> >
> > Can't we do a bit better? If we want to make the world safer again,
> > what additional complexity is required to allow us to tell the
> > "missing by design" and "corrupt repository" apart?
>
> I'm open to suggestions here. It would be nice to extend the
> fetch-pack/upload-pack protocol to return a list of the SHAa
> (and maybe the sizes) of the omitted blobs, so that a partial
> clone or fetch would still be able to be integrity checked.
Yeah, the early external-odb patches did this. It lets you do a more
accurate fsck, and it also helps diff avoid faulting in large-object
cases (because we can mark them as binary for "free" by comparing the
size to big_file_threshold).
So I think it makes a lot of sense in the large-blob case, where
transmitting a type/size/sha1 tuple is way more efficient than sending
the blob itself. But it's less clear for "sparse" cases where just
enumerating the set of blobs may be prohibitively large.
I have a feeling that the "sparse" thing needs to be handled separately
from "partial". IOW, the client needs to tell the server "I'm only
interested in the path foo/bar, so just send that". Then you don't find
out about the types and sizes outside of that path, but you don't need
to; the sparse path is stored locally and fsck knows to avoid looking
into it.
-Peff
^ permalink raw reply
* Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Jeff King @ 2017-03-09 7:31 UTC (permalink / raw)
To: Jeff Hostetler
Cc: git, gitster, markbt, benpeart, jonathantanmy, Jeff Hostetler
In-Reply-To: <1488994685-37403-3-git-send-email-jeffhost@microsoft.com>
On Wed, Mar 08, 2017 at 05:37:57PM +0000, Jeff Hostetler wrote:
> From: Jeff Hostetler <git@jeffhostetler.com>
>
> Teach pack-objects to omit blobs from the generated packfile.
>
> When the --partial-by-size=n[kmg] argument is used, only blobs
> smaller than the requested size are included. When n is zero,
> no blobs are included.
>
> When the --partial-special argument is used, git special files,
> such as ".gitattributes" and ".gitignores" are included.
>
> When both are given, the union of two are included.
I understand why one would want to do:
--partial-by-size=100 --partial-special
and get the union. The first one restricts, and the second one adds back
in. But I don't understand why "--partial-special" by itself makes any
sense. Wouldn't we already be including all blobs, and it would be a
noop?
Also, I was thinking a bit on Junio's comment elsewhere on whether
read_object_list_from_stdin() should do the same limiting. I think the
answer is "probably not", because whoever is generating that object list
can cull the set. You could do it today with something like:
git rev-list --objects HEAD |
git cat-file --batch-check='%(objectsize) %(objecttype) %(objectname) %(rest)' |
perl -lne 's/^(\d+) (\S+) //; print if $2 ne "blob" || $1 < 100' |
git pack-objects
But if we are going to add this --partial-by-size for the pack-objects
traversal, shouldn't we just add it to rev-list? Then:
git rev-list --objects --partial-by-size=100 --partial-special |
git pack-objects
works, and you should get it in the pack-objects basically for free (I
think you'd have to allow through the "--partial" arguments on stdin,
and make sure the rev-list implementation is done via
traverse_commit_list).
As a bonus, I suspect it would make the --partial-special path-handling
easier, because you'd see each tree entry rather than the fully
constructed path (so no more monkeying around with "/").
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 7e052bb..2df2f49 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000;
>
> static unsigned long window_memory_limit = 0;
>
> +static signed long partial_by_size = -1;
I would have expected this to be an off_t, though I think
OPT_MAGNITUDE() forces you into "unsigned long". I guess it is nothing
new for Git; we use "unsigned long" for single object sizes elsewhere,
so systems with a 32-bit long are out of luck anyway until we fix that.
The signed "long" here is unfortunate, as it limits us to 2G on such
systems. Maybe it is not worth worrying too much about. The "big object"
threshold is usually around 500MB. I think the failure behavior is not
great, though (asking for "3G" would go negative and effectively be
ignored).
I think handling all cases would involve swapping out OPT_MAGNITUDE()
for a special callback that writes the "yes, the user set this" bit in a
separate variable.
-Peff
^ permalink raw reply
* Re: [PATCH] t2027: avoid using pipes
From: Christian Couder @ 2017-03-09 8:08 UTC (permalink / raw)
To: Prathamesh Chavan; +Cc: git
In-Reply-To: <0102015aae7b8536-00c57d0a-1d48-4153-a202-87c4ea9e0e19-000000@eu-west-1.amazonses.com>
On Wed, Mar 8, 2017 at 4:13 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> The exit code of the upstream of a pipe is ignored thus we should avoid
> using it.
You might want to say more specifically that we should avoid piping a
git command into another one as this could mask a failure of the git
command.
> By writing out the output of the git command to a file, we
> can test the exit codes of both the commands.
>
> Signed-off-by: Prathamesh <pc44800@gmail.com>
> ---
> t/t2027-worktree-list.sh | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f..daa7a04 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
> test_when_finished "rm -rf here && git worktree prune" &&
> git worktree add --detach here master &&
> echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
> - git worktree list | sed "s/ */ /g" >actual &&
> + git worktree list >out && sed "s/ */ /g" <out >actual &&
I think it's better if the 'sed' command is on a separate line.
Also you may have used just "out" instead of "<out" in the 'sed' command...
> test_cmp expect actual
> '
>
> @@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
> cd linked &&
> echo "worktree $(pwd)" >expected &&
> echo "ref: .broken" >../.git/HEAD &&
> - git worktree list --porcelain | head -n 3 >actual &&
> + git worktree list --porcelain >out && head -n 3 out >actual &&
... as above you use "out" not "<out" in the 'head' command.
> test_cmp ../expected actual &&
> - git worktree list | head -n 1 >actual.2 &&
> + git worktree list >out && head -n 1 out >actual.2 &&
> grep -F "(error)" actual.2
> )
> '
^ permalink raw reply
* Re: [PATCH] t2027: avoid using pipes
From: Prathamesh Chavan @ 2017-03-09 8:56 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD0GtRdjCMcbhjgA0rVaAMFtyto8JxfqbivODarBB0eK8w@mail.gmail.com>
On Thu, Mar 9, 2017 at 1:38 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Wed, Mar 8, 2017 at 4:13 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> The exit code of the upstream of a pipe is ignored thus we should avoid
>> using it.
>
> You might want to say more specifically that we should avoid piping a
> git command into another one as this could mask a failure of the git
> command.
Yes. I will add be specific, and update my patch.
>
>> By writing out the output of the git command to a file, we
>> can test the exit codes of both the commands.
>>
>> Signed-off-by: Prathamesh <pc44800@gmail.com>
>> ---
>> t/t2027-worktree-list.sh | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>> index 848da5f..daa7a04 100755
>> --- a/t/t2027-worktree-list.sh
>> +++ b/t/t2027-worktree-list.sh
>> @@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
>> test_when_finished "rm -rf here && git worktree prune" &&
>> git worktree add --detach here master &&
>> echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
>> - git worktree list | sed "s/ */ /g" >actual &&
>> + git worktree list >out && sed "s/ */ /g" <out >actual &&
>
> I think it's better if the 'sed' command is on a separate line.
>
> Also you may have used just "out" instead of "<out" in the 'sed' command...
>
Actually I noticed that:
$ git grep sed |grep "<" |wc -l
307
As at most places, wherever pipes aren't being used, the input to sed command is
passed using "<". Hence I chose to use "<" at places specifically at
places where sed
was used, even after knowing that just "out" will work.
>> test_cmp expect actual
>> '
>>
>> @@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
>> cd linked &&
>> echo "worktree $(pwd)" >expected &&
>> echo "ref: .broken" >../.git/HEAD &&
>> - git worktree list --porcelain | head -n 3 >actual &&
>> + git worktree list --porcelain >out && head -n 3 out >actual &&
>
> ... as above you use "out" not "<out" in the 'head' command.
>
>> test_cmp ../expected actual &&
>> - git worktree list | head -n 1 >actual.2 &&
>> + git worktree list >out && head -n 1 out >actual.2 &&
>> grep -F "(error)" actual.2
>> )
>> '
^ permalink raw reply
* [PATCH v2] t2027: avoid using pipes
From: Prathamesh Chavan @ 2017-03-09 9:39 UTC (permalink / raw)
To: git
In-Reply-To: <0102015aae7b8536-00c57d0a-1d48-4153-a202-87c4ea9e0e19-000000@eu-west-1.amazonses.com>
The exit code of the upstream of a pipe is ignored thus we should avoid
using it. By writing out the output of the git command to a file, we
can test the exit codes of both the commands.
Signed-off-by: Prathamesh <pc44800@gmail.com>
---
t/t2027-worktree-list.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f..daa7a04 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
test_when_finished "rm -rf here && git worktree prune" &&
git worktree add --detach here master &&
echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git worktree list | sed "s/ */ /g" >actual &&
+ git worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -40,7 +40,7 @@ test_expect_success '"list" all worktrees from linked' '
test_when_finished "rm -rf here && git worktree prune" &&
git worktree add --detach here master &&
echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C here worktree list | sed "s/ */ /g" >actual &&
+ git -C here worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -73,7 +73,7 @@ test_expect_success '"list" all worktrees from bare main' '
git -C bare1 worktree add --detach ../there master &&
echo "$(pwd)/bare1 (bare)" >expect &&
echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C bare1 worktree list | sed "s/ */ /g" >actual &&
+ git -C bare1 worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -96,7 +96,7 @@ test_expect_success '"list" all worktrees from linked with a bare main' '
git -C bare1 worktree add --detach ../there master &&
echo "$(pwd)/bare1 (bare)" >expect &&
echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C there worktree list | sed "s/ */ /g" >actual &&
+ git -C there worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
cd linked &&
echo "worktree $(pwd)" >expected &&
echo "ref: .broken" >../.git/HEAD &&
- git worktree list --porcelain | head -n 3 >actual &&
+ git worktree list --porcelain >out && head -n 3 out >actual &&
test_cmp ../expected actual &&
- git worktree list | head -n 1 >actual.2 &&
+ git worktree list >out && head -n 1 out >actual.2 &&
grep -F "(error)" actual.2
)
'
@@ -134,7 +134,7 @@ test_expect_success 'linked worktrees are sorted' '
test_commit new &&
git worktree add ../first &&
git worktree add ../second &&
- git worktree list --porcelain | grep ^worktree >actual
+ git worktree list --porcelain >out && grep ^worktree out >actual
) &&
cat >expected <<-EOF &&
worktree $(pwd)/sorted/main
--
https://github.com/git/git/pull/336
^ permalink raw reply related
* [PATCH v2] t2027: avoid using pipes
From: Prathamesh Chavan @ 2017-03-09 9:53 UTC (permalink / raw)
To: git
In-Reply-To: <0102015ab26fcf13-1659be12-a85c-47be-9a77-8f1b0b8a3897-000000@eu-west-1.amazonses.com>
Whenever a git command is present in the upstream of a pipe, its failure
gets masked by piping and hence it should be avoided for testing the
upstream git command. By writing out the output of the git command to
a file, we can test the exit codes of both the commands as a failure exit
code in any command is able to stop the && chain.
Signed-off-by: Prathamesh <pc44800@gmail.com>
---
t/t2027-worktree-list.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f..daa7a04 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
test_when_finished "rm -rf here && git worktree prune" &&
git worktree add --detach here master &&
echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git worktree list | sed "s/ */ /g" >actual &&
+ git worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -40,7 +40,7 @@ test_expect_success '"list" all worktrees from linked' '
test_when_finished "rm -rf here && git worktree prune" &&
git worktree add --detach here master &&
echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C here worktree list | sed "s/ */ /g" >actual &&
+ git -C here worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -73,7 +73,7 @@ test_expect_success '"list" all worktrees from bare main' '
git -C bare1 worktree add --detach ../there master &&
echo "$(pwd)/bare1 (bare)" >expect &&
echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C bare1 worktree list | sed "s/ */ /g" >actual &&
+ git -C bare1 worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -96,7 +96,7 @@ test_expect_success '"list" all worktrees from linked with a bare main' '
git -C bare1 worktree add --detach ../there master &&
echo "$(pwd)/bare1 (bare)" >expect &&
echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
- git -C there worktree list | sed "s/ */ /g" >actual &&
+ git -C there worktree list >out && sed "s/ */ /g" <out >actual &&
test_cmp expect actual
'
@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
cd linked &&
echo "worktree $(pwd)" >expected &&
echo "ref: .broken" >../.git/HEAD &&
- git worktree list --porcelain | head -n 3 >actual &&
+ git worktree list --porcelain >out && head -n 3 out >actual &&
test_cmp ../expected actual &&
- git worktree list | head -n 1 >actual.2 &&
+ git worktree list >out && head -n 1 out >actual.2 &&
grep -F "(error)" actual.2
)
'
@@ -134,7 +134,7 @@ test_expect_success 'linked worktrees are sorted' '
test_commit new &&
git worktree add ../first &&
git worktree add ../second &&
- git worktree list --porcelain | grep ^worktree >actual
+ git worktree list --porcelain >out && grep ^worktree out >actual
) &&
cat >expected <<-EOF &&
worktree $(pwd)/sorted/main
--
https://github.com/git/git/pull/336
^ permalink raw reply related
* Re: BUG Report: git branch ignore --no-abbrev flag
From: Guillaume Wenzek @ 2017-03-09 9:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <xmqqzigv1lc3.fsf@gitster.mtv.corp.google.com>
Thanks for looking into it,
The full version number is "2.12.0.246.ga2ecc84866" but I don't think
that's an official number, I'm using my company (Google) apt
repository.
The git package date is 2017/03/03 while previous version was from
2017/02/16, the commit you identified is actually between the two, so
that may be it.
On 8 March 2017 at 22:59, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Guillaume Wenzek <guillaume.wenzek@gmail.com> writes:
>>
>>> After updating to git 2.12.0 on Monday I noticed that the "git branch"
>>> wasn't behaving as usual.
>>
>> Are you sure you are trying 2.12? v2.12.0 and before should behave
>> the same way and honor --no-abbrev as far as I know.
>>
>> On the other hand, 'master' has 93e8cd8b ("Merge branch
>> 'kn/ref-filter-branch-list'", 2017-02-27), which seems to introduce
>> the regression.
>>
>> Karthik?
>
> I haven't fully checked if filter.abbrev is set correctly, but I
> noticed the output format is formulated without taking the value of
> filter.abbrev into account at all, so this is an attempt to fix
> that omission.
>
> I also notice that filter.abbrev is _ONLY_ used by builtin/branch.c and
> the actual ref-filter code does not have to know anything about it.
>
> We probably should eliminate filter.abbrev field from the structure
> and use a regular variable in builtin/branch.c and use it to pass
> the result of command line parsing from cmd_branch() down to
> build_format() as an argument.
>
> But that is outside the scope of regression fix.
>
>
> builtin/branch.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index cbaa6d03c0..537c47811a 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -335,9 +335,18 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
> branch_get_color(BRANCH_COLOR_CURRENT));
>
> if (filter->verbose) {
> + struct strbuf obname = STRBUF_INIT;
> +
> + if (filter->abbrev < 0)
> + strbuf_addf(&obname, "%%(objectname:short)");
> + else if (!filter->abbrev)
> + strbuf_addf(&obname, "%%(objectname)");
> + else
> + strbuf_addf(&obname, " %%(objectname:short=%d) ", filter->abbrev);
> +
> strbuf_addf(&local, "%%(align:%d,left)%%(refname:lstrip=2)%%(end)", maxwidth);
> strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> - strbuf_addf(&local, " %%(objectname:short=7) ");
> + strbuf_addf(&local, " %s ", obname.buf);
>
> if (filter->verbose > 1)
> strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> @@ -346,10 +355,12 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
> else
> strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
>
> - strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:lstrip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> - "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> + strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:lstrip=2)%%(end)%s"
> + "%%(if)%%(symref)%%(then) -> %%(symref:short)"
> + "%%(else) %s %%(contents:subject)%%(end)",
> branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
> - branch_get_color(BRANCH_COLOR_RESET));
> + branch_get_color(BRANCH_COLOR_RESET), obname.buf);
> + strbuf_release(&obname);
> } else {
> strbuf_addf(&local, "%%(refname:lstrip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> branch_get_color(BRANCH_COLOR_RESET));
^ permalink raw reply
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Jeff King @ 2017-03-09 10:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170308202025.17900-1-avarab@gmail.com>
On Wed, Mar 08, 2017 at 08:20:25PM +0000, Ævar Arnfjörð Bjarmason wrote:
> Change the branch & tag commands to have a --no-contains option in
> addition to their longstanding --contains options.
>
> The use-case I have for this is mainly to find the last-good rollout
> tag given a known-bad <commit>. Right given a hypothetically bad
> commit v2.10.1-3-gcf5c7253e0 now you can find that with this hacky
> one-liner:
>
> (./git tag -l 'v[0-9]*'; ./git tag -l 'v[0-9]*' --contains v2.10.1-3-gcf5c7253e0)|sort|uniq -c|grep -E '^ *1 '|awk '{print $2}'
>
> But with the --no-contains option you can now get the exact same
> output with:
>
> ./git tag -l 'v[0-9]*' --no-contains v2.10.1-3-gcf5c7253e0 | sort
I think that's a good goal.
I'm not sure about the name. I would have expected "--no-contains" to
reset the list of "--contains" commits to the empty set. That's an
option convention we've been slowly moving towards (e.g., with
OPT_STRING_LIST).
What you've added here _does_ match "--no-merged", though. I'm not sure
of the best way forward. At the very least, "--no-contains" is currently
an error, so you would not be changing existing behavior.
> Once I'd implemented this for "tag" it was easy enough to add it for
> "branch". I haven't added it to "for-each-ref" but that would be
> trivial if anyone cares, but that use-case would be even more obscure
> than adding it to "branch", so I haven't bothered.
I'd prefer to have it consistently in all three. We should be able to
tell people to use for-each-ref in their scripts, and that's harder if
it is missing features.
> The "describe" command also has a --contains option, but its semantics
> are unrelated to what tag/branch/for-each-ref use --contains for, and
> I don't see how a --no-contains option for it would make any sense.
Yeah, I think that feature is orthogonal.
> -static int commit_contains(struct ref_filter *filter, struct commit *commit)
> +static int commit_contains(struct ref_filter *filter, struct commit *commit, const int with_commit)
> {
> + struct commit_list *tmp = with_commit ? filter->with_commit : filter->no_commit;
> if (filter->with_commit_tag_algo)
> - return contains_tag_algo(commit, filter->with_commit);
> - return is_descendant_of(commit, filter->with_commit);
> + return contains_tag_algo(commit, tmp);
> + return is_descendant_of(commit, tmp);
> }
Perhaps it would be simpler if the caller just passed the right
commit_list rather than a flag. We unfortunately do still need to pass
the "filter" (for the algorithm field), but the caller is then:
if (filter->with_commit &&
!commit_contains(filter, filter->with_commit, commit))
return 0;
if (filter->no_commit &&
commit_contains(filter, filter->no_commit, commit))
return 0;
which avoids the 0/1 flag whose meaning is not immediately apparent at
the callsite. One day we can hopefully unify the two algorithms and
ditch the extra filter parameter.
I almost suggested that there simply be an option to invert the match
(like --invert-contains or something). But what you have here is more
flexible, if somebody ever wanted to do:
git tag --contains X --no-contains Y
That could be useful if a feature was introduced in X and then got buggy
in Y.
> @@ -1708,8 +1782,91 @@ run_with_limited_stack () {
>
> test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
>
> +# These are all the tags we've created above
> +cat >expect.no-contains <<EOF
> [...80 tags...]
> +EOF
That's a lot of tags, and I'd worry it makes the test a little brittle.
Can we limit the set of tags with a name-match? It shouldn't affect the
purpose of the test (the deep stack comes from traversing the commits,
not the number of tags).
-Peff
^ permalink raw reply
* Re: BUG Report: git branch ignore --no-abbrev flag
From: Jeff King @ 2017-03-09 10:38 UTC (permalink / raw)
To: Guillaume Wenzek; +Cc: Junio C Hamano, Karthik Nayak, git
In-Reply-To: <CAAvNd=gAvONS-XP2vOLExdWiwDXTPbB5beiXikwdig7xC5OKow@mail.gmail.com>
On Thu, Mar 09, 2017 at 10:44:42AM +0100, Guillaume Wenzek wrote:
> The full version number is "2.12.0.246.ga2ecc84866" but I don't think
> that's an official number, I'm using my company (Google) apt
> repository.
That's built from the commit at a2ecc84866, which is in the "next"
branch (and is 246 commits ahead of v2.12.0). So that explains why you
are seeing the behavior. The code _isn't_ in v2.12.0, but you're running
a much more recent version.
-Peff
^ permalink raw reply
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Ævar Arnfjörð Bjarmason @ 2017-03-09 10:41 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309100910.z4h7bwqzxw2xynyu@sigill.intra.peff.net>
On Thu, Mar 9, 2017 at 11:09 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 08, 2017 at 08:20:25PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the branch & tag commands to have a --no-contains option in
>> addition to their longstanding --contains options.
>>
>> The use-case I have for this is mainly to find the last-good rollout
>> tag given a known-bad <commit>. Right given a hypothetically bad
>> commit v2.10.1-3-gcf5c7253e0 now you can find that with this hacky
>> one-liner:
>>
>> (./git tag -l 'v[0-9]*'; ./git tag -l 'v[0-9]*' --contains v2.10.1-3-gcf5c7253e0)|sort|uniq -c|grep -E '^ *1 '|awk '{print $2}'
>>
>> But with the --no-contains option you can now get the exact same
>> output with:
>>
>> ./git tag -l 'v[0-9]*' --no-contains v2.10.1-3-gcf5c7253e0 | sort
>
> I think that's a good goal.
>
> I'm not sure about the name. I would have expected "--no-contains" to
> reset the list of "--contains" commits to the empty set. That's an
> option convention we've been slowly moving towards (e.g., with
> OPT_STRING_LIST).
>
> What you've added here _does_ match "--no-merged", though. I'm not sure
> of the best way forward. At the very least, "--no-contains" is currently
> an error, so you would not be changing existing behavior.
I initially started hacking this up as --not-contains, but after
briefly chatting with Christian about it off-list he suggested --no-*.
Since as you point out it's consistent with --no-merge. I have no
strong view on it, I just want the feature whatever the flag is
called.
>> Once I'd implemented this for "tag" it was easy enough to add it for
>> "branch". I haven't added it to "for-each-ref" but that would be
>> trivial if anyone cares, but that use-case would be even more obscure
>> than adding it to "branch", so I haven't bothered.
>
> I'd prefer to have it consistently in all three. We should be able to
> tell people to use for-each-ref in their scripts, and that's harder if
> it is missing features.
Agreed. I'd already hacked that up this morning for a v2. It works &
has tests at https://github.com/avar/git/tree/avar/no-contains-2
>> The "describe" command also has a --contains option, but its semantics
>> are unrelated to what tag/branch/for-each-ref use --contains for, and
>> I don't see how a --no-contains option for it would make any sense.
>
> Yeah, I think that feature is orthogonal.
*Nod* just adding a note about it in case anyone's puzzled about why
describe doesn't have --no-contains, elaborated & clarified this a bit
in my WIP v2.
>> -static int commit_contains(struct ref_filter *filter, struct commit *commit)
>> +static int commit_contains(struct ref_filter *filter, struct commit *commit, const int with_commit)
>> {
>> + struct commit_list *tmp = with_commit ? filter->with_commit : filter->no_commit;
>> if (filter->with_commit_tag_algo)
>> - return contains_tag_algo(commit, filter->with_commit);
>> - return is_descendant_of(commit, filter->with_commit);
>> + return contains_tag_algo(commit, tmp);
>> + return is_descendant_of(commit, tmp);
>> }
>
> Perhaps it would be simpler if the caller just passed the right
> commit_list rather than a flag. We unfortunately do still need to pass
> the "filter" (for the algorithm field), but the caller is then:
>
> if (filter->with_commit &&
> !commit_contains(filter, filter->with_commit, commit))
> return 0;
> if (filter->no_commit &&
> commit_contains(filter, filter->no_commit, commit))
> return 0;
>
> which avoids the 0/1 flag whose meaning is not immediately apparent at
> the callsite. One day we can hopefully unify the two algorithms and
> ditch the extra filter parameter.
My C rustyness is showing. Yeah that's much better, thanks, changed it
to that in my WIP v2.
> I almost suggested that there simply be an option to invert the match
> (like --invert-contains or something). But what you have here is more
> flexible, if somebody ever wanted to do:
>
> git tag --contains X --no-contains Y
Yeah that's really useful. E.g. this shows the branches I branched off
(or have locally) from 2.6..2.8:
$ ./git branch --contains v2.6.0 --no-contains v2.8.0
avar/monkeypatch-untracked-cache-disabled
avar/uc-notifs21
dturner/pclouds-watchman-noshm
But I'd expect this to show all the tags between the two:
$ ./git tag --contains v2.6.0 --no-contains v2.8.0
$
But it just returns an empty list. Manually disabling the
contains_tag_algo() path (i.e. effectively locally reverting your
ffc4b8012d) makes it "work", but of course it's much slower now. I
haven't dug into why it's not working yet.
Also I wonder if this should be an error:
$ ./git [tag|branch|for-each-ref] --contains A --no-contains A
I.e. when you give the same argument to both, this can never return
anything for obvious reasons.
>> @@ -1708,8 +1782,91 @@ run_with_limited_stack () {
>>
>> test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
>>
>> +# These are all the tags we've created above
>> +cat >expect.no-contains <<EOF
>> [...80 tags...]
>> +EOF
>
> That's a lot of tags, and I'd worry it makes the test a little brittle.
> Can we limit the set of tags with a name-match? It shouldn't affect the
> purpose of the test (the deep stack comes from traversing the commits,
> not the number of tags).
I'll make this less sucky in v2 somehow. I did it this way because no
existing test was checking all the tags we'd created at the end, so
this does that by proxy now, but I agree it's too verbose. Will fix
it.
^ permalink raw reply
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Jeff King @ 2017-03-09 10:46 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <CACBZZX53rMiB5-cA_7-SeU2Dt7d_Cr7_GgyC0rjQQPPf4qyCqw@mail.gmail.com>
On Thu, Mar 09, 2017 at 11:41:59AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I almost suggested that there simply be an option to invert the match
> > (like --invert-contains or something). But what you have here is more
> > flexible, if somebody ever wanted to do:
> >
> > git tag --contains X --no-contains Y
>
> Yeah that's really useful. E.g. this shows the branches I branched off
> (or have locally) from 2.6..2.8:
>
> $ ./git branch --contains v2.6.0 --no-contains v2.8.0
> avar/monkeypatch-untracked-cache-disabled
> avar/uc-notifs21
> dturner/pclouds-watchman-noshm
Oh, that's a clever application.
> But I'd expect this to show all the tags between the two:
>
> $ ./git tag --contains v2.6.0 --no-contains v2.8.0
> $
>
> But it just returns an empty list. Manually disabling the
> contains_tag_algo() path (i.e. effectively locally reverting your
> ffc4b8012d) makes it "work", but of course it's much slower now. I
> haven't dug into why it's not working yet.
I'm almost certain this is because the contains_tag_algo one doesn't
clean up the flag bits it sets on the commit objects. So running it
twice in the same process is going to give you nonsense results.
Coincidentally, I've been looking into resurrecting the cleaner approach
that I sent long ago:
http://public-inbox.org/git/20140625233429.GA20457@sigill.intra.peff.net/
But it's sufficiently complex that it's probably worth fixing the
existing algorithm to clean up its bits in the meantime.
> Also I wonder if this should be an error:
>
> $ ./git [tag|branch|for-each-ref] --contains A --no-contains A
>
> I.e. when you give the same argument to both, this can never return
> anything for obvious reasons.
It's clearly nonsense, but I don't think there's any need for it to be
an error. GIGO.
-Peff
^ permalink raw reply
* Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |
From: Zenobiusz Kunegunda @ 2017-03-09 11:01 UTC (permalink / raw)
To: René Scharfe; +Cc: git@vger.kernel.org
In-Reply-To: <7d947891-ce40-23e7-2bc7-0f76dee53665@web.de>
OK, I printed euids inside function calling getcwd().
Every single EUID is the same and have expected value. The same as any other local application run by this user. Permissions of every directory in the path are OK.
/bin/pwd -P inside directory returned exact path and exited with exit status 0.
Od: "René Scharfe" <l.s.r@web.de>
Do: "Zenobiusz Kunegunda" <zenobiusz.kunegunda@interia.pl>;
Wysłane: 18:38 Środa 2017-03-08
Temat: Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |
>
>> Am 06.03.2017 um 17:10 schrieb Zenobiusz Kunegunda:
>> OS: FreeBSD 10.3-STABLE
>>
>> Story: I was trying to install openproject using this manual
>> https://www.openproject.org/open-source/download/manual-installation-guide/
>>
>> Everything was fine till command $ bundle install --deployment
>> --without postgres sqlite development test therubyracer docker
>>
>> works witg git version: 1.9.5 ( branch from repo ) does not work with
>> git version: 2.10 ( branch from from repo ) 2.11 ( both from FreeBSD
>> and from git repository) 2.12 ( branch from repo )
>>
>> On another server that passed but there was npm problem.
>>
>> This is error for $ bundle install --deployment --without postgres
>> sqlite development test therubyracer docker
>
> I suspect you might get better responses from the makers of bundler
> (http://bundler.io/, http://groups.google.com/group/ruby-bundler).
>
>> Fetching dependency metadata from https://rubygems.org/. fatal: Could
>> not get current working directory: Permission denied
>>
>> Retrying `git fetch --force --quiet --tags
>> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7"`
>> due to error (2/4): Bundler::Source::Git::GitCommandError Git error:
>> command `git fetch --force --quiet --tags
>> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7"`
>> in directory
>> /usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/bundler/gems/awesome_nested_set-7bd473e845e2
>> has failed. If this error persists you could try removing the cache
>> directory
>> '/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7'fatal:
>> Could not get current working directory: Permission denied
>
> These long and repetitive messages make me feel dizzy. So git fetch was
> executed in
> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/bundler/gems/awesome_nested_set-7bd473e845e2"
> and reported that it was not allowed to get the current working
> directory, right? That's odd.
>
> I suspect that older versions of git ignored the error, used an empty
> string and went with that instead of an absolute path, but that's just a
> guess.
>
> Was git perhaps started by bundler under a different user ID? You could
> check that e.g. by printing the return value of geteuid(2).
>
> "/bin/pwd -P" executed in the same directory under the same user ID as
> git should error out and report "Permission denied" as well. Are the
> permissions bits and ACLs for that directory and its parents OK?
>
> Thanks,
> René
>
>
^ permalink raw reply
* Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |
From: Zenobiusz Kunegunda @ 2017-03-09 11:11 UTC (permalink / raw)
To: René Scharfe, git; +Cc: git@vger.kernel.org
In-Reply-To: <7d947891-ce40-23e7-2bc7-0f76dee53665@web.de>
OK, I printed euids inside function calling getcwd().
Every single EUID is the same and have expected value. The same as any
other local application run by this user. Permissions of every directory in
the path are OK.
/bin/pwd -P inside directory returned exact path and exited with exit
status 0.Od: "René Scharfe" <l.s.r@web.de>
Do: "Zenobiusz Kunegunda" <zenobiusz.kunegunda@interia.pl>;
Wysłane: 18:38 Środa 2017-03-08
Temat: Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |
>
>> Am 06.03.2017 um 17:10 schrieb Zenobiusz Kunegunda:
>> OS: FreeBSD 10.3-STABLE
>>
>> Story: I was trying to install openproject using this manual
>> https://www.openproject.org/open-source/download/manual-installation-guide/
>>
>> Everything was fine till command $ bundle install --deployment
>> --without postgres sqlite development test therubyracer docker
>>
>> works witg git version: 1.9.5 ( branch from repo ) does not work with
>> git version: 2.10 ( branch from from repo ) 2.11 ( both from FreeBSD
>> and from git repository) 2.12 ( branch from repo )
>>
>> On another server that passed but there was npm problem.
>>
>> This is error for $ bundle install --deployment --without postgres
>> sqlite development test therubyracer docker
>
> I suspect you might get better responses from the makers of bundler
> (http://bundler.io/, http://groups.google.com/group/ruby-bundler).
>
>> Fetching dependency metadata from https://rubygems.org/. fatal: Could
>> not get current working directory: Permission denied
>>
>> Retrying `git fetch --force --quiet --tags
>> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7"`
>> due to error (2/4): Bundler::Source::Git::GitCommandError Git error:
>> command `git fetch --force --quiet --tags
>> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7"`
>> in directory
>> /usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/bundler/gems/awesome_nested_set-7bd473e845e2
>> has failed. If this error persists you could try removing the cache
>> directory
>> '/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/cache/bundler/git/awesome_nested_set-209215f38dc7f6765d32201897f8688e973f4de7'fatal:
>> Could not get current working directory: Permission denied
>
> These long and repetitive messages make me feel dizzy. So git fetch was
> executed in
> "/usr/home/USER/openproject/vendor/bundle/ruby/2.2.0/bundler/gems/awesome_nested_set-7bd473e845e2"
> and reported that it was not allowed to get the current working
> directory, right? That's odd.
>
> I suspect that older versions of git ignored the error, used an empty
> string and went with that instead of an absolute path, but that's just a
> guess.
>
> Was git perhaps started by bundler under a different user ID? You could
> check that e.g. by printing the return value of geteuid(2).
>
> "/bin/pwd -P" executed in the same directory under the same user ID as
> git should error out and report "Permission denied" as well. Are the
> permissions bits and ACLs for that directory and its parents OK?
>
> Thanks,
> René
>
>
^ permalink raw reply
* Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
From: Johannes Schindelin @ 2017-03-09 11:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, René Scharfe, git, Stefan Beller,
Jeff King
In-Reply-To: <xmqq4lz331wb.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]
Hi,
On Wed, 8 Mar 2017, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> >> > diff --git a/abspath.c b/abspath.c
> >> > index 2f0c26e0e2c..b02e068aa34 100644
> >> > --- a/abspath.c
> >> > +++ b/abspath.c
> >> > @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
> >> > return strbuf_realpath(&realpath, path, 0);
> >> > }
> >> >
> >> > -char *real_pathdup(const char *path)
> >> > +char *real_pathdup(const char *path, int die_on_error)
> >>
> >> Adding a gentle variant (with the current implementation) and making
> >> real_pathdup() die on error would be nicer, as it doesn't require
> >> callers to pass magic flag values. Most cases use the dying variant,
> >> so such a patch would have to touch less places:
> >
> > I agree with Junio and Rene that a gentle version would make the api
> > slightly nicer (and more consistant with some of the other api's we
> > have in git).
> >
> > This is exactly what I should have done back when I originally made
> > the change. Sorry for missing this!
>
> While I agree that the shape of the code Rene gave us here is what we
> would have liked to have in the original, it is a bit too late for that.
>
> As I already mentioned, as a regression fix patch, I find what Dscho
> posted more sensible, because it makes it obvious that all existing
> callsites were looked at while constructing the patch and more
> importantly, it forces somebody to look at all the new callers of the
> function that were added by the topics in flight, by changing the
> func-signature and forcing compilation failure.
While I would have agreed earlier that René's patch looks less intrusive,
I have to point out that there would not have been any possible regression
if the original patch had introduced the die_on_error parameter. It would
have made the contract *obvious*.
The nicer API made the contract unobvious, and that was the reason that
the bug could hide.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 0/2] Fix crashes due to real_pathdup() potentially returning NULL
From: Johannes Schindelin @ 2017-03-09 11:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Brandon Williams, Stefan Beller
In-Reply-To: <20170308161742.lpbf6gn6qhoex7et@sigill.intra.peff.net>
Hi Peff,
On Wed, 8 Mar 2017, Jeff King wrote:
> On Wed, Mar 08, 2017 at 04:43:27PM +0100, Johannes Schindelin wrote:
>
> > We may want to consider fast-tracking this into v2.12.1, and to that
> > end, I would appreciate code reviews that focus on the correctness of
> > this patch and that try to consider undesired side effects.
>
> I don't see how it could be not-correct, in the sense that every caller
> now passes the die_on_error flag (restoring the original behavior)
> except for the one which clearly checks for a NULL return immediately
> afterward.
Indeed. The principal reason why I extended the function signature was so
that any bugs would become obvious.
> The only exception would be if there were new calls to real_pathdup()
> that did not originally use real_path(). But:
>
> # 7241764076 introduced real_pathdup
> git log -Sreal_pathdup 7241764076..
>
> shows only one other introduction, and it's just duplicating an existing
> call.
Thanks for digging that up. I really only looked at the existing code in
`master` to figure out whether the return values were checked against NULL
or not.
> It's possible that some of these _could_ handle the error case more
> gracefully (I already fixed one such case in 3a1345af2). But even if
> we wanted to do so, that should come separately on top of this patch.
> This can go to 'maint' as a regression fix, and then that gives a stable
> base for making potential improvements.
Fully agree.
Thank you for the thorough review,
Johannes
^ permalink raw reply
* Re: [PATCH v3 0/9] Fix the early config
From: Johannes Schindelin @ 2017-03-09 11:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Duy Nguyen
In-Reply-To: <xmqqinnj1jb6.fsf@gitster.mtv.corp.google.com>
Hi,
On Wed, 8 Mar 2017, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> Or are you discussing a more general issue, iow, anything that can
> >> work without repository (i.e. those who do _gently version of the
> >> setup and act on *nongit_ok) should pretend as if there were no
> >> (broken) repository and take the "no we are not in a repository"
> >> codepath?
> >
> > Yes, exactly. It would have been less confusing if I picked something
> > that passed nongit_ok. Like hash-object:
... or like testing the early config directly?
> > $ git init
> > $ echo content >file
> > $ git hash-object file
> > d95f3ad14dee633a758d2e331151e950dd13e4ed
> >
> > $ echo '[core]repositoryformatversion = 10' >.git/config
> > $ git hash-object file
> > warning: Expected git repo version <= 1, found 10
> > d95f3ad14dee633a758d2e331151e950dd13e4ed
> >
> > The warning is fine and reasonable here. But then:
> >
> > $ echo '[core]repositoryformatversion = foobar' >.git/config
> > $ git hash-object file
> > fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' in file .git/config: invalid unit
> >
> > That's wrong. We're supposed to be gentle. And ditto:
> >
> > $ echo '[co' >.git/config
> > $ git hash-object file
> > fatal: bad config line 1 in file .git/config
> >
> > Those last two should issue a warning at most, and then let the command
> > continue.
>
> Yeah, I agree with that as one of the worthy goals. IIUC, we
> decided to leave that outside of this series and later fix on top,
> which is fine by me, too.
How about this on top, then:
-- snipsnap --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH] t1309: document cases where we would want early config not to
die()
Jeff King came up with a couple examples that demonstrate how the new
read_early_config() that looks harder for the current .git/ directory
could die() in an undesirable way.
Let's add those cases to the test script, to document what we would like
to happen when early config encounters problems.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1309-early-config.sh | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 0c55dee514c..027eca63a3c 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -47,4 +47,29 @@ test_expect_success 'ceiling #2' '
test xdg = "$(cat output)"
'
+test_with_config ()
+{
+ rm -rf throwaway &&
+ git init throwaway &&
+ (
+ cd throwaway &&
+ echo "$*" >.git/config &&
+ test-config read_early_config early.config
+ )
+}
+
+test_expect_success 'ignore .git/ with incompatible repository version' '
+ test_with_config "[core]repositoryformatversion = 999999" 2>err &&
+ grep "warning:.* Expected git repo version <= [1-9]" err
+'
+
+test_expect_failure 'ignore .git/ with invalid repository version' '
+ test_with_config "[core]repositoryformatversion = invalid"
+'
+
+
+test_expect_failure 'ignore .git/ with invalid config' '
+ test_with_config "["
+'
+
test_done
--
2.12.0.windows.1.7.g94dafc3b124
^ permalink raw reply related
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Ævar Arnfjörð Bjarmason @ 2017-03-09 12:12 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309104657.7pwreyozxo2tdhk4@sigill.intra.peff.net>
On Thu, Mar 9, 2017 at 11:46 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 09, 2017 at 11:41:59AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I almost suggested that there simply be an option to invert the match
>> > (like --invert-contains or something). But what you have here is more
>> > flexible, if somebody ever wanted to do:
>> >
>> > git tag --contains X --no-contains Y
>>
>> Yeah that's really useful. E.g. this shows the branches I branched off
>> (or have locally) from 2.6..2.8:
>>
>> $ ./git branch --contains v2.6.0 --no-contains v2.8.0
>> avar/monkeypatch-untracked-cache-disabled
>> avar/uc-notifs21
>> dturner/pclouds-watchman-noshm
>
> Oh, that's a clever application.
>
>> But I'd expect this to show all the tags between the two:
>>
>> $ ./git tag --contains v2.6.0 --no-contains v2.8.0
>> $
>>
>> But it just returns an empty list. Manually disabling the
>> contains_tag_algo() path (i.e. effectively locally reverting your
>> ffc4b8012d) makes it "work", but of course it's much slower now. I
>> haven't dug into why it's not working yet.
>
> I'm almost certain this is because the contains_tag_algo one doesn't
> clean up the flag bits it sets on the commit objects. So running it
> twice in the same process is going to give you nonsense results.
Yeah indeed.
I tried to hack something up to avoid this, but the
lookup_commit_reference_gently() we call will return the same
object.parent pointer for two invocations, i.e. the underlying
{commit,object}.c API has a cache of objects it returns, couldn't find
some way to quickly make it burst that cache.
The other approach of making contains_tag_algo() itself detect that
it's been called before (or us passing a flag) and going around
setting commit.object.flags on everything to 0 seemed even more
brittle, particularly since I think between filter->with_commit &
filter->no_commit we might end up visiting different commits, so it's
not easy to just clear it.
I'm happy to hack on it given some pointers, will visit it again, but
for now unless I'm missing something obvious / you can point out some
way to hack this up I'll just submit v2 with the combination of
--contains & --no-contains dying with a TODO message.
The patch without that functionality is still really useful, and we
can implement that later.
> Coincidentally, I've been looking into resurrecting the cleaner approach
> that I sent long ago:
>
> http://public-inbox.org/git/20140625233429.GA20457@sigill.intra.peff.net/
>
> But it's sufficiently complex that it's probably worth fixing the
> existing algorithm to clean up its bits in the meantime.
>
>> Also I wonder if this should be an error:
>>
>> $ ./git [tag|branch|for-each-ref] --contains A --no-contains A
>>
>> I.e. when you give the same argument to both, this can never return
>> anything for obvious reasons.
>
> It's clearly nonsense, but I don't think there's any need for it to be
> an error. GIGO.
Yeah, make sense.
^ permalink raw reply
* Re: [PATCH v3 0/9] Fix the early config
From: Jeff King @ 2017-03-09 12:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Duy Nguyen
In-Reply-To: <alpine.DEB.2.20.1703091243260.3767@virtualbox>
On Thu, Mar 09, 2017 at 12:51:06PM +0100, Johannes Schindelin wrote:
> On Wed, 8 Mar 2017, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > >> Or are you discussing a more general issue, iow, anything that can
> > >> work without repository (i.e. those who do _gently version of the
> > >> setup and act on *nongit_ok) should pretend as if there were no
> > >> (broken) repository and take the "no we are not in a repository"
> > >> codepath?
> > >
> > > Yes, exactly. It would have been less confusing if I picked something
> > > that passed nongit_ok. Like hash-object:
>
> ... or like testing the early config directly?
I was trying to demonstrate that the problem existed already without
your patch series.
> -- snipsnap --
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Subject: [PATCH] t1309: document cases where we would want early config not to
> die()
>
> Jeff King came up with a couple examples that demonstrate how the new
> read_early_config() that looks harder for the current .git/ directory
> could die() in an undesirable way.
>
> Let's add those cases to the test script, to document what we would like
> to happen when early config encounters problems.
Yep, these all look fine.
-Peff
^ permalink raw reply
* Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()
From: Michael Haggerty @ 2017-03-09 12:24 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-8-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Keep repo-related path handling in one place. This will make it easier
> to add submodule/multiworktree support later.
>
> This automatically adds the "if submodule then use the submodule version
> of git_path" to other call sites too. But it does not mean those
> operations are sumodule-ready. Not yet.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 7b4ea4c56..72f4e1746 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -1251,10 +1263,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
> size_t path_baselen;
> int err = 0;
>
> - if (refs->submodule)
> - err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
> - else
> - strbuf_git_path(&path, "%s", dirname);
> + files_refname_path(refs, &path, dirname);
> path_baselen = path.len;
>
> if (err) {
I just noticed another thing. After this change, `err` is never set, so
the `if (err)` block (and `err` itself) can be deleted.
> [...]
Michael
^ 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