Git development
 help / color / mirror / Atom feed
* Re: Merge conflicts in .gitattributes can cause trouble
From: Johannes Schindelin @ 2016-10-18 12:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Lars Schneider, git, Jeff King, me
In-Reply-To: <xmqqvawqvp6y.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 17 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I would vote for:
> >
> > 4. We keep letting Git read in the *current* version of .gitattributes
> >    *before* the merge, and apply those attributes while performing the
> >    merge.
> 
> Even though this needs a major surgery to the way the attr subsystem
> reads from these files, I think it is conceptually the cleanest.

To the contrary. As far as I can see, when calling `git merge`, Git
currently *does* read .gitattributes from the file, and if that fails,
falls back to reading that file from the index.

In other words, option 4. is the current behavior no change required.

Ciao,
Dscho

^ permalink raw reply

* Git log exclude/remotes/branches options not working as expected
From: Robert Dailey @ 2016-10-18 13:33 UTC (permalink / raw)
  To: Git

I have 3 remotes registered in my clone:

origin, fork, drive

When I do:

$ git log --oneline --decorate --graph

I only want to see branches under:

refs/heads/*
refs/remotes/origin/*

I tried the following:

$ git log --oneline --decorate --graph --simplify-by-decoration
--remote=origin topic1..master

However, I still see refs present in the graph for 'drive' and 'fork'
remote tracking branches. I can't tell if these are shown simply
because other refs not excluded by my options happen to also be at
that SHA1, or if the log command is still generating the graph based
on other branches.

What I'm expecting is that I literally see NONE of those excluded refs
on the graph, even if other included refs also happen to be positioned
at those commits. It's the visualization of the refs I'm concerned
about: I have a lot of remote tracking branches in those remotes that
clutter the view; I'd rather not see them at all (in addition to the
graph not considering them when it is being built/generated). Am I
misunderstanding the purpose here? How can I achieve my goals?
Documentation hasn't really helped me out here.

^ permalink raw reply

* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Santiago Torres @ 2016-10-18 13:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1szm8ukf.fsf@gitster.mtv.corp.google.com>

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

> * st/verify-tag (2016-10-10) 7 commits
>  - t/t7004-tag: Add --format specifier tests
>  - t/t7030-verify-tag: Add --format specifier tests
>  - builtin/tag: add --format argument for tag -v
>  - builtin/verify-tag: add --format to verify-tag
>  - tag: add format specifier to gpg_verify_tag
>  - ref-filter: add function to print single ref_array_item
>  - gpg-interface, tag: add GPG_VERIFY_QUIET flag
> 
>  "git tag" and "git verify-tag" learned to put GPG verification
>  status in their "--format=<placeholders>" output format.
> 
>  Is this ready for 'next'?

Hi, I saw this on the previous "what's cooking." Is there anything I
need to do on my side to make sure this is ready for next?

Thanks!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Luke Shumaker @ 2016-10-18 15:06 UTC (permalink / raw)
  To: git; +Cc: pclouds, peff, Luke Shumaker

The superficial aspect of this change is that git-daemon now allows paths
that start with a "~".  Previously, if git-daemon was run with
"--base-path=/srv/git", it was impossible to get it to serve
"/srv/git/~foo/bar.git".  An odd edge-case that was broken.

But from a source-code standpoint, the change is in path.c:enter_repo().  I
have adjusted it to take separate "strict_prefix" and "strict_suffix"
arguments, rather than a single "strict" argument.

I also make it clearer what the purpose of each path buffer is for, by
renaming them to chdir_path and ret_path; chdir_path is the path that we
pass to chdir(); return_path is the path we return to the user.  Using this
nomenclature, we can more easily explain the behavior of the function.
There are 3 DWIM measures that enter_repo() provides: tilde expansion,
suffix guessing, and gitfile expansion; it also trims trailing slashes.
Here is how they are applied to each path:

    +------------------------------+----------------+----------------+
    | Before this commit           | chdir_path     | ret_path       |
    +------------------------------+----------------+----------------+
    | trim trailing slashes        | !strict        | !strict        |
    | tilde expansion              | !strict        | false          |
    | suffix guessing              | !strict        | !strict        |
    | gitfile expansion (< 2.6.3)  | !strict        | false          |
    | gitfile expansion (>= 2.6.3) | true           | strict         |
    +------------------------------+----------------+----------------+
    | With this commit             | chdir_path     | ret_path       |
    +------------------------------+----------------+----------------+
    | trim trailing slashes        | true           | true           |
    | tilde expansion              | !strict_prefix | false          |
    | suffix guessing              | !strict_suffix | !strict_suffix |
    | gitfile expansion            | true           | false          |
    +------------------------------+----------------+----------------+

The separation of "strict" into "strict_prefix" and "strict_suffix" is
necessary for git-daemon because it has separate --strict-paths (affects
prefix and suffix) and --user-path (just prefix) flags that can be toggled
separately.

In the other programs where enter_repo() is called, I continued the
existing behavior of tying the prefix and suffix strictness together
together; though I am not entirely sure that they should all be enabling
tilde expansion.  But for now, their behavior hasn't changed.

Signed-off-by: Luke Shumaker <lukeshu@sbcglobal.net>
---
 builtin/receive-pack.c   |   2 +-
 builtin/upload-archive.c |   2 +-
 cache.h                  |   2 +-
 daemon.c                 |  42 +++++++--------
 http-backend.c           |   2 +-
 path.c                   | 135 +++++++++++++++++++++++++----------------------
 upload-pack.c            |   2 +-
 7 files changed, 96 insertions(+), 91 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 011db00..f430e96 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1860,7 +1860,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 	setup_path();
 
-	if (!enter_repo(service_dir, 0))
+	if (!enter_repo(service_dir, 0, 0))
 		die("'%s' does not appear to be a git repository", service_dir);
 
 	git_config(receive_pack_config, NULL);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2caedf1..00d4ced 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -25,7 +25,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		usage(upload_archive_usage);
 
-	if (!enter_repo(argv[1], 0))
+	if (!enter_repo(argv[1], 0, 0))
 		die("'%s' does not appear to be a git repository", argv[1]);
 
 	/* put received options in sent_argv[] */
diff --git a/cache.h b/cache.h
index 4cba08e..6380be0 100644
--- a/cache.h
+++ b/cache.h
@@ -1024,7 +1024,7 @@ enum scld_error safe_create_leading_directories_const(const char *path);
 
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
-const char *enter_repo(const char *path, int strict);
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix);
 static inline int is_absolute_path(const char *path)
 {
 	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
diff --git a/daemon.c b/daemon.c
index 425aad0..118d337 100644
--- a/daemon.c
+++ b/daemon.c
@@ -170,27 +170,23 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		return NULL;
 	}
 
-	if (*dir == '~') {
-		if (!user_path) {
-			logerror("'%s': User-path not allowed", dir);
-			return NULL;
-		}
-		if (*user_path) {
-			/* Got either "~alice" or "~alice/foo";
-			 * rewrite them to "~alice/%s" or
-			 * "~alice/%s/foo".
-			 */
-			int namlen, restlen = strlen(dir);
-			const char *slash = strchr(dir, '/');
-			if (!slash)
-				slash = dir + restlen;
-			namlen = slash - dir;
-			restlen -= namlen;
-			loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
-			snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
-				 namlen, dir, user_path, restlen, slash);
-			dir = rpath;
-		}
+	if (*dir == '~' && *user_path && user_path[0] != '\0') {
+		/* Got either "~alice" or "~alice/foo";
+		 * rewrite them:
+		 *
+		 * ~alice     -> ~alice/user_dir
+		 * ~alice/foo -> ~alice/user_dir/foo
+		 */
+		int namlen, restlen = strlen(dir);
+		const char *slash = strchr(dir, '/');
+		if (!slash)
+			slash = dir + restlen;
+		namlen = slash - dir;
+		restlen -= namlen;
+		loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
+		snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
+		         namlen, dir, user_path, restlen, slash);
+		dir = rpath;
 	}
 	else if (interpolated_path && hi->saw_extended_args) {
 		struct strbuf expanded_path = STRBUF_INIT;
@@ -223,14 +219,14 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		dir = rpath;
 	}
 
-	path = enter_repo(dir, strict_paths);
+	path = enter_repo(dir, strict_paths || !user_path, strict_paths);
 	if (!path && base_path && base_path_relaxed) {
 		/*
 		 * if we fail and base_path_relaxed is enabled, try without
 		 * prefixing the base path
 		 */
 		dir = directory;
-		path = enter_repo(dir, strict_paths);
+		path = enter_repo(dir, strict_paths || !user_path, strict_paths);
 	}
 
 	if (!path) {
diff --git a/http-backend.c b/http-backend.c
index adc8c8c..d71ed81 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -697,7 +697,7 @@ int cmd_main(int argc, const char **argv)
 		not_found(&hdr, "Request not supported: '%s'", dir);
 
 	setup_path();
-	if (!enter_repo(dir, 0))
+	if (!enter_repo(dir, 0, 0))
 		not_found(&hdr, "Not a git repository: '%s'", dir);
 	if (!getenv("GIT_HTTP_EXPORT_ALL") &&
 	    access("git-daemon-export-ok", F_OK) )
diff --git a/path.c b/path.c
index fe3c4d9..636349e 100644
--- a/path.c
+++ b/path.c
@@ -646,96 +646,105 @@ char *expand_user_path(const char *path)
 }
 
 /*
- * First, one directory to try is determined by the following algorithm.
+ * chdir() to, and set_git_dir() to the directory found with "path", using the
+ * following algorithm.
  *
- * (0) If "strict" is given, the path is used as given and no DWIM is
- *     done. Otherwise:
- * (1) "~/path" to mean path under the running user's home directory;
- * (2) "~user/path" to mean path under named user's home directory;
- * (3) "relative/path" to mean cwd relative directory; or
- * (4) "/absolute/path" to mean absolute directory.
+ * (0) "relative/path" to mean cwd relative directory; or
+ * (1) "/absolute/path" to mean absolute directory.
+ * (2) trim trailing slashes
  *
- * Unless "strict" is given, we check "%s/.git", "%s", "%s.git/.git", "%s.git"
- * in this order. We select the first one that is a valid git repository, and
- * chdir() to it. If none match, or we fail to chdir, we return NULL.
+ * Unless "strict_prefix" is given:
+ * (3) "~/path" to mean path under the running user's home directory;
+ * (4) "~user/path" to mean path under named user's home directory;
  *
- * If all goes well, we return the directory we used to chdir() (but
- * before ~user is expanded), avoiding getcwd() resolving symbolic
- * links.  User relative paths are also returned as they are given,
- * except DWIM suffixing.
+ * Unless "strict_suffix" is given:
+ * (5) check "%s/.git", "%s", "%s.git/.git", "%s.git" in this order. We select
+ *     the first one that is a valid git repository, and chdir() to it. If none
+ *     match, we return NULL.
+ *
+ * And then, unconditionally:
+ * (6) If the result is a .git file (instead of a directory) that points to a
+ *     directory elsewhere, follow it.
+ *
+ * If all goes well, we return the input path with suffix alteration (steps 2,
+ * 5, 6) applied, but without prefix alteration (user paths) applied. The
+ * returned value is a pointer to a static buffer.
  */
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix)
 {
-	static struct strbuf validated_path = STRBUF_INIT;
-	static struct strbuf used_path = STRBUF_INIT;
+	/* chdir_path is the path we chdir() to */
+	static struct strbuf chdir_path = STRBUF_INIT;
+	/* ret_path is the path we return */
+	static struct strbuf ret_path = STRBUF_INIT;
+
+	int len;
+	const char *gitfile;
 
 	if (!path)
 		return NULL;
 
-	if (!strict) {
+	len = strlen(path);
+
+	/* strip trailing slashes */
+	while ((1 < len) && (path[len-1] == '/'))
+		len--;
+
+	/*
+	 * We can handle arbitrary-sized buffers, but this remains as a
+	 * sanity check on untrusted input.
+	 */
+	if (PATH_MAX <= len)
+		return NULL;
+
+	strbuf_reset(&chdir_path);
+	strbuf_add(&chdir_path, path, len);
+	strbuf_reset(&ret_path);
+	strbuf_add(&ret_path, path, len);
+
+	if (!strict_prefix && chdir_path.buf[0] == '~') {
+		/* operate only on chdir_path */
+		char *newpath = expand_user_path(chdir_path.buf);
+		if (!newpath)
+			return NULL;
+		strbuf_attach(&chdir_path, newpath, strlen(newpath),
+		              strlen(newpath));
+	}
+
+	if (!strict_suffix) {
+		/* operate on both chdir_path and ret_path */
 		static const char *suffix[] = {
 			"/.git", "", ".git/.git", ".git", NULL,
 		};
-		const char *gitfile;
-		int len = strlen(path);
 		int i;
-		while ((1 < len) && (path[len-1] == '/'))
-			len--;
-
-		/*
-		 * We can handle arbitrary-sized buffers, but this remains as a
-		 * sanity check on untrusted input.
-		 */
-		if (PATH_MAX <= len)
-			return NULL;
-
-		strbuf_reset(&used_path);
-		strbuf_reset(&validated_path);
-		strbuf_add(&used_path, path, len);
-		strbuf_add(&validated_path, path, len);
-
-		if (used_path.buf[0] == '~') {
-			char *newpath = expand_user_path(used_path.buf);
-			if (!newpath)
-				return NULL;
-			strbuf_attach(&used_path, newpath, strlen(newpath),
-				      strlen(newpath));
-		}
 		for (i = 0; suffix[i]; i++) {
 			struct stat st;
-			size_t baselen = used_path.len;
-			strbuf_addstr(&used_path, suffix[i]);
-			if (!stat(used_path.buf, &st) &&
+			size_t baselen = chdir_path.len;
+			strbuf_addstr(&chdir_path, suffix[i]);
+			if (!stat(chdir_path.buf, &st) &&
 			    (S_ISREG(st.st_mode) ||
-			    (S_ISDIR(st.st_mode) && is_git_directory(used_path.buf)))) {
-				strbuf_addstr(&validated_path, suffix[i]);
+			     (S_ISDIR(st.st_mode) && is_git_directory(chdir_path.buf)))) {
+				strbuf_addstr(&ret_path, suffix[i]);
 				break;
 			}
-			strbuf_setlen(&used_path, baselen);
+			strbuf_setlen(&chdir_path, baselen);
 		}
 		if (!suffix[i])
 			return NULL;
-		gitfile = read_gitfile(used_path.buf);
-		if (gitfile) {
-			strbuf_reset(&used_path);
-			strbuf_addstr(&used_path, gitfile);
-		}
-		if (chdir(used_path.buf))
-			return NULL;
-		path = validated_path.buf;
 	}
-	else {
-		const char *gitfile = read_gitfile(path);
-		if (gitfile)
-			path = gitfile;
-		if (chdir(path))
-			return NULL;
+
+	gitfile = read_gitfile(chdir_path.buf);
+	if (gitfile) {
+		/* operate only on chdir_path */
+		strbuf_reset(&chdir_path);
+		strbuf_addstr(&chdir_path, gitfile);
 	}
+	if (chdir(chdir_path.buf))
+		return NULL;
 
 	if (is_git_directory(".")) {
 		set_git_dir(".");
 		check_repository_format();
-		return path;
+		return ret_path.buf;
 	}
 
 	return NULL;
diff --git a/upload-pack.c b/upload-pack.c
index ca7f941..c73b776 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -859,7 +859,7 @@ int cmd_main(int argc, const char **argv)
 
 	dir = argv[0];
 
-	if (!enter_repo(dir, strict))
+	if (!enter_repo(dir, strict, strict))
 		die("'%s' does not appear to be a git repository", dir);
 
 	git_config(upload_pack_config, NULL);
-- 
2.10.0


^ permalink raw reply related

* git checkout crashes after server being updated to Debian X86_64
From: Raffael Reichelt @ 2016-10-18 15:17 UTC (permalink / raw)
  To: git

Hello! 

I have a serious problem with git, After my provider had updated to a X86_64 architecture git crashes with various memory-related errors. This is happening remote when pushing to the repository from my local machine as well as trying it on a shell on the server itself.

This are the error-messages:

fatal: Out of memory, realloc failed
fatal: recursion detected in die handler
fatal: recursion detected in die handler

or
fatal: unable to create threaded lstat
fatal: recursion detected in die handler
or
fatal: unable to create threaded lstat
*** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
fatal: recursion detected in die handler
Aborted

It’s obviously not a problem of the repository - happens with all of them. I think it is also not a question of size - happens with a 80M Repository as well as with a 500M one.

Any way: did a 

git fsck
Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
Prüfe Objekte: 100% (56305/56305), Fertig.

git gc --auto --prune=today —aggressive
git repack

Additionally I played around some config parameters  so my config now looks like:
[http]
    postbuffer = 524288000
[pack]
    threads = 1
    deltaCacheSize = 128m
    packSizeLimit = 128m
    windowMemory = 128m
[core]
    packedGitLimit = 128m
    packedGitWindowSize = 128m
    repositoryformatversion = 0
    filemode = true
    bare = true

I am running 
git version 2.1.4
 
on
Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux

Anyone out there to help me getting out of this trouble?

Regards,
Raffael


^ permalink raw reply

* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 15:24 UTC (permalink / raw)
  To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <20161017221623.7299-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Currently a URL for the superproject ending in
>
> (A)    .../path/to/dir
> (B)    .../path/to/dir/
> (C)    .../path/to/dir/.
> (D)    .../path/to/dir/./.
> (E)    .../path/to/dir/.///.//.
>
> is treated the same in (A) and (B), but (C, D, E) are different.

You may know what you meant to say with "treated", but the readers
do not know "treated" in what situation you are talking about.  We
need to tell the readers that the bug being fixed is about resolving
a relative URL "../<something>" off of the URL of the superproject
to compute the remote URL for a submodule repository.

> We never produce the URLs in (C,D,E) ourselves, they come to use, because
> the user used it as the URL for cloning a superproject.
> Normalize these paths.

As you know the externally-visible impact of this change (which I
asked you, but didn't see an on-list answer to, by the way), please
describe what this means to the end user in the log message.  It is
normally done in an earlier part of the log message to describe the
problem being solved and its background.

If I understand the issue correctly, it may go like this:

	The remote URL for the submodule can be specified relative
	to the URL of the superproject in .gitmodules.  A top-level
	git://site.xz/toplevel.git can specify in its .gitmodules

		[submodule "sub"]
			url = ../submodule.git
			path = sub

	to say that git://site.xz/submodule.git is where the
	submodule bound at its "sub/" is found.

	However, when the toplevel is cloned like this:

		git clone git://site.xz/toplevel.git/. top

	i.e. the toplevel specifies its URL with trailing "/.", the
	code set the URL to git://site.xz/toplevel.git/submodule.git
	for the submodule, which is nonsense.  This was because the
	logic failed to treat trailing "/." any differently from
	trailing "/<anything-without-slash>" when resolving a
	relative URL "../<something>" off of it.  Stripping "/." at
	the end does *not* take you one level up, even though
	stripping "/<anything-without-slash>" does!
	
And then describe the solution/bugfix, listing A-C (or A-E) and
telling that these will be treated the same way.


^ permalink raw reply

* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Junio C Hamano @ 2016-10-18 15:53 UTC (permalink / raw)
  To: Santiago Torres; +Cc: git
In-Reply-To: <20161018133518.hwbv3gbeddjjfvcz@LykOS.localdomain>

Santiago Torres <santiago@nyu.edu> writes:

>> * st/verify-tag (2016-10-10) 7 commits
>>  - t/t7004-tag: Add --format specifier tests
>>  - t/t7030-verify-tag: Add --format specifier tests
>>  - builtin/tag: add --format argument for tag -v
>>  - builtin/verify-tag: add --format to verify-tag
>>  - tag: add format specifier to gpg_verify_tag
>>  - ref-filter: add function to print single ref_array_item
>>  - gpg-interface, tag: add GPG_VERIFY_QUIET flag
>> 
>>  "git tag" and "git verify-tag" learned to put GPG verification
>>  status in their "--format=<placeholders>" output format.
>> 
>>  Is this ready for 'next'?
>
> Hi, I saw this on the previous "what's cooking." Is there anything I
> need to do on my side to make sure this is ready for next?

Posting this exact message to the list would be an excellent way
;-).

Hopefully some competent reviewer comes and points me at a thread
where s/he says the series was already reviewed and in good shape
soonish, and your message may be a good trigger to make it happen.



^ permalink raw reply

* Re: [PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts
From: Junio C Hamano @ 2016-10-18 15:54 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610181339550.197091@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In the meantime, I'd be happy to just add a comment that this function is
> intended for oneliners, but that it will also read multi-line files and
> only strip off the EOL marker from the last line.
>
> Would that work for you?

That would be ideal, I would think.

^ permalink raw reply

* Re: [PATCH v3 16/25] sequencer: support amending commits
From: Junio C Hamano @ 2016-10-18 15:56 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610181352010.197091@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 17 Oct 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > This teaches the run_git_commit() function to take an argument that will
>> > allow us to implement "todo" commands that need to amend the commit
>> > messages ("fixup", "squash" and "reword").
>> 
>> Likewise to 15/25, i.e. Good, though the growth by these two steps
>> starts to make me wonder if these three options should be crammed
>> into an unsigned "flags" bitword.
>
> After looking at the diff with the added complications of ORing and ANDing
> the flags, I'd much rather prefer to stay with the three flags being kept
> separately. It's not like we need to save bits, but we need to preserve
> readability as much as possible, I'd wager.

That's OK.  I just wanted to make sure pros-and-cons have been
already considered.

The primary merit of using flags bitword is not to save bits; it is
done to limit the damage to the codebase when we need to add yet
another knob, by the way.

^ permalink raw reply

* Re: [PATCH v3 4/6] trailer: make args have their own struct
From: Junio C Hamano @ 2016-10-18 16:05 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Jonathan Tan, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <CAGZ79kYuaHvO-aXeCocDseTLe13g=PesKxQgOrcOREtM1CsWcg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

>> @@ -29,6 +29,12 @@ struct trailer_item {
>>         struct list_head list;
>>         char *token;
>>         char *value;
>> +};
>> +
>> +struct arg_item {
>> +       struct list_head list;
>> +       char *token;
>> +       char *value;
>>         struct conf_info conf;
>>  };
>
> (Unrelated side note:) When first seeing this diff, I assumed the diff
> heuristic is going wild, because it doesn't add a full struct.
> But on a second closer look, I realize this is the only correct diff,
> because we do not account for moved lines from one struct to the
> other.

It probably is not "the only" correct diff, as you actually could
shift it the other way by one to three lines.  I am not sure which
one among four possible diff is the easiest to grok, though.  Both
the above (picking the highest possible position) and the below (the
other extreme) are probably easier to read than anything in between.

 struct trailer_item {
+	struct list_head list;
+	char *token;
+	char *value;
+};
+
+struct arg_item {
 	struct list_head list;
 	char *token;
 	char *value;
 	struct conf_info conf;
 };
 

^ permalink raw reply

* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Junio C Hamano @ 2016-10-18 16:36 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Stefan Beller, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <cb622421-1857-d136-2618-ec62d438790d@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

>>     * rs/c-auto-resets-attributes:
>>       pretty: avoid adding reset for %C(auto) if output is empty
>>
>> And neither of the two colon containing line remotely resembles how
>> a typical RFC-822 header is formatted.  So that may serve as a hint
>> to how we can tighten it without introducing false negative.
>
> The only "offending" character is the space (according to RFC 822),
> but that sounds like a good rule to have.

I suspect that we should be willing to deviate from the letter of
RFC to reject misidentification.  I saw things like

	Thanks to: Jonathan Tan <jt@host.xz>
	Signed-off-by: A U Thor <au@th.or>

in the wild (notice the SP between Thanks and to), for example.
Rejecting leading whitespace as a line that does *not* start the
header (hence its colon does not count) may be a good compromise.

> I think that "Signed-off-by:" is not guaranteed to be
> present.

But do we really care about that case where there is no S-o-b:?  I
personally do not think so.

> Defining a trailer line as "a line starting with a token,
> then optional whitespace, then separator", maybe the following rule:
> - at least one trailer line generated by Git ("(cherry picked by" or
> "Signed-off-by") or configured in the "trailer" section in gitconfig
> OR
> - at least 3/4 logical trailer lines (I'm wondering if this should be
> 100% trailer lines)

I'd strongly suggest turning that OR to AND.  We will not safely be
able to write a commit log message that describes how S-o-b lines
are handled in its last paragraph otherwise.

I do not care too deeply about 3/4, but I meant to allow 75% cruft
but no more than that, and the fact that the threashold is set at
way more than 50% is important.  IOW, if you have

	Ordinary log message here...

	S-o-b: A U Thor <au@th.or>
	[a short description that is typically a single liner
        in the real world use pattern we saw in the world, but
	could overflow to become multi line cruft]
	S-o-b: R E Layer <re@lay.er>

"last paragraph" is 5 lines long, among which 60% are cruft that is
below the 75% threshold, and "am -s" can still add the S-o-b of the
committer at the end of that existing last paragraph.  Making it too
strict would raise the false negative ratio.

^ permalink raw reply

* Re: git checkout crashes after server being updated to Debian X86_64
From: René Scharfe @ 2016-10-18 16:42 UTC (permalink / raw)
  To: Raffael Reichelt, git
In-Reply-To: <6B2BE996-F696-4EB4-91CA-849D40B8802D@gmail.com>

Am 18.10.2016 um 17:17 schrieb Raffael Reichelt:
> Hello!
>
> I have a serious problem with git, After my provider had updated to a
> X86_64 architecture git crashes with various memory-related errors.
> This is happening remote when pushing to the repository from my local
> machine as well as trying it on a shell on the server itself.
>
> This are the error-messages:
>
> fatal: Out of memory, realloc failed
> fatal: recursion detected in die handler
> fatal: recursion detected in die handler
>
> or
> fatal: unable to create threaded lstat
> fatal: recursion detected in die handler
> or
> fatal: unable to create threaded lstat
> *** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
> fatal: recursion detected in die handler
> Aborted
>
> It’s obviously not a problem of the repository - happens with all of
> them. I think it is also not a question of size - happens with a 80M
> Repository as well as with a 500M one.
>
> Any way: did a
>
> git fsck
> Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
> Prüfe Objekte: 100% (56305/56305), Fertig.
>
> git gc --auto --prune=today —aggressive
> git repack
>
> Additionally I played around some config parameters  so my config now looks like:
> [http]
>     postbuffer = 524288000
> [pack]
>     threads = 1
>     deltaCacheSize = 128m
>     packSizeLimit = 128m
>     windowMemory = 128m
> [core]
>     packedGitLimit = 128m
>     packedGitWindowSize = 128m
>     repositoryformatversion = 0
>     filemode = true
>     bare = true
>
> I am running
> git version 2.1.4
>
> on
> Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
>
> Anyone out there to help me getting out of this trouble?

Git 2.1.4 is the version that comes with Debian stable according to 
https://packages.debian.org/jessie/git, so I guess using a more recent 
version is not a reasonable option.

What do "file $(which git)" and "ulimit -a" return?  Do you have an 
x86-64 binary and no unnecessarily low limits set?

René


^ permalink raw reply

* Re: git checkout crashes after server being updated to Debian X86_64
From: Raffael Reichelt @ 2016-10-18 17:00 UTC (permalink / raw)
  To: René Scharfe; +Cc: git
In-Reply-To: <1c70da04-d26b-f0af-8939-edc96de586d9@web.de>

Hello Renè!
file is returning 

/usr/bin/git: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32,  BuildID[sha1]=ee62e538d6fe6673d3ba49f0e66bfec784cc32bc, stripped

and ulimit is:
core file size          (blocks, -c) 0
data seg size           (kbytes, -d) unlimited
scheduling priority             (-e) 1
file size               (blocks, -f) unlimited
pending signals                 (-i) 16382
max locked memory       (kbytes, -l) 64
max memory size         (kbytes, -m) unlimited
open files                      (-n) 512
pipe size            (512 bytes, -p) 8
POSIX message queues     (bytes, -q) 819200
real-time priority              (-r) 0
stack size              (kbytes, -s) 8192
cpu time               (seconds, -t) 1800
max user processes              (-u) 42
virtual memory          (kbytes, -v) 786432
file locks                      (-x) unlimited

Support told me git is limited to 600M

Regrads,
Rafael

> Am 18.10.2016 um 18:42 schrieb René Scharfe <l.s.r@web.de>:
> 
> Am 18.10.2016 um 17:17 schrieb Raffael Reichelt:
>> Hello!
>> 
>> I have a serious problem with git, After my provider had updated to a
>> X86_64 architecture git crashes with various memory-related errors.
>> This is happening remote when pushing to the repository from my local
>> machine as well as trying it on a shell on the server itself.
>> 
>> This are the error-messages:
>> 
>> fatal: Out of memory, realloc failed
>> fatal: recursion detected in die handler
>> fatal: recursion detected in die handler
>> 
>> or
>> fatal: unable to create threaded lstat
>> fatal: recursion detected in die handler
>> or
>> fatal: unable to create threaded lstat
>> *** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
>> fatal: recursion detected in die handler
>> Aborted
>> 
>> It’s obviously not a problem of the repository - happens with all of
>> them. I think it is also not a question of size - happens with a 80M
>> Repository as well as with a 500M one.
>> 
>> Any way: did a
>> 
>> git fsck
>> Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
>> Prüfe Objekte: 100% (56305/56305), Fertig.
>> 
>> git gc --auto --prune=today —aggressive
>> git repack
>> 
>> Additionally I played around some config parameters  so my config now looks like:
>> [http]
>>    postbuffer = 524288000
>> [pack]
>>    threads = 1
>>    deltaCacheSize = 128m
>>    packSizeLimit = 128m
>>    windowMemory = 128m
>> [core]
>>    packedGitLimit = 128m
>>    packedGitWindowSize = 128m
>>    repositoryformatversion = 0
>>    filemode = true
>>    bare = true
>> 
>> I am running
>> git version 2.1.4
>> 
>> on
>> Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
>> 
>> Anyone out there to help me getting out of this trouble?
> 
> Git 2.1.4 is the version that comes with Debian stable according to https://packages.debian.org/jessie/git, so I guess using a more recent version is not a reasonable option.
> 
> What do "file $(which git)" and "ulimit -a" return?  Do you have an x86-64 binary and no unnecessarily low limits set?
> 
> René


^ permalink raw reply

* Re: Git log exclude/remotes/branches options not working as expected
From: Junio C Hamano @ 2016-10-18 17:02 UTC (permalink / raw)
  To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499C+SACpBrOEk-V4QMKoq2egmMfYSwUN1tZBBhFOTWomRw@mail.gmail.com>

Robert Dailey <rcdailey.lists@gmail.com> writes:

> I have 3 remotes registered in my clone:
>
> origin, fork, drive
>
> When I do:
>
> $ git log --oneline --decorate --graph
>
> I only want to see branches under:
>
> refs/heads/*
> refs/remotes/origin/*
>
> I tried the following:
>
> $ git log --oneline --decorate --graph --simplify-by-decoration
> --remote=origin topic1..master

I am guessing that the above is not what you actually typed and
s/remote/remotes/ is what you did.

According to "git log --help":

    --remotes[=<pattern>]
        Pretend as if all the refs in refs/remotes are listed on the
        command line as <commit>. If <pattern> is given, limit
        remote-tracking branches to ones matching given shell glob. If
        pattern lacks ?, *, or [, /* at the end is implied.

So your command line is equivalent to

    $ git log --oneline --decorate --graph --simplify-by-decoration \
      refs/remotes/origin/master refs/remotes/origin/topic ... \
      topic1..master

(replace the second line with all the remote-tracking branches you
have for "origin").

There is nothing that tells "--decorate" which refs to base its
decoration on, so it is reasonable to expect that a commit that
happens to match a tip of remote-tracking branches from other
remotes are decorated as such, and I think that is what you are
seeing.

> What I'm expecting is that I literally see NONE of those excluded refs
> ... How can I achieve my goals?

The decorations are added in log-tree.c::load_ref_decorations() and
it does not get anything that says "I want to see decorations based
on only these refs", so a short answer is that there is no canned
way to do this in today's codebase.

Having said that, I would say it is a reasonable new feature to
have.  Another related wishlist item might say "decorate only based
on tags, not branches or remote-tracking refs".

You can achieve your goals by teaching that codepath to take such
new pieces of information, come up with a new command line option
[*1*] and add a code to parse your command line option to either
builtin/log.c or revisions.c and pass it down the callchain that
leads to load_ref_decorations().


[Footnote]

*1* Unfortunately "--decorate=<option>" is already taken to specify
the decoration levels, so you would need a different one.

^ permalink raw reply

* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Junio C Hamano @ 2016-10-18 17:08 UTC (permalink / raw)
  To: Luke Shumaker; +Cc: git, pclouds, peff
In-Reply-To: <20161018150629.23205-1-lukeshu@sbcglobal.net>

Luke Shumaker <lukeshu@sbcglobal.net> writes:

> The superficial aspect of this change is that git-daemon now allows paths
> that start with a "~".  Previously, if git-daemon was run with
> "--base-path=/srv/git", it was impossible to get it to serve
> "/srv/git/~foo/bar.git".

I am not sure I understand what you are saying here.  Do you mean

    I have a path on my server /srv/git/~foo/bar.git; the tilde does
    not mean anything special--it is just a byte in a valid pathname.

    I want to allow my users to say

	git fetch git://my.server/~foo/bar.git

    and fetch from that repository, but "git daemon" lacks the way
    to configure to allow it.

If that is the case, what happens instead?  Due to the leading
"~foo/" getting noticed as an attempt to use the user-path expansion
it is not treated as just a literal character?

I am not sure if it is even a bug.  As you can easily lose that
tilde that appears in front of subdirectory of /srv/git/ or replace
it with something else (e.g. "u/"), this smells like "Don't do it if
it hurts" thing to me.


^ permalink raw reply

* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 17:15 UTC (permalink / raw)
  To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <xmqqbmyisiae.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> +static void strip_url_ending(char *url, size_t *_len)
>> +{
>> +	int check_url_stripping = 1;
>> +	size_t len = _len ? *_len : strlen(url);
>> +
>> +	while (check_url_stripping) {
>> +		check_url_stripping = 0;
>> +		if (is_dir_sep(url[len-2]) && url[len-1] == '.') {
>
> This is "strip /. at the end" it seems.
>
> Does anything in the loop control guarantees 2 <= len at this point?
>
>> +			url[len-2] = '\0';
>> +			len -= 2;
>> +			check_url_stripping = 1;
>> +		}
>> +
>> +		if (is_dir_sep(url[len-1])) {
>
> This is "strip / at the end" it seems.
>
> Does anything in the loop control guarantees 1 <= len at this point?
>
>> +			url[len-1] = '\0';
>> +			len--;
>> +			check_url_stripping = 1;
>> +		}
>> +	}

I also somehow find the "check-url-stripping" variable ugly.

	while (URL still has something that could be stripped) {
		if (ends with "/.") {
			strip "/.";
			continue;
		}
		if (ends with "/") {
			strip "/";
			continue;
		}
		break;
	}

perhaps?

^ permalink raw reply

* Re: [PATCH] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 17:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
	Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqr37dpoip.fsf@gitster.mtv.corp.google.com>

On Tue, Oct 18, 2016 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I also somehow find the "check-url-stripping" variable ugly.
>
>         while (URL still has something that could be stripped) {

for(;;) {

here ? (this code would not need a variable, and
for wins over while:
$ git grep "while (1)" |wc -l
107
$ git grep "for (;;)" |wc -l
128
)

>                 if (ends with "/.") {
>                         strip "/.";
>                         continue;
>                 }
>                 if (ends with "/") {
>                         strip "/";
>                         continue;
>                 }
>                 break;
>         }
>
> perhaps?

^ permalink raw reply

* Re: Merge conflicts in .gitattributes can cause trouble
From: Junio C Hamano @ 2016-10-18 17:30 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Lars Schneider, git, Jeff King, me
In-Reply-To: <alpine.DEB.2.20.1610181437180.197091@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> To the contrary. As far as I can see, when calling `git merge`, Git
> currently *does* read .gitattributes from the file, and if that fails,
> falls back to reading that file from the index.

Hmph.

Assuming that the merge always goes in the index order, I think you
are right.  When we need to merge path/to/dir/.gitattributes, we
would need to know all the .gitattributes files that may affect that
path, so .gitattributes, path/.gitattributes, path/to/.gitattributes
and the file being merged are all read into core before anything
happens and these are kept in attr_stack while merging anything
underneath path/to/dir/ hierarchy without being re-read from the
filesystem.  The original contents of path/to/dir/.gitattributes
cached in the attr_stack will be discarded when we start to merge
things outside path/to/dir (e.g. merging path/to/another/file), but
at that point the contents of path/to/dir/.gitattributes no longer
matters to the path being merged, so unless the merge somehow jumps
around it is OK.

It may be a fragile assumption in the longer term that the merge
always goes in the index order, but I think the assumption holds in
the current codebase, and the update planned immediately in the
future.


^ permalink raw reply

* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 17:51 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
	Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZEFanPpdrE4sD64LJHmyrBiHafo1VzoGtoGh+wO_HCuw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

> for(;;) {
>
> here ? (this code would not need a variable, and
> for wins over while:
> $ git grep "while (1)" |wc -l
> 107
> $ git grep "for (;;)" |wc -l
> 128
> )

I dunno; the numbers tells me there is no strong preference by wide
margin either way.

I am not sure if the end shape does not really need an exit
condition.  If there is a need for one, "for (; condition;)" would
look strange.  If there isn't, "for (;;)" is actually my personal
preference over "while (1)".


^ permalink raw reply

* [PATCH] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 17:52 UTC (permalink / raw)
  To: gitster
  Cc: git, j6t, Johannes.Schindelin, venv21, dennis, jrnieder, bmwill,
	Stefan Beller

The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules.  A top-level
git://site.xz/toplevel.git can specify in its .gitmodules

        [submodule "sub"]
                url = ../submodule.git
                path = sub

to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.

However, when the toplevel is cloned like this:

        git clone git://site.xz/toplevel.git/. top

i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense.  This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it.  Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!

Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?

The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.

We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 * reworded the commit message, taken from Junio, but added more explanation
   why we want to introduce this patch. 
 * added the length check
 * use an infinite loop with break instead of a variable
   to determine the ending condition.

 builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
 t/t0060-path-utils.sh       | 11 +++++++----
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 260f46f..ac03cb3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
 	return 0;
 }
 
+static void strip_url_ending(char *url, size_t *_len)
+{
+	size_t len = _len ? *_len : strlen(url);
+
+	for (;;) {
+		if (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {
+			url[len-2] = '\0';
+			len -= 2;
+			continue;
+		}
+		if (len > 0 && is_dir_sep(url[len-1])) {
+			url[len-1] = '\0';
+			len --;
+			continue;
+		}
+
+		break;
+	}
+
+	if (_len)
+		*_len = len;
+}
+
 /*
  * The `url` argument is the URL that navigates to the submodule origin
  * repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * the superproject working tree otherwise.
  *
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url      url              outcome          expectation
- * http://a.com/b  ../c             http://a.com/c   as is
- * http://a.com/b/ ../c             http://a.com/c   same as previous line, but
- *                                                   ignore trailing slash in url
- * http://a.com/b  ../../c          http://c         error out
- * http://a.com/b  ../../../c       http:/c          error out
- * http://a.com/b  ../../../../c    http:c           error out
- * http://a.com/b  ../../../../../c    .:c           error out
+ * remote_url       url              outcome          expectation
+ * http://a.com/b   ../c             http://a.com/c   as is
+ * http://a.com/b/  ../c             http://a.com/c   same as previous line, but
+ *                                                    ignore trailing '/' in url
+ * http://a.com/b/. ../c             http://a.com/c   same as previous line, but
+ *                                                    ignore trailing '/.' in url
+ * http://a.com/b   ../../c          http://c         error out
+ * http://a.com/b   ../../../c       http:/c          error out
+ * http://a.com/b   ../../../../c    http:c           error out
+ * http://a.com/b   ../../../../../c    .:c           error out
  * NEEDSWORK: Given how chop_last_dir() works, this function is broken
  * when a local part has a colon in its path component, too.
  */
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
 	struct strbuf sb = STRBUF_INIT;
 	size_t len = strlen(remoteurl);
 
-	if (is_dir_sep(remoteurl[len-1]))
-		remoteurl[len-1] = '\0';
+	strip_url_ending(remoteurl, &len);
 
 	if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
 		is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
 	}
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
-	if (ends_with(url, "/"))
-		strbuf_setlen(&sb, sb.len - 1);
 	free(remoteurl);
 
+	strip_url_ending(sb.buf, &sb.len);
+
 	if (starts_with_dot_slash(sb.buf))
 		out = xstrdup(sb.buf + 2);
 	else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
 test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
 test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
 test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
 test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
 test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
 test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
 test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
 test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
-- 
2.10.1.480.g573bd76


^ permalink raw reply related

* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Luke Shumaker @ 2016-10-18 18:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Luke Shumaker, git, pclouds, peff
In-Reply-To: <xmqqvawppote.fsf@gitster.mtv.corp.google.com>

On Tue, 18 Oct 2016 13:08:45 -0400,
Junio C Hamano wrote:
> 
> Luke Shumaker <lukeshu@sbcglobal.net> writes:
> 
> > The superficial aspect of this change is that git-daemon now allows paths
> > that start with a "~".  Previously, if git-daemon was run with
> > "--base-path=/srv/git", it was impossible to get it to serve
> > "/srv/git/~foo/bar.git".
> 
> I am not sure I understand what you are saying here.  Do you mean
> 
>     I have a path on my server /srv/git/~foo/bar.git; the tilde does
>     not mean anything special--it is just a byte in a valid pathname.
> 
>     I want to allow my users to say
> 
> 	git fetch git://my.server/~foo/bar.git
> 
>     and fetch from that repository, but "git daemon" lacks the way
>     to configure to allow it.

Yes, that is what I am saying.

> If that is the case, what happens instead?  Due to the leading
> "~foo/" getting noticed as an attempt to use the user-path expansion
> it is not treated as just a literal character?

What happens instead is

	if (*dir == '~') {
		if (!user_path) {
			logerror("'%s': User-path not allowed", dir);
			return NULL;
		}

which to the user looks like

	git clone git://my.server/~foo/bar.git
	Cloning into 'bar'...
	fatal: remote error: access denied or repository not exported: ~foo/bar.git

> I am not sure if it is even a bug.  As you can easily lose that
> tilde that appears in front of subdirectory of /srv/git/ or replace
> it with something else (e.g. "u/"), this smells like "Don't do it if
> it hurts" thing to me.

I buy into "Don't do it if it hurts", but that doesn't mean it's not a
bug on an uncommon edge-case.  Note that it doesn't hurt with
git-shell or cgit (I haven't checked with gitweb).

Many programs (especially shell scripts) fail to deal with filenames
containing a space.  "Don't put spaces in filenames if it hurts".
It's still a bug in the program.

Similarly, `git gui` used to not be able to add a file in a directory
starting with '~' (when one clicked the file named "~foo/bar", it
said something along the lines of "/home/~foo/bar is outside
repository"), and one had to use `git add '~foo/bar` directly.
"Don't do it if it hurts"; it was still a bug.

  Aside: one (somewhat silly) non-user reason that I've seen for a
  directory to start with '~' is that it sorts after all other ASCII
  characters; it moves the directory to the end of any lists.

-- 
Happy hacking,
~ Luke Shumaker

^ permalink raw reply

* Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-18 19:35 UTC (permalink / raw)
  To: Git

Hello git experts,

I have in the past attempted to integrate submodules into my primary
repository using the same directory name. However, this has always
caused headache when going to and from branches that take you between
when this integration occurred and when it didn't. It's a bit hard to
explain. Basically, if I have a submodule "foo", and I delete that
submodule and physically add its files under the same directory "foo",
when I do a pull to get this change from another clone, it fails
saying:

error: The following untracked working tree files would be overwritten
by checkout:
        foo/somefile.txt
Please move or remove them before you switch branches.
Aborting
could not detach HEAD


Obviously, git can't delete the submodule because the files have also
been added directly. I don't think it is built to handle this
scenario. Here is the series of commands I ran to "integrate" the
submodule (replace the submodule with a directory containing the exact
contents of the submodule itself):

#!/usr/bin/env bash
mv "$1" "${1}_"
git submodule deinit "$1"
git rm "$1"
mv "${1}_" "$1"
git add "$1/**"

The above script is named git-integrate-submodule, I run it like so:

$ git integrate-submodule foo

Then I do:

$ git commit -m 'Integrated foo submodule'

Is there any way to make this work nicely? The only solution I've
found is to obviously rename the directory before adding the physical
files, for example name it foo1. Because they're different, they never
"clash".

^ permalink raw reply

* [PATCH] sha1_name: remove ONELINE_SEEN bit
From: Junio C Hamano @ 2016-10-18 19:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

28a4d94044 ("object name: introduce ':/<oneline prefix>' notation",
2007-02-24) started using its own bit from object->flags to mark
commits used while parsing the ":/token" extended SHA-1 syntax to
name a commit temporarily, and this was kept even when f7bff00314
("sha1_name.c: fix parsing of ":/token" syntax", 2010-08-02) found
and fixed a bug in its implementation.

The use of that flag bit, however, is limited to a single function,
get_sha1_oneline(), which first sets it for the commits sitting at
the tips of refs, uses the bit to avoid duplicate traversal while
walking the history, and then cleans the bit from all commits it
walked.

Which is exactly what the general-purpose TMP_MARK bit meant to be
used for isolated case was invented for.  Replace ONELINE_SEEN with
TMP_MARK and retire the former.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object.h    |  1 -
 sha1_name.c | 10 ++++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/object.h b/object.h
index f8b644263f..f8e218eccd 100644
--- a/object.h
+++ b/object.h
@@ -37,7 +37,6 @@ struct object_array {
  * bundle.c:                               16
  * http-push.c:                            16-----19
  * commit.c:                               16-----19
- * sha1_name.c:                                     20
  */
 #define FLAG_BITS  27
 
diff --git a/sha1_name.c b/sha1_name.c
index ca7ddd6f2c..fa0e6701a3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -7,6 +7,7 @@
 #include "refs.h"
 #include "remote.h"
 #include "dir.h"
+#include "revision.h"
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 
@@ -855,9 +856,6 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
  * For future extension, all other sequences beginning with ':/!' are reserved.
  */
 
-/* Remember to update object flag allocation in object.h */
-#define ONELINE_SEEN (1u<<20)
-
 static int handle_one_ref(const char *path, const struct object_id *oid,
 			  int flag, void *cb_data)
 {
@@ -899,7 +897,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 		return -1;
 
 	for (l = list; l; l = l->next) {
-		l->item->object.flags |= ONELINE_SEEN;
+		l->item->object.flags |= TMP_MARK;
 		commit_list_insert(l->item, &backup);
 	}
 	while (list) {
@@ -907,7 +905,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 		struct commit *commit;
 		int matches;
 
-		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
+		commit = pop_most_recent_commit(&list, TMP_MARK);
 		if (!parse_object(commit->object.oid.hash))
 			continue;
 		buf = get_commit_buffer(commit, NULL);
@@ -924,7 +922,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 	regfree(&regex);
 	free_commit_list(list);
 	for (l = backup; l; l = l->next)
-		clear_commit_marks(l->item, ONELINE_SEEN);
+		clear_commit_marks(l->item, TMP_MARK);
 	free_commit_list(backup);
 	return found ? 0 : -1;
 }

^ permalink raw reply related

* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 19:49 UTC (permalink / raw)
  To: Stefan Beller
  Cc: git, j6t, Johannes.Schindelin, venv21, dennis, jrnieder, bmwill
In-Reply-To: <20161018175247.28326-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Some users may rely on this by always cloning with '/.' and having
> an additional '../' in the relative path for the submodule, and this
> patch breaks them. So why introduce this patch?
>
> The fix in c12922024 (submodule: ignore trailing slash on superproject
> URL, 2016-10-10) and prior discussion revealed, that Git and Git
> for Windows treat URLs differently, as currently Git for Windows
> strips off a trailing dot from paths when calling a Git binary
> unlike when running a shell. Which means Git for Windows is already
> doing the right thing for the case mentioned above, but it would fail
> our current tests, that test for the broken behavior and it would
> confuse users working across platforms. So we'd rather fix it
> in Git to ignore any of these trailing no ops in the path properly.
>
> We never produce the URLs with a trailing '/.' in Git ourselves,
> they come to us, because the user used it as the URL for cloning
> a superproject. Normalize these paths.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  * reworded the commit message, taken from Junio, but added more explanation
>    why we want to introduce this patch. 

The additional explanation is very good.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 260f46f..ac03cb3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
>  	return 0;
>  }
>  
> +static void strip_url_ending(char *url, size_t *_len)
> +{
> +	size_t len = _len ? *_len : strlen(url);

Stare at our codebase and you'd notice that we avoid using names
with leading underscore deliberately and use trailing one instead
when we name a throw-away name like this.  Let's do the same here.
I.e.

	static void strip_url_ending(char *url, size_t *len_)
	{
		size_t len = len_ ? *len_ : strlen(url);

> +	for (;;) {
> +		if (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {
> +			url[len-2] = '\0';

"len-1" and "len-2" are usually spelled with SP on both sides of
binary operators.

> +			len -= 2;
> +			continue;
> +		}
> +		if (len > 0 && is_dir_sep(url[len-1])) {
> +			url[len-1] = '\0';
> +			len --;

And post-decrement sticks to whatever it is decrementing without SP.

> +			continue;
> +		}

^ permalink raw reply

* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Johannes Sixt @ 2016-10-18 20:06 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Stefan Beller, git@vger.kernel.org, Karl A.,
	Dennis Kaarsemaker
In-Reply-To: <2aa41392-a7ed-cd48-2737-bd852757ab35@kdbg.org>

Am 17.10.2016 um 21:32 schrieb Johannes Sixt:
> I think that we could reduce the confusion by converting all $PWD to
> $(pwd) in these test cases. I don't remember why I suggested to use $PWD
> for one of the arguments of the test cases (the second must be $(pwd)),
> but the most likely reason is only that we save a process.

Something like this works for me on Windows. I can update the patch
after the "normalize funny urls" topic settles.

---- 8< ----
t0060: sidestep surprising path mangling results on Windows

When an MSYS program (such as the bash that drives the test suite)
invokes git on Windows, absolute Unix style paths are transformed into
Windows native absolute paths (drive letter form). However, this
transformation also includes some simplifications that are not just
straight-forward textual substitutions:

- When the path ends in "/.", then the dot is stripped, but not the
  directory separator.

- When the path contains "..", then it is optimized away if possible,
  e.g., "/c/dir/foo/../bar" becomes "c:/dir/bar".

These additional transformations violate the assumptions of some
submodule path tests. We can avoid them when the input is already a
Windows native path, because then MSYS leaves the path unmolested.

Convert the uses of $PWD to $(pwd); the latter returns a native Windows
path.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t0060-path-utils.sh | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..444b5a4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -305,8 +305,9 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config
 test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs
 test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow
 
-# In the tests below, the distinction between $PWD and $(pwd) is important:
-# on Windows, $PWD is POSIX style (/c/foo), $(pwd) has drive letter (c:/foo).
+# In the tests below, $(pwd) must be used because it is a native path on
+# Windows and avoids MSYS's path mangling (which simplifies "foo/../bar" and
+# strips the dot from trailing "/.").
 
 test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
 test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
@@ -314,7 +315,7 @@ test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/s
 test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
 test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
-test_submodule_relative_url "../" "$PWD/addtest" "../repo" "$(pwd)/repo"
+test_submodule_relative_url "../" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
 test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 
@@ -327,16 +328,16 @@ test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
 test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
-test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
-test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
-test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
-test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
+test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
+test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
+test_submodule_relative_url "(null)" "$(pwd)" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
+test_submodule_relative_url "(null)" "$(pwd)" "./å äö" "$(pwd)/å äö"
+test_submodule_relative_url "(null)" "$(pwd)/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$(pwd)/submodule" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$(pwd)/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
+test_submodule_relative_url "(null)" "$(pwd)/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
 test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
 test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
-- 
2.10.0.343.g37bc62b


^ permalink raw reply related


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