* Re: [PATCH v2] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Jeff King @ 2016-11-14 14:02 UTC (permalink / raw)
To: Tobias Klauser; +Cc: git, gitster
In-Reply-To: <20161114133905.6632-1-tklauser@distanz.ch>
On Mon, Nov 14, 2016 at 02:39:05PM +0100, Tobias Klauser wrote:
> The delta_limit parameter to diffcore_count_changes() has been unused
> since commit ba23bbc8e ("diffcore-delta: make change counter to byte
> oriented again.", 2006-03-04).
>
> Remove the parameter and adjust all callers.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
> v2: In the commit message, reference the correct commit where the parameter
> usage was removed. Spotted by Jeff King.
Looks good. Thanks for following up.
-Peff
^ permalink raw reply
* [PATCH v2] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Tobias Klauser @ 2016-11-14 13:39 UTC (permalink / raw)
To: git, gitster; +Cc: Jeff King
The delta_limit parameter to diffcore_count_changes() has been unused
since commit ba23bbc8e ("diffcore-delta: make change counter to byte
oriented again.", 2006-03-04).
Remove the parameter and adjust all callers.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
v2: In the commit message, reference the correct commit where the parameter
usage was removed. Spotted by Jeff King.
diff.c | 2 +-
diffcore-break.c | 1 -
diffcore-delta.c | 1 -
diffcore-rename.c | 4 ----
diffcore.h | 1 -
5 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 8981477c436d..ec8728362dae 100644
--- a/diff.c
+++ b/diff.c
@@ -2023,7 +2023,7 @@ static void show_dirstat(struct diff_options *options)
if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
diff_populate_filespec(p->one, 0);
diff_populate_filespec(p->two, 0);
- diffcore_count_changes(p->one, p->two, NULL, NULL, 0,
+ diffcore_count_changes(p->one, p->two, NULL, NULL,
&copied, &added);
diff_free_filespec_data(p->one);
diff_free_filespec_data(p->two);
diff --git a/diffcore-break.c b/diffcore-break.c
index 881a74f29e4f..c64359f489c8 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -73,7 +73,6 @@ static int should_break(struct diff_filespec *src,
if (diffcore_count_changes(src, dst,
&src->cnt_data, &dst->cnt_data,
- 0,
&src_copied, &literal_added))
return 0;
diff --git a/diffcore-delta.c b/diffcore-delta.c
index 2ebedb32d18a..ebe70fb06851 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -166,7 +166,6 @@ int diffcore_count_changes(struct diff_filespec *src,
struct diff_filespec *dst,
void **src_count_p,
void **dst_count_p,
- unsigned long delta_limit,
unsigned long *src_copied,
unsigned long *literal_added)
{
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 54a2396653df..f7444c86bde3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -145,7 +145,6 @@ static int estimate_similarity(struct diff_filespec *src,
* call into this function in that case.
*/
unsigned long max_size, delta_size, base_size, src_copied, literal_added;
- unsigned long delta_limit;
int score;
/* We deal only with regular files. Symlink renames are handled
@@ -191,11 +190,8 @@ static int estimate_similarity(struct diff_filespec *src,
if (!dst->cnt_data && diff_populate_filespec(dst, 0))
return 0;
- delta_limit = (unsigned long)
- (base_size * (MAX_SCORE-minimum_score) / MAX_SCORE);
if (diffcore_count_changes(src, dst,
&src->cnt_data, &dst->cnt_data,
- delta_limit,
&src_copied, &literal_added))
return 0;
diff --git a/diffcore.h b/diffcore.h
index c11b8465fc8e..623024135478 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -142,7 +142,6 @@ extern int diffcore_count_changes(struct diff_filespec *src,
struct diff_filespec *dst,
void **src_count_p,
void **dst_count_p,
- unsigned long delta_limit,
unsigned long *src_copied,
unsigned long *literal_added);
--
2.9.0
^ permalink raw reply related
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Lars Schneider @ 2016-11-14 9:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git, Jeff King
In-Reply-To: <xmqqtwbcyyfe.fsf@gitster.mtv.corp.google.com>
> On 13 Nov 2016, at 02:13, Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> Thanks. Dscho, does this fix both of these issues to you?
>>
>> Apparently it does because the CI jobs for `master` and for `next` pass.
>
> OK, thanks for a quick confirmation.
>
>> The one for `pu` still times out, of course.
>
> Earlier you said 'pu' did even not build, but do we know where this
> "still times out" comes from? As long as I don't merge anything
> prematurely, which I need to be careful about, it shouldn't impact
> the upcoming release, but we'd need to figure it out before moving
> things forward post release.
What is the goal for 'pu'?
(1) Builds clean on all platforms + passes all tests
(2) Builds clean on all platforms
(3) Builds clean on Linux
(4) Just makes new topics easily available to a broader audience
My understanding was always (4) but the discussion above sounds
more like (1) or (2)?
--
Git 'pu' does not compile on macOS right now:
builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized
whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
...
More info here:
https://api.travis-ci.org/jobs/175417712/log.txt?deansi=true
--
Cheers,
Lars
^ permalink raw reply
* Re: [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: stefan.naewe @ 2016-11-14 8:04 UTC (permalink / raw)
To: Johannes.Schindelin; +Cc: git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1611111933001.9967@virtualbox>
Am 11.11.2016 um 19:33 schrieb Johannes Schindelin:
> Hi Stefan,
>
> On Fri, 11 Nov 2016, Johannes Schindelin wrote:
>
>> Will keep you posted,
>
> I published the prerelease:
>
> https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.2
That version brings all my PATH entries back!
Tanks a lot!
Stefan
--
----------------------------------------------------------------
/dev/random says: Catholic (n.) A cat with a drinking problem.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-14 3:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jacob Keller, Dennis Kaarsemaker, Git mailing list
In-Reply-To: <alpine.DEB.2.20.1611121106110.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Fri, 11 Nov 2016, Jacob Keller wrote:
>
>> On Fri, Nov 11, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> >
>> >> No tests or documentation updates yet, and I'm not sure whether
>> >> --follow-symlinks in other modes than --no-index should be supported, ignored
>> >> (as it is now) or cause an error, but I'm leaning towards the third option.
>> >
>> > My knee-jerk reaction is:
>> >
>> > * The --no-index mode should default to your --follow-symlinks
>> > behaviour, without any option to turn it on or off.
>> >
>>
>> I agree. We shouldn't have to specify this for no-index.
>
> Ummm. *My* idea of --no-index was for it to behave as similar to the
> --index version as possible. For example when comparing directories
> containing symlinks. You seem intent on breaking this scenario.
Perhaps a viable compromise between the two is to only always
dereference at the top-level (i.e. the trees to be compared) under
"--no-index" mode and not changing anything else?
The original use case by Dennis is not even about doing a recursive
two-directories-in-a-filesystem comparison and encountering a
symbolic link (it was to compare two BLOBs, which happen to be
output from two commands).
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Junio C Hamano @ 2016-11-14 2:57 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git, Jeff King
In-Reply-To: <1479001205.3471.1.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> Documentation/fetch-push-security.txt | 9 +++++++++
A new (consolidated) piece like this that can be included in
multiple places is a good idea. I wonder if the original
description in "namespaces" thing can be moved here and then
"namespaces" page can be made to also borrow from this?
> Documentation/git-fetch.txt | 2 ++
> Documentation/git-pull.txt | 2 ++
> Documentation/git-push.txt | 2 ++
> 4 files changed, 15 insertions(+)
> create mode 100644 Documentation/fetch-push-security.txt
>
> diff --git a/Documentation/fetch-push-security.txt b/Documentation/fetch-push-security.txt
> new file mode 100644
> index 0000000..00944ed
> --- /dev/null
> +++ b/Documentation/fetch-push-security.txt
> @@ -0,0 +1,9 @@
> +SECURITY
> +--------
> +The fetch and push protocols are not designed to prevent a malicious
> +server from stealing data from your repository that you did not intend to
> +share. The possible attacks are similar to the ones described in the
> +"SECURITY" section of linkgit:gitnamespaces[7]. If you have private data
> +that you need to protect from the server, keep it in a separate
> +repository.
Yup, and then "do not push to untrustworthy place without checking
what you are pushing", too?
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
These three look sensible.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-14 1:55 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZTWFuzWBjGUX_nV4rVVDRpaabmj0-M6S7aJkX3w+dK2Jw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index 600b703..f4ad297 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -96,7 +96,9 @@ refname::
>>> slash-separated path components from the front of the refname
>>> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>> `<N>` must be a positive integer. If a displayed ref has fewer
>>> - components than `<N>`, the command aborts with an error.
>>> + components than `<N>`, the command aborts with an error. For the base
>>> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
>>> + `:base`. For the entire directory path append `:dir`.
Sorry that I missed this so far and I do not know how many recent
rerolls had them like this, but I am not sure about these :base and
:dir suffixes. From their names I think readers would expect that
they do rough equivalents to basename() and dirname() applied to the
refname, but the example contradicts with that intuition. The
result of applying basename() to 'refs/boo/bar/boz' would be 'boz'
and not 'foo' as the example says.
So assuming that :base and :dir are unrelated to basename() and
dirname():
- I think calling these :base and :dir may be misleading
- More importantly, what do these do? I do not think of a good
description that generalizes "base of refs/foo/bar/boz is foo" to
explain your :base.
- A :dir that corresponds to the :base that picks 'foo' from
'refs/foo/bar/boz' needs an example, too.
Or is the above example simply a typo? Is refs/foo/bar/boz:base
'boz', not 'foo'?
^ permalink raw reply
* Draft of Git Rev News edition 21
From: Christian Couder @ 2016-11-13 20:55 UTC (permalink / raw)
To: git
Cc: Thomas Ferris Nicolaisen, Jakub Narebski, Junio C Hamano,
Jeff King, Stefan Beller, Jacob Keller, Aaron Pelly,
Nguyen Thai Ngoc Duy, Johannes Schindelin, Alexei Lozovsky,
Martin Braun, Brendan Forster, Parker Moore, Shawn Pearce
Hi,
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-21.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/199
You can also reply to this email.
I tried to cc everyone who appears in this edition but maybe I missed
some people, sorry about that.
Thomas, Jakub Narębski (who is now part of the team, welcome Jakub!)
and myself plan to publish this edition on Wednesday November 16.
Thanks,
Christian.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Stefano Zacchiroli @ 2016-11-13 17:50 UTC (permalink / raw)
To: Christian Couder
Cc: Josh Triplett, Junio C Hamano, git, Shawn O. Pierce, Jeff King
In-Reply-To: <CAP8UFD13sDOFuyZMWuoJeLFt_LAsfAHFBHpRwcdAGmA22xNEKQ@mail.gmail.com>
Hi everyone,
On Sat, Nov 05, 2016 at 12:04:08AM +0100, Christian Couder wrote:
> On Fri, Nov 4, 2016 at 10:19 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Fri, Nov 04, 2016 at 09:47:41PM +0100, Christian Couder wrote:
> >>
> >> Couldn't a RefTree be used to store refs that point to the base
> >> commit, the tip commit and the blob that contains the cover letter,
> >> and maybe also a ref pointing to the RefTree of the previous version
> >> of the series?
> >
> > That's really interesting! The Software Heritage project is working on
> > something similar, because they want to store all the refs as part of
> > their data model as well. I'll point them to the reftree work.
>
> Yeah, I know them :-) and I think I have already told Stefano
> Zacchiroli about this, but I am not sure anymore.
> Anyway I am CC'ing him.
Thanks Christian (and Josh, on swh-devel) for pointing me to this.
As a bit of background, the conceptual data model we have adopted for
Software Heritage [1] is indeed that of a global Merkle DAG, very much
inspired by Git, but where we deduplicate past the boundaries of
individual VCS repositories. This way we can store only once the same
software artifacts (blobs, trees, commits, etc.) even when they can be
found at different software origins [2] (be it due to GitHub-like forks,
projects moving around, or simply rogue copies of the same code
scattered around the Internet).
[1]: https://www.softwareheritage.org/
[2]: "software origin" is Software Heritage terminology, which just
stands for places on the Internet where we can find source code
In our original design the topmost entries in our Merkle hierarchy used
to be commits and tags, similar to what Git does. But then we realized
that doing so inhibited us from sharing entire repository states across
multiple software origins or multiple visits of the same software
origin. So we decided to add "repository snapshot objects" as our
topmost entries, which are essentially git-like objects that map refs to
the ID of the corresponding (typed-)objects. Rationale and a more
lengthy description of this is available on our wiki [3]. It is not
implemented yet, but we're pretty sold on the design at this point.
[3]: https://wiki.softwareheritage.org/index.php?title=Repository_snapshot_objects
Now, even if my only awareness of what's going on in Git upstream is
limited to sporadic chats with Josh and Christian :-), it seems to me
that various ideas in the Git ecosystem go in the same direction of our
snapshot objects (git-series, RefTree). Which is understandable, given a
number of use cases might be served by this.
I don't think we have much to contribute to discussion or implementation
here, and for our needs it doesn't really matter which one gets
implemented. That's because we need an implementation of the concept
which is *external* to Git anyhow. But even if it happens to exist
within actual VCS, it's not a big deal for us, as we do have ways to
distinguish "synthetic" objects in the DAG that we create for our own
needs from "real" objects coming from actual software origins.
(Another example of this concept we already have is when we inject
distribution source packages or tarballs in our archive. In that case we
create synthetic commits that points to the tree extracted from the
tarball/package, preserving the ability to distinguish them from real
commits coming from VCS out there.)
If you think we can help in any other way, other than sharing our
experiences and design considerations that is, please let me know! (I'm
not subscribed to the Git upstream mailing list, but feel free to Cc:-me
in conversations related to this topic.)
Cheers.
--
Stefano Zacchiroli . zack@upsilon.cc . upsilon.cc/zack . . o . . . o . o
Computer Science Professor . CTO Software Heritage . . . . . o . . . o o
Former Debian Project Leader . OSI Board Director . . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
^ permalink raw reply
* [PATCH] attr: mark a file-local symbol as static
From: Ramsay Jones @ 2016-11-13 16:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Stefan,
If you need to re-roll your 'sb/attr' branch, could you please
squash this into the relevant patch.
Alternatively, since there is only a single call site for git_attr()
(on line #1005), you could perhaps remove git_attr() and inline that
call. (However, that does make that line exceed 80 columns).
Thanks!
ATB,
Ramsay Jones
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 667ba85..84c4b08 100644
--- a/attr.c
+++ b/attr.c
@@ -169,7 +169,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
return a;
}
-struct git_attr *git_attr(const char *name)
+static struct git_attr *git_attr(const char *name)
{
return git_attr_internal(name, strlen(name));
}
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-13 14:07 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqHKVUfNm+jCsbMNazHpVhB46h60k75JzS35CrkR-d-UQ@mail.gmail.com>
Hello,
On Wed, Nov 9, 2016 at 5:28 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>> Add tests and documentation for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 34 +++++++++++++++++++---------------
>> ref-filter.c | 29 +++++++++++++++++++++++++----
>> t/t6300-for-each-ref.sh | 24 ++++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 600b703..f4ad297 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -96,7 +96,9 @@ refname::
>> slash-separated path components from the front of the refname
>> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>> `<N>` must be a positive integer. If a displayed ref has fewer
>> - components than `<N>`, the command aborts with an error.
>> + components than `<N>`, the command aborts with an error. For the base
>> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
>> + `:base`. For the entire directory path append `:dir`.
>>
>> objecttype::
>> The type of the object (`blob`, `tree`, `commit`, `tag`).
>> @@ -114,22 +116,23 @@ objectname::
>>
>> upstream::
>> The name of a local ref which can be considered ``upstream''
>> - from the displayed ref. Respects `:short` and `:strip` in the
>> - same way as `refname` above. Additionally respects `:track`
>> - to show "[ahead N, behind M]" and `:trackshort` to show the
>> - terse version: ">" (ahead), "<" (behind), "<>" (ahead and
>> - behind), or "=" (in sync). `:track` also prints "[gone]"
>> - whenever unknown upstream ref is encountered. Append
>> - `:track,nobracket` to show tracking information without
>> - brackets (i.e "ahead N, behind M"). Has no effect if the ref
>> - does not have tracking information associated with it.
>> + from the displayed ref. Respects `:short`, `:strip`, `:base`
>> + and `:dir` in the same way as `refname` above. Additionally
>> + respects `:track` to show "[ahead N, behind M]" and
>> + `:trackshort` to show the terse version: ">" (ahead), "<"
>> + (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
>> + also prints "[gone]" whenever unknown upstream ref is
>> + encountered. Append `:track,nobracket` to show tracking
>> + information without brackets (i.e "ahead N, behind M"). Has
>> + no effect if the ref does not have tracking information
>> + associated with it.
>>
>> push::
>> The name of a local ref which represents the `@{push}`
>> location for the displayed ref. Respects `:short`, `:strip`,
>> - `:track`, and `:trackshort` options as `upstream`
>> - does. Produces an empty string if no `@{push}` ref is
>> - configured.
>> + `:track`, `:trackshort`, `:base` and `:dir` options as
>> + `upstream` does. Produces an empty string if no `@{push}` ref
>> + is configured.
>>
>
> At this point would it make more sense to document the extra values
> here in one block separately? For example, the upstream atom is
> getting pretty complex with all those options. Additionally, some of
> the options can be combined, like nobracket, but others cannot be
> comined so It may be worth documenting how and when those combinations
> work?
>
I do agree. The documentation seems cluttered. It'd be nice to clean
it up. Maybe
after this series? Or any suggestions on how to go about it?
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-11-13 2:44 UTC (permalink / raw)
To: Junio C Hamano, Jon Loeliger; +Cc: git
In-Reply-To: <xmqqpomiuu8q.fsf@gitster.mtv.corp.google.com>
On Sun, 2016-10-30 at 01:16 -0700, Junio C Hamano wrote:
> 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.
FWIW, I can imagine a protocol that would prove possession for all
objects, which would completely fix the problem. Each object would
have a "private" hash computed recursively over the object graph, just
like the ordinary object hash, but with a different seed. The object
database would be extended to cache the private hash of every object.
Then, during a fetch or push, when the two sides identify a matching
object, the side that would otherwise have had to send the object sends
the private hash. Support for storing multiple hashes per object might
also be useful in some way for the migration to a stronger hash
function than SHA-1.
The next best solution, which doesn't require a protocol change but
requires a little user intervention, is to have a configuration option
per remote for a set of refs whose reachable objects are known to be
safe to send to the server. This set presumably includes the remote's
own remote-tracking refs. During fetch and push, the client looks for
matches only among these "safe" objects, effectively emulating a
repository containing only the safe objects. A fetch may update the
remote-tracking refs to point to unsafe objects that were already in
the local repository, effectively making them safe, but only after the
server sends the content of these objects (and the client validates the
hashes!).
Unfortunately, I'm not signing up to implement either solution. :(
Matt
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-11-13 2:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwbuuuuy.fsf@gitster.mtv.corp.google.com>
On Sun, 2016-10-30 at 01:03 -0700, Junio C Hamano wrote:
> 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 ;-)
A blanket statement like that without explanation is not very helpful
to users who do find themselves needing to pull from or push to a
server they don't absolutely trust. The only "definitely safe" option
it leaves them is to run the entire thing in a sandbox. A statement of
the nature of the risk is much more helpful: users can determine that
they don't care about the risk, or if it does, what the easiest
workaround is.
The new risk we discovered in this thread is of leakage of private data
from the local repository. To avoid that risk, it's sufficient for
users to move private data to a separate repository, so that's the
advice I propose to give. Are you aware of issues with fetch/push with
potential impact beyond leakage of private data, which would make my
proposed text insufficient? I was giving "arbitrary code execution" as
an example of what the impact of such an issue could be.
> 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).
OK. I'll admit that I didn't even look for a policy on reporting of
security issues because I believed the issue had low enough impact that
a report to a dedicated security contact point would be unwelcome.
Maybe that was reckless. The new text sounds good, if you put it in a
place where people like me would see it. :/
Matt
^ permalink raw reply
* Re: Feature request - show result of URL rewrites
From: Junio C Hamano @ 2016-11-13 1:46 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Git User, git
In-Reply-To: <1478974642.3227.4.camel@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> Git lets you rewrite URLs using "url.<base>.insteadOf"
>> ...
>> Can you add a git-config option to show the result of this rewriting
>> whenever this occurs, as debugging more complicated rules can be
>> difficult/wasn't obvious without Wireshark.
>>
>> E.g. you could have the option 'url.printRewrites [True/False]' which
>> would print the line "Rewrote url 'git://github.com/git/git' to 'http
>> s://github.com/git/git'" to terminal/stdout when set to True.
>
> Such a configuration would be superfluous, the GIT_TRACE and
> GIT_CURL_VERBOSE environment variables already provide all the
> debugging information you need here.
While i tend to agree with you that this kind of thing should not be
a new "configuration" that you need to unset after you are done
debugging, and should instead be done with a single-shot request
mechanism like environment variables, I do not think your response
is a fair and useful one. Perhaps it is fair for the original
request that wants to have a knob to learn what the original was
rewritten to, but then the original request did not aim high enough
to get a useful feature, I would suspect.
GIT_TRACE or CURL_VERBOSE may show you the rewritten URL, and the
end user (presumably) knows what the original URL s/he handed Git to
go to, but I do not think these two are sufficient to find out what
triggered one URL to be rewritten to the other URL, which probably
is what the user wants to know the most.
Your response may become useful if remove.c::alias_url() is taught
to react to GIT_TRACE (or some other debugging mechanism we already
have), so that it reports what rule caused what URL to be rewritten
to what other URL.
This is just to illustrate where to patch and not meant to even
compile (e.g. "debug" is not even a variable defined anywhere), but
starting from something like this, perhaps?
remote.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/remote.c b/remote.c
index ad6c5424ed..09dca9468f 100644
--- a/remote.c
+++ b/remote.c
@@ -63,11 +63,9 @@ static int valid_remote(const struct remote *remote)
static const char *alias_url(const char *url, struct rewrites *r)
{
int i, j;
- struct counted_string *longest;
- int longest_i;
+ struct counted_string *longest = NULL;
+ int longest_i = -1, instead_of_j = -1;
- longest = NULL;
- longest_i = -1;
for (i = 0; i < r->rewrite_nr; i++) {
if (!r->rewrite[i])
continue;
@@ -77,12 +75,24 @@ static const char *alias_url(const char *url, struct rewrites *r)
longest->len < r->rewrite[i]->instead_of[j].len)) {
longest = &(r->rewrite[i]->instead_of[j]);
longest_i = i;
+ instead_of_j = j;
}
}
}
if (!longest)
return url;
+ if (debug) {
+ fprintf(stderr, "rewriting %s to %s%s\n",
+ url,
+ r->rewrite[longest_i]->base, url + longest->len);
+ fprintf(stderr, "due to '%.*s.insteadof' = '%.*s'\n",
+ r->rewrite[longest_i]->baselen,
+ r->rewrite[longest_i]->base,
+ r->rewrite[longest_i]->instead_of[j].len,
+ r->rewrite[longest_i]->instead_of[j].s);
+ }
+
return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
}
^ permalink raw reply related
* [PATCH] fetch/push: document that private data can be leaked
From: Matt McCutchen @ 2016-11-13 1:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <xmqqy416uvan.fsf@gitster.mtv.corp.google.com>
A malicious server may be able to use the fetch and push protocols to
steal data from a user's repository that the user did not intend to
share, via attacks similar to those described in the gitnamespaces(7)
man page. Mention this in the git-fetch(1), git-pull(1), and git-push(1)
man pages and recommend using separate repositories for private data and
interaction with untrusted servers.
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
And here's a proposed patch. Based on the maint branch, ac84098.
Documentation/fetch-push-security.txt | 9 +++++++++
Documentation/git-fetch.txt | 2 ++
Documentation/git-pull.txt | 2 ++
Documentation/git-push.txt | 2 ++
4 files changed, 15 insertions(+)
create mode 100644 Documentation/fetch-push-security.txt
diff --git a/Documentation/fetch-push-security.txt b/Documentation/fetch-push-security.txt
new file mode 100644
index 0000000..00944ed
--- /dev/null
+++ b/Documentation/fetch-push-security.txt
@@ -0,0 +1,9 @@
+SECURITY
+--------
+The fetch and push protocols are not designed to prevent a malicious
+server from stealing data from your repository that you did not intend to
+share. The possible attacks are similar to the ones described in the
+"SECURITY" section of linkgit:gitnamespaces[7]. If you have private data
+that you need to protect from the server, keep it in a separate
+repository.
+
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 9e42169..a461b4b 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -192,6 +192,8 @@ The first command fetches the `maint` branch from the repository at
objects will eventually be removed by git's built-in housekeeping (see
linkgit:git-gc[1]).
+include::fetch-push-security.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..0af2de9 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -237,6 +237,8 @@ If you tried a pull which resulted in complex conflicts and
would want to start over, you can recover with 'git reset'.
+include::fetch-push-security.txt[]
+
BUGS
----
Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 47b77e6..5ebef9e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -559,6 +559,8 @@ Commits A and B would no longer belong to a branch with a symbolic name,
and so would be unreachable. As such, these commits would be removed by
a `git gc` command on the origin repository.
+include::fetch-push-security.txt[]
+
GIT
---
Part of the linkgit:git[1] suite
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] upload-pack: Optionally allow fetching any sha1
From: Junio C Hamano @ 2016-11-13 1:23 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <1478885028-30357-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 43665ab..8d3db40 100755
It seems that I haven't heard of 43665ab.
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -306,6 +306,28 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
> test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
> '
Specifically, the above seems to be missing in my tree.
Perhaps you noticed the lack of test for allowReachableSHA1InWant
and added one, but forgot to send it out, while building this patch
on top?
> +uploadpack.allowAnySHA1InWant::
> + Allow `upload-pack` to accept a fetch request that asks for any
> + object at all.
> + Defaults to `false`.
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Junio C Hamano @ 2016-11-13 1:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1611121237230.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Thanks. Dscho, does this fix both of these issues to you?
>
> Apparently it does because the CI jobs for `master` and for `next` pass.
OK, thanks for a quick confirmation.
> The one for `pu` still times out, of course.
Earlier you said 'pu' did even not build, but do we know where this
"still times out" comes from? As long as I don't merge anything
prematurely, which I need to be careful about, it shouldn't impact
the upcoming release, but we'd need to figure it out before moving
things forward post release.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 11/17] ref-filter: introduce symref_atom_parser() and refname_atom_parser()
From: Karthik Nayak @ 2016-11-12 20:12 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xrHRMZemNfo3gjJryBRR5b4T8VT3t9_CBE5hJJfv+D3PQ@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:22 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> What's the reasoning for using separate functions here if they are
> exactly identical except for name? Do we intend to add separate
> options for this? I don't really have a problem with separate
> functions here since it helps avoid confusion but they are identical
> otherwise...
>
I see no need to have separate functions. Maybe in the future when we
plan on adding
specific options we could split them. I'll drop the
symref_atom_parser() function and use
refname_atom_parser() both places.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 08/17] ref-filter: add support for %(upstream:track,nobracket)
From: Karthik Nayak @ 2016-11-12 20:01 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xrm-MMRa9RuaHUGNWJmH8UcYPj3RZCsy1uvVRMuLeP0ZQ@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:15 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>> This is needed when we port branch.c to use ref-filter's printing APIs.
>>
>
> Makes sense. Seems a bit weird that we have the brackets normally
> rather than adding them as an option, but I think this is ok. We don't
> want to change all previous uses in this case.
>
> My only suggestion here would be to add code so that the options die()
> when we use nobracket along with trackshort or without track. This
> ensures that the nobracket option only applies to track mode?
>
Sure, seems reasonable. There's also the fact that even though nobracket
actually only supports the 'track' option. It might make sense to
leave it as it
is, i.e not die() when used with the other options cause, it kinda stays true to
it being used. e.g. %(upstream:nobracket,trackshort) does give 'trackshort'
without brackets, even though that's the default option and only way we print
'trackshort' information, it seems to make sense from the users point of view.
>> Add test and documentation for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 8 +++--
>> ref-filter.c | 67 +++++++++++++++++++++++++-------------
>> t/t6300-for-each-ref.sh | 2 ++
>> 3 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fd365eb..3953431 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -118,9 +118,11 @@ upstream::
>> `refname` above. Additionally respects `:track` to show
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> - or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it. `:track` also prints
>> - "[gone]" whenever unknown upstream ref is encountered.
>> + or "=" (in sync). `:track` also prints "[gone]" whenever
>> + unknown upstream ref is encountered. Append `:track,nobracket`
>> + to show tracking information without brackets (i.e "ahead N,
>> + behind M"). Has no effect if the ref does not have tracking
>> + information associated with it.
>>
>
> Ok so my comment on the previous patch is fixed here, the new wording
> makes it much more clear that [gone] is not the same thing as no
> information. So I don't think we should bother changing the previous
> patch in the series. This might want to document that nobracket works
> even without track, even if it doesn't actually do anything? Or make
> the code more strict in that we die() if the values are put together
> that make no sense?
>
Speaking from what I said above, maybe it makes sense to leave it like it is?
>> push::
>> The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6d51b80..4d7e414 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -46,8 +46,10 @@ static struct used_atom {
>> union {
>> char color[COLOR_MAXLEN];
>> struct align align;
>> - enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>> - remote_ref;
>> + struct {
>> + enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
>> + unsigned int nobracket: 1;
>> + } remote_ref;
>> struct {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> @@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>
>> static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> {
>> - if (!arg)
>> - atom->u.remote_ref = RR_NORMAL;
>> - else if (!strcmp(arg, "short"))
>> - atom->u.remote_ref = RR_SHORTEN;
>> - else if (!strcmp(arg, "track"))
>> - atom->u.remote_ref = RR_TRACK;
>> - else if (!strcmp(arg, "trackshort"))
>> - atom->u.remote_ref = RR_TRACKSHORT;
>> - else
>> - die(_("unrecognized format: %%(%s)"), atom->name);
>> + struct string_list params = STRING_LIST_INIT_DUP;
>> + int i;
>> +
>> + if (!arg) {
>> + atom->u.remote_ref.option = RR_NORMAL;
>> + return;
>> + }
>> +
>> + atom->u.remote_ref.nobracket = 0;
>> + string_list_split(¶ms, arg, ',', -1);
>> +
>> + for (i = 0; i < params.nr; i++) {
>> + const char *s = params.items[i].string;
>> +
>> + if (!strcmp(s, "short"))
>> + atom->u.remote_ref.option = RR_SHORTEN;
>> + else if (!strcmp(s, "track"))
>
> Should we add die()s here to disallow setting the remote_ref option
> multiple times? Otherwise, we should document that the last one wins?
> Not sure it's really a big deal here, but we could do it to ensure
> consistency for options which don't make sense together?
>
It makes sense to add a small note that the last option wins, I suppose
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 07/17] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
From: Karthik Nayak @ 2016-11-12 18:48 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xoKxGiNJO4g=Nm+KY=5=u8g2edoLOoe5K0ZAPrvoGj-Ug@mail.gmail.com>
On Wed, Nov 9, 2016 at 5:07 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> Make changes to t/t6300-for-each-ref.sh and
>> Documentation/git-for-each-ref.txt to reflect this change.
>>
>
> This will change behavior if people were expecting it to remain
> silent, but I think this could be considered a bug.
>
Didn't get you.
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Helped-by : Jacob Keller <jacob.keller@gmail.com>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 3 ++-
>> ref-filter.c | 4 +++-
>> t/t6300-for-each-ref.sh | 2 +-
>> 3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 92184c4..fd365eb 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -119,7 +119,8 @@ upstream::
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it.
>> + tracking information associated with it. `:track` also prints
>> + "[gone]" whenever unknown upstream ref is encountered.
>>
>
> I think this is poorly worded. If I understand, "has no effect if the
> ref does not have tracking information" so in that case we still print
> nothing, right? but otherwise we print [gone] only when the upstream
> ref no longer actually exists locally? I wonder if there is a better
> wording for this? I don't have one. Any suggestions to avoid confusing
> these two cases?
>
Dropping this, since its taken care of in the next patch.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: Feature request - show result of URL rewrites
From: Dennis Kaarsemaker @ 2016-11-12 18:17 UTC (permalink / raw)
To: Git User, git
In-Reply-To: <284668531.626.1478972994238.JavaMail.root@ichabod>
On Sat, 2016-11-12 at 18:49 +0100, Git User wrote:
> Hello
>
> Hopefully this is the right place to submit feature requests - let me
> know if there's somewhere else I should use!
>
> Git lets you rewrite URLs using "url.<base>.insteadOf"
>
> https://stackoverflow.com/a/11383587
> https://git-scm.com/docs/git-config
>
> Can you add a git-config option to show the result of this rewriting
> whenever this occurs, as debugging more complicated rules can be
> difficult/wasn't obvious without Wireshark.
>
> E.g. you could have the option 'url.printRewrites [True/False]' which
> would print the line "Rewrote url 'git://github.com/git/git' to 'http
> s://github.com/git/git'" to terminal/stdout when set to True.
Such a configuration would be superfluous, the GIT_TRACE and
GIT_CURL_VERBOSE environment variables already provide all the
debugging information you need here.
D.
^ permalink raw reply
* Feature request - show result of URL rewrites
From: Git User @ 2016-11-12 17:49 UTC (permalink / raw)
To: git
Hello
Hopefully this is the right place to submit feature requests - let me know if there's somewhere else I should use!
Git lets you rewrite URLs using "url.<base>.insteadOf"
https://stackoverflow.com/a/11383587
https://git-scm.com/docs/git-config
Can you add a git-config option to show the result of this rewriting whenever this occurs, as debugging more complicated rules can be difficult/wasn't obvious without Wireshark.
E.g. you could have the option 'url.printRewrites [True/False]' which would print the line "Rewrote url 'git://github.com/git/git' to 'https://github.com/git/git'" to terminal/stdout when set to True.
Thanks
^ permalink raw reply
* Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter
From: Lars Schneider @ 2016-11-12 13:57 UTC (permalink / raw)
To: Jeff King; +Cc: Dennis Kaarsemaker, Matthieu Moy, git, me
In-Reply-To: <20161111202710.7zpalypsnorqeclq@sigill.intra.peff.net>
> On 11 Nov 2016, at 21:27, Jeff King <peff@peff.net> wrote:
>
> On Fri, Nov 11, 2016 at 09:02:52PM +0100, Dennis Kaarsemaker wrote:
>
>>>> Are you sure about that? If I do:
>>>>
>>>> echo url=https://example.com/repo.git |
>>>> git credential fill
>>>>
>>>> I get prompted for a username and password.
>>>
>>>
>>> Hm.. either I don't understand you or I expressed myself unclear.
>>>
>>> Let's say a user runs:
>>>
>>> $ git clone https://myrepo.git
>>>
>>> If no credential helper is setup, then Git asks the user for credentials.
>>> Afterwards Git starts downloading stuff. At some point Git will run my
>>> smudge filter on some files and in my case the smudge filter needs the
>>> Git credentials. AFAIK, the smudge filter has no way to get the credentials
>>> from Git at this point - not even by invoking "git credential".
>>> Is this correct?
>>
>> I think that's correct, but the same argument goes both ways: unless I
>> use a credential helper, or explicitely give a filter application my
>> credentials, I don't want a helper to be able to get to those
>> credentials. I'd consider that a security bug.
>
> Yeah, agreed. They are logically two separate operations, so I think it
> is a feature that they do not implicitly share credentials.
>
> I think the only place where we implicitly share credentials is when
> serving an HTTP fetch or push requires multiple HTTP requests. And there
> it seems pretty sane to do so.
Agreed. Thanks for your thoughts on this!
- Lars
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Lars Schneider @ 2016-11-12 13:33 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <a90fe336-5b62-4278-64e6-e73cb65a02a8@kdbg.org>
> On 11 Nov 2016, at 22:22, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 11.11.2016 um 22:07 schrieb Junio C Hamano:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> OK, then let's have
>>>
>>> filter_git () {
>>> rm -f rot13-filter.log &&
>>> git "$@"
>>> ...
>>>
>>> and call that -rc1.
>>
>> That is, to queue this on top of ls/filter-process and merge it
>> down.
>>
>> -- >8 --
>> Subject: t0021: remove debugging cruft
>>
>> The redirection of the standard error stream to a temporary file is
>> a leftover cruft during debugging. Remove it.
>>
>> Besides, it is reported by folks on the Windows that the test is
>> flaky with this redirection; somebody gets confused and this
>> merely-redirected-to file gets marked as delete-pending by git.exe
>> and makes it finish with a non-zero exit status when "git checkout"
>> finishes. Windows folks may want to figure that one out, but for
>> the purpose of this test, it shouldn't become a show-stopper.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Nicely summarized! Thank you very much!
>
> I've double-checked again that the redirection is really not used by the tests. The name does occur in a .gitignore, but I guess that is not important for the background filter process. It would be good if Lars could confirm this.
Yesterday, I wasn't sure if silencing stderr was required by the test.
Junio's assessment and solution is spot on.
Thanks a lot,
Lars
>
>> ---
>> t/t0021-conversion.sh | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index cb72fa49de..5ce2535017 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -27,8 +27,7 @@ file_size () {
>>
>> filter_git () {
>> rm -f rot13-filter.log &&
>> - git "$@" 2>git-stderr.log &&
>> - rm -f git-stderr.log
>> + git "$@"
>> }
>>
>> # Compare two files and ensure that `clean` and `smudge` respectively are
>>
>
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Johannes Schindelin @ 2016-11-12 11:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Jeff King
In-Reply-To: <xmqqa8d53ky9.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 11 Nov 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > We have to use $PWD instead of $(pwd) because on Windows the latter
> > would add a C: style path to bash's Unix-style $PATH variable, which
> > becomes confused by the colon after the drive letter. ($PWD is a
> > Unix-style path.)
> >
> > In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> > assembles a Unix-style path list with the colon as separators. It
> > converts the value to a Windows-style path list with the semicolon as
> > path separator when it forwards the variable to git.exe. The same
> > confusion happens when bash's original value is contaminated with
> > Windows style paths.
> >
> > Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> > ---
> > Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
> >> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
> >>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>> ...
> >
> > When the MSYS program such as bash invokes a non-MSYS program, it
> > translates the Unix-style paths in arguments and environment variables
> > to Windows stlye. We only have to ensure that we inject only Unix-style
> > paths in these places so as not to confuse the conversion algorithm.
> > Most of the time, we do not have to worry.
> >
> > On the other hand, when we write a path to a file that git.exe consumes
> > or receive a path from git.exe, i.e., when the path travels through
> > stdout and stdin, no automatic translation happens (which is quite
> > understandable), and we have do the translation explicitly. An example
> > for such a case is when we write a .git/info/alternates file via the
> > shell.
> >
> >> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
> >> moment.
> >
> > Here it is. I had proposed the t0021 part earlier, but it fell through
> > the cracks during the temporary maintainer change.
>
> Thanks. Dscho, does this fix both of these issues to you?
Apparently it does because the CI jobs for `master` and for `next` pass.
The one for `pu` still times out, of course.
Ciao,
Dscho
^ 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