* [PATCH 0/2] Demonstrate a critical worktree/gc bug
From: Johannes Schindelin @ 2017-02-08 12:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
... and a half-working workaround for the auto-gc case.
This patch series really is just another attempt to prod Duy into fixing
this instead of dabbling with shiny new toys ;-)
Changes since "v0":
- split out the test case, both to make it easier for Duy to integrate
it into the patch series that fixes the bug, as well as to provide a
little more, very gentle pressure to demonstrate the severity of this
problem.
Johannes Schindelin (2):
worktree: demonstrate data lost to auto-gc
worktree: demonstrate a half-working workaround for auto-gc
t/t6500-gc.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Published-As: https://github.com/dscho/git/releases/tag/gc-worktree-v1
Fetch-It-Via: git fetch https://github.com/dscho/git gc-worktree-v1
--
2.11.1.windows.1
^ permalink raw reply
* [PATCH 1/2] refs.c: add resolve_ref_submodule()
From: Nguyễn Thái Ngọc Duy @ 2017-02-08 11:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170208113144.8201-1-pclouds@gmail.com>
This is basically the extended version of resolve_gitlink_ref() where we
have access to more info from the underlying resolve_ref_recursively() call.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs.c | 20 ++++++++++++++------
refs.h | 3 +++
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index cd36b64ed9..02e35d83f3 100644
--- a/refs.c
+++ b/refs.c
@@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
resolve_flags, sha1, flags);
}
-int resolve_gitlink_ref(const char *submodule, const char *refname,
- unsigned char *sha1)
+const char *resolve_ref_submodule(const char *submodule, const char *refname,
+ int resolve_flags, unsigned char *sha1,
+ int *flags)
{
size_t len = strlen(submodule);
struct ref_store *refs;
- int flags;
while (len && submodule[len - 1] == '/')
len--;
if (!len)
- return -1;
+ return NULL;
if (submodule[len]) {
/* We need to strip off one or more trailing slashes */
@@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
}
if (!refs)
- return -1;
+ return NULL;
+
+ return resolve_ref_recursively(refs, refname, resolve_flags, sha1, flags);
+}
+
+int resolve_gitlink_ref(const char *submodule, const char *refname,
+ unsigned char *sha1)
+{
+ int flags;
- if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) ||
+ if (!resolve_ref_submodule(submodule, refname, 0, sha1, &flags) ||
is_null_sha1(sha1))
return -1;
return 0;
diff --git a/refs.h b/refs.h
index 9fbff90e79..74542468d8 100644
--- a/refs.h
+++ b/refs.h
@@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);
*/
int resolve_gitlink_ref(const char *submodule, const char *refname,
unsigned char *sha1);
+const char *resolve_ref_submodule(const char *submodule, const char *refname,
+ int resolve_flags, unsigned char *sha1,
+ int *flags);
/*
* Return true iff abbrev_name is a possible abbreviation for
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH RFC 0/2] Kill manual ref parsing code in worktree.c
From: Nguyễn Thái Ngọc Duy @ 2017-02-08 11:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty,
Nguyễn Thái Ngọc Duy
A hundred years ago I added this code because a "standalone" ref
parsing code was not available from refs.c and the file was going through
some heavy changes that refactoring its ref parsing code out was not
an option. I promised to kill this parse_ref() eventually. I'm
fulfilling it today (or soon).
I would really like to you double check the approach I'm using here
(using submodule interface for accessing refs from another worktree)
since that may be the way forward to fix the "gc losing objects" in
multi worktrees. I've given it lots of thoughts in the last 24 hours.
Still can't find any fundamental flaw...
Nguyễn Thái Ngọc Duy (2):
refs.c: add resolve_ref_submodule()
worktree.c: use submodule interface to access refs from another worktree
branch.c | 3 +-
refs.c | 20 +++++++++----
refs.h | 3 ++
worktree.c | 99 +++++++++++++++-----------------------------------------------
worktree.h | 2 +-
5 files changed, 44 insertions(+), 83 deletions(-)
--
2.11.0.157.gd943d85
^ permalink raw reply
* Re: Non-zero exit code without error
From: Serdar Sahin @ 2017-02-08 10:26 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD18Sbqo-_ZVyYTJtwNaRc8bFSd0KEYQ1oRH7-G+xnJTJg@mail.gmail.com>
Hi Christian,
We are using a private repo (Github Enterprise). 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. 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
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
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
MacOSX:code_repository.git serdar$
On Wed, Feb 8, 2017 at 1:07 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> Hi,
>
> On Tue, Feb 7, 2017 at 12:27 PM, Serdar Sahin <serdar@peakgames.net> wrote:
>> Hi,
>>
>> When we execute the following lines, the exit code is 1, but it is
>> unclear what is the reason of this exit code. Do you have any idea?
>>
>> git clone --mirror --depth 50 --no-single-branch
>> git@github.hede.com:Casual/hodo-server.git
>
> First, could you tell us the git version you are using on the client
> and on the server, and if this a new problem with newer versions?
> Also is the repos accessible publicly or is it possible to reproduce
> on another repo?
> And what happens using other protocols like HTTP/S?
>
>> Cloning into bare repository 'hodo-server.git'...
>> remote: Counting objects: 3371, done.
>> remote: Compressing objects: 100% (1219/1219), done.
>> remote: Total 3371 (delta 2344), reused 2971 (delta 2098), pack-reused 0
>> Receiving objects: 100% (3371/3371), 56.77 MiB | 2.18 MiB/s, done.
>> Resolving deltas: 100% (2344/2344), done.
>>
>> echo $?
>> 0
>>
>> cd hodo-server.git/
>>
>> GIT_CURL_VERBOSE=1 GIT_TRACE=1 git fetch --depth 50 origin
>> cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee
>> 14:12:35.215889 git.c:350 trace: built-in: git 'fetch'
>> '--depth' '50' 'origin' 'cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee'
>> 14:12:35.217273 run-command.c:336 trace: run_command: 'ssh'
>> 'git@github.hede.com' 'git-upload-pack '\''Casual/hodo-server.git'\'''
>> 14:12:37.301122 run-command.c:336 trace: run_command: 'gc' '--auto'
>> 14:12:37.301866 exec_cmd.c:189 trace: exec: 'git' 'gc' '--auto'
>> 14:12:37.304473 git.c:350 trace: built-in: git 'gc' '--auto'
>>
>> echo $?
>> 1
>
> What happens if you just run 'git gc --auto' after that?
^ permalink raw reply
* Re: subtree merging fails
From: Stavros Liaskos @ 2017-02-08 10:42 UTC (permalink / raw)
To: David Aguilar; +Cc: Samuel Lijin, git@vger.kernel.org
In-Reply-To: <20170207184437.c6uuuxcmhi434vbc@gmail.com>
@Samuel Lijin
I tried now and I get:
"merge: branch_name
- not something we can merge".
Maybe that is something easy to fix but currently I am using a
workaround script so I am not putting any more effort at this at the
moment.
@David Aguilar
That's true but the trailing slash is already there. This commands
looks promising. An update would be GREAT!
FYI that's the script I am using to address this problem:
#!/bin/bash
function initial {
if git remote | grep -q lisa_remote
then
echo "Subtree delete & update"
git checkout lisa_branch
git pull
git checkout master
git merge --squash -s subtree --no-commit lisa_branch
git merge --squash --allow-unrelated-histories -s subtree
--no-commit lisa_branch
else
echo "Add subtree"
git remote add lisa_remote git@xxxxxxxx:lisa/lisa.git
git fetch lisa_remote
git checkout -b lisa_branch lisa_remote/master
git checkout master
git read-tree --prefix=lisaSubTree/ -u lisa_branch
gitrm
git rm --cached -r lisaSubTree/.gitignore
git rm --cached -r lisaSubTree/*
fi
}
initial
On Tue, Feb 7, 2017 at 7:44 PM, David Aguilar <davvid@gmail.com> wrote:
> On Tue, Feb 07, 2017 at 08:59:06AM -0600, Samuel Lijin wrote:
>> Have you tried using (without -s subtree) -X subtree=path/to/add/subtree/at?
>>
>> From the man page:
>>
>> subtree[=<path>]
>> This option is a more advanced form of subtree
>> strategy, where the strategy
>> makes a guess on how two trees must be shifted to match
>> with each other when
>> merging. Instead, the specified path is prefixed (or
>> stripped from the
>> beginning) to make the shape of two trees to match.
>
> I'm not 100% certain, but it's highly likely that the subtree=<prefix>
> argument needs to include a trailing slash "/" in the prefix,
> otherwise files will be named e.g. "fooREADME" instead of
> "foo/README" when prefix=foo.
>
> These days I would steer users towards the "git-subtree" command in
> contrib/ so that users don't need to deal with these details. It
> handles all of this stuff for you.
>
> https://github.com/git/git/blob/master/contrib/subtree/git-subtree.txt
>
> https://github.com/git/git/tree/master/contrib/subtree
>
> Updating the progit book to also mention git-subtree, in addition to the
> low-level methods, would probably be a good user-centric change.
> --
> David
^ permalink raw reply
* Bug with fixup and autosquash
From: Ashutosh Bapat @ 2017-02-08 10:10 UTC (permalink / raw)
To: git
Hi Git-developers,
First of all thanks for developing this wonderful scm tool. It's sooo versatile.
I have been using git rebase heavily these days and seem to have found a bug.
If there are two commit messages which have same prefix e.g.
yyyyyy This is prefix
xxxxxx This is prefix and message
xxxxxx comitted before yyyyyy
Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
zzzzzz fixup! This is prefix
When I run git rebase -i --autosquash, the script it shows me looks like
pick xxxxxx This is prefix and message
fixup zzzzzz fixup! This is prefix
pick yyyyyy This is prefix
I think the correct order is
pick xxxxxx This is prefix and message
pick yyyyyy This is prefix
fixup zzzzzz fixup! This is prefix
Is that right?
I am using
[ubuntu]git --version
git version 1.7.9.5
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
^ permalink raw reply
* Re: Non-zero exit code without error
From: Christian Couder @ 2017-02-08 10:07 UTC (permalink / raw)
To: Serdar Sahin; +Cc: git
In-Reply-To: <CAL7ZE5xYVM6=C+SJLJ2HMFZ2gvuduw8p0UnS0RnBaXibj0mgDw@mail.gmail.com>
Hi,
On Tue, Feb 7, 2017 at 12:27 PM, Serdar Sahin <serdar@peakgames.net> wrote:
> Hi,
>
> When we execute the following lines, the exit code is 1, but it is
> unclear what is the reason of this exit code. Do you have any idea?
>
> git clone --mirror --depth 50 --no-single-branch
> git@github.hede.com:Casual/hodo-server.git
First, could you tell us the git version you are using on the client
and on the server, and if this a new problem with newer versions?
Also is the repos accessible publicly or is it possible to reproduce
on another repo?
And what happens using other protocols like HTTP/S?
> Cloning into bare repository 'hodo-server.git'...
> remote: Counting objects: 3371, done.
> remote: Compressing objects: 100% (1219/1219), done.
> remote: Total 3371 (delta 2344), reused 2971 (delta 2098), pack-reused 0
> Receiving objects: 100% (3371/3371), 56.77 MiB | 2.18 MiB/s, done.
> Resolving deltas: 100% (2344/2344), done.
>
> echo $?
> 0
>
> cd hodo-server.git/
>
> GIT_CURL_VERBOSE=1 GIT_TRACE=1 git fetch --depth 50 origin
> cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee
> 14:12:35.215889 git.c:350 trace: built-in: git 'fetch'
> '--depth' '50' 'origin' 'cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee'
> 14:12:35.217273 run-command.c:336 trace: run_command: 'ssh'
> 'git@github.hede.com' 'git-upload-pack '\''Casual/hodo-server.git'\'''
> 14:12:37.301122 run-command.c:336 trace: run_command: 'gc' '--auto'
> 14:12:37.301866 exec_cmd.c:189 trace: exec: 'git' 'gc' '--auto'
> 14:12:37.304473 git.c:350 trace: built-in: git 'gc' '--auto'
>
> echo $?
> 1
What happens if you just run 'git gc --auto' after that?
^ permalink raw reply
* [PATCH] refs-internal.c: make files_log_ref_write() static
From: Nguyễn Thái Ngọc Duy @ 2017-02-08 9:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
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 c041d4ba21..6a0bf94bf1 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);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 25444cf5b0..4c9215d33f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -220,10 +220,6 @@ struct ref_transaction {
enum ref_transaction_state state;
};
-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);
-
/*
* Check for entries in extras that are within the specified
* directory, where dirname is a reference directory name including
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: "disabling bitmap writing, as some objects are not being packed"?
From: Duy Nguyen @ 2017-02-08 8:37 UTC (permalink / raw)
To: David Turner; +Cc: Junio C Hamano, Git Mailing List, Jeff King
In-Reply-To: <1486542299.1938.47.camel@novalis.org>
On Wed, Feb 8, 2017 at 3:24 PM, David Turner <novalis@novalis.org> wrote:
> On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:
>> On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:
>> > On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:
>> >> And we can't grep for fatal errors anyway. The problem that led to
>> >> 329e6e8794 was this line
>> >>
>> >> warning: There are too many unreachable loose objects; run 'git
>> >> prune' to remove them.
>> >>
>> >> which is not fatal.
>> >
>> > So, speaking of that message, I noticed that our git servers were
>> > getting slow again and found that message in gc.log.
>> >
>> > I propose to make auto gc not write that message either. Any objections?
>>
>> Does that really help? auto gc would run more often, but unreachable
>> loose objects are still present and potentially make your servers
>> slow? Should these servers run periodic and explicit gc/prune?
>
> At least pack files wouldn't accumulate. This is the major cause of
> slowdown, since each pack file must be checked for each object.
>
> (And, also, maybe those unreachable loose objects are too new to get
> gc'd, but if we retry next week, we'll gc them).
I was about to suggest a config option that lets you run auto gc
unconditionally, which, I think, is better than suppressing the
message. Then I found gc.autoDetach. If you set it to false globally,
I think you'll get the behavior you want.
On second thought, perhaps gc.autoDetach should default to false if
there's no tty, since its main point it to stop breaking interactive
usage. That would make the server side happy (no tty there).
--
Duy
^ permalink raw reply
* Re: "disabling bitmap writing, as some objects are not being packed"?
From: David Turner @ 2017-02-08 8:24 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List, Jeff King
In-Reply-To: <CACsJy8C81+D=UG4pZ4e+URQqKRCPG=5bLiCHbGCQamvE-2y2MQ@mail.gmail.com>
On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:
> On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:
> > On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:
> >> And we can't grep for fatal errors anyway. The problem that led to
> >> 329e6e8794 was this line
> >>
> >> warning: There are too many unreachable loose objects; run 'git
> >> prune' to remove them.
> >>
> >> which is not fatal.
> >
> > So, speaking of that message, I noticed that our git servers were
> > getting slow again and found that message in gc.log.
> >
> > I propose to make auto gc not write that message either. Any objections?
>
> Does that really help? auto gc would run more often, but unreachable
> loose objects are still present and potentially make your servers
> slow? Should these servers run periodic and explicit gc/prune?
At least pack files wouldn't accumulate. This is the major cause of
slowdown, since each pack file must be checked for each object.
(And, also, maybe those unreachable loose objects are too new to get
gc'd, but if we retry next week, we'll gc them).
^ permalink raw reply
* Re: Request re git status
From: Samuel Lijin @ 2017-02-08 7:44 UTC (permalink / raw)
To: Jacob Keller; +Cc: Duy Nguyen, Phil Hord, Ron Pero, Git
In-Reply-To: <CA+P7+xqg9b+49P6bO65E0q4a87BkNL76XGiUjcMg+UmDcU=WPg@mail.gmail.com>
On Wed, Feb 8, 2017 at 12:30 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Feb 7, 2017 at 10:13 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Feb 8, 2017 at 2:18 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>> Personally, I think that the fact that Git forces the user to think
>>> about it in terms of "oh I have to fetch" instead of that happening
>>> automatically, it helps teach the model to the user. If it happened in
>>> the background then the user might not be confronted with the
>>> distributed nature of the tool.
>>
>> I agree. But I think there is some room for improvement. Do we know
>> when the last fetch of the relevant upstream is? If we do, and if it's
>> been "a while" (configurable), then we should make a note suggesting
>> fetching again in git-status.
>>
>> This is not exactly my own idea. Gentoo's portage (i.e. friends with
>> apt-get, yum... if you're not familiar) also has this explicit "fetch"
>> operation, which is called sync. If you haven't sync'd in a while and
>> try to install new package, you get a friendly message (that helps me
>> a couple times).
>> --
>> Duy
Arch's pacman -S sync operation also has the -y flag, which updates
the local package databases, and can be used in conjunction with the
-u upgrade flag to upgrade repositories.
> That seems reasonable.
>
> Thanks,
> Jake
To be clear, I'm not advocating changing the *default* behavior of git
status; I agree that it wouldn't make sense. And although personally I
constantly update remotes manually (to the point where I abhor using
pull), I do think there's room to add an option to "fetch the
remote-tracking branch" to git status.
^ permalink raw reply
* Re: "disabling bitmap writing, as some objects are not being packed"?
From: Duy Nguyen @ 2017-02-08 6:45 UTC (permalink / raw)
To: David Turner; +Cc: Junio C Hamano, Git Mailing List, Jeff King
In-Reply-To: <1486515795.1938.45.camel@novalis.org>
On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:
> On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:
>> And we can't grep for fatal errors anyway. The problem that led to
>> 329e6e8794 was this line
>>
>> warning: There are too many unreachable loose objects; run 'git
>> prune' to remove them.
>>
>> which is not fatal.
>
> So, speaking of that message, I noticed that our git servers were
> getting slow again and found that message in gc.log.
>
> I propose to make auto gc not write that message either. Any objections?
Does that really help? auto gc would run more often, but unreachable
loose objects are still present and potentially make your servers
slow? Should these servers run periodic and explicit gc/prune?
--
Duy
^ permalink raw reply
* Re: Request re git status
From: Jacob Keller @ 2017-02-08 6:30 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Samuel Lijin, Phil Hord, Ron Pero, Git
In-Reply-To: <CACsJy8CWJvwSnmdpMg=atu+M8=4ksrTAYTgZyF5U7JnOCnPAkg@mail.gmail.com>
On Tue, Feb 7, 2017 at 10:13 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 2:18 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> Personally, I think that the fact that Git forces the user to think
>> about it in terms of "oh I have to fetch" instead of that happening
>> automatically, it helps teach the model to the user. If it happened in
>> the background then the user might not be confronted with the
>> distributed nature of the tool.
>
> I agree. But I think there is some room for improvement. Do we know
> when the last fetch of the relevant upstream is? If we do, and if it's
> been "a while" (configurable), then we should make a note suggesting
> fetching again in git-status.
>
> This is not exactly my own idea. Gentoo's portage (i.e. friends with
> apt-get, yum... if you're not familiar) also has this explicit "fetch"
> operation, which is called sync. If you haven't sync'd in a while and
> try to install new package, you get a friendly message (that helps me
> a couple times).
> --
> Duy
That seems reasonable.
Thanks,
Jake
^ permalink raw reply
* Re: Fwd: Possibly nicer pathspec syntax?
From: Duy Nguyen @ 2017-02-08 6:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.2.20.1702072112160.25002@i7.lan>
On Wed, Feb 8, 2017 at 12:12 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Two-patch series to follow.
glossary-content.txt update for both patches would be nice.
--
Duy
^ permalink raw reply
* Re: The --name-only option for git log does not play nicely with --graph
From: Duy Nguyen @ 2017-02-08 6:24 UTC (permalink / raw)
To: Davide Del Vento; +Cc: Git Mailing List
In-Reply-To: <CAMh-zaPdSGaDvQSiWx0p7zUmfDAFDWUyHkY4BTs=j85Ue65XnA@mail.gmail.com>
On Wed, Feb 8, 2017 at 6:11 AM, Davide Del Vento <ddvento@ucar.edu> wrote:
> `git log --graph --name-only` works fine, but the name is not
> properly indented as it is for `git log --graph --name-status`
>
> I tested this in git v1.9.1 the only one I have access at the moment
Confirmed still happens on master. --stat plays nicely with --graph
though, so it's probably just some small fixes somewhere in diff*.c.
--
Duy
^ permalink raw reply
* Re: Request re git status
From: Duy Nguyen @ 2017-02-08 6:13 UTC (permalink / raw)
To: Jacob Keller; +Cc: Samuel Lijin, Phil Hord, Ron Pero, Git
In-Reply-To: <CA+P7+xoZHOtURfbBbHHTpC3DsGxaGOVToqmW5wTg2EniRpL-Cg@mail.gmail.com>
On Wed, Feb 8, 2017 at 2:18 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> Personally, I think that the fact that Git forces the user to think
> about it in terms of "oh I have to fetch" instead of that happening
> automatically, it helps teach the model to the user. If it happened in
> the background then the user might not be confronted with the
> distributed nature of the tool.
I agree. But I think there is some room for improvement. Do we know
when the last fetch of the relevant upstream is? If we do, and if it's
been "a while" (configurable), then we should make a note suggesting
fetching again in git-status.
This is not exactly my own idea. Gentoo's portage (i.e. friends with
apt-get, yum... if you're not familiar) also has this explicit "fetch"
operation, which is called sync. If you haven't sync'd in a while and
try to install new package, you get a friendly message (that helps me
a couple times).
--
Duy
^ permalink raw reply
* Re: [PATCH] dir: avoid allocation in fill_directory()
From: Duy Nguyen @ 2017-02-08 6:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Brandon Williams, Git List
In-Reply-To: <73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de>
On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l.s.r@web.de> wrote:
> Pass the match member of the first pathspec item directly to
> read_directory() instead of using common_prefix() to duplicate it first,
> thus avoiding memory duplication, strlen(3) and free(3).
How about killing common_prefix()? There are two other callers in
ls-files.c and commit.c and it looks safe to do (but I didn't look
very hard).
> diff --git a/dir.c b/dir.c
> index 65c3e681b8..4541f9e146 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)
>
> int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
> {
> - char *prefix;
> + const char *prefix;
> size_t prefix_len;
>
> /*
> * Calculate common prefix for the pathspec, and
> * use that to optimize the directory walk
> */
> - prefix = common_prefix(pathspec);
> - prefix_len = prefix ? strlen(prefix) : 0;
> + prefix_len = common_prefix_len(pathspec);
> + prefix = prefix_len ? pathspec->items[0].match : "";
There's a subtle difference. Before the patch, prefix[prefix_len] is
NUL. After the patch, it's not always true. If some code (incorrectly)
depends on that, this patch exposes it. I had a look inside
read_directory() though and it looks like no such code exists. So, all
good.
>
> /* Read the directory and prune it */
> read_directory(dir, prefix, prefix_len, pathspec);
>
> - free(prefix);
> return prefix_len;
> }
--
Duy
^ permalink raw reply
* [PATCH v2] rev-list-options.txt: update --all about HEAD
From: Nguyễn Thái Ngọc Duy @ 2017-02-08 6:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170207133850.14056-1-pclouds@gmail.com>
This is the document patch for f0298cf1c6 (revision walker: include a
detached HEAD in --all - 2009-01-16).
Even though that commit is about detached HEAD, as Jeff pointed out,
always adding HEAD in that case may have subtle differences with
--source or --exclude. So the document mentions nothing about the
detached-ness.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
v2 drops "detached".
Documentation/rev-list-options.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5da7cf5a8d..a02f7324c0 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -133,8 +133,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
for all following revision specifiers, up to the next `--not`.
--all::
- Pretend as if all the refs in `refs/` are listed on the
- command line as '<commit>'.
+ Pretend as if all the refs in `refs/`, along with `HEAD`, are
+ listed on the command line as '<commit>'.
--branches[=<pattern>]::
Pretend as if all the refs in `refs/heads` are listed
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH 2/2] pathspec: don't error out on all-exclusionary pathspec patterns
From: Linus Torvalds @ 2017-02-08 5:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 7 Feb 2017 21:08:15 -0800
Subject: [PATCH 2/2] pathspec: don't error out on all-exclusionary pathspec patterns
Instead of erroring out and telling the user that they should add a
positive pattern that covers everything else, just _do_ that.
For commands where we honor the current cwd by default (ie grep, ls-files
etc), we make that default positive pathspec be the current working
directory. And for commands that default to the whole project (ie diff,
log, etc), the default positive pathspec is the whole project.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
pathspec.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index ecad03406..d8f78088c 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -522,7 +522,7 @@ void parse_pathspec(struct pathspec *pathspec,
}
pathspec->nr = n;
- ALLOC_ARRAY(pathspec->items, n);
+ ALLOC_ARRAY(pathspec->items, n+1);
item = pathspec->items;
prefixlen = prefix ? strlen(prefix) : 0;
@@ -546,10 +546,16 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->magic |= item[i].magic;
}
- if (nr_exclude == n)
- die(_("There is nothing to exclude from by :(exclude) patterns.\n"
- "Perhaps you forgot to add either ':/' or '.' ?"));
-
+ /*
+ * If everything is an exclude pattern, add one positive pattern
+ * that matches everyting. We allocated an extra one for this.
+ */
+ if (nr_exclude == n) {
+ if (!(flags & PATHSPEC_PREFER_CWD))
+ prefixlen = 0;
+ init_pathspec_item(item + n, 0, prefix, prefixlen, "");
+ pathspec->nr++;
+ }
if (pathspec->magic & PATHSPEC_MAXDEPTH) {
if (flags & PATHSPEC_KEEP_ORDER)
--
2.12.0.rc0.1.g02555c1b2.dirty
^ permalink raw reply related
* Re: Fwd: Possibly nicer pathspec syntax?
From: Linus Torvalds @ 2017-02-08 5:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqq4m05xph4.fsf@gitster.mtv.corp.google.com>
On Tue, 7 Feb 2017, Junio C Hamano wrote:
> > + // Special case alias for '!'
>
> /* style? */
Will fix.
> I somehow do not think this is correct. I expect
>
> cd t && git grep -e foo -- :^perf/
>
> to look into things in 't' except for things in 't/perf', but the
> above will grab hits from ../Documentation/ etc. We need to pay
> attention to PATHSPEC_PREFER_CWD bit in the flags word, I think.
Ok, that's easy enough.
Two-patch series to follow.
Linus
^ permalink raw reply
* [PATCH 1/2] pathspec magic: add '^' as alias for '!'
From: Linus Torvalds @ 2017-02-08 5:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 7 Feb 2017 21:05:28 -0800
Subject: [PATCH 1/2] pathspec magic: add '^' as alias for '!'
The choice of '!' for a negative pathspec ends up not only not matching
what we do for revisions, it's also a horrible character for shell
expansion since it needs quoting.
So add '^' as an alternative alias for an excluding pathspec entry.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
pathspec.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/pathspec.c b/pathspec.c
index 7ababb315..ecad03406 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -224,6 +224,12 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
char ch = *pos;
int i;
+ /* Special case alias for '!' */
+ if (ch == '^') {
+ *magic |= PATHSPEC_EXCLUDE;
+ continue;
+ }
+
if (!is_pathspec_magic(ch))
break;
--
2.12.0.rc0.1.g02555c1b2.dirty
^ permalink raw reply related
* Re: Fwd: Possibly nicer pathspec syntax?
From: Junio C Hamano @ 2017-02-08 4:42 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <CA+55aFyAEaMKA+2oPJct4ffJ0-_z2vrYxmQ9yrkbxzB3Hk6WfQ@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> People don't expect set theory from their pathspecs. They expect their
> pathspecs to limit the output. They've learnt that within a
> subdirectory, the pathspec limits to that subdirectory. And now it
> suddenly starts showing things outside the subdirectory?
>
> At that point no amount of "but but think about set theory" will
> matter, methinks.
I do not feel too strongly about it, either, but when we invoke
"what would people who go down into subdirectories expect", the
issue becomes a lot bigger.
"git diff/log" without any pathspec in a subdirectory still shows
the whole diff. "git grep" looks for hits inside that subdirectory,
on the other hand.
I think the former design decision is mostly a historical accident.
"The project tree as the whole is what matters" was one reason, and
another is that historically we didn't have ":/"--defaulting to the
whole tree and telling people to give "." was easier than defaulting
to the current and telling people to give "../.." after counting how
many levels deep you started at. If we were designing the system
today with "." and ":/", I wouldn't be surprised if we chose "always
limit to cwd if there is no pathspec" for any command for the sake
of consistency. We can easily say "if you want whole-tree, pass :/"
instead.
But we do not live in that world, and I do not think change them to
make things consistent is what we are discussing in this thread.
Given users accept and expect that "diff" without pathspec is whole
tree, while "grep" without pathspec is limited to cwd, what is the
reasonable definition of "everything from which negative ones are
excluded"? That is the question I am trying to answer.
As Mike mentioned earlier in the thread, if we used "." (limit to
cwd) for "diff/log" when there are only negative pathspec elements,
the resulting UI would become inconsistent within a single command,
as in my world view, giving no pathspec means "work on everything
you would ordinarily work on", a positive pathspec means "among
everything you would ordinarily work on, only work on the ones that
match this pattern", and giving only a negative one ought to mean
"among everything you would ordinarily work on, only work on the
ones that do NOT match this pattern."
> pathspec.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 7ababb315..2a91247bc 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -224,6 +224,12 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
> char ch = *pos;
> int i;
>
> + // Special case alias for '!'
/* style? */
> + if (ch == '^') {
> + *magic |= PATHSPEC_EXCLUDE;
> + continue;
> + }
> +
> if (!is_pathspec_magic(ch))
> break;
>
> + /*
> + * If everything is an exclude pattern, add one positive pattern
> + * that matches everyting. We allocated an extra one for this.
> + */
> + if (nr_exclude == n) {
> + init_pathspec_item(item + n, 0, "", 0, "");
> + pathspec->nr++;
> + }
I somehow do not think this is correct. I expect
cd t && git grep -e foo -- :^perf/
to look into things in 't' except for things in 't/perf', but the
above will grab hits from ../Documentation/ etc. We need to pay
attention to PATHSPEC_PREFER_CWD bit in the flags word, I think.
A real change probably wants to become a two-patch series (one for
the new :^ alias, the other is for "everything except...") with log
message ;-)
^ permalink raw reply
* Re: Fwd: Possibly nicer pathspec syntax?
From: Linus Torvalds @ 2017-02-08 3:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqa89xxtnd.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2644 bytes --]
On Tue, Feb 7, 2017 at 7:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> But that is not what I was talking about. Let's simplify. I'd say
> for any command that acts on "everything" when pathspec is not
> given, the two sets of actual paths affected by these two:
>
> git cmd -- "net/"
> git cmd -- ":!net/"
>
> should have no overlap (obviously) and when you take union of the
> two sets, that should equal to
>
> git cmd --
>
> i.e. no pathspecs.
Well, as mentioned, I won't ever care. I'm certainly ok with the "make
the default positive entry be everything".
I just suspect that from a user perspective that actually delves into
the subdirectories, the much bigger question will be: "I gave you a
pathspec, and suddenly you start giving me stuff from outside the area
entirely".
And then you can say "well, just add '.' to the pathspec", and you'd
be right, but I still think it's not what a naive user would expect.
People don't expect set theory from their pathspecs. They expect their
pathspecs to limit the output. They've learnt that within a
subdirectory, the pathspec limits to that subdirectory. And now it
suddenly starts showing things outside the subdirectory?
At that point no amount of "but but think about set theory" will
matter, methinks.
But I really don't feel strongly about it. The path I sent out (and
the slightly modified version attached in this email) actually acts
the way you suggest. It's certainly the simplest implementation. I
just suspect it's not the implementation people who go down into
subdirectories would want/expect.
>>> 2. I am not sure what ctype.c change is about. Care to elaborate?
>>
>> I didn't see the need for it either until I made the rest of the
>> patch, and it didn't work at all.
>>
>> The pathspec.c code uses "if (is_pathspec_magic(..))" to test whether
>> a character is a short magiic pathspec character. But '^' wasn't in
>> that set, because it was already marked as being (only) in the regex
>> set.
>>
>> Does that whole is_pathspec_magic() thing make any sense when we have
>> an array that specifies the special characters we react to? No it does
>> not.
>>
>> But it is what the code does, and I just made that code work.
>
> Ah, OK.
Side note: here's an alternative patch that avoids that issue
entirely, and also avoids a problem with prefix_magic() being uphappy
when one bit shows up multiple times in the array.
It's slightly hacky in parse_short_magic(), but it's smaller and
simpler, and avoids the two subtle cases. No need for ctype changes,
and no issues with prefix_magic() being somewhat stupid.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1293 bytes --]
pathspec.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 7ababb315..2a91247bc 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -224,6 +224,12 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
char ch = *pos;
int i;
+ // Special case alias for '!'
+ if (ch == '^') {
+ *magic |= PATHSPEC_EXCLUDE;
+ continue;
+ }
+
if (!is_pathspec_magic(ch))
break;
@@ -516,7 +522,7 @@ void parse_pathspec(struct pathspec *pathspec,
}
pathspec->nr = n;
- ALLOC_ARRAY(pathspec->items, n);
+ ALLOC_ARRAY(pathspec->items, n+1);
item = pathspec->items;
prefixlen = prefix ? strlen(prefix) : 0;
@@ -540,10 +546,14 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->magic |= item[i].magic;
}
- if (nr_exclude == n)
- die(_("There is nothing to exclude from by :(exclude) patterns.\n"
- "Perhaps you forgot to add either ':/' or '.' ?"));
-
+ /*
+ * If everything is an exclude pattern, add one positive pattern
+ * that matches everyting. We allocated an extra one for this.
+ */
+ if (nr_exclude == n) {
+ init_pathspec_item(item + n, 0, "", 0, "");
+ pathspec->nr++;
+ }
if (pathspec->magic & PATHSPEC_MAXDEPTH) {
if (flags & PATHSPEC_KEEP_ORDER)
^ permalink raw reply related
* Re: Fwd: Possibly nicer pathspec syntax?
From: Junio C Hamano @ 2017-02-08 3:12 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <CA+55aFwPLtuPciN1o_03CwkKqFWgZd_br9Q14qyr7a7N7mxTeA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> No. The thing is, "git diff" is relative too - for path
> specifications. And the negative entries are pathspecs - and they act
> as relative ones.
>
> IOW, that whole
>
> cd drivers
> git diff A..B -- net/
>
> will actually show the diff for drivers/net - so the pathspec very
> much acts as relative to the cwd.
But that is not what I was talking about. Let's simplify. I'd say
for any command that acts on "everything" when pathspec is not
given, the two sets of actual paths affected by these two:
git cmd -- "net/"
git cmd -- ":!net/"
should have no overlap (obviously) and when you take union of the
two sets, that should equal to
git cmd --
i.e. no pathspecs.
>> 2. I am not sure what ctype.c change is about. Care to elaborate?
>
> I didn't see the need for it either until I made the rest of the
> patch, and it didn't work at all.
>
> The pathspec.c code uses "if (is_pathspec_magic(..))" to test whether
> a character is a short magiic pathspec character. But '^' wasn't in
> that set, because it was already marked as being (only) in the regex
> set.
>
> Does that whole is_pathspec_magic() thing make any sense when we have
> an array that specifies the special characters we react to? No it does
> not.
>
> But it is what the code does, and I just made that code work.
Ah, OK.
^ permalink raw reply
* Re: Fwd: Possibly nicer pathspec syntax?
From: Mike Hommey @ 2017-02-08 2:40 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.2.20.1702071739060.17609@i7.lan>
On Tue, Feb 07, 2017 at 05:48:26PM -0800, Linus Torvalds wrote:
>
>
> On Tue, 7 Feb 2017, Linus Torvalds wrote:
> >
> > [ Clarification from original message, since Junio asked: I didn't
> > actually want the semantics of '.' at all, since in a subdirectory it
> > limits to the current subdirectory. So I'd suggest that in the absence
> > of any positive pattern, there is simply no filtering at all, so
> > whenever I say '.' as a pattern, I really meant ":(top)." which is
> > even more of a cumbersom syntax that the current model really
> > encourages. Crazy. Since I tend to always work in the top directory,
> > the two are the same for me ]
>
> So here's an RFC patch, and I'm quoting the above part of my thinking
> because it's what the patch does, but it turns out that it's probably not
> what we want, and I suspect the "." behavior (as opposed to "no filtering
> at all") is actually better.
>
> Now _I_ don't much care, since I only work from the top level, but without
> the "." behavior, you get into an odd situation that the negative match
> will be relative to the current directory, but then the positive matches
> will be everywhere else.
>
> Obviously, a negative match that has "top" set would change that logic. So
> this patch is purely a request for further discussion.
>
> When I wrote the patch, I actually also removed the now stale entries from
> the 'po' files, but I'm not including that part here because it just
> distracts from the meat of it all. So this diff was actually generated
> with the new syntax:
>
> git diff -p --stat -- :^po/
>
> and the only thing even remotely subtle here is that it changes our ctype
> array to make '^' be both a regex and a pathspec magic character.
>
> Everything else should be pretty darn obvious.
>
> The code *could* just track all the 'relative to top or not' bits in the
> exclusion pattern, and then use whatever top-ness the exclusion patterns
> have (and maybe fall back to the old warning if it had a mixture of
> exclusionary patterns). I'll happily change it to act that way if people
> think that makes sense.
>
> Comments?
It seems to me that `git diff` and `git diff -- :^stuff` should have the
same output if there aren't changes in stuff, and `git diff` does the
same as `git diff -- :/` if you are in a subdirectory, not the same as
`git diff .`.
As such, the default positive match should be ':/' (which is shorter and
less cumbersome than ':(top)', btw)
Mike
^ 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