* Re: [PATCH v2 0/4] delete_ref: support reflog messages
From: Kyle Meyer @ 2017-02-21 16:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqr32sggfo.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> These looked reasonable. I had to resolve conflicts with another
> topic in flight that removed set_worktree_head_symref() function
> while queuing, and I think I resolved it correctly, but please
> double check what is pushed out on 'pu'.
The merge looks right to me, thanks. Thanks also for touching up the
tab/space issue in t3200-branch.sh.
--
Kyle
^ permalink raw reply
* Re: [PATCH v6 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Junio C Hamano @ 2017-02-21 16:55 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <20170219110313.24070-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> - git reset --hard ${GIT_QUIET:+-q}
This hunk is probably the most important one to review in the whole
series, in the sense that these are entirely new code that didn't
exist in the original.
> + if test $# != 0
> + then
> + saved_untracked=
> + if test -n "$(git ls-files --others -- "$@")"
I notice that "ls-files -o" used in the code before this series are
almost always used with --exclude-standard but we do not set up the
standard exclude pattern here. Is there a good reason to use (or
not to use) it here as well?
> + then
> + saved_untracked=$(
> + git ls-files -z --others -- "$@" |
> + xargs -0 git stash create -u all --)
> + fi
Running the same ls-files twice look somewhat wasteful.
I suspect that we avoid "xargs -0" in our code from portability
concern (isn't it a GNU invention?)
> + git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
Hmm, am I being naive to suspect that the above is a roundabout way
to say:
git reset ${GIT_QUIET:+-q} -- "$@"
or is an effect quite different from that intended here?
> + git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
Likewise. Wouldn't the above be equivalent to:
git checkout ${GIT_QUIET:+-q} HEAD -- "$@"
Or is this trying to preserve paths modified in the working tree and
fully added to the index?
> + if test -n "$(git ls-files -z --others -- "$@")"
> + then
> + git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
Likewise. "ls-files --others" being the major part of what "clean"
is about, I suspect the "ls-files piped to clean" is redundant. Do
you even need a test? IOW, doesn't "git clean" with a pathspec that
does not match anything silently succeed without doing anything
harmful?
> + fi
> + if test -n "$saved_untracked"
> + then
> + git stash pop -q $saved_untracked
I see this thing was "created", and the whole point of "create" is
to be different from "save/push" that automatically adds the result
to the stash reflog. Should we be "pop"ing it, or did you mean to
just call apply_stash on it?
> + fi
> + else
> + git reset --hard ${GIT_QUIET:+-q}
> + fi
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-21 17:01 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
In-Reply-To: <20170221073813.w4zrkky2d4drnwbs@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> +. ./test-lib.sh
>
> There are a bunch of other "git -c" tests inside t1300. I don't know if
> it would be better to put them all together.
I considered it, but it is already very long and I suspect it would
be better in the longer term to split the command-line one out into
a separate script, for the reasons you state.
I can move these at the end of 1300 in the meantime, and leave the
split for somebody else to be done later.
> Arguably, those other ones should get pulled out here to the new script.
> More scripts means that the tests have fewer hidden dependencies, and we
> can parallelize the run more. I admit I've shied away from new scripts
> in the past because the number allocation is such a pain.
>
>> +test_expect_success 'last one wins: two level vars' '
>> + echo VAL >expect &&
>> +
>> + # sec.var and sec.VAR are the same variable, as the first
>> + # and the last level of a configuration variable name is
>> + # case insensitive. Test both setting and querying.
>
> I assume by "setting and querying" here you mean "setting via -c,
> querying via git-config".
Yes. Should I update it to be more explicit?
> There are two blocks of tests, each with their own "expect" value.
> Should the top "expect" here be moved down with its block to make that
> more clear?
Perhaps. Let me try that one.
Thanks.
^ permalink raw reply
* [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-21 17:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Lars Schneider, Jonathan Tan, sbeller
In-Reply-To: <xmqqino3h3zv.fsf@gitster.mtv.corp.google.com>
The "git -c <var>=<val> cmd" mechanism is to pretend that a
configuration variable <var> is set to <val> while the cmd is
running. The code to do so however downcased <var> in its entirety,
which is wrong for a three-level <section>.<subsection>.<variable>.
The <subsection> part needs to stay as-is.
Reported-by: Lars Schneider <larsxschneider@gmail.com>
Diagnosed-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Changes from v1:
- update the comment before the second loop to find the last
dot.
- move two new tests into existing t1300 (at least for now).
- make it clear that the second of the new tests check two
aspects of "three level vars" by setting up the expectation for
the first half near the actual tests and adding comments on
what it tests before the second half.
config.c | 30 +++++++++++++++++++++++++++++-
t/t1300-repo-config.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 0dfed682b8..4128debc71 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,34 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+/*
+ * downcase the <section> and <variable> in <section>.<variable> or
+ * <section>.<subsection>.<variable> and do so in place. <subsection>
+ * is left intact.
+ */
+static void canonicalize_config_variable_name(char *varname)
+{
+ char *cp, *last_dot;
+
+ /* downcase the first segment */
+ for (cp = varname; *cp; cp++) {
+ if (*cp == '.')
+ break;
+ *cp = tolower(*cp);
+ }
+ if (!*cp)
+ return;
+
+ /* find the last dot (we start from the first dot we just found) */
+ for (last_dot = cp; *cp; cp++)
+ if (*cp == '.')
+ last_dot = cp;
+
+ /* downcase the last segment */
+ for (cp = last_dot; *cp; cp++)
+ *cp = tolower(*cp);
+}
+
int git_config_parse_parameter(const char *text,
config_fn_t fn, void *data)
{
@@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- strbuf_tolower(pair[0]);
+ canonicalize_config_variable_name(pair[0]->buf);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 923bfc5a26..7a16f66a9d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1097,6 +1097,52 @@ test_expect_success 'multiple git -c appends config' '
test_cmp expect actual
'
+test_expect_success 'last one wins: two level vars' '
+
+ # sec.var and sec.VAR are the same variable, as the first
+ # and the last level of a configuration variable name is
+ # case insensitive.
+
+ echo VAL >expect &&
+
+ git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+ test_cmp expect actual &&
+ git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+ test_cmp expect actual &&
+
+ git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+ test_cmp expect actual &&
+ git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+
+ # v.a.r and v.A.r are not the same variable, as the middle
+ # level of a three-level configuration variable name is
+ # case sensitive.
+
+ echo val >expect &&
+ git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+ test_cmp expect actual &&
+
+ # v.a.r and V.a.R are the same variable, as the first
+ # and the last level of a configuration variable name is
+ # case insensitive.
+
+ echo VAL >expect &&
+ git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
'
--
2.12.0-rc2-221-ga92f6f5816
^ permalink raw reply related
* Re: [RFC] Subtle differences in passing configs to git clone
From: Jeff King @ 2017-02-21 17:47 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git List
In-Reply-To: <EC270E42-9431-446C-96F9-E1A0C3E45333@gmail.com>
On Tue, Feb 21, 2017 at 12:36:25PM +0100, Lars Schneider wrote:
> I stumbled across the following today:
>
> (1) git -c foo.bar="foobar" clone <URL>
>
> --> uses the config temporarily
>
>
> (2) git clone -c foo.bar="foobar" <URL>
>
> --> uses the config and writes it to .git/config
>
> This was introduced in 84054f7 ("clone: accept config options on the
> command line") and it makes total sense.
Yep, they were designed to match.
> However, I think this subtitle difference can easily confuse users.
>
> I think we should tell the users that we've written to .git/config.
> Maybe something like this:
>
> git clone -c foo.bar="foobar" <URL>
> Cloning into 'test'...
> Writing foo.bar="foobar" to local config...
> remote: Counting objects: 2152, done.
> remote: Compressing objects: 100% (33/33), done.
> remote: Total 2152 (delta 19), reused 0 (delta 0), pack-reused 2119
> Receiving objects: 100% (2152/2152), 328.66 KiB | 217.00 KiB/s, done.
> Resolving deltas: 100% (1289/1289), done.
>
> What do you think?
<shrug> I don't find it confusing, but I can see how one might. Since
"clone" is already pretty chatty, I don't mind adding the extra message.
-Peff
^ permalink raw reply
* Re: Inconsistent results of git blame --porcelain when detecting copies from other files
From: Jeff King @ 2017-02-21 17:48 UTC (permalink / raw)
To: Sokolov, Konstantin; +Cc: gitster@pobox.com, git@vger.kernel.org
In-Reply-To: <71BF70CE41AEE741896AF3A5450D86F11F42694F@DEFTHW99EH3MSX.ww902.siemens.net>
On Tue, Feb 21, 2017 at 12:25:37PM +0000, Sokolov, Konstantin wrote:
> Thanks for going into the issue. As far as I understand 2.12 won't
> change the discussed behavior of --procelain. We will switch to
> --line-procelain. After the current discussion it seems to be less
> error prone, more future-proof and our current parser can handle it
> without any changes.
Right, the 2.12 change is only fixing a case where the "previous"
key/value line was not repeated at the correct spots. The same parsing
rules still hold.
-Peff
^ permalink raw reply
* Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
From: Jeff King @ 2017-02-21 17:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Schneider, Jonathan Tan, sbeller
In-Reply-To: <xmqqd1ebh3ak.fsf_-_@gitster.mtv.corp.google.com>
On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:
> * Changes from v1:
>
> - update the comment before the second loop to find the last
> dot.
>
> - move two new tests into existing t1300 (at least for now).
>
> - make it clear that the second of the new tests check two
> aspects of "three level vars" by setting up the expectation for
> the first half near the actual tests and adding comments on
> what it tests before the second half.
Thanks, this addresses all of my (admittedly minor) concerns.
-Peff
^ permalink raw reply
* Re: [PATCH v2] config: preserve <subsection> case for one-shot config on the command line
From: Stefan Beller @ 2017-02-21 17:57 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git@vger.kernel.org, Lars Schneider, Jonathan Tan
In-Reply-To: <20170221175021.znvpfvnlfjh4jsrf@sigill.intra.peff.net>
On Tue, Feb 21, 2017 at 9:50 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:
>
>> * Changes from v1:
>>
>> - update the comment before the second loop to find the last
>> dot.
>>
>> - move two new tests into existing t1300 (at least for now).
>>
>> - make it clear that the second of the new tests check two
>> aspects of "three level vars" by setting up the expectation for
>> the first half near the actual tests and adding comments on
>> what it tests before the second half.
>
> Thanks, this addresses all of my (admittedly minor) concerns.
>
> -Peff
This patch looks different than what I last looked at. I like it.
Though the manual search of the last dot instead of using
strrchr seems to be over-optimizing to me.
Anyway it is still very understandable.
Thanks,
Stefan
^ permalink raw reply
* Re: url.<base>.insteadOf vs. submodules
From: Stefan Beller @ 2017-02-21 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: Toolforger, git@vger.kernel.org
In-Reply-To: <20170221070653.65ho2anbp55uzjeu@sigill.intra.peff.net>
On Mon, Feb 20, 2017 at 11:06 PM, Jeff King <peff@peff.net> wrote:
>
> We'll see if the submodule folks have any ideas on how to implement
> that.
>
So from reading your discussion, the user expectation is to have
`git submodule {init, update --init, sync}`
to pay attention to url.<base>.insteadOf when setting up the
submodule.<name>.URL, such that the modified URL is used for the
initial clone of the submodule (and hence any subsequent usage within
the submodule).
That sounds like a good idea to me.
Two caveates:
* After running `git submodule init`, you change url.<base>.insteadOf
in the superproject. How do we need to word the documentation to
have users expecting this change doesn't affect submodules?
(See above Any vs. "Any except (initialized) submodules")
* So with the point above the insteadOf config only applies to the
init/sync process, (i.e. once in time, ideally).
Is that confusing or actually simplifying the submodule workflow?
Thanks,
Stefan
^ permalink raw reply
* [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
From: Junio C Hamano @ 2017-02-21 18:53 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqino5jia8.fsf@gitster.mtv.corp.google.com>
The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.
The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the <section> (and subsection) while making sure that <section>
consists of iskeychar() letters, the function itself that makes sure
that the first letter in <variable> is isalpha(), and get_value()
that grabs the remainder of the <variable> name while making sure
that it consists of iskeychar() letters.
Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
> One thing I noticed is that "git config --get X" will correctly
> diagnose that a dot-less X is not a valid variable name, but we do
> not seem to diagnose "git -c X=V <cmd>" as invalid.
>
> Perhaps we should, but it is not the focus of this topic.
And here is a follow-up on that tangent.
config.c | 48 +++++++++++++++++++++++++++++++++++++-----------
t/t1300-repo-config.sh | 7 +++++++
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+static inline int iskeychar(int c)
+{
+ return isalnum(c) || c == '-';
+}
+
/*
* downcase the <section> and <variable> in <section>.<variable> or
* <section>.<subsection>.<variable> and do so in place. <subsection>
* is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the <section> (and subsection) while making sure that <section>
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in <variable> is isalpha(), and
+ * get_value() that grabs the remainder of the <variable> name while
+ * making sure that it consists of iskeychar() letters. Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
*/
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
{
- char *cp, *last_dot;
+ char *cp, *first_dot, *last_dot;
/* downcase the first segment */
for (cp = varname; *cp; cp++) {
if (*cp == '.')
break;
+ if (!iskeychar(*cp))
+ return -1;
*cp = tolower(*cp);
}
if (!*cp)
- return;
+ return -1; /* no dot anywhere? */
+
+ first_dot = cp;
+ if (first_dot == varname)
+ return -1; /* no section? */
/* find the last dot (we start from the first dot we just found) */
- for (last_dot = cp; *cp; cp++)
+ for (; *cp; cp++)
if (*cp == '.')
last_dot = cp;
+ if (!last_dot[1])
+ return -1; /* no variable? */
+
/* downcase the last segment */
- for (cp = last_dot; *cp; cp++)
+ for (cp = last_dot + 1; *cp; cp++) {
+ if (cp == last_dot + 1 && !isalpha(*cp))
+ return -1;
+ else if (!iskeychar(*cp))
+ return -1;
*cp = tolower(*cp);
+ }
+ return 0;
}
int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- canonicalize_config_variable_name(pair[0]->buf);
+ if (canonicalize_config_variable_name(pair[0]->buf))
+ return error("bogus config parameter: %s", text);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
}
}
-static inline int iskeychar(int c)
-{
- return isalnum(c) || c == '-';
-}
-
static int get_value(config_fn_t fn, void *data, struct strbuf *name)
{
int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..92a5d0dfb1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,13 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
'
+for VAR in a .a a. a.0b a."b c". a."b c".0d
+do
+ test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
+ test_must_fail git -c $VAR=VAL config -l
+ '
+done
+
test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
'
--
2.12.0-rc2-221-ga92f6f5816
^ permalink raw reply related
* Re: Partnership with Git
From: Junio C Hamano @ 2017-02-21 19:04 UTC (permalink / raw)
To: Dov Grobgeld; +Cc: Nikita Malikov, Konstantin Khomoutov, git
In-Reply-To: <CA++fsGEvYPeP1UYniHF7OnowTH8-UeH3Kwe2KqaYMRouWVzEbg@mail.gmail.com>
Dov Grobgeld <dov.grobgeld@gmail.com> writes:
> As git is free software, you are free to use it in any way you see fit, as
> long as you adhere to its licensing terms, and to the copyright
> restrictions on using the term "git". Thus there is no need to ask
> permission and there does not on the git side exist any entity interested
> in "cross marketing activities".
s/copyright/trademark/.
As one of Software Freedom Conservancy projects, I suspect that the
Git PLC git@sfconservancy.org may be the closest to such "entity"
that represents open source Git community's interest to the outside
world with help from lawyers.
Not that I think Git PLC is interested in such a cross marketting
arrangement, but if Devart wants to advertise on their webpage
saying "we support Git by making contributions to SFC" or something
like that, they are the people to talk to.
^ permalink raw reply
* Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
From: Stefan Beller @ 2017-02-21 19:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq37f7gyuj.fsf_-_@gitster.mtv.corp.google.com>
On Tue, Feb 21, 2017 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.
>
> The configuration variable names that come from files are validated
> in git_config_parse_source(), which uses get_base_var() that grabs
> the <section> (and subsection) while making sure that <section>
> consists of iskeychar() letters, the function itself that makes sure
> that the first letter in <variable> is isalpha(), and get_value()
> that grabs the remainder of the <variable> name while making sure
> that it consists of iskeychar() letters.
>
> Perform an equivalent check in canonicalize_config_variable_name()
> to catch invalid configuration variable names that come from the
> command line.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > One thing I noticed is that "git config --get X" will correctly
> > diagnose that a dot-less X is not a valid variable name, but we do
> > not seem to diagnose "git -c X=V <cmd>" as invalid.
> >
> > Perhaps we should, but it is not the focus of this topic.
>
> And here is a follow-up on that tangent.
Combining this thought with another email[1] that flew by,
do we also need to have this treatment for "git clone -c"
[1] http://public-inbox.org/git/EC270E42-9431-446C-96F9-E1A0C3E45333@gmail.com/
>
> +for VAR in a .a a. a.0b a."b c". a."b c".0d
> +do
> + test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
> + test_must_fail git -c $VAR=VAL config -l
> + '
> +done
> +
> test_expect_success 'git -c is not confused by empty environment' '
> GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
Do we also want to test obscure cases of expected success?
e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
would be a valid key to be handed to git?
^ permalink raw reply
* Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge
From: Junio C Hamano @ 2017-02-21 19:32 UTC (permalink / raw)
To: Ross Lagerwall; +Cc: git
In-Reply-To: <20170218002341.23099-1-rosslagerwall@gmail.com>
Ross Lagerwall <rosslagerwall@gmail.com> writes:
> If a branch is configured with a default remote but no
> branch.<name>.merge and then the remote is removed, git fails to remove
> the remote with:
> "fatal: could not unset 'branch.<name>.merge'"
>
> Instead, ignore this since it is not an error and shouldn't prevent the
> remote from being removed.
>
> Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com>
> ---
I was waiting for others to comment on this patch but nobody seems
to be interested. Which is a bit sad, because except for minor
nits, this patch is very well done.
The explanation of the motivation and solution in the proposed log
message is excellent. It would have been perfect if you described
HOW you get into a state where branch.<name>.remote is pointing at
the remote being removed, without having branch.<name>.merge in the
first place, but even if such a state is invalid or unplausible,
removing the remote should be a usable way to recover from such a
situation.
And the proposed solution in the diff seems to correctly implement
what the description of the solution in the log message (modulo a
minor nit).
> builtin/remote.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index e52cf3925..5dd22c2eb 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
> strbuf_reset(&buf);
> strbuf_addf(&buf, "branch.%s.%s",
> item->string, *k);
> - git_config_set(buf.buf, NULL);
> + result = git_config_set_gently(buf.buf, NULL);
> + if (result && result != CONFIG_NOTHING_SET)
> + die(_("COULd not unset '%s'"), buf.buf);
With s/COUL/coul/, the result would be more in line with our
existing practice.
> }
> }
> }
We do want an additional test so that this fix will not be broken
again in the future by mistake, perhaps in t5505.
As it is unclear to me how you got into a state where branch.*.remote
exists without branch.*.merge, the attached patch to the test manually
removes it, which probably falls into a "deliberate sabotage" category.
If there are a valid sequence of operations that leads to such a state
without being a deliberate sabotage, we should use it instead in the
real test.
Thanks.
builtin/remote.c | 4 +++-
t/t5505-remote.sh | 19 +++++++++++++++++++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index e52cf3925b..01055b7272 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.%s",
item->string, *k);
- git_config_set(buf.buf, NULL);
+ result = git_config_set_gently(buf.buf, NULL);
+ if (result && result != CONFIG_NOTHING_SET)
+ die(_("could not unset '%s'"), buf.buf);
}
}
}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..af85a624fc 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting non-existent branch'
)
'
+test_expect_success 'remove remote with a branch without configured merge' '
+ test_when_finished "(
+ git -C test checkout master;
+ git -C test branch -D two;
+ git -C test config --remove-section remote.two;
+ git -C test config --remove-section branch.second;
+ true
+ )" &&
+ (
+ cd test &&
+ git remote add two ../two &&
+ git fetch two &&
+ git checkout -t -b second two/master &&
+ git checkout master &&
+ git config --unset branch.second.merge &&
+ git remote rm two
+ )
+'
+
test_expect_success 'rename errors out early when deleting non-existent branch' '
(
cd test &&
^ permalink raw reply related
* Re: Git bisect does not find commit introducing the bug
From: Alex Hoffman @ 2017-02-21 19:40 UTC (permalink / raw)
To: Philip Oakley
Cc: Oleg Taranenko, Jakub Narębski, Jacob Keller, Johannes Sixt,
Christian Couder, Stephan Beyer, git
In-Reply-To: <4C20E99781EC4D6D82D48FBF9D9472A1@PhilipOakley>
> isn't that spelt `--ancestry-path` ?
> (--ancestry-path has it's own issues such as needing an --first-parent-show
> option, but that's possibly a by the by)
Indeed it is spelled `--ancestry-path`. And interestingly enough you
may use it multiple times with the wanted effect in our case (e.g when
the user has multiple good commit and a single bad commit before
running the bisect itself). Also it is `--first-parent` (not
`--first-parent-show`), but I do not understand why do we need this
option? What kind of issues does `--ancestry-path` have?
Best regards,
VG
^ permalink raw reply
* Re: [PATCH 06/15] update submodules: add submodule config parsing
From: Stefan Beller @ 2017-02-21 19:42 UTC (permalink / raw)
To: Jacob Keller
Cc: Git mailing list, brian m. carlson, Jonathan Nieder,
Brandon Williams, Junio C Hamano
In-Reply-To: <CA+P7+xozip8TuvyUe9vAPYLAg=QFieExhOyR7a0pgGFhiuO3jw@mail.gmail.com>
On Fri, Feb 17, 2017 at 10:24 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>
> Ok so this function here reads a recurse submodules parameter which is
> a boolean or it can be set to the word "checkout"? Why does checkout
> need its own value separate from true? Just so that we have a synonym?
> or so that we can expand on it in the future?
I think eventually we want all the commands that touch the worktree to
be able to cope with submodules.
Now what should e.g. git-revert --recurse-submodules do?
yes == "checkout" means we'd revert the superproject commit and
if that commit changed any submodule pointers we'd just "checkout"
those states in the submodule.
For revert you could also imagine to have
git-revert --recurse-submodules=revert-in-subs
that would not repoint the submodule pointer to the old state, but
would try to revert $OLD..$NEW in the submodule and take the newly
reverted state as the new submodule pointer.
As I want to focus on checkout first, I went with "yes == checkout"
here (or rather the other way round).
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Junio C Hamano @ 2017-02-21 20:11 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Jacob Keller, Git Mailing List
In-Reply-To: <CA+55aFwdUxCvmi28T3yn1K4rqn2bZmJBdTRr7tSbMa-g5izHbw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> That source showing should never have been in "show_decorations()" in
> the first place. It just happened to be a convenient place for it.
>
> So this attached patch is just my original patch updated to split up
> "show_source()" from "show_decorations()", and show it where it used
> to be.
The updated organization smells a lot better to me ;-)
Most of the time it is convenient to have "show source" at the
beginning of a single helper that is to show both, but oneline
format is so special that it makes it inconvenient to have them at
the same place.
I can lose the patch to 4202 (update the expectation for --source)
I added to the previous one, but the patch to 4207 (update the
expectation for --decorate) needs to be kept with this round.
Will replace; thanks.
^ permalink raw reply
* Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'
From: Junio C Hamano @ 2017-02-21 20:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kbR2QQyYO1dnQ0jW3-ztKEFj1MtJfDqEc0xoftMFeN=WA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Combining this thought with another email[1] that flew by,
> do we also need to have this treatment for "git clone -c"
You tell me ;-)
Do we share the same parser? If not, should we make them share the
same code?
>> +for VAR in a .a a. a.0b a."b c". a."b c".0d
>> +do
>> + test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
>> + test_must_fail git -c $VAR=VAL config -l
>> + '
>> +done
>> +
>> test_expect_success 'git -c is not confused by empty environment' '
>> GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
>
> Do we also want to test obscure cases of expected success?
> e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
> would be a valid key to be handed to git?
I wasn't aiming for anything obscure (and a."b c".d is not at all
obscure); as the new tests like "git -c V.a.R config --get V.A.R"
added in the previous step makes sure that the second level is not
molested and passed as is, so it is less urgent to see what can and
cannot come at the second level.
I didn't check if the existing coverage was sufficient, but we
certainly should test that three-level names with non-alpha and
non-keychar letters in the second are allowed in the overall "git
config" test, not limited to the case where the configuration comes
on a one-shot command line but from files. I tend to think that is
a separate issue, though.
^ permalink raw reply
* Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge
From: Ross Lagerwall @ 2017-02-21 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtw7nfift.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:
> Ross Lagerwall <rosslagerwall@gmail.com> writes:
>
> > If a branch is configured with a default remote but no
> > branch.<name>.merge and then the remote is removed, git fails to remove
> > the remote with:
> > "fatal: could not unset 'branch.<name>.merge'"
> >
> > Instead, ignore this since it is not an error and shouldn't prevent the
> > remote from being removed.
> >
> > Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com>
> > ---
>
> I was waiting for others to comment on this patch but nobody seems
> to be interested. Which is a bit sad, because except for minor
> nits, this patch is very well done.
>
> The explanation of the motivation and solution in the proposed log
> message is excellent. It would have been perfect if you described
> HOW you get into a state where branch.<name>.remote is pointing at
> the remote being removed, without having branch.<name>.merge in the
> first place, but even if such a state is invalid or unplausible,
> removing the remote should be a usable way to recover from such a
> situation.
I got into this situation by setting branch.<name>.remote directly. I
was using push.default=current, and wanted a bare "git push" on the
branch to push to a different remote from origin (which it defaults to).
Configuring branch.<name>.remote made git do the right thing.
Perhaps what I did was invalid, I'm not sure, but it achieved what I
wanted.
>
> And the proposed solution in the diff seems to correctly implement
> what the description of the solution in the log message (modulo a
> minor nit).
>
> > builtin/remote.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index e52cf3925..5dd22c2eb 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
> > strbuf_reset(&buf);
> > strbuf_addf(&buf, "branch.%s.%s",
> > item->string, *k);
> > - git_config_set(buf.buf, NULL);
> > + result = git_config_set_gently(buf.buf, NULL);
> > + if (result && result != CONFIG_NOTHING_SET)
> > + die(_("COULd not unset '%s'"), buf.buf);
>
> With s/COUL/coul/, the result would be more in line with our
> existing practice.
Oops. That's what I get for coding when I should have been sleeping.
>
> > }
> > }
> > }
>
> We do want an additional test so that this fix will not be broken
> again in the future by mistake, perhaps in t5505.
>
> As it is unclear to me how you got into a state where branch.*.remote
> exists without branch.*.merge, the attached patch to the test manually
> removes it, which probably falls into a "deliberate sabotage" category.
> If there are a valid sequence of operations that leads to such a state
> without being a deliberate sabotage, we should use it instead in the
> real test.
>
See my explanation above. I wouldn't call it "deliberate sabotage", but
rather using config knobs in unexpected ways.
The test case looks reasonable. Do you want me to resend a patch with
the test case included (and nit fixed), or will you fix it up?
Thanks,
--
Ross Lagerwall
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Linus Torvalds @ 2017-02-21 20:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jacob Keller, Git Mailing List
In-Reply-To: <xmqqpoibfgo3.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 21, 2017 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The updated organization smells a lot better to me ;-)
So I have been using the original patch for a bit over a week now, and
I have to say that I'm not sure it's the right thing to do after all.
Most of the time I much prefer the "decorations at the end" thing,
because it just looks better, and the commit log oneliners line up
nicely.
But then occasionally I end up liking the old interface better, just
because there's long commit lines, and showing the decoration at the
end effectively hides it.
So I vacillate between the two formats, and so I'm not sure this patch
is worth the change in behavior after all.
In fact, I played around with some formats, and the one I lines the
most was actually one that split the line for decorations, but that
one was admittedly pretty funky. It gives output like
b9df16a4c (HEAD -> master)
pathspec: don't error out on all-exclusionary pathspec patterns
91a491f05 pathspec magic: add '^' as alias for '!'
c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
Git 2.12-rc2
076c05393 Hopefully the final batch of mini-topics before the final
c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
e048a257b Merge branch 'js/mingw-isatty'
(which looks better with colorization than it looks in the email).
But I'm not even going to send out that patch, because it was such an
atrocious hack to line things up.
Linus
^ permalink raw reply
* Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.
From: Junio C Hamano @ 2017-02-21 20:48 UTC (permalink / raw)
To: Andreas Heiduk; +Cc: git
In-Reply-To: <2b0ce702-60de-534b-8a86-5c7ae84060de@gmail.com>
Andreas Heiduk <asheiduk@gmail.com> writes:
> Add a hint for script writers where additional quoting can be configured.
>
> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
> ---
> Documentation/git-ls-files.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 446209e..19e0636 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -198,7 +198,8 @@ path. (see linkgit:git-read-tree[1] for more information on state)
>
> When `-z` option is not used, TAB, LF, and backslash characters
> in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively.
> +respectively. The path is also quoted according to the
> +configuration variable `core.quotePath` (see linkgit:git-config[1]).
I was waiting for others to comment on this patch but nobody seems
to be interested. Which is a bit sad, as this may not be a bad
idea.
If we refer to core.quotePath, the mention of control characters
being quoted can also be omitted, I think, as that is part of what
appears in the description of core.quotePath variable.
Alternatively, instead of referring to another page, we can spend
the additional lines to say what is more interesting to most of the
readers from that page, e.g.
When `-z` option is not used, a pathname with "unusual" characters
in it is quoted by enclosing it in a double-quote pair and with
backslashes the same way strings in C source code are quoted. By
setting core.quotePath configuration to false, the bytes whose
values are higher than 0x80 are output verbatim.
^ permalink raw reply
* Re: [PATCH 06/15] update submodules: add submodule config parsing
From: Jacob Keller @ 2017-02-21 20:48 UTC (permalink / raw)
To: Stefan Beller
Cc: Git mailing list, brian m. carlson, Jonathan Nieder,
Brandon Williams, Junio C Hamano
In-Reply-To: <CAGZ79kZyFfC9Xx-p8dpoAFFpz48BqmftpMonuxeiKg1sV68iuQ@mail.gmail.com>
On Tue, Feb 21, 2017 at 11:42 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Feb 17, 2017 at 10:24 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> Ok so this function here reads a recurse submodules parameter which is
>> a boolean or it can be set to the word "checkout"? Why does checkout
>> need its own value separate from true? Just so that we have a synonym?
>> or so that we can expand on it in the future?
>
> I think eventually we want all the commands that touch the worktree to
> be able to cope with submodules.
>
> Now what should e.g. git-revert --recurse-submodules do?
> yes == "checkout" means we'd revert the superproject commit and
> if that commit changed any submodule pointers we'd just "checkout"
> those states in the submodule.
>
> For revert you could also imagine to have
> git-revert --recurse-submodules=revert-in-subs
> that would not repoint the submodule pointer to the old state, but
> would try to revert $OLD..$NEW in the submodule and take the newly
> reverted state as the new submodule pointer.
>
> As I want to focus on checkout first, I went with "yes == checkout"
> here (or rather the other way round).
Ok I understand, but this seems like the variable could eventually
start to included more and more complex things? For now, "checkout"
means "when changing submodules prefer to check out contents" right?
I guess that sort of makes some sense.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule
From: Stefan Beller @ 2017-02-21 20:56 UTC (permalink / raw)
To: Jacob Keller
Cc: Git mailing list, brian m. carlson, Jonathan Nieder,
Brandon Williams, Junio C Hamano
In-Reply-To: <CA+P7+xqpxTt7KibOrVr2ekjLy6Hva4KJ6nCaN22j-Qpspky3aQ@mail.gmail.com>
On Fri, Feb 17, 2017 at 10:36 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@google.com> wrote:
>> In later patches we introduce the --recurse-submodule flag for commands
>> that modify the working directory, e.g. git-checkout.
>>
>> It is potentially expensive to check if a submodule needs an update,
>> because a common theme to interact with submodules is to spawn a child
>> process for each interaction.
>>
>> So let's introduce a function that checks if a submodule needs
>> to be checked for an update before attempting the update.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> submodule.c | 27 +++++++++++++++++++++++++++
>> submodule.h | 13 +++++++++++++
>> 2 files changed, 40 insertions(+)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 591f4a694e..2a37e03420 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
>> config_update_recurse_submodules = value;
>> }
>>
>> +int touch_submodules_in_worktree(void)
>> +{
>> + /*
>> + * Update can't be "none", "merge" or "rebase",
>> + * treat any value as OFF, except an explicit ON.
>> + */
>> + return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
>> +}
>
> Ok, so here, we're just checking whether the value is
> RECURSE_SUBMODULES_ON. The comment doesn't make sense to me at all.
Yes the comment was not updated in the last round of patches and is
now out of context.
> What is "update" and why "can't" it be those values? How is this code
> treating thise values as OFF exept for an explicit ON?
Please disregard the comment; I'll remove it in the next reroll.
The submodule API is in such a way that
config_update_recurse_submodules
>
> At first I thought this comment was related to check in the previous
> patch. I think I see the patch is "recurse submodules = true" or
> "recurse submodules = checkout" as a specific strategy? Ie:
> recurse-submodules=checkout" means "recurse into submodules and update
> them using checkout strategy?
Yes that is what I had in mind. See previous comment, in a later series
we could extend that to other strategies such as "revert-in-submodules"
for git-revert or "rebase", "merge" as we curreently have for
"git submodule update".
> Maybe this should be called something like
> recurse_into_submodules_in_worktree() though that is pretty verbose.
I like that. (It's less than double the number of characters, so it's
fine, isn't it?)
Maybe we can abbreviate worktree by "wt" ans "submodules by subs:
/* recurse into submodules in the worktree? */
int rec_subs_wt;
That looks short enough to qualify as non-Java.
> I'm not sure I have a good name really. But I wonder why we need this
> in the first place. Basically, we set RECURSE_SUBMODULES_* value which
> could be OFF, ON, or even future extensions of CHECKOUT, REBASE,
> MERGE, etc?
>
> But shouldn't we just return the mode, and let the later code decide
> "oh. actually I don't support this mode". For now, obviously we
> wouldn't set any of the new modes yet.
Mh, makes sense. Maybe I tricked myself into premature optimization,
because I'd expect most of the users not caring about submodules, such
that we want to have a *really* cheap way of saying "no, not interesting in
submodules", which is what this method mainly offers.
Junio also remarked this and the following
"is_active_submodule_with_strategy" to be bad design.
I'll redo those, such that the caller decides what to do with each strategy.
>
>> +
>> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
>> + enum submodule_update_type strategy)
>> +{
>> + const struct submodule *sub;
>> +
>> + if (!S_ISGITLINK(ce->ce_mode))
>> + return 0;
>> +
>> + if (!touch_submodules_in_worktree())
>> + return 0;
>> +
>> + sub = submodule_from_path(null_sha1, ce->name);
>> + if (!sub)
>> + return 0;
>> +
>> + return sub->update_strategy.type == strategy;
>> +}
>> +
>
> I liked Junio's suggestion where this just returns the strategy that
> it can use, or 0 if it shouldn't be updated. Then, other code just
> decides: Yes, I can use this strategy or no I cannot.
>
> Should this be tied in with the strategy used by the
> recurse_submodules above? ie: when/if we support recursing using other
> strategies, shouldn't we make these match here, so that if the recurse
> mode is "checkout" we don't checkout a submodule that was configured
> to be rebased? Or do you want to blindly apply checkout to all
> submodules even if they don't have strategy?
>
> I assume you do not, since you check here with passing a strategy
> value, and then see if it matches.
>
> this could be named something like:
>
> get_active_submodule_strategy() and return the strategy directly. It
> would then return 0 in any case where it shouldn't be updated. Later
> code can then check the strategy.
0 is already taken as SM_UPDATE_UNSPECIFIED,
so maybe we'd introduce a new update command
SM_UPDATE_IGNORE = -1 or rather use
SM_UPDATE_NONE instead.
>
>> static int has_remote(const char *refname, const struct object_id *oid,
>> int flags, void *cb_data)
>> {
>> diff --git a/submodule.h b/submodule.h
>> index b4e60c08d2..46d9f0f293 100644
>> --- a/submodule.h
>> +++ b/submodule.h
>> @@ -65,6 +65,19 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
>> const struct diff_options *opt);
>> extern void set_config_fetch_recurse_submodules(int value);
>> extern void set_config_update_recurse_submodules(int value);
>> +
>> +/*
>> + * Traditionally Git ignored changes made for submodules.
>> + * This function checks if we are interested in the given submodule
>> + * for any kind of operation.
>
> This comment seems a bit weird.
correct, I'll reword that.
>
>> + */
>> +extern int touch_submodules_in_worktree(void);
>> +/*
>> + * Check if the given ce entry is a submodule with the given update
>> + * strategy configured.
>
> I like Junio's suggestion of this "getting the current configured
> strategy for a submodule. When we aren't set to recurse into
> submodules we (obviously) return that we have no strategy since we're
> not going to update it at all.
>
>> + */
>> +extern int is_active_submodule_with_strategy(const struct cache_entry *ce,
>> + enum submodule_update_type strategy);
>> extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
>> extern int fetch_populated_submodules(const struct argv_array *options,
>> const char *prefix, int command_line_option,
>> --
>> 2.12.0.rc1.16.ge4278d41a0.dirty
>>
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Jeff King @ 2017-02-21 21:08 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Jacob Keller, Git Mailing List
In-Reply-To: <CA+55aFwT2HUBzZO8Gpt9tHoJtdRxv9oe3TDoSH5jcEOixRNBXg@mail.gmail.com>
On Tue, Feb 21, 2017 at 12:40:11PM -0800, Linus Torvalds wrote:
> In fact, I played around with some formats, and the one I lines the
> most was actually one that split the line for decorations, but that
> one was admittedly pretty funky. It gives output like
>
> b9df16a4c (HEAD -> master)
> pathspec: don't error out on all-exclusionary pathspec patterns
> 91a491f05 pathspec magic: add '^' as alias for '!'
> c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
> 20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
> Git 2.12-rc2
> 076c05393 Hopefully the final batch of mini-topics before the final
> c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
> 62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
> 1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
> bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
> e048a257b Merge branch 'js/mingw-isatty'
>
> (which looks better with colorization than it looks in the email).
>
> But I'm not even going to send out that patch, because it was such an
> atrocious hack to line things up.
I was going to suggest a custom format string that does the same, but
what we have is not _quite_ flexible enough.
You can use "%+d" to insert a newline only when "%d" is not empty. But
it always inserts _before_ the decoration, not after. Likewise, you
cannot say "if it's not empty, then insert %d and a leading tab".
The for-each-ref formatting code has %(if), but it's not unified with
the commit-format ones.
So the best I could come up with is:
git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
git log --format=twoline
which looks like:
80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
(origin/master, origin/HEAD)
20769079d Git 2.12-rc2
(tag: v2.12.0-rc2)
076c05393 Hopefully the final batch of mini-topics before the final
c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
-Peff
^ permalink raw reply
* [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'
From: Junio C Hamano @ 2017-02-21 21:24 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Stefan Beller
In-Reply-To: <xmqqlgszffm0.fsf@gitster.mtv.corp.google.com>
The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.
The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the <section> (and subsection) while making sure that <section>
consists of iskeychar() letters, the function itself that makes sure
that the first letter in <variable> is isalpha(), and get_value()
that grabs the remainder of the <variable> name while making sure
that it consists of iskeychar() letters.
Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Now with an updated test; while writing it it uncovered a bug in
the original test that expected to fail---they failed alright but
sometimes failed for a wrong reason.
config.c | 48 +++++++++++++++++++++++++++++++++++++-----------
t/t1300-repo-config.sh | 16 ++++++++++++++++
2 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+static inline int iskeychar(int c)
+{
+ return isalnum(c) || c == '-';
+}
+
/*
* downcase the <section> and <variable> in <section>.<variable> or
* <section>.<subsection>.<variable> and do so in place. <subsection>
* is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the <section> (and subsection) while making sure that <section>
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in <variable> is isalpha(), and
+ * get_value() that grabs the remainder of the <variable> name while
+ * making sure that it consists of iskeychar() letters. Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
*/
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
{
- char *cp, *last_dot;
+ char *cp, *first_dot, *last_dot;
/* downcase the first segment */
for (cp = varname; *cp; cp++) {
if (*cp == '.')
break;
+ if (!iskeychar(*cp))
+ return -1;
*cp = tolower(*cp);
}
if (!*cp)
- return;
+ return -1; /* no dot anywhere? */
+
+ first_dot = cp;
+ if (first_dot == varname)
+ return -1; /* no section? */
/* find the last dot (we start from the first dot we just found) */
- for (last_dot = cp; *cp; cp++)
+ for (; *cp; cp++)
if (*cp == '.')
last_dot = cp;
+ if (!last_dot[1])
+ return -1; /* no variable? */
+
/* downcase the last segment */
- for (cp = last_dot; *cp; cp++)
+ for (cp = last_dot + 1; *cp; cp++) {
+ if (cp == last_dot + 1 && !isalpha(*cp))
+ return -1;
+ else if (!iskeychar(*cp))
+ return -1;
*cp = tolower(*cp);
+ }
+ return 0;
}
int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- canonicalize_config_variable_name(pair[0]->buf);
+ if (canonicalize_config_variable_name(pair[0]->buf))
+ return error("bogus config parameter: %s", text);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
}
}
-static inline int iskeychar(int c)
-{
- return isalnum(c) || c == '-';
-}
-
static int get_value(config_fn_t fn, void *data, struct strbuf *name)
{
int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..ea371020fa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,22 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
'
+for VAR in a .a a. a.0b a."b c". a."b c".0d
+do
+ test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
+ test_must_fail git -c "$VAR=VAL" config -l
+ '
+done
+
+for VAR in a.b a."b c".d
+do
+ test_expect_success "git -c $VAR=VAL works with valid '$VAR'" '
+ echo VAL >expect &&
+ git -c "$VAR=VAL" config --get "$VAR" >actual &&
+ test_cmp expect actual
+ '
+done
+
test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
'
--
2.12.0-rc2-222-gff02733afe
^ permalink raw reply related
* Re: [RFC PATCH] show decorations at the end of the line
From: Junio C Hamano @ 2017-02-21 21:47 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Jacob Keller, Git Mailing List
In-Reply-To: <20170221210808.3ryri33ve7w7csdp@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The for-each-ref formatting code has %(if), but it's not unified with
> the commit-format ones.
>
> So the best I could come up with is:
>
> git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
> git log --format=twoline
>
> which looks like:
>
> 80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
> (origin/master, origin/HEAD)
> 20769079d Git 2.12-rc2
> (tag: v2.12.0-rc2)
> 076c05393 Hopefully the final batch of mini-topics before the final
> c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
> 62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
> 1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
Yeah, I had a similar thought to use something around "%n%-d", but
$ git log --format='%h%n%-d%C(auto) %s %C(auto)'
is not it.
I guess we could pile on another hack to make the sign between % and
the format specifier cumulative and then "%n%-+d" may do what we
want, but we need a true %(if)...%(then)...%(else)...%(end) support
if we really want to do this thing properly.
^ 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