Git development
 help / color / mirror / Atom feed
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 21:59 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Brandon Williams, Junio C Hamano, git@vger.kernel.org,
	Jonathan Tan
In-Reply-To: <CAGZ79kaqzssfN_bRQYpqC9HsKmyQZNCQcs+T5ke95Sf-C5PaRQ@mail.gmail.com>

On Thu, Dec 01, 2016 at 01:56:32PM -0800, Stefan Beller wrote:

> > Bleh. Looks like it happens as part of the recently-added
> > get_common_dir(). I'm not sure if that is ever relevant for submodules,
> > but I guess in theory you could have a submodule clone that is part of a
> > worktree?
> 
> Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)
> 
> For now each submodule has its own complete git dir, but the vision
> would be to have a common git dir for submodules in the common
> superprojects git dir as well, such that objects are shared actually. :)

Fair enough. Given that it seems to behave OK even in error cases, the
simple stat() test may be the best option, then. I do think we should
consider adding a few test cases to make sure it continues to behave in
the error cases (just because we are relying partially on what git's
setup code happens to do currently, and we'd want to protect ourselves
against regressions).

-Peff

^ permalink raw reply

* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-12-01 21:56 UTC (permalink / raw)
  To: Jeff King
  Cc: Brandon Williams, Junio C Hamano, git@vger.kernel.org,
	Jonathan Tan
In-Reply-To: <20161201205944.2py2ijranq4g2wap@sigill.intra.peff.net>

On Thu, Dec 1, 2016 at 12:59 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 01, 2016 at 12:54:44PM -0800, Brandon Williams wrote:
>
>> > I think this more robust check is probably a good idea, that way we
>> > don't step into a submodule with a .git directory that isn't really a
>> > .git dir.
>>
>> Looks like this is a no-go as well...the call to is_git_directory() ends
>> up calling real_path...which ends up performing the chdir call, which
>> puts us right back to where we started!  (as a side note I was using
>> is_git_directory else where...which I now know I can't use)
>
> Bleh. Looks like it happens as part of the recently-added
> get_common_dir(). I'm not sure if that is ever relevant for submodules,
> but I guess in theory you could have a submodule clone that is part of a
> worktree?

Sure we can, for a test that we don't have that, see the embedgitdirs series. ;)

For now each submodule has its own complete git dir, but the vision
would be to have a common git dir for submodules in the common
superprojects git dir as well, such that objects are shared actually. :)

>
> -Peff

^ permalink raw reply

* Re: "Your branch is ahead of 'origin' by X commits"
From: Jeff King @ 2016-12-01 21:47 UTC (permalink / raw)
  To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: git@vger.kernel.org
In-Reply-To: <D465BDE6.B7DE%ernesto.alfonsogonzalez@ge.com>

On Thu, Dec 01, 2016 at 07:49:40PM +0000, Alfonsogonzalez, Ernesto (GE Digital) wrote:

> $ git diff origin/master
> $ git status
> On branch master
> Your branch is ahead of 'origin' by 108 commits.
>   (use "git push" to publish your local commits)
> Untracked files:
>   (use "git add <file>..." to include in what will be committed)

The "master" we are talking about here must always be
"refs/heads/master", since it will have come from resolving the HEAD
symbolic ref.

But here:

> $ git show origin/master --oneline
> 92d392c Merge pull request #21 from org/branch
> 
> $ git show master --oneline
> 92d392c Merge pull request #21 from org/branch

The "master" in the second case could possibly find "master" as another
name. Is it possible you have a .git/master file (this may have been
created by accidentally running "git update-ref master" instead of "git
update-ref refs/heads/master")?

Or other things you could check:

  # see what's on HEAD, which we know points to refs/heads/master
  git show HEAD

  # or just check refs/heads/master itself
  git show refs/heads/master

  # or just ask what "master" resolves to
  git rev-parse --symbolic-full-name master

That last one actually seems to complain that "refname 'master' is
ambiguous' if you do have .git/master. I think that's a minor bug, as it
should presumably follow the normal disambiguation rules used for lookup
(in which .git/master always takes precedence over refs/heads/master).

-Peff

^ permalink raw reply

* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-01 21:40 UTC (permalink / raw)
  To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-5-git-send-email-bmwill@google.com>

On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:

> Add the from_user parameter to the 'is_transport_allowed' function.
> This allows callers to query if a transport protocol is allowed, given
> that the caller knows that the protocol is coming from the user (1) or
> not from the user (0), such as redirects in libcurl.  If unknown, a -1
> should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> to determine if the protocol came from the user.

Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
already in 'next' anyway, though I guess we are due for a post-release
reset of 'next').

> diff --git a/http.c b/http.c
> index fee128b..e74c0f0 100644
> --- a/http.c
> +++ b/http.c
> @@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
>  	curl_easy_setopt(result, CURLOPT_POST301, 1);
>  #endif
>  #if LIBCURL_VERSION_NUM >= 0x071304
> -	if (is_transport_allowed("http"))
> +	if (is_transport_allowed("http", 0))
>  		allowed_protocols |= CURLPROTO_HTTP;
> -	if (is_transport_allowed("https"))
> +	if (is_transport_allowed("https", 0))
>  		allowed_protocols |= CURLPROTO_HTTPS;
> -	if (is_transport_allowed("ftp"))
> +	if (is_transport_allowed("ftp", 0))
>  		allowed_protocols |= CURLPROTO_FTP;
> -	if (is_transport_allowed("ftps"))
> +	if (is_transport_allowed("ftps", 0))
>  		allowed_protocols |= CURLPROTO_FTPS;
>  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
>  #else

This is better, but I think we still need to deal with http-alternates
on top.

I think we'd need to move this allowed_protocols setup into a function
like:

  int generate_allowed_protocols(int from_user)
  {
	int ret;
	if (is_transport_allowed("http", from_user))
		ret |= CURLPROTO_HTTP;
	... etc ...
	return ret;
  }

and then create a protocol list for each situation:

  allowed_protocols = generate_allowed_protocols(-1);
  allowed_redir_protocols = generate_allowed_protocols(0);

and then we know we can always set up the redir protocols:

  curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);

and which we feed for CURLOPT_PROTOCOLS depends on whether we are
following an http-alternates redirect or not. But I suspect it will be a
nasty change to plumb through the idea of "this request is on behalf of
an http-alternates redirect".

Given how few people probably care, I'm tempted to document it as a
quirk and direct people to the upcoming http.followRedirects. The newly
proposed default value of that disables http-alternates entirely anyway.

-Peff

^ permalink raw reply

* Re: [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 21:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7f7j8iz6.fsf@gitster.mtv.corp.google.com>

On Thu, Dec 01, 2016 at 12:58:21PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > This takes the output of `diff-tree -z --raw` and feeds it
> >> > back to the later stages of the diff machinery to produce
> >> > diffs in other formats.
> >> 
> >> A full circle.  This reminds me of the experiment done more than 10
> >> years ago at the beginning of the "diffcore transformations" design.
> >
> > Heh, I didn't even think to dig for prior art on the list.
> 
> It took me a while to dig it up because the topic is so old, but
> 
> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
> 
> is the thread I had in mind.  The idea of rename detection followed
> soon afterwards.

Thanks for an interesting read. Your diff-tree-helper patch is very
similar to what I wrote.

I do think the right decision was made back then. The single-process
model is much more efficient, and it was over 10 years until somebody
actually wanted to expose the functionality to a script (and even now,
I'm not convinced enough people want it to even merit inclusion).

-Peff

^ permalink raw reply

* "Your branch is ahead of 'origin' by X commits"
From: Alfonsogonzalez, Ernesto (GE Digital) @ 2016-12-01 19:49 UTC (permalink / raw)
  To: git@vger.kernel.org

Hi,

Git status tells me "Your branch is ahead of 'origin' by 108 commits.²,
but my local and origin/master are pointing to the same commit.

What am I doing wrong?

$ git diff origin/master
$ git status
On branch master
Your branch is ahead of 'origin' by 108 commits.
  (use "git push" to publish your local commits)
Untracked files:
  (use "git add <file>..." to include in what will be committed)

...
nothing added to commit but untracked files present (use "git add" to
track)
$ 



$ git show origin/master --oneline
92d392c Merge pull request #21 from org/branch

$ git show master --oneline
92d392c Merge pull request #21 from org/branch


$ git --version 
git version 2.10.2




Thanks,

Ernesto


^ permalink raw reply

* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 21:04 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <xmqq7f7j8iz6.fsf@gitster.mtv.corp.google.com>

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

> It took me a while to dig it up because the topic is so old, but
>
> https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/
>
> is the thread I had in mind.  The idea of rename detection followed
> soon afterwards.

... which was this one:

https://public-inbox.org/git/7vr7g4m0lz.fsf_-_@assigned-by-dhcp.cox.net/#t


^ permalink raw reply

* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 20:59 UTC (permalink / raw)
  To: Brandon Williams; +Cc: Junio C Hamano, git, sbeller, jonathantanmy
In-Reply-To: <20161201205444.GG54082@google.com>

On Thu, Dec 01, 2016 at 12:54:44PM -0800, Brandon Williams wrote:

> > I think this more robust check is probably a good idea, that way we
> > don't step into a submodule with a .git directory that isn't really a
> > .git dir.
> 
> Looks like this is a no-go as well...the call to is_git_directory() ends
> up calling real_path...which ends up performing the chdir call, which
> puts us right back to where we started!  (as a side note I was using
> is_git_directory else where...which I now know I can't use)

Bleh. Looks like it happens as part of the recently-added
get_common_dir(). I'm not sure if that is ever relevant for submodules,
but I guess in theory you could have a submodule clone that is part of a
worktree?

-Peff

^ permalink raw reply

* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 20:58 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20161201205504.flgaf7dwv3b3dkkd@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > This takes the output of `diff-tree -z --raw` and feeds it
>> > back to the later stages of the diff machinery to produce
>> > diffs in other formats.
>> 
>> A full circle.  This reminds me of the experiment done more than 10
>> years ago at the beginning of the "diffcore transformations" design.
>
> Heh, I didn't even think to dig for prior art on the list.

It took me a while to dig it up because the topic is so old, but

https://public-inbox.org/git/Pine.LNX.4.58.0504251832480.18901@ppc970.osdl.org/

is the thread I had in mind.  The idea of rename detection followed
soon afterwards.

^ permalink raw reply

* Re: [RFC/PATCH] add diff-pairs tool
From: Junio C Hamano @ 2016-12-01 20:52 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20161201204042.6yslbyrg7l6ghhww@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> This takes the output of `diff-tree -z --raw` and feeds it
> back to the later stages of the diff machinery to produce
> diffs in other formats.

A full circle.  This reminds me of the experiment done more than 10
years ago at the beginning of the "diffcore transformations" design.


^ permalink raw reply

* Re: [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 20:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmwv8j9m.fsf@gitster.mtv.corp.google.com>

On Thu, Dec 01, 2016 at 12:52:05PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This takes the output of `diff-tree -z --raw` and feeds it
> > back to the later stages of the diff machinery to produce
> > diffs in other formats.
> 
> A full circle.  This reminds me of the experiment done more than 10
> years ago at the beginning of the "diffcore transformations" design.

Heh, I didn't even think to dig for prior art on the list.

In a sense this is just bringing the full power of diffcore out to the
scripting interface. The one missing component is that you can't
actually call diffcore_std() in the middle. The full pipeline would I
guess be something more like:

  git diff-tree --raw -z $a $b |
  git detect-renames |
  git diff-pairs -p

or something. In my model it's sufficient for the rename detection to
happen as part of the first diff-tree (since it's a whole-tree operation
anyway, there's no benefit to breaking it up into chunks).

-Peff

^ permalink raw reply

* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-12-01 20:54 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, sbeller, jonathantanmy
In-Reply-To: <20161201191603.GB54082@google.com>

On 12/01, Brandon Williams wrote:
> On 12/01, Jeff King wrote:
> > On Thu, Dec 01, 2016 at 10:46:23AM -0800, Junio C Hamano wrote:
> > 
> > > > mkpath() is generally an unsafe function because it uses a static
> > > > buffer, but it's handy and safe for handing values to syscalls like
> > > > this.
> > > 
> > > I think your "unsafe" is not about thread-safety but about "the
> > > caller cannot rely on returned value staying valid for long haul".
> > > If this change since v5 is about thread-safety, I am not sure if it
> > > is safe to use mkpath here.
> > 
> > Oh, good point. I meant "staying valid", but somehow totally forgot that
> > we cared about thread reentrancy here. As if I hadn't just spent an hour
> > debugging a thread problem.
> > 
> > My suggestion is clearly nonsense.
> > 
> > > I am a bit wary of making the check too sketchy like this, but this
> > > is not about determining if a random "path" that has ".git" in a
> > > superproject working tree is a submodule or not (that information
> > > primarily comes from the superproject index), so I tend to agree
> > > with the patch that it is sufficient to check presence of ".git"
> > > alone.
> > 
> > The real danger is that it is a different check than the child process
> > is going to use, so they may disagree (see the almost-infinite-loop
> > discussion elsewhere).
> > 
> > It feels quite hacky, but checking:
> > 
> >   if (is_git_directory(suspect))
> > 	return 1; /* actual git dir */
> >   if (!stat(suspect, &st) && S_ISREG(st.st_mode))
> > 	return 1; /* gitfile; may or may not be valid */
> >   return 0;
> > 
> > is a little more robust, because the child process will happily skip a
> > non-repo ".git" and keep walking back up to the superproject. Whereas if
> > it sees any ".git" file, even if it is bogus, it will barf then and
> > there.
> > 
> > I'm actually not sure if that latter behavior is a bug or not. I don't
> > think it was really planned out, and it obviously is inconsistent with
> > the other repo-discovery cases. But it is a convenient side effect for
> > submodules, and I doubt anybody is bothered by it in practice.
> > 
> > -Peff
> 
> I think this more robust check is probably a good idea, that way we
> don't step into a submodule with a .git directory that isn't really a
> .git dir.

Looks like this is a no-go as well...the call to is_git_directory() ends
up calling real_path...which ends up performing the chdir call, which
puts us right back to where we started!  (as a side note I was using
is_git_directory else where...which I now know I can't use)

-- 
Brandon Williams

^ permalink raw reply

* Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion
From: Junio C Hamano @ 2016-12-01 20:42 UTC (permalink / raw)
  To: Austin English; +Cc: Git Mailing List
In-Reply-To: <CACC5Q1eFM_G4wKopkbxabLEu8+nbt66wF1jKSoTuL1vnS5Tb4Q@mail.gmail.com>

Austin English <austinenglish@gmail.com> writes:

> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
> index ed8b4ff..5fb2dc5 100755
> --- a/Documentation/install-webdoc.sh
> +++ b/Documentation/install-webdoc.sh
> @@ -18,7 +18,7 @@ do
>  	else
>  		echo >&2 "# install $h $T/$h"
>  		rm -f "$T/$h"
> -		mkdir -p $(dirname "$T/$h")
> +		mkdir -p "$(dirname "$T/$h")"
>  		cp "$h" "$T/$h"
>  	fi
>  done

We know $h is safe without quoting (see what the for loop iterates
over a list and binding each element of it to this variable), but T
is the parameter given to this script, which comes from these

install-html: html
	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)

install-webdoc : html
	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)

in the Makefile.  So quoting the result of $(dirname "$T/$h") is
just as necessary as quoting the argument given to this dirname.

But I do not think it is sufficient, if we are truly worried about
people who specify a path that contains IFS whitespace in DESTDIR,
WEBDOC_DEST, htmldir and other *dir variables used in the Makefile.
The references to these variables, when they are mentioned on the
command lines of Makefile actions, all need to be quoted.  The
remainder of the Makefile tells me that we decided that we are not
worried about those people at all.

So while I could take your patch as-is, I am not sure how much value
it adds to the overall callchain that would reach the location that
is updated by the patch.  If you run

	make DESTDIR="/tmp/My Temporary Place" install

it would still not do the right thing even with your patch, I would
suspect.

Thanks.

^ permalink raw reply

* [RFC/PATCH] add diff-pairs tool
From: Jeff King @ 2016-12-01 20:40 UTC (permalink / raw)
  To: git

This is a small tool I cooked up for splitting the tree-diff out from
the generation of the patch text, and doing them in separate processes.
It's a little more expensive than doing it all in one process (besides
the process/pipe overhead, we may load blob content in the tree diff for
rename detection, and then again to do the actual patch). But it gives a
lot of flexibility when handling large diffs. You can save the tree-diff
and then feed it back in chunks to generate the actual patches, and do
so across many invocations (if you had, say, a website that showed diffs
and wanted to show small bits of them and let the user progressively
click through to see more).

Other things I tried but didn't work:

  - feeding two blobs to git-diff doesn't use plumbing, and loses
    information about filenames, etc. It also requires one process per
    pair.

  - feeding pathspecs back to git-diff-tree, like:

       git diff-tree --name-only $a $b |
       while read filename; do
         git diff-tree $a $b -- $filename
       done

    mostly works, but loses rename information (and is also slightly
    less efficient, as it has to walk the trees again).

So instead, I made a tool that takes the pairs generated by "diff-tree
--raw" and formats them as diff-tree would have if it had been done
in-process.

It's rather elegant, I think. But the main reason this is RFC is that
I'm not sure it's actually _useful_ to other people. It's a pretty
narrow use case.

The other reason it's an RFC is that it has a few rough edges. It works
well for my narrow use case, but there are cases where it probably
doesn't:

  - it expects "diff-tree -z" input, which keeps parsing simple. It
    probably wouldn't be too hard to have it handle the normal output,
    too.

  - it should handle submodules OK, but I don't know what it would do if
    you fed it things like dirty submodules from diff-files output.

  - some option combinations between the initial tree-diff and the
    followup diff do not make sense. For instance, if you do a
    non-recursive tree-diff (or one with "-t"), you'll have tree entries
    in your --raw output. Feeding that to "diff-pairs -p" will silently
    ignore the tree entries, because there's no way to represent them as
    a patch.

    Mostly these fall under "well, don't do that", but possibly the tool
    could be more aggressive about complaining, or coming up with some
    sensible behavior.

So anyway. Consider this a rough cut. What I'm really looking for at
this stage is whether anybody is interested enough in this for me to
bother polishing it.

-- >8 --
This takes the output of `diff-tree -z --raw` and feeds it
back to the later stages of the diff machinery to produce
diffs in other formats. Because the interim format contains
any whole-tree copy/rename information, you can safely feed
segments of the tree diff to get progressive patch-format
diffs. So something like:

  git diff-tree -r -z $a $b |
  git diff-pairs -p

should give you the same output that `git diff-tree -p`
would have.  Likewise, feeding each pair individually works,
too:

  git diff-tree -r -z -M $a $b |
  perl -0ne '
	my $meta = $_;
	my $path = <>;
	# only renames have an extra path
	my $path2 = <> if $meta =~ /[RC]\d+/;

	print STDERR "feeding one diff\n";
	open(my $fh, "|git diff-pairs -p");
	print $fh $meta, $path, $path2;
  '

The renames will still be shown just as if the diff had been
done in one process.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore                       |   1 +
 Documentation/git-diff-pairs.txt |  60 +++++++++++++++
 Makefile                         |   1 +
 builtin.h                        |   1 +
 builtin/diff-pairs.c             | 162 +++++++++++++++++++++++++++++++++++++++
 git.c                            |   1 +
 t/t4063-diff-pairs.sh            |  82 ++++++++++++++++++++
 7 files changed, 308 insertions(+)
 create mode 100644 Documentation/git-diff-pairs.txt
 create mode 100644 builtin/diff-pairs.c
 create mode 100755 t/t4063-diff-pairs.sh

diff --git a/.gitignore b/.gitignore
index 05cb58a3d..805751f2e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -48,6 +48,7 @@
 /git-diff
 /git-diff-files
 /git-diff-index
+/git-diff-pairs
 /git-diff-tree
 /git-difftool
 /git-difftool--helper
diff --git a/Documentation/git-diff-pairs.txt b/Documentation/git-diff-pairs.txt
new file mode 100644
index 000000000..4e3b6c909
--- /dev/null
+++ b/Documentation/git-diff-pairs.txt
@@ -0,0 +1,60 @@
+git-diff-pairs(1)
+=================
+
+NAME
+----
+git-diff-pairs - Compare blob pairs generated by `diff-tree --raw`
+
+SYNOPSIS
+--------
+[verse]
+'git diff-pairs' [diff_format_options]
+
+DESCRIPTION
+-----------
+
+Given the output of `diff-tree -z` on its stdin, `diff-pairs` will
+reformat that output into whatever format is requested on its command
+line.  For example:
+
+-----------------------------
+git diff-tree -z -M $a $b |
+git diff-pairs -p
+-----------------------------
+
+will compute the tree diff in one step (including renames), and then
+`diff-pairs` will compute and format the blob-level diffs for each pair.
+This can be used to modify the raw diff in the middle (without having to
+parse or re-create more complicated formats like `--patch`), or to
+compute diffs progressively over the course of multiple invocations of
+`diff-pairs`.
+
+Each blob pair is fed to the diff machinery individually and the output
+flushed immediately, meaning it is safe to interactively read and write
+from `diff-pairs`.
+
+OPTIONS
+-------
+
+All diff options below are accepted, but note that tree-wide options
+like `-M` are effectively noops, as we consider only one pair at a time.
+
+include::diff-options.txt[]
+
+BUGS
+----
+
+`diff-pairs` should handle any input generated by `diff-tree --raw -z`.
+It may choke or otherwise misbehave on output from `diff-files`, etc.
+
+Here's an incomplete list of things that `diff-pairs` could do, but
+doesn't (mostly in the name of simplicity):
+
+ - Only `-z` input is accepted, not normal `--raw` input.
+
+ - Abbreviated sha1s are rejected in the input from `diff-tree`; if you
+   want to abbreviate the output, you can pass `--abbrev` to
+   `diff-pairs`.
+
+ - Pathspecs are not handled by `diff-pairs`; you can limit the diff via
+   the initial `diff-tree` invocation.
diff --git a/Makefile b/Makefile
index f53fcc90d..c7ee3cd61 100644
--- a/Makefile
+++ b/Makefile
@@ -886,6 +886,7 @@ BUILTIN_OBJS += builtin/credential.o
 BUILTIN_OBJS += builtin/describe.o
 BUILTIN_OBJS += builtin/diff-files.o
 BUILTIN_OBJS += builtin/diff-index.o
+BUILTIN_OBJS += builtin/diff-pairs.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
 BUILTIN_OBJS += builtin/fast-export.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f..f62134ec1 100644
--- a/builtin.h
+++ b/builtin.h
@@ -59,6 +59,7 @@ extern int cmd_describe(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
+extern int cmd_diff_pairs(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch(int argc, const char **argv, const char *prefix);
diff --git a/builtin/diff-pairs.c b/builtin/diff-pairs.c
new file mode 100644
index 000000000..f80c49fef
--- /dev/null
+++ b/builtin/diff-pairs.c
@@ -0,0 +1,162 @@
+#include "cache.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "revision.h"
+
+static const char diff_pairs_usage[] =
+"git diff-pairs [diff-options]\n"
+"\n"
+"Reads pairs of blobs from stdin in 'diff-tree -z' syntax:\n"
+"\n"
+"  :<mode_a> <mode_b> <sha1_a> <sha1_b> <type>\\0<path>\0[path2\0]\n"
+"\n"
+"and outputs the diff for each a/b pair to stdout.";
+
+static unsigned parse_mode(const char *mode, char **endp)
+{
+	unsigned long ret;
+
+	errno = 0;
+	ret = strtoul(mode, endp, 8);
+	if (errno || *endp == mode || *(*endp)++ != ' ' || (unsigned)ret != ret)
+		die("unable to parse mode: %s", mode);
+	return ret;
+}
+
+static void parse_oid(char *p, char **endp, struct object_id *oid)
+{
+	*endp = p + GIT_SHA1_HEXSZ;
+	if (get_oid_hex(p, oid) || *(*endp)++ != ' ')
+		die("unable to parse object id: %s", p);
+}
+
+static unsigned short parse_score(char *score)
+{
+	unsigned long ret;
+	char *endp;
+
+	errno = 0;
+	ret = strtoul(score, &endp, 10);
+	ret *= MAX_SCORE / 100;
+	if (errno || endp == score || *endp || (unsigned short)ret != ret)
+		die("unable to parse rename/copy score: %s", score);
+	return ret;
+}
+
+/*
+ * The pair-creation is mostly done by diff_change and diff_addremove,
+ * which queue the filepair without returning it. So we have to resort
+ * to pulling it out of the global diff queue.
+ */
+static void set_pair_status(char status)
+{
+	/*
+	 * If we have no items in the queue, for some reason the pair wasn't
+	 * worth queueing. This generally shouldn't happen (since it means
+	 * dropping some parts of the diff), but the user can trigger it with
+	 * things like --ignore-submodules. If they do, the only sensible thing
+	 * is for us to play along and skip it.
+	 */
+	if (!diff_queued_diff.nr)
+		return;
+
+	diff_queued_diff.queue[0]->status = status;
+}
+
+int cmd_diff_pairs(int argc, const char **argv, const char *prefix)
+{
+	struct rev_info revs;
+	struct strbuf meta = STRBUF_INIT;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf path_dst = STRBUF_INIT;
+
+	init_revisions(&revs, prefix);
+	git_config(git_diff_basic_config, NULL);
+	revs.disable_stdin = 1;
+	argc = setup_revisions(argc, argv, &revs, NULL);
+
+	/* Don't allow pathspecs at all. */
+	if (argc > 1)
+		usage(diff_pairs_usage);
+
+	if (!revs.diffopt.output_format)
+		revs.diffopt.output_format = DIFF_FORMAT_RAW;
+
+	while (1) {
+		unsigned mode_a, mode_b;
+		struct object_id oid_a, oid_b;
+		char status;
+		char *p;
+
+		if (strbuf_getline_nul(&meta, stdin) == EOF)
+			break;
+
+		p = meta.buf;
+		if (*p == ':')
+			p++;
+
+		mode_a = parse_mode(p, &p);
+		mode_b = parse_mode(p, &p);
+
+		parse_oid(p, &p, &oid_a);
+		parse_oid(p, &p, &oid_b);
+
+		status = *p++;
+
+		if (strbuf_getline_nul(&path, stdin) == EOF)
+			die("got EOF while reading path");
+
+		switch (status) {
+		case DIFF_STATUS_ADDED:
+			diff_addremove(&revs.diffopt, '+',
+				       mode_b, oid_b.hash,
+				       1, path.buf, 0);
+			set_pair_status(status);
+			break;
+
+		case DIFF_STATUS_DELETED:
+			diff_addremove(&revs.diffopt, '-',
+				       mode_a, oid_a.hash,
+				       1, path.buf, 0);
+			set_pair_status(status);
+			break;
+
+		case DIFF_STATUS_TYPE_CHANGED:
+		case DIFF_STATUS_MODIFIED:
+			diff_change(&revs.diffopt,
+				    mode_a, mode_b,
+				    oid_a.hash, oid_b.hash,
+				    1, 1, path.buf, 0, 0);
+			set_pair_status(status);
+			break;
+
+		case DIFF_STATUS_RENAMED:
+		case DIFF_STATUS_COPIED:
+			{
+				struct diff_filespec *a, *b;
+				struct diff_filepair *pair;
+
+				if (strbuf_getline_nul(&path_dst, stdin) == EOF)
+					die("got EOF while reading secondary path");
+
+				a = alloc_filespec(path.buf);
+				b = alloc_filespec(path_dst.buf);
+				fill_filespec(a, oid_a.hash, 1, mode_a);
+				fill_filespec(b, oid_b.hash, 1, mode_b);
+
+				pair = diff_queue(&diff_queued_diff, a, b);
+				pair->status = status;
+				pair->score = parse_score(p);
+				pair->renamed_pair = 1;
+			}
+			break;
+
+		default:
+			die("unknown diff status: %c", status);
+		}
+
+		diff_flush(&revs.diffopt);
+	}
+
+	return 0;
+}
diff --git a/git.c b/git.c
index dce529fcb..4063c5366 100644
--- a/git.c
+++ b/git.c
@@ -423,6 +423,7 @@ static struct cmd_struct commands[] = {
 	{ "diff", cmd_diff },
 	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
 	{ "diff-index", cmd_diff_index, RUN_SETUP },
+	{ "diff-pairs", cmd_diff_pairs, RUN_SETUP },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
diff --git a/t/t4063-diff-pairs.sh b/t/t4063-diff-pairs.sh
new file mode 100755
index 000000000..a99ec6655
--- /dev/null
+++ b/t/t4063-diff-pairs.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='basic diff-pairs tests'
+. ./test-lib.sh
+
+# This creates a diff with added, modified, deleted, renamed, copied, and
+# typechange entries. That includes one in a subdirectory for non-recursive
+# tests, and both exact and inexact similarity scores.
+test_expect_success 'create commit with various diffs' '
+	echo to-be-gone >deleted &&
+	echo original >modified &&
+	echo now-a-file >symlink &&
+	test_seq 200 >two-hundred &&
+	test_seq 201 500 >five-hundred &&
+	git add . &&
+	test_tick &&
+	git commit -m base &&
+	git tag base &&
+
+	echo now-here >added &&
+	echo new >modified &&
+	rm deleted &&
+	mkdir subdir &&
+	echo content >subdir/file &&
+	mv two-hundred renamed &&
+	test_seq 201 500 | sed s/300/modified/ >copied &&
+	rm symlink &&
+	git add -A . &&
+	test_ln_s_add dest symlink &&
+	test_tick &&
+	git commit -m new &&
+	git tag new
+'
+
+test_expect_success 'diff-pairs recreates --raw' '
+	git diff-tree -r -M -C -C base new >expect &&
+	# note that diff-pairs uses the default abbrev,
+	# so we must tweak that for identical output
+	git diff-tree -r -M -C -C -z base new |
+	git diff-pairs --no-abbrev >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff-pairs can create -p output' '
+	git diff-tree -p -M -C -C base new >expect &&
+	git diff-tree -r -M -C -C -z base new |
+	git diff-pairs -p >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'non-recursive --raw retains tree entry' '
+	git diff-tree base new >expect &&
+	git diff-tree -z base new |
+	git diff-pairs --no-abbrev >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'split input across multiple diff-pairs' '
+	write_script split-raw-diff "$PERL_PATH" <<-\EOF &&
+	$/ = "\0";
+	while (<>) {
+	  my $meta = $_;
+	  my $path = <>;
+	  # renames have an extra path
+	  my $path2 = <> if $meta =~ /[RC]\d+/;
+
+	  open(my $fh, ">", sprintf "diff%03d", $.);
+	  print $fh $meta, $path, $path2;
+	}
+	EOF
+
+	git diff-tree -p -M -C -C base new >expect &&
+
+	git diff-tree -r -z -M -C -C base new |
+	./split-raw-diff &&
+	for i in diff*; do
+		git diff-pairs -p <$i
+	done >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.319.g1f4e1e0

^ permalink raw reply related

* [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-01 20:25 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-1-git-send-email-bmwill@google.com>

Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0), such as redirects in libcurl.  If unknown, a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 http.c      |  8 ++++----
 transport.c |  8 +++++---
 transport.h | 13 ++++++++++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/http.c b/http.c
index fee128b..e74c0f0 100644
--- a/http.c
+++ b/http.c
@@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_POST301, 1);
 #endif
 #if LIBCURL_VERSION_NUM >= 0x071304
-	if (is_transport_allowed("http"))
+	if (is_transport_allowed("http", 0))
 		allowed_protocols |= CURLPROTO_HTTP;
-	if (is_transport_allowed("https"))
+	if (is_transport_allowed("https", 0))
 		allowed_protocols |= CURLPROTO_HTTPS;
-	if (is_transport_allowed("ftp"))
+	if (is_transport_allowed("ftp", 0))
 		allowed_protocols |= CURLPROTO_FTP;
-	if (is_transport_allowed("ftps"))
+	if (is_transport_allowed("ftps", 0))
 		allowed_protocols |= CURLPROTO_FTPS;
 	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
 #else
diff --git a/transport.c b/transport.c
index 186de9a..8a3597b 100644
--- a/transport.c
+++ b/transport.c
@@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)
 	return PROTOCOL_ALLOW_USER_ONLY;
 }
 
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int from_user)
 {
 	const struct string_list *whitelist = protocol_whitelist();
 	if (whitelist)
@@ -735,7 +735,9 @@ int is_transport_allowed(const char *type)
 	case PROTOCOL_ALLOW_NEVER:
 		return 0;
 	case PROTOCOL_ALLOW_USER_ONLY:
-		return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+		if (from_user < 0)
+			from_user = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+		return from_user;
 	}
 
 	die("BUG: invalid protocol_allow_config type");
@@ -743,7 +745,7 @@ int is_transport_allowed(const char *type)
 
 void transport_check_allowed(const char *type)
 {
-	if (!is_transport_allowed(type))
+	if (!is_transport_allowed(type, -1))
 		die("transport '%s' not allowed", type);
 }
 
diff --git a/transport.h b/transport.h
index f4998bc..9820f10 100644
--- a/transport.h
+++ b/transport.h
@@ -153,10 +153,17 @@ extern int transport_summary_width(const struct ref *refs);
 struct transport *transport_get(struct remote *, const char *);
 
 /*
- * Check whether a transport is allowed by the environment. Type should
- * generally be the URL scheme, as described in Documentation/git.txt
+ * Check whether a transport is allowed by the environment.
+ *
+ * Type should generally be the URL scheme, as described in
+ * Documentation/git.txt
+ *
+ * from_user specifies if the transport was given by the user.  If unknown pass
+ * a -1 to read from the environment to determine if the transport was given by
+ * the user.
+ *
  */
-int is_transport_allowed(const char *type);
+int is_transport_allowed(const char *type, int from_user);
 
 /*
  * Check whether a transport is allowed by the environment,
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related

* [PATCH v7 2/4] transport: add protocol policy config option
From: Brandon Williams @ 2016-12-01 20:25 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-1-git-send-email-bmwill@google.com>

Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands.  This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols.  This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.

Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option.  A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option.  If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.

The supported policies are `always`, `never`, and `user`.  The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules).  Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.

Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.

Based on a patch by Jeff King <peff@peff.net>

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/config.txt         |  46 ++++++++++++++
 Documentation/git.txt            |  38 +++++-------
 git-submodule.sh                 |  12 ++--
 t/lib-proto-disable.sh           | 130 +++++++++++++++++++++++++++++++++++++--
 t/t5509-fetch-push-namespaces.sh |   1 +
 t/t5802-connect-helper.sh        |   1 +
 transport.c                      |  75 +++++++++++++++++++++-
 7 files changed, 264 insertions(+), 39 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..5fe50bc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2308,6 +2308,52 @@ pretty.<name>::
 	Note that an alias with the same name as a built-in format
 	will be silently ignored.
 
+protocol.allow::
+	If set, provide a user defined default policy for all protocols which
+	don't explicitly have a policy (`protocol.<name>.allow`).  By default,
+	if unset, known-safe protocols (http, https, git, ssh, file) have a
+	default policy of `always`, known-dangerous protocols (ext) have a
+	default policy of `never`, and all other protocols have a default
+	policy of `user`.  Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+  either unset or has a value of 1.  This policy should be used when you want a
+  protocol to be directly usable by the user but don't want it used by commands which
+  execute clone/fetch/push commands without user input, e.g. recursive
+  submodule initialization.
+
+--
+
+protocol.<name>.allow::
+	Set a policy to be used by protocol `<name>` with clone/fetch/push
+	commands. See `protocol.allow` above for the available policies.
++
+The protocol names currently used by git are:
++
+--
+  - `file`: any local file-based path (including `file://` URLs,
+    or local paths)
+
+  - `git`: the anonymous git protocol over a direct TCP
+    connection (or proxy, if configured)
+
+  - `ssh`: git over ssh (including `host:path` syntax,
+    `ssh://`, etc).
+
+  - `http`: git over http, both "smart http" and "dumb http".
+    Note that this does _not_ include `https`; if you want to configure
+    both, you must do so individually.
+
+  - any external helpers are named by their protocol (e.g., use
+    `hg` to allow the `git-remote-hg` helper)
+--
+
 pull.ff::
 	By default, Git does not create an extra merge commit when merging
 	a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c52cec8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,30 +1150,20 @@ of clones and fetches.
 	cloning a repository to make a backup).
 
 `GIT_ALLOW_PROTOCOL`::
-	If set, provide a colon-separated list of protocols which are
-	allowed to be used with fetch/push/clone. This is useful to
-	restrict recursive submodule initialization from an untrusted
-	repository. Any protocol not mentioned will be disallowed (i.e.,
-	this is a whitelist, not a blacklist). If the variable is not
-	set at all, all protocols are enabled.  The protocol names
-	currently used by git are:
-
-	  - `file`: any local file-based path (including `file://` URLs,
-	    or local paths)
-
-	  - `git`: the anonymous git protocol over a direct TCP
-	    connection (or proxy, if configured)
-
-	  - `ssh`: git over ssh (including `host:path` syntax,
-	    `ssh://`, etc).
-
-	  - `http`: git over http, both "smart http" and "dumb http".
-	    Note that this does _not_ include `https`; if you want both,
-	    you should specify both as `http:https`.
-
-	  - any external helpers are named by their protocol (e.g., use
-	    `hg` to allow the `git-remote-hg` helper)
-
+	If set to a colon-separated list of protocols, behave as if
+	`protocol.allow` is set to `never`, and each of the listed
+	protocols has `protocol.<name>.allow` set to `always`
+	(overriding any existing configuration). In other words, any
+	protocol not mentioned will be disallowed (i.e., this is a
+	whitelist, not a blacklist). See the description of
+	`protocol.allow` in linkgit:git-config[1] for more details.
+
+`GIT_PROTOCOL_FROM_USER`::
+	Set to 0 to prevent protocols used by fetch/push/clone which are
+	configured to the `user` state.  This is useful to restrict recursive
+	submodule initialization from an untrusted repository or for programs
+	which feed potentially-untrusted URLS to git commands.  See
+	linkgit:git-config[1] for more details.
 
 Discussion[[Discussion]]
 ------------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..0a477b4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
 wt_prefix=$(git rev-parse --show-prefix)
 cd_to_toplevel
 
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# If the user has already specified a set of allowed protocols,
-# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
 
 command=
 branch=
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index be88e9a..02f49cb 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -1,10 +1,7 @@
 # Test routines for checking protocol disabling.
 
-# test cloning a particular protocol
-#   $1 - description of the protocol
-#   $2 - machine-readable name of the protocol
-#   $3 - the URL to try cloning
-test_proto () {
+# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
+test_whitelist () {
 	desc=$1
 	proto=$2
 	url=$3
@@ -62,6 +59,129 @@ test_proto () {
 			test_must_fail git clone --bare "$url" tmp.git
 		)
 	'
+
+	test_expect_success "clone $desc (env var has precedence)" '
+		rm -rf tmp.git &&
+		(
+			GIT_ALLOW_PROTOCOL=none &&
+			export GIT_ALLOW_PROTOCOL &&
+			test_must_fail git -c protocol.allow=always clone --bare "$url" tmp.git &&
+			test_must_fail git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+		)
+	'
+}
+
+test_config () {
+	desc=$1
+	proto=$2
+	url=$3
+
+	# Test clone/fetch/push with protocol.<type>.allow config
+	test_expect_success "clone $desc (enabled with config)" '
+		rm -rf tmp.git &&
+		git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+	'
+
+	test_expect_success "fetch $desc (enabled)" '
+		git -C tmp.git -c protocol.$proto.allow=always fetch
+	'
+
+	test_expect_success "push $desc (enabled)" '
+		git -C tmp.git -c protocol.$proto.allow=always  push origin HEAD:pushed
+	'
+
+	test_expect_success "push $desc (disabled)" '
+		test_must_fail git -C tmp.git -c protocol.$proto.allow=never push origin HEAD:pushed
+	'
+
+	test_expect_success "fetch $desc (disabled)" '
+		test_must_fail git -C tmp.git -c protocol.$proto.allow=never fetch
+	'
+
+	test_expect_success "clone $desc (disabled)" '
+		rm -rf tmp.git &&
+		test_must_fail git -c protocol.$proto.allow=never clone --bare "$url" tmp.git
+	'
+
+	# Test clone/fetch/push with protocol.user.allow and its env var
+	test_expect_success "clone $desc (enabled)" '
+		rm -rf tmp.git &&
+		git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+	'
+
+	test_expect_success "fetch $desc (enabled)" '
+		git -C tmp.git -c protocol.$proto.allow=user fetch
+	'
+
+	test_expect_success "push $desc (enabled)" '
+		git -C tmp.git -c protocol.$proto.allow=user push origin HEAD:pushed
+	'
+
+	test_expect_success "push $desc (disabled)" '
+		(
+			cd tmp.git &&
+			GIT_PROTOCOL_FROM_USER=0 &&
+			export GIT_PROTOCOL_FROM_USER &&
+			test_must_fail git -c protocol.$proto.allow=user push origin HEAD:pushed
+		)
+	'
+
+	test_expect_success "fetch $desc (disabled)" '
+		(
+			cd tmp.git &&
+			GIT_PROTOCOL_FROM_USER=0 &&
+			export GIT_PROTOCOL_FROM_USER &&
+			test_must_fail git -c protocol.$proto.allow=user fetch
+		)
+	'
+
+	test_expect_success "clone $desc (disabled)" '
+		rm -rf tmp.git &&
+		(
+			GIT_PROTOCOL_FROM_USER=0 &&
+			export GIT_PROTOCOL_FROM_USER &&
+			test_must_fail git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+		)
+	'
+
+	# Test clone/fetch/push with protocol.allow user defined default
+	test_expect_success "clone $desc (enabled)" '
+		rm -rf tmp.git &&
+		git config --global protocol.allow always &&
+		git clone --bare "$url" tmp.git
+	'
+
+	test_expect_success "fetch $desc (enabled)" '
+		git -C tmp.git fetch
+	'
+
+	test_expect_success "push $desc (enabled)" '
+		git -C tmp.git push origin HEAD:pushed
+	'
+
+	test_expect_success "push $desc (disabled)" '
+		git config --global protocol.allow never &&
+		test_must_fail git -C tmp.git push origin HEAD:pushed
+	'
+
+	test_expect_success "fetch $desc (disabled)" '
+		test_must_fail git -C tmp.git fetch
+	'
+
+	test_expect_success "clone $desc (disabled)" '
+		rm -rf tmp.git &&
+		test_must_fail git clone --bare "$url" tmp.git
+	'
+}
+
+# test cloning a particular protocol
+#   $1 - description of the protocol
+#   $2 - machine-readable name of the protocol
+#   $3 - the URL to try cloning
+test_proto () {
+	test_whitelist "$@"
+
+	test_config "$@"
 }
 
 # set up an ssh wrapper that will access $host/$repo in the
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac3..75c570a 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
 . ./test-lib.sh
 
 test_expect_success setup '
+	git config --global protocol.ext.allow user &&
 	test_tick &&
 	git init original &&
 	(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d..c6c2661 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
 . ./test-lib.sh
 
 test_expect_success setup '
+	git config --global protocol.ext.allow user &&
 	test_tick &&
 	git commit --allow-empty -m initial &&
 	test_tick &&
diff --git a/transport.c b/transport.c
index d57e8de..2c0ec76 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)
 	return enabled ? &allowed : NULL;
 }
 
+enum protocol_allow_config {
+	PROTOCOL_ALLOW_NEVER = 0,
+	PROTOCOL_ALLOW_USER_ONLY,
+	PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+							const char *value)
+{
+	if (!strcasecmp(value, "always"))
+		return PROTOCOL_ALLOW_ALWAYS;
+	else if (!strcasecmp(value, "never"))
+		return PROTOCOL_ALLOW_NEVER;
+	else if (!strcasecmp(value, "user"))
+		return PROTOCOL_ALLOW_USER_ONLY;
+
+	die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+	char *key = xstrfmt("protocol.%s.allow", type);
+	char *value;
+
+	/* first check the per-protocol config */
+	if (!git_config_get_string(key, &value)) {
+		enum protocol_allow_config ret =
+			parse_protocol_config(key, value);
+		free(key);
+		free(value);
+		return ret;
+	}
+	free(key);
+
+	/* if defined, fallback to user-defined default for unknown protocols */
+	if (!git_config_get_string("protocol.allow", &value)) {
+		enum protocol_allow_config ret =
+			parse_protocol_config("protocol.allow", value);
+		free(value);
+		return ret;
+	}
+
+	/* fallback to built-in defaults */
+	/* known safe */
+	if (!strcmp(type, "http") ||
+	    !strcmp(type, "https") ||
+	    !strcmp(type, "git") ||
+	    !strcmp(type, "ssh") ||
+	    !strcmp(type, "file"))
+		return PROTOCOL_ALLOW_ALWAYS;
+
+	/* known scary; err on the side of caution */
+	if (!strcmp(type, "ext"))
+		return PROTOCOL_ALLOW_NEVER;
+
+	/* unknown; by default let them be used only directly by the user */
+	return PROTOCOL_ALLOW_USER_ONLY;
+}
+
 int is_transport_allowed(const char *type)
 {
-	const struct string_list *allowed = protocol_whitelist();
-	return !allowed || string_list_has_string(allowed, type);
+	const struct string_list *whitelist = protocol_whitelist();
+	if (whitelist)
+		return string_list_has_string(whitelist, type);
+
+	switch (get_protocol_config(type)) {
+	case PROTOCOL_ALLOW_ALWAYS:
+		return 1;
+	case PROTOCOL_ALLOW_NEVER:
+		return 0;
+	case PROTOCOL_ALLOW_USER_ONLY:
+		return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+	}
+
+	die("BUG: invalid protocol_allow_config type");
 }
 
 void transport_check_allowed(const char *type)
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related

* [PATCH v7 1/4] lib-proto-disable: variable name fix
From: Brandon Williams @ 2016-12-01 20:25 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-1-git-send-email-bmwill@google.com>

The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 t/lib-proto-disable.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
 	proto=$2
 	url=$3
 
-	test_expect_success "clone $1 (enabled)" '
+	test_expect_success "clone $desc (enabled)" '
 		rm -rf tmp.git &&
 		(
 			GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
 		)
 	'
 
-	test_expect_success "fetch $1 (enabled)" '
+	test_expect_success "fetch $desc (enabled)" '
 		(
 			cd tmp.git &&
 			GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
 		)
 	'
 
-	test_expect_success "push $1 (enabled)" '
+	test_expect_success "push $desc (enabled)" '
 		(
 			cd tmp.git &&
 			GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
 		)
 	'
 
-	test_expect_success "push $1 (disabled)" '
+	test_expect_success "push $desc (disabled)" '
 		(
 			cd tmp.git &&
 			GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
 		)
 	'
 
-	test_expect_success "fetch $1 (disabled)" '
+	test_expect_success "fetch $desc (disabled)" '
 		(
 			cd tmp.git &&
 			GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
 		)
 	'
 
-	test_expect_success "clone $1 (disabled)" '
+	test_expect_success "clone $desc (disabled)" '
 		rm -rf tmp.git &&
 		(
 			GIT_ALLOW_PROTOCOL=none &&
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related

* [PATCH v7 3/4] http: always warn if libcurl version is too old
From: Brandon Williams @ 2016-12-01 20:25 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-1-git-send-email-bmwill@google.com>

Now that there are default "known-good" and "known-bad" protocols which
are allowed/disallowed by 'is_transport_allowed' we should always warn
the user that older versions of libcurl can't respect the allowed
protocols for redirects.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 http.c      | 5 ++---
 transport.c | 5 -----
 transport.h | 6 ------
 3 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/http.c b/http.c
index 4c4a812..fee128b 100644
--- a/http.c
+++ b/http.c
@@ -735,9 +735,8 @@ static CURL *get_curl_handle(void)
 		allowed_protocols |= CURLPROTO_FTPS;
 	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
 #else
-	if (transport_restrict_protocols())
-		warning("protocol restrictions not applied to curl redirects because\n"
-			"your curl version is too old (>= 7.19.4)");
+	warning("protocol restrictions not applied to curl redirects because\n"
+		"your curl version is too old (>= 7.19.4)");
 #endif
 	if (getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
diff --git a/transport.c b/transport.c
index 2c0ec76..186de9a 100644
--- a/transport.c
+++ b/transport.c
@@ -747,11 +747,6 @@ void transport_check_allowed(const char *type)
 		die("transport '%s' not allowed", type);
 }
 
-int transport_restrict_protocols(void)
-{
-	return !!protocol_whitelist();
-}
-
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	const char *helper;
diff --git a/transport.h b/transport.h
index b8e4ee8..f4998bc 100644
--- a/transport.h
+++ b/transport.h
@@ -164,12 +164,6 @@ int is_transport_allowed(const char *type);
  */
 void transport_check_allowed(const char *type);
 
-/*
- * Returns true if the user has attempted to turn on protocol
- * restrictions at all.
- */
-int transport_restrict_protocols(void);
-
 /* Transport options which apply to git:// and scp-style URLs */
 
 /* The program to use on the remote side to send a pack */
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related

* [PATCH v7 0/4] transport protocol policy configuration
From: Brandon Williams @ 2016-12-01 20:25 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480621447-52399-1-git-send-email-bmwill@google.com>

Changed the last patch in the series to use the parameter 'from_user' instead
of 'redirect'.  This allows us to use the same logic polarity and maintain use
of the same vocabulary.

Brandon Williams (4):
  lib-proto-disable: variable name fix
  transport: add protocol policy config option
  http: always warn if libcurl version is too old
  transport: add from_user parameter to is_transport_allowed

 Documentation/config.txt         |  46 +++++++++++++
 Documentation/git.txt            |  38 ++++-------
 git-submodule.sh                 |  12 ++--
 http.c                           |  13 ++--
 t/lib-proto-disable.sh           | 142 ++++++++++++++++++++++++++++++++++++---
 t/t5509-fetch-push-namespaces.sh |   1 +
 t/t5802-connect-helper.sh        |   1 +
 transport.c                      |  84 ++++++++++++++++++++---
 transport.h                      |  19 +++---
 9 files changed, 289 insertions(+), 67 deletions(-)

-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply

* [PATCHv3 5/5] submodule: add embed-git-dir function
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20161201202554.19944-1-sbeller@google.com>

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt   | 14 ++++++
 builtin/submodule--helper.c       | 39 ++++++++++++++++-
 dir.c                             | 78 +++++++++++++++++++++++++++++++++
 dir.h                             |  4 ++
 git-submodule.sh                  |  7 ++-
 submodule.h                       |  1 -
 t/t7412-submodule-embedgitdirs.sh | 92 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 232 insertions(+), 3 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] embedgitdirs [--] [<path>...]
 
 
 DESCRIPTION
@@ -245,6 +246,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+embedgitdirs::
+	Move the git directory of submodules into its superprojects
+	`$GIT_DIR/modules` path and then connect the git directory and
+	its working directory by setting the `core.worktree` and adding
+	a .git file pointing to the git directory interned into the
+	superproject.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 -------
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 806e29ce4e..f2df166d15 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,42 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+static int embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	unsigned flags = RELOCATE_GITDIR_RECURSE_SUBMODULES;
+
+	struct option embed_gitdir_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
+			RELOCATE_GITDIR_RECURSE_SUBMODULES),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper embed-git-dir [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+			     git_submodule_helper_usage, 0);
+
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	for (i = 0; i < list.nr; i++)
+		relocate_gitdir(prefix, list.entries[i]->name, flags);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url", resolve_relative_url, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"init", module_init, 0},
-	{"remote-branch", resolve_remote_submodule_branch, 0}
+	{"remote-branch", resolve_remote_submodule_branch, 0},
+	{"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/dir.c b/dir.c
index bfa8c8a9a5..7b3abc1340 100644
--- a/dir.c
+++ b/dir.c
@@ -15,6 +15,9 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
+#include "submodule-config.h"
+#include "run-command.h"
+#include "worktree.h"
 
 struct path_simplify {
 	int len;
@@ -2748,3 +2751,78 @@ void untracked_cache_add_to_index(struct index_state *istate,
 {
 	untracked_cache_invalidate_path(istate, path);
 }
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void relocate_gitdir(const char *prefix, const char *path, unsigned flags)
+{
+	char *old_git_dir;
+	const char *new_git_dir;
+	const struct submodule *sub;
+	struct worktree **worktrees;
+	int i;
+
+	worktrees = get_submodule_worktrees(path);
+	for (i = 0; worktrees[i]; i++)
+		;
+	if (i > 1)
+		die(_("relocate_gitdir for submodule with more than one worktree not supported"));
+
+	old_git_dir = xstrfmt("%s/.git", path);
+	if (read_gitfile(old_git_dir))
+		/* If it is an actual gitfile, it doesn't need migration. */
+		goto out;
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("Could not lookup name for submodule '%s'"),
+		      path);
+
+	new_git_dir = git_path("modules/%s", sub->name);
+	if (safe_create_leading_directories_const(new_git_dir) < 0)
+		die(_("could not create directory '%s'"), new_git_dir);
+
+	if (!prefix)
+		prefix = get_super_prefix();
+	printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
+		prefix ? prefix : "", path,
+		real_path(old_git_dir), new_git_dir);
+
+	if (rename(old_git_dir, new_git_dir) < 0)
+		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+
+	connect_work_tree_and_git_dir(path, new_git_dir);
+
+out:
+	if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf sb = STRBUF_INIT;
+
+		if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
+			die("BUG: we don't know how to pass the flags down?");
+
+		if (get_super_prefix())
+			strbuf_addstr(&sb, get_super_prefix());
+		strbuf_addstr(&sb, path);
+		strbuf_addch(&sb, '/');
+
+		cp.dir = path;
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
+					    "submodule--helper",
+					   "embed-git-dirs", NULL);
+		prepare_submodule_repo_env(&cp.env_array);
+		if (run_command(&cp))
+			die(_("Could not migrate git directory in submodule '%s'"),
+			    path);
+
+		strbuf_release(&sb);
+	}
+
+	free(old_git_dir);
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..0b5e99b21d 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,8 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
+
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern void relocate_gitdir(const char *prefix, const char *path, unsigned flags);
+
 #endif
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..b7e124f340 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
 	done
 }
 
+cmd_embedgitdirs()
+{
+	git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
+}
+
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.
@@ -1140,7 +1145,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | init | deinit | update | status | summary | sync | embedgitdirs)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/submodule.h b/submodule.h
index d9e197a948..1e42222cff 100644
--- a/submodule.h
+++ b/submodule.h
@@ -74,5 +74,4 @@ int parallel_submodules(void);
  * retaining any config in the environment.
  */
 void prepare_submodule_repo_env(struct argv_array *out);
-
 #endif
diff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh
new file mode 100755
index 0000000000..e3443b88cd
--- /dev/null
+++ b/t/t7412-submodule-embedgitdirs.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='Test submodule embedgitdirs
+
+This test verifies that `git submodue embedgitdirs` moves a submodules git
+directory into the superproject.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a real submodule' '
+	git init sub1 &&
+	test_commit -C sub1 first &&
+	git submodule add ./sub1 &&
+	test_tick &&
+	git commit -m superproject
+'
+
+test_expect_success 'embed the git dir' '
+	>expect.1 &&
+	>expect.2 &&
+	>actual.1 &&
+	>actual.2 &&
+	git status >expect.1 &&
+	git -C sub1 rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	git fsck &&
+	test -f sub1/.git &&
+	test -d .git/modules/sub1 &&
+	git status >actual.1 &&
+	git -C sub1 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup nested submodule' '
+	git init sub1/nested &&
+	test_commit -C sub1/nested first_nested &&
+	git -C sub1 submodule add ./nested &&
+	test_tick &&
+	git -C sub1 commit -m "add nested" &&
+	git add sub1 &&
+	git commit -m "sub1 to include nested submodule"
+'
+
+test_expect_success 'embed the git dir in a nested submodule' '
+	git status >expect.1 &&
+	git -C sub1/nested rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	test -f sub1/nested/.git &&
+	test -d .git/modules/sub1/modules/nested &&
+	git status >actual.1 &&
+	git -C sub1/nested rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a gitlink with missing .gitmodules entry' '
+	git init sub2 &&
+	test_commit -C sub2 first &&
+	git add sub2 &&
+	git commit -m superproject
+'
+
+test_expect_success 'embedding the git dir fails for incomplete submodules' '
+	git status >expect.1 &&
+	git -C sub2 rev-parse HEAD >expect.2 &&
+	test_must_fail git submodule embedgitdirs &&
+	git -C sub2 fsck &&
+	test -d sub2/.git &&
+	git status >actual &&
+	git -C sub2 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a submodule with multiple worktrees' '
+	# first create another unembedded git dir in a new submodule
+	git init sub3 &&
+	test_commit -C sub3 first &&
+	git submodule add ./sub3 &&
+	test_tick &&
+	git commit -m "add another submodule" &&
+	git -C sub3 worktree add ../sub3_second_work_tree
+'
+
+test_expect_success 'embed a submodule with multiple worktrees' '
+	test_must_fail git submodule embedgitdirs sub3 2>error &&
+	test_i18ngrep "not supported" error
+'
+
+test_done
-- 
2.10.2.613.g22f2156


