* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-05 20:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161201235856.GL54082@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 12/01, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > I started taking a look at your http redirect series (I really should
>> > have taking a look at it sooner) and I see exactly what you're talking
>> > about. We can easily move this logic into a function to make it easier
>> > to generate the two whitelists.
>>
>> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
>> expect that a future reroll would make things better.
>
> Yeah I expected we would see an issue since both these series collide in
> http.c
>
> I'm sending out another reroll of this series so that in Jeff's he can
> just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> option, which should make this test stop barfing.
I was hoping to eventually merge Peff's series to older maintenance
tracks. How bad would it be if we rebased the v8 of this series
together with Peff's series to say v2.9 (or even older if it does
not look too bad)?
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Stefan Beller @ 2016-12-05 19:57 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <1480964316-99305-2-git-send-email-bmwill@google.com>
On Mon, Dec 5, 2016 at 10:58 AM, Brandon Williams <bmwill@google.com> wrote:
> The current implementation of real_path uses chdir() in order to resolve
> symlinks. Unfortunately this isn't thread-safe as chdir() affects a
> process as a whole and not just an individual thread. Instead perform
> the symlink resolution by hand so that the calls to chdir() can be
> removed, making real_path reentrant.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
Thanks for working on this, some comments below:
>
> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> + if (path->len > 1) {
> + char *last_slash = find_last_dir_sep(path->buf);
What happens when there is no dir_sep?
> +/* gets the next component in 'remaining' and places it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
It's more than just getting it, it also chops it off of 'remaining' ?
> + strbuf_reset(&resolved);
> +
> + if (is_absolute_path(path)) {
> + /* absolute path; start with only root as being resolved */
> + strbuf_addch(&resolved, '/');
> + strbuf_addstr(&remaining, path + 1);
This is where we would wait for input of Windows savy people.
> + } else {
> + /* relative path; can use CWD as the initial resolved path */
> + if (strbuf_getcwd(&resolved)) {
> + if (die_on_error)
> + die_errno("Could not get current working directory");
I am looking at xgetcwd, which words it slightly differently.
if (strbuf_getcwd(&sb))
die_errno(_("unable to get current working directory"));
Not sure if aligning them would be a good idea?
Going by "git grep die_errno" as well as our Coding guidelines,
we don't want to see capitalized error messages.
> +
> + if (next.len == 0) {
> + continue; /* empty component */
which means we resolve over path//with//double//slashes just fine,
as well as /./ parts. :)
> }
>
> - if (sb.len) {
> - if (!cwd.len && strbuf_getcwd(&cwd)) {
> + /* append the next component and resolve resultant path */
"resultant" indicates you have a math background. :)
But I had to look it up, I guess it is fine that way,
though "resulting" may cause less mental friction
for non native speakers.
> + if (!(errno == ENOENT && !remaining.len)) {
> if (die_on_error)
> - die_errno("Could not get current working directory");
> + die_errno("Invalid path '%s'",
> + resolved.buf);
> else
> goto error_out;
> }
> + } else if (S_ISLNK(st.st_mode)) {
As far as I can tell, we could keep the symlink strbuf
at a smaller scope here? (I was surprised how many strbufs
are declared at the beginning of the function)
> + //strbuf_release(&resolved);
This is why the cover letter toned down expectations ?
(no // as comment, maybe remove that line?)
Thanks,
Stefan
^ permalink raw reply
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Brandon Williams @ 2016-12-05 19:31 UTC (permalink / raw)
To: Stefan Beller
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <CAGZ79kbEtR7_6ZvBsjkc=8q+nnq9FoPv9HNWdRyuR2CQGqQ2oA@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> On Mon, Dec 5, 2016 at 11:25 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/02, Stefan Beller wrote:
> >>
> >> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
> >> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
> >> ! test -s actual
> >> '
> >
> > Should you use test_must_fail and not '!'?
>
> We use test_must_fail for git and '!' for non git thigns (test, grep etc),
> as the test suite is about testing git.
>
> The test_must_fail expects the command to be run to
> * not reurn 0 (success)
> * not segfault
> * not return some other arbitrary return codes
> indicating abnormal failure (125 IIRC)
>
> So in a way test_must_fail translates to:
> "I want to run this git command and it should fail
> gracefully because at this state, it is the best git can do"
>
> The '!' however is just inverting the boolean expression.
> We assume test, grep, et al. to be flawless here. ;)
Ah, alright. Thanks for the info :)
--
Brandon Williams
^ permalink raw reply
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Stefan Beller @ 2016-12-05 19:30 UTC (permalink / raw)
To: Brandon Williams
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <20161205192527.GA68588@google.com>
On Mon, Dec 5, 2016 at 11:25 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/02, Stefan Beller wrote:
>>
>> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
>> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
>> ! test -s actual
>> '
>
> Should you use test_must_fail and not '!'?
We use test_must_fail for git and '!' for non git thigns (test, grep etc),
as the test suite is about testing git.
The test_must_fail expects the command to be run to
* not reurn 0 (success)
* not segfault
* not return some other arbitrary return codes
indicating abnormal failure (125 IIRC)
So in a way test_must_fail translates to:
"I want to run this git command and it should fail
gracefully because at this state, it is the best git can do"
The '!' however is just inverting the boolean expression.
We assume test, grep, et al. to be flawless here. ;)
Stefan
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Brandon Williams @ 2016-12-05 19:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: David.Turner, git, sandals, hvoigt, gitster
In-Reply-To: <20161203003022.29797-18-sbeller@google.com>
On 12/02, Stefan Beller wrote:
> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
> + test_config checkout.recurseSubmodules 1 &&
> + git checkout base &&
> + git checkout -b advanced-base &&
> + git -C submodule commit --allow-empty -m "empty commit" &&
> + git add submodule &&
> + git commit -m "advance submodule" &&
> + git checkout base &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached base &&
> + git checkout advanced-base &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached advanced-base &&
> + git checkout --recurse-submodules base
> +'
> +
This test doesn't look like it looks into the submodule to see if the
submodule has indeed changed. Unless diff-index and diff-files recurse
into the submodules?
--
Brandon Williams
^ permalink raw reply
* Re: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: Brandon Williams @ 2016-12-05 19:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: David.Turner, git, sandals, hvoigt, gitster
In-Reply-To: <20161203003022.29797-16-sbeller@google.com>
On 12/02, Stefan Beller wrote:
>
> test_expect_success '"checkout <submodule>" honors diff.ignoreSubmodules' '
> @@ -63,6 +70,260 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
> ! test -s actual
> '
Should you use test_must_fail and not '!'?
--
Brandon Williams
^ permalink raw reply
* [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-05 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, jacob.keller
In-Reply-To: <1480964316-99305-1-git-send-email-bmwill@google.com>
The current implementation of real_path uses chdir() in order to resolve
symlinks. Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread. Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 122 insertions(+), 61 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2825de8..6f546e0 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,38 @@ int is_directory(const char *path)
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+ if (path->len > 1) {
+ char *last_slash = find_last_dir_sep(path->buf);
+ strbuf_setlen(path, last_slash - path->buf);
+ }
+}
+
+/* gets the next component in 'remaining' and places it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+ char *start = NULL;
+ char *end = NULL;
+
+ strbuf_reset(next);
+
+ /* look for the next component */
+ /* Skip sequences of multiple path-separators */
+ for (start = remaining->buf; is_dir_sep(*start); start++)
+ /* nothing */;
+ /* Find end of the path component */
+ for (end = start; *end && !is_dir_sep(*end); end++)
+ /* nothing */;
+
+ strbuf_add(next, start, end - start);
+ /* remove the component from 'remaining' */
+ strbuf_remove(remaining, 0, end - remaining->buf);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#define MAXSYMLINKS 5
/*
* Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)
* absolute_path().) The return value is a pointer to a static
* buffer.
*
- * The input and all intermediate paths must be shorter than MAX_PATH.
* The directory part of path (i.e., everything up to the last
* dir_sep) must denote a valid, existing directory, but the last
* component need not exist. If die_on_error is set, then die with an
@@ -33,22 +62,16 @@ int is_directory(const char *path)
*/
static const char *real_path_internal(const char *path, int die_on_error)
{
- static struct strbuf sb = STRBUF_INIT;
+ static struct strbuf resolved = STRBUF_INIT;
+ struct strbuf remaining = STRBUF_INIT;
+ struct strbuf next = STRBUF_INIT;
+ struct strbuf symlink = STRBUF_INIT;
char *retval = NULL;
-
- /*
- * If we have to temporarily chdir(), store the original CWD
- * here so that we can chdir() back to it at the end of the
- * function:
- */
- struct strbuf cwd = STRBUF_INIT;
-
- int depth = MAXDEPTH;
- char *last_elem = NULL;
+ int num_symlinks = 0;
struct stat st;
/* We've already done it */
- if (path == sb.buf)
+ if (path == resolved.buf)
return path;
if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&sb);
- strbuf_addstr(&sb, path);
-
- while (depth--) {
- if (!is_directory(sb.buf)) {
- char *last_slash = find_last_dir_sep(sb.buf);
- if (last_slash) {
- last_elem = xstrdup(last_slash + 1);
- strbuf_setlen(&sb, last_slash - sb.buf + 1);
- } else {
- last_elem = xmemdupz(sb.buf, sb.len);
- strbuf_reset(&sb);
- }
+ strbuf_reset(&resolved);
+
+ if (is_absolute_path(path)) {
+ /* absolute path; start with only root as being resolved */
+ strbuf_addch(&resolved, '/');
+ strbuf_addstr(&remaining, path + 1);
+ } else {
+ /* relative path; can use CWD as the initial resolved path */
+ if (strbuf_getcwd(&resolved)) {
+ if (die_on_error)
+ die_errno("Could not get current working directory");
+ else
+ goto error_out;
+ }
+ strbuf_addstr(&remaining, path);
+ }
+
+ /* Iterate over the remaining path components */
+ while (remaining.len > 0) {
+ get_next_component(&next, &remaining);
+
+ if (next.len == 0) {
+ continue; /* empty component */
+ } else if (next.len == 1 && !strcmp(next.buf, ".")) {
+ continue; /* '.' component */
+ } else if (next.len == 2 && !strcmp(next.buf, "..")) {
+ /* '..' component; strip the last path component */
+ strip_last_component(&resolved);
+ continue;
}
- if (sb.len) {
- if (!cwd.len && strbuf_getcwd(&cwd)) {
+ /* append the next component and resolve resultant path */
+ if (resolved.buf[resolved.len - 1] != '/')
+ strbuf_addch(&resolved, '/');
+ strbuf_addbuf(&resolved, &next);
+
+ if (lstat(resolved.buf, &st)) {
+ /* error out unless this was the last component */
+ if (!(errno == ENOENT && !remaining.len)) {
if (die_on_error)
- die_errno("Could not get current working directory");
+ die_errno("Invalid path '%s'",
+ resolved.buf);
else
goto error_out;
}
+ } else if (S_ISLNK(st.st_mode)) {
+ ssize_t len;
+ strbuf_reset(&symlink);
- if (chdir(sb.buf)) {
+ if (num_symlinks++ > MAXSYMLINKS) {
if (die_on_error)
- die_errno("Could not switch to '%s'",
- sb.buf);
+ die("More than %d nested symlinks "
+ "on path '%s'", MAXSYMLINKS, path);
else
goto error_out;
}
- }
- if (strbuf_getcwd(&sb)) {
- if (die_on_error)
- die_errno("Could not get current working directory");
- else
- goto error_out;
- }
- if (last_elem) {
- if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
- strbuf_addch(&sb, '/');
- strbuf_addstr(&sb, last_elem);
- free(last_elem);
- last_elem = NULL;
- }
-
- if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
- struct strbuf next_sb = STRBUF_INIT;
- ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
+ len = strbuf_readlink(&symlink, resolved.buf,
+ st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- sb.buf);
+ resolved.buf);
else
goto error_out;
}
- strbuf_swap(&sb, &next_sb);
- strbuf_release(&next_sb);
- } else
- break;
+
+ if (is_absolute_path(symlink.buf)) {
+ /*
+ * absolute symlink
+ * reset resolved path to root
+ */
+ strbuf_setlen(&resolved, 1);
+ } else {
+ /*
+ * relative symlink
+ * strip off the last component since it will
+ * be replaced with the contents of the symlink
+ */
+ strip_last_component(&resolved);
+ }
+
+ /*
+ * if there are still remaining components to resolve
+ * then append them to symlink
+ */
+ if (remaining.len) {
+ strbuf_addch(&symlink, '/');
+ strbuf_addbuf(&symlink, &remaining);
+ }
+
+ /*
+ * use the symlink as the remaining components that
+ * need to be resloved
+ */
+ strbuf_swap(&symlink, &remaining);
+ }
}
- retval = sb.buf;
+ retval = resolved.buf;
+
error_out:
- free(last_elem);
- if (cwd.len && chdir(cwd.buf))
- die_errno("Could not change back to '%s'", cwd.buf);
- strbuf_release(&cwd);
+ //strbuf_release(&resolved);
+ strbuf_release(&remaining);
+ strbuf_release(&next);
+ strbuf_release(&symlink);
return retval;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH] making real_path thread-safe
From: Brandon Williams @ 2016-12-05 18:58 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, jacob.keller
The goal of this patch is to make the function real_path thread-safe. To do,
the current logic which uses chdir() to do the symlink resolution needed to be
rewritten to perform the symlink resolution by hand.
I wanted to get the bulk of the work out there for review quickly so
technically this patch doesn't make real_path thread safe yet as it still uses
a static buffer to serve up the resultant path. On a future iteration I plan
on removing this and pushing ownership of the returned string to the callers.
For more information on why this change is desirable you can see the following
threads:
https://public-inbox.org/git/20161129010538.GA121643@google.com/
https://public-inbox.org/git/1480555714-186183-1-git-send-email-bmwill@google.com/
Brandon Williams (1):
real_path: make real_path thread-safe
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 122 insertions(+), 61 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Junio C Hamano @ 2016-12-05 18:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612051126320.117539@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Seriously, do you really think it is a good idea to have
> git_config_get_value() *ignore* any value in .git/config?
When we do not know ".git/" directory we see is the repository we
were told to work on, then we should ignore ".git/config" file. So
allowing git_config_get_value() to _ignore_ ".git/config" before the
program calls setup_git_directory() does have its uses.
But I agree that "difftool.useBuiltin" that flips between two
implementations is a different use case than the above. Both
implementations eventually want to work on ".git/" and would
want to read from ".git/config".
> We need to fix this.
I have a feeling that "environment modifications that cannot be
undone" we used as the rationale in 73c2779f42 ("builtin-am:
implement skeletal builtin am", 2015-08-04) might be overly
pessimistic and in order to implement undo_setup_git_directory(),
all we need to do may just be matter of doing a chdir(2) back to
prefix and unsetting GIT_PREFIX environment, but I haven't looked
into details of the setup sequence recently.
^ permalink raw reply
* Re: [BUG] git gui can't commit multiple files
From: Timon @ 2016-12-05 18:36 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <20161204204057.32dnkjx6ixv3swez@gmail.com>
On 12/4/16, David Aguilar <davvid@gmail.com> wrote:
> On Sun, Dec 04, 2016 at 05:36:46PM +0100, Timon wrote:
>> This is a regression in git 2.11.0 (version 2.10.2 is fine).
>>
>> In git-gui I select multiple files in the Unstaged Changes (using
>> shift+click) and press ctrl+t to stage them. Then only one files gets
>> staged instead of all of the selected files.
>> The same happens when unstaging files.
>>
>> Git-cola also exhibits the same behavior. Although there I could stage
>> multiple files if I used a popup menu instead of the keyboard shortcut
>> (I'm guessing it goes through a different code path?).
>
> Can you elaborate a bit?
>
> I just tested git-cola with Git 2.11 and it worked fine for me.
> I selected several files and used the Ctrl+s hotkey to stage the
> selected files. They all got staged.
>
> If you have a test repo, or reproduction recipe, I'd be curious
> to try it out.
> --
> David
>
Can you try with git gui?
Though I guess it's probably specific to my distro or configuration.
I'm running 64bit gentoo with:
linux 4.8.12
gcc 5.4.0
glibc 2.23-r3
tk 8.6.6
gettext 0.19.8.1
openssl 1.0.2j
Not sure if that's helpful though.
^ permalink raw reply
* Re: [PATCH] clone,fetch: explain the shallow-clone option a little more clearly
From: Junio C Hamano @ 2016-12-05 18:28 UTC (permalink / raw)
To: Alex Henrie; +Cc: pclouds, git
In-Reply-To: <20161204220359.30807-1-alexhenrie24@gmail.com>
Alex Henrie <alexhenrie24@gmail.com> writes:
> "deepen by excluding" does not make sense because excluding a revision
> does not deepen a repository; it makes the repository more shallow.
I think that an intuitive way the feature should work may be:
- You started with "git fetch --depth=20" and then later say "I
have only a very short segment of the recent history, but I want
a history that dates back to v1.0" with "--shallow-exclude=v1.0".
In this case, you would be deepening.
- You instead started with "git fetch --depth=20000" that dated
back to v0.5. "--shallow-exclude=v1.0" you say today would mean
"I have very old cruft I no longer look at. I just want my
history lead back to v1.0 and no earlier". In such a case, you
indeed would be making the repository shallower.
I however offhand do not think the feature can be used to make the
repository shallower, and I agree your changes to the usage string
probably describe what the option does more correctly.
I however suspect that the feature is simply buggy and it would
eventually want to allow to shorten the history as well. At that
point we may want to work on the verb 'deepen' there, too.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> builtin/clone.c | 2 +-
> builtin/fetch.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 6c76a6e..e3cb808 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -99,7 +99,7 @@ static struct option builtin_clone_options[] = {
> OPT_STRING(0, "shallow-since", &option_since, N_("time"),
> N_("create a shallow clone since a specific time")),
> OPT_STRING_LIST(0, "shallow-exclude", &option_not, N_("revision"),
> - N_("deepen history of shallow clone by excluding rev")),
> + N_("deepen history of shallow clone, excluding rev")),
> OPT_BOOL(0, "single-branch", &option_single_branch,
> N_("clone only one branch, HEAD or --branch")),
> OPT_BOOL(0, "shallow-submodules", &option_shallow_submodules,
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b6a5597..fc74c84 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -122,7 +122,7 @@ static struct option builtin_fetch_options[] = {
> OPT_STRING(0, "shallow-since", &deepen_since, N_("time"),
> N_("deepen history of shallow repository based on time")),
> OPT_STRING_LIST(0, "shallow-exclude", &deepen_not, N_("revision"),
> - N_("deepen history of shallow clone by excluding rev")),
> + N_("deepen history of shallow clone, excluding rev")),
> OPT_INTEGER(0, "deepen", &deepen_relative,
> N_("deepen history of shallow clone")),
> { OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-05 18:10 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <ab1e7ce9-1022-0c72-2f72-63e3b9182bc9@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Heh, that was the first version of the patch. However, I got to thinking
> about why --abbrev=7 was there in the first place; the only reason I
> could think of was to defeat local configuration to get a measure of
> reproducibility.
>
> Unfortunately, you can't get the 'auto' behaviour from --abbrev
> (on the pu branch):
>
> $ ./git describe --abbrev=-1
> v2.11.0-286-g109e8
> $ ./git describe --abbrev=0
> v2.11.0
> $ ./git describe
> v2.11.0-286-g109e8a99d
> $
What is the reason why the last one is undesirable? Is it because
the user may have core.abbrev set to some value in the configuration
and you want to override it to force "auto"?
I am not sure how rigid GIT-VERSION-GEN wants to be to countermand
such an explicit user preference (i.e. existing configuration).
> I did think about using '-c core.abbrev=auto',
Having said that, if countermanding end-user's configuration is
desireble, I agree that "-c core.abbrev=auto" is the way to do so.
> but that would depend on Junio's patch (nothing wrong with that,
> of course):
You caught me. I'll need to polish that into a usable shape soon
then. And that is orthogonal to the "does it make sense to force
'auto' in this context?" question.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-05 18:01 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <20161205053258.jtnqq64gp5n7vtni@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>
>> Hi Junio,
>>
>> I recently noticed that:
>>
>> $ make >pout 2>&1
>> $ ./git version
>> git version 2.11.0.286.g109e8a9
>> $ git describe
>> v2.11.0-286-g109e8a99d
>> $
>>
>> ... for non-release builds, the commit part of the version
>> string was still using an --abbrev=7.
>
> It seems like this kind of discussion ought to go in the commit message.
> :)
>
> That said, I think the right patch may be to just drop --abbrev
> entirely.
> ...
> I think at that point it was a noop, as 7 should have been the default.
> And now we probably ought to drop it, so that we can use the
> auto-scaling default.
Yeah, I agree.
It does mean that snapshot binaries built out of the same commit in
the same repository before and after a repack have higher chances of
getting named differently, which may surprise people, but that
already is possible with a fixed length if the repacking involves
pruning (albeit with lower probabilities), and I do not think it is
a problem.
^ permalink raw reply
* Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values
From: Junio C Hamano @ 2016-12-05 17:49 UTC (permalink / raw)
To: Jeff King; +Cc: larsxschneider, git
In-Reply-To: <20161205131219.kdq4zafith26vro2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> A pathname value in a clean/smudge filter process "key=value" pair can
>> contain the '=' character (introduced in edcc858). Make the user aware
>> of this issue in the docs, add a corresponding test case, and fix the
>> issue in filter process value parser of the example implementation in
>> contrib.
>
> Yeah, I just naturally assumed it would work this way, as it's pretty
> standard in our other key=value protocols. But certainly it's reasonable
> to document it (and to keep the t0021 filter as a good example).
There are diplomatic ways and other ways to say the same thing, it
seems, and yours is almost always more diplomatic than mine ;-)
My knee-jerk reaction last night that I didn't send was "I would
understand if the new test were about quotes or whitespaces, but is
it even useful to make sure that the value can have an equal sign in
it?"
^ permalink raw reply
* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Jack Bates @ 2016-12-05 17:45 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net>
On 05/12/16 12:26 AM, Jeff King wrote:
> On Sun, Dec 04, 2016 at 11:19:46PM -0800, Junio C Hamano wrote:
>>> - if (no_index)
>>> + if (no_index) {
>>> /* If this is a no-index diff, just run it and exit there. */
>>> + startup_info->have_repository = 0;
>>> diff_no_index(&rev, argc, argv);
>>> + }
>>
>> This kind of change makes me nervous (partly because I am not seeing
>> the whole code but only this part of the patch).
>>
>> Some code may react to "have_repository" being zero and do the right
>> thing (which I think is what you are using from your previous "we
>> did one of the three cases" change here), but the codepath that led
>> to "have_repository" being set to non-zero previously must have done
>> a lot more than just flipping that field to non-zero, and setting
>> zero to this field alone would not "undo" what it did.
>
> I _think_ it's OK because the only substantive change would be the
> chdir() to the top of the working tree. But that information is carried
> through by revs->prefix, which we act on regardless of the value of
> startup_info->have_repository when we call prefix_filename().
>
> I agree that it may be an accident waiting to happen, though, as soon as
> some buried sub-function needs to care about the distinction.
>
>> I wonder if we're better off if we made sure that diff_no_index()
>> works the same way regardless of the value of "have_repository"
>> field?
>
> If you mean adding a diffopt flag like "just abbreviate everything to
> FALLBACK_DEFAULT_ABBREV even if we're in a repository", and then setting
> that in diff_no_index(), I agree that is a lot cleaner.
>
> I'm still not 100% convinced that it's actually the correct behavior,
> but at least doing a more contained version wouldn't take away other
> functionality like reading config.
I don't have a strong reason for wanting these three cases to behave
identically, I was merely surprised that they don't. I think you
expected them to behave the same as well? I'll withdraw this patch.
Conceptually I do think of "git diff" as having two separate modes, "in
repository" and "out of repository", with the --no-index option forcing
the "out of repository" mode. But maybe there are good reasons why this
isn't accurate, or maybe it doesn't matter that it's not 100% accurate.
In summary, currently all of the three cases are "no index" but only the
first case doesn't "have repository".
Thank you for your thoughtful feedback!
^ permalink raw reply
* Feature request: Set git svn options in .git/config file
From: Juergen Kosel @ 2016-12-05 17:34 UTC (permalink / raw)
To: git
Hello,
while working with a git-svn repository, I like to use the command
line options --use-log-author and --add-author-from for all calls of
git svn fetch,
git svn rebase,
git svn dcommit, ...
Doing so consequently, the commit-ids of each git repository, which
has been cloned from the same svn-repository match.
Unfortunately, it is possible to forget these options on the command
line. (And 2nd, tortoise-git does not surport it, see
https://gitlab.com/tortoisegit/tortoisegit/issues/2824 ).
Therefore I believe, that it would be the best solution to store the
settings of --add-author-from, --use-log-author and maybe
--authors-prog in the .git/config file.
Greetings
Juergen
^ permalink raw reply
* Bug: stash staged file move loses original file deletion
From: Matthew Patey @ 2016-12-05 14:37 UTC (permalink / raw)
To: git
Git version 2.8.0 (sorry can't update to more recent on my machine) on Ubuntu.
After moving a file, if the new file is in the index but the deletion
of the old file is not staged, git stash loses the deletion operation.
Repro:
1. git mv a b
This will have both the "deletion" and the "file added" in the index
2. git stash; git stash pop
Now file a is still in the index, but file b deletion is not staged.
3. git stash; git stash show -v
This will show that the deletion operation is not in the stash
4. git stash pop
Again confirms the issue, file a is in the index, but file b is
present and unmodified in the working directory.
^ permalink raw reply
* [ANNOUNCE] Git Merge Contributor's Summit Feb 2, 2017, Brussels
From: Jeff King @ 2016-12-05 13:35 UTC (permalink / raw)
To: git
Git Merge 2017 is happening on Feb 3rd; there will be a Contributor's
Summit the day before. The details are largely what I wrote earlier in
[1], but here's a recap:
When: Thursday, Feb 2, 2017. 10am-3pm.
Where: The Egg[2], Brussels, Belgium
What: Git nerds having a round-table discussion
Who: All contributors to git or related projects in the git ecosystem
are invited; if you're not sure if you qualify, just ask!
Registration for the conference is open, and you need to register
specifically for the Contributor's Summit to attend. There's a special
code for registering for the Contributor's Summit; email me off-list to
get instructions. It works either when registering for the main
conference, or separately. You don't have to attend the main conference
on Friday to come to the Contributor's Summit, but I encourage you to.
As I mentioned before, the content is up to us. So start thinking of
things you'd like to discuss. We can probably organize topics on the
kernel.org wiki (if anybody really wants to start things going, feel
free to start a wiki page).
Let me know if you have any questions.
-Peff
[1] http://public-inbox.org/git/20161025162829.jcy6fmnmdjual6io@sigill.intra.peff.net/
[2] https://www.google.com/maps/place/The+Egg/@50.8357424,4.321425,15z/data=!4m13!1m7!3m6!1s0x0:0xe028c6680611d1da!2sThe+Egg!3b1!8m2!3d50.8331946!4d4.3270469!3m4!1s0x0:0xe028c6680611d1da!8m2!3d50.8331946!4d4.3270469
^ permalink raw reply
* Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
From: Jeff King @ 2016-12-05 13:23 UTC (permalink / raw)
To: Lars Schneider
Cc: Christian Couder, git, Junio C Hamano, Nguyen Thai Ngoc Duy,
Mike Hommey, Eric Wong, Christian Couder
In-Reply-To: <A5ABBF3E-BED9-4FF3-9DE5-B529DEF0B8E8@gmail.com>
On Sat, Dec 03, 2016 at 07:47:51PM +0100, Lars Schneider wrote:
> > - "<command> have": the command should output the sha1, size and
> > type of all the objects the external ODB contains, one object per
> > line.
>
> This looks impractical. If a repo has 10k external files with
> 100 versions each then you need to read/transfer 1m hashes (this is
> not made up - I am working with Git repos than contain >>10k files
> in GitLFS).
Are you worried about the client-to-server communication, or the pipe
between git and the helper? I had imagined that the client-to-server
communication happen infrequently and be cached.
But 1m hashes is 20MB, which is still a lot to dump over the pipe.
Another option is that Git defines a simple on-disk data structure
(e.g., a flat file of sorted 20-byte binary sha1s), and occasionally
asks the filter "please update your on-disk index".
That still leaves open the question of how the external odb script
efficiently gets updates from the server. It can use an ETag or similar
to avoid downloading an identical copy, but if one hash is added, we'd
want to know that efficiently. That is technically outside the scope of
the git<->external-odb interface, but obviously it's related. The design
of the on-disk format might be make that problem easier or harder on the
external-odb script.
> Wouldn't it be better if Git collects all hashes that it currently
> needs and then asks the external ODBs if they have them?
I think you're going to run into latency problems when git wants to ask
"do we have this object" and expects the answer to be no. You wouldn't
want a network request for each.
And I think it would be quite complex to teach all operations to work on
a promise-like system where the answer to "do we have it" might be
"maybe; check back after you've figured out the whole batch of hashes
you're interested in".
> > - "<command> get <sha1>": the command should then read from the
> > external ODB the content of the object corresponding to <sha1> and
> > output it on stdout.
> >
> > - "<command> put <sha1> <size> <type>": the command should then read
> > from stdin an object and store it in the external ODB.
>
> Based on my experience with Git clean/smudge filters I think this kind
> of single shot protocol will be a performance bottleneck as soon as
> people store more than >1000 files in the external ODB.
> Maybe you can reuse my "filter process protocol" (edcc858) here?
Yeah. This interface comes from my original proposal, which used the
rationale "well, the files are big, so process startup shouldn't be a
big deal". And I don't think I wrote it down, but an implicit rationale
was "it seems to work for LFS, so it should work here too". But of
course LFS hit scaling problems, and so would this. It was one of the
reasons I was interested in making sure your filter protocol could be
used as a generic template, and I think we would want to do something
similar here.
> > * Transfer
> >
> > To tranfer information about the blobs stored in external ODB, some
> > special refs, called "odb ref", similar as replace refs, are used.
> >
> > For now there should be one odb ref per blob. Each ref name should be
> > refs/odbs/<odbname>/<sha1> where <sha1> is the sha1 of the blob stored
> > in the external odb named <odbname>.
> >
> > These odb refs should all point to a blob that should be stored in the
> > Git repository and contain information about the blob stored in the
> > external odb. This information can be specific to the external odb.
> > The repos can then share this information using commands like:
> >
> > `git fetch origin "refs/odbs/<odbname>/*:refs/odbs/<odbname>/*"`
I'd worry about scaling this part. Traditionally our refs storage does
not scale very well.
-Peff
^ permalink raw reply
* Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values
From: Jeff King @ 2016-12-05 13:12 UTC (permalink / raw)
To: larsxschneider; +Cc: git
In-Reply-To: <20161203194516.12879-1-larsxschneider@gmail.com>
On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> A pathname value in a clean/smudge filter process "key=value" pair can
> contain the '=' character (introduced in edcc858). Make the user aware
> of this issue in the docs, add a corresponding test case, and fix the
> issue in filter process value parser of the example implementation in
> contrib.
Yeah, I just naturally assumed it would work this way, as it's pretty
standard in our other key=value protocols. But certainly it's reasonable
to document it (and to keep the t0021 filter as a good example).
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] restricting http redirects
From: Jeff King @ 2016-12-05 13:08 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
On Thu, Dec 01, 2016 at 04:03:37AM -0500, Jeff King wrote:
> Jann Horn brought up on the git-security list some interesting
> social-engineering attacks around the way Git handles HTTP redirects.
> These patches are my attempt to harden our redirect handling against
> these attacks.
There's one other possible attack I thought of while discussing [1],
that is worth mentioning.
We limited the number of http redirects in b25811646 (http: limit
redirection depth, 2015-09-22). But what about http-alternates? Could
you redirect to yourself via http-alternates and convince a client to
loop infinitely?
It looks like no, because we do not seem to handle recursive
alternates at all in the http walker. Which means that repositories with
recursive local alternates cannot be fetched over dumb-http. But it also
means that we don't have to worry about limiting the recursion depth.
-Peff
[1] http://public-inbox.org/git/fe33de5b5f0b3da68b249cc4a49a6d7@3c843fe6ba8f3c586a21345a2783aa0/
^ permalink raw reply
* Re: Git v2.11.0 breaks max depth nested alternates
From: Philip Oakley @ 2016-12-05 12:07 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle J. McKay, Junio C Hamano, Git mailing list
In-Reply-To: <20161205071822.ndeswelgj5epej5k@sigill.intra.peff.net>
From: "Jeff King" <peff@peff.net>
> On Sun, Dec 04, 2016 at 11:22:52AM -0000, Philip Oakley wrote:
>
>> > Ever since 722ff7f876 (receive-pack: quarantine objects until
>> > pre-receive accepts, 2016-10-03, v2.11.0), Git has been quarantining
>> > objects and packs received during an incoming push into a separate
>> > objects directory and using the alternates mechanism to make them
>> > available until they are either accepted and moved into the main
>> > objects directory or rejected and discarded.
>>
>> Is there a step here that after the accepted/rejected stage, it should
>> then
>> decrement the limit back to its original value. The problem description
>> suggests that might be the case.
>
> No. I thought that at first, too, but this increment happens in the
> sub-process which is using the extra level of alternates for its entire
> lifetime. So it "resets" it by exiting, and the parent process never
> increments its internal value at all.
>
Thanks for the clarification.
--
Philip
^ permalink raw reply
* Re: [PATCH 4/4] shallow.c: remove useless test
From: Duy Nguyen @ 2016-12-05 12:00 UTC (permalink / raw)
To: Jeff King; +Cc: Rasmus Villemoes, Git Mailing List
In-Reply-To: <20161203052422.hhaj3idboo6r6dz5@sigill.intra.peff.net>
On Sat, Dec 3, 2016 at 12:24 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Dec 02, 2016 at 09:31:04PM +0100, Rasmus Villemoes wrote:
>
>> It seems to be odd to do x=y if x==y. Maybe there's a bug somewhere near
>> this, but as is this is somewhat confusing.
>
> Yeah, this code is definitely wrong, but I'm not sure what it's trying
> to do. This is the first time I've looked at it.
I'm sorry I don't know why it's there either :( The first version that
has this was v3 [1] which still uses "util" pointdf instead of the
commit slab but the logic does not differ much.
This is the place when we "paint" the parent commit with the same
bitmap as the child I mentioned earlier (though I think I mentioned it
backward). You see similar code in the same loop just a bit earlier:
if the commit has not been painted, it gets a new bitmap, otherwise
new refs are OR'd to its bitmap. It's the OR part (when the bitmaps
pointed by *p_refs and *refs differ, it's not just about pointer
comparison) that's probably missing here.
But it looks like we can safely delete the " || *p_refs == *refs" part
because the commit in question is inserted back to the commit list
"head" and revisited in the next iteration. If its bitmap is different
from the child's, then in the next iteration it should hit the "if
(memcmp(tmp, *refs, bitmap_size))" line above, in the same loop, then
the new bit will be added. If it's marked UNINTERESTING though, that
won't happen. I'll need more time to stare at this code...
[1] http://public-inbox.org/git/1385351754-9954-9-git-send-email-pclouds@gmail.com/
--
Duy
^ permalink raw reply
* Re: difftool -d not populating left correctly when not in git root
From: Johannes Schindelin @ 2016-12-05 11:46 UTC (permalink / raw)
To: David Aguilar; +Cc: Frank Becker, git, John Keeping, Junio C Hamano
In-Reply-To: <20161204223139.kk2ejrfc5elxmsro@gmail.com>
Hi David,
On Sun, 4 Dec 2016, David Aguilar wrote:
> Date: Sun, 4 Dec 2016 14:27:17 -0800
> Subject: [PATCH] difftool: properly handle being launched from a subdirectory
>
> 9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
> are handled in a subdirectory, but it introduced a regression in how
> entries outside of the subdirectory are handled by the dir-diff.
>
> When preparing the right-side of the diff we only construct the parts
> that changed.
>
> When constructing the left side we construct an index, but we were
> constructing it relative to the subdirectory, and thus it ends up empty
> because we are in a subdirectory and the paths are incorrect.
>
> Teach difftool to chdir to the toplevel of the repository before
> preparing its temporary indexes. This ensures that all of the
> toplevel-relative paths are valid.
>
> Add a test case to exercise this use case.
>
> Reported-by: Frank Becker <fb@mooflu.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
I applied this to my builtin-difftool branch (which, as per Peff's
suggestion, runs t7800 both with and without the builtin difftool) and the
tests pass:
https://github.com/dscho/git/commit/9b9a69c5ee975a4a2565343ae0a9199a6ac89609
Which means that the builtin difftool already had fixed the bug, more by
coincidence than by design, I have to admit.
Ciao,
Johannes
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 11:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <20161205053258.jtnqq64gp5n7vtni@sigill.intra.peff.net>
On 05/12/16 05:32, Jeff King wrote:
> On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>> I recently noticed that:
>>
>> $ make >pout 2>&1
>> $ ./git version
>> git version 2.11.0.286.g109e8a9
>> $ git describe
>> v2.11.0-286-g109e8a99d
>> $
>>
>> ... for non-release builds, the commit part of the version
>> string was still using an --abbrev=7.
>
> It seems like this kind of discussion ought to go in the commit message.
> :)
>
> That said, I think the right patch may be to just drop --abbrev
> entirely.
Heh, that was the first version of the patch. However, I got to thinking
about why --abbrev=7 was there in the first place; the only reason I
could think of was to defeat local configuration to get a measure of
reproducibility.
Unfortunately, you can't get the 'auto' behaviour from --abbrev
(on the pu branch):
$ ./git describe --abbrev=-1
v2.11.0-286-g109e8
$ ./git describe --abbrev=0
v2.11.0
$ ./git describe
v2.11.0-286-g109e8a99d
$
I did think about using '-c core.abbrev=auto', but that would
depend on Junio's patch (nothing wrong with that, of course):
$ git version
git version 2.11.0
$ git -c core.abbrev=auto describe
fatal: bad numeric config value 'auto' for 'core.abbrev': invalid unit
$ ./git -c core.abbrev=auto describe
v2.11.0-286-g109e8a99d
$
What do you think?
ATB,
Ramsay Jones
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox