[PATCH v2] pack-objects: warn on split packs disabling bitmaps
Junio C Hamano wrote:
> Eric Wong writes:
> > +++ b/Documentation/git-pack-objects.txt
> > @@ -110,7 +110,8 @@ base-name::
> > --max-pack-size=::
> > Maximum size of each output pack file. The size can be suffixed with
> > "k", "m", or "g". The minimum size allowed is limited to 1 MiB.
> > - If specified, multiple packfiles may be created.
> > + If specified, multiple packfiles may be created, which also
> > + prevents the creation of a bitmap index.
>
> This is a good update, judging with the yardstick I set in the
> previous paragraph in this review.
Thanks for the review; made some adjustments and have v2 below.
> > --- a/Documentation/git-repack.txt
> > +++ b/Documentation/git-repack.txt
> > @@ -106,7 +106,8 @@ other objects in that pack they already have locally.
> > --max-pack-size=::
> > Maximum size of each output pack file. The size can be suffixed with
> > "k", "m", or "g". The minimum size allowed is limited to 1 MiB.
> > - If specified, multiple packfiles may be created.
> > + If specified, multiple packfiles may be created, causing
> > + `--write-bitmap-index` and `repack.writeBitmaps` to be ignored.
>
> And this is not. Just say "bitmap index is not created".
Ah, I've now synced the same --max-pack-size doc from
git-pack-objects.txt you liked into v2 below.
I worded my original differently between pack-objects and repack
since I figured repack is more likely used by end users;
and perhaps warranted an explanation that didn't require
describing the actual problem...
But I suppose "repack" isn't commonly called anymore, either.
> > @@ -115,7 +116,9 @@ other objects in that pack they already have locally.
> > Write a reachability bitmap index as part of the repack. This
> > only makes sense when used with `-a` or `-A`, as the bitmaps
> > must be able to refer to all reachable objects. This option
> > - overrides the setting of `pack.writeBitmaps`.
> > + overrides the setting of `repack.writeBitmaps`. This option
> > + has no effect if a multiple packfiles are created due to
> > + reaching `pack.packSizeLimit` or `--max-pack-size`.
>
> Dropping "due to ..." makes it perfect.
Done, along with:
s/effect if a multiple/effect when multiple/
"if a" was definitely a typo, "if" is probably alright, but
I suspect "when" is even better.
-8<-
Subject: [PATCH] pack-objects: warn on split packs disabling bitmaps
It can be tempting for a server admin to want a stable set of
long-lived packs for dumb clients; but also want to enable
bitmaps to serve smart clients more quickly.
Unfortunately, such a configuration is impossible;
so at least warn users of this incompatibility since
commit 21134714787a02a37da15424d72c0119b2b8ed71
("pack-objects: turn off bitmaps when we split packs").
Tested the warning by inspecting the output of:
make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
Signed-off-by: Eric Wong
---
Documentation/config.txt | 12
Documentation/git-pack-objects.txt | 3 ++-
Documentation/git-repack.txt | 8 +---
builtin/pack-objects.c | 9 -
4 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3d8bc97..3ea3372 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2165,8 +2165,11 @@ pack.packSizeLimit::
The maximum size of a pack. This setting only affects
packing to a file when repacking, i.e. the git:// protocol
is unaffected. It can be overridden by the `--max-pack-size`
- option of linkgit:git-repack[1]. The minimum size allowed is
- limited to 1 MiB. The default is unlimited.
+ option of linkgit:git-repack[1]. Reaching this limit results
+ in the creation of multiple packfiles; which in turn prevents
+ bitmaps from being created.
+ The minimum size allowed is limited to 1 MiB.
+ The default is unlimited.
Common unit suffixes of 'k', 'm', or 'g' are
supported.
@@ -2566,8 +2569,9 @@ repack.writeBitmaps::
objects to disk (e.g., when `git repack -a` is run). This
index can speed up the "counting objects" phase of subsequent
packs created for clones and fetches, at the cost of some disk
- space and extra time spent on the initial repack. Defaults to
- false.
+ space and extra time spent on the initial repack. This has
+ no effect if multiple packfiles are created.
+ Defaults to false.
rerere.autoUpdate::
When set to true, `git-rerere` updates the index with the
diff --git a/Documentation/git-pack-objects.txt
b/Documentation/git-pack-objects.txt
index bbea529..19cdcd0 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -110,7 +110,8 @@ base-name::
--max-pack-size=::
Maximum size of each output pack file. The size can be suffixed with
"k", "m",
Re: BUG on OSX `git p4 submit` can fail when the workspace root doesn't exist locally.
On 27 April 2016 at 21:53, Stefan Beller wrote: > On Wed, Apr 27, 2016 at 11:06 AM, Jacob Smith wrote: >> I debugged the issue using the script here: >> https://github.com/git/git/blob/master/git-p4.py >> I'm not sure how close to the main repo that is. >> >> On Wed, Apr 27, 2016 at 11:28 AM, Stefan Beller wrote: >>> On Wed, Apr 27, 2016 at 9:15 AM, Jacob Smith wrote: On OS X, >>> >>> Do you use the Git as provided from OS X? In that case you better report >>> the bug >>> to Apple, as their version of Git is slightly different (not close on >>> upstream, by both >>> having additional patches as well as not following the upstream closely >>> IIUC). > > In that case, I have cc'd Luke and Lars, who work on p4 Which version of p4 are you using? Your suggested fix looks plausible though. Possibly it needs both chdirs() so that "p4 sync" will work if the caller is using a .p4config file in the p4 client directory to set the P4CLIENT. Thanks! Luke -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2016, #07; Mon, 25)
Hi Junio, On Wed, 27 Apr 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Hi Junio, > > > > On Mon, 25 Apr 2016, Junio C Hamano wrote: > > > >> * js/win32-mmap (2016-04-22) 3 commits > >> (merged to 'next' on 2016-04-22 at cd39c60) > >> + mmap(win32): avoid expensive fstat() call > >> + mmap(win32): avoid copy-on-write when it is unnecessary > >> + win32mmap: set errno appropriately > >> > >> mmap emulation on Windows has been optimized. > > > > Please note that it is not purely an optimization. It is also a bug fix in > > case of a pretty full disk: on Windows, mmap() is backed by the page file > > if it is in copy-on-write mode, and that can fail when the free space on > > the drive that has the page file drops below a certain threshold. > > Thanks. How does this look then? > > * js/win32-mmap (2016-04-22) 3 commits > (merged to 'next' on 2016-04-22 at cd39c60) > + mmap(win32): avoid expensive fstat() call > + mmap(win32): avoid copy-on-write when it is unnecessary > + win32mmap: set errno appropriately > > mmap emulation on Windows has been optimized and work better without > consuming paging store when not needed. > > Will merge to 'master'. Perfect! > Reducing paging file consumption still falls into optimization in my > dictionary, though ;-) Well, I considered this a bug fix when I worked on it: the command should not have failed, and with my fix, it didn't ;-) Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
Jeff King wrote: > On Wed, Apr 27, 2016 at 09:53:24PM +, Eric Wong wrote: > > > It can be tempting for a server admin to want a stable set of > > long-lived packs for dumb clients; but also want to enable > > bitmaps to serve smart clients more quickly. > But I did want to mention one thing, which is that long-lived split > packs are a tradeoff, even for dumb clients. The pack format cannot do > deltas between packs, so the sum of your split packs is larger than a > single pack would be. That's a good thing for somebody who cloned > earlier, and wants to only a few small packs on top. But it's much worse > for somebody who wants to do a fresh clone, and has to grab all of the > packs either way. Definitely a trade off, but a fresh clone with packs might only be (at worst) doubling or tripling bandwidth use on both sides? However, the CPU/memory cost of packing is at least an order of magnitude (more likely several orders of magnitude) more expensive on the server. The client most likely won't care about CPU/memory usage, though. > > Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of > > our ML archives, soonish. I can serve terabytes of dumb HTTP > > traffic all day long without breaking a sweat; but smart > > packing of big repos worries me; especially when feeding > > slow clients and having to leave processes running > > (or buffering pack output to disk). So perhaps I'll teach > > my HTTP server play dumb whenever CPU/memory usage is high. > > Yeah, CPU and memory load for serving large clones is a problem. Memory > especially scales with number of objects (because we keep the whole > packing list in memory for the entirety of the write). At GitHub, we > have some changes to try to serve things verbatim from the on-disk pack > without even creating an in-memory list of objects (it's just a bitmap > of which objects in the packfile to send), and that reduces CPU and > memory load quite a bit. Cleaning up and submitting those patches has > been on my todo list for a while, but I just haven't gotten to it. I'm > of course happy to share the messy state if you want to pick through it > yourself. Sure thing! I can't promise I'll have time, either, but being able to serve packs verbatim would be great; especially if you could multiplex it with epoll/kqueue for folks on slow pipes (and maybe use sendfile, but perhaps that's not worth the effort with TLS everywhere nowadays). I was also wondering if fresh clones could be memoized entirely. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary
Hi Hannes,
On Wed, 27 Apr 2016, Johannes Sixt wrote:
> Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:
> > On Tue, 26 Apr 2016, Johannes Sixt wrote:
> > > Should we insert a check for MAP_PRIVATE to catch
> > > unexpected use-cases (think of the index-helper daemon effort)?
> >
> > I agree, we should have such a check. The line above the `die("Invalid
> > usage ...")` that you can read as first line in above-quoted hunk reads:
> >
> > if (!(flags & MAP_PRIVATE))
> >
> > So I think we're fine :-)
>
> Oh, well... I thought I had checked the code before I wrote my question, but
> it seems I was blind... ;)
Don't worry! I really appreciate your review!
Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pull: make --rebase --verify-signatures illegal
Previously git-pull would silently ignore the --verify-signatures
option.
Signed-off-by: Alexander 'z33ky' Hirsch <1ze...@gmail.com>
---
We had some discussion back in December about a patch that added
--verify-signatures to git-pull, that was declined.
I proposed making git-pull --rebase --verify-signatures illegal then,
although it still remained open whether to just warn or make it an
error.
In this patch I opted into making it an error, which breaks the
previously accepted git pull --rebase --verify-signatures of course,
albeit I'd argue that it probably didn't do what the user expected
anyways.
I don't know if there are compatibility concerns with this though.
builtin/pull.c | 2 ++
t/t5520-pull.sh | 14 ++
2 files changed, 16 insertions(+)
diff --git a/builtin/pull.c b/builtin/pull.c
index d98f481..b6e1507 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_push(&args, "--no-autostash");
else if (opt_autostash == 1)
argv_array_push(&args, "--autostash");
+ if (opt_verify_signatures && strcmp(opt_verify_signatures,
"--verify-signatures") == 0)
+ die(_("The --verify-signatures option does not work for
--rebase."));
argv_array_push(&args, "--onto");
argv_array_push(&args, sha1_to_hex(merge_head));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 739c089..cb8f741 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should
override pull.rebase' '
test new = "$(git show HEAD:file2)"
'
+test_expect_success "pull --rebase --verify-signatures is illegal" '
+ git reset --hard before-rebase &&
+ test_must_fail git pull --rebase --verify-signatures . copy 2>err &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
+ test_i18ngrep "The --verify-signatures option does not work for
--rebase." err
+'
+
+test_expect_success "pull --rebase --no-verify-signatures" '
+ git reset --hard before-rebase &&
+ git pull --rebase --no-verify-signatures . copy &&
+ test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+ test new = "$(git show HEAD:file2)"
+'
+
# add a feature branch, keep-merge, that is merged into master, so the
# test can try preserving the merge commit (or not) with various
# --rebase flags/pull.rebase settings.
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix memory leak in git_connect with CONNECT_DIAG_URL
Signed-off-by: Mike Hommey --- connect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/connect.c b/connect.c index c53f3f1..dccf673 100644 --- a/connect.c +++ b/connect.c @@ -755,6 +755,7 @@ struct child_process *git_connect(int fd[2], const char *url, free(hostandport); free(path); free(conn); + strbuf_release(&cmd); return NULL; } -- 2.8.1.5.g18c8a48 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/2] http: support sending custom HTTP headers
We introduce a way to send custom HTTP headers with all requests.
This allows us, for example, to send an extra token from build agents
for temporary access to private repositories. (This is the use case that
triggered this patch.)
This feature can be used like this:
git -c http.extraheader='Secret: sssh!' fetch $URL $REF
Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.
For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.
Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.
Signed-off-by: Johannes Schindelin
---
Documentation/config.txt| 6 ++
http-push.c | 10 +-
http.c | 35 ---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8
t/t5551-http-fetch-smart.sh | 7 +++
7 files changed, 61 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1655,6 +1655,12 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.extraHeader::
+ Pass an additional HTTP header when communicating with a server. If
+ more than one such entry exists, all of them are added as extra
+ headers. To allow overriding the settings inherited from the system
+ config, an empty value will reset the extra headers to the empty list.
+
http.cookieFile::
File containing previously stored cookie lines which should be used
in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum
dav_header_flag options)
{
struct strbuf buf = STRBUF_INIT;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
if (options & DAV_HEADER_IF) {
strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
static void start_move(struct transfer_request *request)
{
struct active_request_slot *slot;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
slot = get_active_slot();
slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path,
long timeout)
char *ep;
char timeout_header[25];
struct remote_lock *lock = NULL;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
char *escaped;
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
struct slot_results results;
struct strbuf in_buffer = STRBUF_INIT;
struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
struct remote_ls_ctx ls;
@@ -1204,7 +1204,7 @@ static int locking_available(void)
struct slot_results results;
struct strbuf in_buffer = STRBUF_INIT;
struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
int lock_flags = 0;
char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 100644
--- a/http.c
+++ b/http.c
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
static struct curl_slist *pragma_header;
static struct curl_slist *no_pragma_header;
+static struct curl_slist *extra_http_headers;
static struct active_request_slot *active_queue_head;
@@ -323,6 +324,19 @@ static int http_options(const char *var, const char
*value, void *cb)
#endif
}
+ if (!strcmp("http.extraheader", var)) {
+ if (!value) {
+ return config_error_nonbool(var);
+ } else if (!*value) {
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+ } else {
+
[PATCH v5 2/2] submodule: pass on http.extraheader config settings
To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.
This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.
Example: git -c http.extraheader="Secret: Sauce" submodule update --init
Signed-off-by: Johannes Schindelin
---
builtin/submodule--helper.c | 4 +++-
t/t5551-http-fetch-smart.sh | 11 ++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..b338f93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const
char *prefix)
*/
static int submodule_config_ok(const char *var)
{
- if (starts_with(var, "credential."))
+ if (starts_with(var, "credential.") ||
+ (starts_with(var, "http.") &&
+ends_with(var, ".extraheader")))
return 1;
return 0;
}
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..1794168 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -286,7 +286,16 @@ test_expect_success 'custom http headers' '
test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
git -c http.extraheader="x-magic-one: abra" \
-c http.extraheader="x-magic-two: cadabra" \
- fetch "$HTTPD_URL/smart_headers/repo.git"
+ fetch "$HTTPD_URL/smart_headers/repo.git" &&
+ git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
+ git config -f .gitmodules submodule.sub.path sub &&
+ git config -f .gitmodules submodule.sub.url \
+ "$HTTPD_URL/smart_headers/repo.git" &&
+ git submodule init sub &&
+ test_must_fail git submodule update sub &&
+ git -c http.extraheader="x-magic-one: abra" \
+ -c http.extraheader="x-magic-two: cadabra" \
+ submodule update sub
'
stop_httpd
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/2] Add support for sending additional HTTP headers
My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.
I apologize for sending out v5 after v4 was already acknowledged: my
initial testing was on simple repositories and I forgot that my build
agents need to handle submodules, too.
So here goes v5, the only change being the addition of the second patch
that adds support for passing the extra headers to git-submodule through
the command-line.
Johannes Schindelin (2):
http: support sending custom HTTP headers
submodule: pass on http.extraheader config settings
Documentation/config.txt| 6 ++
builtin/submodule--helper.c | 4 +++-
http-push.c | 10 +-
http.c | 35 ---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8
t/t5551-http-fetch-smart.sh | 16
8 files changed, 73 insertions(+), 11 deletions(-)
Interdiff vs v4:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..b338f93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const
char *prefix)
*/
static int submodule_config_ok(const char *var)
{
- if (starts_with(var, "credential."))
+ if (starts_with(var, "credential.") ||
+ (starts_with(var, "http.") &&
+ ends_with(var, ".extraheader")))
return 1;
return 0;
}
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..1794168 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -286,7 +286,16 @@ test_expect_success 'custom http headers' '
test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
git -c http.extraheader="x-magic-one: abra" \
-c http.extraheader="x-magic-two: cadabra" \
- fetch "$HTTPD_URL/smart_headers/repo.git"
+ fetch "$HTTPD_URL/smart_headers/repo.git" &&
+ git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
+ git config -f .gitmodules submodule.sub.path sub &&
+ git config -f .gitmodules submodule.sub.url \
+ "$HTTPD_URL/smart_headers/repo.git" &&
+ git submodule init sub &&
+ test_must_fail git submodule update sub &&
+ git -c http.extraheader="x-magic-one: abra" \
+ -c http.extraheader="x-magic-two: cadabra" \
+ submodule update sub
'
stop_httpd
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
> On 25 Apr 2016, at 23:24, Jeff King wrote:
>
> On Mon, Apr 25, 2016 at 01:59:03PM -0700, Jacob Keller wrote:
>
However, I noticed that git config command line instructions such as
"-c filter.lfs.smudge=" are not passed to Git submodule operations. Thus
this does not work as expected:
git -c filter.lfs.smudge= -c filter.lfs.required=false clone
--recursive
>>>
>>> I have cc'd Jacob Keller, who authored origin/jk/submodule-c-credential,
>>> which does work in that area (deciding which config option to pass down
>>> into the submodule commands).
>>>
>>
>> This is a tricky question. The problem is that some configurations are
>> obviously not intended to go into the submodules, but determining how
>> is somewhat troublesome. There was some discussion on this previous
>> thread when we added support for credential options to pass through.
>
> Right. I think it may be reasonable to pass through filter.* in the
> whitelist. They are not activated without a matching .gitattributes
> entry in the repository (and people would generally configure them in
> their user-level ~/.gitconfig for that reason).
>
> It does mean that somebody would be stuck who really wanted to run the
> smudge filter in their local repo, but for some reason not in the
> subrepos. I am trying to think of a case in which that might be
> security-relevant if you didn't trust the sub-repos[1]. But I really
> don't see it. The filter is arbitrary code, but that's specified by the
> user; we're just feeding it possibly untrusted blobs.
This looks like a very promising solution and I can't think of a
security problem either (I checked the older thread [1] you
referenced, too)!
I got my Git-LFS use case working with the patch below.
For me it was necessary to export GIT_CONFIG_PARAMETERS
to make it available to the Git process if the process is
invoked as follows [2]:
(sanitize_submodule_env; cd "$sm_path" && git ")
Exporting the variable would not be necessary in this case:
(cd "$sm_path" && sanitize_submodule_env git ")
Unfortunately that does not work and I think the reason is that
clear_local_git_env (invoked by sanitize_submodule_env) clears
the $GIT_DIR, too.
If there is a reason against exporting GIT_CONFIG_PARAMETERS,
then this would work, too:
(sanitize_submodule_env; cd "$sm_path" &&
GIT_CONFIG_PARAMETERS=$GIT_CONFIG_PARAMETERS git )
Would the patch below be an acceptable solution?
Thanks,
Lars
[1] http://thread.gmane.org/gmane.comp.version-control.git/264840
[2]
https://github.com/git/git/blob/3ad15fd5e17bbb73fb1161ff4e9c3ed254d5b243/git-submodule.sh#L811
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..9231089 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -129,6 +129,8 @@ static int submodule_config_ok(const char *var)
{
if (starts_with(var, "credential."))
return 1;
+ if (starts_with(var, "filter."))
+ return 1;
return 0;
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 2a84d7e..b02f5b9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -199,7 +199,7 @@ sanitize_submodule_env()
{
sanitized_config=$(git submodule--helper sanitize-config)
clear_local_git_env
- GIT_CONFIG_PARAMETERS=$sanitized_config
+ export GIT_CONFIG_PARAMETERS=$sanitized_config
}
#
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
On Thu, Apr 28, 2016 at 01:06:45PM +0200, Lars Schneider wrote:
> I got my Git-LFS use case working with the patch below.
> For me it was necessary to export GIT_CONFIG_PARAMETERS
> to make it available to the Git process if the process is
> invoked as follows [2]:
>
> (sanitize_submodule_env; cd "$sm_path" && git ")
Hrm. I'm not sure why you need to export. Or perhaps, I am not sure why
it ever works in the first place in git-submodule.sh. In this code:
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2a84d7e..b02f5b9 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -199,7 +199,7 @@ sanitize_submodule_env()
> {
> sanitized_config=$(git submodule--helper sanitize-config)
> clear_local_git_env
> - GIT_CONFIG_PARAMETERS=$sanitized_config
> + export GIT_CONFIG_PARAMETERS=$sanitized_config
> }
If you already have $GIT_CONFIG_PARAMETERS exported when we enter the
function, then we should not need to re-export it when changing the
value in the final line (the export bit is retained by the shell). But
if you don't have it set already, then $sanitized_config must by
definition be empty.
So it should do the right thing without the export.
At the same time, clear_local_git_env() will call "unset" on
GIT_CONFIG_PARAMETERS. Which would clear the export bit, meaning the
final line doesn't ever have any impact on sub-programs, and the whole
thing is totally broken. But then, why does the test in t5550 pass?
Confused...
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:03:47PM +0200, Johannes Schindelin wrote:
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..b338f93 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const
> char *prefix)
> */
> static int submodule_config_ok(const char *var)
> {
> - if (starts_with(var, "credential."))
> + if (starts_with(var, "credential.") ||
> + (starts_with(var, "http.") &&
> + ends_with(var, ".extraheader")))
> return 1;
> return 0;
> }
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
Implement the GIT_TRACE_CURL environment variable to allow a
greater degree of detail of GIT_CURL_VERBOSE, in particular
the complete transport header and all the data payload exchanged.
It might be useful if a particular situation could require a more
thorough debugging analysis. Document the new GIT_TRACE_CURL
environment variable.
Helped-by: Torsten Bögershausen
Helped-by: Ramsay Jones
Helped-by: Junio C Hamano
Helped-by: Eric Sunshine
Helped-by: Jeff King
Signed-off-by: Elia Pinto
---
Documentation/git.txt | 8
http.c| 109 +-
http.h| 4 ++
3 files changed, 120 insertions(+), 1 deletion(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 8afe349..958db0f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1075,6 +1075,14 @@ of clones and fetches.
cloning of shallow repositories.
See 'GIT_TRACE' for available trace output options.
+'GIT_TRACE_CURL'::
+ Enables a curl full trace dump of all incoming and outgoing data,
+ including descriptive information, of the git transport protocol.
+ This is similar to doing curl --trace-ascii on the command line.
+ This option overrides setting the GIT_CURL_VERBOSE environment
+ variable.
+ See 'GIT_TRACE' for available trace output options.
+
'GIT_LITERAL_PATHSPECS'::
Setting this variable to `1` will cause Git to treat all
pathspecs literally, rather than as glob patterns. For example,
diff --git a/http.c b/http.c
index 985b995..5c2c729 100644
--- a/http.c
+++ b/http.c
@@ -11,6 +11,7 @@
#include "gettext.h"
#include "transport.h"
+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
#if LIBCURL_VERSION_NUM >= 0x070a08
long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
#else
@@ -478,6 +479,108 @@ static void set_curl_keepalive(CURL *c)
}
#endif
+static void curl_dump(const char *text, unsigned char *ptr, size_t size, char
nohex, char nopriv)
+{
+ size_t i;
+ struct strbuf out = STRBUF_INIT;
+ unsigned int width = 0x10;
+
+ /* without the hex output, we can fit more on screen */
+ if (nohex) width = 0x50;
+
+ strbuf_addf(&out, "%s, %10.10ld bytes (0x%8.8lx)\n",
+ text, (long)size, (long)size);
+ trace_strbuf(&trace_curl, &out);
+ strbuf_reset(&out);
+
+ for (i = 0; i < size; i += width) {
+ size_t w;
+ strbuf_addf(&out, "%s: ", text);
+ if (!nohex) {
+ for (w = 0; w < width; w++)
+ if (i + w < size)
+ strbuf_addf(&out, "%02x ", ptr[i + w]);
+ else
+ strbuf_addf(&out," ");
+ }
+ for (w = 0; (w < width) && (i + w < size); w++) {
+ if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
+ && ptr[i + w + 1] == '\n') {
+ i += (w + 2 - width);
+ break;
+ }
+ strbuf_addch(&out, (ptr[i + w] >= 0x20)
+ && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');
+ if (nohex && (i + w + 2 < size)
+ && ptr[i + w + 1] == '\r'
+ && ptr[i + w + 2] == '\n') {
+ i += (w + 3 - width);
+ break;
+ }
+ }
+ /* if we are called with nopriv we skip the Authorization field
if present
+* and print a blank line
+ */
+ if ( nopriv && strstr(out.buf, "Authorization:"))
+ strbuf_reset(&out);
+
+ strbuf_addch(&out, '\n');
+ trace_strbuf(&trace_curl, &out);
+ strbuf_release(&out);
+ }
+}
+
+int curl_trace_want(void)
+{
+ return trace_want(&trace_curl);
+}
+
+int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void
*userp)
+{
+ const char *text;
+ (void)handle; /* prevent compiler unused parameter warning if
checked */
+ (void)userp;/* prevent compiler unused parameter warning if
checked */
+ char nopriv = 0;/* default: there are no sensitive data
+* in the trace to be skipped
+ */
+
+ switch (type) {
+ case CURLINFO_TEXT:
+ trace_printf_key(&trace_curl, "== Info: %s", data);
+ default:/* we ignore unknown types by default */
+ return 0;
+
+ case CURLINFO_HEADER_OUT:
+ text = "=> Send header";
+ nopriv = 1;
+ break;
+ case CURLINFO_DATA_OUT:
+ text = "=> Send data
[PATCHv4 0/2] Implement the GIT_TRACE_CURL environment variable
This is the fourth version but in reality is the complete rewriting of the patches discussed here (here called V1) $gmane/290520 $gmane/290521 *Changes from V3 ($gmane/292040) - add missing static to curl_dump - reorder the patch order - tried to fix all (but i am not sure) the problems reported by Julio ($gmane/292055) - * squash the documentation with the http.c commit. * in the trace prefix each line to indicate it is about sending a header, and drop the initial hex count * auto-censor Authorization headers by default as suggested by Jeff King ($gmane/292074) *Changes from V2 ($gmane/291868) - fix garbage comment in http.c (i am very sorry for that) - add final '.' to the commit message for imap-send.c and to other commit messages - typofix double ; in http.c - merge the nice cleanup and code refactoring of Ramsay Jones (Thank you very much !!) - squash the previous commit 2/4 *Changes from V1 - introduced GIT_TRACE_CURL variable with its documentation - changed the name of the temporary variable "i" in "w" in the helper routine - used the c escape sequences instead of the hex equivalent - dropped the previous GIT_DEBUG_CURL env var - curl_dump and curl_trace factored out to a shared implementation in http.c Elia Pinto (2): http.c: implement the GIT_TRACE_CURL environment variable imap-send.c: introduce the GIT_TRACE_CURL enviroment variable Documentation/git.txt | 8 http.c| 109 +- http.h| 4 ++ imap-send.c | 6 +++ 4 files changed, 126 insertions(+), 1 deletion(-) -- 2.8.1.487.gf8c3767.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable
Permit the use of the GIT_TRACE_CURL environment variable calling
the curl_trace and curl_dump http.c helper routine.
Helped-by: Torsten Bögershausen
Helped-by: Ramsay Jones
Helped-by: Junio C Hamano
Helped-by: Eric Sunshine
Helped-by: Jeff King
Signed-off-by: Elia Pinto
---
imap-send.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/imap-send.c b/imap-send.c
index 938c691..61c6787 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+ if (curl_trace_want()) {
+ curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+ curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
+ curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
+ }
+
return curl;
}
--
2.8.1.487.gf8c3767.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
On Thu, Apr 28, 2016 at 07:25:11AM -0400, Jeff King wrote:
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 2a84d7e..b02f5b9 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -199,7 +199,7 @@ sanitize_submodule_env()
> > {
> > sanitized_config=$(git submodule--helper sanitize-config)
> > clear_local_git_env
> > - GIT_CONFIG_PARAMETERS=$sanitized_config
> > + export GIT_CONFIG_PARAMETERS=$sanitized_config
> > }
>
> If you already have $GIT_CONFIG_PARAMETERS exported when we enter the
> function, then we should not need to re-export it when changing the
> value in the final line (the export bit is retained by the shell). But
> if you don't have it set already, then $sanitized_config must by
> definition be empty.
>
> So it should do the right thing without the export.
>
> At the same time, clear_local_git_env() will call "unset" on
> GIT_CONFIG_PARAMETERS. Which would clear the export bit, meaning the
> final line doesn't ever have any impact on sub-programs, and the whole
> thing is totally broken. But then, why does the test in t5550 pass?
>
> Confused...
Ah. t5550 passes because it does not exercise this code path at all. We
try a recursive clone, which calls "git submodule update --init", which
does not seem to clear the config at all. So it works even without
14111fc49.
I tried to improve the test by adding git-fetch (note that I also fixed
a bug where we use $HTTP_URL instead of $HTTPD_URL, and added some
whitespace to make the result more readable):
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 69ef388..6ec3ba3 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -103,12 +103,23 @@ test_expect_success 'cmdline credential config passes
into submodules' '
git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
git commit -m "add submodule"
) &&
+
set_askpass wrong pass@host &&
test_must_fail git clone --recursive super super-clone &&
rm -rf super-clone &&
+
set_askpass wrong pass@host &&
- git -c "credential.$HTTP_URL.username=user@host" \
+ git -c "credential.$HTTPD_URL.username=user@host" \
clone --recursive super super-clone &&
+ expect_askpass pass user@host &&
+
+ set_askpass wrong pass@host &&
+ test_must_fail git -C super-clone fetch --recurse-submodules &&
+
+ set_askpass wrong pass@host &&
+ git -C super-clone \
+ -c "credential.$HTTPD_URL.username=user@host" \
+ fetch --recurse-submodules &&
expect_askpass pass user@host
'
but that doesn't pass, even with the export fix! That's because fetch
doesn't go through git-submodule at all; it calls "git fetch" itself,
and uses local_repo_env, which clears the config. It needs to learn to
use the same mechanism that sanitize_submodule_env() does.
So AFAICT 14111fc49 is totally broken. It doesn't actually work for
git-submodule (because of the missing export), nor for git-fetch
(because that skips the shell script), and the one case we are testing
already worked without it (but probably _should_ be sanitizing the
config, so is buggy, too).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
> On 28 Apr 2016, at 13:25, Jeff King wrote:
>
> On Thu, Apr 28, 2016 at 01:06:45PM +0200, Lars Schneider wrote:
>
>> I got my Git-LFS use case working with the patch below.
>> For me it was necessary to export GIT_CONFIG_PARAMETERS
>> to make it available to the Git process if the process is
>> invoked as follows [2]:
>>
>> (sanitize_submodule_env; cd "$sm_path" && git ")
>
> Hrm. I'm not sure why you need to export. Or perhaps, I am not sure why
> it ever works in the first place in git-submodule.sh. In this code:
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 2a84d7e..b02f5b9 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -199,7 +199,7 @@ sanitize_submodule_env()
>> {
>> sanitized_config=$(git submodule--helper sanitize-config)
>> clear_local_git_env
>> -GIT_CONFIG_PARAMETERS=$sanitized_config
>> +export GIT_CONFIG_PARAMETERS=$sanitized_config
>> }
>
> If you already have $GIT_CONFIG_PARAMETERS exported when we enter the
> function, then we should not need to re-export it when changing the
> value in the final line (the export bit is retained by the shell). But
> if you don't have it set already, then $sanitized_config must by
> definition be empty.
>
> So it should do the right thing without the export.
>
> At the same time, clear_local_git_env() will call "unset" on
> GIT_CONFIG_PARAMETERS. Which would clear the export bit, meaning the
> final line doesn't ever have any impact on sub-programs, and the whole
> thing is totally broken. But then, why does the test in t5550 pass?
>
> Confused...
I am no expert in the Submodule code but I think the cloning of
the submodules is not yet guarded with sanitize_submodule_env [3].
That means the submodule is cloned with the GIT_CONFIG_PARAMETERS
of the super project. That might explain why t5550 passes as the
credential config is only used in that area.
The submodule checkout is guarded with sanitize_submodule_env
and therefore my Git-LFS filter use case is affect.
Does this sound reasonable?
Thanks,
Lars
[3]
https://github.com/git/git/blob/3ad15fd5e17bbb73fb1161ff4e9c3ed254d5b243/git-submodule.sh#L704-L711
[4]
https://github.com/git/git/blob/3ad15fd5e17bbb73fb1161ff4e9c3ed254d5b243/git-submodule.sh#L811
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
On Thu, Apr 28, 2016 at 08:05:04AM -0400, Jeff King wrote: > So AFAICT 14111fc49 is totally broken. It doesn't actually work for > git-submodule (because of the missing export), nor for git-fetch > (because that skips the shell script), and the one case we are testing > already worked without it (but probably _should_ be sanitizing the > config, so is buggy, too). This last bit is not quite accurate. The test in t5550 doesn't pass without 14111fc49. But it _does_ pass if we make sanitize_submodule_env() in the shell script a noop. That's because it is going through clone_submodule() in the C code, which uses the C-only prepare_submodule_repo_env(). So that case _is_ correct right now. It's just that t5550 isn't testing the shell script part, which is broken. Probably running "git submodule update" in the resulting clone would cover that. And for the fetch case, we probably just need to be calling prepare_submodule_repo_env() there, too. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Hi Peff,
Cc:ing Jacob, the author of the CONFIG_DATA_ENVIRONMENT sanitizing code.
On Thu, 28 Apr 2016, Jeff King wrote:
> On Thu, Apr 28, 2016 at 12:03:47PM +0200, Johannes Schindelin wrote:
>
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 3bd6883..b338f93 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv,
> > const char *prefix)
> > */
> > static int submodule_config_ok(const char *var)
> > {
> > - if (starts_with(var, "credential."))
> > + if (starts_with(var, "credential.") ||
> > + (starts_with(var, "http.") &&
> > +ends_with(var, ".extraheader")))
> > return 1;
> > return 0;
> > }
>
> Should we consider just white-listing all of "http.*"?
>
> That would help other cases which have come up, like:
>
> http://thread.gmane.org/gmane.comp.version-control.git/264840
>
> which wants to turn off http.sslverify. That would mean it turns off for
> every submodule, too, but if you want to be choosy about your http
> variables, you should be using the "http.$URL.sslverify" form, to only
> affect specific servers (whether they are in submodules or not).
I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.
So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.
But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.
The commit message that introduced this particular filtering has this
rationale to let only credential.* through:
GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.
Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.
Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?
Jacob, Junio?
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] gitk: changes for the "Tags and heads" view
On Sun, Mar 27, 2016 at 11:06 AM, Michael Rappazzo wrote: > Changes since v2[1]: > - Instead of getting the remote info for each local branch individually, >grab it all at once and store the result > - Instead of a command line option to enable the new sorting option, >enable it with a preference which is stored in the config. > > v1 can be found here[2]. > > [1] http://thread.gmane.org/gmane.comp.version-control.git/289244 > [2] http://thread.gmane.org/gmane.comp.version-control.git/288544 > > Michael Rappazzo (2): > gitk: alter the ordering for the "Tags and heads" view > gitk: add an option to enable sorting the "Tags and heads" view by ref > type > > gitk | 79 > > 1 file changed, 66 insertions(+), 13 deletions(-) > > -- > 2.7.4 > I am still looking for comments on this patch. Also, is there a 'pu' repo for gitk? Currently, I am only tracking git://ozlabs.org/~paulus/gitk -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] pull: make --rebase --verify-signatures illegal
Previously git-pull would silently ignore the --verify-signatures
option.
Signed-off-by: Alexander 'z33ky' Hirsch <1ze...@gmail.com>
---
I made the error-message conform to the CodingGuidelines (removed
capitalization and full stop).
Also, in the previous mail I said that I proposed a patch for git-pull
last December, when I actually meant git-rebase.
builtin/pull.c | 2 ++
t/t5520-pull.sh | 14 ++
2 files changed, 16 insertions(+)
diff --git a/builtin/pull.c b/builtin/pull.c
index d98f481..b6e1507 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_push(&args, "--no-autostash");
else if (opt_autostash == 1)
argv_array_push(&args, "--autostash");
+ if (opt_verify_signatures && strcmp(opt_verify_signatures,
"--verify-signatures") == 0)
+ die(_("the --verify-signatures option does not work for
--rebase"));
argv_array_push(&args, "--onto");
argv_array_push(&args, sha1_to_hex(merge_head));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 739c089..cb8f741 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should
override pull.rebase' '
test new = "$(git show HEAD:file2)"
'
+test_expect_success "pull --rebase --verify-signatures is illegal" '
+ git reset --hard before-rebase &&
+ test_must_fail git pull --rebase --verify-signatures . copy 2>err &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
+ test_i18ngrep "The --verify-signatures option does not work for
--rebase." err
+'
+
+test_expect_success "pull --rebase --no-verify-signatures" '
+ git reset --hard before-rebase &&
+ git pull --rebase --no-verify-signatures . copy &&
+ test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+ test new = "$(git show HEAD:file2)"
+'
+
# add a feature branch, keep-merge, that is merged into master, so the
# test can try preserving the merge commit (or not) with various
# --rebase flags/pull.rebase settings.
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] fixes for sanitized submodule config
On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote: > So that case _is_ correct right now. It's just that t5550 isn't testing > the shell script part, which is broken. Probably running "git submodule > update" in the resulting clone would cover that. > > And for the fetch case, we probably just need to be calling > prepare_submodule_repo_env() there, too. So here's a series which fixes sanitizing in the "git-submodule" shell script, along with "git fetch". And cleans up a few things along the way. [1/5]: t5550: fix typo in $HTTPD_URL [2/5]: t5550: break submodule config test into multiple sub-tests [3/5]: submodule: export sanitize GIT_CONFIG_PARAMETERS [4/5]: submodule--helper: move config-sanitizing to submodule.c [5/5]: submodule: use prepare_submodule_repo_env consistently -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] t5550: fix typo in $HTTPD_URL
Commit 14111fc (git: submodule honor -c credential.* from command line, 2016-02-29) accidentally wrote $HTTP_URL. It happened to work because we ended up with "credential..helper", which we treat the same as "credential.helper", applying it to all URLs. Signed-off-by: Jeff King --- t/t5550-http-fetch-dumb.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 48e2ab6..81cc57f 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -103,7 +103,7 @@ test_expect_success 'cmdline credential config passes into submodules' ' test_must_fail git clone --recursive super super-clone && rm -rf super-clone && set_askpass wrong pass@host && - git -c "credential.$HTTP_URL.username=user@host" \ + git -c "credential.$HTTPD_URL.username=user@host" \ clone --recursive super super-clone && expect_askpass pass user@host ' -- 2.8.1.617.gbdccc2d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] t5550: break submodule config test into multiple sub-tests
Right now we test only the cloning case, but there are other interesting cases (e.g., fetching). Let's pull the setup bits into their own test, which will make things flow more logically once we start adding more tests which use the setup. Let's also introduce some whitespace to the clone-test to split the two parts: making sure it fails without our cmdline config, and that it succeeds with it. Signed-off-by: Jeff King --- t/t5550-http-fetch-dumb.sh | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 81cc57f..e8e91bb 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -91,17 +91,21 @@ test_expect_success 'configured username does not override URL' ' expect_askpass pass user@host ' -test_expect_success 'cmdline credential config passes into submodules' ' +test_expect_success 'set up repo with http submodules' ' git init super && set_askpass user@host pass@host && ( cd super && git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub && git commit -m "add submodule" - ) && + ) +' + +test_expect_success 'cmdline credential config passes to submodule via clone' ' set_askpass wrong pass@host && test_must_fail git clone --recursive super super-clone && rm -rf super-clone && + set_askpass wrong pass@host && git -c "credential.$HTTPD_URL.username=user@host" \ clone --recursive super super-clone && -- 2.8.1.617.gbdccc2d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
Commit 14111fc (git: submodule honor -c credential.* from command line, 2016-02-29) taught git-submodule.sh to save the sanitized value of $GIT_CONFIG_PARAMETERS when clearing the environment for a submodule. However, it failed to export the result, meaning that it had no effect for any sub-programs. We didn't catch this in our initial tests because we checked only the "clone" case, which does not go through the shell script at all. Provoking "git submodule update" to do a fetch demonstrates the bug. Noticed-by: Lars Schneider Signed-off-by: Jeff King --- git-submodule.sh | 1 + t/t5550-http-fetch-dumb.sh | 17 + 2 files changed, 18 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index 2a84d7e..3a40d4b 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -200,6 +200,7 @@ sanitize_submodule_env() sanitized_config=$(git submodule--helper sanitize-config) clear_local_git_env GIT_CONFIG_PARAMETERS=$sanitized_config + export GIT_CONFIG_PARAMETERS } # diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index e8e91bb..13ac788 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -112,6 +112,23 @@ test_expect_success 'cmdline credential config passes to submodule via clone' ' expect_askpass pass user@host ' +test_expect_success 'cmdline credential config passes submodule update' ' + # advance the submodule HEAD so that a fetch is required + git commit --allow-empty -m foo && + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD && + sha1=$(git rev-parse HEAD) && + git -C super-clone update-index --cacheinfo 16,$sha1,sub && + + set_askpass wrong pass@host && + test_must_fail git -C super-clone submodule update && + + set_askpass wrong pass@host && + git -C super-clone \ + -c "credential.$HTTPD_URL.username=user@host" \ + submodule update && + expect_askpass pass user@host +' + test_expect_success 'fetch changes via http' ' echo content >>file && git commit -a -m two && -- 2.8.1.617.gbdccc2d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
These functions should be used by any code which spawns a
submodule process, which may happen in submodule.c (e.g.,
for spawning fetch). Let's move them there and make them
public so that submodule--helper can continue to use them.
Sine they're now public, let's also provide a basic overview
of their intended use.
Signed-off-by: Jeff King
---
builtin/submodule--helper.c | 48
submodule.c | 49 +
submodule.h | 16 +++
3 files changed, 65 insertions(+), 48 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..de3ad5b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv, const
char *prefix)
return 0;
}
-/*
- * Rules to sanitize configuration variables that are Ok to be passed into
- * submodule operations from the parent project using "-c". Should only
- * include keys which are both (a) safe and (b) necessary for proper
- * operation.
- */
-static int submodule_config_ok(const char *var)
-{
- if (starts_with(var, "credential."))
- return 1;
- return 0;
-}
-
-static int sanitize_submodule_config(const char *var, const char *value, void
*data)
-{
- struct strbuf *out = data;
-
- if (submodule_config_ok(var)) {
- if (out->len)
- strbuf_addch(out, ' ');
-
- if (value)
- sq_quotef(out, "%s=%s", var, value);
- else
- sq_quote_buf(out, var);
- }
-
- return 0;
-}
-
-static void prepare_submodule_repo_env(struct argv_array *out)
-{
- const char * const *var;
-
- for (var = local_repo_env; *var; var++) {
- if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
- struct strbuf sanitized_config = STRBUF_INIT;
- git_config_from_parameters(sanitize_submodule_config,
- &sanitized_config);
- argv_array_pushf(out, "%s=%s", *var,
sanitized_config.buf);
- strbuf_release(&sanitized_config);
- } else {
- argv_array_push(out, *var);
- }
- }
-
-}
-
static int clone_submodule(const char *path, const char *gitdir, const char
*url,
const char *depth, const char *reference, int quiet)
{
diff --git a/submodule.c b/submodule.c
index 90825e1..02eaf0e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -13,6 +13,7 @@
#include "argv-array.h"
#include "blob.h"
#include "thread-utils.h"
+#include "quote.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
static int parallel_jobs = 1;
@@ -1129,3 +1130,51 @@ int parallel_submodules(void)
{
return parallel_jobs;
}
+
+/*
+ * Rules to sanitize configuration variables that are Ok to be passed into
+ * submodule operations from the parent project using "-c". Should only
+ * include keys which are both (a) safe and (b) necessary for proper
+ * operation.
+ */
+static int submodule_config_ok(const char *var)
+{
+ if (starts_with(var, "credential."))
+ return 1;
+ return 0;
+}
+
+int sanitize_submodule_config(const char *var, const char *value, void *data)
+{
+ struct strbuf *out = data;
+
+ if (submodule_config_ok(var)) {
+ if (out->len)
+ strbuf_addch(out, ' ');
+
+ if (value)
+ sq_quotef(out, "%s=%s", var, value);
+ else
+ sq_quote_buf(out, var);
+ }
+
+ return 0;
+}
+
+void prepare_submodule_repo_env(struct argv_array *out)
+{
+ const char * const *var;
+
+ for (var = local_repo_env; *var; var++) {
+ if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
+ struct strbuf sanitized_config = STRBUF_INIT;
+ git_config_from_parameters(sanitize_submodule_config,
+ &sanitized_config);
+ argv_array_pushf(out, "%s=%s", *var,
sanitized_config.buf);
+ strbuf_release(&sanitized_config);
+ } else {
+ argv_array_push(out, *var);
+ }
+ }
+
+}
diff --git a/submodule.h b/submodule.h
index 7ef3775..7577b3b 100644
--- a/submodule.h
+++ b/submodule.h
@@ -61,4 +61,20 @@ int push_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_nam
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
+/*
+ * This function is intended as a callback for use with
+ * git_config_from_parameters(). It ignores any config options which
+ * are not suitable for passing along to a submodule
[PATCH 5/5] submodule: use prepare_submodule_repo_env consistently
Before 14111fc (git: submodule honor -c credential.* from
command line, 2016-02-29), it was sufficient for code which
spawned a process in a submodule to just set the child
process's "env" field to "local_repo_env" to clear the
environment of any repo-specific variables.
That commit introduced a more complicated procedure, in
which we clear most variables but allow through sanitized
config. For C code, we used that procedure only for cloning,
but not for any of the programs spawned by submodule.c. As a
result, things like "git fetch --recurse-submodules" behave
differently than "git clone --recursive"; the former will
not pass through the sanitized config.
We can fix this by using prepare_submodule_repo_env()
everywhere in submodule.c.
Signed-off-by: Jeff King
---
submodule.c| 14 +++---
t/t5550-http-fetch-dumb.sh | 11 +++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/submodule.c b/submodule.c
index 02eaf0e..4e76b98 100644
--- a/submodule.c
+++ b/submodule.c
@@ -394,7 +394,7 @@ static int submodule_needs_pushing(const char *path, const
unsigned char sha1[20
argv[1] = sha1_to_hex(sha1);
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
@@ -481,7 +481,7 @@ static int push_submodule(const char *path)
const char *argv[] = {"push", NULL};
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -527,7 +527,7 @@ static int is_submodule_commit_present(const char *path,
unsigned char sha1[20])
argv[3] = sha1_to_hex(sha1);
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -710,7 +710,7 @@ static int get_next_submodule(struct child_process *cp,
if (is_directory(git_dir)) {
child_process_init(cp);
cp->dir = strbuf_detach(&submodule_path, NULL);
- cp->env = local_repo_env;
+ prepare_submodule_repo_env(&cp->env_array);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -825,7 +825,7 @@ unsigned is_submodule_modified(const char *path, int
ignore_untracked)
argv[2] = "-uno";
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
@@ -886,7 +886,7 @@ int submodule_uses_gitfile(const char *path)
/* Now test that all nested submodules use a gitfile too */
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.no_stderr = 1;
@@ -919,7 +919,7 @@ int ok_to_remove_submodule(const char *path)
return 0;
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 13ac788..3484b6f 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -112,6 +112,17 @@ test_expect_success 'cmdline credential config passes to
submodule via clone' '
expect_askpass pass user@host
'
+test_expect_success 'cmdline credential config passes submodule via fetch' '
+ set_askpass wrong pass@host &&
+ test_must_fail git -C super-clone fetch --recurse-submodules &&
+
+ set_askpass wrong pass@host &&
+ git -C super-clone \
+ -c "credential.$HTTPD_URL.username=user@host" \
+ fetch --recurse-submodules &&
+ expect_askpass pass user@host
+'
+
test_expect_success 'cmdline credential config passes submodule update' '
# advance the submodule HEAD so that a fetch is required
git commit --allow-empty -m foo &&
--
2.8.1.617.gbdccc2d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] How to pass Git config command line instructions to Submodule commands?
On Thu, Apr 28, 2016 at 02:05:20PM +0200, Lars Schneider wrote: > I am no expert in the Submodule code but I think the cloning of > the submodules is not yet guarded with sanitize_submodule_env [3]. > That means the submodule is cloned with the GIT_CONFIG_PARAMETERS > of the super project. That might explain why t5550 passes as the > credential config is only used in that area. > > The submodule checkout is guarded with sanitize_submodule_env > and therefore my Git-LFS filter use case is affect. > > Does this sound reasonable? Yes, that's exactly what's going on. git-submodule.sh's code _is_ broken, which is what you're seeing. I've just posted a series to fix this. I think it would be reasonable to add filter.* to the whitelist on top of that series. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
feature request for cc-cmd
Hi,
currently I am using --cc-cmd for larger patch series which are of
interest of different parties (e.g. kernel tree wide stuff) and I do not
want to spam all of them by patches which are not of their interest but
I still want them to receive the cover letter because that is useful to
get a context of a change. In order to do that each particular commit
has
Cc: email
{Acked,Reported,Reviewed,...}-by: email
in the patch header and I use the following
$ cat cc-cmd.sh
!/bin/bash
if [[ $1 == *gitsendemail.msg* || $1 == *cover-letter* ]]; then
grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq
else
grep '<.*@.*>' -h $1 | sed 's/^.*: //' | sort | uniq
fi
git send-email --cc-cmd ./cc-cmd.sh --to lkml patches...
which works reasonably well except I have to git format-patch the whole
series before posting. Quite often I do git send-email commit-range
though. This way is also little bit error prone because I have to make
sure no unrelated patch files are in the same directory. Would it be
possible that the cc script would get a list of all patch files for the
cover email? I have noticed that git send-email from the sha range will
use /tmp/$rand_dir/*.patch so this should be possible I guess. I would
update my script to
!/bin/bash
grep '<.*@.*>' -h $1 | sed 's/^.*: //' | sort | uniq
if [[ $1 == *gitsendemail.msg* || $1 == *cover-letter* ]]; then
shift
grep '<.*@.*>' -h $* | sed 's/^.*: //' | sort | uniq
fi
Would something like that make sense or there is an easier way
which I am just not aware of?
Thanks a lot!
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote: > > Should we consider just white-listing all of "http.*"? > > > > That would help other cases which have come up, like: > > > > http://thread.gmane.org/gmane.comp.version-control.git/264840 > > > > which wants to turn off http.sslverify. That would mean it turns off for > > every submodule, too, but if you want to be choosy about your http > > variables, you should be using the "http.$URL.sslverify" form, to only > > affect specific servers (whether they are in submodules or not). > > I considered that, and thought that it might be dangerous, what with me > not vetting carefully which http.* variables are safe to pass on to the > submodules' update and which are not. > > So I had a look now, and the most prominent potential problem is the > http.cookieFile setting, which could be reused all of a sudden if we > made my patch more general. > > But then, we are talking about the code that filters what gets passed via > the *command-line*. And to be quite honest, I am not sure that we should > actually filter out *any* of these settings. The intent of the whitelist (from my recollection of the discussion) is to filter out config that must be repo-specific. E.g., core.worktree or core.bare should definitely _not_ be passed to a submodule. I don't know if there are others. We started with a whitelist because it was the smallest and safest change away from the status quo. A blacklist would also work, with the risk that we might let through nonsense in some cases (but only if the user triggers us to do so). > The commit message that introduced this particular filtering has this > rationale to let only credential.* through: > > GIT_CONFIG_PARAMETERS is special, and we actually do want to > preserve these settings. However, we do not want to preserve all > configuration as many things should be left specific to the parent > project. > > Add a git submodule--helper function, sanitize-config, which shall be > used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs > except a small subset that are known to be safe and necessary. > > Dunno. I tried to err on the side of caution... But this sounds maybe a > bit *too* cautious? So if we all agree that the sanitizing is really about preventing repo-specific variables from leaking, and not any kind of security boundary, I think we should generally be pretty liberal in whitelisting things. I can certainly come up with a pathological case where using it as a security boundary may have some practical use, but in general I think it is mostly getting in the way of what users are trying to do. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote: > > Should we consider just white-listing all of "http.*"? > > > > That would help other cases which have come up, like: > > > > http://thread.gmane.org/gmane.comp.version-control.git/264840 > > > > which wants to turn off http.sslverify. That would mean it turns off for > > every submodule, too, but if you want to be choosy about your http > > variables, you should be using the "http.$URL.sslverify" form, to only > > affect specific servers (whether they are in submodules or not). > > I considered that, and thought that it might be dangerous, what with me > not vetting carefully which http.* variables are safe to pass on to the > submodules' update and which are not. BTW, just in case you or anybody else ends up playing around with this and finds your tests do not work as expected: the config pass-through feature is somewhat broken for anything except cloning. I just posted some fixes in: http://thread.gmane.org/gmane.comp.version-control.git/292466/focus=292875 -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fixes for sanitized submodule config
Hi Peff, On Thu, 28 Apr 2016, Jeff King wrote: > On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote: > > > So that case _is_ correct right now. It's just that t5550 isn't testing > > the shell script part, which is broken. Probably running "git submodule > > update" in the resulting clone would cover that. > > > > And for the fetch case, we probably just need to be calling > > prepare_submodule_repo_env() there, too. > > So here's a series which fixes sanitizing in the "git-submodule" shell > script, along with "git fetch". And cleans up a few things along the > way. Nice! I reviewed those changes and they all look sensible to me (did not apply them locally for lack of time, though). Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary
Some remote systems can employ restricted shells that aren't very smart
with quotes, so avoid quoting when it's not strictly necessary.
The list of "safe" characters comes from Mercurial's shell quoting
function used for its ssh client side. There likely are more that could
be added to the list.
Signed-off-by: Mike Hommey
---
connect.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/connect.c b/connect.c
index 96c8c1d..919bf9e 100644
--- a/connect.c
+++ b/connect.c
@@ -668,6 +668,17 @@ static void prepare_connect_command(struct strbuf *cmd,
const char *prog,
strbuf_addstr(cmd, prog);
strbuf_addch(cmd, ' ');
}
+ if (quote) {
+ const char *p;
+ for (p = path; *p; p++) {
+ if (!isalnum(*p) && *p != '@' && *p != '%' &&
+ *p != '_' && *p != '+' && *p != '=' && *p != ':' &&
+ *p != ',' && *p != '.' && *p != '/' && *p != '-')
+ break;
+ }
+ if (!*p)
+ quote = 0;
+ }
if (quote)
sq_quote_buf(cmd, path);
else
--
2.8.1.5.g18c8a48
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] git_connect: allow a file descriptor to be allocated for stderr
It can be useful to the caller of git_connect() to get access to stderr,
so add a flag that makes start_command allocate a file descriptor for
it.
Signed-off-by: Mike Hommey
---
connect.c | 2 ++
connect.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/connect.c b/connect.c
index 919bf9e..9feedd8 100644
--- a/connect.c
+++ b/connect.c
@@ -764,6 +764,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
conn->env = local_repo_env;
conn->use_shell = 1;
conn->in = conn->out = -1;
+ if (flags & CONNECT_WANT_STDERR)
+ conn->err = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
int putty = 0, tortoiseplink = 0;
diff --git a/connect.h b/connect.h
index 01f14cd..fb3331b 100644
--- a/connect.h
+++ b/connect.h
@@ -5,6 +5,7 @@
#define CONNECT_DIAG_URL (1u << 1)
#define CONNECT_IPV4 (1u << 2)
#define CONNECT_IPV6 (1u << 3)
+#define CONNECT_WANT_STDERR (1u << 4)
extern struct child_process *git_connect(int fd[2], const char *url, const
char *prog, int flags);
extern int finish_connect(struct child_process *conn);
extern int git_connection_is_socket(struct child_process *conn);
--
2.8.1.5.g18c8a48
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] git_connect: extend to take a pseudo format string for the program to run
Currently, the path extracted from the url is passed as last argument to
the program/command passed to git_connect(). In every case the function
is used in the git code base, it's enough, but in order to allow the
reuse of e.g. the GIT_SSH/GIT_SSH_COMMAND logic, additional flexibility
is welcome.
With this change, when the program/command passed to git_connect()
contains a "%s", that "%s" is replaced with the path from the url,
allowing the path to be at a different position than last on the
executed command line.
Signed-off-by: Mike Hommey
---
connect.c | 29 -
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/connect.c b/connect.c
index dccf673..96c8c1d 100644
--- a/connect.c
+++ b/connect.c
@@ -658,6 +658,25 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
static struct child_process no_fork = CHILD_PROCESS_INIT;
+static void prepare_connect_command(struct strbuf *cmd, const char *prog,
+const char *path, int quote)
+{
+ const char *found = strstr(prog, "%s");
+ if (found)
+ strbuf_add(cmd, prog, found - prog);
+ else {
+ strbuf_addstr(cmd, prog);
+ strbuf_addch(cmd, ' ');
+ }
+ if (quote)
+ sq_quote_buf(cmd, path);
+ else
+ strbuf_addstr(cmd, path);
+
+ if (found)
+ strbuf_addstr(cmd, found + 2);
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -717,18 +736,18 @@ struct child_process *git_connect(int fd[2], const char
*url,
* Note: Do not add any other headers here! Doing so
* will cause older git-daemon servers to crash.
*/
+ prepare_connect_command(&cmd, prog, path, 0);
packet_write(fd[1],
-"%s %s%chost=%s%c",
-prog, path, 0,
+"%s%chost=%s%c",
+cmd.buf, 0,
target_host, 0);
+ strbuf_release(&cmd);
free(target_host);
} else {
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
- strbuf_addstr(&cmd, prog);
- strbuf_addch(&cmd, ' ');
- sq_quote_buf(&cmd, path);
+ prepare_connect_command(&cmd, prog, path, 1);
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
--
2.8.1.5.g18c8a48
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] git_connect: add a flag to consider the path part of ssh urls relative
In Mercurial ssh urls, the path part of the url is relative to the home
directory of the account being logged to instead of being absolute.
Add a flag allowing git_connect() to handle this kind of usecase.
Signed-off-by: Mike Hommey
---
connect.c | 10 +++---
connect.h | 1 +
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/connect.c b/connect.c
index 9feedd8..0df6297 100644
--- a/connect.c
+++ b/connect.c
@@ -592,7 +592,7 @@ static char *get_port(char *host)
* The caller must free() the returned strings.
*/
static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
- char **ret_path)
+ char **ret_path, int relative_ssh)
{
char *url;
char *host, *path;
@@ -642,7 +642,10 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
end = path; /* Need to \0 terminate host here */
if (separator == ':')
path++; /* path starts after ':' */
- if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
+ if (protocol == PROTO_SSH && relative_ssh) {
+ if (path[0] == separator)
+ path++;
+ } else if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
if (path[1] == '~')
path++;
}
@@ -712,7 +715,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
*/
signal(SIGCHLD, SIG_DFL);
- protocol = parse_connect_url(url, &hostandport, &path);
+ protocol = parse_connect_url(url, &hostandport, &path,
+flags & CONNECT_RELATIVE_SSH);
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
diff --git a/connect.h b/connect.h
index fb3331b..1377028 100644
--- a/connect.h
+++ b/connect.h
@@ -6,6 +6,7 @@
#define CONNECT_IPV4 (1u << 2)
#define CONNECT_IPV6 (1u << 3)
#define CONNECT_WANT_STDERR (1u << 4)
+#define CONNECT_RELATIVE_SSH (1u << 5)
extern struct child_process *git_connect(int fd[2], const char *url, const
char *prog, int flags);
extern int finish_connect(struct child_process *conn);
extern int git_connection_is_socket(struct child_process *conn);
--
2.8.1.5.g18c8a48
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 0/4] git_connect: add some flexibility
As you may be aware, I'm working on a git remote helper to access mercurial repositories (https://github.com/glandium/git-cinnabar/). At the moment, a small part is written in C, relying on the git code base, but eventually, there would be more C. As I want to get rid of the dependency on Mercurial itself, I'm planning to implement the wire protocol parts in git-cinnabar. And while at it, I figured I'd evaluate if I can't just rely on some git internals, from C code. So I've turned to the git_connect function, that implements the niceties around GIT_SSH and GIT_SSH_COMMAND, and also handles ssh client specificities. (I'd rather not have to copy the code or reimplement it). It also turns out to be a convenient wrapper around start_command() for local urls. The git commands that git_connect is invoked for all take the repository path as their last argument. In mercurial's case, the command is: hg -R $path serve --stdio which doesn't match that pattern. So one hack I was thinking about was scan the url on my end, extract the path, replace it with "--stdio", and pass "hg -R $path serve" as command. Unfortunately, parse_connect_url is static, which means I'd either have to change connect.c to expose it, or copy it. Since I'd rather avoid copying code, I figured that since I was going to have to change connect.c, I might as well go with something less hacky, assuming it's accepted mainline. So following here are four patches that allow me to connect, via ssh, to hg.mozilla.org, and access mercurial repositories there using: git_connect(fd, url, "hg -R %s serve --stdio", CONNECT_RELATIVE_SSH | CONNECT_WANT_STDERR) And this works for local urls too, invoking `hg serve` locally. Note that what the second patch does could be done in sq_quote_buf instead, arguably. I'm certainly open to any better ideas as long as they can make it to mainline :). Mike Hommey (4): git_connect: extend to take a pseudo format string for the program to run git_connect: avoid quoting the path on the command line when it's not necessary git_connect: allow a file descriptor to be allocated for stderr git_connect: add a flag to consider the path part of ssh urls relative connect.c | 52 connect.h | 2 ++ 2 files changed, 46 insertions(+), 8 deletions(-) -- 2.8.1.5.g18c8a48 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On Thu, Apr 28, 2016 at 11:57:47AM +, Elia Pinto wrote:
> +static void curl_dump(const char *text, unsigned char *ptr, size_t size,
> char nohex, char nopriv)
We usually use "int" for our boolean flags. Space savings don't matter
outside of a struct (and if they did, you should be using a single flags
field), and this way the user does not have to guess whether the "char"
is significant.
It looks like we never pass anything but "1" for nohex. Can we drop this
parameter entirely? But see below...
> +{
> + size_t i;
> + struct strbuf out = STRBUF_INIT;
> + unsigned int width = 0x10;
> +
> + /* without the hex output, we can fit more on screen */
> + if (nohex) width = 0x50;
Maybe it is just me, but I think this is more readable using decimal
constants. I mind it less in checking ASCII values like 0x20, but here I
think just saying "80" is more customary.
> + for (i = 0; i < size; i += width) {
> + size_t w;
> + strbuf_addf(&out, "%s: ", text);
I really like this new format. Doing:
GIT_TRACE_CURL=1 git ... 2>&1 | grep '=> Send header: '
is very readable.
However, I did run into an interesting case. The output looks like:
10:24:04.540803 http.c:527 => Send header: Host: github.com
10:24:04.540809 http.c:527 => Send header: x
10:24:04.540811 http.c:527 => Send header: User-Agent:
git/2.8.1.341.g2caf4c9.dirty
What's that weird "x" line?
It turns out that the line before it is:
Authorization: Basic some-really-long-opaque-token-that-ends-in-x
Since we break at a newline _or_ at the width, that gets broken onto the
following line. The Authorization line hits the code below to suppress
the output.
So not only do I find the breaking of the line hard to read, but it
means we may leak data from the Authorization line that got broken into
the next chunk (here it was only one character, but with a sufficiently
long header, it could be real data).
So I think we probably want to _just_ break at newlines, however long
they are.
But that probably isn't a good idea for binary data. So I'd suggest that
sending/receiving headers break on newlines, and actual body data should
respect the width field (we may still have line-oriented data in the
body which would be easier to read without line-breaking, but if you are
debugging that you are better off with GIT_TRACE_PACKET anyway).
> + for (w = 0; (w < width) && (i + w < size); w++) {
> +if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
> + && ptr[i + w + 1] == '\n') {
> + i += (w + 2 - width);
> + break;
> +}
This loop puzzled me for a bit. When we end early due to a newline, we
subtract out the width here. I guess that's to accomodate the "i +=
width" that the outer for-loop is going to do.
If you follow my suggestion above to split the code paths for
line-oriented and fixed-width data, then this all gets much simpler.
> + /* if we are called with nopriv we skip the Authorization field
> if present
> + * and print a blank line
> + */
> + if ( nopriv && strstr(out.buf, "Authorization:"))
> + strbuf_reset(&out);
Style: multi-line comments should look like:
/*
* the comment
* goes here
*/
and there should be no whitespace after the opening "(".
Removing the field entirely may be a bit confusing when you're
debugging. Instead, perhaps we can just redact the interesting bits,
like:
diff --git a/http.c b/http.c
index 8ab0adc..30e8858 100644
--- a/http.c
+++ b/http.c
@@ -481,7 +481,11 @@ static void curl_dump(const char *text, unsigned char
*ptr, size_t size, char no
for (i = 0; i < size; i += width) {
size_t w;
+ size_t prefix_len;
+ const char *header;
+
strbuf_addf(&out, "%s: ", text);
+ prefix_len = out.len;
if (!nohex) {
for (w = 0; w < width; w++)
if (i + w < size)
@@ -507,8 +511,17 @@ static void curl_dump(const char *text, unsigned char
*ptr, size_t size, char no
/* if we are called with nopriv we skip the Authorization field
if present
* and print a blank line
*/
- if ( nopriv && strstr(out.buf, "Authorization:"))
- strbuf_reset(&out);
+ if (nopriv &&
+ skip_prefix(out.buf + prefix_len, "Authorization:",
&header)) {
+ /* The first token is the type, which is OK to log */
+ while (isspace(*header))
+ header++;
+ while (*header && !isspace(*header))
+ header++;
+ /* Everything else is opaque and possi
Re: [PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable
On Thu, Apr 28, 2016 at 11:57:48AM +, Elia Pinto wrote:
> diff --git a/imap-send.c b/imap-send.c
> index 938c691..61c6787 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
> if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
> curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
>
> + if (curl_trace_want()) {
> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> + curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> + curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
> + }
In the only other caller of curl_trace_want(), we repeat these exact
same lines (and really, what else would one do with that flag besides
enable curl_trace?).
Perhaps a better abstraction would be:
int setup_curl_trace(CURL *handle)
{
if (!trace_want(&trace_curl))
return 0;
curl_easy_setopt(handle, CURLOPT_VERBOSE, 1L);
curl_easy_setopt(handle, CURLOPT_DEBUGFUNCTION, curl_trace);
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
return 1;
}
You could even get rid of the return value, too. We do not use it here,
but just let GIT_TRACE_CURL naturally override GIT_CURL_VERBOSE by
setting the DEBUGFUNCTION. In the other caller, we do:
if (curl_trace_want()) {
... set up handle ...
} else {
... check and setup up GIT_CURL_VERBOSE ...
}
but we can do the else block regardless. It is a noop if tracing is set
up.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King wrote: > Right now we test only the cloning case, but there are other > interesting cases (e.g., fetching). Let's pull the setup > bits into their own test, which will make things flow more > logically once we start adding more tests which use the > setup. > > Let's also introduce some whitespace to the clone-test to > split the two parts: making sure it fails without our > cmdline config, and that it succeeds with it. > > Signed-off-by: Jeff King > --- > t/t5550-http-fetch-dumb.sh | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh > index 81cc57f..e8e91bb 100755 > --- a/t/t5550-http-fetch-dumb.sh > +++ b/t/t5550-http-fetch-dumb.sh > @@ -91,17 +91,21 @@ test_expect_success 'configured username does not > override URL' ' > expect_askpass pass user@host > ' > > -test_expect_success 'cmdline credential config passes into submodules' ' > +test_expect_success 'set up repo with http submodules' ' set up or setup? $ grep -r "set up" |wc -l 69 $ grep -r "setup" |wc -l 1162 Apart from that nit, this patch looks good to me. > git init super && > set_askpass user@host pass@host && > ( > cd super && > git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub && > git commit -m "add submodule" > - ) && > + ) > +' > + > +test_expect_success 'cmdline credential config passes to submodule via > clone' ' > set_askpass wrong pass@host && > test_must_fail git clone --recursive super super-clone && > rm -rf super-clone && > + > set_askpass wrong pass@host && > git -c "credential.$HTTPD_URL.username=user@host" \ > clone --recursive super super-clone && > -- > 2.8.1.617.gbdccc2d > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
On Thu, Apr 28, 2016 at 6:36 AM, Jeff King wrote: > Commit 14111fc (git: submodule honor -c credential.* from > command line, 2016-02-29) accidentally wrote $HTTP_URL. It > happened to work because we ended up with "credential..helper", > which we treat the same as "credential.helper", applying it > to all URLs. You say "credential.helper" twice here? I think that's confusing. The patch looks perfectly fine but I am having trouble parsing this description. 'We end up with X which we treat the same as X"? > > Signed-off-by: Jeff King > --- > t/t5550-http-fetch-dumb.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh > index 48e2ab6..81cc57f 100755 > --- a/t/t5550-http-fetch-dumb.sh > +++ b/t/t5550-http-fetch-dumb.sh > @@ -103,7 +103,7 @@ test_expect_success 'cmdline credential config passes > into submodules' ' > test_must_fail git clone --recursive super super-clone && > rm -rf super-clone && > set_askpass wrong pass@host && > - git -c "credential.$HTTP_URL.username=user@host" \ > + git -c "credential.$HTTPD_URL.username=user@host" \ > clone --recursive super super-clone && > expect_askpass pass user@host > ' > -- > 2.8.1.617.gbdccc2d > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] Ordering of remotes for fetch --all
Hi Jeff, thanks for your comments. On Mon, Apr 25, 2016 at 11:37 PM, Jeff King wrote: > On Mon, Apr 25, 2016 at 11:15:05PM +0200, Guido Martínez wrote: > >> I run a server with several git mirrors, that are updated every hour. On >> that same server, users clone those projects and work on them. We use >> the local mirrors to reduce network load: the users can fetch from the >> mirror first (to get most of the objects with zero network cost) and >> then fetch the real remote (to make sure they're completely up to date). >> >> I would like this to be configurable in each git working directory, >> so users can just configure the order they want and then just do "git >> remote update". >> >> I'm aware one can get this behavior by editing .git/config and >> ordering the remotes as one wishes, but I find that very hacky and not >> scripting-friendly. > > You can also define your own ordered groups, like: > > $ git config remotes.foo "one two three" > $ git fetch foo 2>&1 | grep ^Fetching > Fetching one > Fetching two > Fetching three > > That's not _exactly_ the same, because you can't give a partial ordering > of one high-priority remote and then say "all the rest, in whatever > order you want", because there's no way to say "all the rest". > > You _can_ say: > > git config remotes.foo "high-priority --all" > > but the final "--all" will fetch from high-priority again. An > alternative feature would be to teach remotes.* groups to cull > duplicates, if that's not acceptable. These are good, but my main drive was to be able to just "git remote update" without any more information. In your cases I need to call update "foo". Also as you mention you either need to edit foo when adding a repo, or duplicating the pull from the high-prio one. Another approach would be to add a "fetchdep" pointing to another remote, and then do a topological sort on fetch --all. This can also be used on "git pull", to first pull from the mirror without any extra command. Maybe it's not such a big deal, but I think it's a nice feature to have. It allows for a stupidly simple mirroring/prefetch scheme, without any proxy or anything fancy. Not sure if it suits the needs of anyone else, though... Would there be interest in me implementing the "fetchdep" alternative? Thanks! Guido > > I don't have a strong opinion against your approach, though. Just > exploring alternatives. > > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
On Thu, Apr 28, 2016 at 08:24:05AM -0700, Jacob Keller wrote: > On Thu, Apr 28, 2016 at 6:36 AM, Jeff King wrote: > > Commit 14111fc (git: submodule honor -c credential.* from > > command line, 2016-02-29) accidentally wrote $HTTP_URL. It > > happened to work because we ended up with "credential..helper", > > which we treat the same as "credential.helper", applying it > > to all URLs. > > You say "credential.helper" twice here? I think that's confusing. The > patch looks perfectly fine but I am having trouble parsing this > description. 'We end up with X which we treat the same as X"? Note the two dots in the first one. There is no variable $HTTP_URL, so: credential.$HTTP_URL.helper becomes: credential..helper -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King wrote: > Commit 14111fc (git: submodule honor -c credential.* from > command line, 2016-02-29) taught git-submodule.sh to save > the sanitized value of $GIT_CONFIG_PARAMETERS when clearing > the environment for a submodule. However, it failed to > export the result, meaning that it had no effect for any > sub-programs. > > We didn't catch this in our initial tests because we checked > only the "clone" case, which does not go through the shell > script at all. Provoking "git submodule update" to do a > fetch demonstrates the bug. > > Noticed-by: Lars Schneider > Signed-off-by: Jeff King > --- > git-submodule.sh | 1 + > t/t5550-http-fetch-dumb.sh | 17 + > 2 files changed, 18 insertions(+) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 2a84d7e..3a40d4b 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -200,6 +200,7 @@ sanitize_submodule_env() > sanitized_config=$(git submodule--helper sanitize-config) > clear_local_git_env > GIT_CONFIG_PARAMETERS=$sanitized_config > + export GIT_CONFIG_PARAMETERS > } > > # > diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh > index e8e91bb..13ac788 100755 > --- a/t/t5550-http-fetch-dumb.sh > +++ b/t/t5550-http-fetch-dumb.sh > @@ -112,6 +112,23 @@ test_expect_success 'cmdline credential config passes to > submodule via clone' ' > expect_askpass pass user@host > ' > > +test_expect_success 'cmdline credential config passes submodule update' ' > + # advance the submodule HEAD so that a fetch is required > + git commit --allow-empty -m foo && > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD && > + sha1=$(git rev-parse HEAD) && > + git -C super-clone update-index --cacheinfo 16,$sha1,sub && The use of update-index seems elegant to me, though different than any submodule test I wrote so far. :) > + > + set_askpass wrong pass@host && > + test_must_fail git -C super-clone submodule update && > + > + set_askpass wrong pass@host && > + git -C super-clone \ > + -c "credential.$HTTPD_URL.username=user@host" \ > + submodule update && > + expect_askpass pass user@host > +' > + > test_expect_success 'fetch changes via http' ' > echo content >>file && > git commit -a -m two && > -- > 2.8.1.617.gbdccc2d > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
On Thu, Apr 28, 2016 at 08:21:21AM -0700, Stefan Beller wrote: > > -test_expect_success 'cmdline credential config passes into submodules' ' > > +test_expect_success 'set up repo with http submodules' ' > > set up or setup? > > $ grep -r "set up" |wc -l > 69 > $ grep -r "setup" |wc -l > 1162 > > Apart from that nit, this patch looks good to me. "set up" is a verb phrase. "setup" is a noun or an adjective. I think we quite often use the latter as a verb. Whether that is grammatically depends on your philosophy, I think. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
On Thu, Apr 28, 2016 at 8:21 AM, Stefan Beller wrote: > On Thu, Apr 28, 2016 at 6:37 AM, Jeff King wrote: >> Right now we test only the cloning case, but there are other >> interesting cases (e.g., fetching). Let's pull the setup >> bits into their own test, which will make things flow more >> logically once we start adding more tests which use the >> setup. >> >> Let's also introduce some whitespace to the clone-test to >> split the two parts: making sure it fails without our >> cmdline config, and that it succeeds with it. >> >> Signed-off-by: Jeff King >> --- >> t/t5550-http-fetch-dumb.sh | 8 ++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh >> index 81cc57f..e8e91bb 100755 >> --- a/t/t5550-http-fetch-dumb.sh >> +++ b/t/t5550-http-fetch-dumb.sh >> @@ -91,17 +91,21 @@ test_expect_success 'configured username does not >> override URL' ' >> expect_askpass pass user@host >> ' >> >> -test_expect_success 'cmdline credential config passes into submodules' ' >> +test_expect_success 'set up repo with http submodules' ' > > set up or setup? > > $ grep -r "set up" |wc -l > 69 > $ grep -r "setup" |wc -l > 1162 > > Apart from that nit, this patch looks good to me. > Yes this looks quite a bit more readable. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
On Thu, Apr 28, 2016 at 8:25 AM, Jeff King wrote: > On Thu, Apr 28, 2016 at 08:24:05AM -0700, Jacob Keller wrote: > >> On Thu, Apr 28, 2016 at 6:36 AM, Jeff King wrote: >> > Commit 14111fc (git: submodule honor -c credential.* from >> > command line, 2016-02-29) accidentally wrote $HTTP_URL. It >> > happened to work because we ended up with "credential..helper", >> > which we treat the same as "credential.helper", applying it >> > to all URLs. >> >> You say "credential.helper" twice here? I think that's confusing. The >> patch looks perfectly fine but I am having trouble parsing this >> description. 'We end up with X which we treat the same as X"? > > Note the two dots in the first one. There is no variable $HTTP_URL, so: > > credential.$HTTP_URL.helper > > becomes: > > credential..helper > > -Peff Ah yes, very tiny text and tired morning eyes you are right. Makes more sense now. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King wrote: > Commit 14111fc (git: submodule honor -c credential.* from > command line, 2016-02-29) taught git-submodule.sh to save > the sanitized value of $GIT_CONFIG_PARAMETERS when clearing > the environment for a submodule. However, it failed to > export the result, meaning that it had no effect for any > sub-programs. > > We didn't catch this in our initial tests because we checked > only the "clone" case, which does not go through the shell > script at all. Provoking "git submodule update" to do a > fetch demonstrates the bug. > > Noticed-by: Lars Schneider > Signed-off-by: Jeff King > --- > git-submodule.sh | 1 + > t/t5550-http-fetch-dumb.sh | 17 + > 2 files changed, 18 insertions(+) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 2a84d7e..3a40d4b 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -200,6 +200,7 @@ sanitize_submodule_env() > sanitized_config=$(git submodule--helper sanitize-config) > clear_local_git_env > GIT_CONFIG_PARAMETERS=$sanitized_config > + export GIT_CONFIG_PARAMETERS why not export GIT_CONFIG_PARAMETERS=$santized_config ? Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote:
> > +test_expect_success 'cmdline credential config passes submodule update' '
> > + # advance the submodule HEAD so that a fetch is required
> > + git commit --allow-empty -m foo &&
> > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
> > + sha1=$(git rev-parse HEAD) &&
> > + git -C super-clone update-index --cacheinfo 16,$sha1,sub &&
>
> The use of update-index seems elegant to me, though different than
> any submodule test I wrote so far. :)
Yeah, I actually wrestled with finding the shortest recipe to convince
git-submodule to actually call git-fetch. Suggestions welcome if there's
something more canonical.
But I think we have to advance the submodule pointer in some way to
convince it to want to fetch (I also tried deleting the refs in the
cloned module, but that seemed hacky).
I guess the way it would happen in real life is that the "origin" remote
("super" here, not "super-clone") would make the change and commit the
submodule, and then "super-clone" would pull it.
That seemed even more convoluted to me.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
On Thu, Apr 28, 2016 at 6:38 AM, Jeff King wrote:
> These functions should be used by any code which spawns a
> submodule process, which may happen in submodule.c (e.g.,
> for spawning fetch). Let's move them there and make them
> public so that submodule--helper can continue to use them.
>
> Sine they're now public, let's also provide a basic overview
> of their intended use.
>
> Signed-off-by: Jeff King
> ---
> builtin/submodule--helper.c | 48
> submodule.c | 49
> +
> submodule.h | 16 +++
> 3 files changed, 65 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..de3ad5b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv,
> const char *prefix)
> return 0;
> }
>
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> - if (starts_with(var, "credential."))
> - return 1;
> - return 0;
> -}
> -
> -static int sanitize_submodule_config(const char *var, const char *value,
> void *data)
> -{
> - struct strbuf *out = data;
> -
> - if (submodule_config_ok(var)) {
> - if (out->len)
> - strbuf_addch(out, ' ');
> -
> - if (value)
> - sq_quotef(out, "%s=%s", var, value);
> - else
> - sq_quote_buf(out, var);
> - }
> -
> - return 0;
> -}
> -
> -static void prepare_submodule_repo_env(struct argv_array *out)
> -{
> - const char * const *var;
> -
> - for (var = local_repo_env; *var; var++) {
> - if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> - struct strbuf sanitized_config = STRBUF_INIT;
> - git_config_from_parameters(sanitize_submodule_config,
> - &sanitized_config);
> - argv_array_pushf(out, "%s=%s", *var,
> sanitized_config.buf);
> - strbuf_release(&sanitized_config);
> - } else {
> - argv_array_push(out, *var);
> - }
> - }
> -
> -}
> -
> static int clone_submodule(const char *path, const char *gitdir, const char
> *url,
>const char *depth, const char *reference, int
> quiet)
> {
> diff --git a/submodule.c b/submodule.c
> index 90825e1..02eaf0e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -13,6 +13,7 @@
> #include "argv-array.h"
> #include "blob.h"
> #include "thread-utils.h"
> +#include "quote.h"
>
> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> static int parallel_jobs = 1;
> @@ -1129,3 +1130,51 @@ int parallel_submodules(void)
> {
> return parallel_jobs;
> }
> +
> +/*
> + * Rules to sanitize configuration variables that are Ok to be passed into
> + * submodule operations from the parent project using "-c". Should only
> + * include keys which are both (a) safe and (b) necessary for proper
> + * operation.
> + */
> +static int submodule_config_ok(const char *var)
> +{
> + if (starts_with(var, "credential."))
> + return 1;
> + return 0;
> +}
> +
> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> + struct strbuf *out = data;
> +
> + if (submodule_config_ok(var)) {
> + if (out->len)
> + strbuf_addch(out, ' ');
> +
> + if (value)
> + sq_quotef(out, "%s=%s", var, value);
> + else
> + sq_quote_buf(out, var);
> + }
> +
> + return 0;
> +}
> +
> +void prepare_submodule_repo_env(struct argv_array *out)
> +{
> + const char * const *var;
> +
> + for (var = local_repo_env; *var; var++) {
> + if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> + struct strbuf sanitized_config = STRBUF_INIT;
> + git_config_from_parameters(sanitize_submodule_config,
> + &sanitized_config);
> + argv_array_pushf(out, "%s=%s", *var,
> sanitized_config.buf);
> + strbuf_release(&sanitized_config);
> + } else {
> + argv_array_push(out, *var);
> + }
> + }
> +
> +}
> diff --git a/submodule.h b/submodule.h
> index 7ef3775..7577b3b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -61,4 +61,20 @@ int push_unpushed_submodules(unsign
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 08:28:29AM -0700, Jacob Keller wrote: > > diff --git a/git-submodule.sh b/git-submodule.sh > > index 2a84d7e..3a40d4b 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -200,6 +200,7 @@ sanitize_submodule_env() > > sanitized_config=$(git submodule--helper sanitize-config) > > clear_local_git_env > > GIT_CONFIG_PARAMETERS=$sanitized_config > > + export GIT_CONFIG_PARAMETERS > > why not > > export GIT_CONFIG_PARAMETERS=$santized_config Portability. Try: $ dash -c 'one="foo bar"; export two=$one; echo $two' foo $ bash -c 'one="foo bar"; export two=$one; echo $two' foo bar I think: export GIT_CONFIG_PARAMETERS="$sanitized_config" solves that. Some antique shells do not like "export x=y" at all, but I don't know if any of them are still relevant. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
On Thu, Apr 28, 2016 at 08:30:45AM -0700, Stefan Beller wrote: > > +/* > > + * This function is intended as a callback for use with > > + * git_config_from_parameters(). It ignores any config options which > > + * are not suitable for passing along to a submodule, and accumulates the > > rest > > + * in "data", which must be a pointer to a strbuf. > > So why is it a void* then? You could make it a strbuf* here, so you > would not have to document it? > Oh right, because of git_config_from_parameters(sanitize_submodule_config, ... Exactly (and why it is all the more important do document it!). > > The end result can > > + * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. > > s/sub-process/process operating on submodules/, maybe ? > > While it is technically a sub-process, I started to have an aversion > against "sub"-things > unless strictly required. :) Technically it can be any process which then spawns a process operating on a submodule. Maybe "another process" would be enough? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 6:49 AM, Jeff King wrote: > On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote: > >> > Should we consider just white-listing all of "http.*"? >> > >> > That would help other cases which have come up, like: >> > >> > http://thread.gmane.org/gmane.comp.version-control.git/264840 >> > >> > which wants to turn off http.sslverify. That would mean it turns off for >> > every submodule, too, but if you want to be choosy about your http >> > variables, you should be using the "http.$URL.sslverify" form, to only >> > affect specific servers (whether they are in submodules or not). >> >> I considered that, and thought that it might be dangerous, what with me >> not vetting carefully which http.* variables are safe to pass on to the >> submodules' update and which are not. >> >> So I had a look now, and the most prominent potential problem is the >> http.cookieFile setting, which could be reused all of a sudden if we >> made my patch more general. >> >> But then, we are talking about the code that filters what gets passed via >> the *command-line*. And to be quite honest, I am not sure that we should >> actually filter out *any* of these settings. > > The intent of the whitelist (from my recollection of the discussion) is > to filter out config that must be repo-specific. E.g., core.worktree or > core.bare should definitely _not_ be passed to a submodule. > > I don't know if there are others. We started with a whitelist because it > was the smallest and safest change away from the status quo. A blacklist > would also work, with the risk that we might let through nonsense in > some cases (but only if the user triggers us to do so). > >> The commit message that introduced this particular filtering has this >> rationale to let only credential.* through: >> >> GIT_CONFIG_PARAMETERS is special, and we actually do want to >> preserve these settings. However, we do not want to preserve all >> configuration as many things should be left specific to the parent >> project. >> >> Add a git submodule--helper function, sanitize-config, which shall be >> used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs >> except a small subset that are known to be safe and necessary. >> >> Dunno. I tried to err on the side of caution... But this sounds maybe a >> bit *too* cautious? > > So if we all agree that the sanitizing is really about preventing > repo-specific variables from leaking, and not any kind of security > boundary, I think we should generally be pretty liberal in whitelisting > things. > > I can certainly come up with a pathological case where using it as a > security boundary may have some practical use, but in general I think it > is mostly getting in the way of what users are trying to do. > > -Peff I think I prefer a blacklist approach, since it reduces the need for future changes, since most cases will either not put config on the environment or (based on feedback on the mailing list and bug reports) the user will believe it should be applied. A black list which only removed configurations we know are harmful would be easier to maintain but risks new additions forgetting to do so. A whitelist means we only fix things as they come up but also means we aren't "breaking" anything that works today, where as a blacklist could break something that works today. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 8:28 AM, Jeff King wrote:
> On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote:
>
>> > +test_expect_success 'cmdline credential config passes submodule update' '
>> > + # advance the submodule HEAD so that a fetch is required
>> > + git commit --allow-empty -m foo &&
>> > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
>> > + sha1=$(git rev-parse HEAD) &&
>> > + git -C super-clone update-index --cacheinfo 16,$sha1,sub &&
>>
>> The use of update-index seems elegant to me, though different than
>> any submodule test I wrote so far. :)
>
> Yeah, I actually wrestled with finding the shortest recipe to convince
> git-submodule to actually call git-fetch. Suggestions welcome if there's
> something more canonical.
>
> But I think we have to advance the submodule pointer in some way to
> convince it to want to fetch (I also tried deleting the refs in the
> cloned module, but that seemed hacky).
>
> I guess the way it would happen in real life is that the "origin" remote
> ("super" here, not "super-clone") would make the change and commit the
> submodule, and then "super-clone" would pull it.
>
> That seemed even more convoluted to me.
That's what I write in the other submodule tests, because I think we should
test closest to reality.
Instead of deleting the refs code, another hacky way would be
echo $newsha1 > .git/module/remote/origin/refs/heads/master
but the update-index is less hacky, so let's keep that.
I did not want to point out an issue with it, just that I was pleasantly
surprised to see such a short test.
Thanks,
Stefan
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote: > I think I prefer a blacklist approach, since it reduces the need for > future changes, since most cases will either not put config on the > environment or (based on feedback on the mailing list and bug reports) > the user will believe it should be applied. > > A black list which only removed configurations we know are harmful > would be easier to maintain but risks new additions forgetting to do > so. A whitelist means we only fix things as they come up but also > means we aren't "breaking" anything that works today, where as a > blacklist could break something that works today. I think the key thing with a blacklist is somebody has to go to the work to audit the existing keys. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
On Thu, Apr 28, 2016 at 8:36 AM, Jeff King wrote: > On Thu, Apr 28, 2016 at 08:28:29AM -0700, Jacob Keller wrote: > >> > diff --git a/git-submodule.sh b/git-submodule.sh >> > index 2a84d7e..3a40d4b 100755 >> > --- a/git-submodule.sh >> > +++ b/git-submodule.sh >> > @@ -200,6 +200,7 @@ sanitize_submodule_env() >> > sanitized_config=$(git submodule--helper sanitize-config) >> > clear_local_git_env >> > GIT_CONFIG_PARAMETERS=$sanitized_config >> > + export GIT_CONFIG_PARAMETERS >> >> why not >> >> export GIT_CONFIG_PARAMETERS=$santized_config > > Portability. Try: > > $ dash -c 'one="foo bar"; export two=$one; echo $two' > foo > > $ bash -c 'one="foo bar"; export two=$one; echo $two' > foo bar > > I think: > > export GIT_CONFIG_PARAMETERS="$sanitized_config" > > solves that. Some antique shells do not like "export x=y" at all, but I > don't know if any of them are still relevant. > > -Peff Fair enough. quotes would most likely work and I know I am in the habbit of using quotes a bit more liberally but this will work correctly so I wouldn't change it. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fixes for sanitized submodule config
On Thu, Apr 28, 2016 at 7:02 AM, Johannes Schindelin wrote: > Hi Peff, > > On Thu, 28 Apr 2016, Jeff King wrote: > >> On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote: >> >> > So that case _is_ correct right now. It's just that t5550 isn't testing >> > the shell script part, which is broken. Probably running "git submodule >> > update" in the resulting clone would cover that. >> > >> > And for the fetch case, we probably just need to be calling >> > prepare_submodule_repo_env() there, too. >> >> So here's a series which fixes sanitizing in the "git-submodule" shell >> script, along with "git fetch". And cleans up a few things along the >> way. > > Nice! > > I reviewed those changes and they all look sensible to me (did not apply > them locally for lack of time, though). Same here. > > Ciao, > Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fixes for sanitized submodule config
On Thu, Apr 28, 2016 at 8:56 AM, Stefan Beller wrote: > On Thu, Apr 28, 2016 at 7:02 AM, Johannes Schindelin > wrote: >> Hi Peff, >> >> On Thu, 28 Apr 2016, Jeff King wrote: >> >>> On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote: >>> >>> > So that case _is_ correct right now. It's just that t5550 isn't testing >>> > the shell script part, which is broken. Probably running "git submodule >>> > update" in the resulting clone would cover that. >>> > >>> > And for the fetch case, we probably just need to be calling >>> > prepare_submodule_repo_env() there, too. >>> >>> So here's a series which fixes sanitizing in the "git-submodule" shell >>> script, along with "git fetch". And cleans up a few things along the >>> way. >> >> Nice! >> >> I reviewed those changes and they all look sensible to me (did not apply >> them locally for lack of time, though). > > Same here. > Same as well. Looks good, and good catch finding the bug! Thanks, Jake >> >> Ciao, >> Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 8:39 AM, Jeff King wrote: > On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote: > >> I think I prefer a blacklist approach, since it reduces the need for >> future changes, since most cases will either not put config on the >> environment or (based on feedback on the mailing list and bug reports) >> the user will believe it should be applied. >> >> A black list which only removed configurations we know are harmful >> would be easier to maintain but risks new additions forgetting to do >> so. A whitelist means we only fix things as they come up but also >> means we aren't "breaking" anything that works today, where as a >> blacklist could break something that works today. > > I think the key thing with a blacklist is somebody has to go to the work > to audit the existing keys. Would it be sufficient to wait until someone screams at the mailing list for some key to be blacklisted? (I mean in the short term that would be of less quality, but relying on the larger community would result in a better end result? So your going through is just a jump start this process of listening to the community?) Thanks, Stefan > > -Peff > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary
On Thu, Apr 28, 2016 at 7:12 AM, Mike Hommey wrote:
> Some remote systems can employ restricted shells that aren't very smart
> with quotes, so avoid quoting when it's not strictly necessary.
>
> The list of "safe" characters comes from Mercurial's shell quoting
> function used for its ssh client side. There likely are more that could
> be added to the list.
>
Would it make sense to move the new code into its own function and
document it with this paragraph of the commit message, i.e. hinting
at Mercurial safe characters or others?
> Signed-off-by: Mike Hommey
> ---
> connect.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index 96c8c1d..919bf9e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -668,6 +668,17 @@ static void prepare_connect_command(struct strbuf *cmd,
> const char *prog,
> strbuf_addstr(cmd, prog);
> strbuf_addch(cmd, ' ');
> }
> + if (quote) {
> + const char *p;
> + for (p = path; *p; p++) {
> + if (!isalnum(*p) && *p != '@' && *p != '%' &&
> + *p != '_' && *p != '+' && *p != '=' && *p != ':'
> &&
> + *p != ',' && *p != '.' && *p != '/' && *p != '-')
> + break;
> + }
> + if (!*p)
> + quote = 0;
> + }
> if (quote)
> sq_quote_buf(cmd, path);
> else
> --
> 2.8.1.5.g18c8a48
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
> On 28 Apr 2016, at 15:38, Jeff King wrote:
>
> These functions should be used by any code which spawns a
> submodule process, which may happen in submodule.c (e.g.,
> for spawning fetch). Let's move them there and make them
> public so that submodule--helper can continue to use them.
>
> Sine they're now public, let's also
s/Sine/Since
Thanks for fixing this entire issue, Peff!
- Lars
> provide a basic overview
> of their intended use.
>
> Signed-off-by: Jeff King
> ---
> builtin/submodule--helper.c | 48
> submodule.c | 49 +
> submodule.h | 16 +++
> 3 files changed, 65 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..de3ad5b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv,
> const char *prefix)
>return 0;
> }
>
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> -if (starts_with(var, "credential."))
> -return 1;
> -return 0;
> -}
> -
> -static int sanitize_submodule_config(const char *var, const char *value,
> void *data)
> -{
> -struct strbuf *out = data;
> -
> -if (submodule_config_ok(var)) {
> -if (out->len)
> -strbuf_addch(out, ' ');
> -
> -if (value)
> -sq_quotef(out, "%s=%s", var, value);
> -else
> -sq_quote_buf(out, var);
> -}
> -
> -return 0;
> -}
> -
> -static void prepare_submodule_repo_env(struct argv_array *out)
> -{
> -const char * const *var;
> -
> -for (var = local_repo_env; *var; var++) {
> -if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> -struct strbuf sanitized_config = STRBUF_INIT;
> -git_config_from_parameters(sanitize_submodule_config,
> - &sanitized_config);
> -argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> -strbuf_release(&sanitized_config);
> -} else {
> -argv_array_push(out, *var);
> -}
> -}
> -
> -}
> -
> static int clone_submodule(const char *path, const char *gitdir, const char
> *url,
> const char *depth, const char *reference, int quiet)
> {
> diff --git a/submodule.c b/submodule.c
> index 90825e1..02eaf0e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -13,6 +13,7 @@
> #include "argv-array.h"
> #include "blob.h"
> #include "thread-utils.h"
> +#include "quote.h"
>
> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> static int parallel_jobs = 1;
> @@ -1129,3 +1130,51 @@ int parallel_submodules(void)
> {
>return parallel_jobs;
> }
> +
> +/*
> + * Rules to sanitize configuration variables that are Ok to be passed into
> + * submodule operations from the parent project using "-c". Should only
> + * include keys which are both (a) safe and (b) necessary for proper
> + * operation.
> + */
> +static int submodule_config_ok(const char *var)
> +{
> +if (starts_with(var, "credential."))
> +return 1;
> +return 0;
> +}
> +
> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> +struct strbuf *out = data;
> +
> +if (submodule_config_ok(var)) {
> +if (out->len)
> +strbuf_addch(out, ' ');
> +
> +if (value)
> +sq_quotef(out, "%s=%s", var, value);
> +else
> +sq_quote_buf(out, var);
> +}
> +
> +return 0;
> +}
> +
> +void prepare_submodule_repo_env(struct argv_array *out)
> +{
> +const char * const *var;
> +
> +for (var = local_repo_env; *var; var++) {
> +if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> +struct strbuf sanitized_config = STRBUF_INIT;
> +git_config_from_parameters(sanitize_submodule_config,
> + &sanitized_config);
> +argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> +strbuf_release(&sanitized_config);
> +} else {
> +argv_array_push(out, *var);
> +}
> +}
> +
> +}
> diff --git a/submodule.h b/submodule.h
> index 7ef3775..7577b3b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -61,4 +61,20 @@ int push_unpushed_submodules(unsigned char new_sha1[20],
> const char *remotes_nam
> void connect_work_tree_and_git_dir(const char *work_tree, const char
> *git_dir);
> int parallel_submodules(void);
>
> +/*
> + * This function is intended as a callback for use with
> + * git_config_from_parameters(). It ignores any config options which
> + * are not suitable for passing along to
Re: [PATCH 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'
On Mon, Apr 25, 2016 at 7:55 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >>> + /* >>> +* Since lockfile.c keeps a linked list of all created >>> +* lock_file structures, it isn't safe to free(lock_file). >>> +*/ >>> + struct lock_file *lock_file; >> >> Is there ever a time when lock_file is removed from the list (such as >> upon successful write of the index), in which case it would be safe to >> free() this? > > I do not think you need to think about "free"ing. Yeah, lockfile.h says: * * Automatic cruft removal. If the program exits after we lock a * file but before the changes have been committed, we want to make * sure that we remove the lockfile. This is done by remembering the * lockfiles we have created in a linked list and setting up an * `atexit(3)` handler and a signal handler that clean up the * lockfiles. This mechanism ensures that outstanding lockfiles are * cleaned up if the program exits (including when `die()` is * called) or if the program is terminated by a signal. and: * The caller: * * * Allocates a `struct lock_file` either as a static variable or on * the heap, initialized to zeros. Once you use the structure to * call the `hold_lock_file_for_*()` family of functions, it belongs * to the lockfile subsystem and its storage must remain valid * throughout the life of the program (i.e. you cannot use an * on-stack variable to hold this structure). > Even if the libified version of the apply internal can be called > multiple times to process multiple patch inputs, there is no need to > run multiple instances of it yet. And a lockfile, after the caller > finishes interacting with one file using it by calling commit or > rollback, can be reused to interact with another file. > > So the cleanest would be to: > > * make the caller of apply API responsible for preparing a static >or (allocating and leaking) lockfile instance, Ok. > * make it pass a pointer to the lockfile to the apply API so that >it can store it in apply_state, and Ok, I will add a new "struct lock_file *" parameter to init_apply_state(), for the caller of the apply API to do that. Though I think it should be ok for the caller to pass a NULL pointer and in this case have init_apply_state() allocate the lockfile file instance. > * have the caller use apply API feeding one patch at a time in a >loop, allowing the same lockfile instance used for each >"invocation". Ok, the same lockfile instance will be used for each invocation. > I sounded as if I were advocating non-reentrant implementation in > the introductory paragraph, but that is not the intention. If you > want to do N threads, you would prepare N lockfile instances, give > them to N apply API instances to be stored in N apply_state > instances, and you would have N parallel applications, if you wanted > to. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Strangeness with git-add and nested repositories
On Wed, Apr 27, 2016 at 11:10 PM, Andrew J wrote: > Hi Stefan, > > On Wed, Apr 27, 2016 at 9:08 AM, Stefan Beller wrote: >> I think (pure speculation), that it the error is in the context >> (repository) switching logic. >> What happens if you alter the order, i.e. give testfile first and then >> the files in the nested >> repos? > > Interestingly, reversing the order produces the same result (the > testfile is added, the nested files are not). > > I've also noticed that running something like 'git status testfile > nestedfiles' results in the nested files being omitted from the git > status output; I'd expect them to be printed by git-status as > untracked files. So it appears the problem is not isolated to git-add. Looking at the code, git-add uses parse_pathspec, which is used my most commands[https://github.com/git/git/blob/master/builtin/add.c#L364] and I think that function needs to learn to cover paths for different repositories. Maybe we can pass in a flag, which allows parse_pathspec to get files from the different repos and then it's up to each command how to deal with that list of files. > > To give some context, my use case is that I have a parent project that > links to numerous chromium libraries, thus my parent project needs > access to many of chromium's headers at build time. I wanted to make > these headers available to other developers without them needing to > check out all of chromium. > So I add all the chromium headers to the parent project with something like: > find deps/chromium/src -name "*.h" | xargs git add -- > I was weirded out to find that many of the header files weren't being > added, as I've already described. > > I ultimately worked around this by doing: > find chromium/src -name "*.h" | xargs -n 1 git add > Since each file gets added separately, this is quite slow. So it'd be > nice if this little bug was fixed someday :) > > As you probably know, Chromium is comprised of many hundreds of nested > repos, so that aided in manifesting this issue. > > Thanks, > Andrew -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Strangeness with git-add and nested repositories
Stefan Beller writes: > I think (pure speculation), that it the error is in the context > (repository) switching logic. > What happens if you alter the order, i.e. give testfile first and then > the files in the nested > repos? > > git add -- file path/to/subdir/file > > should do internally IMHO: > > git add file > git -C path-to-subdir add file My undertanding of what _should_ happen in the world order as currently defined (not necessarily implemented) is: * "git add -- A B" must work the same way as "git add -- B A" and "git add -- A; git add -- B" * "git add -- path/to/subdir/file", when any of path/, path/to/, path/to/subdir/ is a Git repository that is different from the current Git repository, must fail. IOW, if 'path' is a repository (whether it is known as a submodule to the repository whose working tree contains it, or it is an untracked directory from the containing repository's point of view), the index of the containing repository cannot get path/$anything in it. If you managed to do so, you found a bug [*1*]. path/.git/index can of course have "to/subdir/file" in it, and from that point of view, "git -C path/to/subdir add file" may one day become an improved world order. It is just we haven't discussed that possibility or reached concensus that it is a good idea. [Footnote] *1* Of course, some of the bugs in this class may fundamentally be unfixable and would fall into the same category as "doctor, it hurts when I do this--don't do it then". For example, you may treat path/ as the top of the working tree of another repository whose git-dir is not at path/.git by arranging GIT_WORK_TREE and GIT_DIR environment variables, but you may do so only when you actually are accessing the contents of path/ as its own project. And when you are using the enclosing project (whose .git/ would sit next to path/), there is no way for "git add path/to/file" to know that everything under "path/" does not belong to the current repository and instead it is part of the project rooted at path/, which is an obvious example of "fundamentally unfixable" case. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Strangeness with git-add and nested repositories
Junio C Hamano writes: > * "git add -- path/to/subdir/file", when any of path/, path/to/, >path/to/subdir/ is a Git repository that is different from the >current Git repository, must fail. If any of the leading directories in that long path is actually a git repository that is different from the current one, it shouldn't have added it. IOW, adding the path is a bug. "git add A/B/C" usually enforces this rule by - check A; if A is a separate repository (A/.git exists, etc.), do not add this path; - check A/B; if A/B is a separate repository, ignore; - check A/B/C; if A/B/C is a separate repository, ignore. otherwise add it. When given multiple paths, e.g. "git add A/B/C A/D", it tries to "optimize" things by first finding common leading directory (in this case "A/") and doing something slightly different, and I think the bug Andrew saw lies in that codepath. It is likely that the code is forgetting to make sure that there is no top of enclosed working tree in the common leading directory part of the path. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote: > > I think the key thing with a blacklist is somebody has to go to the work > > to audit the existing keys. > > Would it be sufficient to wait until someone screams at the mailing list > for some key to be blacklisted? (I mean in the short term that would be > of less quality, but relying on the larger community would result in a better > end result? So your going through is just a jump start this process of > listening to the community?) Yeah, I think ultimately we will rely on the community. But I would feel a lot more comfortable if somebody made at least a single pass. I'll be curious what Junio says, too. I generally defer to him on how conservative we want to be in cases like this. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
Hi, On Thu, 28 Apr 2016, Jeff King wrote: > On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote: > > > > +test_expect_success 'cmdline credential config passes submodule update' ' > > > + # advance the submodule HEAD so that a fetch is required > > > + git commit --allow-empty -m foo && > > > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD && > > > + sha1=$(git rev-parse HEAD) && > > > + git -C super-clone update-index --cacheinfo 16,$sha1,sub && > > > > The use of update-index seems elegant to me, though different than > > any submodule test I wrote so far. :) > > Yeah, I actually wrestled with finding the shortest recipe to convince > git-submodule to actually call git-fetch. Suggestions welcome if there's > something more canonical. FWIW that's exactly how I did things in https://github.com/dscho/git/commit/89d0024450b0e6e9997ad9e3d681248bde1bafc0 :-) Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Strangeness with git-add and nested repositories
On Thu, Apr 28, 2016 at 9:39 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> I think (pure speculation), that it the error is in the context >> (repository) switching logic. >> What happens if you alter the order, i.e. give testfile first and then >> the files in the nested >> repos? >> >> git add -- file path/to/subdir/file >> >> should do internally IMHO: >> >> git add file >> git -C path-to-subdir add file > > My undertanding of what _should_ happen in the world order as > currently defined (not necessarily implemented) is: > > * "git add -- A B" must work the same way as "git add -- B A" and >"git add -- A; git add -- B" I agree. > > * "git add -- path/to/subdir/file", when any of path/, path/to/, >path/to/subdir/ is a Git repository that is different from the >current Git repository, must fail. I agree that this is the current expectation for the world order. However I would like to propose to change that eventually. (Once the submodule groups are there and we can treat submodules as a special form of narrow checkout, we want to have the feature of adding across submodules and even committing across submodules/repos, I would think) > > IOW, if 'path' is a repository (whether it is known as a submodule > to the repository whose working tree contains it, or it is an > untracked directory from the containing repository's point of view), > the index of the containing repository cannot get path/$anything in > it. If you managed to do so, you found a bug [*1*]. > > path/.git/index can of course have "to/subdir/file" in it, and from > that point of view, "git -C path/to/subdir add file" may one day > become an improved world order. It is just we haven't discussed > that possibility or reached concensus that it is a good idea. > > > [Footnote] > > *1* Of course, some of the bugs in this class may fundamentally be > unfixable and would fall into the same category as "doctor, it > hurts when I do this--don't do it then". For example, you may > treat path/ as the top of the working tree of another repository > whose git-dir is not at path/.git by arranging GIT_WORK_TREE and > GIT_DIR environment variables, but you may do so only when you > actually are accessing the contents of path/ as its own project. > And when you are using the enclosing project (whose .git/ would > sit next to path/), there is no way for "git add path/to/file" > to know that everything under "path/" does not belong to the > current repository and instead it is part of the project rooted > at path/, which is an obvious example of "fundamentally > unfixable" case. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixed grammar mistake in the french localization
CC'ed Jean-Noël who maintains French translation. Antonin writes: > "tous le dépôts distants" -> "tous les dépôts distants" > --- > po/fr.po | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/po/fr.po b/po/fr.po > index 88b0b8a7..36c7c99 100644 > --- a/po/fr.po > +++ b/po/fr.po > @@ -6135,7 +6135,7 @@ msgstr "git fetch --all []" > > #: builtin/fetch.c:92 builtin/pull.c:166 > msgid "fetch from all remotes" > -msgstr "récupérer depuis tous le dépôts distants" > +msgstr "récupérer depuis tous les dépôts distants" > > #: builtin/fetch.c:94 builtin/pull.c:169 > msgid "append to .git/FETCH_HEAD instead of overwriting" > > -- > https://github.com/git/git/pull/221 > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pull: make --rebase --verify-signatures illegal
On Thu, Apr 28, 2016 at 2:52 AM, Alexander 'z33ky' Hirsch
<1ze...@gmail.com> wrote:
> Previously git-pull would silently ignore the --verify-signatures
> option.
>
> Signed-off-by: Alexander 'z33ky' Hirsch <1ze...@gmail.com>
> ---
>
> We had some discussion back in December about a patch that added
> --verify-signatures to git-pull, that was declined.
> I proposed making git-pull --rebase --verify-signatures illegal then,
> although it still remained open whether to just warn or make it an
> error.
>
> In this patch I opted into making it an error, which breaks the
> previously accepted git pull --rebase --verify-signatures of course,
> albeit I'd argue that it probably didn't do what the user expected
> anyways.
> I don't know if there are compatibility concerns with this though.
>
> builtin/pull.c | 2 ++
> t/t5520-pull.sh | 14 ++
> 2 files changed, 16 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d98f481..b6e1507 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,
> argv_array_push(&args, "--no-autostash");
> else if (opt_autostash == 1)
> argv_array_push(&args, "--autostash");
> + if (opt_verify_signatures && strcmp(opt_verify_signatures,
> "--verify-signatures") == 0)
> + die(_("The --verify-signatures option does not work for
> --rebase."));
Can you explain why it doesn't work (In the commit message)?
>
> argv_array_push(&args, "--onto");
> argv_array_push(&args, sha1_to_hex(merge_head));
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 739c089..cb8f741 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should
> override pull.rebase' '
> test new = "$(git show HEAD:file2)"
> '
>
> +test_expect_success "pull --rebase --verify-signatures is illegal" '
> + git reset --hard before-rebase &&
> + test_must_fail git pull --rebase --verify-signatures . copy 2>err &&
> + test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
> + test_i18ngrep "The --verify-signatures option does not work for
> --rebase." err
> +'
> +
> +test_expect_success "pull --rebase --no-verify-signatures" '
> + git reset --hard before-rebase &&
> + git pull --rebase --no-verify-signatures . copy &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)"
> +'
> +
> # add a feature branch, keep-merge, that is merged into master, so the
> # test can try preserving the merge commit (or not) with various
> # --rebase flags/pull.rebase settings.
> --
> 2.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] pull: make --rebase --verify-signatures illegal
Alexander 'z33ky' Hirsch <1ze...@gmail.com> writes:
> Previously git-pull would silently ignore the --verify-signatures
> option.
I do not see a point of making it error out. Adding a warning about
the option being ignored might be a worthwhile thing to do (e.g. it
may solicit responses from those who have been depending on it not
erroring out, saying "these warnings are pointless and noisy--here
is my valid use case that ends up passing --rebase and --v-s at the
same time"), though.
> Signed-off-by: Alexander 'z33ky' Hirsch <1ze...@gmail.com>
> ---
>
> I made the error-message conform to the CodingGuidelines (removed
> capitalization and full stop).
>
> Also, in the previous mail I said that I proposed a patch for git-pull
> last December, when I actually meant git-rebase.
>
> builtin/pull.c | 2 ++
> t/t5520-pull.sh | 14 ++
> 2 files changed, 16 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d98f481..b6e1507 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,
> argv_array_push(&args, "--no-autostash");
> else if (opt_autostash == 1)
> argv_array_push(&args, "--autostash");
> + if (opt_verify_signatures && strcmp(opt_verify_signatures,
> "--verify-signatures") == 0)
> + die(_("the --verify-signatures option does not work for
> --rebase"));
>
> argv_array_push(&args, "--onto");
> argv_array_push(&args, sha1_to_hex(merge_head));
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 739c089..cb8f741 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should
> override pull.rebase' '
> test new = "$(git show HEAD:file2)"
> '
>
> +test_expect_success "pull --rebase --verify-signatures is illegal" '
> + git reset --hard before-rebase &&
> + test_must_fail git pull --rebase --verify-signatures . copy 2>err &&
> + test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
> + test_i18ngrep "The --verify-signatures option does not work for
> --rebase." err
> +'
> +
> +test_expect_success "pull --rebase --no-verify-signatures" '
> + git reset --hard before-rebase &&
> + git pull --rebase --no-verify-signatures . copy &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)"
> +'
> +
> # add a feature branch, keep-merge, that is merged into master, so the
> # test can try preserving the merge commit (or not) with various
> # --rebase flags/pull.rebase settings.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On Thu, Apr 28, 2016 at 4:57 AM, Elia Pinto wrote: > Implement the GIT_TRACE_CURL environment variable to allow a > greater degree of detail of GIT_CURL_VERBOSE, in particular > the complete transport header and all the data payload exchanged. > It might be useful if a particular situation could require a more > thorough debugging analysis. Document the new GIT_TRACE_CURL > environment variable. > > Helped-by: Torsten Bögershausen > Helped-by: Ramsay Jones > Helped-by: Junio C Hamano > Helped-by: Eric Sunshine > Helped-by: Jeff King > Signed-off-by: Elia Pinto > --- > Documentation/git.txt | 8 > http.c| 109 > +- > http.h| 4 ++ > 3 files changed, 120 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 8afe349..958db0f 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -1075,6 +1075,14 @@ of clones and fetches. > cloning of shallow repositories. > See 'GIT_TRACE' for available trace output options. > > +'GIT_TRACE_CURL':: > + Enables a curl full trace dump of all incoming and outgoing data, > + including descriptive information, of the git transport protocol. > + This is similar to doing curl --trace-ascii on the command line. > + This option overrides setting the GIT_CURL_VERBOSE environment > + variable. How does it overwrite the GIT_CURL_VERBOSE variable? After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things: * apparently GIT_CURL_VERBOSE is used as a boolean, so I presume we assume True for GIT_CURL_VERBOSE, but extend it? * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in the release notes for 2.3.0, not sure if that counts as documentation) As you know the area, care to send a documentation patch for GIT_CURL_VERBOSE? I am trying to understand how much more information I get by using GIT_TRACE_CURL instead of GIT_CURL_VERBOSE. GIT_TRACE_CURL follows the standard of GIT_TRACE_$subsystem, so I guess that will be the encouraged way of debugging and GIT_CURL_VERBOSE will not be encouraged to the user? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
Jeff King writes:
>> + for (w = 0; (w < width) && (i + w < size); w++) {
>> + if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
>> +&& ptr[i + w + 1] == '\n') {
>> +i += (w + 2 - width);
>> +break;
>> + }
>
> This loop puzzled me for a bit. When we end early due to a newline, we
> subtract out the width here. I guess that's to accomodate the "i +=
> width" that the outer for-loop is going to do.
I think I essentially said the same thing on the previous round and
I thought I suggested to restructure the loop to primarily aim to
split at line-end (instead of the above which primarily aims to
split at width but line-end may cause a premature split).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/4] git_connect: add some flexibility
Mike Hommey writes: > As you may be aware, I'm working on a git remote helper to access > mercurial repositories (https://github.com/glandium/git-cinnabar/). > > At the moment, a small part is written in C, relying on the git code > base, but eventually, there would be more C. > > As I want to get rid of the dependency on Mercurial itself, I'm planning > to implement the wire protocol parts in git-cinnabar. While all of the above sounds like a good thing to do, what I do not understand is why you need to even touch git_connect() at all, and we certainly do *not* want you to touch it to make its external interface unnecessarily ugly and complex with features that are only necessary if it needs to talk to non-git services. In other words, why can't this cinnabar thing live on the other side of transport API, just like Git transport itself does not know about cURL and HTTP when talking with https:// repositories? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote: > > +'GIT_TRACE_CURL':: > > + Enables a curl full trace dump of all incoming and outgoing data, > > + including descriptive information, of the git transport protocol. > > + This is similar to doing curl --trace-ascii on the command line. > > + This option overrides setting the GIT_CURL_VERBOSE environment > > + variable. > > How does it overwrite the GIT_CURL_VERBOSE variable? You can't use both, as they are both triggered using the CURLOPT_VERBOSE option of curl. The main difference is that with GIT_CURL_VERBOSE, we rely on curl to print the information to stderr. With GIT_CURL_TRACE, we do the printing ourselves (so we can tweak the output format, send it to places other than stderr, etc). > After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things: > > * apparently GIT_CURL_VERBOSE is used as a boolean, > so I presume we assume True for GIT_CURL_VERBOSE, but > extend it? It's not a boolean. If the variable exists at all, we turn on verbose output (so I guess you can consider it a boolean, but we do not parse its contents as boolean; GIT_CURL_VERBOSE=false does not do what you might think). > * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in > the release notes for 2.3.0, not sure if that counts as documentation) > As you know the area, care to send a documentation patch for > GIT_CURL_VERBOSE? I think there is no need for GIT_CURL_VERBOSE once we have GIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_* interface we use elsewhere. So I think we should consider GIT_CURL_VERBOSE deprecated (though I do not mind keeping it for old-timers since it is literally one line of code). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, 2016-04-27 at 16:37 -0400, Jeff King wrote: > On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote: > > > > I thought the point is that one is a lesser check than the other, > > > and > > > we > > > need different rules for different situations. So we might allow > > > deletion on a refname that does not pass check_refname_format(), > > > but > > > we > > > must make sure it is not going to cause any mischief (e.g., > > > escaping > > > ".git" and deleting random files). > > > > > > But anything writing a _new_ refname (whether the actual ref, or > > > referencing it via a symref) should be using > > > check_refname_format() > > > before writing. > > > > Unfortunately, neither check is lesser -- refname_is_safe allows > > refs/heads//foo but not a/b while check_refname_format allows a/b > > but > > not refs/heads//foo. So sometimes we need both, while other times > > we > > just need one :( > > IMHO, that sounds like a bug. check_refname_format() should > conceptually[1] be a superset of refname_is_safe(). Is there a case > where we would want to _allow_ a refname that is not safe to look at > on > disk? The only such case I can think of is the case where there is a symref to such a bad refname, and we want to delete said symref. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > If a casual reader sees this code: > > > > ref_transaction_delete(transaction, r->name, r->sha1, > >REF_ISPRUNING | REF_NODEREF, NULL, &err) > > > > it gives an incorrect impression that there may also be a valid > > case > > to make a "delete" call with ISPRUNING alone without NODEREF, in > > other codepaths and under certain conditions, and write an > > incorrect > > > > ref_transaction_delete(transaction, refname, sha1, > >REF_ISPRUNING, NULL, &err) > > > > in her new code. Or a careless programmer and reviewer may not > > even > > memorize and remember what the new world order is when they see > > such > > a code and let it pass. > > > > As I understand that we declare that "to prune a ref from set of > > loose refs is to prune the named one, never following a symbolic > > ref" is the new world order with this patch, making sure that > > ISPRUNING automatically and always mean NODEREF will eliminate the > > possibility that any new code makes an incorrect call to "delete", > > which I think is much better. > > ... but my understanding of the point of this patch may be flawed, > in which case I of course am willing to be enlightened ;-) Since there is a manual check for that case, the code will fail at test time. But I don't have strong feelings and am happy to go either way on this. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On Thu, Apr 28, 2016 at 10:44 AM, Jeff King wrote: > On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote: > >> > +'GIT_TRACE_CURL':: >> > + Enables a curl full trace dump of all incoming and outgoing data, >> > + including descriptive information, of the git transport protocol. >> > + This is similar to doing curl --trace-ascii on the command line. >> > + This option overrides setting the GIT_CURL_VERBOSE environment >> > + variable. >> >> How does it overwrite the GIT_CURL_VERBOSE variable? > > You can't use both, as they are both triggered using the CURLOPT_VERBOSE > option of curl. The main difference is that with GIT_CURL_VERBOSE, we > rely on curl to print the information to stderr. With GIT_CURL_TRACE, we > do the printing ourselves (so we can tweak the output format, send it to > places other than stderr, etc). Well that's the information I'd rather find in the documentation than in a mailing list archive ;) > >> After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things: >> >> * apparently GIT_CURL_VERBOSE is used as a boolean, >> so I presume we assume True for GIT_CURL_VERBOSE, but >> extend it? > > It's not a boolean. If the variable exists at all, we turn on verbose > output (so I guess you can consider it a boolean, but we do not parse > its contents as boolean; GIT_CURL_VERBOSE=false does not do what you > might think). > >> * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in >> the release notes for 2.3.0, not sure if that counts as documentation) >> As you know the area, care to send a documentation patch for >> GIT_CURL_VERBOSE? > > I think there is no need for GIT_CURL_VERBOSE once we have > GIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_* > interface we use elsewhere. > > So I think we should consider GIT_CURL_VERBOSE deprecated (though I do > not mind keeping it for old-timers since it is literally one line of > code). I see, so by this patch there is no need to document GIT_CURL_VERBOSE any more? > > -Peff > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable
On Thu, Apr 28, 2016 at 10:48:38AM -0700, Stefan Beller wrote: > >> How does it overwrite the GIT_CURL_VERBOSE variable? > > > > You can't use both, as they are both triggered using the CURLOPT_VERBOSE > > option of curl. The main difference is that with GIT_CURL_VERBOSE, we > > rely on curl to print the information to stderr. With GIT_CURL_TRACE, we > > do the printing ourselves (so we can tweak the output format, send it to > > places other than stderr, etc). > > Well that's the information I'd rather find in the documentation > than in a mailing list archive ;) Sure, but I'm not sure what of part of that you want to put in the documentation. I was just explaining why the implementation constrains us to overriding, and there really aren't any other sane options. I don't think we want to get into defining the exact set of information, which is not up to us anyway. We're just relaying whatever curl gives us. IMHO, we do not even need to mention GIT_CURL_VERBOSE here at all. That is an undocumented interface that can hopefully just be forgotten over time. > > So I think we should consider GIT_CURL_VERBOSE deprecated (though I do > > not mind keeping it for old-timers since it is literally one line of > > code). > > I see, so by this patch there is no need to document > GIT_CURL_VERBOSE any more? Exactly. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule init: fail gracefully with a missing .gitmodules file
When there is no .gitmodules file availabe to initialize a submodule
from, `submodule_from_path` just returns NULL. We need to check for
that and abort gracefully. When `submodule init` was implemented in shell,
a missing .gitmodules file would result in an error message
No url found for submodule path '%s' in .gitmodules
While that is technically true, I think we can broaden the error message
and just say there is no section for the submodule in the .gitmodules file.
Signed-off-by: Stefan Beller
---
This goes on top of sb/submodule-init (I added the base-commit
below, just in case anyone uses that feature already. Though I did
that manually :)
Thanks,
Stefan
builtin/submodule--helper.c | 10 +++---
t/t7400-submodule-basic.sh | 8
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6d4f27..af5406e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -314,13 +314,17 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
- sub = submodule_from_path(null_sha1, path);
-
if (prefix) {
strbuf_addf(&sb, "%s%s", prefix, path);
displaypath = strbuf_detach(&sb, NULL);
} else
- displaypath = xstrdup(sub->path);
+ displaypath = xstrdup(path);
+
+ sub = submodule_from_path(null_sha1, path);
+
+ if (!sub)
+ die(_("No section found for submodule path '%s' in
.gitmodules"),
+ displaypath);
/*
* Copy url setting when it is not set yet.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f99f674..f2b3519 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -18,6 +18,14 @@ test_expect_success 'setup - initial commit' '
git branch initial
'
+test_expect_success 'sane abort on missing .gitmodules file' '
+ test_when_finished "git update-index --remove sub" &&
+ git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
+ # missing the .gitmodules file here
+ test_must_fail git submodule init 2>actual &&
+ test_i18ngrep "No section found for submodule path" actual
+'
+
test_expect_success 'configuration parsing' '
test_when_finished "rm -f .gitmodules" &&
cat >.gitmodules <<-\EOF &&
--
2.8.0.26.g3604242.dirty
base-commit: 3604242f080a813d6f20a7394def422d1e55b30e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 03/19] index-helper: new daemon for caching index and related stuff
David Turner writes:
> From: Nguyễn Thái Ngọc Duy
> ...
> The biggest gain is not having to verify the trailing SHA-1, which
> takes lots of time especially on large index files. But this also
> opens doors for further optimiztions:
optimizAtion
> Git can poke the daemon via unix domain sockets to tell it to refresh
> the index cache, or to keep it alive some more minutes. It can't give
> any real index data directly to the daemon. Real data goes to disk
> first, then the daemon reads and verifies it from there. Poking only
> happens for $GIT_DIR/index, not temporary index files.
Is this limited to "poking", or the helper daemon is not involved in
codepaths that handle temporary index at all? It makes sense if it
is the latter, and it doesn't if it were the former, but it is
unclear in this paragraph.
> $GIT_DIR/index-helper.sock is the socket for the daemon process. The
> daemon reads from the socket and executes commands.
>
> Named pipes were considered for portability reasons, but then commands
> that need replies from the daemon would have open their own pipes,
"would have TO open"?
> since a named pipe should only have one reader. Unix domain sockets
> don't have this problem.
> diff --git a/Documentation/git-index-helper.txt
> b/Documentation/git-index-helper.txt
> new file mode 100644
> index 000..77687c0
> --- /dev/null
> +++ b/Documentation/git-index-helper.txt
> @@ -0,0 +1,47 @@
> +git-index-helper(1)
> +===
> +
> +NAME
> +
> +git-index-helper - A simple cache daemon for speeding up index file access
> +
> +SYNOPSIS
> +
> +[verse]
> +'git index-helper' [options]
> +
> +DESCRIPTION
> +---
> +Keep the index file in memory for faster access. This daemon is per
> +repository.
Not an objection but a question. Does this mean "per index file"?
What the users would need to be aware of is that it is not like
running a single daemon instance helps the toplevel project with its
entire 600 submodules checked out you have--you would need that many
instances of the helper, which is already conveyed well with "per
repository". But I was wondering if it helps users experimenting
with the multiple worktree feature if we said "per index file". It
would make it more clear that you would run two instances of the
helper daemon when you use another worktree in addition to your
primary repository.
> +... The following commands are used to control the daemon:
> +
> +"refresh"::
> + Reread the index.
> +
> +"poke":
> + Let the daemon know the index is to be read. It keeps the
> + daemon alive longer, unless `--exit-after=0` is used.
> +
> +All commands and replies are terminated by a 0 byte.
s/0/NUL/
> diff --git a/cache.h b/cache.h
> index 4180e2b..43fb314 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -334,6 +334,8 @@ struct index_state {
> struct cache_time timestamp;
> unsigned name_hash_initialized : 1,
>keep_mmap : 1,
> + from_shm : 1,
> + to_shm : 1,
keep_mmap bit had its own commit, which made it more-or-less
unnecessary to have a separate explanation, but it is unclear how
these two bits are envisioned to be used...
> diff --git a/index-helper.c b/index-helper.c
> new file mode 100644
> index 000..976e913
> --- /dev/null
> +++ b/index-helper.c
> @@ -0,0 +1,285 @@
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +#include "exec_cmd.h"
> +#include "split-index.h"
> +#include "lockfile.h"
> +#include "cache.h"
> +#include "unix-socket.h"
> +#include "pkt-line.h"
> +
> +struct shm {
> + unsigned char sha1[20];
> + void *shm;
> + size_t size;
> +};
> +
> +static struct shm shm_index;
> +static struct shm shm_base_index;
> +
> +static void release_index_shm(struct shm *is)
> +{
> + if (!is->shm)
> + return;
> + munmap(is->shm, is->size);
> + unlink(git_path("shm-index-%s", sha1_to_hex(is->sha1)));
> + is->shm = NULL;
> +}
> +
> +static void cleanup_shm(void)
> +{
> + release_index_shm(&shm_index);
> + release_index_shm(&shm_base_index);
> +}
> +
> +static void cleanup(void)
> +{
> + unlink(git_path("index-helper.sock"));
> + cleanup_shm();
> +}
> +
> +static void cleanup_on_signal(int sig)
> +{
> + /* We ignore sigpipes -- that's just a client being broken. */
Is it really "a broken" client, in which case we would want to know
about the breakage so that we can fix it? Or is it a case of "a
client wasn't interested in what we had to say, which is perfectly
fine"?
> + if (sig == SIGPIPE)
> + return;
> + cleanup();
> + sigchain_pop(sig);
> + raise(sig);
> +}
> +
> +static int shared_mmap_create(int file_flags, int file_mode, size_t size,
> + void **new_mmap, int mmap_prot, int mmap_flags,
> + const char *path)
> +{
I can understand size, new_mmap and path, but do all others need the
customizability?
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Jeff King writes: > On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote: > >> > I think the key thing with a blacklist is somebody has to go to the work >> > to audit the existing keys. >> >> Would it be sufficient to wait until someone screams at the mailing list >> for some key to be blacklisted? (I mean in the short term that would be >> of less quality, but relying on the larger community would result in a better >> end result? So your going through is just a jump start this process of >> listening to the community?) > > Yeah, I think ultimately we will rely on the community. But I would feel > a lot more comfortable if somebody made at least a single pass. > > I'll be curious what Junio says, too. I generally defer to him on how > conservative we want to be in cases like this. Starting from an empty whitelist and waiting for people to scream with valid use cases would automatically give us the single pass to identify the set of essential ones that users must be able to pass, no? Of course, the screamed proposal to add something to whitelist must be vetted (i.e. "yeah, we can see passing X in _your_ usecase might be useful, but here are downsides (e.g. security implications) of allowing X in other usecases"). And we might even find that it is insufficient safety to allow/disallow per variable name during that discussion, in which case choice between whitelist and blacklist becomes moot. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:06:56PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote: > > > >> > I think the key thing with a blacklist is somebody has to go to the work > >> > to audit the existing keys. > >> > >> Would it be sufficient to wait until someone screams at the mailing list > >> for some key to be blacklisted? (I mean in the short term that would be > >> of less quality, but relying on the larger community would result in a > >> better > >> end result? So your going through is just a jump start this process of > >> listening to the community?) > > > > Yeah, I think ultimately we will rely on the community. But I would feel > > a lot more comfortable if somebody made at least a single pass. > > > > I'll be curious what Junio says, too. I generally defer to him on how > > conservative we want to be in cases like this. > > Starting from an empty whitelist and waiting for people to scream > with valid use cases would automatically give us the single pass to > identify the set of essential ones that users must be able to pass, > no? It's definitely sufficient, it's just annoying if a user shows up every week and says "I want X.Y", and then somebody else shows up a week later and says "I want X.Z". Are we serving any purpose in vetting each one (and if so, what)? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule init: fail gracefully with a missing .gitmodules file
On Thu, Apr 28, 2016 at 11:28 AM, Stefan Beller wrote:
> When there is no .gitmodules file availabe to initialize a submodule
> from, `submodule_from_path` just returns NULL. We need to check for
> that and abort gracefully. When `submodule init` was implemented in shell,
> a missing .gitmodules file would result in an error message
>
> No url found for submodule path '%s' in .gitmodules
>
> While that is technically true, I think we can broaden the error message
> and just say there is no section for the submodule in the .gitmodules file.
Actually I am not so sure any more about this part. In a resend I will use
the old error message again.
This fix is also required for `submodule--helper update-clone`.
>
> Signed-off-by: Stefan Beller
> ---
>
> This goes on top of sb/submodule-init (I added the base-commit
> below, just in case anyone uses that feature already. Though I did
> that manually :)
>
> Thanks,
> Stefan
>
> builtin/submodule--helper.c | 10 +++---
> t/t7400-submodule-basic.sh | 8
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b6d4f27..af5406e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -314,13 +314,17 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> /* Only loads from .gitmodules, no overlay with .git/config */
> gitmodules_config();
>
> - sub = submodule_from_path(null_sha1, path);
> -
> if (prefix) {
> strbuf_addf(&sb, "%s%s", prefix, path);
> displaypath = strbuf_detach(&sb, NULL);
> } else
> - displaypath = xstrdup(sub->path);
> + displaypath = xstrdup(path);
> +
> + sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub)
> + die(_("No section found for submodule path '%s' in
> .gitmodules"),
> + displaypath);
>
> /*
> * Copy url setting when it is not set yet.
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index f99f674..f2b3519 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -18,6 +18,14 @@ test_expect_success 'setup - initial commit' '
> git branch initial
> '
>
> +test_expect_success 'sane abort on missing .gitmodules file' '
> + test_when_finished "git update-index --remove sub" &&
> + git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
> + # missing the .gitmodules file here
> + test_must_fail git submodule init 2>actual &&
> + test_i18ngrep "No section found for submodule path" actual
> +'
> +
> test_expect_success 'configuration parsing' '
> test_when_finished "rm -f .gitmodules" &&
> cat >.gitmodules <<-\EOF &&
> --
> 2.8.0.26.g3604242.dirty
>
> base-commit: 3604242f080a813d6f20a7394def422d1e55b30e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixed grammar mistake in the french localization
Cherry-picked, fixed and PR to Jiang Xin JN -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Jeff King writes: > It's definitely sufficient, it's just annoying if a user shows up every > week and says "I want X.Y", and then somebody else shows up a week later > and says "I want X.Z". > > Are we serving any purpose in vetting each one (and if so, what)? Personally I do not think we would need to filter _anything_ if we can tell that the user directly said git -c var1=val1 -c var2=val2 $cmd ... and "git $cmd" ended up needing to spawn another "git" subcommand, possibly in some other repository (i.e. "$cmd" in this case is likely to be "submodule", but in principle it does not have to be). If the user somehow gives variables like core.worktree that are inappropriate to be applied across repositories, that's user's problem, i.e. "don't do it then if it hurts". If we are doing any filtering, however, it is always hard, if not impossible, to take away what we originally granted, even by mistake, for any reason, even for correctness or for security, in a later release. We probably could sidestep it by introducing an end-user configurable "whitelist" somewhere. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:28 PM, Junio C Hamano wrote: > Jeff King writes: > >> It's definitely sufficient, it's just annoying if a user shows up every >> week and says "I want X.Y", and then somebody else shows up a week later >> and says "I want X.Z". >> >> Are we serving any purpose in vetting each one (and if so, what)? > > Personally I do not think we would need to filter _anything_ if we > can tell that the user directly said > > git -c var1=val1 -c var2=val2 $cmd ... > > and "git $cmd" ended up needing to spawn another "git" subcommand, > possibly in some other repository (i.e. "$cmd" in this case is > likely to be "submodule", but in principle it does not have to be). > If the user somehow gives variables like core.worktree that are > inappropriate to be applied across repositories, that's user's > problem, i.e. "don't do it then if it hurts". So when going with that philosophy, the user might be missing switches like -c-for-this-repo-only core.worktree=... -c submodule.worktree=align-relative-to-parent i.e. when shifting the responsibility to the user, we need to have switches to pass options to the repository or a subset of submodules? > > If we are doing any filtering, however, it is always hard, if not > impossible, to take away what we originally granted, even by > mistake, for any reason, even for correctness or for security, in a > later release. > > We probably could sidestep it by introducing an end-user > configurable "whitelist" somewhere. > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Johannes Schindelin writes:
> - if (starts_with(var, "credential."))
> + if (starts_with(var, "credential.") ||
> + (starts_with(var, "http.") &&
> + ends_with(var, ".extraheader")))
I know you are fond of indenting with HT without aligning things,
but this is going too far in the quest of making the code
unreadable.
if (starts_with(var, "credential.") ||
(starts_with(var, "http.") && ends_with(var, ".extraheader")))
would make iteasier to see what are the top-level items (there are two)
and how they are related (just one of them needs to be satisfied).
Assuming that we will discover more variables that can be safely
passed, I'd rather see the above written like this, though:
if (starts_with(var, "credential."))
return 1;
if (starts_with(var, "http.") && ends_with(var, ".extraheader"))
return 1;
return 0;
Or even something along this line:
struct whitelist {
const char *prefix;
const char *suffix;
} whitelist[] = {
{ "credential.", NULL },
{ "http.", ".extraheader" },
};
for (i = 0; i < ARRAY_SIZE(whitelist); i++) {
struct whitelist *w = &whitelist[i];
if ((!w->prefix || starts_with(var, w->prefix)) &&
(!w->suffix || ends_with(var, w->suffix)))
return 1;
}
return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Stefan Beller writes: > So when going with that philosophy, the user might be missing > switches like > > -c-for-this-repo-only core.worktree=... -c > submodule.worktree=align-relative-to-parent > > i.e. when shifting the responsibility to the user, we need to have > switches to pass options to the repository or a subset of submodules? I think that is an excellent illlustration of the issue by an example of what we shouldn't do ;-) "git" is not always about submodules, so "-c-but-not-for-submodules" option does not belong to "git" wrapper. Users use "git -c" and hope to affect what happens in submodules, only because "git submodule" support is still immature and does not have options to do that. You certainly smell a linkage between "pass options to a selected subset of submodules" and your recent "give labels to submodules so that they can be named with *group syntax" topic, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
Junio C Hamano writes: > Users use "git -c" and hope to affect what happens in submodules, > only because "git submodule" support is still immature and does not > have options to do that. You certainly smell a linkage between > "pass options to a selected subset of submodules" and your recent > "give labels to submodules so that they can be named with *group > syntax" topic, no? By "options to do that", I meant "options to specify 'these configurations are passed down to part of this command that operates on these submodules'". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:52 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> So when going with that philosophy, the user might be missing >> switches like >> >> -c-for-this-repo-only core.worktree=... -c >> submodule.worktree=align-relative-to-parent >> >> i.e. when shifting the responsibility to the user, we need to have >> switches to pass options to the repository or a subset of submodules? > > I think that is an excellent illlustration of the issue by an > example of what we shouldn't do ;-) So rather have a white / black list? Could we have a pre curated list with the list easily changed by the user? So instead of screaming at the mailing list they can work around easily, and eventually someone fixes the "stupid" default? Maybe a user would want to do git config --global submodule-credentials-filter-white-list "host.*" instead? (Naming intentionally bad) > > "git" is not always about submodules, so "-c-but-not-for-submodules" > option does not belong to "git" wrapper. > > Users use "git -c" and hope to affect what happens in submodules, > only because "git submodule" support is still immature and does not > have options to do that. You certainly smell a linkage between > "pass options to a selected subset of submodules" and your recent > "give labels to submodules so that they can be named with *group > syntax" topic, no? I thought about that for a split second, but no. I mean it in the most general way possible. It doesn't even have to be a submodule related thing at all. I can imagine that `git gc` could learn to walk nested repos (not submodules, just repos on disk inside the work tree). And for that use case we'd maybe want to have a setting to pack the nested repos more aggressively than the toplevel repo. (Not sure if there would be a use case or such a thing, but it is the first I came up with) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 1/2] submodule init: fail gracefully with a missing .gitmodules file
When there is no .gitmodules file availabe to initialize a submodule
from, `submodule_from_path` just returns NULL. We need to check for
that and abort gracefully. When `submodule init` was implemented in shell,
a missing .gitmodules file would result in an error message
No url found for submodule path '%s' in .gitmodules
Replicate that error message for now.
When the .gitmodules file is missing we can probably fail even earlier
for all of the submodules with an improved error message.
Signed-off-by: Stefan Beller
---
applies on submodule-init.
Thanks,
Stefan
builtin/submodule--helper.c | 10 +++---
t/t7400-submodule-basic.sh | 8
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6d4f27..ce9d11e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -314,13 +314,17 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
- sub = submodule_from_path(null_sha1, path);
-
if (prefix) {
strbuf_addf(&sb, "%s%s", prefix, path);
displaypath = strbuf_detach(&sb, NULL);
} else
- displaypath = xstrdup(sub->path);
+ displaypath = xstrdup(path);
+
+ sub = submodule_from_path(null_sha1, path);
+
+ if (!sub)
+ die(_("No url found for submodule path '%s' in .gitmodules"),
+ displaypath);
/*
* Copy url setting when it is not set yet.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f99f674..df6b4da 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -18,6 +18,14 @@ test_expect_success 'setup - initial commit' '
git branch initial
'
+test_expect_success 'submodule init aborts on missing .gitmodules file' '
+ test_when_finished "git update-index --remove sub" &&
+ git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
+ # missing the .gitmodules file here
+ test_must_fail git submodule init 2>actual &&
+ test_i18ngrep "No url found for submodule path" actual
+'
+
test_expect_success 'configuration parsing' '
test_when_finished "rm -f .gitmodules" &&
cat >.gitmodules <<-\EOF &&
--
2.8.0.28.ga4e36c9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] submodule--helper update-clone: abort gracefully on missing .gitmodules
When there is no .gitmodules file availabe to initialize a submodule
from, `submodule_from_path` just returns NULL. We need to check for
that and abort gracefully.
When `git submodule update` was implemented in shell, this error out
with the warning
Submodule path '%s' not initialized
Maybe you want to use 'update --init'?
Replicate that behavior for now instead of crashing.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 38 +-
t/t7400-submodule-basic.sh | 8
2 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ce9d11e..5d05393 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -593,6 +593,25 @@ struct submodule_update_clone {
SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0}
+
+static void next_submodule_warn_missing(struct submodule_update_clone *suc,
+ struct strbuf *out, const char *displaypath)
+{
+ /*
+* Only mention uninitialized submodules when their
+* paths have been specified.
+*/
+ if (suc->warn_if_uninitialized) {
+ strbuf_addf(out,
+ _("Submodule path '%s' not initialized"),
+ displaypath);
+ strbuf_addch(out, '\n');
+ strbuf_addstr(out,
+ _("Maybe you want to use 'update --init'?"));
+ strbuf_addch(out, '\n');
+ }
+}
+
/**
* Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to
* run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise.
@@ -627,6 +646,11 @@ static int prepare_to_clone_next_submodule(const struct
cache_entry *ce,
else
displaypath = ce->name;
+ if (!sub) {
+ next_submodule_warn_missing(suc, out, displaypath);
+ goto cleanup;
+ }
+
if (suc->update.type == SM_UPDATE_NONE
|| (suc->update.type == SM_UPDATE_UNSPECIFIED
&& sub->update_strategy.type == SM_UPDATE_NONE)) {
@@ -644,19 +668,7 @@ static int prepare_to_clone_next_submodule(const struct
cache_entry *ce,
strbuf_addf(&sb, "submodule.%s.url", sub->name);
git_config_get_string(sb.buf, &url);
if (!url) {
- /*
-* Only mention uninitialized submodules when their
-* path have been specified
-*/
- if (suc->warn_if_uninitialized) {
- strbuf_addf(out,
- _("Submodule path '%s' not initialized"),
- displaypath);
- strbuf_addch(out, '\n');
- strbuf_addstr(out,
- _("Maybe you want to use 'update --init'?"));
- strbuf_addch(out, '\n');
- }
+ next_submodule_warn_missing(suc, out, displaypath);
goto cleanup;
}
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index df6b4da..814ee63 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -26,6 +26,14 @@ test_expect_success 'submodule init aborts on missing
.gitmodules file' '
test_i18ngrep "No url found for submodule path" actual
'
+test_expect_success 'submodule update aborts on missing .gitmodules file' '
+ test_when_finished "git update-index --remove sub" &&
+ git update-index --add --cacheinfo 16,$(git rev-parse HEAD),sub &&
+ # missing the .gitmodules file here
+ git submodule update sub 2>actual &&
+ test_i18ngrep "Submodule path .sub. not initialized" actual
+'
+
test_expect_success 'configuration parsing' '
test_when_finished "rm -f .gitmodules" &&
cat >.gitmodules <<-\EOF &&
--
2.8.0.28.ga4e36c9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] trailer: load config to handle core.commentChar
Fall throught git_default_config when reading config to update the
comment_line_char from default '#' to possible different value set in
core.commentChar.
Signed-off-by: Rafal Klys
---
Added fallthru instead of reading config third time.
Added test, updated commit message.
I even tried to change that to only one pass, but looks like it would require a
bit more coding, so maybe next time.
Thanks for feedback, I'm impressed that contributing to Git is so easy for
newbies (never sent a patch via email before!) and that the response is so
quick.
t/t7513-interpret-trailers.sh | 30 ++
trailer.c | 3 ++-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..e6f9d8e 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -267,6 +267,36 @@ test_expect_success 'with message that has comments' '
test_cmp expected actual
'
+test_expect_success 'with message that has comments using non-default
core.commentChar' '
+ git config core.commentChar x &&
+ test_when_finished "git config --unset core.commentChar" &&
+ cat basic_message >message_with_comments &&
+ sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
+ x comment
+
+ x other comment
+ Cc: Z
+ x yet another comment
+ Reviewed-by: Johan
+ Reviewed-by: Z
+ x last comment
+
+ EOF
+ cat basic_patch >>message_with_comments &&
+ cat basic_message >expected &&
+ cat >>expected <<-\EOF &&
+ x comment
+
+ Reviewed-by: Johan
+ Cc: Peff
+ x last comment
+
+ EOF
+ cat basic_patch >>expected &&
+ git interpret-trailers --trim-empty --trailer "Cc: Peff"
message_with_comments >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with message that has an old style conflict block' '
cat basic_message >message_with_comments &&
sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index 8e48a5c..2ec0883 100644
--- a/trailer.c
+++ b/trailer.c
@@ -483,7 +483,8 @@ static int git_trailer_default_config(const char *conf_key,
const char *value, v
const char *trailer_item, *variable_name;
if (!skip_prefix(conf_key, "trailer.", &trailer_item))
- return 0;
+ /* for core.commentChar */
+ return git_default_config(conf_key, value, cb);
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
--
2.8.1.69.g6de72cc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixed grammar mistake in the french localization
Jean-Noël AVILA writes: > Cherry-picked, fixed and PR to Jiang Xin > Thanks > JN > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
David Turner writes:
> Since there is a manual check for that case, the code will fail at test
> time.
I see a difference between "We make it hard to write incorrect code"
and "We keep it easy to write incorrect code but a runtime check
will catch mistakes anyway", and I tend to prefer the former.
I do think the "manual check" needs to be kept in an adjusted form
even if we decide to take the suggested update. A caller can pass
0x04 instead of REF_ISPRUNING after all---but that is exactly the
case where you have to work hard to write incorrect code.
One existing check that is not touched with 15/29 also needs to be
adjusted. An incremental update relative to 15/29 would look like
this:
refs.c | 4 ++--
refs/files-backend.c | 4 ++--
refs/refs-internal.h | 6 --
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 5dc2473..bb81dfd 100644
--- a/refs.c
+++ b/refs.c
@@ -790,8 +790,8 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update called for transaction that is not open");
- if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
- die("BUG: REF_ISPRUNING set without REF_NODEREF");
+ if ((flags & REF_ISPRUNING_) && !(flags & REF_NODEREF))
+ die("BUG: REF_ISPRUNING_ set without REF_NODEREF");
if (new_sha1 && !is_null_sha1(new_sha1) &&
check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..012e3d0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
- REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
+ REF_ISPRUNING, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
error("%s", err.buf);
@@ -3178,7 +3178,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
goto cleanup;
}
- if (!(update->flags & REF_ISPRUNING))
+ if (!(update->flags & REF_ISPRUNING_))
string_list_append(&refs_to_delete,
update->lock->ref_name);
}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..0a2bf1f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,9 +15,11 @@
/*
* Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned. As this must always be done with REF_NODEREF, we make
+ * the public version REF_ISPRUNING to contain REF_NODEREF.
*/
-#define REF_ISPRUNING 0x04
+#define REF_ISPRUNING_ 0x04
+#define REF_ISPRUNING (REF_ISPRUNING_ | REF_NODEREF)
/*
* Used as a flag in ref_update::flags when the reference should be
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'
Christian Couder writes: >> I do not think you need to think about "free"ing. > > Yeah, lockfile.h says: > ... > and: > ... Yup, we are now on the same page. >> Even if the libified version of the apply internal can be called >> multiple times to process multiple patch inputs, there is no need to >> run multiple instances of it yet. And a lockfile, after the caller >> finishes interacting with one file using it by calling commit or >> rollback, can be reused to interact with another file. lockfile.h says this about the above paragraph, which is a more important part ;-) * When finished writing, the caller can: * ... * Even after the lockfile is committed or rolled back, the * `lock_file` object must not be freed or altered by the caller. * However, it may be reused; just pass it to another call of * `hold_lock_file_for_update()`. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:28:21PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > It's definitely sufficient, it's just annoying if a user shows up every
> > week and says "I want X.Y", and then somebody else shows up a week later
> > and says "I want X.Z".
> >
> > Are we serving any purpose in vetting each one (and if so, what)?
>
> Personally I do not think we would need to filter _anything_ if we
> can tell that the user directly said
>
> git -c var1=val1 -c var2=val2 $cmd ...
>
> and "git $cmd" ended up needing to spawn another "git" subcommand,
> possibly in some other repository (i.e. "$cmd" in this case is
> likely to be "submodule", but in principle it does not have to be).
> If the user somehow gives variables like core.worktree that are
> inappropriate to be applied across repositories, that's user's
> problem, i.e. "don't do it then if it hurts".
Right, we are talking about that direct case here. And any time our
filter heuristic lets something through, it is probably "if it hurts
don't do it" as the worst case.
So I think the only two cases worth filtering are:
1. Ones where we _know_ that the config is nonsense to pass along,
_and_ where a user might conceivably make use of the
just-the-top-level version of it (core.worktree
comes to mind, though of course they are probably better served by
"--work-tree" in such a case).
2. An option where we think there may be some security implication.
Setting "http.sslverify" to false does have some security
implications ("oops, I only meant to turn off verification for the
root repo, and I got MiTM-attacked for the submodules!"). But it's
so obscure and unlikely that I think the benefit outweighs it.
And I can't think of any other cases whose security implications
aren't similarly unlikely. But I haven't carefully gone down the
list (and as I said, I'd be hesitant to support a blacklist until
_somebody_ takes the time to do so).
> If we are doing any filtering, however, it is always hard, if not
> impossible, to take away what we originally granted, even by
> mistake, for any reason, even for correctness or for security, in a
> later release.
Yep, agreed.
I am OK staying with a whitelist. But I think we should be fairly
lenient in whitelisting hierarchies that people have a use for, and
which do not violate (1) or (2) above.
> We probably could sidestep it by introducing an end-user
> configurable "whitelist" somewhere.
Ugh. Please no. I do not want to have to think about explaining to
somebody that they can accomplish what they want with submodules, but
only by pre-configuring their ~/.gitconfig to allow certain keys so that
they can pass the appropriate config on the command line.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings
On Thu, Apr 28, 2016 at 12:52:03PM -0700, Junio C Hamano wrote: > "git" is not always about submodules, so "-c-but-not-for-submodules" > option does not belong to "git" wrapper. > > Users use "git -c" and hope to affect what happens in submodules, > only because "git submodule" support is still immature and does not > have options to do that. You certainly smell a linkage between > "pass options to a selected subset of submodules" and your recent > "give labels to submodules so that they can be named with *group > syntax" topic, no? Keep in mind that submodule interactions may be triggered from other non-submodule commands. So "git fetch", for instance, may end up caring about whether you pass "http.*" or "credential.*" down to the submodules. I do not think "fetch" should grow submodule-specific options, so that pretty much leaves "git" options as the only place left. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