^ permalink raw reply related

* [PATCHv3 4/5] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20161201202554.19944-1-sbeller@google.com>

In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
worktrees for submodules.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 worktree.c | 40 ++++++++++++++++++++++++++++++----------
 worktree.h |  5 +++++
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/worktree.c b/worktree.c
index f7869f8d60..fb1f72bbf3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
 	int is_bare = 0;
 	int is_detached = 0;
 
-	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_add_absolute_path(&worktree_path, git_common_dir);
 	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
 	if (is_bare)
 		strbuf_strip_suffix(&worktree_path, "/.");
 
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_addf(&path, "%s/HEAD", git_common_dir);
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
@@ -109,7 +109,8 @@ static struct worktree *get_main_worktree(void)
 	return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+					    const char *id)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -120,7 +121,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (!id)
 		die("Missing linked worktree name");
 
-	strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
+	strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
 	if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
 		/* invalid gitdir file */
 		goto done;
@@ -133,7 +134,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
@@ -173,7 +174,7 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
-struct worktree **get_worktrees(void)
+struct worktree **get_worktrees_internal(const char *git_common_dir)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -183,10 +184,10 @@ struct worktree **get_worktrees(void)
 
 	list = xmalloc(alloc * sizeof(struct worktree *));
 
-	if ((list[counter] = get_main_worktree()))
+	if ((list[counter] = get_main_worktree(git_common_dir)))
 		counter++;
 
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	strbuf_addf(&path, "%s/worktrees", git_common_dir);
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (dir) {
@@ -195,7 +196,7 @@ struct worktree **get_worktrees(void)
 			if (is_dot_or_dotdot(d->d_name))
 				continue;
 
-			if ((linked = get_linked_worktree(d->d_name))) {
+			if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
 				ALLOC_GROW(list, counter + 1, alloc);
 				list[counter++] = linked;
 			}
@@ -209,6 +210,25 @@ struct worktree **get_worktrees(void)
 	return list;
 }
 
+struct worktree **get_worktrees(void)
+{
+	return get_worktrees_internal(get_git_common_dir());
+}
+
+struct worktree **get_submodule_worktrees(const char *path)
+{
+	const char *submodule_common_dir;
+	struct strbuf sb = STRBUF_INIT;
+	struct worktree **ret;
+	strbuf_addf(&sb, "%s/.git", path);
+	submodule_common_dir = resolve_gitdir(sb.buf);
+
+	ret = get_worktrees_internal(submodule_common_dir);
+
+	strbuf_release(&sb);
+	return ret;
+}
+
 const char *get_worktree_git_dir(const struct worktree *wt)
 {
 	if (!wt)
diff --git a/worktree.h b/worktree.h
index 90e1311fa7..c93845516c 100644
--- a/worktree.h
+++ b/worktree.h
@@ -25,6 +25,11 @@ struct worktree {
  */
 extern struct worktree **get_worktrees(void);
 
+/*
+ * Get the worktrees of a submodule named by `path`.
+ */
+extern struct worktree **get_submodule_worktrees(const char *path);
+
 /*
  * Return git dir of the worktree. Note that the path may be relative.
  * If wt is NULL, git dir of current worktree is returned.
-- 
2.10.2.613.g22f2156


^ permalink raw reply related

* [PATCHv3 0/5] submodule embedgitdirs
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller

v3:
* have a slightly more generic function "relocate_gitdir".
  The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
  This also lays the groundwork for later doing the proper thing,
  as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir

* diff to v2 (as queued by Junio) below.

Stefan Beller (5):
  submodule: use absolute path for computing relative path connecting
  submodule helper: support super prefix
  test-lib-functions.sh: teach test_commit -C <dir>
  worktree: get worktrees from submodules
  submodule: add embed-git-dir function

 Documentation/git-submodule.txt   | 14 ++++++
 builtin/submodule--helper.c       | 68 ++++++++++++++++++++++++-----
 dir.c                             | 78 +++++++++++++++++++++++++++++++++
 dir.h                             |  4 ++
 git-submodule.sh                  |  7 ++-
 git.c                             |  2 +-
 submodule.c                       | 12 ++---
 submodule.h                       |  1 -
 t/t7412-submodule-embedgitdirs.sh | 92 +++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh           | 20 ++++++---
 worktree.c                        | 40 ++++++++++++-----
 worktree.h                        |  5 +++
 12 files changed, 309 insertions(+), 34 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

v2:
* fixed commit message for patch:
 "submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C <dir>" unchanged

v1:
The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory.

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 75cdbf45b8..f2df166d15 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1081,11 +1081,14 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
 	int i;
 	struct pathspec pathspec;
 	struct module_list list = MODULE_LIST_INIT;
+	unsigned flags = RELOCATE_GITDIR_RECURSE_SUBMODULES;
 
 	struct option embed_gitdir_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
+		OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
+			RELOCATE_GITDIR_RECURSE_SUBMODULES),
 		OPT_END()
 	};
 
@@ -1104,7 +1107,7 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	for (i = 0; i < list.nr; i++)
-		migrate_submodule_gitdir(prefix, list.entries[i]->name, 1);
+		relocate_gitdir(prefix, list.entries[i]->name, flags);
 
 	return 0;
 }
diff --git a/dir.c b/dir.c
index bfa8c8a9a5..7b3abc1340 100644
--- a/dir.c
+++ b/dir.c
@@ -15,6 +15,9 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
+#include "submodule-config.h"
+#include "run-command.h"
+#include "worktree.h"
 
 struct path_simplify {
 	int len;
@@ -2748,3 +2751,78 @@ void untracked_cache_add_to_index(struct index_state *istate,
 {
 	untracked_cache_invalidate_path(istate, path);
 }
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void relocate_gitdir(const char *prefix, const char *path, unsigned flags)
+{
+	char *old_git_dir;
+	const char *new_git_dir;
+	const struct submodule *sub;
+	struct worktree **worktrees;
+	int i;
+
+	worktrees = get_submodule_worktrees(path);
+	for (i = 0; worktrees[i]; i++)
+		;
+	if (i > 1)
+		die(_("relocate_gitdir for submodule with more than one worktree not supported"));
+
+	old_git_dir = xstrfmt("%s/.git", path);
+	if (read_gitfile(old_git_dir))
+		/* If it is an actual gitfile, it doesn't need migration. */
+		goto out;
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("Could not lookup name for submodule '%s'"),
+		      path);
+
+	new_git_dir = git_path("modules/%s", sub->name);
+	if (safe_create_leading_directories_const(new_git_dir) < 0)
+		die(_("could not create directory '%s'"), new_git_dir);
+
+	if (!prefix)
+		prefix = get_super_prefix();
+	printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
+		prefix ? prefix : "", path,
+		real_path(old_git_dir), new_git_dir);
+
+	if (rename(old_git_dir, new_git_dir) < 0)
+		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+
+	connect_work_tree_and_git_dir(path, new_git_dir);
+
+out:
+	if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf sb = STRBUF_INIT;
+
+		if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
+			die("BUG: we don't know how to pass the flags down?");
+
+		if (get_super_prefix())
+			strbuf_addstr(&sb, get_super_prefix());
+		strbuf_addstr(&sb, path);
+		strbuf_addch(&sb, '/');
+
+		cp.dir = path;
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
+					    "submodule--helper",
+					   "embed-git-dirs", NULL);
+		prepare_submodule_repo_env(&cp.env_array);
+		if (run_command(&cp))
+			die(_("Could not migrate git directory in submodule '%s'"),
+			    path);
+
+		strbuf_release(&sb);
+	}
+
+	free(old_git_dir);
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..0b5e99b21d 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,8 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
+
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern void relocate_gitdir(const char *prefix, const char *path, unsigned flags);
+
 #endif
