* Re: [PATCH] show-branch: fix crash with long ref name
From: Christian Couder @ 2017-02-14 21:29 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git, Maxim Kuvyrkov
In-Reply-To: <20170214195513.7zae6x22advkrms6@sigill.intra.peff.net>
On Tue, Feb 14, 2017 at 8:55 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > This fixes the problem, but I think we can simplify it quite a bit by
>> > using resolve_refdup(). Here's the patch series I ended up with:
>> >
>> > [1/3]: show-branch: drop head_len variable
>> > [2/3]: show-branch: store resolved head in heap buffer
>> > [3/3]: show-branch: use skip_prefix to drop magic numbers
Yeah, I noticed there were a number of things that could be improved
in the area, but I didn't want to spend too much time on this, so
thanks for this series.
>> Yes, the whole thing is my fault ;-) and I agree with what these
>> patches do.
>>
>> The second one lacks free(head) but I think that is OK; it is
>> something we allocate in cmd_*() and use pretty much thruout the
>> rest of the program.
>
> Yes, I actually tested the whole thing under ASAN (which was necessary
> to notice the problem), which complained about the leak. I don't mind
> adding a free(head), but there are a bunch of similar "leaks" in that
> function, so I didn't bother.
Yeah, I didn't bother either.
> I notice Christian's patch added a few tests. I don't know if we'd want
> to squash them in (I didn't mean to override his patch at all; I was
> about to send mine out when I noticed his, and I wondered if we wanted
> to combine the two efforts).
I think it would be nice to have at least one test. Feel free to
squash mine if you want.
^ permalink raw reply
* [PATCH v2 1/2] completion: extract utility to complete paths from tree-ish
From: cornelius.weig @ 2017-02-14 21:24 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, szeder.dev, bitte.keine.werbung.einwerfen, j6t
In-Reply-To: <4f8a0aaa-4ce1-d4a6-d2e1-28aac7209c90@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
The function __git_complete_revlist_file understands how to complete a
path such as 'topic:ref<TAB>'. In that case, the revision (topic) and
the path component (ref) are both part of the same word. However,
some cases require that the revision is specified elsewhere than the
current word for completion, such as 'git checkout topic ref<TAB>'.
In order to allow callers to specify the revision, extract a utility
function to complete paths from a tree-ish object. The utility will be
used later to implement path completion for git-checkout.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 73 +++++++++++++++++++---------------
1 file changed, 41 insertions(+), 32 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..4ab119d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -442,6 +442,46 @@ __git_compute_merge_strategies ()
__git_merge_strategies=$(__git_list_merge_strategies)
}
+# __git_complete_tree_file requires 2 argument:
+# 1: the the tree-like to look at for completion
+# 2: the path component to complete
+__git_complete_tree_file ()
+{
+ local pfx ls ref="$1" cur_="$2"
+ case "$cur_" in
+ ?*/*)
+ pfx="${cur_%/*}"
+ cur_="${cur_##*/}"
+ ls="$ref:$pfx"
+ pfx="$pfx/"
+ ;;
+ *)
+ ls="$ref"
+ ;;
+ esac
+
+ case "$COMP_WORDBREAKS" in
+ *:*) : great ;;
+ *) pfx="$ref:$pfx" ;;
+ esac
+
+ __gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
+ | sed '/^100... blob /{
+ s,^.* ,,
+ s,$, ,
+ }
+ /^120000 blob /{
+ s,^.* ,,
+ s,$, ,
+ }
+ /^040000 tree /{
+ s,^.* ,,
+ s,$,/,
+ }
+ s/^.* //')" \
+ "$pfx" "$cur_" ""
+}
+
__git_complete_revlist_file ()
{
local pfx ls ref cur_="$cur"
@@ -452,38 +492,7 @@ __git_complete_revlist_file ()
?*:*)
ref="${cur_%%:*}"
cur_="${cur_#*:}"
- case "$cur_" in
- ?*/*)
- pfx="${cur_%/*}"
- cur_="${cur_##*/}"
- ls="$ref:$pfx"
- pfx="$pfx/"
- ;;
- *)
- ls="$ref"
- ;;
- esac
-
- case "$COMP_WORDBREAKS" in
- *:*) : great ;;
- *) pfx="$ref:$pfx" ;;
- esac
-
- __gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
- | sed '/^100... blob /{
- s,^.* ,,
- s,$, ,
- }
- /^120000 blob /{
- s,^.* ,,
- s,$, ,
- }
- /^040000 tree /{
- s,^.* ,,
- s,$,/,
- }
- s/^.* //')" \
- "$pfx" "$cur_" ""
+ __git_complete_tree_file "$ref" "$cur_"
;;
*...*)
pfx="${cur_%...*}..."
--
2.10.2
^ permalink raw reply related
* [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: cornelius.weig @ 2017-02-14 21:24 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, szeder.dev, bitte.keine.werbung.einwerfen, j6t
In-Reply-To: <20170214212404.31469-1-cornelius.weig@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.
This commit ensures that after specifying a ref, further non-option
words are completed as paths. Four cases are considered:
- If the word contains a ':', do not treat it as reference and use
regular revlist completion.
- If no ref is found on the command line, complete non-options as refs
as before.
- If the ref is HEAD or @, complete only with modified files because
checking out unmodified files is a noop.
This case also applies if no ref is given, but '--' is present.
- If a ref other than HEAD or @ is found, offer only valid paths from
that revision.
Note that one corner-case is not covered by the current implementation:
if a refname contains a ':' and is followed by '--' the completion would
not recognize the valid refname.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 39 +++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4ab119d..df46f62 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1068,7 +1068,7 @@ _git_bundle ()
_git_checkout ()
{
- __git_has_doubledash && return
+ local i c=2 ref="" seen_double_dash=""
case "$cur" in
--conflict=*)
@@ -1081,13 +1081,36 @@ _git_checkout ()
"
;;
*)
- # check if --track, --no-track, or --no-guess was specified
- # if so, disable DWIM mode
- local flags="--track --no-track --no-guess" track=1
- if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
- track=''
- fi
- __gitcomp_nl "$(__git_refs '' $track)"
+ while [ $c -lt $cword ]; do
+ i="${words[c]}"
+ case "$i" in
+ --) seen_double_dash=1 ;;
+ -*|?*:*) ;;
+ *) ref="$i"; break ;;
+ esac
+ ((c++))
+ done
+
+ case "$ref,$seen_double_dash,$cur" in
+ ,,*:*)
+ __git_complete_revlist_file
+ ;;
+ ,,*)
+ # check for --track, --no-track, or --no-guess
+ # if so, disable DWIM mode
+ local flags="--track --no-track --no-guess" track=1
+ if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+ track=''
+ fi
+ __gitcomp_nl "$(__git_refs '' $track)"
+ ;;
+ ,1,*|@,*|HEAD,*)
+ __git_complete_index_file "--modified"
+ ;;
+ *)
+ __git_complete_tree_file "$ref" "$cur"
+ ;;
+ esac
;;
esac
}
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] completion: complete modified files for checkout with '--'
From: Cornelius Weig @ 2017-02-14 21:13 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list, j6t, Richard Wagner
In-Reply-To: <CAM0VKj=d+hAiF6_8TLuJfccNiPtHyg9F6zESA8SuTEeaLsrw4Q@mail.gmail.com>
On 02/14/2017 01:50 AM, SZEDER Gábor wrote:
> On Tue, Feb 14, 2017 at 12:33 AM, <cornelius.weig@tngtech.com> wrote:
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>>
>> The command line completion for git-checkout bails out when seeing '--'
>> as an isolated argument. For git-checkout this signifies the start of a
>> list of files which are to be checked out. Checkout of files makes only
>> sense for modified files,
>
> No, there is e.g. 'git checkout that-branch this-path', too.
Very true. Thanks for prodding me to this palpable oversight.
My error was to aim for a small improvement. I think the correct
approach is to improve the overall completion of git-checkout. IMHO it
is a completion bug that after giving a ref, completion will still offer
refs, e.g.
$ git checkout HEAD <TAB><TAB> --> list of refs
As far as I can see, giving two refs to checkout is always an error. The
correct behavior in the example above would be to offer paths instead.
I'll follow up with an improved version which considers these cases.
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Junio C Hamano @ 2017-02-14 21:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1702142150220.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 13 Feb 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > That is why I taught the Git for Windows CI job that tests the four
>> > upstream Git integration branches to *also* bisect test breakages and
>> > then upload comments to the identified commit on GitHub
>>
>> Good. I do not think it is useful to try 'pu' as an aggregate and
>> expect it to always build and work [*1*], but your "bisect and
>> pinpoint" approach makes it useful to identify individual topic that
>> brings in a breakage.
>
> Sadly the many different merge bases[*1*] between `next` and `pu` (which
> are the obvious good/bad points for bisecting automatically) bring my
> build agents to its knees. I may have to disable the bisecting feature as
> a consequence.
Probably a less resource intensive approach is to find the tips of
the topics not in 'next' but in 'pu' and test them. That would give
you which topic(s) are problematic, which is a better starting point
than "Oh, 'pu' does not build". After identifying which branch is
problematic, bisection of individual topic would be of more manageable
size.
$ git log --first-parent --oneline 'pu^{/^### match next}..pu'
will you the merges of topics left outside 'next'. I often reorder
to make the ones that look more OK than others closer to the bottom,
and if the breakages caused by them are caught earlier than they hit
'next', that would be ideal.
This is one of these times I wish "git bisect --first-parent" were
available. The above "log" gives me 27 merges right now, which
should be bisectable within 5 rounds to identify a single broken
topic (if there is only one breakage, that is).
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Johannes Schindelin @ 2017-02-14 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git-for-windows, git
In-Reply-To: <xmqq60kdbqmy.fsf@gitster.mtv.corp.google.com>
Hi,
On Mon, 13 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > That is why I taught the Git for Windows CI job that tests the four
> > upstream Git integration branches to *also* bisect test breakages and
> > then upload comments to the identified commit on GitHub
>
> Good. I do not think it is useful to try 'pu' as an aggregate and
> expect it to always build and work [*1*], but your "bisect and
> pinpoint" approach makes it useful to identify individual topic that
> brings in a breakage.
Sadly the many different merge bases[*1*] between `next` and `pu` (which
are the obvious good/bad points for bisecting automatically) bring my
build agents to its knees. I may have to disable the bisecting feature as
a consequence.
Ciao,
Johannes
Footnote *1*: There are currently 21, some of which stemming back from a
year ago. For bisecting, they all have to be tested individually, putting
a major dent into bisect's otherwise speedy process.
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Olaf Hering @ 2017-02-14 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh93w8q0r.fsf@gitster.mtv.corp.google.com>
Am Tue, 14 Feb 2017 12:40:36 -0800
schrieb Junio C Hamano <gitster@pobox.com>:
> Olaf Hering <olaf@aepfle.de> writes:
>
> > How is git send-email and git am supposed to handle a text file
> > which lacks a newline at the very end? This is about git 2.11.0.
>
> I think this has always worked, though.
For me it complains in line 721, which is the problematic one.
I try to apply from mutt via (cd /some/dir && git am), but that
probably does not make a difference.
How would I debug it?
Olaf
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Jeff King @ 2017-02-14 20:47 UTC (permalink / raw)
To: Olaf Hering; +Cc: git
In-Reply-To: <20170214201104.GA26407@aepfle.de>
On Tue, Feb 14, 2017 at 09:11:04PM +0100, Olaf Hering wrote:
> How is git send-email and git am supposed to handle a text file which
> lacks a newline at the very end? This is about git 2.11.0.
That workflow should handle this case, and the resulting applied patch
should not have a newline.
> Right now the patch in an email generated with 'git send-email' ends
> with '\ No newline at end of file', which 'git am' can not handle. To
> me it looks like whatever variant of "diff" is used does the right thing
> and indicates the lack of newline. Just the used variant of "patch" does
> not deal with it.
I can't reproduce here:
# new repo with nothing in it (the base commit is to have something to
# reset back to)
git init
git commit --allow-empty -m base
# our file with no trailing newline
printf foo >file
git add file
git commit -m no-newline
# now make a patch email; it should have the "\ No newline" bit at the
# end.
git format-patch -1
cat 0001-no-newline.patch
# and now reset back and try to apply it
git reset --hard HEAD^
git am 0001-no-newline.patch
# double check that it has no newline
xxd <file
I'm using format-patch instead of send-email, but that is the underlying
command that send-email is using. Is it possible that your patch is
getting munged during email transit in a way that destroy the "No
newline" message?
-Peff
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Junio C Hamano @ 2017-02-14 20:40 UTC (permalink / raw)
To: Olaf Hering; +Cc: git
In-Reply-To: <20170214201104.GA26407@aepfle.de>
Olaf Hering <olaf@aepfle.de> writes:
> How is git send-email and git am supposed to handle a text file which
> lacks a newline at the very end? This is about git 2.11.0.
I think this has always worked, though.
$ cd /var/tmp/x
$ git init am-incomplete-line
$ cd am-incomplete-line/
$ echo one line >file
$ git add file
$ git commit -a -m initial
[master (root-commit) 27b4668] initial
1 file changed, 1 insertion(+)
create mode 100644 file
$ echo -n an incomplete line >>file
$ git diff file
diff --git a/file b/file
index e3c0674..f2ec9f0 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
one line
+an incomplete line
\ No newline at end of file
$ git commit -a -m 'incomplete second'
[master 57075ab] incomplete second
1 file changed, 1 insertion(+)
$ git format-patch -1
0001-incomplete-second.txt
$ cat 0001-incomplete-second.txt
From 57075ab402e2d3714ebc9e2e9d4efd8dbfd74d5a Mon Sep 17 00:00:00 2001
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 14 Feb 2017 12:35:50 -0800
Subject: [PATCH] incomplete second
---
file | 1 +
1 file changed, 1 insertion(+)
diff --git a/file b/file
index e3c0674..f2ec9f0 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
one line
+an incomplete line
\ No newline at end of file
--
2.12.0-rc1-235-g2fb706ef99
$ git checkout HEAD^
$ git am ./0001-incomplete-second.txt
Applying: incomplete second
$ git diff master
$ exit
^ permalink raw reply
* [PATCH 2/2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2017-02-14 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net>
From: Jonathan Nieder <jrnieder@gmail.com>
To push from or fetch to the current repository, remote helpers need
to know what repository that is. Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.
There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository. GIT_DIR is not useful in this
scenario:
- if we are not in a repository, we don't need to set GIT_DIR to
override an existing GIT_DIR value from the environment. If GIT_DIR
is present then we would be in a repository if it were valid and
would have called die() if it weren't.
- not setting GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So in the worst case it may expend
a little extra effort to try to find a repository and fail (for
example, remote-curl would do this to try to find repository-level
configuration).
So leave GIT_DIR unset in this case. This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.
Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':
$ cd /tmp
$ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
fatal: BUG: setup_git_env called without repository
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I dropped this down to a single test instance, and used the nongit
helper to shorten it.
Possible patches on top:
- if we want to test this across more protocols, we can. I'm not sure
I see all that much value in it, given that we know the source of
the bug.
We probably _should_ have some kind of standard test-battery that
hits all protocols, or at the very least hits both dumb/smart http.
But waiting for that may be making the perfect the enemy of the
good. So I'm OK with doing it piece-wise for now if people really
feel we need to cover more protocols.
- Jonathan's original had some nice remote-ext tests, but they were
sufficiently complex that they should be spun into their own patch
with more explanation.
t/t5550-http-fetch-dumb.sh | 9 +++++++++
transport-helper.c | 5 +++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7..b69ece1d6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -34,6 +34,15 @@ test_expect_success 'clone http repository' '
test_cmp file clone/file
'
+test_expect_success 'list refs from outside any repository' '
+ cat >expect <<-EOF &&
+ $(git rev-parse master) HEAD
+ $(git rev-parse master) refs/heads/master
+ EOF
+ nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'create password-protected repository' '
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35eb..e4fd98238 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
- argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
- get_git_dir());
+ if (have_git_dir())
+ argv_array_pushf(&helper->env_array, "%s=%s",
+ GIT_DIR_ENVIRONMENT, get_git_dir());
code = start_command(helper);
if (code < 0 && errno == ENOENT)
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [RFC PATCH] show decorations at the end of the line
From: Stephan Beyer @ 2017-02-14 20:36 UTC (permalink / raw)
To: Junio C Hamano, Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <xmqq1sv2fq6m.fsf@gitster.mtv.corp.google.com>
Hi,
On 02/13/2017 09:30 AM, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> I've signed off on this, because I think it's an "obvious" improvement,
>>> but I'm putting the "RFC" in the subject line because this is clearly a
>>> subjective thing.
>>
>> Side note: the one downside of showing the decorations at the end of
>> the line is that now they are obviously at the end of the line - and
>> thus likely to be more hidden by things like line truncation.
>
> Side note: I refrained from commenting on this patch because
> everybody knows that the what I would say anyway ;-) and I didn't
> want to speak first to discourage others from raising their opinion.
A further side note: the current behavior of
git log --oneline --decorate
is equivalent to
git log --pretty='format:%C(auto)%h%d %s'
and Linus' preferred version is equivalent to
git log --pretty='format:%C(auto)%h %s%d'
Most Git users I know have their own favorite version of git log
--pretty=format:... sometimes with --graph as an alias ("git lg" or "git
logk" (because its output reminds of gitk) or something).
I don't know what the main benefit of this patch would be, but if it
gets accepted, it should probably be mentioned somewhere that the old
behavior is easily accessible using the line mentioned above.
Cheers
Stephan
^ permalink raw reply
* [PATCH 1/2] remote: avoid reading $GIT_DIR config in non-repo
From: Jeff King @ 2017-02-14 20:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net>
The "git ls-remote" command can be run outside of a
repository, but needs to look up configured remotes. The
config code is smart enough to handle this case itself, but
we also check the historical "branches" and "remotes" paths
in $GIT_DIR. The git_path() function causes us to blindly
look at ".git/remotes", even if we know we aren't in a git
repository.
For now, this is just an unlikely bug (you probably don't
have such a file if you're not in a repository), but it will
become more obvious once we merge b1ef400ee (setup_git_env:
avoid blind fall-back to ".git", 2016-10-20):
[now]
$ git ls-remote
fatal: No remote configured to list refs from.
[with b1ef400ee]
$ git ls-remote
fatal: BUG: setup_git_env called without repository
We can fix this by skipping these sources entirely when
we're outside of a repository.
The test is a little more complex than the demonstration
above. Rather than detect the correct behavior by parsing
the error message, we can actually set up a case where the
remote name we give is a valid repository, but b1ef400ee
would cause us to die in the configuration step.
This test doesn't fail now, but it future-proofs us for the
b1ef400ee change.
Signed-off-by: Jeff King <peff@peff.net>
---
remote.c | 2 +-
t/t5512-ls-remote.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index bf1bf2309..9f83fe2c4 100644
--- a/remote.c
+++ b/remote.c
@@ -693,7 +693,7 @@ static struct remote *remote_get_1(const char *name,
name = get_default(current_branch, &name_given);
ret = make_remote(name, 0);
- if (valid_remote_nick(name)) {
+ if (valid_remote_nick(name) && have_git_dir()) {
if (!valid_remote(ret))
read_remotes_file(ret);
if (!valid_remote(ret))
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 55fc83fc0..94fc9be9c 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -248,4 +248,13 @@ test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-complian
test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
'
+test_expect_success 'ls-remote works outside repository' '
+ # It is important for this repo to be inside the nongit
+ # area, as we want a repo name that does not include
+ # slashes (because those inhibit some of our configuration
+ # lookups).
+ nongit git init --bare dst.git &&
+ nongit git ls-remote dst.git
+'
+
test_done
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2017-02-14 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <xmqqtw7w8uay.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 14, 2017 at 11:08:05AM -0800, Junio C Hamano wrote:
> Thanks for prodding. I'm tempted to just rip out everything other
> than the 5-liner fix to transport.c and apply it, expecting that a
> follow-up patch with updated tests to come sometime not in too
> distant future.
I think we can at least include one basic test. Also, as it turns out
the problem I was seeing _wasn't_ the same one Jonathan fixed. There's
another bug. :)
So here's a patch series that fixes my bug, and then a cut-down version
of Jonathan's patch. I'd be happy to see more tests come on top. I don't
think there's a huge rush on getting any of this into master. There are
bugs in the existing code, but they're very hard to trigger in practice
(e.g., a non-repo which happens to have a bunch of repo-like files). It
only becomes an issue in 'next' when we die("BUG") to flush these cases
out.
[1/2]: remote: avoid reading $GIT_DIR config in non-repo
[2/2]: remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
remote.c | 2 +-
t/t5512-ls-remote.sh | 9 +++++++++
t/t5550-http-fetch-dumb.sh | 9 +++++++++
transport-helper.c | 5 +++--
4 files changed, 22 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply
* missing handling of "No newline at end of file" in git am
From: Olaf Hering @ 2017-02-14 20:11 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 441 bytes --]
How is git send-email and git am supposed to handle a text file which
lacks a newline at the very end? This is about git 2.11.0.
Right now the patch in an email generated with 'git send-email' ends
with '\ No newline at end of file', which 'git am' can not handle. To
me it looks like whatever variant of "diff" is used does the right thing
and indicates the lack of newline. Just the used variant of "patch" does
not deal with it.
Olaf
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-14 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christian Couder, git, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <xmqqlgt88t0r.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 14, 2017 at 11:35:48AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This fixes the problem, but I think we can simplify it quite a bit by
> > using resolve_refdup(). Here's the patch series I ended up with:
> >
> > [1/3]: show-branch: drop head_len variable
> > [2/3]: show-branch: store resolved head in heap buffer
> > [3/3]: show-branch: use skip_prefix to drop magic numbers
> >
> > builtin/show-branch.c | 39 ++++++++++++---------------------------
> > 1 file changed, 12 insertions(+), 27 deletions(-)
>
> Yes, the whole thing is my fault ;-) and I agree with what these
> patches do.
>
> The second one lacks free(head) but I think that is OK; it is
> something we allocate in cmd_*() and use pretty much thruout the
> rest of the program.
Yes, I actually tested the whole thing under ASAN (which was necessary
to notice the problem), which complained about the leak. I don't mind
adding a free(head), but there are a bunch of similar "leaks" in that
function, so I didn't bother.
I notice Christian's patch added a few tests. I don't know if we'd want
to squash them in (I didn't mean to override his patch at all; I was
about to send mine out when I noticed his, and I wondered if we wanted
to combine the two efforts).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers
From: Jeff King @ 2017-02-14 19:53 UTC (permalink / raw)
To: Pranit Bauva
Cc: Christian Couder, Git List, Junio C Hamano, Maxim Kuvyrkov,
Christian Couder
In-Reply-To: <CAFZEwPOYGRGd5PWSfLRd6vMs35TT1ZzUSfr79fwRA4VzVjAWXA@mail.gmail.com>
On Wed, Feb 15, 2017 at 12:23:52AM +0530, Pranit Bauva wrote:
> Did you purposely miss the one in line number 278 of
> builtin/show-branch.c because I think you only touched up the parts
> which were related to "refs/" but didn't explicitly mention it in the
> commit message?
>
> if (starts_with(pretty_str, "[PATCH] "))
> pretty_str += 8;
Not purposely. I was just fixing up the bits that I had noticed in the
earlier patches.
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index c03d3ec7c..19756595d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
> pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
> pretty_str = pretty.buf;
> }
> - if (starts_with(pretty_str, "[PATCH] "))
> - pretty_str += 8;
> + skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
Yeah, I agree this the same type of fix and is worth including. Thanks!
-Peff
^ permalink raw reply
* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
From: Jeff King @ 2017-02-14 19:51 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20170214185621.GC44208@google.com>
On Tue, Feb 14, 2017 at 10:56:21AM -0800, Brandon Williams wrote:
> On 02/14, Jeff King wrote:
> > - /* Check revs and then paths */
> > + /*
> > + * We have to find "--" in a separate pass, because its presence
> > + * influences how we will parse arguments that come before it.
> > + */
> > + for (i = 0; i < argc; i++) {
> > + if (!strcmp(argv[i], "--")) {
> > + seen_dashdash = 1;
> > + break;
> > + }
> > + }
>
> So this simply checks if "--" is an argument that was provided. This
> then allows grep to know ahead of time how to handle revs/paths
> preceding a "--" or in the absences of the "--". Seems sensible to me.
By the way, we have to check again later for "--" when parsing the revs
themselves. In theory you could set seen_dashdash to the offset of the
dashdash in the array, and do the iteration more like:
for (i = 0; i < dashdash_pos; i++)
handle_rev(argv[i]);
for (i = dashdash_pos + 1; i < argc; i++)
handle_path(argv[i]);
But our loops also handle the case where there is no "--" at all, and I
think that approach ends up convoluting the logic. I didn't go very far
in that direction before giving it up, though.
-Peff
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Junio C Hamano @ 2017-02-14 19:35 UTC (permalink / raw)
To: Jeff King; +Cc: Christian Couder, git, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This fixes the problem, but I think we can simplify it quite a bit by
> using resolve_refdup(). Here's the patch series I ended up with:
>
> [1/3]: show-branch: drop head_len variable
> [2/3]: show-branch: store resolved head in heap buffer
> [3/3]: show-branch: use skip_prefix to drop magic numbers
>
> builtin/show-branch.c | 39 ++++++++++++---------------------------
> 1 file changed, 12 insertions(+), 27 deletions(-)
Yes, the whole thing is my fault ;-) and I agree with what these
patches do.
The second one lacks free(head) but I think that is OK; it is
something we allocate in cmd_*() and use pretty much thruout the
rest of the program.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
From: Junio C Hamano @ 2017-02-14 19:18 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
In-Reply-To: <20170214060555.yzh6hhi2t7pkeqvi@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If we see "git grep pattern rev -- file" then we apply the
> usual rev/pathspec disambiguation rules: any "rev" before
> the "--" must be a revision, and we do not need to apply the
> verify_non_filename() check.
>
> But there are two bugs here:
>
> 1. We keep a seen_dashdash flag to handle this case, but
> we set it in the same left-to-right pass over the
> arguments in which we parse "rev".
>
> So when we see "rev", we do not yet know that there is
> a "--", and we mistakenly complain if there is a
> matching file.
>
> We can fix this by making a preliminary pass over the
> arguments to find the "--", and only then checking the rev
> arguments.
>
> 2. If we can't resolve "rev" but there isn't a dashdash,
> that's OK. We treat it like a path, and complain later
> if it doesn't exist.
>
> But if there _is_ a dashdash, then we know it must be a
> rev, and should treat it as such, complaining if it
> does not resolve. The current code instead ignores it
> and tries to treat it like a path.
>
> This patch fixes both bugs, and tries to comment the parsing
> flow a bit better.
Good. Thanks.
^ permalink raw reply
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Junio C Hamano @ 2017-02-14 19:08 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Duy Nguyen, Stefan Beller
In-Reply-To: <20170214061607.qyucfue335aqgji2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:
>
>> On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:
>>
>> > Thanks. Here's the patch again, now with commit messages and a test.
>> > Thanks for the analysis and sorry for the slow turnaround.
>>
>> Thanks for following up. While working on a similar one recently, I had
>> the nagging feeling that there was a case we had found that was still to
>> be dealt with, and I think this was it. :)
>>
>> The patch to the C code looks good. I have a few comments on the tests:
>
> I happened to run into this problem myself today, so I thought I'd prod
> you. I think your patch looks good. Hopefully I didn't scare you off
> with my comments. :)
Thanks for prodding. I'm tempted to just rip out everything other
than the 5-liner fix to transport.c and apply it, expecting that a
follow-up patch with updated tests to come sometime not in too
distant future.
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-14 18:58 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jeff Hostetler
In-Reply-To: <ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
>
>>>From my point of view, it is not crucial. The next Git for Windows version
>> will have it, of course, and Hannes is always running with his set of
>> patches, he can easily cherry-pick this one, too.
>
> The patch fixes the problem for me here.
>
> I am a bit hesitant to call it "not crucial": The symptom I observed
> is certainly not a show stopper; but who knows where else it is
> important that stderr goes to the terminal earlier than other
> output. As it is a new regression, it should be included in the next
> release.
I tend to agree with you on the "not crucial" bit.
I applied the patch with "Tested-by: Johannes Sixt <j6t@kdbg.org>"
on top of 1d3f065e0e ("mingw: follow-up to "replace isatty() hack"",
2017-01-18), which was the tip of js/mingw-isatty topic that was
merged to both master and maint during this cycle. Unless I hear
the "fix" turns out to be undesirable in the coming few days, let's
plan to merge it to 'master' by the final.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
From: Brandon Williams @ 2017-02-14 18:56 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20170214060555.yzh6hhi2t7pkeqvi@sigill.intra.peff.net>
On 02/14, Jeff King wrote:
> - /* Check revs and then paths */
> + /*
> + * We have to find "--" in a separate pass, because its presence
> + * influences how we will parse arguments that come before it.
> + */
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + seen_dashdash = 1;
> + break;
> + }
> + }
So this simply checks if "--" is an argument that was provided. This
then allows grep to know ahead of time how to handle revs/paths
preceding a "--" or in the absences of the "--". Seems sensible to me.
> +
> + /*
> + * Resolve any rev arguments. If we have a dashdash, then everything up
> + * to it must resolve as a rev. If not, then we stop at the first
> + * non-rev and assume everything else is a path.
> + */
> for (i = 0; i < argc; i++) {
> const char *arg = argv[i];
> unsigned char sha1[20];
> @@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>
> if (!strcmp(arg, "--")) {
> i++;
> - seen_dashdash = 1;
> break;
> }
>
> - /* Stop at the first non-rev */
> - if (get_sha1_with_context(arg, 0, sha1, &oc))
> + if (get_sha1_with_context(arg, 0, sha1, &oc)) {
> + if (seen_dashdash)
> + die(_("unable to resolve revision: %s"), arg);
> break;
> + }
>
> object = parse_object_or_die(sha1, arg);
> if (!seen_dashdash)
> @@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
> }
>
> - /* The rest are paths */
> + /*
> + * Anything left over is presumed to be a path. But in the non-dashdash
> + * "do what I mean" case, we verify and complain when that isn't true.
> + */
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers
From: Pranit Bauva @ 2017-02-14 18:53 UTC (permalink / raw)
To: Jeff King
Cc: Christian Couder, Git List, Junio C Hamano, Maxim Kuvyrkov,
Christian Couder
In-Reply-To: <20170214172829.6w3cnnqy6ozxl424@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]
Hey Peff,
On Tue, Feb 14, 2017 at 10:58 PM, Jeff King <peff@peff.net> wrote:
> We make several starts_with() calls, only to advance
> pointers. This is exactly what skip_prefix() is for, which
> lets us avoid manually-counted magic numbers.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/show-branch.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 404c4d09a..c03d3ec7c 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -470,17 +470,14 @@ static void snarf_refs(int head, int remotes)
> }
> }
>
> -static int rev_is_head(char *head, char *name,
> +static int rev_is_head(const char *head, const char *name,
> unsigned char *head_sha1, unsigned char *sha1)
> {
> if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
> return 0;
> - if (starts_with(head, "refs/heads/"))
> - head += 11;
> - if (starts_with(name, "refs/heads/"))
> - name += 11;
> - else if (starts_with(name, "heads/"))
> - name += 6;
> + skip_prefix(head, "refs/heads/", &head);
> + if (!skip_prefix(name, "refs/heads/", &name))
> + skip_prefix(name, "heads/", &name);
> return !strcmp(head, name);
> }
>
> @@ -799,8 +796,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
> has_head++;
> }
> if (!has_head) {
> - int offset = starts_with(head, "refs/heads/") ? 11 : 0;
> - append_one_rev(head + offset);
> + const char *name = head;
> + skip_prefix(name, "refs/heads/", &name);
> + append_one_rev(name);
> }
> }
>
Did you purposely miss the one in line number 278 of
builtin/show-branch.c because I think you only touched up the parts
which were related to "refs/" but didn't explicitly mention it in the
commit message?
if (starts_with(pretty_str, "[PATCH] "))
pretty_str += 8;
If not, you can squash this patch attached. Sorry, couldn't send it in
mail because of proxy issues.
Regards,
Pranit Bauva
[-- Attachment #2: 0001-squash-show-branch-use-skip_prefix-to-drop-magic-num.patch --]
[-- Type: text/x-patch, Size: 868 bytes --]
From 2e80d4458df659765011450621ee34459dc749f9 Mon Sep 17 00:00:00 2001
From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Tue, 14 Feb 2017 23:53:36 +0530
Subject: [PATCH] !squash: show-branch: use skip_prefix to drop magic numbers
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
builtin/show-branch.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index c03d3ec7c..19756595d 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
pretty_str = pretty.buf;
}
- if (starts_with(pretty_str, "[PATCH] "))
- pretty_str += 8;
+ skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
if (!no_name) {
if (name && name->head_name) {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-14 18:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702141545380.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> OK. Should this go directly to 'master', as the isatty thing is
>> already in?
>
> From my point of view, it is not crucial. The next Git for Windows version
> will have it, of course, and Hannes is always running with his set of
> patches, he can easily cherry-pick this one, too.
What hat were you wearing when you said the above "From my point of
view"? Were you the "Git for Windows" maintainer, or were you a
contributor and a member of the Git development community that works
to improve the upstream? If it was not clear, I was asking the
question to you wearing the latter hat.
To put it differently, what is our position, as the upstream
developers, toward those who are on Windows and wants to build from
the source? It's not just Hannes.
Is our position "unless you are extremely competent and are willing
to cherry-pick missing things from Git for Windows tree as needed,
we recommend you to build Git for Windows source instead"?
It is inevitable that the upstream lags behind in Windows specific
changes. Even though you have been trickling Windows specific
changes (both things in compat/ and also changes to the generic
parts, updating "c == '/'" to "is_dir_sep(c)") in, and I have been
accepting them for the past few years, in order to reduce the size
of the patch pile Git for Windows needs on top of the upstream,
until the patch pile becomes empty, it will always be the case.
So I won't object if that were our position. I just need to know
what it is to adjust my expectations, and as far as I am concerned,
you and Hannes are the two people whose thinking I'd trust regarding
what to do with/to Windows users; even though I keep saying "our"
position, I am asking yours and Hannes's.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/7] grep: re-order rev-parsing loop
From: Brandon Williams @ 2017-02-14 18:48 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20170214060417.yq27zdcgmrckjmua@sigill.intra.peff.net>
On 02/14, Jeff King wrote:
> - /* Is it a rev? */
> - if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
> - struct object *object = parse_object_or_die(sha1, arg);
> - if (!seen_dashdash)
> - verify_non_filename(prefix, arg);
> - add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
> - continue;
> - }
> - break;
> +
> + /* Stop at the first non-rev */
> + if (get_sha1_with_context(arg, 0, sha1, &oc))
> + break;
> +
> + object = parse_object_or_die(sha1, arg);
> + if (!seen_dashdash)
> + verify_non_filename(prefix, arg);
> + add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
This is much more readable!
--
Brandon Williams
^ 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