* Re: [PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Jeff King @ 2016-11-04 16:37 UTC (permalink / raw)
To: Tobias Klauser; +Cc: git, gitster
In-Reply-To: <20161104102436.23892-1-tklauser@distanz.ch>
On Fri, Nov 04, 2016 at 11:24:36AM +0100, Tobias Klauser wrote:
> The delta_limit parameter to diffcore_count_changes() has been unused
> since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
> Remove the parameter and adjust all callers.
Sounds like a good idea to get rid of an unused parameter, but I think
this went away in ba23bbc8e (diffcore-delta: make change counter to byte
oriented again., 2006-03-04).
The patch itself looks good.
-Peff
^ permalink raw reply
* Re: git -C has unexpected behaviour
From: Johannes Schindelin @ 2016-11-04 16:36 UTC (permalink / raw)
To: Felix Nairz; +Cc: git
In-Reply-To: <CADJspfL3zVCPv+mfRM_v4ukUggQkhGH7KB50a+HLPZXZqn1pXw@mail.gmail.com>
Hi Felix,
On Fri, 4 Nov 2016, Felix Nairz wrote:
> Now, to the unexpected part, which I think is a bug:
>
> If the TestData folder is there, but empty (I deleted all the files),
> then running
>
> git -C .\TestData reset --hard
>
> will NOT throw me an error but run
>
> git reset --hard
>
> on the git repository (not the submodule in the sub-directory!),
> without warning, or error.
I *think* that this is actually intended. Please note that -C is *not* a
synonym of --git-dir. It is more of a synonym of
cd .\TestData
git reset --hard
In other words, you probably expected -C to specify the top-level
directory of a worktree, and you expected Git to error out when it is not.
Instead, Git will treat -C as a directory *from where to start*; It *can*
be a subdirectory of the top-level directory of the worktree.
Please note that
git --git-dir=.\TestData\.git reset --hard
will not work, though, as it mistakes the current directory for the
top-level directory of the worktree. What you want to do is probably
git -C .\TestData --git-dir=.git reset --hard
This will tell Git to change the current working directory to the
top-level of your intended worktree, *and* state that the repository needs
to be in .git (which can be a file containing "gitdir: <real-git-dir>",
which is the default in submodules).
If the repository is *not* found, this command will exit with a failure.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] branch: remove unused parameter to create_branch()
From: Jeff King @ 2016-11-04 16:30 UTC (permalink / raw)
To: Tobias Klauser; +Cc: git, gitster
In-Reply-To: <20161104151949.13384-1-tklauser@distanz.ch>
On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:
> The name parameter to create_branch() has been unused since commit
> 55c4a673070f ("Prevent force-updating of the current branch"). Remove
> the parameter and adjust the callers accordingly. Also remove the
> parameter from the function's documentation comment.
This seemed eerily familiar, and it turns out I wrote this as a
preparatory step for a different topic a while back, but never finished
it.
So clearly a good change, though we might want to explain a bit more why
it's correct that the parameter is unused. Here's what I wrote:
This function used to have the caller pass in the current value of
HEAD, in order to make sure we didn't clobber HEAD. In 55c4a6730,
that logic moved to validate_new_branchname(), which just resolves
HEAD itself. The parameter to create_branch is now unused.
I also ended up reformatting the documentation comment, but that's
purely optional. My patch is below for reference. Feel free to grab any
bits of it that you agree with.
-- >8 --
Subject: [PATCH] create_branch: drop unused "head" parameter
This function used to have the caller pass in the current
value of HEAD, in order to make sure we didn't clobber HEAD.
In 55c4a6730, that logic moved to validate_new_branchname(),
which just resolves HEAD itself. The parameter to
create_branch is now unused.
Since we have to update and re-wrap the docstring describing
the parameters anyway, let's take this opportunity to break
it out into a list, which makes it easier to find the
parameters.
Signed-off-by: Jeff King <peff@peff.net>
---
branch.c | 3 +--
branch.h | 22 ++++++++++++++--------
builtin/branch.c | 4 ++--
builtin/checkout.c | 2 +-
4 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/branch.c b/branch.c
index a5a8dcbd0..0d459b3cf 100644
--- a/branch.c
+++ b/branch.c
@@ -228,8 +228,7 @@ N_("\n"
"will track its remote counterpart, you may want to use\n"
"\"git push -u\" to set the upstream config as you push.");
-void create_branch(const char *head,
- const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
diff --git a/branch.h b/branch.h
index b2f964933..3103eb9ad 100644
--- a/branch.h
+++ b/branch.h
@@ -4,15 +4,21 @@
/* Functions for acting on the information about branches. */
/*
- * Creates a new branch, where head is the branch currently checked
- * out, name is the new branch name, start_name is the name of the
- * existing branch that the new branch should start from, force
- * enables overwriting an existing (non-head) branch, reflog creates a
- * reflog for the branch, and track causes the new branch to be
- * configured to merge the remote branch that start_name is a tracking
- * branch for (if any).
+ * Creates a new branch, where:
+ *
+ * - name is the new branch name
+ *
+ * - start_name is the name of the existing branch that the new branch should
+ * start from
+ *
+ * - force enables overwriting an existing (non-head) branch
+ *
+ * - reflog creates a reflog for the branch
+ *
+ * - track causes the new branch to be configured to merge the remote branch
+ * that start_name is a tracking branch for (if any).
*/
-void create_branch(const char *head, const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
int force, int reflog,
int clobber_head, int quiet, enum branch_track track);
diff --git a/builtin/branch.c b/builtin/branch.c
index d5d93a8c0..60cc5c8e8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -807,7 +807,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
* create_branch takes care of setting up the tracking
* info and making sure new_upstream is correct
*/
- create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+ create_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
} else if (unset_upstream) {
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
@@ -853,7 +853,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
branch_existed = ref_exists(branch->refname);
- create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
+ create_branch(argv[0], (argc == 2) ? argv[1] : head,
force, reflog, 0, quiet, track);
/*
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b2a5b31d..512492aad 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -630,7 +630,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
}
}
else
- create_branch(old->name, opts->new_branch, new->name,
+ create_branch(opts->new_branch, new->name,
opts->new_branch_force ? 1 : 0,
opts->new_branch_log,
opts->new_branch_force ? 1 : 0,
--
2.11.0.rc0.263.g6f44bc3
^ permalink raw reply related
* Re: git -C has unexpected behaviour
From: Stefan Beller @ 2016-11-04 16:10 UTC (permalink / raw)
To: Felix Nairz; +Cc: git@vger.kernel.org
In-Reply-To: <CADJspfL3zVCPv+mfRM_v4ukUggQkhGH7KB50a+HLPZXZqn1pXw@mail.gmail.com>
On Fri, Nov 4, 2016 at 7:28 AM, Felix Nairz <felix.nairz@gmail.com> wrote:
> Hi guys,
>
> I ran into some really weird git behaviour today.
>
> My git --version is: git version 2.8.1.windows.1
>
> We have a git repository with a submodule called TestData. The data in
> there is modified and reset as part of our unit tests.
>
> The submodule is a sub-folder of the git repository called TestData.
> So the relative path from the git repository to the submodule is
> .\TestData
>
> If I delete the entire TestData folder and run
> git -C .\TestData reset --hard
>
> I will get the following error:
> git : fatal: Cannot change to '.\TestData': No such file or directory
> This is as expected.
>
>
> Now, to the unexpected part, which I think is a bug:
>
> If the TestData folder is there, but empty (I deleted all the files),
And "all the files" includes the ".git" file, which git uses to find out if
it is in a git repository. So it keeps walking up until it finds a .git
file/directory, which is the parent project.
Once a git directory is found, the main function of git initializes some
data structures, e.g. the "path prefix" inside the repository which would be
" .\TestData" in your case. then the actual command is found and run.
So what it is doing is:
"Suppose you are in the TestData directory of the parent project and then
run the command ..."
My gut reaction was to propose to check if any GITLINK (submodule)
is a prefix of said "path prefix" and then rather initialize and operate on
the submodule.
However I do not think this is a good idea:
* Git wants to be fast and checking if we are in any submodule
slows down the common case.
* Historically commands in un-initialized or deinitialized submodules
behave as if in the parent project. I think if we'd fix this issue, other
people would complain as their workflow is harmed.
>
> Because of this we have had losses of uncommitted changes a few times
> now (loosing a few days of work, and getting a bit paranoid),
* commit early, commit often such that the losses are less than a few days.
* do not remove the submodule directory as a whole thing
(make sure the .git file is there and not wiped)
* instead use "git -C TestData clean -dffx && git -C TestData reset --hard"
https://git-scm.com/docs/git-clean
> could never find the root cause for this until today, where I found
> out that it happens when the TestData directory is empty.
I am undecided if it is a bug or a feature. Fixing this as a bug
would be a performance penalty. Not fixing it may incur data losses.
I dunno.
>
> Thank for looking into this, and I am looking forward to hear your
> opinions about this.
>
> Best Regards, Felix Nairz
^ permalink raw reply
* [PATCH] branch: remove unused parameter to create_branch()
From: Tobias Klauser @ 2016-11-04 15:19 UTC (permalink / raw)
To: git; +Cc: gitster
The name parameter to create_branch() has been unused since commit
55c4a673070f ("Prevent force-updating of the current branch"). Remove
the parameter and adjust the callers accordingly. Also remove the
parameter from the function's documentation comment.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
branch.c | 3 +--
branch.h | 15 +++++++--------
builtin/branch.c | 4 ++--
builtin/checkout.c | 2 +-
4 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/branch.c b/branch.c
index a5a8dcbd0ed9..0d459b3cfe50 100644
--- a/branch.c
+++ b/branch.c
@@ -228,8 +228,7 @@ N_("\n"
"will track its remote counterpart, you may want to use\n"
"\"git push -u\" to set the upstream config as you push.");
-void create_branch(const char *head,
- const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
diff --git a/branch.h b/branch.h
index b2f964933270..8e63d1b6f964 100644
--- a/branch.h
+++ b/branch.h
@@ -4,15 +4,14 @@
/* Functions for acting on the information about branches. */
/*
- * Creates a new branch, where head is the branch currently checked
- * out, name is the new branch name, start_name is the name of the
- * existing branch that the new branch should start from, force
- * enables overwriting an existing (non-head) branch, reflog creates a
- * reflog for the branch, and track causes the new branch to be
- * configured to merge the remote branch that start_name is a tracking
- * branch for (if any).
+ * Creates a new branch, where name is the new branch name, start_name
+ * is the name of the existing branch that the new branch should start
+ * from, force enables overwriting an existing (non-head) branch, reflog
+ * creates a reflog for the branch, and track causes the new branch to
+ * be configured to merge the remote branch that start_name is a
+ * tracking branch for (if any).
*/
-void create_branch(const char *head, const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
int force, int reflog,
int clobber_head, int quiet, enum branch_track track);
diff --git a/builtin/branch.c b/builtin/branch.c
index d5d93a8c03fe..60cc5c8e8da0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -807,7 +807,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
* create_branch takes care of setting up the tracking
* info and making sure new_upstream is correct
*/
- create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+ create_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
} else if (unset_upstream) {
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
@@ -853,7 +853,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
branch_existed = ref_exists(branch->refname);
- create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
+ create_branch(argv[0], (argc == 2) ? argv[1] : head,
force, reflog, 0, quiet, track);
/*
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b2a5b31d423..512492aad909 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -630,7 +630,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
}
}
else
- create_branch(old->name, opts->new_branch, new->name,
+ create_branch(opts->new_branch, new->name,
opts->new_branch_force ? 1 : 0,
opts->new_branch_log,
opts->new_branch_force ? 1 : 0,
--
2.9.0
^ permalink raw reply related
* git -C has unexpected behaviour
From: Felix Nairz @ 2016-11-04 14:28 UTC (permalink / raw)
To: git
Hi guys,
I ran into some really weird git behaviour today.
My git --version is: git version 2.8.1.windows.1
We have a git repository with a submodule called TestData. The data in
there is modified and reset as part of our unit tests.
The submodule is a sub-folder of the git repository called TestData.
So the relative path from the git repository to the submodule is
.\TestData
If I delete the entire TestData folder and run
git -C .\TestData reset --hard
I will get the following error:
git : fatal: Cannot change to '.\TestData': No such file or directory
This is as expected.
Now, to the unexpected part, which I think is a bug:
If the TestData folder is there, but empty (I deleted all the files),
then running
git -C .\TestData reset --hard
will NOT throw me an error but run
git reset --hard
on the git repository (not the submodule in the sub-directory!),
without warning, or error. This is easy to reproduce by having an
empty .\TestData folder, and just changing any file in your git
repository before running
git -C .\TestData reset --hard
and seeing the local file changes gone.
Because of this we have had losses of uncommitted changes a few times
now (loosing a few days of work, and getting a bit paranoid), but
could never find the root cause for this until today, where I found
out that it happens when the TestData directory is empty.
Thank for looking into this, and I am looking forward to hear your
opinions about this.
Best Regards, Felix Nairz
^ permalink raw reply
* [PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Tobias Klauser @ 2016-11-04 10:24 UTC (permalink / raw)
To: git; +Cc: gitster
The delta_limit parameter to diffcore_count_changes() has been unused
since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
Remove the parameter and adjust all callers.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
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: Git issue
From: Jacob Keller @ 2016-11-04 4:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Philip Oakley, Halde, Faiz, Git mailing list
In-Reply-To: <xmqq8tt2opw0.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Tue, Nov 01, 2016 at 08:50:23PM -0000, Philip Oakley wrote:
>>
>>> > From "git help update-index":
>>> >
>>> > --[no-]assume-unchanged
>>> > When this flag is specified, the object names recorded for
>>> > the paths are not updated. Instead, this option sets/unsets
>>> > the "assume unchanged" bit for the paths. ...
>>
>> I think the "Instead" is "we are not doing the usual update-index thing
>> of reading the new data from disk; instead, we are _just_ setting the
>> bit". Perhaps that can be spelled out more clearly, but I think just
>> dropping "Instead" is a step backwards.
>
> I tend to agree; the biggest problem with this part of the
> description I think is that it starts by saying what it does not do,
> which may help people who expect the command to do its usual thing
> but otherwise is a secondary information. How about ripping it out
> and moving it after the primary description, i.e.
>
> Set or unset the "assume unchanged" bit for the paths,
> without changing the object names recorded for them, in the
> index.
>
>>> Given the number of misrepresentations (on the web) of what the bit does,
>>> and the ongoing misunderstandings of users it does feel like the man page
>>> article could be refreshed to be more assertive about the users promise, and
>>> Git's cautions.
>>
>> I dunno. I know this has long been a source of confusion, but I
>> specifically dug in the docs to see what we had, and I thought what I
>> quoted above was pretty clear. That has "only" been around for about 2
>> years, and is fighting against other mis-advice on the Internet, though.
>> So I'm not sure if it is badly worded, or if people simply do not see
>> it.
>
> I share the same reaction, and I find that the update in ccadb25f73
> ("doc: make clear --assume-unchanged's user contract", 2014-12-06)
> that introduced the "the user promises not to change and allows Git
> to assume" is sufficiently clear.
>
This is some what of a tangent, but....
One of the primary pieces of advice I've seen that encourages its use
was to set it for a submodule after a repository switched to using
submodules. The reasoning being that users were often running "git
commit -a" and accidentally including changes to their submodule
without realizing it because they were not used to keeping the
submodule up to date.
I saw advice for this and it mostly appeared to work, but I wonder if
there is a better solution. I failed to train people to simply stop
committing everything with "git add ." or "git commit -a" as this was
something they were too used to from using previous version control
systems.
Is there an alternative that would prevent "add everything" from
adding these files so that users wouldn't commit rewinds to
submodules?
Thanks,
Jake
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Jeff King @ 2016-11-04 0:11 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: git
In-Reply-To: <20161104000351.GP4611@redhat.com>
On Fri, Nov 04, 2016 at 01:03:51AM +0100, Andrea Arcangeli wrote:
> > can see why it is confused and does what it does (the whole email is
> > inside a double-quoted portion that is never closed, so it probably
> > thinks there is no hostname portion at all).
>
> I would see it too, if it actually sent the email to the garbled email
> address it generated, but it actually got the right email address
> (because it sent the email to the right address), but it decided to
> show a different email address in the mail header than the one it sent
> the email to. But I figure this is the wrong list for such questions :).
It has to do with the "envelope recipient" versus the RFC822 header. Git
provides the envelope recipient on the command-line, and that is what is
used in the SMTP conversation. In theory the MTA does not need to ever
even look at the contents of the message itself. Some do not, but some
have features like rewriting the in-message headers (e.g., to turn
"to: user" into "to: user@hostname"). You can probably disable that
header-rewriting feature in your config, but I don't know very much
about configuring postfix.
-Peff
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Andrea Arcangeli @ 2016-11-04 0:03 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161103141848.42pg6iow24prign5@sigill.intra.peff.net>
On Thu, Nov 03, 2016 at 10:18:48AM -0400, Jeff King wrote:
> bogus header. The munging that postfix does makes things worse, but I
I agree it makes things worse.
> can see why it is confused and does what it does (the whole email is
> inside a double-quoted portion that is never closed, so it probably
> thinks there is no hostname portion at all).
I would see it too, if it actually sent the email to the garbled email
address it generated, but it actually got the right email address
(because it sent the email to the right address), but it decided to
show a different email address in the mail header than the one it sent
the email to. But I figure this is the wrong list for such questions :).
> I think if any change were to be made, it would be to recognize this
> bogosity and either clean it up or abort. That ideally would happen via
If postfix continues to do what it does, a strict checking would be my
preference of course, assuming it won't break anything that currently
works...
If not it's fine too, as nothing particularly "unexpected" happened in
git.
At this point double checking that what postfix does is legit is
perhaps more worthwhile than adding strictness to git.
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Johannes Sixt @ 2016-11-03 21:09 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <20161103204438.zfe653c2bsv3zqkx@sigill.intra.peff.net>
Am 03.11.2016 um 21:44 schrieb Jeff King:
> On Thu, Nov 03, 2016 at 09:40:01PM +0100, Johannes Sixt wrote:
>> We have to use $PWD instead of $(pwd) because on Windows...
>
> Thanks. I have to admit I remain confused about which one to use at any
> given invocation
No worries. It *is* complex, and I don't expect anyone who is not deep
in the business to get this thing right except by mere chance.
> (I would have thought that switching to $PWD causes
> problems elsewhere in the script, but I guess file redirection is
> capable of handling either type).
Correct.
-- Hannes
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Stefan Beller @ 2016-11-03 20:53 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Brandon Williams, Duy Nguyen, git@vger.kernel.org
In-Reply-To: <18e8d0bb-ceb0-17dd-1318-f07585f8709b@kdbg.org>
On Thu, Nov 3, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 28.10.2016 um 20:54 schrieb Stefan Beller:
>>
>> previous discussion at
>> https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>>
>> This implements the discarded series':
>> jc/attr
>> jc/attr-more
>> sb/pathspec-label
>> sb/submodule-default-paths
>>
>> This includes
>> * The fixes for windows
>
>
> I've tested the incarnation currently in pu (1928fcc65dc6), which also has
> these fixes, and they work well after a cursory test (it at least passes the
> 3 test scripts that the series touches).
Thanks!
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Johannes Sixt @ 2016-11-03 20:47 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-1-sbeller@google.com>
Am 28.10.2016 um 20:54 schrieb Stefan Beller:
> previous discussion at https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>
> This implements the discarded series':
> jc/attr
> jc/attr-more
> sb/pathspec-label
> sb/submodule-default-paths
>
> This includes
> * The fixes for windows
I've tested the incarnation currently in pu (1928fcc65dc6), which also
has these fixes, and they work well after a cursory test (it at least
passes the 3 test scripts that the series touches).
> t/t0003-attributes.sh | 26 ++
> t/t6134-pathspec-with-labels.sh | 185 +++++++++
> t/t7400-submodule-basic.sh | 134 +++++++
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Jeff King @ 2016-11-03 20:44 UTC (permalink / raw)
To: Johannes Sixt
Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <d36d8b51-f2d7-a2f5-89ea-369f49556e10@kdbg.org>
On Thu, Nov 03, 2016 at 09:40:01PM +0100, Johannes Sixt wrote:
> > TEST_ROOT="$(pwd)"
> > +PATH=$TEST_ROOT:$PATH
>
> This causes problems on Windows. We need the following squashed in.
>
> ---- 8< ----
> [PATCH] squash! t0021: put $TEST_ROOT in $PATH
>
> 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.)
Thanks. I have to admit I remain confused about which one to use at any
given invocation (I would have thought that switching to $PWD causes
problems elsewhere in the script, but I guess file redirection is
capable of handling either type).
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Johannes Sixt @ 2016-11-03 20:40 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <20161102181824.mi6lmfnfckvrav7n@sigill.intra.peff.net>
Am 02.11.2016 um 19:18 schrieb Jeff King:
> We create a rot13.sh script in the trash directory, but need
> to call it by its full path when we have moved our cwd to
> another directory. Let's just put $TEST_ROOT in our $PATH so
> that the script is always found.
>
> This is a minor convenience for rot13.sh, but will be a
> major one when we switch rot13-filter.pl to a script in the
> same directory, as it means we will not have to deal with
> shell quoting inside the filter-process config.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t0021-conversion.sh | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dfde22549..c1ad20c61 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes'
> . ./test-lib.sh
>
> TEST_ROOT="$(pwd)"
> +PATH=$TEST_ROOT:$PATH
This causes problems on Windows. We need the following squashed in.
---- 8< ----
[PATCH] squash! t0021: put $TEST_ROOT in $PATH
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.)
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t0021-conversion.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4e4b9a6be3..cff2b7259d 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
. ./test-lib.sh
-TEST_ROOT="$(pwd)"
+TEST_ROOT="$PWD"
PATH=$TEST_ROOT:$PATH
write_script <<\EOF "$TEST_ROOT/rot13.sh"
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* [PATCH (optional)] t0021: use arithmetic expansion to trim whitespace from wc -c output
From: Johannes Sixt @ 2016-11-03 20:22 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <fb4d62de-fbb5-a2b4-8eba-b135125dafa9@kdbg.org>
Instead of a pipeline with sed and a useless use of cat, return the
unmodified text of wc -c from function file_size, but substitute the
result with arithmetic expansion to get rid of the leading whitespace
that some version of wc -c print.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
This is a pure optimization that reduces the number of forks, which
helps a bit on Windows.
There would be a solution with perl that does not require trimming
of whitespace, but perl startup times are unbearable on Windows.
wc -c is better.
t/t0021-conversion.sh | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index db71acacb3..42b529f615 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -22,7 +22,7 @@ generate_random_characters () {
}
file_size () {
- cat "$1" | wc -c | sed "s/^[ ]*//"
+ wc -c <"$1"
}
filter_git () {
@@ -369,10 +369,10 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -381,14 +381,14 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -399,8 +399,8 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
- IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -418,10 +418,10 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -451,7 +451,7 @@ test_expect_success PERL 'required process filter takes precedence' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log
@@ -474,7 +474,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -603,11 +603,11 @@ test_expect_success PERL 'process filter should restart after unexpected write f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]
+ IN: smudge smudge-write-fail.r $(($SF)) [OK] -- OUT: $(($SF)) [WRITE FAIL]
START
init handshake complete
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -649,9 +649,9 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge error.r $(($SE)) [OK] -- OUT: 0 [ERROR]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -688,7 +688,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
+ IN: smudge abort.r $(($SA)) [OK] -- OUT: 0 [ABORT]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* [PATCH] t0021: expect more variations in the output of uniq -c
From: Johannes Sixt @ 2016-11-03 20:12 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <3763DDDB-9D53-4877-8399-32DF1780CAB7@gmail.com>
Some versions of uniq -c write the count left-justified, other version
write it right-justified. Be prepared for both kinds.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Here it is as a proper patch.
Am 03.11.2016 um 01:41 schrieb Lars Schneider:
>> On 2 Nov 2016, at 18:03, Johannes Sixt <j6t@kdbg.org> wrote:
>> + sort "$FILE" | uniq -c |
>> + sed -e "s/^ *[0-9][0-9]* *IN: /x IN: /" >"$FILE.tmp" &&
>
> This looks good (thanks for cleaning up the redundant clean/smudge
> stuff - that was a refactoring artifact!). One minor nit: doesn't sed
> understand '[0-9]+' ?
I don't think so.
>> + mv "$FILE.tmp" "$FILE" || return
>
> Why '|| return' here?
If there is an error in the pipeline or in the mv command, the for loop
would not exit otherwise. The subsequent test_cmp most likely fails,
but the || return is more correct.
t/t0021-conversion.sh | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index a20b9f58e3..db71acacb3 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -40,10 +40,9 @@ test_cmp_count () {
actual=$2
for FILE in "$expect" "$actual"
do
- sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
- sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
- sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
- mv "$FILE.tmp" "$FILE"
+ sort "$FILE" | uniq -c |
+ sed -e "s/^ *[0-9][0-9]*[ ]*IN: /x IN: /" >"$FILE.tmp" &&
+ mv "$FILE.tmp" "$FILE" || return
done &&
test_cmp "$expect" "$actual"
}
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:56 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103185052.q46x4hlcwne7kpc5@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
>
> > On 11/03, Jeff King wrote:
> > >
> > > So this seems like a reasonable direction to me. It obviously needs
> > > documentation and tests. Arguably there should be a fallback "allow"
> > > value when a protocol is not mentioned in the config so that you could
> > > convert the default from "user" to "never" if you wanted your config to
> > > specify a pure whitelist.
> >
> > Yes I agree there should probably be a fallback value of 'never' maybe?
> > What you currently have preserves the behavior of what git does
> > now, if we did instead have a fallback of 'never' it would break current
> > users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
> > crazy protocols). We could ease into it though and start with default
> > to allow and then transition to a true whitelist sometime after this
> > change has been made?
>
> I don't see the value in moving the out-of-the-box install to any
> default except "user". Right now the experience of using a third-party
> helper is something like:
>
> cp git-remote-hg /somewhere/in/your/PATH
> git clone hg::whatever
>
> We restrict its use in submodules by default, which is unlikely to bite
> many people. But if we started falling back to "never" all the time,
> then that second command would break until you officially "approve"
> remote-hg in your config.
>
> I was thinking of just something to let people decide to have that level
> of paranoia themselves (especially if they want to just set up a
> whole-system white list via the config without bothering with
> environment variables). Like:
>
> git config --system protocol.allow never
> git config --system protocol.https.allow always
>
> That behaves exactly like:
>
> export GIT_ALLOW_PROTOCOL=https
>
> except it just works everywhere, without having to tweak the environment
> of every process.
>
Ah ok, so essentially letting the user specify a default behaviour
themselves.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:50 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103184538.GE182568@google.com>
On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
> On 11/03, Jeff King wrote:
> >
> > So this seems like a reasonable direction to me. It obviously needs
> > documentation and tests. Arguably there should be a fallback "allow"
> > value when a protocol is not mentioned in the config so that you could
> > convert the default from "user" to "never" if you wanted your config to
> > specify a pure whitelist.
>
> Yes I agree there should probably be a fallback value of 'never' maybe?
> What you currently have preserves the behavior of what git does
> now, if we did instead have a fallback of 'never' it would break current
> users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
> crazy protocols). We could ease into it though and start with default
> to allow and then transition to a true whitelist sometime after this
> change has been made?
I don't see the value in moving the out-of-the-box install to any
default except "user". Right now the experience of using a third-party
helper is something like:
cp git-remote-hg /somewhere/in/your/PATH
git clone hg::whatever
We restrict its use in submodules by default, which is unlikely to bite
many people. But if we started falling back to "never" all the time,
then that second command would break until you officially "approve"
remote-hg in your config.
I was thinking of just something to let people decide to have that level
of paranoia themselves (especially if they want to just set up a
whole-system white list via the config without bothering with
environment variables). Like:
git config --system protocol.allow never
git config --system protocol.https.allow always
That behaves exactly like:
export GIT_ALLOW_PROTOCOL=https
except it just works everywhere, without having to tweak the environment
of every process.
> > Do you have interest in picking this up and running with it?
>
> Yep! Thanks for the help in shaping this.
Great, thanks. I'm happy to review or discuss further as necessary.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:45 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103182428.j3r574evsk7ypfie@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
>
> So this seems like a reasonable direction to me. It obviously needs
> documentation and tests. Arguably there should be a fallback "allow"
> value when a protocol is not mentioned in the config so that you could
> convert the default from "user" to "never" if you wanted your config to
> specify a pure whitelist.
Yes I agree there should probably be a fallback value of 'never' maybe?
What you currently have preserves the behavior of what git does
now, if we did instead have a fallback of 'never' it would break current
users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
crazy protocols). We could ease into it though and start with default
to allow and then transition to a true whitelist sometime after this
change has been made?
>
> Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
> paranoid (though we should keep it indefinitely either way for backwards
> compatibility).
>
> Do you have interest in picking this up and running with it?
Yep! Thanks for the help in shaping this.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103181954.GD182568@google.com>
On Thu, Nov 03, 2016 at 11:19:54AM -0700, Brandon Williams wrote:
> On 11/03, Jeff King wrote:
> > +
> > + /* unknown; let them be used only directly by the user */
> > + return PROTOCOL_ALLOW_USER_ONLY;
> > +}
> > +
> > int is_transport_allowed(const char *type)
> > {
> > - const struct string_list *allowed = protocol_whitelist();
> > - return !allowed || string_list_has_string(allowed, type);
> > + const struct string_list *whitelist = protocol_whitelist();
> > + if (whitelist)
> > + return string_list_has_string(whitelist, type);
> > +
> > + switch (get_protocol_config(type)) {
> > + case PROTOCOL_ALLOW_ALWAYS:
> > + return 1;
> > + case PROTOCOL_ALLOW_NEVER:
> > + return 0;
> > + case PROTOCOL_ALLOW_USER_ONLY:
> > + return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> > + }
>
> I know this is just a rough patch you wiped up but one question:
> With the 'user' state, how exactly do you envision this env variable
> working? Do we want the user to have to explicitly set
> GIT_PROTOCOL_FROM_USER in their environment and then have these other
> commands (like git-submodule) explicitly clear the env var or would we
> rather these subcommands set a variable indicating they aren't coming
> from the user and the deafult state (no var set) is a user run command?
See the follow-up I just posted, but basically, the rules are:
- if you don't say anything, then the URL is from the user
- git-submodule would set it to "0" (i.e., tell us to be more careful)
- tools like "go get" would similarly set it to "0" if they are
passing untrusted URLs
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:24 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103175327.nn2yasvlsxsy22be@sigill.intra.peff.net>
On Thu, Nov 03, 2016 at 01:53:27PM -0400, Jeff King wrote:
> I'd design the new system from scratch, and have it kick in _only_ when
> GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
> have the safe behavior until they are ready to move to the new format.
>
> Something like the patch below (which is just for illustration, and not
> tested beyond compilation).
Here's that same patch with a few tweaks:
- it changes git-submodule to use the new, more flexible system (which
also gets a it a lot more test coverage)
- it tweaks two tests which use the "ext" helper to enable it (since
it's blacklisted by default; I have mixed feelings on that, but I
see why Blake wants it, as it would have protected things like "go
get" out of the box).
- it adds "file://" as a known-good protocol, even for submodules,
which matches the current code. I am not sure if this is reasonable
or not. A malicious repository probably can't do much by pointing
you to cloning your own repo as a submodule unless you then _also_
run some arbitrary code to expose it, at which point it's generally
game-over anyway.
And I'd expect automated services (like GitHub Pages) to already
have a cut-down whitelist via GIT_ALLOW_PROTOCOL (and I happen to
know that it goes).
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
paranoid (though we should keep it indefinitely either way for backwards
compatibility).
Do you have interest in picking this up and running with it?
-Peff
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d..0a477b4c9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
wt_prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# If the user has already specified a set of allowed protocols,
-# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
command=
branch=
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac36d..75c570adc 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git init original &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d58..c6c266187 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index d57e8dec2..cd603fbf5 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,70 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ /* XXX maybe also interpret git_config_bool() here? */
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
}
void transport_check_allowed(const char *type)
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103175327.nn2yasvlsxsy22be@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> +
> + /* unknown; let them be used only directly by the user */
> + return PROTOCOL_ALLOW_USER_ONLY;
> +}
> +
> int is_transport_allowed(const char *type)
> {
> - const struct string_list *allowed = protocol_whitelist();
> - return !allowed || string_list_has_string(allowed, type);
> + const struct string_list *whitelist = protocol_whitelist();
> + if (whitelist)
> + return string_list_has_string(whitelist, type);
> +
> + switch (get_protocol_config(type)) {
> + case PROTOCOL_ALLOW_ALWAYS:
> + return 1;
> + case PROTOCOL_ALLOW_NEVER:
> + return 0;
> + case PROTOCOL_ALLOW_USER_ONLY:
> + return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> + }
I know this is just a rough patch you wiped up but one question:
With the 'user' state, how exactly do you envision this env variable
working? Do we want the user to have to explicitly set
GIT_PROTOCOL_FROM_USER in their environment and then have these other
commands (like git-submodule) explicitly clear the env var or would we
rather these subcommands set a variable indicating they aren't coming
from the user and the deafult state (no var set) is a user run command?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:08 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Jonathan Nieder, git@vger.kernel.org,
Blake Burkhart
In-Reply-To: <20161103180224.7mhpziotpinmd3tr@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
>
> > > > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > > > because I view a whitelist or blacklist as well black and white and
> > > > having this user state adds in a gray area.
> > >
> > > Well the "user" state is to differentiate between the
> > > * "I consciously typed `git clone ...` (and e.g. I know what happens as
> > > I know the server admin and they are trustworthy.)
> > > * a repository contains a possible hostile .gitmodules file such
> > > that I am not aware of the network connection.
> >
> > This is still a gray area to me. I think that if we have a whitelist of
> > protocols then it should be a true whitelist and not have some means of
> > going around it. It just seems like something that could be exploited.
>
> How do you implement:
>
> git clone --recursive trusted:foo.git
>
> and use your ssh keys for the "trusted" server, but not for any servers
> mentioned in .gitmodules?
>
> You need some way of distinguishing between the two contexts (and
> setting policy for each).
>
> -Peff
Interesting. Ok I can see how this would be a useful now. Thanks for
the example :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:02 UTC (permalink / raw)
To: Brandon Williams
Cc: Stefan Beller, Jonathan Nieder, git@vger.kernel.org,
Blake Burkhart
In-Reply-To: <20161103175131.GB182568@google.com>
On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
> > > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > > because I view a whitelist or blacklist as well black and white and
> > > having this user state adds in a gray area.
> >
> > Well the "user" state is to differentiate between the
> > * "I consciously typed `git clone ...` (and e.g. I know what happens as
> > I know the server admin and they are trustworthy.)
> > * a repository contains a possible hostile .gitmodules file such
> > that I am not aware of the network connection.
>
> This is still a gray area to me. I think that if we have a whitelist of
> protocols then it should be a true whitelist and not have some means of
> going around it. It just seems like something that could be exploited.
How do you implement:
git clone --recursive trusted:foo.git
and use your ssh keys for the "trusted" server, but not for any servers
mentioned in .gitmodules?
You need some way of distinguishing between the two contexts (and
setting policy for each).
-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