diff --git a/submodule.c b/submodule.c
index d330b567a3..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1263,68 +1263,3 @@ void prepare_submodule_repo_env(struct argv_array *out)
 	}
 	argv_array_push(out, "GIT_DIR=.git");
 }
-
-/*
- * Migrate the given submodule (and all its submodules recursively) from
- * having its git directory within the working tree to the git dir nested
- * in its superprojects git dir under modules/.
- */
-void migrate_submodule_gitdir(const char *prefix, const char *path,
-			      int recursive)
-{
-	char *old_git_dir;
-	const char *new_git_dir;
-	const struct submodule *sub;
-
-	old_git_dir = xstrfmt("%s/.git", path);
-	if (read_gitfile(old_git_dir))
-		/* If it is an actual gitfile, it doesn't need migration. */
-		goto out;
-
-	sub = submodule_from_path(null_sha1, path);
-	if (!sub)
-		die(_("Could not lookup name for submodule '%s'"),
-		      path);
-
-	new_git_dir = git_common_path("modules/%s", sub->name);
-	if (safe_create_leading_directories_const(new_git_dir) < 0)
-		die(_("could not create directory '%s'"), new_git_dir);
-
-	if (!prefix)
-		prefix = get_super_prefix();
-	printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
-		prefix ? prefix : "", path,
-		real_path(old_git_dir), new_git_dir);
-
-	if (rename(old_git_dir, new_git_dir) < 0)
-		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
-			old_git_dir, new_git_dir);
-
-	connect_work_tree_and_git_dir(path, new_git_dir);
-
-out:
-	if (recursive) {
-		struct child_process cp = CHILD_PROCESS_INIT;
-		struct strbuf sb = STRBUF_INIT;
-
-		if (get_super_prefix())
-			strbuf_addstr(&sb, get_super_prefix());
-		strbuf_addstr(&sb, path);
-		strbuf_addch(&sb, '/');
-
-		cp.dir = path;
-		cp.git_cmd = 1;
-		cp.no_stdin = 1;
-		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
-					    "submodule--helper",
-					   "embed-git-dirs", NULL);
-		prepare_submodule_repo_env(&cp.env_array);
-		if (run_command(&cp))
-			die(_("Could not migrate git directory in submodule '%s'"),
-			    path);
-
-		strbuf_release(&sb);
-	}
-
-	free(old_git_dir);
-}
diff --git a/submodule.h b/submodule.h
index e5975d1f3d..1e42222cff 100644
--- a/submodule.h
+++ b/submodule.h
@@ -74,7 +74,4 @@ int parallel_submodules(void);
  * retaining any config in the environment.
  */
 void prepare_submodule_repo_env(struct argv_array *out);
