* Re: [PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()
From: Duy Nguyen @ 2017-02-14 9:40 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CAGZ79kYkc-_=RiK1uJ+ndhQu=B8u=UDVusXZu-dYe7KnGNye3Q@mail.gmail.com>
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?
--
Duy
^ permalink raw reply
* Re: [PATCH 07/11] files-backend: remove the use of git_path()
From: Duy Nguyen @ 2017-02-14 9:38 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CAGZ79kbJXUY=UGvHtkeLLj-qMaoOyTwa2dr3-FqEdYi8eFs4LA@mail.gmail.com>
On Tue, Feb 14, 2017 at 6:09 AM, Stefan Beller <sbeller@google.com> wrote:
>> +
>> + if (submodule) {
>> + refs->submodule = xstrdup_or_null(submodule);
>
> drop the _or_null here?
>
> So in this patch we have either
> * submodule set
> * or gitdir/gitcommondir set
>
> which means that we exercise the commondir for regular repos.
> In the future when we want to be able to have a combination of worktrees
> and submodules this ought to be possible by setting submodule != NULL
> and still populating the gitdir/commondir buffers.
You probably have seen it by now. In the near future, submodule is
completely gone from here. We convert to a .git dir before we pass in
here. In a farther future, gitcommondir will be gone too with all the
per-worktree logic in this file. A linked worktree consists of two
backends actually, one per-worktree (which remains files-based), the
other for shared refs, which could be files, lmdb or whatever.
Stacking up submodule on top of a linked worktree should not be a
problem.
--
Duy
^ permalink raw reply
* Re: [PATCH 09/11] refs: move submodule code out of files-backend.c
From: Duy Nguyen @ 2017-02-14 9:32 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CAGZ79kannQDWWYN5rHr6q-6=o1_ajeEjLn8Wzsc4RjqkfYOYwg@mail.gmail.com>
On Tue, Feb 14, 2017 at 6:35 AM, Stefan Beller <sbeller@google.com> wrote:
>> + git_dir = read_gitfile(buf.buf);
>
> if buf.buf is a (git) directory as opposed to a git file,
> we error out in read_gitfile. Did you mean to use
> read_gitfile_gently here or rather even resolve_gitdir_gently ?
This is what strbuf_git_path_submodule() does. I don't know the
backstory so I'm going to keep it as it is to keep the behavior
exactly (or very close) as before. We can replace it with a better
version (with explanation and all).
>> + if (git_dir) {
>
> when not using the _gently version git_dir is always
> non NULL here (or we're dead)?
>
>> + strbuf_reset(&buf);
>> + strbuf_addstr(&buf, git_dir);
>> + }
>> + if (!is_git_directory(buf.buf)) {
>> + gitmodules_config();
>> + sub = submodule_from_path(null_sha1, path);
>> + if (!sub)
>> + goto done;
>> + strbuf_reset(&buf);
>> + strbuf_git_path(&buf, "%s/%s", "modules", sub->name);
>
> You can inline "modules" into the format string?
Hm.. because this is strbuf_git_path_submodule() code. Perhaps it's
better to split it to a separate function and call it from here? Then
you can make more improvements on top that benefit everybody.
>> } else {
>> strbuf_addstr(&refs->gitdir, get_git_dir());
>> strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
>> @@ -1034,8 +1025,6 @@ static struct ref_store *files_ref_store_create(const char *submodule)
>> static void files_assert_main_repository(struct files_ref_store *refs,
>> const char *caller)
>> {
>> - if (refs->submodule)
>> - die("BUG: %s called for a submodule", caller);
>> }
>
> In a followup we'd get rid of files_assert_main_repository
> presumably?
Yes. Can't delete it now because I would need to touch all callers.
--
Duy
^ permalink raw reply
* Re: [PATCH 01/11] refs-internal.c: make files_log_ref_write() static
From: Duy Nguyen @ 2017-02-14 9:23 UTC (permalink / raw)
To: Ramsay Jones
Cc: Git Mailing List, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, Stefan Beller, David Turner
In-Reply-To: <6510f4b2-51bb-eabd-9cd7-30bc4164b25d@ramsayjones.plus.com>
On Tue, Feb 14, 2017 at 3:14 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
>> Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
>> but probably never used outside refs-internal.c
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> refs/files-backend.c | 3 +++
>> refs/refs-internal.h | 4 ----
>> 2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index cdb6b8ff5..75565c3aa 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
>> const char *dirname, size_t len,
>> int incomplete);
>> static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
>> +static int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
>> + const unsigned char *new_sha1, const char *msg,
>> + int flags, struct strbuf *err);
>
> Why? I don't like adding forward declarations unless it
> is absolutely necessary (ie mutually recursive functions),
> and even in the current 'pu' branch (@c04899d50), the
> definition of this function appears before all uses in
> this file. (ie, just add static to the definition).
>
> What am I missing?
It may have been needed at one point. With all the code changes and
movements, I guess I forgot to remove it.
--
Duy
^ permalink raw reply
* Re: Bug in 'git describe' if I have two tags on the same commit.
From: Istvan Pato @ 2017-02-14 8:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Daudt, git
In-Reply-To: <xmqqo9y5de1k.fsf@gitster.mtv.corp.google.com>
This is my fault: this is a lightweight tag.
Thank you!
2017-02-13 21:35 GMT+01:00 Junio C Hamano <gitster@pobox.com>:
> Kevin Daudt <me@ikke.info> writes:
>
>> On Sun, Feb 12, 2017 at 01:15:22PM +0100, Istvan Pato wrote:
>>
>>> (master) [1] % git show-ref --tag
>>> 76c634390... refs/tags/1.0.0
>>> b77c7cd17... refs/tags/1.1.0
>>> b77c7cd17... refs/tags/1.2.0
>>>
>>> (master) % git describe --tags --always
>>> 1.1.0-1-ge9e9ced
>>>
>>> ### Expected: 1.2.0
>>> ...
>>
>> Are these lightweight tags? Only annotated tags have a date associated
>> to them, which is where the rel-notes refers to.
>
> Good eyes. The fact that the two points at the same object means
> that even if they were both annotated tags, they have the same
> tagger dates.
>
> If the code that compares the candidates and picks better tag to
> describe the object with knows the refnames of these "tags", I'd
> imagine that we could use the versioncmp() logic as the final tie
> breaker, but I do not offhand remember if the original refname we
> took the tag (or commit) from is propagated that deep down the
> callchain. It probably does not, so some code refactoring may be
> needed if somebody wants to go in that direction.
>
>
>
>
>
^ permalink raw reply
* Re: Non-zero exit code without error
From: Serdar Sahin @ 2017-02-14 7:56 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <BE964323A3E644BBB01F8672263419EA@peakgames.net>
Thanks. I know it is hard to get an idea without being able to
reproduce it. I think there is no alternative without debugging it
with GDB.
I’ve tried your suggestions and nothing has changed.
I’ve also update the GIT as you suggested, but it is still the same.
Just to see, if GIT server causes some issues, I’ve pushed to repo to
github public as a private repo, and can reproduce the issue there as
well.
Thanks for your support.
On Tue, Feb 14, 2017 at 10:53 AM, Serdar Sahin <serdar@peakgames.net> wrote:
> Hi Christian,
>
> Thanks. I know it is hard to get an idea without being able to reproduce it.
> I think there is no alternative without debugging it with GDB.
>
> I’ve tried your suggestions and nothing has changed.
>
> I’ve also update the GIT as you suggested, but it is still the same.
>
> Just to see, if GIT server causes some issues, I’ve pushed to repo to github
> public as a private repo, and can reproduce the issue there as well.
>
> Thanks for your support.
>
> --
> Serdar Sahin
> Peak Games
>
> On Saturday, 11 February 2017 at 15:28, Christian Couder wrote:
>
> On Wed, Feb 8, 2017 at 11:26 AM, Serdar Sahin <serdar@peakgames.net> wrote:
>
> Hi Christian,
>
>
> We are using a private repo (Github Enterprise).
>
>
> Maybe you could try 'git fast-export --anonymize ...' on it.
>
> Let me give you the
> details you requested.
>
>
> On Git Server: git version 2.6.5.1574.g5e6a493
>
> On my client: git version 2.10.1 (Apple Git-78)
>
>
> I’ve tried to reproduce it with public repos, but couldn’t do so.
>
>
> You might try using the latest released version (2.11.1).
>
> For example you could install the last version on the client and then
> clone from the server with --bare and use this bare repo as if it was
> the server.
>
> You could also try `git fsck` to see if there are problems on your repo.
>
> Are there submodules or something a bit special?
>
> In the end it's difficult for us to help if we cannot reproduce, so
> your best bet might be to debug yourself using gdb for example.
>
> If I
> could get an error/log output, that would be sufficient.
>
>
> I am also including the full output below. (also git gc)
>
>
> MacOSX:test serdar$ git clone --mirror --depth 50 --no-single-branch
> git@git.privateserver.com:Casual/code_repository.git
>
>
> You could also try with different options above...
>
> Cloning into bare repository 'code_repository.git'...
>
> remote: Counting objects: 3362, done.
>
> remote: Compressing objects: 100% (1214/1214), done.
>
> remote: Total 3362 (delta 2335), reused 2968 (delta 2094), pack-reused 0
>
> Receiving objects: 100% (3362/3362), 56.77 MiB | 1.83 MiB/s, done.
>
> Resolving deltas: 100% (2335/2335), done.
>
> MacOSX:test serdar$ cd code_repository.git/
>
> MacOSX:code_repository.git serdar$ GIT_CURL_VERBOSE=1 GIT_TRACE=1 git
> fetch --depth 50 origin cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee
>
>
> ... and above.
>
> Also it looks like you use ssh so something like GIT_SSH_COMMAND="ssh
> -vv" might help more than GIT_CURL_VERBOSE=1
>
> 13:23:15.648337 git.c:350 trace: built-in: git 'fetch'
> '--depth' '50' 'origin' 'cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee'
>
> 13:23:15.651127 run-command.c:336 trace: run_command: 'ssh'
> 'git@git.privateserver.com' 'git-upload-pack
> '\''Casual/code_repository.git'\'''
>
> 13:23:17.750015 run-command.c:336 trace: run_command: 'gc' '--auto'
>
> 13:23:17.750829 exec_cmd.c:189 trace: exec: 'git' 'gc' '--auto'
>
> 13:23:17.753983 git.c:350 trace: built-in: git 'gc' '--auto'
>
> MacOSX:code_repository.git serdar$ echo $?
>
> 1
>
> MacOSX:code_repository.git serdar$ GIT_CURL_VERBOSE=1 GIT_TRACE=1 git gc
> --auto
>
> 13:23:45.899038 git.c:350 trace: built-in: git 'gc' '--auto'
>
> MacOSX:code_repository.git serdar$ echo $?
>
> 0
>
>
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Eric Wong @ 2017-02-14 7:13 UTC (permalink / raw)
To: Arif Khokar
Cc: Johannes Schindelin, Jakub Narębski, Jeff King,
Stefan Beller, meta, git
In-Reply-To: <7dbe0866-4a9b-7afe-8f51-ca1d5524d4a4@hotmail.com>
Arif Khokar <arif.i.khokar@gmail.com> wrote:
> On 02/13/2017 09:37 AM, Johannes Schindelin wrote:
> >I actually had expected *you* to put in a little bit of an effort, too. In
> >fact, I was very disappointed that you did not even look into porting that
> >script to use public-inbox instead of GMane.
>
> I wasn't aware of that expectation. My idea was to use NNTP as a way to
> facilitate the development of a new git utility that would serve as the
> inverse of git-send-email (sort of like the relationship between git
> format-patch and git am), rather than using a
Speaking for myself, I usually don't expect much, especially
from newcomers. So I am disappointed to see Dscho's disappointment
aimed at you, Arif. Especially since you're not a regular and
we have no idea how much free time, attention span, or familiarity
with Bourne shell you have.
> IIRC, I had posted some proof-of-concept Perl code to do so back in August
> in <DM5PR17MB1353B99EBD5F4FD23360DD41D3ED0@DM5PR17MB1353.namprd17.prod.outlook.com>
>
> Looking at public-inbox now at the archives of this group, it appears that
> several of the messages I sent weren't archived for some reason (and I
> didn't see any more responses to what I posted at the time). The messages
> are accessible via NNTP when connecting to gmane though.
It looks like it went to gmane via the meta@public-inbox.org to
gmane.mail.public-inbox.general mirror, not via the git@vger mirror.
I can't find it on git@vger's mail-archive.com mirror, either:
https://mail-archive.com/search?q=Arif+Khokar&l=git%40vger.kernel.org
> Also, looking at the source of the message I referenced, it appears that my
> MUA decided to base64 encode the message for some reason (which may have
> resulted in it getting filtered by those who I sent the message to).
It probably wasn't base64, but maybe it was one of these:
http://vger.kernel.org/majordomo-taboos.txt
Or it was the SPF softfail which you can see in the headers on both
gmane and public-inbox.
It might even be the '_' (underscore) in your other address.
But even Junio gets dropped by vger sometimes:
https://public-inbox.org/git/20170127035753.GA2604@dcvr/
But if I had to guess, vger gets hit by truckloads of spam and
the the backscatter volume could become unimaginable, so perhaps
it has good reason to discard silently.
Anyways, the eventual goal of public-inbox is to flip the
mailing list model backwards into "archives first" mode,
so a message needs to make it into public archives before
it goes out to subscribers. That might prevent or avoid
such problems... *shrug*
^ permalink raw reply
* Re: Developing git with IDE
From: Jeff King @ 2017-02-14 6:22 UTC (permalink / raw)
To: Oleg Taranenko; +Cc: git
In-Reply-To: <CABEd3j-kxA+ap7vqr85X-4HpQCvShPJUsS2Qq0BrMEK09BYS7A@mail.gmail.com>
On Tue, Feb 14, 2017 at 12:15:00AM +0100, Oleg Taranenko wrote:
> being last decade working with java & javascript I completely lost
> relation to c/c++ world. Trying to get into git internals I'm facing
> with issue what IDE is more suitable for developing git @ macos ?
>
> Have googled, but any my search queries following to non-relevant
> themes, like supporting git in IDEs etc.
>
> my first attempt - CLion (as far as I'm Jetbrains fan) - got failed,
> as far as doesn't support makefile-based projects, only CMake.
>
> There are a number of free C/C++ dev tools: Xcode, CodeBlocks,
> CodeLite. Gnat, Qt creator, Dev C++, C++ Builder (Borland? :),
> Eclipse, NetBeans... what else?
>
> Because of lack my modern C experience, could somebody share his own
> attempts/thoughts/use cases how more convenient dive into the git
> development process on the Mac?
I think most people just use a good editor (emacs or vim), and no IDE. I
do recommend using ctags or similar (and there is a "make tags" target
to build the tags) to help with jumping between functions.
> Tried to find in the git distribution Documentation more information
> about this, nothing as well... Would be nice to have a kind of
> 'Getting Started Manual'
There is Documentation/CodingGuidelines, but that's mostly about how to
_write_ code, not read it. Some protocols and subsystems are covered in
Documentation/technical. If you want a "big picture", I think you'd do
best to read something like:
https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain
That talks about the system as a whole, not the code, but the layout of
the code follows the overall system design (e.g., the entry point for
the "log" command is cmd_log(), and you can see which subsystems it uses
from there).
-Peff
^ permalink raw reply
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2017-02-14 6:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Duy Nguyen, Junio C Hamano, Stefan Beller
In-Reply-To: <20161230004845.rknafqsyosmyr6z2@sigill.intra.peff.net>
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. :)
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Sixt @ 2017-02-14 6:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702132337470.3496@virtualbox>
Am 13.02.2017 um 23:38 schrieb Johannes Schindelin:
> In addition, you build from a custom MINGW/MSys1 setup, correct?
Correct. Specifically, I use the build tools from "msysgit" times, but
build outside the premanufactured build environement; i.e., the
"THIS_IS_MSYSGIT" section in config.mak.uname is not used.
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/7] grep rev/path parsing fixes
From: Jeff King @ 2017-02-14 6:10 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
On Tue, Feb 14, 2017 at 01:00:21AM -0500, Jeff King wrote:
> On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:
>
> > > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > > cannot be used with revs." error would occur. If there is a repo and
> > > "foo" is not a rev, this command would proceed as usual. If there is no
> > > repo, the "setup_git_env called without repository" error would occur.
> > > (This is my understanding from reading the code - I haven't tested it
> > > out.)
> >
> > Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> > repo generates the same BUG. I suspect that "--no-index" should just
> > disable looking up revs entirely, even if we are actually in a
> > repository directory.
>
> 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
>
> builtin/grep.c | 78 +++++++++++++++++++++++++++++++++++++++------------------
> t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 119 insertions(+), 25 deletions(-)
Just to clarify: these are all existing bugs, and I think these are
probably maint-worthy patches (even the --no-index ones; though we don't
BUG on the out-of-repo without the patch from 'next', the code is still
doing the wrong thing in subtle ways).
But AFAIK they are all much older bugs than the upcoming v2.12, so there
is no pressing need to fit them into the upcoming release.
-Peff
^ permalink raw reply
* [PATCH 7/7] grep: do not diagnose misspelt revs with --no-index
From: Jeff King @ 2017-02-14 6:08 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
If we are using --no-index, then our arguments cannot be
revs in the first place. Not only is it pointless to
diagnose them, but if we are not in a repository, we should
not be trying to resolve any names.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 2 +-
t/t7810-grep.sh | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index c4c632594..1454bef49 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1201,7 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i);
+ verify_filename(prefix, argv[j], j == i && use_index);
}
parse_pathspec(&pathspec, 0,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index c051c7ee8..0ff9f6cae 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1043,6 +1043,11 @@ test_expect_success 'grep --no-index prefers paths to revs' '
test_cmp expect actual
'
+test_expect_success 'grep --no-index does not "diagnose" revs' '
+ test_must_fail git grep --no-index o :1:hello.c 2>err &&
+ test_i18ngrep ! -i "did you mean" err
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 6/7] grep: avoid resolving revision names in --no-index case
From: Jeff King @ 2017-02-14 6:07 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
We disallow the use of revisions with --no-index, but we
don't actually check and complain until well after we've
parsed the revisions.
This is the cause of a few problems:
1. We shouldn't be calling get_sha1() at all when we aren't
in a repository, as it might access the ref or object
databases. For now, this should generally just return
failure, but eventually it will become a BUG().
2. When there's a "--" disambiguator and you're outside a
repository, we'll complain early with "unable to resolve
revision". But we can give a much more specific error.
3. When there isn't a "--" disambiguator, we still do the
normal rev/path checks. This is silly, as we know we
cannot have any revs with --no-index. Everything we see
must be a path.
Outside of a repository this doesn't matter (since we
know it won't resolve), but inside one, we may complain
unnecessarily if a filename happens to also match a
refname.
This patch skips the get_sha1() call entirely in the
no-index case, and behaves as if it failed (with the
exception of giving a better error message).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 6 ++++++
t/t7810-grep.sh | 13 +++++++++++++
2 files changed, 19 insertions(+)
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"));
+ break;
+ }
+
if (get_sha1_with_context(arg, 0, sha1, &oc)) {
if (seen_dashdash)
die(_("unable to resolve revision: %s"), arg);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index a6011f9b1..c051c7ee8 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1030,6 +1030,19 @@ test_expect_success 'grep --no-index pattern -- path' '
)
'
+test_expect_success 'grep --no-index complains of revs' '
+ test_must_fail git grep --no-index o master -- 2>err &&
+ test_i18ngrep "no-index cannot be used with revs" err
+'
+
+test_expect_success 'grep --no-index prefers paths to revs' '
+ test_when_finished "rm -f master" &&
+ echo content >master &&
+ echo master:content >expect &&
+ git grep --no-index o master >actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
From: Jeff King @ 2017-02-14 6:05 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
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.
It adds tests that cover the two bugs, but also some related
situations (which already worked, but this confirms that our
fixes did not break anything).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 29 ++++++++++++++++++++++++-----
t/t7810-grep.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 461347adb..e83b33bda 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1149,7 +1149,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
compile_grep_patterns(&opt);
- /* 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;
+ }
+ }
+
+ /*
+ * 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.
+ */
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2c1f7373e..a6011f9b1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,39 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
'
+test_expect_success 'dashdash disambiguates rev as rev' '
+ test_when_finished "rm -f master" &&
+ echo content >master &&
+ echo master:hello.c >expect &&
+ git grep -l o master -- hello.c >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'dashdash disambiguates pathspec as pathspec' '
+ test_when_finished "git rm -f master" &&
+ echo content >master &&
+ git add master &&
+ echo master:content >expect &&
+ git grep o -- master >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'report bogus arg without dashdash' '
+ test_must_fail git grep o does-not-exist
+'
+
+test_expect_success 'report bogus rev with dashdash' '
+ test_must_fail git grep o hello.c --
+'
+
+test_expect_success 'allow non-existent path with dashdash' '
+ # We need a real match so grep exits with success.
+ tree=$(git ls-tree HEAD |
+ sed s/hello.c/not-in-working-tree/ |
+ git mktree) &&
+ git grep o "$tree" -- not-in-working-tree
+'
+
test_expect_success 'grep --no-index pattern -- path' '
rm -fr non &&
mkdir -p non/git &&
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 4/7] grep: re-order rev-parsing loop
From: Jeff King @ 2017-02-14 6:04 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
We loop over the arguments, but every branch of the loop
hits either a "continue" or a "break". Surely we can make
this simpler.
The final conditional is:
if (arg is a rev) {
... handle rev ...
continue;
}
break;
We can rewrite this as:
if (arg is not a rev)
break;
... handle rev ...
That makes the flow a little bit simpler, and will make
things much easier to follow when we add more logic in
future patches.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 081e1b57a..461347adb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,20 +1154,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+ struct object *object;
+
if (!strcmp(arg, "--")) {
i++;
seen_dashdash = 1;
break;
}
- /* 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);
}
/* The rest are paths */
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 3/7] t7810: make "--no-index --" test more robust
From: Jeff King @ 2017-02-14 6:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
This makes sure that we actually use the pathspecs after
"--" correctly (as opposed to just seeing if grep errored
out).
Signed-off-by: Jeff King <peff@peff.net>
---
This could be squashed into the previous patch.
I didn't end up using the "nongit" helper, because we actually have to
do some other setup inside the subshell (this is the reason the earlier
tests in this script don't use the helper, either).
t/t7810-grep.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 29202f0e7..2c1f7373e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -990,7 +990,10 @@ test_expect_success 'grep --no-index pattern -- path' '
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
echo hello >hello &&
- git grep --no-index o -- .
+ echo goodbye >goodbye &&
+ echo hello:hello >expect &&
+ git grep --no-index o -- hello >actual &&
+ test_cmp expect actual
)
'
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 2/7] grep: do not unnecessarily query repo for "--"
From: Jeff King @ 2017-02-14 6:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
From: Jonathan Tan <jonathantanmy@google.com>
When running a command of the form
git grep --no-index pattern -- path
in the absence of a Git repository, an error message will be printed:
fatal: BUG: setup_git_env called without repository
This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation. (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)
Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Unchanged from your original.
builtin/grep.c | 9 +++++----
t/t7810-grep.sh | 12 ++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 5a282c4d0..081e1b57a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+ if (!strcmp(arg, "--")) {
+ i++;
+ seen_dashdash = 1;
+ break;
+ }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
continue;
}
- if (!strcmp(arg, "--")) {
- i++;
- seen_dashdash = 1;
- }
break;
}
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
'
+test_expect_success 'grep --no-index pattern -- path' '
+ rm -fr non &&
+ mkdir -p non/git &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ echo hello >hello &&
+ git grep --no-index o -- .
+ )
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 1/7] grep: move thread initialization a little lower
From: Jeff King @ 2017-02-14 6:02 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214060021.einv7372exbxa23z@sigill.intra.peff.net>
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.
builtin/grep.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..5a282c4d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,6 +1169,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
break;
}
+ /* The rest are paths */
+ if (!seen_dashdash) {
+ int j;
+ for (j = i; j < argc; j++)
+ verify_filename(prefix, argv[j], j == i);
+ }
+
+ parse_pathspec(&pathspec, 0,
+ PATHSPEC_PREFER_CWD |
+ (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+ prefix, argv + i);
+ pathspec.max_depth = opt.max_depth;
+ pathspec.recursive = 1;
+
#ifndef NO_PTHREADS
if (list.nr || cached || show_in_pager)
num_threads = 0;
@@ -1190,20 +1204,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
#endif
- /* The rest are paths */
- if (!seen_dashdash) {
- int j;
- for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i);
- }
-
- parse_pathspec(&pathspec, 0,
- PATHSPEC_PREFER_CWD |
- (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
- prefix, argv + i);
- pathspec.max_depth = opt.max_depth;
- pathspec.recursive = 1;
-
if (recurse_submodules) {
gitmodules_config();
compile_submodule_options(&opt, &pathspec, cached, untracked,
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related
* [PATCH 0/7] grep rev/path parsing fixes
From: Jeff King @ 2017-02-14 6:00 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170214012037.u2eg2n7mvteullcx@sigill.intra.peff.net>
On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:
> > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > cannot be used with revs." error would occur. If there is a repo and
> > "foo" is not a rev, this command would proceed as usual. If there is no
> > repo, the "setup_git_env called without repository" error would occur.
> > (This is my understanding from reading the code - I haven't tested it
> > out.)
>
> Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> repo generates the same BUG. I suspect that "--no-index" should just
> disable looking up revs entirely, even if we are actually in a
> repository directory.
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
builtin/grep.c | 78 +++++++++++++++++++++++++++++++++++++++------------------
t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 119 insertions(+), 25 deletions(-)
-Peff
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Jeff King @ 2017-02-14 5:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Arif Khokar, Johannes Schindelin, Arif Khokar,
Jakub Narębski, Stefan Beller, meta@public-inbox.org,
git@vger.kernel.org, Eric Wong
In-Reply-To: <xmqqlgt99yeo.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 08:41:51PM -0800, Junio C Hamano wrote:
> Arif Khokar <arif.i.khokar@gmail.com> writes:
>
> > One concern I have regarding this idea is whether or not SMTP servers
> > typically replace a Message-Id header set by the client.
>
> The clients are supposed to give Message-IDs, but because some
> clients fail to do so, SMTP server implementations are allowed to
> add an ID to avoid leaving a message nameless (IIRC, 6.3 in
> RFC2821). So "replace" would be in violation.
>
> But some parts of the world ignore RFCs, so...
I know there are some terrible servers out there, but I think we can
discount any such server as horribly broken. Rewriting message-ids would
cause threading problems any time the sender referred to their own
messages. So "format-patch --thread" would fail to work, and even
replying to your own message from your "sent" folder would fail.
-Peff
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Arif Khokar @ 2017-02-14 5:09 UTC (permalink / raw)
To: Junio C Hamano, Arif Khokar
Cc: Johannes Schindelin, Arif Khokar, Jakub Narębski, Jeff King,
Stefan Beller, meta@public-inbox.org, git@vger.kernel.org,
Eric Wong
In-Reply-To: <xmqqlgt99yeo.fsf@gitster.mtv.corp.google.com>
On 02/13/2017 11:41 PM, Junio C Hamano wrote:
> Arif Khokar <arif.i.khokar@gmail.com> writes:
>
>> One concern I have regarding this idea is whether or not SMTP servers
>> typically replace a Message-Id header set by the client.
>
> The clients are supposed to give Message-IDs, but because some
> clients fail to do so, SMTP server implementations are allowed to
> add an ID to avoid leaving a message nameless (IIRC, 6.3 in
> RFC2821). So "replace" would be in violation.
>
> But some parts of the world ignore RFCs, so...
Based on my testing, gmail and comcast (and my work email) will preserve
the Message-Id header set by the client, but
hotmail.com/live.com/outlook.com will replace it with their generated value.
Based on a small sample of email addresses of those who post to this
list, it appears that most people are using their own MTA to send email,
or are using gmail, so they probably wouldn't be affected by the issue.
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Junio C Hamano @ 2017-02-14 4:41 UTC (permalink / raw)
To: Arif Khokar
Cc: Johannes Schindelin, Arif Khokar, Jakub Narębski, Jeff King,
Stefan Beller, meta@public-inbox.org, git@vger.kernel.org,
Eric Wong
In-Reply-To: <acac96da-2404-4f7e-a83d-7648ca448d31@hotmail.com>
Arif Khokar <arif.i.khokar@gmail.com> writes:
> One concern I have regarding this idea is whether or not SMTP servers
> typically replace a Message-Id header set by the client.
The clients are supposed to give Message-IDs, but because some
clients fail to do so, SMTP server implementations are allowed to
add an ID to avoid leaving a message nameless (IIRC, 6.3 in
RFC2821). So "replace" would be in violation.
But some parts of the world ignore RFCs, so...
^ permalink raw reply
* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-14 4:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqwpcvfdao.fsf@gitster.mtv.corp.google.com>
Hey Junio,
>
> See the 3-patch series I just sent out. I didn't think it through
> very carefully (especially the error message the other caller
> produces), but the whole thing _smells_ correct to me.
Okay, got it! I will write-up those changes, and make sure nothing bad
happens. (Also, the one other function that calls handle_revision_opt,
parse_revision_opt needs to be fixed for any changes in
handle_revision_opt.)
I will do all of this in the next week (Unfortunately, exams!) and
submit a new version of this patch (Also, I need to update tests, add
documentation, and remove code that does this shorthand stuff for
other commands as per Matthieu's comments)
--
Best Regards,
Siddharth Kannan.
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Arif Khokar @ 2017-02-14 3:59 UTC (permalink / raw)
To: Arif Khokar, Johannes Schindelin
Cc: Jakub Narębski, Jeff King, Stefan Beller,
meta@public-inbox.org, git@vger.kernel.org, Eric Wong
In-Reply-To: <7dbe0866-4a9b-7afe-8f51-ca1d5524d4a4@hotmail.com>
On 02/13/2017 10:56 PM, Arif Khokar wrote:
> I wasn't aware of that expectation. My idea was to use NNTP as a way to
> facilitate the development of a new git utility that would serve as the
> inverse of git-send-email (sort of like the relationship between git
> format-patch and git am), rather than using a
...custom script that's tightly coupled to gmane and public-inbox.org
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Arif Khokar @ 2017-02-14 3:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jakub Narębski, Jeff King, Stefan Beller,
meta@public-inbox.org, git@vger.kernel.org, Eric Wong,
Arif Khokar
In-Reply-To: <alpine.DEB.2.20.1702131533400.3496@virtualbox>
On 02/13/2017 09:37 AM, Johannes Schindelin wrote:
> Hi Arif,
>
> On Mon, 13 Feb 2017, Arif Khokar wrote:
>> Thanks for the link. One thing that comes to mind that is that it may
>> be better to just download the patches and then manually apply them
>> afterwords rather than doing it in the script itself. Or at least add
>> an option to the script to not automatically invoke git am.
>
> I actually had expected *you* to put in a little bit of an effort, too. In
> fact, I was very disappointed that you did not even look into porting that
> script to use public-inbox instead of GMane.
I wasn't aware of that expectation. My idea was to use NNTP as a way to
facilitate the development of a new git utility that would serve as the
inverse of git-send-email (sort of like the relationship between git
format-patch and git am), rather than using a
IIRC, I had posted some proof-of-concept Perl code to do so back in
August in
<DM5PR17MB1353B99EBD5F4FD23360DD41D3ED0@DM5PR17MB1353.namprd17.prod.outlook.com>
Looking at public-inbox now at the archives of this group, it appears
that several of the messages I sent weren't archived for some reason
(and I didn't see any more responses to what I posted at the time). The
messages are accessible via NNTP when connecting to gmane though.
Also, looking at the source of the message I referenced, it appears that
my MUA decided to base64 encode the message for some reason (which may
have resulted in it getting filtered by those who I sent the message to).
I will look into this more now (given yours and Junio's responses).
>> Getting back to the point I made when this thread was still active, I
>> still think it would be better to be able to list the message-id values
>> in the header or body of the cover letter message of a patch series
>> (preferably the former) in order to facilitate downloading the patches
>> via NNTP from gmane or public-inbox.org. That would make it easier
>> compared to the different, ad-hoc, methods that exist for each email
>> client.
>
> You can always do that yourself: you can modify your cover letter to
> include that information.
Certainly, but it would be nice to be able to have it done automatically
by git format-patch (which I'll look into).
> Note that doing this automatically in format-patch may not be appropriate,
> as 1) the Message-ID could be modified depending on the mail client used
> to send the mails
I think the best approach would be not to make including the message-id
values the default behavior. Specifying a new command-line option to
enable that behavior should address those concerns I think.
> and 2) it is not unheard of that a developer
> finds a bug in the middle of sending a patch series, fixes that bug, and
> regenerates the remainder of the patch series, completely rewriting those
> Message-IDs.
Perhaps, but should something like that not warrant a re-roll of sorts.
That is, one should reply to the partial patch series stating that there
is a bug that renders this particular patch (series) un-usable and the
re-roll could be posted as a reply to the original cover letter?
>> Alternatively, or perhaps in addition to the list of message-ids, a list
>> of URLs to public-inbox.org or gmane messages could also be provided for
>> those who prefer to download patches via HTTP.
>
> At this point, I am a little disinterested in a discussion without code. I
> brought some code to the table, after all.
If you have the time, please take a look at the message-id I referenced.
If you need, I can re-post the proof-of-concept code.
^ 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