* 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
* Re: [PATCH 1/7] grep: move thread initialization a little lower
From: Brandon Williams @ 2017-02-14 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
In-Reply-To: <20170214060237.gvzzbmjzerlqnzjv@sigill.intra.peff.net>
On 02/14, Jeff King wrote:
> Originally, we set up the threads for grep before parsing
> the non-option arguments. In 53b8d931b (grep: disable
> threading in non-worktree case, 2011-12-12), the thread code
> got bumped lower in the function because it now needed to
> know whether we got any revision arguments.
>
> That put a big block of code in between the parsing of revs
> and the parsing of pathspecs, both of which share some loop
> variables. That makes it harder to read the code than the
> original, where the shared loops were right next to each
> other.
>
> Let's bump the thread initialization until after all of the
> parsing is done.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I double-checked to make sure no other code was relying on
> the thread setup having happened. I think we could actually
> bump it quite a bit lower (to right before we actually start
> grepping), but I doubt it matters much in practice.
Looks good. And yes I don't believe anything needs the thread
initialization to happen earlier.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Sixt @ 2017-02-14 18:45 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702141545380.3496@virtualbox>
Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> On Mon, 13 Feb 2017, Junio C Hamano wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>> When removing the hack for isatty(), we actually removed more than just
>>> an isatty() hack: we removed the hack where internal data structures of
>>> the MSVC runtime are modified in order to redirect stdout/stderr.
>>>
>>> Instead of using that hack (that does not work with newer versions of
>>> the runtime, anyway), we replaced it by reopening the respective file
>>> descriptors.
>>>
>>> What we forgot was to mark stderr as unbuffered again.
I do not see how the earlier patch turned stderr from unbuffered to
buffered, as it did not add or remove any setvbuf() call. Can you explain?
>> 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.
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.
Thanks,
-- Hannes
^ permalink raw reply
* Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Stefan Beller @ 2017-02-14 18:43 UTC (permalink / raw)
To: Duy Nguyen
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CACsJy8ChQqUd4poeeKQruQSwdys=ydzxGDC8fU6ZgfrFEEm7NQ@mail.gmail.com>
On Tue, Feb 14, 2017 at 2:04 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Feb 14, 2017 at 6:55 AM, Stefan Beller <sbeller@google.com> wrote:
>>>
>>> +/*
>>> + * Return the ref_store instance for the specified submodule. For the
>>> + * main repository, use submodule==NULL; such a call cannot fail.
>>
>> So now we have both a get_main as well as a get_submodule function,
>> but the submodule function can return the main as well?
>>
>> I'd rather see this as a BUG; or asking another way:
>> What is the difference between get_submodule_ref_store(NULL)
>> and get_main_ref_store() ?
>
> Technical debts :) In order to do that, we need to make sure
> _submodule() never takes NULL as main repo. But current code does. On
> example is revision.c where submodule==NULL is the main repo. In the
> end, I agree that get_submodule_ref_store(NULL) should be a bug.
>
>> As you went through all call sites (by renaming the function), we'd
>> be able to tell that there is no caller with NULL, or is it?
>
> Direct call sites are just middle men though, e.g.
> for_each_ref_submodule(). I'll attempt to clean this up at some point
> in future (probably at the same time I attempt to kill *_submodule ref
> api). But I think for now I'll just put a TODO or FIXME comment here.
ok, fine with me.
My line of thinking when originally responding was to put the FIXME
comment at the caller site, and there we'd differentiate between the
two cases and call the appropriate function.
> --
> Duy
^ permalink raw reply
* Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Junio C Hamano @ 2017-02-14 18:24 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stefan Beller, git@vger.kernel.org, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CACsJy8ChQqUd4poeeKQruQSwdys=ydzxGDC8fU6ZgfrFEEm7NQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Direct call sites are just middle men though, e.g.
> for_each_ref_submodule(). I'll attempt to clean this up at some point
> in future (probably at the same time I attempt to kill *_submodule ref
> api). But I think for now I'll just put a TODO or FIXME comment here.
So we'd have get_*_ref_store() for various cases and then will have
a unifying for_each_ref_in_refstore() that takes a ref-store, which
supersedes all the ad-hoc iterators like for_each_ref_submodule(),
for_each_namespaced_ref(), etc?
That's a very sensible longer-term goal, methinks.
I am wondering what we should do to for_each_{tag,branch,...}_ref().
Would that become
ref_store = get_ref_main_store();
tag_ref_store = filter_ref_store(ref_store, "refs/tags/");
for_each_ref_in_refstore(tag_ref_store, ...);
or do you plan to have some other pattern?
^ permalink raw reply
* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
From: Jonathan Tan @ 2017-02-14 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20170214180453.tpze2hdv3eytxfju@sigill.intra.peff.net>
On 02/14/2017 10:04 AM, Jeff King wrote:
> On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:
>
>> On 02/13/2017 10:07 PM, Jeff King wrote:
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index e83b33bda..c4c632594 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>> break;
>>> }
>>>
>>> + if (!use_index) {
>>> + if (seen_dashdash)
>>> + die(_("--no-index cannot be used with revs"));
>>
>> There is a subsequent check that prints "--no-index or --untracked cannot be
>> used with revs." - maybe we should just expand this part to incorporate that
>> case. (That is, write `if (!use_index || untracked)` instead of `if
>> (!use_index)`.) This also allows us to preserve the error message, which
>> might be useful for someone using a translated version of Git.
>
> I wasn't sure if we wanted to treat "untracked" in the same way.
> Certainly we can catch the error here for the seen_dashdash case, but is
> it also the case that:
>
> echo content >master
> git grep --untracked pattern master
>
> should treat "master" as a path?
>
> -Peff
It is already the case that it cannot be treated as a rev:
$ git grep --untracked pattern master
fatal: --no-index or --untracked cannot be used with revs.
So I think it would be better if it was treated as a path, for
consistency with --no-index. I'm OK either way, though.
^ permalink raw reply
* git-p4.py caching
From: Martin-Louis Bright @ 2017-02-14 18:16 UTC (permalink / raw)
To: git
hi!
I am using git-p4.py to migrate a lot of medium and large Perforce
depots into git. I almost exclusively go one way: from Perforce to
git. I also frequently re-clone/re-migrate as the Perforce migration
client spec is refined.
For this, I have added rudimentary caching in git-p4.py so that
Perforce requests are not repeated over the network.
I have it working and tested on a ~150MB repo and the migration time was halved.
Is this something that would be of interest to the larger community?
--Martin
^ permalink raw reply
* Re: [PATCH v2] clean: use warning_errno() when appropriate
From: Junio C Hamano @ 2017-02-14 18:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King
In-Reply-To: <20170214095449.15585-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> All these warning() calls are preceded by a system call. Report the
> actual error to help the user understand why we fail to remove
> something.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> v2 dances with errno
Thanks.
>
> builtin/clean.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaae..3569736f6 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> struct strbuf quoted = STRBUF_INIT;
> struct dirent *e;
> int res = 0, ret = 0, gone = 1, original_len = path->len, len;
> + int saved_errno;
> struct string_list dels = STRING_LIST_INIT_DUP;
>
> *dir_gone = 1;
> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> if (!dir) {
> /* an empty dir could be removed even if it is unreadble */
> res = dry_run ? 0 : rmdir(path->buf);
> + saved_errno = errno;
> if (res) {
> quote_path_relative(path->buf, prefix, "ed);
I think this part should be more like
res = ... : rmdir(...);
if (res) {
int saved_errno = errno;
... do other things that can touch errno ...
errno = saved_errno;
... now we know what the original error was ...
The reason to store the errno in saved_errno here is not because we
want to help code after "if (res) {...}", but the patch sent as-is
gives that impression and is confusing to the readers.
Perhaps all hunks of this patch share the same issue? I could
locally amend, of course, but I'd like to double check before doing
so myself---perhaps you did it this way for a good reason that I am
missing?
^ permalink raw reply
* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
From: Jeff King @ 2017-02-14 18:04 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <eef97cc4-d616-b298-bc99-b2772b757190@google.com>
On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:
> On 02/13/2017 10:07 PM, Jeff King wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index e83b33bda..c4c632594 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> > break;
> > }
> >
> > + if (!use_index) {
> > + if (seen_dashdash)
> > + die(_("--no-index cannot be used with revs"));
>
> There is a subsequent check that prints "--no-index or --untracked cannot be
> used with revs." - maybe we should just expand this part to incorporate that
> case. (That is, write `if (!use_index || untracked)` instead of `if
> (!use_index)`.) This also allows us to preserve the error message, which
> might be useful for someone using a translated version of Git.
I wasn't sure if we wanted to treat "untracked" in the same way.
Certainly we can catch the error here for the seen_dashdash case, but is
it also the case that:
echo content >master
git grep --untracked pattern master
should treat "master" as a path?
-Peff
^ permalink raw reply
* Re: [PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()
From: Stefan Beller @ 2017-02-14 17:40 UTC (permalink / raw)
To: Duy Nguyen
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CACsJy8D=qFQ2_62e4oO1pSBz4JfZV4Zcoai=Ghjw5DTaNxrwog@mail.gmail.com>
On Tue, Feb 14, 2017 at 1:40 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Feb 14, 2017 at 5:37 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> All refs outside refs/ directory is per-worktree, not just HEAD.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>> refs/refs-internal.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>>> index f4aed49f5..69d02b6ba 100644
>>> --- a/refs/refs-internal.h
>>> +++ b/refs/refs-internal.h
>>> @@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
>>>
>>> static inline int is_per_worktree_ref(const char *refname)
>>> {
>>> - return !strcmp(refname, "HEAD") ||
>>> + return !starts_with(refname, "refs/") ||
>>> starts_with(refname, "refs/bisect/");
>>
>> you're loosing HEAD here? (assuming we get HEAD in
>> short form here, as well as long form refs/HEAD)
>
> I don't understand. if refname is HEAD then both !strcmp(...) and
> !starts_with(refname, "refs/") return 1. If it's refs/HEAD, both
> return 0. In other words, there's no functional changes?
eh, my bad. You're right.
^ permalink raw reply
* [PATCH 3/3] show-branch: use skip_prefix to drop magic numbers
From: Jeff King @ 2017-02-14 17:28 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Junio C Hamano, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net>
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);
}
}
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* [PATCH 2/3] show-branch: store resolved head in heap buffer
From: Jeff King @ 2017-02-14 17:27 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Junio C Hamano, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net>
We resolve HEAD and copy the result to a fixed-size buffer
with memcpy, never checking that it actually fits. This bug
dates back to 8098a178b (Add git-symbolic-ref, 2005-09-30).
Before that we used readlink(), which took a maximum buffer
size.
We can fix this by using resolve_refdup(), which duplicates
the buffer on the heap. That also lets us just check
for a NULL pointer to see if we have resolved HEAD, and
drop the extra head_p variable.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/show-branch.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e4c488b8c..404c4d09a 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -473,8 +473,7 @@ static void snarf_refs(int head, int remotes)
static int rev_is_head(char *head, char *name,
unsigned char *head_sha1, unsigned char *sha1)
{
- if ((!head[0]) ||
- (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
+ if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
return 0;
if (starts_with(head, "refs/heads/"))
head += 11;
@@ -620,8 +619,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
int all_heads = 0, all_remotes = 0;
int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
- char head[128];
- const char *head_p;
+ char *head;
struct object_id head_oid;
int merge_base = 0;
int independent = 0;
@@ -786,17 +784,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
snarf_refs(all_heads, all_remotes);
}
- head_p = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
- head_oid.hash, NULL);
- if (head_p) {
- size_t head_len = strlen(head_p);
- memcpy(head, head_p, head_len + 1);
- }
- else {
- head[0] = 0;
- }
+ head = resolve_refdup("HEAD", RESOLVE_REF_READING,
+ head_oid.hash, NULL);
- if (with_current_branch && head_p) {
+ if (with_current_branch && head) {
int has_head = 0;
for (i = 0; !has_head && i < ref_name_cnt; i++) {
/* We are only interested in adding the branch
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* [PATCH 1/3] show-branch: drop head_len variable
From: Jeff King @ 2017-02-14 17:26 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Junio C Hamano, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214172526.hzpm3d3ubd3vjnzr@sigill.intra.peff.net>
We copy the result of resolving HEAD into a buffer and keep
track of its length. But we never actually use the length
for anything besides the copy. Let's stop passing it around.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/show-branch.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f3403a..e4c488b8c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -470,7 +470,7 @@ static void snarf_refs(int head, int remotes)
}
}
-static int rev_is_head(char *head, int headlen, char *name,
+static int rev_is_head(char *head, char *name,
unsigned char *head_sha1, unsigned char *sha1)
{
if ((!head[0]) ||
@@ -622,7 +622,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
char head[128];
const char *head_p;
- int head_len;
struct object_id head_oid;
int merge_base = 0;
int independent = 0;
@@ -790,11 +789,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
head_p = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
head_oid.hash, NULL);
if (head_p) {
- head_len = strlen(head_p);
+ size_t head_len = strlen(head_p);
memcpy(head, head_p, head_len + 1);
}
else {
- head_len = 0;
head[0] = 0;
}
@@ -805,7 +803,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
* HEAD points at.
*/
if (rev_is_head(head,
- head_len,
ref_name[i],
head_oid.hash, NULL))
has_head++;
@@ -866,7 +863,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
for (i = 0; i < num_rev; i++) {
int j;
int is_head = rev_is_head(head,
- head_len,
ref_name[i],
head_oid.hash,
rev[i]->object.oid.hash);
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-14 17:25 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Junio C Hamano, Maxim Kuvyrkov, Christian Couder
In-Reply-To: <20170214154816.12625-1-chriscool@tuxfamily.org>
On Tue, Feb 14, 2017 at 04:48:16PM +0100, Christian Couder wrote:
> @@ -791,11 +791,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
> head_oid.hash, NULL);
> if (head_p) {
> head_len = strlen(head_p);
> - memcpy(head, head_p, head_len + 1);
> + head_cpy = xstrdup(head_p);
> }
> else {
> head_len = 0;
> - head[0] = 0;
> + head_cpy = xstrdup("");
> }
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(-)
-Peff
^ permalink raw reply
* Re: [PATCH 0/7] grep rev/path parsing fixes
From: Jonathan Tan @ 2017-02-14 16:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
On 02/13/2017 10:00 PM, Jeff King wrote:
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
>
> [1/7]: grep: move thread initialization a little lower
> [2/7]: grep: do not unnecessarily query repo for "--"
> [3/7]: t7810: make "--no-index --" test more robust
> [4/7]: grep: re-order rev-parsing loop
> [5/7]: grep: fix "--" rev/pathspec disambiguation
> [6/7]: grep: avoid resolving revision names in --no-index case
> [7/7]: grep: do not diagnose misspelt revs with --no-index
Thanks - these look good to me. I replied to 6/7 with a comment, but I
also think that these are good as-is. Also, 3/7 can probably be squashed in.
^ permalink raw reply
* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
From: Jonathan Tan @ 2017-02-14 16:53 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20170214060729.v4r24y5tuaov3jrh@sigill.intra.peff.net>
On 02/13/2017 10:07 PM, Jeff King wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index e83b33bda..c4c632594 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> break;
> }
>
> + if (!use_index) {
> + if (seen_dashdash)
> + die(_("--no-index cannot be used with revs"));
There is a subsequent check that prints "--no-index or --untracked
cannot be used with revs." - maybe we should just expand this part to
incorporate that case. (That is, write `if (!use_index || untracked)`
instead of `if (!use_index)`.) This also allows us to preserve the error
message, which might be useful for someone using a translated version of
Git.
> + break;
> + }
> +
> if (get_sha1_with_context(arg, 0, sha1, &oc)) {
> if (seen_dashdash)
> die(_("unable to resolve revision: %s"), arg);
^ permalink raw reply
* [PATCH] show-branch: fix crash with long ref name
From: Christian Couder @ 2017-02-14 15:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Maxim Kuvyrkov, Christian Couder
This is a minimum fix for a crash with a long ref name.
Note that if the refname is too long (for example if you
use 100 instead of 50 in the test script) you could get
an error like:
fatal: cannot lock ref 'refs/heads/1_2_3_4_ ... _98_99_100_':
Unable to create '... /.git/refs/heads/1_2_3_4_ ... _98_99_100_.lock':
File name too long
when creating the ref instead of a crash when using
show-branch and that would be ok.
So a simpler fix could have been just something like:
- char head[128];
+ char head[1024];
But if the filesystem ever allows filenames longer than 1024
characters then the crash could appear again.
Reported by: Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/show-branch.c | 14 +++++++-------
t/t3204-show-branch-refname.sh | 19 +++++++++++++++++++
2 files changed, 26 insertions(+), 7 deletions(-)
create mode 100755 t/t3204-show-branch-refname.sh
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f3403ab..3c0fe55eec 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -620,7 +620,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
int all_heads = 0, all_remotes = 0;
int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
- char head[128];
+ char *head_cpy;
const char *head_p;
int head_len;
struct object_id head_oid;
@@ -791,11 +791,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
head_oid.hash, NULL);
if (head_p) {
head_len = strlen(head_p);
- memcpy(head, head_p, head_len + 1);
+ head_cpy = xstrdup(head_p);
}
else {
head_len = 0;
- head[0] = 0;
+ head_cpy = xstrdup("");
}
if (with_current_branch && head_p) {
@@ -804,15 +804,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
/* We are only interested in adding the branch
* HEAD points at.
*/
- if (rev_is_head(head,
+ if (rev_is_head(head_cpy,
head_len,
ref_name[i],
head_oid.hash, NULL))
has_head++;
}
if (!has_head) {
- int offset = starts_with(head, "refs/heads/") ? 11 : 0;
- append_one_rev(head + offset);
+ int offset = starts_with(head_cpy, "refs/heads/") ? 11 : 0;
+ append_one_rev(head_cpy + offset);
}
}
@@ -865,7 +865,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (1 < num_rev || extra < 0) {
for (i = 0; i < num_rev; i++) {
int j;
- int is_head = rev_is_head(head,
+ int is_head = rev_is_head(head_cpy,
head_len,
ref_name[i],
head_oid.hash,
diff --git a/t/t3204-show-branch-refname.sh b/t/t3204-show-branch-refname.sh
new file mode 100755
index 0000000000..83b64e3032
--- /dev/null
+++ b/t/t3204-show-branch-refname.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='test show-branch with long refname'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+ test_commit first &&
+ long_refname=$(printf "%s_" $(seq 1 50)) &&
+ git checkout -b "$long_refname"
+'
+
+test_expect_success 'show-branch with long refname' '
+
+ git show-branch first
+'
+
+test_done
--
2.12.0.rc0
^ permalink raw reply related
* Re: [BUG] Memory corruption crash with "git bisect start"
From: Christian Couder @ 2017-02-14 14:52 UTC (permalink / raw)
To: Maxim Kuvyrkov; +Cc: git
In-Reply-To: <A7AEC719-6E15-4622-8E21-3E11BAF74A3C@linaro.org>
On Tue, Feb 14, 2017 at 12:56 PM, Maxim Kuvyrkov
<maxim.kuvyrkov@linaro.org> wrote:
> I'm seeing the following memory corruption crash on a script-constructed repo when starting git bisect. I'm seeing this crash both with system git of Ubuntu Xenial and with freshly-compiled git master from the official repo.
>
> The log of the crash is attached.
Yeah, thanks for the report.
I took a look and it looks like git bisect crashes when it runs git
show-branch and it can be reproduced with just `git show-branch
bottom` instead of `git bisect start bottom top`.
> It is possible that the bug is in Xenial glibc, in which case -- please let me know and I will take it up with the glibc developers.
I have Ubuntu GLIBC 2.23-0ubuntu5 and I get a crash too.
^ 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