-
-extern void migrate_submodule_gitdir(const char *prefix,
-				     const char *path, int recursive);
 #endif
diff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh
index 1cf5d4b4af..e3443b88cd 100755
--- a/t/t7412-submodule-embedgitdirs.sh
+++ b/t/t7412-submodule-embedgitdirs.sh
@@ -74,4 +74,19 @@ test_expect_success 'embedding the git dir fails for incomplete submodules' '
 	test_cmp expect.2 actual.2
 '
 
+test_expect_success 'setup a submodule with multiple worktrees' '
+	# first create another unembedded git dir in a new submodule
+	git init sub3 &&
+	test_commit -C sub3 first &&
+	git submodule add ./sub3 &&
+	test_tick &&
+	git commit -m "add another submodule" &&
+	git -C sub3 worktree add ../sub3_second_work_tree
+'
+
+test_expect_success 'embed a submodule with multiple worktrees' '
+	test_must_fail git submodule embedgitdirs sub3 2>error &&
+	test_i18ngrep "not supported" error
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index f7869f8d60..fb1f72bbf3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
 	int is_bare = 0;
 	int is_detached = 0;
 
-	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_add_absolute_path(&worktree_path, git_common_dir);
 	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
 	if (is_bare)
 		strbuf_strip_suffix(&worktree_path, "/.");
 
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_addf(&path, "%s/HEAD", git_common_dir);
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
@@ -109,7 +109,8 @@ static struct worktree *get_main_worktree(void)
 	return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+					    const char *id)
 {
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -120,7 +121,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (!id)
 		die("Missing linked worktree name");
 
-	strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
+	strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
 	if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
 		/* invalid gitdir file */
 		goto done;
@@ -133,7 +134,7 @@ static struct worktree *get_linked_worktree(const char *id)
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
@@ -173,7 +174,7 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
-struct worktree **get_worktrees(void)
+struct worktree **get_worktrees_internal(const char *git_common_dir)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -183,10 +184,10 @@ struct worktree **get_worktrees(void)
 
 	list = xmalloc(alloc * sizeof(struct worktree *));
 
-	if ((list[counter] = get_main_worktree()))
+	if ((list[counter] = get_main_worktree(git_common_dir)))
 		counter++;
 
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	strbuf_addf(&path, "%s/worktrees", git_common_dir);
 	dir = opendir(path.buf);
 	strbuf_release(&path);
 	if (dir) {
@@ -195,7 +196,7 @@ struct worktree **get_worktrees(void)
 			if (is_dot_or_dotdot(d->d_name))
 				continue;
 
-			if ((linked = get_linked_worktree(d->d_name))) {
+			if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
 				ALLOC_GROW(list, counter + 1, alloc);
 				list[counter++] = linked;
 			}
@@ -209,6 +210,25 @@ struct worktree **get_worktrees(void)
 	return list;
 }
 
+struct worktree **get_worktrees(void)
+{
+	return get_worktrees_internal(get_git_common_dir());
+}
+
+struct worktree **get_submodule_worktrees(const char *path)
+{
+	const char *submodule_common_dir;
+	struct strbuf sb = STRBUF_INIT;
+	struct worktree **ret;
+	strbuf_addf(&sb, "%s/.git", path);
+	submodule_common_dir = resolve_gitdir(sb.buf);
+
+	ret = get_worktrees_internal(submodule_common_dir);
+
+	strbuf_release(&sb);
+	return ret;
+}
+
 const char *get_worktree_git_dir(const struct worktree *wt)
 {
 	if (!wt)
diff --git a/worktree.h b/worktree.h
index 90e1311fa7..c93845516c 100644
--- a/worktree.h
+++ b/worktree.h
@@ -25,6 +25,11 @@ struct worktree {
  */
 extern struct worktree **get_worktrees(void);
 
+/*
+ * Get the worktrees of a submodule named by `path`.
+ */
+extern struct worktree **get_submodule_worktrees(const char *path);
+
 /*
  * Return git dir of the worktree. Note that the path may be relative.
  * If wt is NULL, git dir of current worktree is returned.


-- 
2.10.2.613.g22f2156


^ permalink raw reply related

* [PATCHv3 3/5] test-lib-functions.sh: teach test_commit -C <dir>
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20161201202554.19944-1-sbeller@google.com>

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C <dir>" similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib-functions.sh | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 	 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
+# Call test_commit with the arguments
+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # <file>, <contents>, and <tag> all default to <message>.
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
 	notick= &&
 	signoff= &&
+	indir= &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
 		--signoff)
 			signoff="$1"
 			;;
+		-C)
+			indir="$2"
+			shift
+			;;
 		*)
 			break
 			;;
 		esac
 		shift
 	done &&
+	indir=${indir:+"$indir"/} &&
 	file=${2:-"$1.t"} &&
-	echo "${3-$1}" > "$file" &&
-	git add "$file" &&
+	echo "${3-$1}" > "$indir$file" &&
+	git ${indir:+ -C "$indir"} add "$file" &&
 	if test -z "$notick"
 	then
 		test_tick
 	fi &&
-	git commit $signoff -m "$1" &&
-	git tag "${4:-$1}"
+	git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+	git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments "<message> <commit>", where <commit>
-- 
2.10.2.613.g22f2156


^ permalink raw reply related

* [PATCHv3 2/5] submodule helper: support super prefix
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20161201202554.19944-1-sbeller@google.com>

Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
 git.c                       |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..806e29ce4e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
+	int option;
 };
 
 static struct cmd_struct commands[] = {
-	{"list", module_list},
-	{"name", module_name},
-	{"clone", module_clone},
-	{"update-clone", update_clone},
-	{"relative-path", resolve_relative_path},
-	{"resolve-relative-url", resolve_relative_url},
-	{"resolve-relative-url-test", resolve_relative_url_test},
-	{"init", module_init},
-	{"remote-branch", resolve_remote_submodule_branch}
+	{"list", module_list, 0},
+	{"name", module_name, 0},
+	{"clone", module_clone, 0},
+	{"update-clone", update_clone, 0},
+	{"relative-path", resolve_relative_path, 0},
+	{"resolve-relative-url", resolve_relative_url, 0},
+	{"resolve-relative-url-test", resolve_relative_url_test, 0},
+	{"init", module_init, 0},
+	{"remote-branch", resolve_remote_submodule_branch, 0}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 		die(_("submodule--helper subcommand must be "
 		      "called with a subcommand"));
 
-	for (i = 0; i < ARRAY_SIZE(commands); i++)
-		if (!strcmp(argv[1], commands[i].cmd))
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		if (!strcmp(argv[1], commands[i].cmd)) {
+			if (get_super_prefix() &&
+			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
+				die("%s doesn't support --super-prefix",
+				    commands[i].cmd);
 			return commands[i].fn(argc - 1, argv + 1, prefix);
+		}
+	}
 
 	die(_("'%s' is not a valid submodule--helper "
 	      "subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
-	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.10.2.613.g22f2156


^ permalink raw reply related

* [PATCHv3 1/5] submodule: use absolute path for computing relative path connecting
From: Stefan Beller @ 2016-12-01 20:25 UTC (permalink / raw)
  To: pclouds; +Cc: git, bmwill, gitster, Stefan Beller
In-Reply-To: <20161201202554.19944-1-sbeller@google.com>

The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.

We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function much harder
to misuse.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	const char *real_work_tree = xstrdup(real_path(work_tree));
+	char *real_git_dir = xstrdup(real_path(git_dir));
+	char *real_work_tree = xstrdup(real_path(work_tree));
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
 	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, real_work_tree, &rel_path));
+		   relative_path(real_git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
 	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", git_dir);
+	strbuf_addf(&file_name, "%s/config", real_git_dir);
 	git_config_set_in_file(file_name.buf, "core.worktree",
-			       relative_path(real_work_tree, git_dir,
+			       relative_path(real_work_tree, real_git_dir,
 					     &rel_path));
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
-	free((void *)real_work_tree);
+	free(real_work_tree);
+	free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.10.2.613.g22f2156


^ 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