Git development
 help / color / mirror / Atom feed
* 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

* [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: 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

* 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: [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: 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: [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

* [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: [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

* 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 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: Git trademark status and policy
From: G. Sylvie Davies @ 2017-02-21 15:55 UTC (permalink / raw)
  To: Git Users, Jeff King
In-Reply-To: <20170202022655.2jwvudhvo4hmueaw@sigill.intra.peff.net>

On Wed, Feb 1, 2017 at 6:26 PM, Jeff King <peff@peff.net> wrote:
> As many of you already know, the Git project (as a member of Software
> Freedom Conservancy) holds a trademark on "Git".  This email will try to
> lay out a bit of the history and procedure around the enforcement of
> that trademark, along with some open questions about policy.
>
> I'll use "we" in the text below, which will generally mean the Git
> Project Leadership Committee (PLC). I.e., the people who represent the
> Git project as part of Conservancy -- me, Junio Hamano, and Shawn
> Pearce.
>
> We approached Conservancy in Feb 2013 about getting a trademark on Git
> to ensure that anything calling itself "Git" remained interoperable with
> Git. Conservancy's lawyer drafted the USPTO application and submitted it
> that summer. The trademark was granted in late 2014 (more on that delay
> in a moment).
>
> Concurrently, we developed a written trademark policy, which you can
> find here:
>
>   https://git-scm.com/trademark
>
> This was started from a template that Conservancy uses and customized by
> Conservancy and the Git PLC.
>
> While the original idea was to prevent people from forking the
> software, breaking compatibility, and still calling it Git, the policy
> covers several other cases.
>
> One is that you can't imply successorship. So you also can't fork the
> software, call it "Git++", and then tell everybody your implementation
> is the next big thing.
>
> Another is that you can't use the mark in a way that implies association
> with or endorsement by the Git project. To some degree this is necessary
> to prevent dilution of the mark for other uses, but there are also cases
> we directly want to prevent.
>
> For example, imagine a software project which is only tangentially
> related to Git. It might use Git as a side effect, or might just be
> "Git-like" in the sense of being a distributed system with chained
> hashes. Let's say as an example that it does backups. We'd prefer it
> not call itself GitBackups. We don't endorse it, and it's just using the
> name to imply association that isn't there. You can come up with similar
> hypotheticals: GitMail that stores mailing list archives in Git, or
> GitWiki that uses Git as a backing store.
>
> Those are all fictitious examples (actually, there _are_ real projects
> that do each of those things, but they gave themselves much more unique
> names). But they're indicative of some of the cases we've seen. I'm
> intentionally not giving the real names here, because my point isn't to
> shame any particular projects, but to discuss general policy.
>
> Careful readers among you may now be wondering about GitHub, GitLab,
> Gitolite, etc. And now we get back to why it took over a year to get the
> trademark granted.
>
> The USPTO initially rejected our application as confusingly similar to
> the existing trademark on GitHub, which was filed in 2008. While one
> might imagine where the "Git" in GitHub comes from, by the time we
> applied to the USPTO, both marks had been widely used in parallel for
> years.  So we worked out an agreement with GitHub which basically says
> "we are mutually OK with the other trademark existing".
>
> (There was another delay caused by a competing application from a
> proprietary version control company that wanted to re-brand portions of
> their system as "GitFocused" (not the real name, but similar in spirit).
> We argued our right to the name and refused to settle; they eventually
> withdrew their application).
>
> So GitHub is essentially outside the scope of the trademark policy, due
> to the history. We also decided to explicitly grandfather some major
> projects that were using similar portmanteaus, but which had generally
> been good citizens of the Git ecosystem (building on Git in a useful
> way, not breaking compatibility). Those include GitLab, JGit, libgit2,
> and some others. The reasoning was generally that it would be a big pain
> for those projects, which have established their own brands, to have to
> switch names. It's hard to hold them responsible for picking a name that
> violated a policy that didn't yet exist.
>
> If the "libgit2" project were starting from scratch today, we'd probably
> ask it to use a different name (because the name may imply that it's an
> official successor). However, we effectively granted permission for this
> use and it would be unfair to disrupt that.
>
> There's one other policy point that has come up: the written policy
> disallows the use of "Git" or the logo on merchandise. This is something
> people have asked about it (e.g., somebody made some Git stress balls,
> and another person was printing keycaps with a Git logo). We have always
> granted it, but wanted to reserve the right in case there was some use
> that we hadn't anticipated that would be confusing or unsavory.
>
> Enforcement of the policy is done as cases are brought to the attention
> of Conservancy and the Git PLC. Sometimes people mail Conservancy
> directly, and sometimes a use is noticed by the Git PLC, which mails
> Conservancy.  In either case, Conservancy's lawyer pings the Git PLC,
> and we decide what to do about it, with advice from the lawyer. The end
> result is usually a letter from the lawyer politely asking them to stop
> using the trademark.
>
> So how does the Git PLC make decisions? We generally try to follow the
> policy in an equitable way, but there are a lot of corner cases. Here
> are some rules of thumb we've worked out:
>
>   - Things that are only tangentially related to Git are out of policy
>     (e.g., if you had a service which rewards bitcoin for people's
>     commits, we'd prefer it not be branded GitRewards).
>
>   - Anything that claims to be Git but does not interoperate is out.
>     We haven't had to use that one yet.
>
>   - Portmanteaus ("GitFoo" or "FooGit") are out. Most of the cases run
>     into this rule. For instance, we asked GitHub to not to use "DGit"
>     to refer to their replicated Git solution, and they[1] rebranded.
>     We also asked "GitTorrent" not to use that name based on this rule.
>
>   - Commands like "git-foo" (so you run "git foo") are generally OK.
>     This is Git's well-known extension mechanism, so it doesn't really
>     imply endorsement (on the other hand, you do not get to complain if
>     you choose too generic a name and conflict with somebody else's use
>     of the same git-foo name).
>
>   - When "git-foo" exists, we've approved "Git Foo" as a matching
>     project name, but we haven't decided on a general rule to cover this
>     case.  The only example here is "Git LFS".
>
> So that's more or less where we're at now.  In my opinion, a few open
> questions are:
>
>   1. Is the portmanteau clause a good idea? GitTorrent is a possibly
>      interesting case there. It's an open source project trying to
>      make a torrent-like protocol for Git. That's something we'd like to
>      have happen. But does the name imply more endorsement than we're
>      willing to give (especially at an early stage)?
>
>   2. Is it a problem that the grandfathering of some names may create a
>      branding advantage? Under the policy today, we wouldn't grant
>      "GitHub" or "GitLab". Does that give an unfair advantage to the
>      incumbents?
>
>      I think the answer is "yes", but the Git PLC is also not sure that
>      there is a good solution. If we'd thought about trademark issues
>      much earlier, we would have been in different circumstances and
>      probably would have made different decisions. But we didn't, so we
>      have to live with how things developed in the meantime.
>
>      Loosening now would be a mistake as it would cause a lot of
>      confusion around the trademark and make it harder for us to stop
>      the uses that we really care about stopping now.
>
>   3. Was granting "Git LFS" the right call? I think the project is a good
>      one and has worked well with the greater Git community. But I think
>      the name has implied some level of "officialness". We obviously
>      need to allow "git-lfs" as a name. But should the policy have said
>      "you can call this LFS, and the command is git-lfs, but don't say
>      'Git LFS'". I'm not sure.
>
>      One option would have been to ask "git-foo" to prefer "Foo for Git"
>      instead of "Git Foo" in their branding (it's too late now for "Git
>      LFS", so this is a hypothetical question for future requests now).
>
>   4. I think the merchandise clause has worked fine, and in general the
>      plan is to grant it in most cases. I have trouble thinking of an
>      item I _wouldn't_ want the Git logo on, and I'd rather err on the
>      side of permissiveness than be the arbiter of taste. And having the
>      Git logo on merchandise generally raises awareness of Git.
>
>      But perhaps people have stronger opinions (either about the type of
>      item, or perhaps the practices of the manufacturer producing it).
>      It's hard to predict how a particular item would impact how people
>      see the Git brand.
>
> -Peff
>
> [1] I used "they" to refer to GitHub, but as many of you know, I am also
>     employed by GitHub. If you are wondering how that works, I generally
>     abstain from any decisions regarding GitHub (and that includes the
>     "Git LFS" decision, which was a project started by GitHub). That
>     leaves two voting PLC members for those decisions; Conservancy gets
>     a tie-breaking vote, but it has never come up.



Is "Gitter" allowed?   (https://gitter.im/).

More info here:

https://en.wikipedia.org/wiki/Gitter

Also, their twitter handle is @gitchat.

Not sure I'd even classify "gitter" as a portmanteau.



- Sylvie

^ permalink raw reply

* Re: Partnership with Git
From: Dov Grobgeld @ 2017-02-21 15:04 UTC (permalink / raw)
  To: Nikita Malikov; +Cc: Konstantin Khomoutov, git
In-Reply-To: <D0D8D50D4A2E47568029FE8BC52C3DC4@datasoft.local>

Nikita,

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". If your want to use git in your products,
you are free to do so without asking, and if you need technical advice for
how to go about integrating git in your products, you can either ask here,
on stackoverflow, (and probably lots of other places) or hire a consultant
that can help you.

Hope this helps,
Dov

On Tue, Feb 21, 2017 at 4:08 PM, Nikita Malikov <nikitam@devart.com> wrote:
>
> Konstantin,
>
> My goal is to establish partnership relations with Git because some of
> Devart's products support Git version control system (for example dbForge
> Studio for SQL Server https://www.devart.com/dbforge/sql/studio/).
> My team and I would be glad to come up with cross-marketing activities
> between Devart and Git. We think that some users of our development products
> would be interested to switch to Git version control system.
>
> Best regards
> Nikita Malikov
>
> --------------------------------------------------
> From: "Konstantin Khomoutov" <kostix+git@007spb.ru>
> Sent: 21 February, 2017 14:22
> To: "Nikita Malikov" <nikitam@devart.com>
> Cc: <git@vger.kernel.org>
> Subject: Re: Partnership with Git
>
>
>> On Tue, 21 Feb 2017 13:21:38 +0200
>> "Nikita Malikov" <nikitam@devart.com> wrote:
>>
>>> My name is Nikita and I'm from Devart https://www.devart.com/.
>>> I would like to contact someone from executive staff of Git. I would
>>> be very thankful for some information how to do this or if someone
>>> contacts me.
>>
>>
>> Git is a free software volunteer project and as such, it has no
>> "executive staff" in the sense enterprises put into it.
>>
>> Can you maybe state your actual goals please?
>>
>> Say, if you're looking for commercial support, we could possibly
>> suggests a couple of pointers.
>>
>>
>
>

^ permalink raw reply

* Re: Partnership with Git
From: Nikita Malikov @ 2017-02-21 14:08 UTC (permalink / raw)
  To: Konstantin Khomoutov; +Cc: git
In-Reply-To: <20170221152216.be2b8401f65650bf766cdd8d@domain007.com>


Konstantin,

My goal is to establish partnership relations with Git because some of 
Devart's products support Git version control system (for example dbForge 
Studio for SQL Server https://www.devart.com/dbforge/sql/studio/).
My team and I would be glad to come up with cross-marketing activities 
between Devart and Git. We think that some users of our development products 
would be interested to switch to Git version control system.

Best regards
Nikita Malikov

--------------------------------------------------
From: "Konstantin Khomoutov" <kostix+git@007spb.ru>
Sent: 21 February, 2017 14:22
To: "Nikita Malikov" <nikitam@devart.com>
Cc: <git@vger.kernel.org>
Subject: Re: Partnership with Git

> On Tue, 21 Feb 2017 13:21:38 +0200
> "Nikita Malikov" <nikitam@devart.com> wrote:
>
>> My name is Nikita and I'm from Devart https://www.devart.com/.
>> I would like to contact someone from executive staff of Git. I would
>> be very thankful for some information how to do this or if someone
>> contacts me.
>
> Git is a free software volunteer project and as such, it has no
> "executive staff" in the sense enterprises put into it.
>
> Can you maybe state your actual goals please?
>
> Say, if you're looking for commercial support, we could possibly
> suggests a couple of pointers.
>
> 



^ permalink raw reply

* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Duy Nguyen @ 2017-02-21 13:25 UTC (permalink / raw)
  To: Michael Haggerty
  Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <25fcb527-595a-7865-41e3-ee7c4c1ad668@alum.mit.edu>

On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>> Since submodule or not is irrelevant to files-backend after the last
>> patch, remove the parameter from files_downcast() and kill
>> files_assert_main_repository().
>>
>> PS. This implies that all ref operations are allowed for submodules. But
>> we may need to look more closely to see if that's really true...
>
> I think you are jumping the gun here.
>
> Even after your changes, there is still a significant difference between
> the main repository and submodules: we have access to the object
> database for the main repository, but not for submodules.

I did jump the gun for another reason: files-backend makes function
calls to the frontend, which unconditionally target the main ref store
(e.g. resolve_ref_unsafe, delete_ref...). Of course, because
store-aware api does not exist. My decision (off-list) to add
test-ref-store was the right call. I would not have seen these because
I was not (and still am not) familiar with files-backend.c enough to
see its dark corners.

files-backend.c is not all unicorn and rainbow :(
-- 
Duy

^ permalink raw reply

* AW: Inconsistent results of git blame --porcelain when detecting copies from other files
From: Sokolov, Konstantin @ 2017-02-21 12:25 UTC (permalink / raw)
  To: peff@peff.net, gitster@pobox.com; +Cc: git@vger.kernel.org
In-Reply-To: <20170220221540.6vemjdvyvwonpqyt@sigill.intra.peff.net>

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.

Regards
Konstantin

-----Ursprüngliche Nachricht-----
Von: Jeff King [mailto:peff@peff.net]
Gesendet: Montag, 20. Februar 2017 23:16
An: Junio C Hamano
Cc: Sokolov, Konstantin (ext) (CT RDA SSI ADM-DE); git@vger.kernel.org
Betreff: Re: Inconsistent results of git blame --porcelain when detecting copies from other files

On Mon, Feb 20, 2017 at 01:30:29PM -0800, Junio C Hamano wrote:

> "Sokolov, Konstantin" <konstantin.sokolov.ext@siemens.com> writes:
>
> > However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:
> > ...
> > This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.
>
> Hmph, this sounds vaguely familiar with
>
>
> http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.int
> ra.peff.net
>
> which is part of Git 2.12-rc0

Yeah, I had the same thought while reading Konstantin's report.

I'm not sure Git is wrong, though. I think it's just the way the porcelain output works.

Here's a minimal reproduction; the interesting thing is when a commit is mentioned twice (as happens on lines 1 and 5 here):

  git init repo
  cd repo

  # use long lines to make sure we trigger content-movement detection
  for i in $(seq 1 5); do
        echo this is really long line number $i
  done >file
  git add file
  git commit -m initial

  sed 's/1/one/; s/5/five/' <file >renamed
  git rm file
  git add renamed
  git commit -m 'rename and use english'

  git blame renamed
  git blame --line-porcelain renamed
  git blame --porcelain renamed

The first blame output looks something like this:

  bab03701 renamed ... line number 1
  ^dda1349 file    ... line number 2
  ^dda1349 file    ... line number 3
  ^dda1349 file    ... line number 4
  bab03701 renamed ... line number 5

so we can see it's the same case. The --line-porcelain similarly matches the commits and filenames.

But the --porcelain output is:

  bab037010dcabaf0509db27bf232d25659b180fa 1 1 1
  ...
  filename renamed
          this is really long line number one
  dda1349d41da859f4c37e018dbed714ba6c1aa18 2 2 3
  ...
  filename file
          this is really long line number 2
  dda1349d41da859f4c37e018dbed714ba6c1aa18 3 3
          this is really long line number 3
  dda1349d41da859f4c37e018dbed714ba6c1aa18 4 4
          this is really long line number 4
  bab037010dcabaf0509db27bf232d25659b180fa 5 5 1
          this is really long line number five

You might be tempted to say that the fifth line comes from "filename file", because that was the last "filename" entry we saw. But that's _not_ how the porcelain output works. That "filename" entry was associated with dda1349, but the line comes from bab0370 here.

The simplest way (IMHO) to parse --porcelain output is:

  - maintain a mapping of commit sha1s to the commit's details

  - whenever you see a "<sha1> <line_nr> <orig_nr> [<size-of-hunk>]"
    line, any key-value fields which follow impact _only_ that sha1, and
    you should update the details for that map entry

  - when you see the actual tab-indented line content, you have gotten
    all of the key-value updates for that sha1. You can now safely do
    what you like with the line entry.

Another way, if you don't want to update your mapping, is to actually pay attention to the size-of-hunk headers. In this case the middle three lines come in their own hunk (which you can see from the "2 2 3" header on the second line). The "filename" field we get applies to that hunk, but once we switch to a different one, the filename field needs to be looked up in the commit mapping.

But it's definitely not correct to blindly apply one "filename" field to subsequent lines in other hunks.

And yes, I do think this is probably more complex than it needs to be.
I didn't write it. And I don't think it is worth the backwards compatibility headache of trying to change it now. It's possible this could be better documented (I didn't look at the documentation to write that explanation; I happened to puzzle it out for somebody else recently who had a similar case. That's what led to the bug-fix in the message you linked).

-Peff

^ permalink raw reply

* Re: Partnership with Git
From: Konstantin Khomoutov @ 2017-02-21 12:22 UTC (permalink / raw)
  To: Nikita Malikov; +Cc: git
In-Reply-To: <02FFA5DBB1C64A8DA1BF45F11EBF9930@datasoft.local>

On Tue, 21 Feb 2017 13:21:38 +0200
"Nikita Malikov" <nikitam@devart.com> wrote:

> My name is Nikita and I'm from Devart https://www.devart.com/.
> I would like to contact someone from executive staff of Git. I would
> be very thankful for some information how to do this or if someone
> contacts me.

Git is a free software volunteer project and as such, it has no
"executive staff" in the sense enterprises put into it.

Can you maybe state your actual goals please?

Say, if you're looking for commercial support, we could possibly
suggests a couple of pointers.

^ permalink raw reply

* Partnership with Git
From: Nikita Malikov @ 2017-02-21 11:21 UTC (permalink / raw)
  To: git


Hello,

My name is Nikita and I'm from Devart https://www.devart.com/.
I would like to contact someone from executive staff of Git. I would be very 
thankful for some information how to do this or if someone contacts me.

Thanks for attention and sorry if I disturbed someone.

Best regards
Nikita Malikov 




^ permalink raw reply

* [RFC] Subtle differences in passing configs to git clone
From: Lars Schneider @ 2017-02-21 11:36 UTC (permalink / raw)
  To: Git List; +Cc: Jeff King

Hi,

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. 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?

Thanks,
Lars

^ permalink raw reply

* Not expected merge conflict output
From: KES @ 2017-02-21  9:24 UTC (permalink / raw)
  To: git

Hi. I have merge conflict and this output:

--- a/crypto/lib/Crypto/Routes.pm
+++ b/crypto/lib/Crypto/Routes.pm
@@@ -98,17 -94,16 +98,36 @@@ sub register 
          ,payment_cancel_landing  =>  '/payment-cancel'
      }});
      # Route configuration is moved from plugin:
++<<<<<<< ours
 +    $rn->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_payment_s
 +    $rn->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_payment_c
 +    $rn->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
 +    $rn->post( '/stripe/payment/create', { required_level => 'user' } )->to( 'Stripe#payment_creat
 +    $rn->post( '/stripe/payment/execute', { required_level => 'user' } )->to( 'Stripe#payment_exec
++||||||| base
++    $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_paymen
++    $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_paymen
++    $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
++    $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  )->name( 'stripe_paymen
++    $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' )->name( 'stripe_paymen
++
++    $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
++=======
+     $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_paymen
+     $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_paymen
+     $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
+     $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  )->name( 'stripe_paymen
+     $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' )->name( 'stripe_paymen
+ 
+     $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
+     $guest->get ( '/stripe/:form' )->to( 'Stripe#button' );
++>>>>>>> theirs
  
 +    $rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
  
 +    # Static routes
 +    # QR code dowlnload url
 +    $app->routes->get('/uploads/qrcode/:file')->name('qr_url');


But I expect this output:

--- a/crypto/lib/Crypto/Routes.pm
+++ b/crypto/lib/Crypto/Routes.pm
@@@ -98,17 -94,16 +98,36 @@@ sub register 
          ,payment_cancel_landing  =>  '/payment-cancel'
      }});
      # Route configuration is moved from plugin:
++<<<<<<< ours
 +    $rn->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_payment_s
 +    $rn->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_payment_c
 +    $rn->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
 +    $rn->post( '/stripe/payment/create', { required_level => 'user' } )->to( 'Stripe#payment_creat
 +    $rn->post( '/stripe/payment/execute', { required_level => 'user' } )->to( 'Stripe#payment_exec
  
 +    $rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
++||||||| base
++    $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_paymen
++    $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_paymen
++    $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
++    $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  )->name( 'stripe_paymen
++    $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' )->name( 'stripe_paymen
++
++    $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
++=======
+     $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' )->name( 'stripe_paymen
+     $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  )->name( 'stripe_paymen
+     $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' );
+     $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  )->name( 'stripe_paymen
+     $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' )->name( 'stripe_paymen
+ 
+     $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
+     $guest->get ( '/stripe/:form' )->to( 'Stripe#button' );
++>>>>>>> theirs
  
 +    # Static routes
 +    # QR code dowlnload url
 +    $app->routes->get('/uploads/qrcode/:file')->name('qr_url');

Because this diff has less difference between ours&&base, Also 

This (second expected patch)
 +    # Static routes
 +    # QR code dowlnload url
 +    $app->routes->get('/uploads/qrcode/:file')->name('qr_url');
is less in compare to (first not expected):
  
 +    $rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
  
 +    # Static routes
 +    # QR code dowlnload url
 +    $app->routes->get('/uploads/qrcode/:file')->name('qr_url');


Did I wrong and first output is right thing or maybe I right and I should open new bug report.
Please explain me

^ permalink raw reply

* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Jeff King @ 2017-02-21  7:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
In-Reply-To: <xmqqino5jia8.fsf@gitster.mtv.corp.google.com>

On Mon, Feb 20, 2017 at 01:58:07AM -0800, Junio C Hamano wrote:

> Since nothing seems to have happened in the meantime, here is what
> I'll queue so that we won't forget for now.  Lars's tests based on
> how the scripted "git submodule" uses "git config" may still be
> valid, but it is somewhat a roundabout way to demonstrate the
> breakage and not very effective way to protect the fix, so I added a
> new test that directly tests "git -c <var>=<val> <command>".

Yeah, I agree that is the best way to cover this code.

> I am not sure if this updated one is worth doing, or the previous
> "strchr and strrchr" I originally wrote was easier to understand.

I think either is OK. I would actually have written it halfway in
between, like:

  static void canonicalize_config_variable_name(char *varname)
  {
          const char *p;

          /* downcase the first segment */
          for (p = varname; *p; p++) {
	          if (*p == '.')
		          break;
                  *p = tolower(*p);
	  }

          /* locate end of middle segment, if there is one */
          p = strrchr(p, '.');
          if (!p)
                  return; /* invalid single-level var, but let it pass */
          for (; *p; p++)
                  *p = tolower(*p);
  }

You can toss that on the bikeshed heap. :)

The other change from yours is that it flips the order of the strrchr
and return. Yours is more efficient in the sense that we know there is
no dot, so we do not have to keep searching at all (though of course
this case is invalid and we would not expect to see it in practice).

But it did take me a minute in yours to figure out why last_dot was
always set to a dot (the answer is that it starts at "cp", which must
itself be a dot, because we would already have returned otherwise).

> 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.

I don't think that was intentional. We just never caught the case. It
might be reasonable to do so (and it's easy to catch here while
canonicalizing). I think there are probably some other cases we _could_
catch, too (e.g., invalid characters for keynames). But I'm not excited
about duplicating the logic from the file-parser.

> Perhaps we should, but it is not the focus of this topic.

Definitely.

> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
> new file mode 100755
> index 0000000000..acb8dc3b15
> --- /dev/null
> +++ b/t/t1351-config-cmdline.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='git -c var=val'
> +
> +. ./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.

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".

> +	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

Looks good.

> +test_expect_success 'last one wins: three level vars' '
> +	echo val >expect &&
> +
> +	# 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.  Test both setting and querying.
> +
> +	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 &&
> +
> +	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
> +'

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?

Other than that nit, the tests look good to me.

-Peff

^ permalink raw reply

* Re: [PATCH v2 0/4] delete_ref: support reflog messages
From: Junio C Hamano @ 2017-02-21  7:18 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: Jeff King, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>

Kyle Meyer <kyle@kyleam.com> writes:

> Kyle Meyer (4):
>   delete_ref: accept a reflog message argument
>   update-ref: pass reflog message to delete_ref()
>   rename_ref: replace empty message in HEAD's log
>   branch: record creation of renamed branch in HEAD's log

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'.

Thanks.

^ permalink raw reply

* Re: [PATCH v2 0/4] delete_ref: support reflog messages
From: Jeff King @ 2017-02-21  7:12 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>

On Mon, Feb 20, 2017 at 08:10:31PM -0500, Kyle Meyer wrote:

> Kyle Meyer (4):
>   delete_ref: accept a reflog message argument
>   update-ref: pass reflog message to delete_ref()
>   rename_ref: replace empty message in HEAD's log
>   branch: record creation of renamed branch in HEAD's log

These look good to me. I think you did a nice job of summarizing the
discussion in the commit messages.

-Peff

^ permalink raw reply

* Re: url.<base>.insteadOf vs. submodules
From: Jeff King @ 2017-02-21  7:06 UTC (permalink / raw)
  To: Toolforger; +Cc: git
In-Reply-To: <28fb85d4-89cd-1f32-3063-2f48d8b935be@durchholz.org>

On Tue, Feb 21, 2017 at 06:11:51AM +0100, Toolforger wrote:

> > I'm not sure I understand. You have a project policy to use certain
> > URLs. But you, the user, want to override that. Why isn't the
> > user-specific config file the right place to put that?
> 
> Ah right, I mistook ~/ for "project root" instead of "home dir".
> Sorry for the confusion.

Ah, OK, that makes more sense.

> > (I think there _is_ a mismatch, in that the change is specific not just
> > to your user, but to the repo. So you would not want to rewrite other
> > references to the same URL in other repos.
> 
> Indeed, and that's actually a problem.
> 
> The setup I'm aiming for is
>   github -> local bare repo -> local clones with worktrees
> 
> If I place insteadOf rules in ~/.gitconfig, I will be unable to pull from
> github to my local bare repos.
> Mmm... I could try to undo the insteadOf configuration from ~/.gitconfig in
> the local bare repos. Not sure whether I have to redirect from the github
> URL to itself.

Yeah, I think you would probably have to do a redirect-to-self to
override the global one.

At one point we discussed having conditional-config that would kick in
based on path-matching. I think it would be another way to do what you
want, but there's nothing merged.

I think anything involving ~/.gitconfig is basically a hack, though.
What you really want is for submodules to better support your
URL-rewriting case, and that's not an unreasonable thing to want.

We'll see if the submodule folks have any ideas on how to implement
that.

-Peff

^ permalink raw reply

* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Junio C Hamano @ 2017-02-21  6:36 UTC (permalink / raw)
  To: Matt McCutchen; +Cc: git
In-Reply-To: <1487470080.3570.8.camel@mattmccutchen.net>

Matt McCutchen <matt@mattmccutchen.net> writes:

> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1.  The more common case
> of requesting a nonexistent ref normally triggers a die() in
> get_fetch_map, so "git fetch" wasn't bothering to check after the fetch
> that it got all the refs it sought, like "git fetch-pack" does near the
> end of cmd_fetch_pack.
>
> Move the code from cmd_fetch_pack to a new function,
> report_unmatched_refs, that is called by fetch_refs_via_pack as part of
> "git fetch".  Also, change filter_refs (which checks whether a request
> for an unadvertised object should be sent to the server) to set a new
> match status on the "struct ref" when the request is not allowed, and
> have report_unmatched_refs check for this status and print a special
> error message, "Server does not allow request for unadvertised object".
>
> Finally, add a simple test case for "git fetch REMOTE SHA1".
>
> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> ---

Hmph, I would have expected this to be done as a three-patch series,

 * move the loop at the end of cmd_fetch_pack() to a separate helper
   function report_unmatched_refs() and call it;

 * add a call to report_unmatched_refs() to the transport layer;

 * enhance report_unmatched_refs() by introducing match_status
   field and adding new code to filter_refs() to diagnose other
   kinds of errors.

The result looks reasonable from a cursory read, though.

Thanks for following it up to the completion.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox