* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Duy Nguyen @ 2016-12-09 13:08 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161208181957.GP116201@google.com>
On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/08, Duy Nguyen wrote:
>> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
>> > On 12/07, Duy Nguyen wrote:
>> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
>> >> > Convert 'create_simplify()' to use the pathspec struct interface from
>> >> > using the '_raw' entry in the pathspec.
>> >>
>> >> It would be even better to kill this create_simplify() and let
>> >> simplify_away() handle struct pathspec directly.
>> >>
>> >> There is a bug in this code, that might have been found if we
>> >> simpify_away() handled pathspec directly: the memcmp() in
>> >> simplify_away() will not play well with :(icase) magic. My bad. If
>> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> >> ignore exclude patterns there too (although not excluding is not a
>> >> bug).
>> >
>> > So are you implying that the simplify struct needs to be killed? That
>> > way the pathspec struct itself is being passed around instead?
>>
>> Yes. simplify struct was a thing when pathspec was an array of char *.
>> At this point I think it can retire (when we have time to retire it)
>
> Alright, then for now I can leave this change as is and have a follow up
> series that kills the simplify struct.
Do let me know if you decide to drop it, so I can put it back in my backlog.
--
Duy
^ permalink raw reply
* [PATCH] describe: add tests for unusual graphs
From: Quinn Grier @ 2016-12-09 13:11 UTC (permalink / raw)
To: git; +Cc: Quinn Grier
git describe may give incorrect results if there are backdated commits
or multiple roots. This commit adds two test_expect_failure tests that
demonstrate these problems.
Signed-off-by: Quinn Grier <quinn@quinngrier.com>
---
t/t6120-describe.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f2694..ca82837 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -206,4 +206,52 @@ test_expect_success 'describe --contains with the exact tags' '
test_cmp expect actual
'
+#
+# A---B*--D master
+# \ /
+# .---C topic
+#
+
+test_expect_failure 'backdated commit' '(
+ test_tick &&
+ b=$GIT_COMMITTER_DATE && test_tick &&
+ test_create_repo backdated-commit &&
+ cd backdated-commit &&
+ git commit --allow-empty -m A && test_tick &&
+ GIT_COMMITTER_DATE=$b git commit --allow-empty -m B && test_tick &&
+ git checkout -b topic :/A &&
+ git commit --allow-empty -m C && test_tick &&
+ git checkout master &&
+ git merge -m D topic && test_tick &&
+ git tag -m B B :/B && test_tick &&
+ git describe :/D >tmp &&
+ sed s/-g.\*// tmp >actual &&
+ echo B-2 >expected &&
+ test_cmp expected actual
+)'
+
+#
+# A---B*--D master
+# /
+# C* other
+#
+
+test_expect_failure 'multiple roots' '(
+ test_tick &&
+ test_create_repo multiple-roots &&
+ cd multiple-roots &&
+ git commit --allow-empty -m A && test_tick &&
+ git commit --allow-empty -m B && test_tick &&
+ git checkout --orphan other &&
+ git commit --allow-empty -m C && test_tick &&
+ git checkout master &&
+ git merge --allow-unrelated-histories -m D other && test_tick &&
+ git tag -m B B :/B && test_tick &&
+ git tag -m C C :/C && test_tick &&
+ git describe :/D >tmp &&
+ sed s/-g.\*// tmp >actual &&
+ echo B-2 >expected &&
+ test_cmp expected actual
+)'
+
test_done
--
2.8.3
^ permalink raw reply related
* Re: Bug: git-sh-setup giving no such file or directory
From: Jeff King @ 2016-12-09 13:45 UTC (permalink / raw)
To: Paul Boyle; +Cc: Anders Kaseorg, git
In-Reply-To: <CABZ0BffSi6h8Zhg8vjo1dZhxXg3fUt_U6TAtqMvpDShOX6HyyA@mail.gmail.com>
On Fri, Dec 09, 2016 at 12:00:36PM +0000, Paul Boyle wrote:
> There appears to be an issue with the latest master.
>
> "git submodule init" is producing the following error:
>
> /home/paul.boyle/bin/git/git-sh-setup: line 46:
> /home/paul.boyle/libexec/git-core/git-sh-i18n: No such file or
> directory
Hmm. Did you run "make install"? Or are you trying to run git directly
out of the build directory?
If the latter, that has been unsupported for a while, though it mostly
works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
or to just .../git/bin-wrappers into your $PATH.
> Broken sha: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
>
> Checking out an older version works fine.
>
> git checkout 'master@{2016-11-01 18:30:00}'
>
> Sha: 3cdd5d19178a54d2e51b5098d43b57571241d0ab
>
> This can be reproduced simply by:
>
> make clean ; make ; git submodule init
>
>
> I didn't track it down further than to a commit sometime in the last month.
You could probably find the exact commit with git-bisect. However, I'd
be surprised if it is anything but 1073094f3 (git-sh-setup: be explicit
where to dot-source git-sh-i18n from., 2016-10-29). Before that commit,
we found git-sh-i18n in the $PATH, which would work if you were adding
git's build directory to your $PATH (but not work for people who
actually did an install).
-Peff
^ permalink raw reply
* Re: Bug: git-sh-setup giving no such file or directory
From: Paul Boyle @ 2016-12-09 14:00 UTC (permalink / raw)
To: Jeff King; +Cc: Anders Kaseorg, git
In-Reply-To: <20161209134544.z2xbly5xjyito62w@sigill.intra.peff.net>
> Hmm. Did you run "make install"? Or are you trying to run git directly
> out of the build directory?
>
> If the latter, that has been unsupported for a while, though it mostly
> works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
> or to just .../git/bin-wrappers into your $PATH.
This was the source of it. Root cause, a stupid user ;) I'd a PATH
setup to the build directory. Changing the path to bin-wrappers fixed
it up.
Thanks!
On Fri, Dec 9, 2016 at 1:45 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Dec 09, 2016 at 12:00:36PM +0000, Paul Boyle wrote:
>
>> There appears to be an issue with the latest master.
>>
>> "git submodule init" is producing the following error:
>>
>> /home/paul.boyle/bin/git/git-sh-setup: line 46:
>> /home/paul.boyle/libexec/git-core/git-sh-i18n: No such file or
>> directory
>
> Hmm. Did you run "make install"? Or are you trying to run git directly
> out of the build directory?
>
> If the latter, that has been unsupported for a while, though it mostly
> works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
> or to just .../git/bin-wrappers into your $PATH.
>
>> Broken sha: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
>>
>> Checking out an older version works fine.
>>
>> git checkout 'master@{2016-11-01 18:30:00}'
>>
>> Sha: 3cdd5d19178a54d2e51b5098d43b57571241d0ab
>>
>> This can be reproduced simply by:
>>
>> make clean ; make ; git submodule init
>>
>>
>> I didn't track it down further than to a commit sometime in the last month.
>
> You could probably find the exact commit with git-bisect. However, I'd
> be surprised if it is anything but 1073094f3 (git-sh-setup: be explicit
> where to dot-source git-sh-i18n from., 2016-10-29). Before that commit,
> we found git-sh-i18n in the $PATH, which would work if you were adding
> git's build directory to your $PATH (but not work for people who
> actually did an install).
>
> -Peff
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Jeff King @ 2016-12-09 14:03 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, gitster, jnareb
In-Reply-To: <20161207153627.1468-19-Karthik.188@gmail.com>
On Wed, Dec 07, 2016 at 09:06:26PM +0530, Karthik Nayak wrote:
> +const char *quote_literal_for_format(const char *s)
> {
> + struct strbuf buf = STRBUF_INIT;
>
> + strbuf_reset(&buf);
> + while (*s) {
> + const char *ep = strchrnul(s, '%');
> + if (s < ep)
> + strbuf_add(&buf, s, ep - s);
> + if (*ep == '%') {
> + strbuf_addstr(&buf, "%%");
> + s = ep + 1;
> + } else {
> + s = ep;
> + }
> }
> + return buf.buf;
> }
You should use strbuf_detach() to return the buffer from a strbuf.
Otherwise it is undefined whether the pointer is allocated or not (and
whether it needs to be freed).
In this case, if "s" is empty, buf.buf would point to a string literal,
but otherwise to allocated memory. strbuf_detach() normalizes that.
But...
> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
This caller never stores the return value, and it ends up leaking. So I
wonder if you wanted "static struct strbuf" in the first place (and that
would explain the strbuf_reset() in your function).
-Peff
^ permalink raw reply
* Re: git add -p with new file
From: Jeff King @ 2016-12-09 14:11 UTC (permalink / raw)
To: Ariel; +Cc: git
In-Reply-To: <alpine.DEB.2.11.1612062012540.13185@cherryberry.dsgml.com>
On Tue, Dec 06, 2016 at 08:18:59PM -0500, Ariel wrote:
> If you do git add -p new_file it says:
>
> No changes.
>
> Which is a rather confusing message. I would expect it to show me the
> content of the file in patch form, in the normal way that -p works, let me
> edit it, etc.
>
> (Note: I am aware I can do -N first, but when I specifically enter the name
> of a new file I feel it should figure out what I mean.)
Keep in mind that the argument to "git add -p" is not a filename, but a
"pathspec" that can match many files. What should:
git init
mkdir subdir
for i in one two; do echo $i >subdir/$i; done
git add -p subdir
do? Or for that matter, "git add -p ."? It's contrary to the rest of
git-add for specifying pathspecs to actually make things _more_
inclusive rather than less.
So I don't think triggering the change of behavior based on the presence
of a pathspec makes sense.
Historically "add -p" has been more like "add -u" in updating tracked
files. We have "-A" for "update everything _and_ new files". It doesn't
seem unreasonable to me to have a variant of "-p" that is similar.
I don't think that variant should just be "add -N everything, and then
run add -p". As Duy points out, the patch for a new file is just one big
block. But the decision of "do I want to start tracking this untracked
file" is potentially an interesting one.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
From: Johannes Sixt @ 2016-12-09 14:33 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, peff, jacob.keller, gitster, ramsay, tboegi,
pclouds
In-Reply-To: <1481241494-6861-2-git-send-email-bmwill@google.com>
Am 09.12.2016 um 00:58 schrieb Brandon Williams:
> 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 one step closer to being 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..92f2a29 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 > offset_1st_component(path->buf)) {
> + char *last_slash = find_last_dir_sep(path->buf);
> + strbuf_setlen(path, last_slash - path->buf);
> + }
> +}
This implementation is not correct because when the input is "/foo", the
result is "" when it should be "/". Also, can the input be a
non-normalized path? When the input is "foo///bar", should the result be
"foo" or would "foo//" be an acceptable result? I think it should be the
former. find_last_dir_sep() returns the last of the three slashes, not
the first one. Therefore, I've rewritten the function thusly:
static void strip_last_component(struct strbuf *path)
{
size_t offset = offset_1st_component(path->buf);
size_t len = path->len;
while (offset < len && !is_dir_sep(path->buf[len - 1]))
len--;
while (offset < len && is_dir_sep(path->buf[len - 1]))
len--;
strbuf_setlen(path, len);
}
> +
> +/* get (and remove) the next component in 'remaining' and place 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);
> +}
Mental note: This function strips leading slashes and the first path
component; post-condition: remaining is empty or has leading slashes.
> +
> /* 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 */
> + int offset = offset_1st_component(path);
> + strbuf_add(&resolved, path, offset);
> + strbuf_addstr(&remaining, path + offset);
OK.
> + } else {
> + /* relative path; can use CWD as the initial resolved path */
> + if (strbuf_getcwd(&resolved)) {
> + if (die_on_error)
> + die_errno("unable to 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 */
Can this happen? I think it can: when 'path' is a root directory, or
ends with path separators.
> + } else if (next.len == 1 && !strcmp(next.buf, ".")) {
> + continue; /* '.' component */
Good.
I don't mind strcmp(), but others might point out that a single
character comparison should be sufficient.
> + } else if (next.len == 2 && !strcmp(next.buf, "..")) {
> + /* '..' component; strip the last path component */
> + strip_last_component(&resolved);
> + continue;
Good.
> }
>
> - if (sb.len) {
> - if (!cwd.len && strbuf_getcwd(&cwd)) {
> + /* append the next component and resolve resultant path */
> + if (!is_dir_sep(resolved.buf[resolved.len - 1]))
> + strbuf_addch(&resolved, '/');
Can we access resolved.buf[resolved.len - 1] here? At this point,
resolved has been filled with an absolute path or from getcwd(); it
cannot be empty in the first iteration. In subsequent iterations, it
cannot be empty as long as strip_last_component() does not make it
empty. Your original version of strip_last_component() could make it
empty; my rewrite should not.
Do we have to check for the path separator at all? Typically, resolved
does not end with a slash, but if we went all the way up to the root,
there is a trailing slash. Good; the condition is required.
> + strbuf_addbuf(&resolved, &next);
> +
> + if (lstat(resolved.buf, &st)) {
> + /* error out unless this was the last component */
> + if (!(errno == ENOENT && !remaining.len)) {
Perhaps it was easier to _write_ the condition in this way, but I would
have an easier time to _read_ it when it is
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; set resolved to root */
> + int offset = offset_1st_component(symlink.buf);
> + strbuf_reset(&resolved);
> + strbuf_add(&resolved, symlink.buf, offset);
> + strbuf_remove(&symlink, 0, offset);
Good. I would have expected some kind of "goto repeat;" because when we
encounter a symlink to an absolute path, most, if not all, work done so
far is obsoleted. But I haven't thought too deeply about this.
> + } 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);
Good. This looks very reasonable.
> + }
> }
>
> - 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(&remaining);
> + strbuf_release(&next);
> + strbuf_release(&symlink);
>
> return retval;
> }
>
^ permalink raw reply
* Re: [PATCH v2 3/4] real_path: create real_pathdup
From: Johannes Sixt @ 2016-12-09 14:35 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, peff, jacob.keller, gitster, ramsay, tboegi,
pclouds
In-Reply-To: <1481241494-6861-4-git-send-email-bmwill@google.com>
Am 09.12.2016 um 00:58 schrieb Brandon Williams:
> +char *real_pathdup(const char *path)
> +{
> + struct strbuf realpath = STRBUF_INIT;
> + char *retval = NULL;
> +
> + if(strbuf_realpath(&realpath, path, 0))
Style nit: blank after if is missing.
-- Hannes
^ permalink raw reply
* [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-09 14:02 UTC (permalink / raw)
To: git
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hello,
I have some repositories where I have a colon in the (local) url for a
remote. That was no problem until now but with 2.11.0, I see the
following problem:
~> git push
Counting objects: 11, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
Total 11 (delta 7), reused 0 (delta 0)
remote: error: object directory /home/xxx does not exist; check .git/objects/info/alternates.
remote: error: object directory yyy.git/objects does not exist; check .git/objects/info/alternates.
remote: fatal: unresolved deltas left after unpacking
error: unpack failed: unpack-objects abnormal exit
To /home/xxx:yyy.git
! [remote rejected] master -> master (unpacker error)
error: failed to push some refs to '/home/xxx:yyy.git'
Prepending the path with "file://" does not help.
It seems that the new git version splits the url at ":" which ends in
two incorrect paths.
Pull seems tho work well currently.
Regards
Klaus
Ps. I am not subscribet to the mailing list, so please keep me in Cc.
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhKuV4ACgkQpnwKsYAZ
9qz81Qv6AsgZHlaEHEybERAvGikjZgUvjyC7dzQ2zCmc8iv0eb8kGLiBtVtInsWr
eo/CiMSdX2emoCD5LQq/sxcRIgIoWpF8m2NEXiBMl94d6YLOpvWV1yC1kQ8qK6bt
Pyeo9/LofAnpLcQRn1am8tFwrcCMLZxSM7cxMxjtP6i+RU0MHc/rS1HqhdzptpsH
jvB0x41X7LNoeRLqG5n8lVlyHP1PiGyP0Dl4Aa9rFBHn+hydiJEmLEwdn9w4wgIz
vo2DZmqGm0r4vTaTP1gQRPxoW6fsBZ1uUWKRMjd947R1eaELpyROj4SFt0bWNqwW
cx9izYUuTg+xSe1KTaSgPlmZn1817DlG2yJ/YduKH8v61ZJ2r6B1awO2tz32g7cA
QdjsnzAyz6eVLrrsJ5OrJPRF2Fl7gM22jo9gs3BJrHeJdC9dU6kVIAR3eryoFvUg
fG/Vl2zvfbMRQAaUDGMyxNk5TGVFB6ANw0KS/NczRvmbPA9kukBz012+8ZY8MHzD
aEvmk/yz
=tDwn
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Jeff King @ 2016-12-09 15:22 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: git
In-Reply-To: <20161209140215.qlam6bexm5irpro2@ikki.ethgen.ch>
On Fri, Dec 09, 2016 at 03:02:15PM +0100, Klaus Ethgen wrote:
> I have some repositories where I have a colon in the (local) url for a
> remote. That was no problem until now but with 2.11.0, I see the
> following problem:
> ~> git push
> Counting objects: 11, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (10/10), done.
> Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
> Total 11 (delta 7), reused 0 (delta 0)
> remote: error: object directory /home/xxx does not exist; check .git/objects/info/alternates.
> remote: error: object directory yyy.git/objects does not exist; check .git/objects/info/alternates.
> remote: fatal: unresolved deltas left after unpacking
Hrm. The problem v2.11's new object-quarantine system. The receiving
side of a push will write into a new temporary object directory, and
refer to the original with the GIT_ALTERNATE_OBJECT_DIRECTORIES
environment variable. But that variable splits its entries on ":", and
has no way of representing a path that includes a ":".
So I think the solution would probably be to introduce some kind of
quoting mechanism to that variable, so that it can pass through
arbitrary paths. Or alternatively use a separate variable, but that does
nothing to help other cases where somebody wants to use xxx:yyy.git as
an alternate.
(One other option is to just declare that the quarantine feature doesn't
work with colons in the pathname, but stop turning it on by default. I'm
not sure I like that, though).
Here's a rough idea of what the quoting solution could look like. It
should make your case work, but I'm not sure what we want to do about
backwards-compatibility, if anything.
---
sha1_file.c | 41 ++++++++++++++++++++++++++++++-----------
tmp-objdir.c | 18 ++++++++++++++++--
2 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..a0b341b5a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -329,13 +329,35 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
return 0;
}
+const char *parse_alt_odb_entry(const char *string, int sep,
+ struct strbuf *out)
+{
+ const char *p;
+ int literal = 0;
+
+ strbuf_reset(out);
+
+ for (p = string; *p; p++) {
+ if (literal) {
+ strbuf_addch(out, *p);
+ literal = 0;
+ } else {
+ if (*p == '\\')
+ literal = 1;
+ else if (*p == sep)
+ return p + 1;
+ else
+ strbuf_addch(out, *p);
+ }
+ }
+ return p;
+}
+
static void link_alt_odb_entries(const char *alt, int len, int sep,
const char *relative_base, int depth)
{
- struct string_list entries = STRING_LIST_INIT_NODUP;
- char *alt_copy;
- int i;
struct strbuf objdirbuf = STRBUF_INIT;
+ struct strbuf entry = STRBUF_INIT;
if (depth > 5) {
error("%s: ignoring alternate object stores, nesting too deep.",
@@ -348,16 +370,13 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
die("unable to normalize object directory: %s",
objdirbuf.buf);
- alt_copy = xmemdupz(alt, len);
- string_list_split_in_place(&entries, alt_copy, sep, -1);
- for (i = 0; i < entries.nr; i++) {
- const char *entry = entries.items[i].string;
- if (entry[0] == '\0' || entry[0] == '#')
+ while (*alt) {
+ alt = parse_alt_odb_entry(alt, sep, &entry);
+ if (!entry.len || entry.buf[0] == '#')
continue;
- link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
+ link_alt_odb_entry(entry.buf, relative_base, depth, objdirbuf.buf);
}
- string_list_clear(&entries, 0);
- free(alt_copy);
+ strbuf_release(&entry);
strbuf_release(&objdirbuf);
}
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..900e15af1 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -70,6 +70,17 @@ static void remove_tmp_objdir_on_signal(int signo)
raise(signo);
}
+static char *backslash_quote(const char *s, int delim)
+{
+ struct strbuf buf = STRBUF_INIT;
+ while (*s) {
+ if (*s == '\\' || *s == delim)
+ strbuf_addch(&buf, '\\');
+ strbuf_addch(&buf, *s++);
+ }
+ return strbuf_detach(&buf, NULL);
+}
+
/*
* These env_* functions are for setting up the child environment; the
* "replace" variant overrides the value of any existing variable with that
@@ -79,12 +90,15 @@ static void remove_tmp_objdir_on_signal(int signo)
*/
static void env_append(struct argv_array *env, const char *key, const char *val)
{
+ char *quoted = backslash_quote(val, PATH_SEP);
const char *old = getenv(key);
if (!old)
- argv_array_pushf(env, "%s=%s", key, val);
+ argv_array_pushf(env, "%s=%s", key, quoted);
else
- argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, val);
+ argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, quoted);
+
+ free(quoted);
}
static void env_replace(struct argv_array *env, const char *key, const char *val)
--
2.11.0.201.g51bd297
^ permalink raw reply related
* [PATCH 0/4] doc: fixes to gitcore-tutorial.txt
From: Kristoffer Haugsbakk @ 2016-12-09 15:51 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
This series of patches attempts to fix some minor mistakes in
gitcore-tutorial.txt that I found while reading it. They are all
concerned with grammar and things like accidentally omitted words.
I previously sent a single patch on 2016-11-04 ("[PATCH] doc: fill in
omitted word"). The patch "doc: omit needless "for"" is a follow-up to
that, taking into consideration the feedback from Junio C Hamano and
Jeff King.
Kristoffer Haugsbakk (4):
doc: add articles (grammar)
doc: add verb in front of command to run
doc: make the intent of sentence clearer
doc: omit needless "for"
Documentation/gitcore-tutorial.txt | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH 1/4] doc: add articles (grammar)
From: Kristoffer Haugsbakk @ 2016-12-09 15:51 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>
Add definite and indefinite articles in three places where they were
missing.
- Use "the" in front of a directory name
- Use "the" in front of "style of cooperation"
- Use an indefinite article in front of "CVS background"
Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
---
Documentation/gitcore-tutorial.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 4546fa0d7..6c434aff3 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1368,7 +1368,7 @@ $ git repack
will do it for you. If you followed the tutorial examples, you
would have accumulated about 17 objects in `.git/objects/??/`
directories by now. 'git repack' tells you how many objects it
-packed, and stores the packed file in `.git/objects/pack`
+packed, and stores the packed file in the `.git/objects/pack`
directory.
[NOTE]
@@ -1543,9 +1543,9 @@ like this:
Working with Others, Shared Repository Style
--------------------------------------------
-If you are coming from CVS background, the style of cooperation
+If you are coming from a CVS background, the style of cooperation
suggested in the previous section may be new to you. You do not
-have to worry. Git supports "shared public repository" style of
+have to worry. Git supports the "shared public repository" style of
cooperation you are probably more familiar with as well.
See linkgit:gitcvs-migration[7] for the details.
--
2.11.0
^ permalink raw reply related
* [PATCH 2/4] doc: add verb in front of command to run
From: Kristoffer Haugsbakk @ 2016-12-09 15:51 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>
Instead of using the command 'git clone' as a verb, use "run" as the
verb indicating the action of executing the command 'git clone'.
Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
---
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 6c434aff3..72ed90ca3 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1478,7 +1478,7 @@ You can repack this private repository whenever you feel like.
A recommended work cycle for a "subsystem maintainer" who works
on that project and has an own "public repository" goes like this:
-1. Prepare your work repository, by 'git clone' the public
+1. Prepare your work repository, by running 'git clone' on the public
repository of the "project lead". The URL used for the
initial cloning is stored in the remote.origin.url
configuration variable.
--
2.11.0
^ permalink raw reply related
* [PATCH 3/4] doc: make the intent of sentence clearer
From: Kristoffer Haugsbakk @ 2016-12-09 15:51 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>
By adding the word "just", which might have been accidentally omitted.
Adding the word "just" makes it clear that the point is to *not* do an
octopus merge simply because you *can* do it. In other words, you
should have a reason for doing it beyond simply having two (seemingly)
independent commits that you need to merge into another branch, since
it's not always the best approach.
The previous sentence made it look more like it was trying to say that
you shouldn't do an octopus merge *because* you can do an octopus merge.
Although this interpretation doesn't make sense and the rest of the
paragraph makes the intended meaning clear, this adjustment should make
the intent of the sentence more immediately clear to the reader.
Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
---
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 72ed90ca3..72ca9c1ef 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1635,7 +1635,7 @@ $ git show-branch
++* [master~2] Pretty-print messages.
------------
-Note that you should not do Octopus because you can. An octopus
+Note that you should not do Octopus just because you can. An octopus
is a valid thing to do and often makes it easier to view the
commit history if you are merging more than two independent
changes at the same time. However, if you have merge conflicts
--
2.11.0
^ permalink raw reply related
* [PATCH 4/4] doc: omit needless "for"
From: Kristoffer Haugsbakk @ 2016-12-09 15:51 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>
What was intended was perhaps "... plumbing does for you" ("you" added), but
simply omitting the word "for" is more terse and gets the intended point across
just as well, if not more so.
I originally went with the approach of writing "for you", but Junio C
Hamano suggested this approach instead.
Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
---
Notes (kristoffers):
The original patch was sent to the mailing list on 2016-11-04, and Junio
replied with his suggested correction on 2016-11-10; see the cover
letter.
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 72ca9c1ef..22309cfb4 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -25,7 +25,7 @@ you want to understand Git's internals.
The core Git is often called "plumbing", with the prettier user
interfaces on top of it called "porcelain". You may not want to use the
plumbing directly very often, but it can be good to know what the
-plumbing does for when the porcelain isn't flushing.
+plumbing does when the porcelain isn't flushing.
Back when this document was originally written, many porcelain
commands were shell scripts. For simplicity, it still uses them as
--
2.11.0
^ permalink raw reply related
* Re: [REGRESSION 2.10.2] problematic "empty auth" changes
From: Johannes Schindelin @ 2016-12-09 15:58 UTC (permalink / raw)
To: David Turner; +Cc: git
In-Reply-To: <1481231552.20894.20.camel@frank>
Hi David,
On Thu, 8 Dec 2016, David Turner wrote:
> On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
>
> > I got a couple of bug reports that claim that 2.10.2 regressed on
> > using network credentials. That is, users regularly hit Enter twice
> > when being asked for user name and password while fetching via
> > https://, and cURL automatically used to fall back to using the login
> > credentials (i.e. authenticating via the Domain controller).
> >
> > Turns out those claims are correct: hitting Enter twice (or using URLs
> > with empty user name/password such as https://:tfs:8080/) work in
> > 2.10.1 and yield "Authentication failed" in 2.10.2.
> >
> > I tracked this down to 5275c3081c (http: http.emptyauth should allow
> > empty (not just NULL) usernames, 2016-10-04) which all of a sudden
> > disallowed empty user names (and now only handles things correctly
> > when http.emptyAuth is set to true specifically).
> >
> > This smells like a real bad regression to me, certainly given the time
> > I had to spend to figure this out (starting from not exactly helpful
> > bug reports, due to being very specific to their setups being
> > private).
> >
> > I am *really* tempted to change the default of http.emptyAuth to true,
> > *at least* for Windows (where it is quite common to use your login
> > credentials to authenticate to corporate servers).
> >
> > Before I do anything rash, though: Do you see any downside to that?
>
> I know of no reason that shouldn't work. Indeed, it's what we use do
> internally. So far, nobody has reported problems.
Good.
> That said, we have exactly three sets of git servers that most users
> talk to (two different internal; and occasionally github.com for
> external stuff). So our coverage is not very broad.
Okay. I think I will extend that coverage rather boldly, then ;-)
> If you're going to do it, tho, don't just do it for Windows users -- do
> it for everyone. Plenty of Unix clients connect to Windows-based auth
> systems.
Makes sense.
> That said, I could imagine that there are cases where it would cause
> failures for a different set of users.
Let's see. At the moment, my main concern is that Git for Windows is
broken for corporate users (i.e. users who rely on the implicit
login authentication provided through their domain accounts). I cannot
imagine that defaulting http.emptyAuth=true could cause any worse
breakage.
It would be different, of course, if http.emptyAuth would *not* allow the
user to type their credentials when accessing something like
https://github.com/dscho/shhh-secret-repository, *only* trying the login
credentials. But that is not the case, with http.emptyAuth=true, login
credentials are attempted first, and when they fail, the user is still
asked interactively for their credentials.
All I can see is that this would be *an improvement*: corporate users
trying to access a Git repository that requires their login credentials
would now not even need to enter empty user name/password.
This alone would be already a good reason to change the default, IMHO.
So here is my plan:
- change the default of http.emptyAuth to true in the next Git for Windows
version
- publish a prerelease for early adopters to test
- contribute this patch here on the Git mailing list, in the hope that it
will make it into the next major version
Ciao,
Dscho
^ permalink raw reply
* Git 2.11.0 on OS X El Capitan 10.11.6
From: Kyle Flesness @ 2016-12-09 16:20 UTC (permalink / raw)
To: git
Hello! Thanks for taking the time to read this bug report, I am operating a mac book pro with OS X El Capitan 10.11.6 and attempting to download git 2.10.1, the installation appears to finalize with no problems but Git is not at the download destination from the designated path. Will 2.11.0 be compatible with OS X El Capitan 10.11.6? Any other solutions you might recommend?
Thank you for your time and hard work!!
Cheers,
Kyle
^ permalink raw reply
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Johannes Schindelin @ 2016-12-09 16:52 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Jeff King
In-Reply-To: <CACsJy8DDwKhOBOxD_yOf2MfhxP4_TDDj3AwCf25ct6YH3TGp6g@mail.gmail.com>
Hi Duy,
On Fri, 9 Dec 2016, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 10:35 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Hopefully these patches will lead to something that we can integrate,
> > and that eventually will make Git's startup sequence much less
> > surprising.
>
> What did it surprise you with?
I mentioned a couple of WTFs elsewhere:
- that it changes the working directory (personally, I am not surprised,
just because I was exposed to the Git source code for 10+ years,
however, I know developers who do not share that experience and find that
feature... interesting)
- that there are multiple code paths to do essentially the same thing, but
in an incompatible manner: setup_git_directory() and setup_git_env()
(and possibly more)
- that some environment variables are set, and need to be unset (or even
possibly reset to no-longer-known values) to allow subprocesses to
access different repositories
- that git_path() can be used before setup_git_directory() without
failure, but wreaks havoc with subsequent git_config() calls.
- that the setup_git_directory() cascade is completely oblivious of
whatever config has already been read (possibly requiring clearing)
- that setup_git_directory() *returns* the prefix, even stores it in
startup_info, but a subsequent call to setup_git_directory() returns
NULL
- that as a consequence, builtins that require the prefix to work if there
is any, but do not necessarily require a repository to begin with, think
`git diff`, *must not* be marked with RUN_SETUP_GENTLY
- that check_repository_format() sets have_repository=1
- that setup_git_directory() is a oneliner, calling
setup_git_directory_gently(NULL), when it would be more logical to
simply make setup_git_directory() accept the "nongit_ok" parameter
- that setup_git_directory_gently() is not at all gentle when the
parameter is NULL
- that resolve_gitdir() does not, in fact, resolve any git directory, but
only tests a specified path whether it refers to a .git/ directory or to
a .git file
- that resolve_gitdir() actually tests for a .git *file*
- that resolve_gitdir() is not used in setup_git_directory_gently_1()
- that resolve_gitdir() tries the order directory,file and
setup_git_directory_gently_1() tries the opposite order
- that the handling of the ceiling directories is hardcoded into the
setup_git_directory_gently_1() function
- that a ceiling directory of /home/hello/world does not prevent Git from
looking at /home/hello/world/.git/
- that canonicalize_ceiling_entry()' relationship to ceiling directories
is rather coincidental when the name suggests that it is very specific
- that canonicalize_ceiling_entry() does not, in fact, canonicalize
non-absolute entries
Need I go on? I could, you know...
> I can see that I disrespect the ceiling directory setting, perhaps
> that's that.
No, I actually see a lot of good reasons for the ceiling directories.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Vasco Almeida @ 2016-12-09 17:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <xmqqoa17quls.fsf@gitster.mtv.corp.google.com>
A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> The incremental update below looks sensible. We'd also want to
> protect this codepath from a misconfigured two-or-more byte sequence
> in core.commentchar, I would suspect, to be consistent.
Are the below changes alright for what you propose? It just checks if
the length of core.commentchar's value is 1, otherwise use '#' as the
comment_line_char.
As a note, when I set core.commentchar with "git config
core.commentChar 'batata'", I get the following error message when I
issue "git add -i":
error: core.commentChar should only be one character
fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', '-');
- my $comment_line_char = Git::config("core.commentchar") || '#';
+ my $comment_line_char = Git::get_comment_line_char;
print $fh Git::comment_lines sprintf(__ <<EOF, $remove_minus, $remove_plus, $comment_line_char),
---
To remove '%s' lines, make them ' ' lines (context).
diff --git a/perl/Git.pm b/perl/Git.pm
index 69cd1dd..e4da913 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1451,6 +1451,20 @@ sub prefix_lines {
return $string;
}
+=item get_comment_line_char ( )
+
+Gets the core.commentchar configuration value.
+The value fallbacks to # if core.commentchar is set to 'auto'.
+
+=cut
+
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ $comment_line_char = '#' if (length($comment_line_char) != 1);
+ return $comment_line_char;
+}
+
=item comment_lines ( STRING [, STRING... ])
Comments lines following core.commentchar configuration.
@@ -1458,7 +1472,7 @@ Comments lines following core.commentchar configuration.
=cut
sub comment_lines {
- my $comment_line_char = config("core.commentchar") || '#';
+ my $comment_line_char = get_comment_line_char;
return prefix_lines("$comment_line_char ", @_);
}
^ permalink raw reply related
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Johannes Schindelin @ 2016-12-09 17:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20161208172626.3ee2nmgsxsh2vovf@sigill.intra.peff.net>
Hi Peff,
On Thu, 8 Dec 2016, Jeff King wrote:
> On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote:
>
> > The idea here is to discover the .git/ directory gently (i.e. without
> > changing the current working directory), and to use it to read the
> > .git/config file early, before we actually called setup_git_directory()
> > (if we ever do that).
>
> Great. Thanks for taking a stab at this.
Well, I figured that I can go through you to get this integrated into
git.git.
> > Notes:
> >
> > - I find the diff pretty ugly: I wish there was a more elegant way to
> > *disable* discovery of .git/ *just* for `init` and `clone`. I
> > considered a function `about_to_create_git_dir()` that is called in a
> > hard-coded manner *only* for `init` and `clone`, but that would
> > introduce another magic side effect, when all I want is to reduce those.
>
> It looks like a lot of that ugliness comes from passing around the "are
> we OK to peek at git-dir config" flag through the various pager-related
> calls.
Correct.
> I don't think it would be bad to use a global for "we do not want a
> repo".
I would think it would be bad, as the entire reason for this patch series
is that we have global state that gets messed up too early (I am speaking
from the point of view of somebody who patched Git locally so that it does
read config variables *before* launching builtins).
> After all, it's just modifying the _existing_ global for "are we in a
> repo".
No it does not.
The read_early_config() function specifically does not leave any traces in
the global namespace. It calls the git_config_with_options() function
without touching the_config_set.
Of course we could introduce a new config set, just for early use. It
would share all the downsides with the current config set.
I would look more favorably on this idea if we were to teach
the_config_set to record a little bit more about the state from which it
was constructed, and to auto-flush-and-re-read when it detects that, say,
git_dir was changed in the meantime.
> And I do not see that global going away anytime soon.
And that is really, really sad.
> Sometimes it's good to make incremental steps towards an end goal, but
> in this case it seems like we just pay the cost of the step without any
> real plan for reaping the benefit in the long term.
Fine. Let's roll with this for now, then.
I just hope that we do not repeat the "slam the door shut to a better
solution" mistake that led to this situation, as the chdir() way did.
> As an alternative, I also think it would be OK to just always have the
> pager config read from local repo, even for init/clone.
For the purpose of this current discussion, I am utterly uninterested in
the pager config. What I want to use the early config for is substantially
different and more relevant: I need to configure some command to run
before, and after, every single Git command here. And this configuration
needs to be per-repository. And no, I do not want to hardcode anything.
So now the cat is out of the bag, I have ulterior motives.
These motives serve a greater good, though: designing read_early_config()
around a very specific use case will always fall short of a well-designed
read_early_config() that could then be used for any use case that
requires, well, reading the config early.
> In other words, to loosen the idea that git-init can _never_ look in the
> current git-dir, and declare that there is a stage before the command is
> initiated, and during which git may read local-repo config. Aliases
> would fall into this, too, so:
>
> git config --local alias.foo init
> git foo /some/other/dir
>
> would work (as it must, because we cannot know that "foo" is "init"
> until we read the config!).
True.
But is this a good excuse to just shrug our shoulders and let git-init
(which we do know very well) fall into the same trap?
> > - For the moment, I do not handle dashed invocations of `init` and
> > `clone` correctly. The real problem is the continued existence of
> > the dashed git-init and git-clone, of course.
>
> I assume you mean setting the CREATES_GIT_DIR flag here? I think it
> would apply to the dashed invocations, too. They strip off the "git-"
> and end up in handle_builtin() just like "git clone" does. I didn't test
> it, though.
You are correct. I misread the code to expect it to spawn another instance
of git.exe.
> > - There is still duplicated code. For the sake of this RFC, I did not
> > address that yet.
>
> Yeah, I did not read your discover function very carefully. Because I
> think the one thing we really don't want to do here is introduce a
> separate lookup process that is not shared by setup_git_directory(). The
> only sane path, I think, is to refactor setup_git_directory() to build
> on top of discover_git_directory(), which implies that the latter
> handles all of the cases.
This could maybe happen later, but for now I do not concern myself with
building the prefix, as the config machinery does not require that
knowledge.
If we go that route, we should of course cache the cwd, git_dir and prefix
of earlier runs, and avoid the entire discovery if we start from the same
cwd.
> > - The read_early_config() function is called multiple times, re-reading
> > all the config files and re-discovering the .git/ directory multiple
> > times, which is quite wasteful. For the sake of this RFC, I did not
> > address that yet.
>
> We already have a config-caching system. If we went with a global
> "config_discover_refs",
Why "_refs"?
> then I think the sequence for something like git-init would become:
>
> 1. When git.c starts, config_discover_refs is set to "true". Pager and
> alias lookup may look in .git/config if it's available, even if
> they go through the configset cache.
>
> 2. As soon as git-init starts, it disables config_discover_refs, and
> it flushes the config cache. Any configset lookups will now examine
> the reduced config.
>
> 3. When git-init has set up the real repo it is operating on, it can
> reenable config_discover_refs (though it may not even need to; that
> flag probably wouldn't have any effect once we've entered the
> repository and have_git_dir() returns true).
That is a bit fiddly, don't you think? The callers have to have very
intimate knowledge of the config reading to remember to set, and re-set,
that global. And to flush when appropriate.
How much nicer would the code be if the call to git_config() would realize
what needs to be done, don't you agree?
Less chance for bugs to be introduced by contributors with flakey
understanding of the config/git_dir machinery, myself included.
> > - t7006 fails and the error message is a bit cryptic (not to mention the
> > involved function trace, which is mind-boggling for what is supposed
> > to be a simply, shell script-based test suite). I did not have time to
> > look into that yet.
>
> Running t7006 I see a lot of old failures turned into successes, which
> is good (because running from a subdirectory now actually respects local
> pager config). The one failure looks like it is testing the wrong thing.
Yeah, but due to the redirections I was not able to figure out what to
change to "fix" this.
> It is checking that we _don't_ respect a local core.pager in some cases,
> as a proxy for whether or not we are breaking things by doing setup too
> early. But the whole point of your series is to fix that, and respect
> the config without causing the setup breakage. After your patches, the
> proxy behavior and the failure case are disconnected. The test should be
> flipped, and ideally another one added that confirms we didn't actually
> run setup_git_directory(), but I'm not sure how to test that directly.
Some dirty tricks come to mind, but I am not even sure that I want to test
for this. Why exactly do we need to avoid calling setup_git_directory() in
that case?
> > - after discover_git_directory_gently() did its work, the code happily
> > uses its result *only* for the current read_early_config() run, and
> > lets setup_git_dir_gently() do the whole work *again*. For the sake of
> > this RFC, I did not address that yet.
>
> If caching happens at the config layer, then we'd probably only call
> this once anyway (or if we did call it again after a config flush, it
> would be a good sign that we should compute its value again).
I meant the git_dir, not the config... The git_dir is discovered, but a
subsequent setup_git_dir_gently() discovers it *again*, in a different
way.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Johannes Schindelin @ 2016-12-09 17:32 UTC (permalink / raw)
To: Vasco Almeida
Cc: Junio C Hamano, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
Jakub Narębski, David Aguilar
In-Reply-To: <1481303956.4934.8.camel@sapo.pt>
[-- Attachment #1: Type: text/plain, Size: 877 bytes --]
Hi Vasco,
On Fri, 9 Dec 2016, Vasco Almeida wrote:
> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> > The incremental update below looks sensible. We'd also want to
> > protect this codepath from a misconfigured two-or-more byte sequence
> > in core.commentchar, I would suspect, to be consistent.
>
> Are the below changes alright for what you propose? It just checks if
> the length of core.commentchar's value is 1, otherwise use '#' as the
> comment_line_char.
> As a note, when I set core.commentchar with "git config
> core.commentChar 'batata'", I get the following error message when I
> issue "git add -i":
>
> error: core.commentChar should only be one character
> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
This is exactly the same issue I fixed for rebase -i recently.
Good eyes,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Jeff King @ 2016-12-09 17:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1612091810500.23160@virtualbox>
On Fri, Dec 09, 2016 at 06:28:10PM +0100, Johannes Schindelin wrote:
> > Great. Thanks for taking a stab at this.
>
> Well, I figured that I can go through you to get this integrated into
> git.git.
I am not sure what you mean here, but it _sounds_ like you are
continuing to be negative about the chances of fixes going into git.git
here. I really don't think that negativity is merited, but even if it
is, making snide comments does not help and mostly just makes the
conversation less pleasant.
> > I don't think it would be bad to use a global for "we do not want a
> > repo".
>
> I would think it would be bad, as the entire reason for this patch series
> is that we have global state that gets messed up too early (I am speaking
> from the point of view of somebody who patched Git locally so that it does
> read config variables *before* launching builtins).
I think those are two different things. One is global state that is
munged as a side effect of setup_git_directory(). The other is global
state that the process sets to say "this is an invariant" so it does not
have to deal with passing it through a huge call chain.
> > After all, it's just modifying the _existing_ global for "are we in a
> > repo".
>
> No it does not.
Perhaps I wasn't clear on my "it" here. I mean that we will continue to
have startup_info->have_repository as a global (and if not that, then
certainly the environment variable GIT_DIR and the cwd are process
globals). I'm proposing a global flag to modify how to interpret those
globals. I don't think that really makes anything worse.
> The read_early_config() function specifically does not leave any traces in
> the global namespace. It calls the git_config_with_options() function
> without touching the_config_set.
I'm not talking about read_early_config() modifying the global state.
I'm talking about main() modifying it so that lower-level functions like
read_early_config() can make use of it.
> I would look more favorably on this idea if we were to teach
> the_config_set to record a little bit more about the state from which it
> was constructed, and to auto-flush-and-re-read when it detects that, say,
> git_dir was changed in the meantime.
I considered that when fixing some bugs in git-init a few months ago,
but I think the cure becomes worse than the problem. Automatic cache
invalidation can have some tricky corner cases, and there really
_aren't_ that many places where we need to do it. Just dropping
git_config_clear() in those places is ugly, but practical.
> > And I do not see that global going away anytime soon.
>
> And that is really, really sad.
I think we differ on that.
> > As an alternative, I also think it would be OK to just always have the
> > pager config read from local repo, even for init/clone.
>
> For the purpose of this current discussion, I am utterly uninterested in
> the pager config. What I want to use the early config for is substantially
> different and more relevant: I need to configure some command to run
> before, and after, every single Git command here. And this configuration
> needs to be per-repository. And no, I do not want to hardcode anything.
I do not see how that changes things. If there is the concept of a
"before the command is run" phase during which we would read from the
current-directory config even for git-init, it seems to me that applies
logically to pagers, aliases, _and_ whatever pre-command magic you are
interested in adding.
> > In other words, to loosen the idea that git-init can _never_ look in the
> > current git-dir, and declare that there is a stage before the command is
> > initiated, and during which git may read local-repo config. Aliases
> > would fall into this, too, so:
> >
> > git config --local alias.foo init
> > git foo /some/other/dir
> >
> > would work (as it must, because we cannot know that "foo" is "init"
> > until we read the config!).
>
> True.
>
> But is this a good excuse to just shrug our shoulders and let git-init
> (which we do know very well) fall into the same trap?
I'm not sure I agree it is a trap. There is a consistent and reasonable
mental model where it is the natural thing. I understand that's not the
one you prefer, but it seems like a practical one to me.
> > We already have a config-caching system. If we went with a global
> > "config_discover_refs",
>
> Why "_refs"?
Er, sorry, slip of the tongue. I think I meant config_discover_git, and
for some reason managed to repeat it over and over. Hopefully you
figured out what I meant.
> > then I think the sequence for something like git-init would become:
> >
> > 1. When git.c starts, config_discover_refs is set to "true". Pager and
> > alias lookup may look in .git/config if it's available, even if
> > they go through the configset cache.
> >
> > 2. As soon as git-init starts, it disables config_discover_refs, and
> > it flushes the config cache. Any configset lookups will now examine
> > the reduced config.
> >
> > 3. When git-init has set up the real repo it is operating on, it can
> > reenable config_discover_refs (though it may not even need to; that
> > flag probably wouldn't have any effect once we've entered the
> > repository and have_git_dir() returns true).
>
> That is a bit fiddly, don't you think? The callers have to have very
> intimate knowledge of the config reading to remember to set, and re-set,
> that global. And to flush when appropriate.
Sure, but I think there are literally 2 callers who care about this.
> How much nicer would the code be if the call to git_config() would realize
> what needs to be done, don't you agree?
No, I wouldn't agree until I had seen a very elegant patch that
implements this and handles any corner cases. I looked into making a
patch before and found that it got a bit ugly (sorry, I don't really
remember the details). I am happy to have you show me that patch, but
pardon me if I remain skeptical until I see it. ;)
> > Running t7006 I see a lot of old failures turned into successes, which
> > is good (because running from a subdirectory now actually respects local
> > pager config). The one failure looks like it is testing the wrong thing.
>
> Yeah, but due to the redirections I was not able to figure out what to
> change to "fix" this.
I think test_local_config_ignored would go away, and its callers become
test_core_pager_overrides. More or less a revert of 73e25e7cc (git
--paginate: do not commit pager choice too early, 2010-06-26).
> > proxy behavior and the failure case are disconnected. The test should be
> > flipped, and ideally another one added that confirms we didn't actually
> > run setup_git_directory(), but I'm not sure how to test that directly.
>
> Some dirty tricks come to mind, but I am not even sure that I want to test
> for this. Why exactly do we need to avoid calling setup_git_directory() in
> that case?
I imagine that "git -p init /path/to/new/repo" would start from the
wrong directory, for instance (or any other builtin which does not have
RUN_SETUP would probably get confused).
> > > - after discover_git_directory_gently() did its work, the code happily
> > > uses its result *only* for the current read_early_config() run, and
> > > lets setup_git_dir_gently() do the whole work *again*. For the sake of
> > > this RFC, I did not address that yet.
> >
> > If caching happens at the config layer, then we'd probably only call
> > this once anyway (or if we did call it again after a config flush, it
> > would be a good sign that we should compute its value again).
>
> I meant the git_dir, not the config... The git_dir is discovered, but a
> subsequent setup_git_dir_gently() discovers it *again*, in a different
> way.
I don't think it's such a huge amount of work that it really matters
from an optimization standpoint. The really ugly thing to me is if it
returns a different answer. And I think the best way to deal with that
is to literally use the same code (and then if you want to cache the
result and not do the work again, it's pretty easy).
-Peff
^ permalink raw reply
* Re: Resend: Gitk: memory consumption improvements
From: Stefan Beller @ 2016-12-09 17:57 UTC (permalink / raw)
To: Markus Hitter; +Cc: git@vger.kernel.org, Paul Mackerras
In-Reply-To: <d10d2b12-4ef1-61e9-0b3c-89aa41c9eeff@jump-ing.de>
On Fri, Dec 9, 2016 at 3:51 AM, Markus Hitter <mah@jump-ing.de> wrote:
>
> It's a month now since I sent three patches to this list for reducing memory consumption of Gitk considerably:
>
> https://public-inbox.org/git/de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de/
> https://public-inbox.org/git/e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de/
> https://public-inbox.org/git/8e1c5923-d2a6-bc77-97ab-3f154b41d2ea@jump-ing.de/
> https://public-inbox.org/git/2cb7f76f-0004-a5b6-79f1-9bb4f979cf14@jump-ing.de/
>
> Everybody cheered, but apparently nobody picked these patches up and applied them to either the Git or Gitk repository. They don't appear in the regular "what's cooking" post either.
The "What's cooking" email is done by Junio (the Git maintainer)
describing the development of Git, whereas gitk is maintained by Paul
if I understand correctly.
If you look at the Git history, the changes to gitk-git/ always come in via
a merge/pull from Paul.
>
> Anything missing? I'd like to put a 'done' checkmark behind this.
>
I'd love to see those patches in use here (via a regular upstream update).
So I guess the way to go for you is to keep bugging Paul for an ack?
Thanks,
Stefan
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Junio C Hamano @ 2016-12-09 18:07 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stephan Beyer, Git Mailing List, Christian Couder,
SZEDER Gábor
In-Reply-To: <CACsJy8CX0HO=LxcEK3K+pCecgFY=40R+gpFoy7CGeN5zEJFJVQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
>
> Having the same operation with different names only increases git
> reputation of bad/inconsistent UI. Either forget is renamed to quit,
> or vice versa. I prefer forget, but the decision is yours and the
> community's. So I'm sending two patches to rename in either direction.
> You can pick one.
I actually was advocating to remove both by making --abort saner.
With an updated --abort that behaves saner, is "rebase --forget"
still necessary?
^ permalink raw reply
* Re: git add -p with new file
From: Ariel @ 2016-12-09 18:26 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8A_YMyEUgX--1tEfJC4aaYfDbFFL8WEs6CHp4a4=mHRjw@mail.gmail.com>
On Wed, 7 Dec 2016, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 8:18 AM, Ariel <asgit@dsgml.com> wrote:
>> If you do git add -p new_file it says:
>> No changes.
>> Which is a rather confusing message. I would expect it to show me the
>> content of the file in patch form, in the normal way that -p works, let me
>> edit it, etc.
> We could improve it a bit, suggesting the user to do git add -N. But
> is there a point of using -p on a new file?
I got into the habit of always using -p as a way of checking my diffs
before committing, so I ran it out of habit on a new file as well and got
that confusing message.
It's even worse if you run it on multiple files, some changed, some added
- the added ones are ignored completely, without any message at all.
> It will be one big chunk, you can't split anything.
That's fine, that's what I would expect.
> Perhaps maybe you want to use 'e' to edit what's added?
I might, but mainly it would show me what it was adding.
-Ariel
^ 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