* Re: [PATCH] valgrind: support test helpers
From: René Scharfe @ 2016-10-29 9:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Duy Nguyen, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1610281050220.3264@virtualbox>
Am 28.10.2016 um 10:51 schrieb Johannes Schindelin:
> On Fri, 28 Oct 2016, René Scharfe wrote:
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>
> Apart from the missing accent ("é") in your SOB: ACK.
I sign off without accent out of habit, to avoid display problems --
better have a plain "e" than something like "<C3><A9>" or worse.
But only now I realized that I can fix at least my end by using an UTF-8
locale (e.g. LANG=C.UTF-8 instead of LANG=C) -- together with PuTTY and
its settings Window, Translation, Remote character set: UTF-8 and
Connection, Data, Terminal-type-string: linux, which I already had. My
terminal just got boosted into the 21st century, woohoo! ;)
René
^ permalink raw reply
* Re: Is the entire working copy “at one branch”?
From: Alexei Lozovsky @ 2016-10-29 9:47 UTC (permalink / raw)
To: Stefan Monov; +Cc: git
In-Reply-To: <CAJtFkWs4qYCqnbJD+zCRCAW3teczb4CdvncvYoMN_VvthJGr=w@mail.gmail.com>
Hi Stefan,
Generally with git, your entire working copy will have the same
revision (set to current branch, aka HEAD). The idea behind this
is that your working copy of a repository should always be in
consistent state.
You can check out specific files or directories from another
revision (mimicking "svn update -r1234 filename"):
$ git checkout branch-or-sha-hash -- filename
However, SVN tracks the 'revision' thing on per-file basis, while
in git this is a property of the working copy. So if you do like
above then git will be telling you that the 'filename' has been
changed (as it is certainly different from its pristine version
in HEAD):
$ git status
On branch master
Changes to be committed:
(use "git reset HEAD <file>..." to unstage)
modified: filename
So it's generally not recommended to do such a thing.
Another thing that you _can do_ in git to mimick SVN is the
'standard layout'. There is a feature called "git worktree" which
allows you to have SVN-like directory structure with multiple
directories linked to different working copies:
$ mkdir my-project
$ cd my-project
$ git clone my-project-repository master
$ mkdir branches
$ cd master
$ git worktree add -b branch-1 ../branches/branch-1
$ git worktree add -b branch-2 ../branches/branch-2
After that you will have directory structure like this:
$ tree my-project
my-project
├── branches
│ ├── branch-1
│ │ ├── 1
│ │ ├── 2
│ │ └── 3
│ └── branch-2
│ ├── 1
│ ├── 2
│ └── banana
└── master
├── 1
└── 2
You can work with these working copies separately, like you
would be working with SVN. Commits in 'master' will go to the
'master' branch, commits made in 'branches/branch-1' will go
to the 'branch-1' branch.
^ permalink raw reply
* Re: [PATCH v2 2/2] convert.c: stream and fast search for binary
From: Duy Nguyen @ 2016-10-29 12:13 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
In-Reply-To: <20161012134727.28365-1-tboegi@web.de>
On Wed, Oct 12, 2016 at 8:47 PM, <tboegi@web.de> wrote:
> From: Torsten Bögershausen <tboegi@web.de>
>
> When statistics are done for the autocrlf handling, the search in
> the content can be stopped, if e.g
> - a search for binary is done, and a NUL character is found
> - a search for CRLF is done, and the first CRLF is found.
>
> Similar when statistics for binary vs non-binary are gathered:
> Whenever a lone CR or NUL is found, the search can be aborted.
>
> When checking out files in "auto" mode, any file that has a "lone CR"
> or a CRLF will not be converted, so the search can be aborted early.
>
> Add the new bit, CONVERT_STAT_BITS_ANY_CR,
> which is set for either lone CR or CRLF.
>
> Many binary files have a NUL very early and it is often not necessary
> to load the whole content of a file or blob into memory.
>
> Split gather_stats() into gather_all_stats() and gather_stats_partly()
> to do a streaming handling for blobs and files in the worktree.
Maybe break this commit down a bit? the gather_all_stats and
gather_stats_partly() seem independent and can standalone. So is the
blob streaming, and get_convert_stats_wt.
--
Duy
^ permalink raw reply
* Re: [PATCH v2 1/2] read-cache: factor out get_sha1_from_index() helper
From: Duy Nguyen @ 2016-10-29 12:22 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
In-Reply-To: <20161012134726.28326-1-tboegi@web.de>
On Wed, Oct 12, 2016 at 8:47 PM, <tboegi@web.de> wrote:
> From: Torsten Bögershausen <tboegi@web.de>
>
> Factor out the retrieval of the sha1 for a given path in
> read_blob_data_from_index() into the function get_sha1_from_index().
>
> This will be used in the next commit, when convert.c can do the
> analyze for "text=auto" without slurping the whole blob into memory
> at once.
>
> Add a wrapper definition get_sha1_from_cache().
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> cache.h | 3 +++
> read-cache.c | 29 ++++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 1604e29..04de209 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -380,6 +380,7 @@ extern void free_name_hash(struct index_state *istate);
> #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
> #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec)
> #define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))
> +#define get_sha1_from_cache(path) get_sha1_from_index (&the_index, (path))
> #endif
>
> enum object_type {
> @@ -1089,6 +1090,8 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
> return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
> }
>
> +const unsigned char *get_sha1_from_index(struct index_state *istate, const char *path);
> +
> /*
> * This internal function is only declared here for the benefit of
> * lookup_replace_object(). Please do not call it directly.
> diff --git a/read-cache.c b/read-cache.c
> index 38d67fa..5a1df14 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2290,13 +2290,27 @@ int index_name_is_other(const struct index_state *istate, const char *name,
>
> void *read_blob_data_from_index(struct index_state *istate, const char *path, unsigned long *size)
> {
> - int pos, len;
> + const unsigned char *sha1;
> unsigned long sz;
> enum object_type type;
> void *data;
>
> - len = strlen(path);
> - pos = index_name_pos(istate, path, len);
> + sha1 = get_sha1_from_index(istate, path);
> + if (!sha1)
> + return NULL;
> + data = read_sha1_file(sha1, &type, &sz);
> + if (!data || type != OBJ_BLOB) {
> + free(data);
> + return NULL;
> + }
> + if (size)
> + *size = sz;
> + return data;
> +}
> +
> +const unsigned char *get_sha1_from_index(struct index_state *istate, const char *path)
Let's try to embrace struct object_id to make our lives easier when
the time comes to convert to a new hash algorithm by returning struct
object_id * here instead of the internal hash.
> +{
> + int pos = index_name_pos(istate, path, strlen(path));
> if (pos < 0) {
> /*
> * We might be in the middle of a merge, in which
> @@ -2312,14 +2326,7 @@ void *read_blob_data_from_index(struct index_state *istate, const char *path, un
> }
> if (pos < 0)
> return NULL;
> - data = read_sha1_file(istate->cache[pos]->oid.hash, &type, &sz);
> - if (!data || type != OBJ_BLOB) {
> - free(data);
> - return NULL;
> - }
> - if (size)
> - *size = sz;
> - return data;
> + return istate->cache[pos]->oid.hash;
> }
>
> void stat_validity_clear(struct stat_validity *sv)
--
Duy
^ permalink raw reply
* Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines
From: Christian Couder @ 2016-10-29 12:25 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <db609e13740415ac4e5e357493661347b0f681f7.1477698917.git.jonathantanmy@google.com>
On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> trailer.c currently splits lines while processing a buffer (and also
> rejoins lines when needing to invoke ignore_non_trailer).
>
> Avoid such line splitting, except when generating the strings
> corresponding to trailers (for ease of use by clients - a subsequent
> patch will allow other components to obtain the layout of a trailer
> block in a buffer, including the trailers themselves). The main purpose
> of this is to make it easy to return pointers into the original buffer
> (for a subsequent patch), but this also significantly reduces the number
> of memory allocations required.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 116 insertions(+), 99 deletions(-)
IMHO it is telling that this needs 17 more lines.
> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
> {
> LIST_HEAD(head);
> LIST_HEAD(arg_head);
> - struct strbuf **lines;
> + struct strbuf sb = STRBUF_INIT;
We often use "sb" as the name of strbuf variables, but I think at
least here (and maybe in other places above) we could use something a
bit more telling, like "input_buf" perhaps.
> int trailer_end;
> FILE *outfile = stdout;
>
^ permalink raw reply
* Re: [PATCH 0/4] Make other git commands use trailer layout
From: Christian Couder @ 2016-10-29 12:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git
In-Reply-To: <xmqqzilovtye.fsf@gitster.mtv.corp.google.com>
On Sat, Oct 29, 2016 at 3:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> This is built off jt/trailer-with-cruft (commit 60ef86a).
>>
>> This patch set makes "commit -s", "cherry-pick -x", and
>> "format-patch --signoff" use the new trailer definition implemented in
>> jt/trailer-with-cruft, with some refactoring along the way. With this
>> patch set, the aforementioned commands would now handle trailers like
>> those described in [1].
>>
>> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>
>
> Ooooh. Looks delicious ;-)
Yeah, I am very happy with this goal being reached :-)
Thanks,
Christian.
^ permalink raw reply
* [PATCH] commit: simplify building parents list
From: René Scharfe @ 2016-10-29 12:55 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Push pptr down into the FROM_MERGE branch of the if/else statement,
where it's actually used, and call commit_list_append() for appending
elements instead of playing tricks with commit_list_insert(). Call
copy_commit_list() in the amend branch instead of open-coding it. Don't
bother setting pptr in the final branch as it's not used thereafter.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/commit.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index a2baa6e..8976c3d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1642,7 +1642,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct commit_list *parents = NULL, **pptr = &parents;
+ struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
@@ -1688,20 +1688,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
if (!reflog_msg)
reflog_msg = "commit (initial)";
} else if (amend) {
- struct commit_list *c;
-
if (!reflog_msg)
reflog_msg = "commit (amend)";
- for (c = current_head->parents; c; c = c->next)
- pptr = &commit_list_insert(c->item, pptr)->next;
+ parents = copy_commit_list(current_head->parents);
} else if (whence == FROM_MERGE) {
struct strbuf m = STRBUF_INIT;
FILE *fp;
int allow_fast_forward = 1;
+ struct commit_list **pptr = &parents;
if (!reflog_msg)
reflog_msg = "commit (merge)";
- pptr = &commit_list_insert(current_head, pptr)->next;
+ pptr = commit_list_append(current_head, pptr);
fp = fopen(git_path_merge_head(), "r");
if (fp == NULL)
die_errno(_("could not open '%s' for reading"),
@@ -1712,7 +1710,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
parent = get_merge_parent(m.buf);
if (!parent)
die(_("Corrupt MERGE_HEAD file (%s)"), m.buf);
- pptr = &commit_list_insert(parent, pptr)->next;
+ pptr = commit_list_append(parent, pptr);
}
fclose(fp);
strbuf_release(&m);
@@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
reflog_msg = (whence == FROM_CHERRY_PICK)
? "commit (cherry-pick)"
: "commit";
- pptr = &commit_list_insert(current_head, pptr)->next;
+ commit_list_insert(current_head, &parents);
}
/* Finally, get the commit message */
--
2.10.2
^ permalink raw reply related
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Jeff King @ 2016-10-29 13:39 UTC (permalink / raw)
To: Matt McCutchen; +Cc: Junio C Hamano, git
In-Reply-To: <1477712029.2904.64.camel@mattmccutchen.net>
On Fri, Oct 28, 2016 at 11:33:49PM -0400, Matt McCutchen wrote:
> On Fri, 2016-10-28 at 18:11 -0700, Junio C Hamano wrote:
> > Ah, I see. My immediate reaction is that you can do worse things in
> > the reverse direction compared to this, but your scenario does sound
> > bad already.
>
> Are you saying that clients connecting to untrusted servers already
> face worse risks that people should know about, so there is no point in
> documenting this one? I guess I don't know about the other risks aside
> from accepting a corrupt object, which should be preventable by
> enabling fetch.fsckObjects. It seems we need either a statement that
> connecting to untrusted servers is officially unsupported or a
> description of the specific risks.
I'm not sure I understand how connecting to a remote server to fetch is
a big problem. The server may learn about the existence of particular
sha1s in your repository, but cannot get their content.
It's the subsequent push that is a problem.
In the scenarios you've described, I'm mostly inclined to say that the
problem is not git or the protocol itself, but rather lax refspecs.
You mentioned earlier:
the server can just generate another ref 'xx' pointing to Y2, assuming
it can entice the victim to set up a corresponding local branch
refs/heads/for-server1/xx and push it back. Or if the victim is for
some reason just mirroring back and forth:
This sounds a lot like "I told git to push a bunch of things without
checking if they were really secret, and it turned out to push some
secret things". IOW I think the problem is not that the server may lie
about what it has, but that the user was not careful about what they
pushed. I dunno. I do not mind making a note in the documentation
explaining the implications of a server lying, but the scenarios seem
pretty contrived to me.
A much more interesting one, IMHO, is a server whose receive-pack lies
about which objects it has (possibly ones it found out about earlier via
fetch), which provokes the client to generate deltas against objects the
server doesn't have (and thereby leaking information about the base
objects).
That is a problem no matter how careful your refspecs are. I suspect it
would be a hard attack to pull off in practice, just because it's going
to depend heavily on the content of the specific objects, what kinds of
deltas you can convince the other side to generate, etc. That might
merit a mention in the git-push documentation.
-Peff
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-10-29 16:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <CAPc5daVOxmowdiTU3ScFv6c_BRVEJ+G92gx_AmmKnR-WxUKv-Q@mail.gmail.com>
On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
> Not sending to the list, where mails from Gmail/phone is known to get
> rejected.
[I guess I can go ahead and quote this to the list.]
> No. I'm saying that the scenario you gave is bad and people should be
> taught not to connect to untrustworthy sites.
To clarify, are you saying:
(1) don't connect to an untrusted server ever (e.g., we don't promise
that the server can't execute arbitrary code on the client), or
(2) don't connect to an untrusted server if the client repository has
data that needs to be kept secret from the server?
The fetch/push attack relates only to #2. If #1, what are the other
risks you are thinking of?
Matt
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-10-29 16:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20161029133959.kpkohjkku3jgwjql@sigill.intra.peff.net>
On Sat, 2016-10-29 at 09:39 -0400, Jeff King wrote:
> I'm not sure I understand how connecting to a remote server to fetch is
> a big problem. The server may learn about the existence of particular
> sha1s in your repository, but cannot get their content.
>
> It's the subsequent push that is a problem.
>
> In the scenarios you've described, I'm mostly inclined to say that the
> problem is not git or the protocol itself, but rather lax refspecs.
> You mentioned earlier:
>
> the server can just generate another ref 'xx' pointing to Y2, assuming
> it can entice the victim to set up a corresponding local branch
> refs/heads/for-server1/xx and push it back. Or if the victim is for
> some reason just mirroring back and forth:
>
> This sounds a lot like "I told git to push a bunch of things without
> checking if they were really secret, and it turned out to push some
> secret things". IOW I think the problem is not that the server may lie
> about what it has, but that the user was not careful about what they
> pushed. I dunno. I do not mind making a note in the documentation
> explaining the implications of a server lying, but the scenarios seem
> pretty contrived to me.
Let's focus on the first scenario. There the user is just pulling and
pushing a master branch. Are you saying that each time the user pulls,
they need to look over all the commits they pulled before pushing them
back? I think that's unrealistic, for example, on a busy project with
centralized code review or if the user is publishing a project-specific
modified version of an upstream library. The natural user expectation
is that anything pulled from a public repository is public.
But let's see what Junio says in the other subthread.
> A much more interesting one, IMHO, is a server whose receive-pack lies
> about which objects it has (possibly ones it found out about earlier via
> fetch), which provokes the client to generate deltas against objects the
> server doesn't have (and thereby leaking information about the base
> objects).
>
> That is a problem no matter how careful your refspecs are. I suspect it
> would be a hard attack to pull off in practice, just because it's going
> to depend heavily on the content of the specific objects, what kinds of
> deltas you can convince the other side to generate, etc. That might
> merit a mention in the git-push documentation.
Sure, if I end up doing a patch, I'll include this.
Matt
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-29 17:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Jeff King, Git Mailing List, Lars Schneider,
Eric Wong
In-Reply-To: <alpine.DEB.2.20.1610291022120.3264@virtualbox>
On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Correct. We cannot change an open file handle's state ("FD_CLOEXEC") on
> Windows, but we can ask open() to open said file handle with the correct
> flag ("O_CLOEXEC", which is mapped to O_NOINHERIT on Windows.
Ok. So then I have no issues with it, and let's use O_CLOEXEC if it
exists and fcntl(FD_CLOEXEC) if O_CLOEXEC doesn't exist.
Linus
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Jon Loeliger @ 2016-10-29 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt McCutchen, git
In-Reply-To: <xmqq7f8sx8lg.fsf@gitster.mtv.corp.google.com>
So, like, Junio C Hamano said:
> Matt McCutchen <matt@mattmccutchen.net> writes:
>
> > Then the server generates a commit X3 that lists Y2 as a parent, even
> > though it doesn't have Y2, and advances 'x' to X3. The victim fetches
> > 'x':
> >
> > victim server
> >
> > Y1---Y2---- (Y2)
> > / \ \
> > ---O---O---X1---X2---X3 ---O---O---X1---X2---X3
> >
> > Then the server rolls back 'x' to X2:
> >
> > victim server
> >
> > Y1---Y2----
> > / \
> > ---O---O---X1---X2---X3 ---O---O---X1---X2
>
> Ah, I see. My immediate reaction is that you can do worse things in
> the reverse direction compared to this, but your scenario does sound
> bad already.
Is there an existing protocol provision, or an extension to
the protocol that would allow a distrustful client to say to
the server, "Really, you have Y2? Prove it." And expect the
server to respond with a SHA1 sequence back to a common SHA
(in this case the left-most O). If so, a user could designate
some branch (Y) as "sensitive". Or, a whole repo could be
so designated and the client then effectivey treats the server
as a semi-hostile witness.
Dunno.
jdl
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Jeff King @ 2016-10-29 19:10 UTC (permalink / raw)
To: Matt McCutchen; +Cc: Junio C Hamano, git
In-Reply-To: <1477757311.1524.21.camel@mattmccutchen.net>
On Sat, Oct 29, 2016 at 12:08:31PM -0400, Matt McCutchen wrote:
> Let's focus on the first scenario. There the user is just pulling and
> pushing a master branch. Are you saying that each time the user pulls,
> they need to look over all the commits they pulled before pushing them
> back? I think that's unrealistic, for example, on a busy project with
> centralized code review or if the user is publishing a project-specific
> modified version of an upstream library. The natural user expectation
> is that anything pulled from a public repository is public.
No, I'm saying if you are running "git push foo master", then you should
expect the contents of "master" to go to "foo". That _could_ have
security implications if you come up with a sequence of events where
secret things made it to "master". But it seems to me that "foo
previously lied to you about what it has" is not the weak link in that
chain. It is not thinking about what secret things are hitting the
master that you are pushing, no matter how they got there.
I agree there is a potential workflow (that you have laid out) where
such lying can cause an innocent-looking sequence of events to disclose
the secret commits. And again, I don't mind a note in the documentation
mentioning that. I just have trouble believing it's a common one in
practice.
The reason I brought up the delta thing, even though it's a much harder
attack to execute, is that it comes up in much more common workflows,
like simply fetching from a private security-sensitive repo into your
"main" public repo (which is an example you brought up, and something I
know that I have personally done in the past for git.git).
-Peff
^ permalink raw reply
* Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions
From: Christian Couder @ 2016-10-29 22:06 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8DPt3EJoSTVEZFbH6xXbh78MbLZ4h+50K4eoFxPYSaN=Q@mail.gmail.com>
On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> +void remove_split_index(struct index_state *istate)
>> +{
>> + if (istate->split_index) {
>> + /*
>> + * can't discard_split_index(&the_index); because that
>> + * will destroy split_index->base->cache[], which may
>> + * be shared with the_index.cache[]. So yeah we're
>> + * leaking a bit here.
>
> In the context of update-index, this is a one-time thing and leaking
> is tolerable. But because it becomes a library function now, this leak
> can become more serious, I think.
>
> The only other (indirect) caller is read_index_from() so probably not
> bad most of the time (we read at the beginning of a command only).
> sequencer.c may discard and re-read the index many times though,
> leaking could be visible there.
So is it enough to check if split_index->base->cache[] is shared with
the_index.cache[] and then decide if discard_split_index(&the_index)
should be called?
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Christian Couder @ 2016-10-29 22:19 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:00 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> When users are using `git update-index --(no-)split-index`, they
>> may expect the split-index feature to be used or not according to
>> the option they just used, but this might not be the case if the
>> new "core.splitIndex" config variable has been set. In this case
>> let's warn about what will happen and why.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> builtin/update-index.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (split_index > 0) {
>> + if (git_config_get_split_index() == 0)
>> + warning("core.splitIndex is set to false; "
>> + "remove or change it, if you really want to "
>> + "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.
Ok, it will be in the next version.
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Christian Couder @ 2016-10-29 22:24 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> + int val = -1;
>> +
>> + if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> + if (0 <= val && val <= 100)
>> + return val;
>> +
>> + error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().
Ok, it will be in the next version.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
From: Christian Couder @ 2016-10-29 22:40 UTC (permalink / raw)
To: Ramsay Jones
Cc: git, Junio C Hamano, Nguyen Thai Ngoc Duy,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <c30212bd-c454-f1a1-d01c-d6a12d20d17d@ramsayjones.plus.com>
On Sun, Oct 23, 2016 at 6:07 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>>
>> +int too_many_not_shared_entries(struct index_state *istate)
>
> This function is a file-loacal symbol; could you please make it
> a static function.
Ok, it will be in the next version.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
From: Christian Couder @ 2016-10-29 22:58 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8CfQ6d3Q74W4rm=rJD69EAzuUe7PdrW-5NDo0vHuDSNpw@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:16 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>> if ((v & 15) < 6)
>> istate->cache_changed |= SPLIT_INDEX_ORDERED;
>> }
>> - if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
>> + if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
>> + too_many_not_shared_entries(istate)) {
>
> It's probably safer to keep this piece unchanged and add this
> somewhere before it
>
> if (too_many_not_shared_entries(istate))
> istate->cache_changed |= SPLIT_INDEX_ORDERED;
>
> We could keep cache_changed consistent until the end this way.
Ok, it will be in the next version.
>> test_expect_success 'enable split index' '
>> + git config splitIndex.maxPercentChange 100 &&
>
> An alternative name might be splitThreshold. I don't know, maybe
> maxPercentChange is better.
I think it is important to say that it is a percent in the name, so I
prefer maxPercentChange.
Thanks,
Christian.
^ permalink raw reply
* [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-30 2:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: 842477, git, Vasco Almeida
v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
translation” broke outside scripts such as guilt that source
git-sh-setup as described in the documentation:
$ . "$(git --exec-path)/git-sh-setup"
sh: 6: .: git-sh-i18n: not found
This also affects contrib/convert-grafts-to-replace-refs.sh and
contrib/rerere-train.sh in tree. Fix this by using git --exec-path to
find git-sh-i18n.
While we’re here, move the sourcing of git-sh-i18n below the shell
portability fixes.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
Is this a supported use of git-sh-setup? Although the documentation is
clear that the end user should not invoke it directly, it seems to imply
that scripts may do this, and in practice it has worked until v2.10.0.
git-sh-setup.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index a8a4576..240c7eb 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -2,9 +2,6 @@
# to set up some variables pointing at the normal git directories and
# a few helper shell functions.
-# Source git-sh-i18n for gettext support.
-. git-sh-i18n
-
# Having this variable in your environment would break scripts because
# you would cause "cd" to be taken to unexpected places. If you
# like CDPATH, define it for your interactive shell sessions without
@@ -46,6 +43,9 @@ git_broken_path_fix () {
# @@BROKEN_PATH_FIX@@
+# Source git-sh-i18n for gettext support.
+. "$(git --exec-path)/git-sh-i18n"
+
die () {
die_with_status 1 "$@"
}
--
2.10.1
^ permalink raw reply related
* Re: Expanding Includes in .gitignore
From: Duy Nguyen @ 2016-10-30 3:09 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Aaron Pelly, Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On Fri, Oct 28, 2016 at 4:04 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
>
>> > I think the normal behavior in such "foo.d" directory is to just sort
>> > the contents lexically and read them in order, as if they were all
>> > concatenated together, and with no recursion. I.e., behave "as if" the
>> > user had run "cat $dir/*".
>>
>> Yea, this is the normal behavior, and the user is expected to order
>> their files lexically such as "00-name", "50-name" and so on. Pretty
>> traditional for a lot of newer configurations.
>
> One thing I will say about this approach is that you can implement it
> without any changes in git by doing:
>
> path=.git/info/exclude
> cat $path.d/* >$path
>
> and I have seen several config mechanisms basically do that (e.g.,
> Debian packaging for a program that doesn't have its own ".d" mechanism,
> but needs to grab config provided by several separate packages).
My first thought at this .git/info/exclude.d was "oh no I have to
teach untracked cache about new dependencies, or at least disable it
until it can deal with exclude.d", but this "cat" approach simplifies
things and should keep untracked cache unchanged.
There may be complication with negative patterns though. The user may
want to limit the effect of negative patterns within individual
exclude files in exclude.d so a negative pattern in exclude.d/a won't
influence anything in exclude.d/b (easier to reason, safer to compose
different exclude sets). The plain "cat" would lose file boundary info
that we need. I'm not sure. But I'll dig more into it when patches
show up.
--
Duy
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Duy Nguyen @ 2016-10-30 3:16 UTC (permalink / raw)
To: Aaron Pelly; +Cc: Junio C Hamano, Jeff King, Git Mailing List
In-Reply-To: <cc23eece-d693-9e40-78fe-3bafe6bcad3a@pelly.co>
On Fri, Oct 28, 2016 at 4:32 PM, Aaron Pelly <aaron@pelly.co> wrote:
> Or how about a new githook that can intelligently create or return the
> details? This would be my least favourite option unless it was
> configured in an obvious place.
I wonder if smudge/clean filters can be used to recreate in-tree
.gitignore from .gitignore.d/*.
--
Duy
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 7:53 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20161029191023.ztrfe76u4gi4l3ci@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... It is not thinking about what secret things are hitting the
> master that you are pushing, no matter how they got there.
>
> I agree there is a potential workflow (that you have laid out) where
> such lying can cause an innocent-looking sequence of events to disclose
> the secret commits. And again, I don't mind a note in the documentation
> mentioning that. I just have trouble believing it's a common one in
> practice.
I'd say I agree with the above. I am not sure how easy people
employing common workflows can be tricked into the scenario Matt
presented, either, but I do not think it would hurt to warn people
that they need to be careful not to pull from or push to an
untrustworthy place or push things you are not sure that are clean.
> The reason I brought up the delta thing, even though it's a much harder
> attack to execute, is that it comes up in much more common workflows,
> like simply fetching from a private security-sensitive repo into your
> "main" public repo (which is an example you brought up, and something I
> know that I have personally done in the past for git.git).
Yup.
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 8:03 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1477757268.1524.20.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
>> Not sending to the list, where mails from Gmail/phone is known to get
>> rejected.
>
> [I guess I can go ahead and quote this to the list.]
>
>> No. I'm saying that the scenario you gave is bad and people should be
>> taught not to connect to untrustworthy sites.
>
> To clarify, are you saying:
>
> (1) don't connect to an untrusted server ever (e.g., we don't promise
> that the server can't execute arbitrary code on the client), or
>
> (2) don't connect to an untrusted server if the client repository has
> data that needs to be kept secret from the server?
You sneaked "arbitrary code execution" into the discussion but I do
not know where it came from. In any case, "don't pull from or push
to untrustworthy place" would be a common sense advice that would
make sense in any scenario ;-)
Just for future reference, when you have ideas/issues that might
have possible security ramifications, I'd prefer to see it first
discussed on a private list we created for that exact purpose, until
we can assess the impact (if any). Right now MaintNotes says this:
If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list <git-security@googlegroups.com>. This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:
- people triaging and fixing reported vulnerabilities
- people operating major git hosting sites with many users
- people packaging and distributing git to large numbers of people
where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.
We may want to tweak the description from "disclose it to us" to
"have a discussion on it with us" (the former makes it sound as if
the topic has to be a definite problem, the latter can include an
idle speculation that may not be realistic attack vector).
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 8:16 UTC (permalink / raw)
To: Jon Loeliger; +Cc: Matt McCutchen, git
In-Reply-To: <E1c0XaZ-0007Ab-QI@mylo.jdl.com>
Jon Loeliger <jdl@jdl.com> writes:
> Is there an existing protocol provision, or an extension to
> the protocol that would allow a distrustful client to say to
> the server, "Really, you have Y2? Prove it."
There is not, but I do not think it would be an effective solution.
The issue is not the lack of protocol support, but how to determine
that the other side needs such a proof for Y2 but not for other
commits. How does your side know what makes Y2 special and why does
yout side think they should not have Y2?
Once you know how to determine Y2 is special, that knowledge can be
used to abort the "push" before even starting. When you are pushing
back the 'master' and that 'master' reaches Y2, which must be kept
secret, you shouldn't be pushing that 'master' to them, whether they
claim to have Y2 or not.
I think the above is just a different way to say what Peff just said
(paraphrasing, do not push what is secret).
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-30 12:54 UTC (permalink / raw)
To: Aaron Pelly; +Cc: Junio C Hamano, git
In-Reply-To: <cc23eece-d693-9e40-78fe-3bafe6bcad3a@pelly.co>
On Fri, Oct 28, 2016 at 10:32:07PM +1300, Aaron Pelly wrote:
> On 28/10/16 15:54, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> However, as I said elsewhere, I'm not convinced this feature is all that
> >> helpful for in-repository .gitignore files, and I think it does
> >> introduce compatibility complications. People with older git will not
> >> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> >> matter.
> >
> > As I do not see the point of making in-tree .gitignore to a forest
> > of .gitignore.d/ at all, compatibility complications is not worth
> > even thinking about, I would have to say.
>
> Well; that saves some work. :)
>
> I do not suggesting making this mandatory. I think it adds value and it
> is a common and understood mechanism. But, if it is abhorrent, consider:
>
> There is precedent for including files in git-config. This could be
> extended to ignore files. The code is not similar, but the concept is. I
> could live with it.
Yes, but note that we don't have in-tree config files, either (to large
degree because of the security implications).
Perhaps Junio can clarify himself, but I took his statement to mean only
that in-tree .gitignore.d is not worth worrying about, but that
$GIT_DIR/info/exclude.d or similar would be OK (but perhaps I
interpreted that way because that's my own position :) ).
> Or how about a new githook that can intelligently create or return the
> details? This would be my least favourite option unless it was
> configured in an obvious place.
That seems more complicated than is really merited, and probably doesn't
perform great either (it's an extra forked process for almost every git
operation). And obviously would not work for an in-tree solution anyway,
as we do not want to run arbitrary code.
-Peff
^ 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