Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack

[Jeff King]

On Tue, May 24, 2016 at 05:59:15PM -0700, Junio C Hamano wrote:

> On Wed, May 18, 2016 at 3:37 PM, Jeff King  wrote:
> > I've often wanted to intercept the call from upload-pack to
> > pack-objects. The final patch in this series goes into more detail, but
> > basically it's good for:
> >
> >   1. Capturing the output from pack-objects for debugging/inspection.
> >
> >   2. Capturing the input to pack-objects to replay for debugging or
> >  performance analysis.
> >
> >   3. Caching pack-objects output.
> >
> > It's pretty trivial to add a hook to run instead of pack-objects (and
> > the hook would just run pack-objects itself). But we don't want to run
> > hooks in upload-pack, because we don't necessarily trust the repository
> > we're running in.
> 
> Although I'd need to study the final step a bit more carefully than I did,
> overall I think these are good changes. The way the callbacks learn
> about the origin of the configuration may have to be rethought in the
> long run, though. We already have been relying on a filename thing
> kept separately as a global variable, and the approach taken by this
> series extends it, so we are not making anything fundamentally worse,
> but at some point we may need to bite the bullet and pass kv-info as
> an extra callback parameter or something.

Yeah, I had the same thought while working on this, but just didn't want
to have to tweak every config callback. As you say, I don't think this
makes anything fundamentally worse, though. I'm inclined to go with this
strategy, especially with the extra die("BUG") safety added here. But I
can look into changing the callbacks if you feel strongly.

-Peff

PS Did you have any thoughts on the t/helper problem mentioned in:

 http://article.gmane.org/gmane.comp.version-control.git/295029

   I suspect it will bite you if you try merging/testing this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects

[Jeff King]

On Thu, May 19, 2016 at 04:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > This is the "could we just set a bool" option I discussed in the commit
> > message. The problem is that it doesn't let the admin say "I don't trust
> > these repositories, but I _do_ want to run just this one hook when
> > serving them, and not any other hooks".
> 
> Indeed. I wonder if there's really any overlap in practice between
> systems administrators on a central Git server that are going to want
> this relatively obscure feature *but* have potentially malicious users
> / repos, or enough to warrant this unusual edge case in how this
> specific hook is configured, as opposed to reducing the special case
> in how the hook is run with something like core.runDangerousHooks.

I dunno. Certainly I am not running such a site. But something like
kernel.org might fit into that boat. For a long time I think people had
actual shell accounts and a common git-daemon served the repositories. I
think that these days it might be more locked-down, though.

> I'm definitely not saying that these patches should be blocked by
> this, but it occurs to me that both your uploadpack.packObjectsHook
> implementation and my proposed core.runDangerousHooks which normalizes
> it a bit in some ways, but leaves it as a special case in others, are
> both stumbling their way toward hacks that we might also solve with
> some generally configurable restrictions system that takes advantage
> of your earlier patches, e.g.:
> 
> $ cat /etc/gitconfig
> # Not "repository" so hooksPath can't be set per-repo
> [core]
> configRestriction = "core.hooksPath: system, global"

I was hoping to avoid setting up configuration restriction via the
configuration files, if only because it implies some ordering in the
parsing. So for example, you'd need to do a separate pass to load the
restrictions system, and then actually parse the config.

I guess that's not too bad with the caching system that's in place,
though.

> Of course those are some rather large hoops to jump through just to
> accomplish this particular thing, but it would be more generally
> composable and you could e.g. say users can't disable gc.auto or
> whatever on their repos if they're hosted on your server. Which of
> course assumes that you control the git binary and they can't run
> their own.

Yeah, I was also hoping to avoid something too baroque. :) I don't know
if there's much value in restricting things like gc.auto. If they can
make arbitrary edits to the config file, they can run arbitrary code. I
think this is _just_ about protecting a git-daemon serving the untrusted
repositories (or a user fetching from an untrusted other-user on the
system).

> Yeah, the reason I'm prodding you about this is because I want to test
> this out at some point, and a *really* nice thing about the Git
> configuration facility is that you can test all these sorts of things
> on a per-repo basis now due to how all the git-config variables work
> now.
> 
> With uploadpack.packObjectsHook you *can* do that by defining a global
> pass-through hook, but it makes it more of a hassle to test changes
> that straddle the divide between testing & production.

One thing I didn't elaborate on is that the "don't respect this key from
the repo config" could be made more featureful. For example, your
core.allowDangerousHooks could just as easily be an environment
variable: $GIT_ALLOW_DANGEROUS_CONFIG. [1] 

And then you could set that on your servers, and only set
uploadpack.packObjectsHook in the repositories you wanted, achieving
your goal.

This does still leave the pack-objects hook unlike the other hooks (in
that it leaves the command in the config rather than in a script), but I
actually like that flexibility. Being able to use "git -c" to set the
hook for a one-shot invocation is kind of nice (though you do have to do
tricks with "--upload-pack=" to get it to cross the remote boundary).

-Peff

[1] We also talked long ago (in the v1.6.x days, regarding a
post-upload-pack hook) of auto-enabling "dangerous" hooks when getuid()
matched the owner of the hook. We could do the same thing for the config
file (though TBH, it is confusing enough of a rule that I think I prefer
something like the explicit environment variable).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port

[Torsten Bögershausen]

On 05/26/2016 01:34 AM, Mike Hommey wrote:

On Tue, May 24, 2016 at 06:44:26AM +0200, Torsten Bögershausen wrote:

On 05/23/2016 11:30 PM, Junio C Hamano wrote:

Torsten Bögershausen  writes:


get_host_and_port(_host, );
+   /* get_host_and_port may not return a port
even when
+* there is one: In the [host:port]:path case,
+* get_host_and_port is called with "[host:port]" and
+* returns "host:port" and NULL.
+* In that specific case, we still need to split the
+* port. */

Is it worth to mention that this case is "still supported legacy" ?

If it's worth mentioning anywhere, it seems to me it would start with
urls.txt?

Mike


I don't know.
urls.txt is for Git users, and connect.c is for Git developers.
urls.txt does not mention that Git follows any RFC when parsing the
URLS', it doesn't claim to be 100% compliant.
Even if it makes sense to do so, as many user simply expect Git to accept
RFC compliant URL's, and it makes the development easier, if there is
an already
written specification, that describes all the details.
The parser is not 100% RFC compliant, one example:
- old-style usgage like "git clone [host:222]:~/path/to/repo are supported

Is it an option to fix get_host_and_port() so that it returns what
the caller expects even when it is given "[host:port]"?  When the
caller passes other forms like "host:port", it expects to get "host"
and "port" parsed out into two variables.  Why can't the caller
expect to see the same happen when feeding "[host:port]" to the
function?


This is somewhat out of my head:
git clone   git://[example.com:123]:/test#illegal, malformated URL
git clone   [example.com:123]:/test   #scp-like URL, legacy
git clone   ssh://[example.com:123]:/test   #illegal, but supported as
legacy, because
git clone  ssh://[user@::1]/test   # was the only way to
specify a user name at a literal IPv6 address

May be we should have some  more test cases for malformated git:// URLs?


None of these malformed urls are rejected with or without my series
applied:

Without:
$ git fetch-pack --diag-url git://[example.com:123]:/test
Diag: url=git://[example.com:123]:/test
Diag: protocol=git
Diag: hostandport=[example.com:123]:
Diag: path=/test
$ git fetch-pack --diag-url
ssh://[example.com:123]:/test
Diag: url=ssh://[example.com:123]:/test
Diag: protocol=ssh
Diag: userandhost=example.com
Diag: port=123
Diag: path=/test

With:
$ git fetch-pack --diag-url git://[example.com:123]:/test
Diag: url=git://[example.com:123]:/test
Diag: protocol=git
Diag: user=NULL
Diag: host=example.com
Diag: port=123
Diag: path=/test
$ git fetch-pack --diag-url ssh://[example.com:123]:/test
Diag: url=ssh://[example.com:123]:/test
Diag: protocol=ssh
Diag: user=NULL
Diag: host=example.com
Diag: port=123
Diag: path=/test

Note in the first case, hostandport is "[example.com:123]:", and that
is treated as host=example.com:123 and port=NULL further down, so my
series is changing something here, but arguably, it makes it less worse.
(note that both with and without my series,
"git://[example.com:123]:42/path" is treated the same, so only a corner
case changed)

Can we go forward with the current series (modulo the comment style
thing Eric noted, and maybe adding a note about the parser handling urls
as per urls.txt), and not bloat scope it? If anything, the state of the
code after the series should make further parser changes easier.

Cheers,

Mike




Thanks for digging.

How about something like this:

/*
 * get_host_and_port may not return a port in the [host:port]:path case.
 * To support this undocumented legacy we still need to split the port.
*/

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fetch: better alignment in ref summary

[Jeff King]

On Sun, May 22, 2016 at 06:20:18PM +0700, Nguyễn Thái Ngọc Duy wrote:

> To make all "->" aligned, we may need to go through the ref list
> twice, or buffer the output and let column.c align it. Either way
> needs a lot more work than this.

I don't think a two-pass approach is _too_ bad. The trickiest thing is
that we handle the "prune" refs separately, even though they go in the
same status table.

However, I tried it, and the results looked much worse for my example
repo than yours. The problem is that I had one gigantic refname, and
that shoved the "->" and everything after far to the right, where they
wrapped to the next line.

Though the stair-stepping in your patch is funny, the output is easier
to read.

I do agree with Junio that we could probably improve the output quite a
bit by not showing each refname twice in the common case. I don't quite
find the "{ -> origin/}whatever" particularly pretty, but something like
that which keeps the variable bits at the end would make this problem
just go away.

> -#define REFCOL_WIDTH  10
> +static int REFCOL_WIDTH = 10;

This should probably go lower-case if it's not a preprocessor macro
anymore. I know it makes the diff a lot noisier, but I think it's worth
it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] travis-ci: disable verbose test output

[Jeff King]

On Sun, May 22, 2016 at 01:00:55PM +0200, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> The verbose output clutters the Travis CI webview and is not really
> useful since test debugging usually happens on a local machine.

I have not really been using the Travis CI results, so perhaps my
opinion does not count. But in other systems, I have found that the more
verbose the CI output, the better, simply because you will inevitably be
faced with a test that breaks on CI and not your local machine, and you
will have no way to get more details.

I don't know if Travis provides a better way to hide the output in the
non-failing cases.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Jeff King]

On Fri, May 20, 2016 at 01:39:06PM -0700, Junio C Hamano wrote:

> > +{
> > +   size_t i;
> > +   struct strbuf out = STRBUF_INIT;
> > +   unsigned int width = 80;
> 
> In a few places Git limits the width of the output, like using function
> name in hunk header lines and drawing diffstat in "diff --stat", we
> do default to limit the total width to 80 display columns.
> 
> Given that this routine prefixes each and every line with a short
> heading like "=> Send header: " that costs at around 15-20 columns,
> and the loop we see below only counts the true payload without
> counting the heading, you would probably want to reduce this
> somewhat so that the whole thing would fit within 80 columns.

I think that may be a losing battle. Remember that we are getting the
usual trace header on top of this, which I think leaves only about 20
characters for actual data on each line.

I kind of doubt people will manually read the body data. In my
experience, debugging git-over-http you want either:

  1. Just the request/response headers to see what was said.

  2. A complete dump of each body with no other formatting (e.g., so you
 can replay it with curl).

This trace output gives you (1). You can in theory generate (2) from it
with a perl snippet, but the non-printing characters have been
irreversibly transformed.

So I dunno. I do not mind having the body stuff there for completeness,
but I doubt I'd use it myself. And I don't care much either way about
how long its lines are.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Formatting variables in the documentation

[Jeff King]

On Mon, May 23, 2016 at 07:57:43PM +0200, Matthieu Moy wrote:

> Samuel GROOT  writes:
> 
> > Since 2.8.3 was out recently, we could flip MAN_BOLD_LITERAL on by
> > default for this cycle to shake out problems as Jeff King suggested
> > [2].
> 
> 2.8.3 was a bufix release, and flipping a controversial flag should
> clearly not be done on a bugfix release. So, in this context, "beginning
> of a cycle" means after a x.y.0 release.
> 
> Anyway, a patch enabling MAN_BOLD_LITERAL by default would need to cook
> in pu and next as any other patches, so the time when the patch is sent
> does not really matter.

Yeah, I think a reasonable plan is:

  1. Somebody produces a patch flipping the default. The patch is
 trivial, but the commit message should tell why, and try to dig up
 any possible problems we might see (e.g., why wasn't this the
 default? Particular versions of tools? Some platforms?)

  2. Assuming no problems, Junio merges the patch to "next". We get
 any reports of issues from people using "next" day-to-day.

  3. Assuming no problems, Junio merges to "master". We hit more people
 (who build from master). And also it would be part of the
 pre-generated pages that Junio ships, so we might get reports
 there.

  4. Eventually it's released. We hope to get no problem reports there,
 though it _does_ hit a wider audience at that point.

Steps 1 and 2 can happen now. As we are in the -rc cycle right now,
probably step 3 would happen post-v2.9. But there's no reason not to
start the clock ticking now.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] archive-tar: convert snprintf to xsnprintf

[Jeff King]

On Tue, May 24, 2016 at 12:44:24AM +, Green, Paul wrote:

> While examining (relatively) recent changes to git, my eye happened to
> notice the following inconsistency on line 184 of the current version
> of archive-tar.c. 
> 
> -sprintf(header->chksum, "%07o", ustar_header_chksum(header));
> +snprintf(header->chksum, sizeof(header->chksum), "%07o", 
> ustar_header_chksum(header));
> 
> I believe the author meant to invoke the xsnprintf function, not the
> snprintf function. I say this because all of the other references to
> sprintf were indeed changed to xsnprintf, with the necessary
> additional 2nd argument.

Yep, it was indeed just a typo. Thanks for noticing.

-- >8 --
Subject: archive-tar: convert snprintf to xsnprintf

Commit f2f0267 (archive-tar: use xsnprintf for trivial
formatting, 2015-09-24) converted cases of "sprintf" to
"xsnprintf", but accidentally left one as just "snprintf".
This meant that we could silently truncate the resulting
buffer instead of flagging an error.

In practice, this is impossible to achieve, as we are
formatting a ustar checksum, which can be at most 7
characters. But the point of xsnprintf is to document and
check for "should be impossible" conditions; this site was
just accidentally mis-converted during f2f0267.

Noticed-by: Paul Green 
Signed-off-by: Jeff King 
---
 archive-tar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/archive-tar.c b/archive-tar.c
index 501ca97..cb99df2 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -181,7 +181,7 @@ static void prepare_header(struct archiver_args *args,
memcpy(header->magic, "ustar", 6);
memcpy(header->version, "00", 2);
 
-   snprintf(header->chksum, sizeof(header->chksum), "%07o", 
ustar_header_chksum(header));
+   xsnprintf(header->chksum, sizeof(header->chksum), "%07o", 
ustar_header_chksum(header));
 }
 
 static int write_extended_header(struct archiver_args *args,
-- 
2.9.0.rc0.307.gc679867

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xemit.c: fix a [-Wchar-subscripts] compiler warning

[René Scharfe]

[sorry for duplicate]

Am 26.05.2016 um 01:06 schrieb Ramsay Jones:


While compiling on cygwin (x86_64), gcc complains thus:

   CC xdiff/xemit.o
   xdiff/xemit.c: In function ‘is_empty_rec’:
   xdiff/xemit.c:163:2: warning: array subscript has type ‘char’ 
[-Wchar-subscripts]
 while (len > 0 && isspace(*rec)) {
 ^


Ah, it's not using our own isspace(), which works fine with signed 
chars, because it doesn't include git-compat-util.h.  I'm spoilt by 
those char classifier macros that can be actually used with chars.. 
Thanks for catching!


René

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7610-mergetool.sh test failure

[Jeff King]

On Wed, May 25, 2016 at 06:16:15PM -0500, Jeff King wrote:

> On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:
> 
> > On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> >  wrote:
> > > t7610-mergetool.sh fails on systems without mktemp.
> > >
> > > mktemp is used in git-mergetool.sh and throws an error when it's not 
> > > available.
> > > error: mktemp is needed when 'mergetool.writeToTemp' is true
> > >
> > > I see 2 options:
> > > 1. code around it, write an own mktemp, maybe use the test-mktemp as a 
> > > basis.
> > > 2. disable the test when mktemp is not available
> > 
> > 3. find and install mktemp?
> 
> I'd agree more with (3) if it was some critical part of git-mergetool.
> But it seems to be a completely optional feature that we use in only one
> place, and git-mergetool even detects this case and complains.
> 
> So another alternative would be for the test to detect either that
> mergetool worked, or that we got the "mktemp is needed" error.

BTW, one thing I happened to note while looking at this: running the
test script will write into /tmp (or wherever your $TMPDIR points).
Probably not a big deal, but I wonder if we should be setting $TMPDIR in
our test harness.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Please add a git config option to make --show-signature the default

[Austin English]

On Wed, May 25, 2016 at 1:18 PM, Mehul Jain  wrote:
> Hi,
>
> On Wed, May 25, 2016 at 9:28 AM, Austin English  
> wrote:
>> I'll try
>> to submit my own patch. In the meantime, it seems appropriate to file
>> a bug so that others can have the opportunity to solve the problem if
>> they're interested.
>
> If you haven't started working on it and if no one else has picked it up
> then I would like to try it out and submit a patch.

Hi Mehul,

I have not started work, Gentoo/Tails have been keeping me busy. I
would greatly appreciate you picking this up. I'm happy to test any
patches if you'd like.

-- 
-Austin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clone: don't use an integer as a NULL pointer

[Ramsay Jones]

On 26/05/16 00:30, Stefan Beller wrote:
> On Wed, May 25, 2016 at 4:12 PM, Ramsay Jones
>  wrote:
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>
>> Hi Stefan,
>>
>> If you need to re-roll your 'sb/submodule-default-paths' branch, could
>> you please squash this into the relevant patch. (commit 8efbe28b,
>> "clone: add --init-submodule= switch", 23-05-2016).
>>
>> Thanks!
>>
>> ATB,
>> Ramsay Jones
> 
> Thanks for pointing out!
> 
> I am sorry for having you write me these emails;

It's no problem ...

> Out of curiosity, how much of this is manual work and how
> much did you automate of this?

... because this particular problem is caught by 'make sparse'.

[OK, I have to write the patch, test, etc. manually, but that's
not a great burden.]

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Samuel GROOT]

On 05/25/2016 08:31 PM, Matthieu Moy wrote:

So, a possible UI would be:

  git send-email --in-reply-to= => just set In-Reply-To: field.

  git send-email --in-reply-to= => set In-Reply-To, To and Cc.

  git send-email --in-reply-to= --cite => in addition, add the
body of the message quoted with '> '.

  git send-email --in-reply-to= --fetch => fetch and do like 
using the default configuration for fetch.


We designed a similar UI, except for the --fetch option:

We wanted to try to fetch the email from a distant server (e.g. gmane)
if that server address was set in the config file, and populate the
To:/Cc: fields.

If the file cannot be downloaded, or server address not set, just fill 
the Reply-to header.


Either way, display what was done with the message-id given (unless
--quiet is set, of course).


This leaves room for:

  git send-email --in-reply-to= --fetch=gmane => fetch from gmane
(details on how to fetch would be in the config file)

This UI wouldn't allow using a file to get only the message-id. But I'm
not sure this is an interesting use-case.


IMHO when you reply to a thread with a patch, it seems
counter-productive to reply without notifying (putting in To:/Cc:) the
original author and people involved in the thread.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

[Stefan Beller]

Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a boolean field 'submodule..shallow' in .gitmodules, which can
be used to recommend whether upstream considers the history important.

This field is honored in the initial clone by default, it can be
ignored by giving the `--no-recommend-shallow` option.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 10 ++--
 builtin/submodule--helper.c |  7 +-
 git-submodule.sh|  9 ++-
 t/t5614-clone-submodules.sh | 52 +
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..c928c0d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,9 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--jobs ] [--] [...]
+ [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
+ [--reference ] [--depth ] [--recursive]
+ [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -384,6 +385,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+--[no-]recommended-depth::
+   This option is only valid for the update command.
+   The initial clone of a submodule will use the recommended
+   `submodule..depth` as provided by the .gitmodules file.
+
 -j ::
 --jobs ::
This option is only valid for the update command.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8da263f..ca33408 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -581,6 +581,7 @@ struct submodule_update_clone {
 
/* configuration parameters which are passed on to the children */
int quiet;
+   int recommend_shallow;
const char *reference;
const char *depth;
const char *recursive_prefix;
@@ -593,7 +594,7 @@ struct submodule_update_clone {
unsigned quickstop : 1;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0}
 
 
@@ -698,6 +699,8 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, "--quiet");
if (suc->prefix)
argv_array_pushl(>args, "--prefix", suc->prefix, NULL);
+   if (suc->recommend_shallow && sub->recommend_shallow == 1)
+   argv_array_push(>args, "--depth=1");
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
@@ -780,6 +783,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
  "specified number of revisions")),
OPT_INTEGER('j', "jobs", _jobs,
N_("parallel jobs")),
+   OPT_BOOL(0, "recommend-shallow", _shallow,
+   N_("whether the initial clone should follow the 
shallow recommendation")),
OPT__QUIET(, N_("don't print cloning progress")),
OPT_END()
};
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..42e0e9f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] 
[--reference ] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -559,6 +559,12 @@ cmd_update()
--checkout)
update="checkout"
;;
+   --recommend-shallow)
+   recommend_shallow="--recommend-shallow"
+   

[PATCH 1/2] submodule-config: keep shallow recommendation around

[Stefan Beller]

The shallow field will be used in a later patch by `submodule update`.
To differentiate between the actual depth (which may be different),
we name it `recommend_shallow` as the field in the .gitmodules file
is only a recommendation by the project.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 10 ++
 submodule-config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index debab29..e11b35d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -199,6 +199,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
+   submodule->recommend_shallow = -1;
 
hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +354,15 @@ static int parse_config(const char *var, const char 
*value, void *data)
else if (parse_submodule_update_strategy(value,
 >update_strategy) < 0)
die(_("invalid value for %s"), var);
+   } else if (!strcmp(item.buf, "shallow")) {
+   if (!me->overwrite &&
+submodule->recommend_shallow != -1)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"shallow");
+   else {
+   submodule->recommend_shallow =
+   git_config_bool(var, value);
+   }
}
 
strbuf_release();
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..b1fdcc0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -18,6 +18,7 @@ struct submodule {
struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
+   int recommend_shallow;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.9.0.rc0.2.g145fc64

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Stefan Beller]

v2:
* Instead of storing the depth, we keep a boolean `shallow` field in the
  `.gitmodules` file.
* slightly renamed options (--recommend-shallow instead of --recommended-depth)
* one more test
* I dropped [PATCH 1/3] submodule update: make use of the existing 
fetch_in_submodule function
  as Junio picked it up separately as sb/submodule-misc-cleanups

v1:
Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule..depth' in .gitmodules, which can be used
to indicate the recommended depth.

Thanks,
Stefan

Stefan Beller (2):
  submodule-config: keep shallow recommendation around
  submodule update: learn `--[no-]recommend-shallow` option

 Documentation/git-submodule.txt | 10 ++--
 builtin/submodule--helper.c |  7 +-
 git-submodule.sh|  9 ++-
 submodule-config.c  | 10 
 submodule-config.h  |  1 +
 t/t5614-clone-submodules.sh | 52 +
 6 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.9.0.rc0.2.g145fc64

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] connect: document why we sometimes call get_port after get_host_and_port

[Mike Hommey]

On Tue, May 24, 2016 at 06:44:26AM +0200, Torsten Bögershausen wrote:
> On 05/23/2016 11:30 PM, Junio C Hamano wrote:
> > Torsten Bögershausen  writes:
> > 
> > > > > > get_host_and_port(_host, );
> > > > > >+/* get_host_and_port may not return a 
> > > > > > port
> > > > > > even when
> > > > > > +* there is one: In the [host:port]:path case,
> > > > > > +* get_host_and_port is called with 
> > > > > > "[host:port]" and
> > > > > > +* returns "host:port" and NULL.
> > > > > > +* In that specific case, we still need to 
> > > > > > split the
> > > > > > +* port. */
> > > > > Is it worth to mention that this case is "still supported legacy" ?
> > > > If it's worth mentioning anywhere, it seems to me it would start with
> > > > urls.txt?
> > > > 
> > > > Mike
> > > > 
> > > I don't know.
> > > urls.txt is for Git users, and connect.c is for Git developers.
> > > urls.txt does not mention that Git follows any RFC when parsing the
> > > URLS', it doesn't claim to be 100% compliant.
> > > Even if it makes sense to do so, as many user simply expect Git to accept
> > > RFC compliant URL's, and it makes the development easier, if there is
> > > an already
> > > written specification, that describes all the details.
> > > The parser is not 100% RFC compliant, one example:
> > > - old-style usgage like "git clone [host:222]:~/path/to/repo are supported
> > Is it an option to fix get_host_and_port() so that it returns what
> > the caller expects even when it is given "[host:port]"?  When the
> > caller passes other forms like "host:port", it expects to get "host"
> > and "port" parsed out into two variables.  Why can't the caller
> > expect to see the same happen when feeding "[host:port]" to the
> > function?
> > 
> This is somewhat out of my head:
> git clone   git://[example.com:123]:/test#illegal, malformated URL
> git clone   [example.com:123]:/test   #scp-like URL, legacy
> git clone   ssh://[example.com:123]:/test   #illegal, but supported as
> legacy, because
> git clone  ssh://[user@::1]/test   # was the only way to
> specify a user name at a literal IPv6 address
> 
> May be we should have some  more test cases for malformated git:// URLs?

None of these malformed urls are rejected with or without my series
applied:

Without:
$ git fetch-pack --diag-url git://[example.com:123]:/test 
Diag: url=git://[example.com:123]:/test
Diag: protocol=git
Diag: hostandport=[example.com:123]:
Diag: path=/test
$ git fetch-pack --diag-url
ssh://[example.com:123]:/test
Diag: url=ssh://[example.com:123]:/test
Diag: protocol=ssh
Diag: userandhost=example.com
Diag: port=123
Diag: path=/test

With:
$ git fetch-pack --diag-url git://[example.com:123]:/test 
Diag: url=git://[example.com:123]:/test
Diag: protocol=git
Diag: user=NULL
Diag: host=example.com
Diag: port=123
Diag: path=/test
$ git fetch-pack --diag-url ssh://[example.com:123]:/test
Diag: url=ssh://[example.com:123]:/test
Diag: protocol=ssh
Diag: user=NULL
Diag: host=example.com
Diag: port=123
Diag: path=/test

Note in the first case, hostandport is "[example.com:123]:", and that
is treated as host=example.com:123 and port=NULL further down, so my
series is changing something here, but arguably, it makes it less worse.
(note that both with and without my series,
"git://[example.com:123]:42/path" is treated the same, so only a corner
case changed)

Can we go forward with the current series (modulo the comment style
thing Eric noted, and maybe adding a note about the parser handling urls
as per urls.txt), and not bloat scope it? If anything, the state of the
code after the series should make further parser changes easier.

Cheers,

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clone: don't use an integer as a NULL pointer

[Stefan Beller]

On Wed, May 25, 2016 at 4:12 PM, Ramsay Jones
 wrote:
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Stefan,
>
> If you need to re-roll your 'sb/submodule-default-paths' branch, could
> you please squash this into the relevant patch. (commit 8efbe28b,
> "clone: add --init-submodule= switch", 23-05-2016).
>
> Thanks!
>
> ATB,
> Ramsay Jones

Thanks for pointing out!

I am sorry for having you write me these emails;
Out of curiosity, how much of this is manual work and how
much did you automate of this?

>
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 22b6eac..a056f72 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -925,7 +925,7 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
> struct strbuf sb = STRBUF_INIT;
> for_each_string_list_item(item, _submodules) {
> strbuf_addf(, "submodule.defaultUpdatePath=%s", 
> item->string);
> -   string_list_append(_config, strbuf_detach(, 
> 0));
> +   string_list_append(_config, strbuf_detach(, 
> NULL));
> }
> }
>
> --
> 2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7610-mergetool.sh test failure

[Jeff King]

On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:

> On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
>  wrote:
> > t7610-mergetool.sh fails on systems without mktemp.
> >
> > mktemp is used in git-mergetool.sh and throws an error when it's not 
> > available.
> > error: mktemp is needed when 'mergetool.writeToTemp' is true
> >
> > I see 2 options:
> > 1. code around it, write an own mktemp, maybe use the test-mktemp as a 
> > basis.
> > 2. disable the test when mktemp is not available
> 
> 3. find and install mktemp?

I'd agree more with (3) if it was some critical part of git-mergetool.
But it seems to be a completely optional feature that we use in only one
place, and git-mergetool even detects this case and complains.

So another alternative would be for the test to detect either that
mergetool worked, or that we got the "mktemp is needed" error.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] xemit.c: fix a [-Wchar-subscripts] compiler warning

[Ramsay Jones]

While compiling on cygwin (x86_64), gcc complains thus:

  CC xdiff/xemit.o
  xdiff/xemit.c: In function ‘is_empty_rec’:
  xdiff/xemit.c:163:2: warning: array subscript has type ‘char’ 
[-Wchar-subscripts]
while (len > 0 && isspace(*rec)) {
^

A comment in the  header reads, in part, like so:

   These macros are intentionally written in a manner that will trigger
   a gcc -Wall warning if the user mistakenly passes a 'char' instead
   of an int containing an 'unsigned char'.

In order to suppress the warning, cast the 'char *' pointer 'rec' to an
'unsigned char *' pointer, prior to passing the dereferenced pointer to
the isspace() macro.

Signed-off-by: Ramsay Jones 
---

Hi René,

If you need to re-roll your 'rs/xdiff-hunk-with-func-line' branch, could
you please squash this (or something like it) into the relevant patch.

[A comment in the linux  header, says that the ctype-info tables ...
   point into arrays of 384, so they can be indexed by any `unsigned
   char' value [0,255]; by EOF (-1); or by any `signed char' value
   [-128,-1).
So, this is not a problem on linux.]

Thanks!

ATB,
Ramsay Jones

 xdiff/xemit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d0c0738..ae9adac 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -160,7 +160,7 @@ static int is_empty_rec(xdfile_t *xdf, xdemitconf_t const 
*xecfg, long ri)
const char *rec;
long len = xdl_get_rec(xdf, ri, );
 
-   while (len > 0 && isspace(*rec)) {
+   while (len > 0 && isspace(*((unsigned char *)rec))) {
rec++;
len--;
}
-- 
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] clone: don't use an integer as a NULL pointer

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Stefan,

If you need to re-roll your 'sb/submodule-default-paths' branch, could
you please squash this into the relevant patch. (commit 8efbe28b,
"clone: add --init-submodule= switch", 23-05-2016).

Thanks!

ATB,
Ramsay Jones

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 22b6eac..a056f72 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -925,7 +925,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
for_each_string_list_item(item, _submodules) {
strbuf_addf(, "submodule.defaultUpdatePath=%s", 
item->string);
-   string_list_append(_config, strbuf_detach(, 
0));
+   string_list_append(_config, strbuf_detach(, 
NULL));
}
}
 
-- 
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

[Eric Wong]

Junio C Hamano  wrote:
> This makes sense as a follow-up to e47a8583 (enable SO_KEEPALIVE for
> connected TCP sockets, 2011-12-06), I think.

Yes, a15d069a19867 for http, too; hat trick :>

Anyways, it might've helped Savannah when they had networking
problems:

  http://mid.gmane.org/20160524214102858920...@bob.proulx.com

They might be running an old version that didn't send keepalive
heartbeats during packing, too.  But SO_KEEPALIVE will still
help during init when --init-timeout is not set.

Perhaps it also makes sense to squash the following xinetd
setting into giteveryday.txt, too; since some users could be
running out-of-date git but reading new documentation on
the web:

--- a/Documentation/giteveryday.txt
+++ b/Documentation/giteveryday.txt
@@ -390,6 +390,7 @@ service git
server  = /usr/bin/git-daemon
server_args = --inetd --export-all --base-path=/pub/scm
log_on_failure  += USERID
+   flags   = KEEPALIVE
 }
 
 +
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] fast-import: invalidate pack_id references after loosening

[Jeff King]

On Wed, May 25, 2016 at 10:54:02PM +, Eric Wong wrote:

> When loosening a pack, the current pack_id gets reused when
> checkpointing and the import does not terminate.  This causes
> problems after checkpointing as the object table, branch, and
> tag lists still contains pre-checkpoint references to the
> recycled pack_id.
> 
> Merely clearing the object_table as suggested by Jeff King in
> http://mid.gmane.org/20160517121330.ga7...@sigill.intra.peff.net
> is insufficient as the marks set still contains references
> to object entries.
> 
> Wrong pack_id references branch and tags lists do not cause
> errors, but can lead to misleading crash reports and core dumps,
> so they are also invalidated.

Nicely explained.

> +static void invalidate_pack_id(unsigned int id)
> +{
> + unsigned int h;
> + unsigned long lu;
> + struct tag *t;
> +
> + for (h = 0; h < ARRAY_SIZE(object_table); h++) {
> + struct object_entry *e;
> +
> + for (e = object_table[h]; e; e = e->next)
> + if (e->pack_id == id)
> + e->pack_id = MAX_PACK_ID;
> + }
> +
> + for (lu = 0; lu < branch_table_sz; lu++) {
> + struct branch *b;
> +
> + for (b = branch_table[lu]; b; b = b->table_next_branch)
> + if (b->pack_id == id)
> + b->pack_id = MAX_PACK_ID;
> + }
> +
> + for (t = first_tag; t; t = t->next_tag)
> + if (t->pack_id == id)
> + t->pack_id = MAX_PACK_ID;
> +}

This looks pretty straightforward. I do notice that we never shrink the
number of items in the object table when checkpointing, and so our
linear walk will grow ever larger. So if one were to checkpoint every
k-th object, it makes the whole operation quadratic in the number of
objects (actually, O(n^2/k) but k is a constant).

That's probably OK in practice, as I think the actual pack-write already
does a linear walk of the object table to generate the pack index. So
presumably nobody checkpoints often enough for it to be a problem. And
the solution would be to keep a separate list of pointers to objects for
the current pack-id, which would trivially fix both this case and the
one in create_index().  So we can punt on it until somebody actually
runs into it, I think.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Junio C Hamano]

Jeff King  writes:

> I think messages to stderr are generally fair game for changing, even in
> plumbing. In many cases they are also translated (and I would argue that
> these messages probably should be translated, too).

I think I agree.  My first reaction to this thread indeed was "Why
isn't this marked for translation?"; as to the change proposed by
the patch itself, my reaction actually was "Meh" ;-)

> That being said, CodingGuidelines says:
>
>- Do not end error messages with a full stop.

Thanks for pointing it out---I forgot about that one.

I do not think there was a concrete reason why they should not end
with a full stop, other than "be consistent with existing ones that
do not end with a full stop", though.

> This isn't an error message exactly, but I think it's in the same vein.
> I will note that we have not historically been consistent here, though
> (as evidenced by the noted message in git-merge).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] t9801 and t9803 broken on next

[Jeff King]

On Wed, May 25, 2016 at 10:49:07PM +, Eric Wong wrote:

> > Thanks for the analysis. I think this is definitely the problem.  After
> > fast-import finalizes a packfile (either at the end of the program or
> > due to a checkpoint), it never discards its internal mapping of objects
> > to pack locations. It _has_ to keep such a mapping before the pack is
> > finalized, because git's regular object database doesn't know about it.
> > But afterwards, it should be able to rely on the object database.
> 
> Almost; but relying on marks is a problem since that set can contain
> mark => object_entry mappings which the normal object database won't
> know about.

Ah, thanks for finding that. I had a feeling there might be other users
of the object_entry structs, but I didn't dig.

Given that and your other responses here, I agree that just invalidating
the pack_id is probably the most sensible route.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] fast-import: invalidate pack_id references after loosening

[Eric Wong]

When loosening a pack, the current pack_id gets reused when
checkpointing and the import does not terminate.  This causes
problems after checkpointing as the object table, branch, and
tag lists still contains pre-checkpoint references to the
recycled pack_id.

Merely clearing the object_table as suggested by Jeff King in
http://mid.gmane.org/20160517121330.ga7...@sigill.intra.peff.net
is insufficient as the marks set still contains references
to object entries.

Wrong pack_id references branch and tags lists do not cause
errors, but can lead to misleading crash reports and core dumps,
so they are also invalidated.

Signed-off-by: Eric Wong 
---
 I started writing a standalone test case; but testing with
 original failing cases would be greatly appreciated.

 Still learning my way around the fast-import code...
 Thanks.

 fast-import.c   | 31 +++-
 t/t9302-fast-import-unpack-limit.sh | 57 +
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 0e8bc6a..b9db4b6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -597,6 +597,33 @@ static struct object_entry *insert_object(unsigned char 
*sha1)
return e;
 }
 
+static void invalidate_pack_id(unsigned int id)
+{
+   unsigned int h;
+   unsigned long lu;
+   struct tag *t;
+
+   for (h = 0; h < ARRAY_SIZE(object_table); h++) {
+   struct object_entry *e;
+
+   for (e = object_table[h]; e; e = e->next)
+   if (e->pack_id == id)
+   e->pack_id = MAX_PACK_ID;
+   }
+
+   for (lu = 0; lu < branch_table_sz; lu++) {
+   struct branch *b;
+
+   for (b = branch_table[lu]; b; b = b->table_next_branch)
+   if (b->pack_id == id)
+   b->pack_id = MAX_PACK_ID;
+   }
+
+   for (t = first_tag; t; t = t->next_tag)
+   if (t->pack_id == id)
+   t->pack_id = MAX_PACK_ID;
+}
+
 static unsigned int hc_str(const char *s, size_t len)
 {
unsigned int r = 0;
@@ -993,8 +1020,10 @@ static void end_packfile(void)
cur_pack_sha1, pack_size);
 
if (object_count <= unpack_limit) {
-   if (!loosen_small_pack(pack_data))
+   if (!loosen_small_pack(pack_data)) {
+   invalidate_pack_id(pack_id);
goto discard_pack;
+   }
}
 
close(pack_data->pack_fd);
diff --git a/t/t9302-fast-import-unpack-limit.sh 
b/t/t9302-fast-import-unpack-limit.sh
index 0f686d2..a04de14 100755
--- a/t/t9302-fast-import-unpack-limit.sh
+++ b/t/t9302-fast-import-unpack-limit.sh
@@ -45,4 +45,61 @@ test_expect_success 'bigger packs are preserved' '
test $(find .git/objects/pack -type f | wc -l) -eq 2
 '
 
+test_expect_success 'lookups after checkpoint works' '
+   hello_id=$(echo hello | git hash-object --stdin -t blob) &&
+   id="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" &&
+   before=$(git rev-parse refs/heads/master^0) &&
+   (
+   cat <<-INPUT_END &&
+   blob
+   mark :1
+   data 6
+   hello
+
+   commit refs/heads/master
+   mark :2
+   committer $id
+   data <&2 "checkpoint did not update branch"
+   exit 1
+   else
+   n=$(($n + 1))
+   fi &&
+   sleep 1 &&
+   from=$(git rev-parse refs/heads/master^0)
+   done &&
+   cat <<-INPUT_END &&
+   commit refs/heads/master
+   committer $id
+   data 

Re: [Opinion gathering] Git remote whitelist/blacklist

[Jeff King]

On Tue, May 24, 2016 at 09:07:53AM -0700, Junio C Hamano wrote:

> On Tue, May 24, 2016 at 5:55 AM, Matthieu Moy
>  wrote:
> > So, when trying a forbidden push, Git would deny it and the only way to
> > force the push would be to remove the blacklist from the config, right?
> >
> > Probably the sanest way to go. I thought about adding a "git push
> > --force-even-if-in-blacklist" or so, but I don't think the feature
> > deserves one specific option (hence add some noise in `git push -h`).
> 
> Yeah, I agree --even-if-in-blacklist is a road to madness, but I wonder
> how this is different from setting pushURL to /dev/null or something
> illegal and replace that phony configuration value when you really need
> to push?

That was my thought on reading this, too. In that scheme, you could do:

  git -c remote.foo.pushurl=example.com:repo.git push ...

to override it.  It would be nice if you could do:

  git -c remote.foo.pushurl= push ...

to "unset" the push-url list and default to the regular fetch url, but
this is one of those multi-value config options that would have to learn
that explicitly.

I suppose one can do:

  git -c remote.foo.pushurl=$(git config remote.foo.url)

but that is getting a bit long.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] t9801 and t9803 broken on next

[Eric Wong]

Jeff King  wrote:
> On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote:
> 
> > I think that is pretty much the problem. Here is what is happening:
> > 
> > 1.  git-p4 imports all changelists for the "main" branch
> > 
> > 2.  git-p4 starts to import a second branch and creates a fast-import
> > "progress checkpoint". This triggers:
> > 
> > --> fast-import.c: cycle_packfile
> > --> fast-import.c: end_packfile
> > --> fast-import.c: loosen_small_pack
> > 
> > At this point we have no packfile anymore.
> > 
> > 3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
> > to fast-import in order to create a new branch. This triggers:
> > 
> > --> fast-import.c: parse_new_commit
> > --> fast-import.c: load_branch
> > --> fast-import.c: load_tree
> > 
> > "load_tree" calls "find_object" and the result has a "pack_id" of 0.
> > I think this indicates that the object is in the packfile. Shouldn't
> > the "pack_id" be "MAX_PACK_ID" instead?

Yes; I think that is correct.  Alternative patch to Jeff's
coming in reply to this message.

> > myoe = find_object(sha1);
> > if (myoe && myoe->pack_id != MAX_PACK_ID) {
> 
> Thanks for the analysis. I think this is definitely the problem.  After
> fast-import finalizes a packfile (either at the end of the program or
> due to a checkpoint), it never discards its internal mapping of objects
> to pack locations. It _has_ to keep such a mapping before the pack is
> finalized, because git's regular object database doesn't know about it.
> But afterwards, it should be able to rely on the object database.

Almost; but relying on marks is a problem since that set can contain
mark => object_entry mappings which the normal object database won't
know about.

> The patch below probably makes your case work, but there are a lot of
> open questions:
> 
>   1. Should we always discard the mapping, even when not loosening
>  objects? I can't think of a real downside to always using git's
>  object lookups.

I'm not sure.  It's safe to clear the top-level table, but it
might speedup some lookups for just oe->type if we keep it
around.

I decided to keep it, anyways, because the mark set references them.

>   2. Can we free memory associated with object_entry structs at this
>  point? They won't be accessible via the hash, but might other bits
>  of the code have kept pointers to them?

Yes, invalid entries are also held in "struct mark_set marks";
this is a major problem with merely clearing the top-level
object table.

>  I suspect it may screw up the statistics that fast-import prints at
>  the end, but that's a minor point.

I still need to check, on that; but yeah, minor.

>   3. I notice that a few other structs (branch and tag) hold onto the
>  pack_id, which will now point to a pack we can't access. Does this
>  matter? I don't think so, because checkpoint() seems to dump the
>  branches and tags.

I don't think it matters unless a crash log or core dump is
created; then it becomes confusing to the person tracking down a
problem, so I've invalidated pack_id.  This doesn't affect
dump_branches or dump_tags from what I can tell.

>   4. In general, should we be loosening objects at checkpoints at all?

I think so.  It should be useful to checkpoint to make objects
available to other read-only processes while leaving a
fast-import running indefinitely.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Jeff King]

On Tue, May 24, 2016 at 02:21:00PM -0700, Stefan Beller wrote:

> On Tue, May 24, 2016 at 1:51 PM, Yong Bakos  wrote:
> > Appending a period to "Everything up-to-date" makes the output message
> > consistent with similar output in builtin/merge.c.
> >
> > Signed-off-by: Yong Bakos 
> > ---
> >  builtin/send-pack.c | 2 +-
> >  transport.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 1ff5a67..67d9304 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> 
> While consistency is a good idea in general, I wonder how that applies here.
> git-send-pack is a low level (i.e. plumbing) command.
> 
>The interface (input, output, set of options and the semantics) to
>these low-level commands are meant to be a lot more stable than
>Porcelain level commands, because these commands are primarily for
>scripted use. The interface to Porcelain commands on the other hand are
>subject to change in order to improve the end user experience.
> 
> So if another porcelain exists and compares the output string
> exactly, this would be a regression for them. That is why I'd refrain
> from updating these strings

I think messages to stderr are generally fair game for changing, even in
plumbing. In many cases they are also translated (and I would argue that
these messages probably should be translated, too).

That being said, CodingGuidelines says:

   - Do not end error messages with a full stop.

This isn't an error message exactly, but I think it's in the same vein.
I will note that we have not historically been consistent here, though
(as evidenced by the noted message in git-merge).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] submodule update: make use of the existing fetch_in_submodule function

[Stefan Beller]

On Wed, May 25, 2016 at 3:41 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  git-submodule.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 5a4dec0..7698102 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -640,7 +640,7 @@ cmd_update()
>>   if test -z "$nofetch"
>>   then
>>   # Fetch remote before determining tracking 
>> $sha1
>> - (sanitize_submodule_env; cd "$sm_path" && 
>> git-fetch) ||
>> + fetch_in_submodule "$sm_path" ||
>>   die "$(eval_gettext "Unable to fetch in 
>> submodule path '\$sm_path'")"
>>   fi
>>   remote_name=$(sanitize_submodule_env; cd "$sm_path" && 
>> get_default_remote)
>
> Makes sense.  The main topic does not depend on this change, I hope,
> as I think it is OK to queue this separately and have it graduate
> before 2.9-rc1.


It doesn't, I should have send this as an independent series/patch.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Submodules: have a depth field in the .gitmodules file

[Stefan Beller]

On Wed, May 25, 2016 at 3:38 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Sometimes the history of a submodule is not considered important by
>> the projects upstream. To make it easier for downstream users, allow
>> a field 'submodule..depth' in .gitmodules, which can be used
>> to indicate the recommended depth.
>
> Hmph.  I can understand and certainly agree with the first sentence,
> but I am not sure if "depth", if it is anything other than "1" or
> "infinity", is a reasonable value.
>
> I'd understand if a project wants to say something like "at this
> moment, history before v2.0 tag does not matter", though.

I fell for the trap, like all depth related problems fall into.
I came up with the easiest solution to be implemented into Git,
not what the user actually wants.

Background for this change is trying to get a similar thing like the
"clone-depth"
field from repo manifests implemented into Git.
And looking at e.g. [1], this is either non existent (infinity) or 1.
So maybe instead of giving a depth recommendation in the .gitmodules,
we only fill in a boolean config "[non]shallow" which defaults to non shallow
in case of not giving the field.

Thanks,
Stefan



[1] https://android.googlesource.com/platform/manifest/+/master/default.xml
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

[Junio C Hamano]

Eric Wong  writes:

> While --init-timeout and --timeout options exist and I've never
> run git-daemon without them, some users may forget to set them
> and encounter hung daemon processes when connections fail.
> Enable socket-level timeouts so the kernel can send keepalive
> probes as necessary to detect failed connections.
>
> Signed-off-by: Eric Wong 
> ---

This makes sense as a follow-up to e47a8583 (enable SO_KEEPALIVE for
connected TCP sockets, 2011-12-06), I think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: enable SO_KEEPALIVE for all sockets

[Jeff King]

On Wed, May 25, 2016 at 03:15:05AM +, Eric Wong wrote:

> While --init-timeout and --timeout options exist and I've never
> run git-daemon without them, some users may forget to set them
> and encounter hung daemon processes when connections fail.
> Enable socket-level timeouts so the kernel can send keepalive
> probes as necessary to detect failed connections.

Makes sense. I wondered if there were any portability issues here, but
it looks like the same code is found on the client side (but we'd still
want it here for cases where the client thinks the connection is dead
but the server does not realize it).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] submodule update: make use of the existing fetch_in_submodule function

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 5a4dec0..7698102 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -640,7 +640,7 @@ cmd_update()
>   if test -z "$nofetch"
>   then
>   # Fetch remote before determining tracking $sha1
> - (sanitize_submodule_env; cd "$sm_path" && 
> git-fetch) ||
> + fetch_in_submodule "$sm_path" ||
>   die "$(eval_gettext "Unable to fetch in 
> submodule path '\$sm_path'")"
>   fi
>   remote_name=$(sanitize_submodule_env; cd "$sm_path" && 
> get_default_remote)

Makes sense.  The main topic does not depend on this change, I hope,
as I think it is OK to queue this separately and have it graduate
before 2.9-rc1.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

[Jeff King]

On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote:

> Check that we are configured to display colors in the given context when
> the user specifies a format string of `%C(auto)`.  This brings that
> behavior in line with the behavior of `%C(auto,)`, which will
> display the given color only when the configuration specifies to do so.
> 
> This allows the user the ability to specify that color should be
> displayed only when the output is a tty, and to use the default color
> for the given context (instead of a hardcoded color value).
> 
> Signed-off-by: Edward Thomson 

I somehow had trouble figuring out the problem from this description and
the patch. It seems to be about much more than just color=auto or a
given context, and more like:

  When %C(auto) is used, we unconditionally turn on color for any
  subsequent placeholders, even if the user said "--no-color", or color
  config is turned off, or it is set to "auto" and we are not going to a
  tty.

It's possible somebody is relying on the ability to unconditionally turn
on color for "auto-colored" placeholders like "%H" or "%d", but I'm
inclined to call this a strict bug-fix, for two reasons:

  1. It says "%C(auto)", not "%C(on)".

  2. This is documented as behaving like "%C(auto,...)", which as you
 note works in a more sane way.

I think it's worth mentioning this explicitly in the commit message. We
could also add "%C(on)", I guess, but it's unclear to me whether anybody
would want it (they would probably just use "--color" in that case,
unless they really want unconditional coloring for just _some_
elements).

I'm adding Duy to the cc as the original author of %C(auto), in case
there is something subtle I'm missing.

> ---
>  pretty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like we didn't have any tests at all for %C(auto). And the tests
for %C(auto,...) were labeled as %C(auto), making it all the more
confusing. Perhaps it is worth squashing this in:

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index b77d4c9..a1dcdb8 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -184,38 +184,38 @@ commit $head1
 foo
 EOF
 
-test_expect_success '%C(auto) does not enable color by default' '
+test_expect_success '%C(auto,...) does not enable color by default' '
git log --format=$AUTO_COLOR -1 >actual &&
has_no_color actual
 '
 
-test_expect_success '%C(auto) enables colors for color.diff' '
+test_expect_success '%C(auto,...) enables colors for color.diff' '
git -c color.diff=always log --format=$AUTO_COLOR -1 >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) enables colors for color.ui' '
+test_expect_success '%C(auto,...) enables colors for color.ui' '
git -c color.ui=always log --format=$AUTO_COLOR -1 >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --color' '
+test_expect_success '%C(auto,...) respects --color' '
git log --format=$AUTO_COLOR -1 --color >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --no-color' '
+test_expect_success '%C(auto,...) respects --no-color' '
git -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual &&
has_no_color actual
 '
 
-test_expect_success TTY '%C(auto) respects --color=auto (stdout is tty)' '
+test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' '
test_terminal env TERM=vt100 \
git log --format=$AUTO_COLOR -1 --color=auto >actual &&
has_color actual
 '
 
-test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '
+test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
(
TERM=vt100 && export TERM &&
git log --format=$AUTO_COLOR -1 --color=auto >actual &&
@@ -223,6 +223,18 @@ test_expect_success '%C(auto) respects --color=auto 
(stdout not tty)' '
)
 '
 
+test_expect_success '%C(auto) respects --color' '
+   git log --color --format="%C(auto)%H" -1 >actual &&
+   printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%C(auto) respects --no-color' '
+   git log --no-color --format="%C(auto)%H" -1 >actual &&
+   git rev-parse HEAD >expect &&
+   test_cmp expect actual
+'
+
 iconv -f utf-8 -t $test_encoding > commit-msg 

Re: [PATCH 0/3] Submodules: have a depth field in the .gitmodules file

[Junio C Hamano]

Stefan Beller  writes:

> Sometimes the history of a submodule is not considered important by
> the projects upstream. To make it easier for downstream users, allow
> a field 'submodule..depth' in .gitmodules, which can be used
> to indicate the recommended depth.

Hmph.  I can understand and certainly agree with the first sentence,
but I am not sure if "depth", if it is anything other than "1" or
"infinity", is a reasonable value.

I'd understand if a project wants to say something like "at this
moment, history before v2.0 tag does not matter", though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gitweb: fix link to parent diff with pathinfo

[Richard Braun]

On Wed, May 25, 2016 at 10:33:32PM +0200, Jakub Narębski wrote:
> Richard, thanks for finding a problematic thing, but you
> need to search more for a true fix.

Noted, I'll get back to you soon (hopefully not too late).

-- 
Richard Braun
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why is my git-http-backend solution using WebDAV on push?

[Junio C Hamano]

Luke Madhanga  writes:

>> A manual CLI call to git-http-backend doesn't include
>> 'application/x-git-receive-pack-advertisement'
>>
>> REQUEST_METHOD=GET GIT_PROJECT_ROOT=/path/to/core/
>> PATH_INFO=/repo.git/info/refs /usr/lib/git-core/git-http-backend

The request client makes to probe is (taking it from Peff's message
that is quoting from your trace):

> > GET /p/git-backend/run/1/info/refs?service=git-receive-pack HTTP/1.1

Your manual CLI call seems not to have "?service=git-receive-pack"
anywhere.  Where did it go?  QUERY_STRING, perhaps?

Here is what I am observing:

$ GIT_HTTP_EXPORT_ALL=Yes \
> REQUEST_METHOD=GET \
> GIT_PROJECT_ROOT=$(pwd)/.git \
> PATH_INFO='/info/refs' \
> QUERY_STRING=service=git-receive-pack \
> git -c http.receivepack=yes http-backend 2>&1 | sed -e '/^.$/q'
Expires: Fri, 01 Jan 1980 00:00:00 GMT
Pragma: no-cache
Cache-Control: no-cache, max-age=0, must-revalidate
Content-Type: application/x-git-receive-pack-advertisement


>>
>> The above command outputs
>>
>> Expires: Fri, 01 Jan 1980 00:00:00 GMT
>> Pragma: no-cache
>> Cache-Control: no-cache, max-age=0, must-revalidate
>> Content-Length: 118
>> Content-Type: text/plain
>>
>> f4648182f5f8eee082c37a83a0072cfc4210e5c5 refs/heads/master
>> 8c4efcd77809bc9b94a59cf94653add8007c6b7d refs/heads/zztest
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] submodule update: learn `--recommended-depth` option

[Stefan Beller]

Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule..depth' in .gitmodules, which can be used
to indicate the recommended depth.

This field is honored in the initial clone by default, it can be
ignored by giving the `--no-recommended-depth` option.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 10 --
 builtin/submodule--helper.c |  8 +++-
 git-submodule.sh|  9 -
 t/t5614-clone-submodules.sh | 34 ++
 4 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..c928c0d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,9 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--jobs ] [--] [...]
+ [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
+ [--reference ] [--depth ] [--recursive]
+ [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -384,6 +385,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+--[no-]recommended-depth::
+   This option is only valid for the update command.
+   The initial clone of a submodule will use the recommended
+   `submodule..depth` as provided by the .gitmodules file.
+
 -j ::
 --jobs ::
This option is only valid for the update command.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8da263f..70bf2f2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -581,6 +581,7 @@ struct submodule_update_clone {
 
/* configuration parameters which are passed on to the children */
int quiet;
+   int use_recommended_depth;
const char *reference;
const char *depth;
const char *recursive_prefix;
@@ -593,7 +594,7 @@ struct submodule_update_clone {
unsigned quickstop : 1;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0}
 
 
@@ -698,6 +699,9 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, "--quiet");
if (suc->prefix)
argv_array_pushl(>args, "--prefix", suc->prefix, NULL);
+   if (suc->use_recommended_depth && sub->recommended_depth > 0)
+   argv_array_pushf(>args, "--depth=%d",
+   sub->recommended_depth);
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
@@ -780,6 +784,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
  "specified number of revisions")),
OPT_INTEGER('j', "jobs", _jobs,
N_("parallel jobs")),
+   OPT_BOOL(0, "recommended-depth", _recommended_depth,
+   N_("whether the initial clone should follow the 
depth recommendation")),
OPT__QUIET(, N_("don't print cloning progress")),
OPT_END()
};
diff --git a/git-submodule.sh b/git-submodule.sh
index 7698102..794d98a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--[no-]recommended-depth] 
[--reference ] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -559,6 +559,12 @@ cmd_update()
--checkout)
update="checkout"
;;
+   --recommended-depth)
+   

[PATCH 1/3] submodule update: make use of the existing fetch_in_submodule function

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..7698102 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -640,7 +640,7 @@ cmd_update()
if test -z "$nofetch"
then
# Fetch remote before determining tracking $sha1
-   (sanitize_submodule_env; cd "$sm_path" && 
git-fetch) ||
+   fetch_in_submodule "$sm_path" ||
die "$(eval_gettext "Unable to fetch in 
submodule path '\$sm_path'")"
fi
remote_name=$(sanitize_submodule_env; cd "$sm_path" && 
get_default_remote)
-- 
2.9.0.rc0.3.g892bdd0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] submodule-config: keep `depth` around

[Stefan Beller]

The depth field will be used in a later patch by `submodule update`.
To differentiate between the actual depth (which may be different),
we name it recommended depth as the field in the .gitmodules file
is only a recommendation by the project.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 16 
 submodule-config.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index debab29..e65a171 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -199,6 +199,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
+   submodule->recommended_depth = -1;
 
hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +354,21 @@ static int parse_config(const char *var, const char 
*value, void *data)
else if (parse_submodule_update_strategy(value,
 >update_strategy) < 0)
die(_("invalid value for %s"), var);
+   } else if (!strcmp(item.buf, "depth")) {
+   if (!value)
+   ret = config_error_nonbool(var);
+   else if (!me->overwrite &&
+submodule->recommended_depth != -1)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"depth");
+   else {
+   int d = strtol(value, NULL, 0);
+   if (d < 0)
+   warning("Invalid parameter '%s' for config 
option "
+   "'submodule.%s.depth'", value, var);
+   else
+   submodule->recommended_depth = d;
+   }
}
 
strbuf_release();
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..5635b6c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -15,6 +15,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+   int recommended_depth;
struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
-- 
2.9.0.rc0.3.g892bdd0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] Submodules: have a depth field in the .gitmodules file

[Stefan Beller]

Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule..depth' in .gitmodules, which can be used
to indicate the recommended depth.

Stefan Beller (3):
  submodule update: make use of the existing fetch_in_submodule function
  submodule-config: keep `depth` around
  submodule update: learn `--recommended-depth` option

 Documentation/git-submodule.txt | 10 --
 builtin/submodule--helper.c |  8 +++-
 git-submodule.sh| 11 +--
 submodule-config.c  | 16 
 submodule-config.h  |  1 +
 t/t5614-clone-submodules.sh | 34 ++
 6 files changed, 75 insertions(+), 5 deletions(-)

-- 
2.9.0.rc0.3.g892bdd0.dirty

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why is my git-http-backend solution using WebDAV on push?

[Luke Madhanga]

H. Interesting.

When you look at the PHP code, you'll see the following

$res = self::proc_open("{$gitcoredir}/git-http-backend", [],
$gitdir, true, [...]);
...
$resbits = explode("\n", $res);
foreach ($resbits as $index => $header) {
if ($header && strpos($header, ':') !== false) {
// Headers
header($header);
unset($resbits[$index]);
} else {
// First blank line is the space between the headers and
the start of the response from Git
break;
}
}
echo ltrim(implode("\n", $resbits));
exit;


Everything being returned is from a direct call to the git-http-backend.

A manual CLI call to git-http-backend doesn't include
'application/x-git-receive-pack-advertisement'

REQUEST_METHOD=GET GIT_PROJECT_ROOT=/path/to/core/
PATH_INFO=/repo.git/info/refs /usr/lib/git-core/git-http-backend

The above command outputs

Expires: Fri, 01 Jan 1980 00:00:00 GMT
Pragma: no-cache
Cache-Control: no-cache, max-age=0, must-revalidate
Content-Length: 118
Content-Type: text/plain

f4648182f5f8eee082c37a83a0072cfc4210e5c5 refs/heads/master
8c4efcd77809bc9b94a59cf94653add8007c6b7d refs/heads/zztest
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] Spaces not allowed in directory names in .git/info/attributes

[Junio C Hamano]

Duy Nguyen  writes:

> Maybe bring back [1] (cquoting paths) and optionally optionally with
> backslash escaping? The conclusion at the end of that thread seems to
> be "ok, we may break rare setups, we just need to be upfront about
> it". Another option is the pathspec way: match quotes literally as
> well.
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/160597/focus=160720

I coaxed the ancient patch that was done in the 1.7.2 timeperiod and
will push out the result near the tip of jc/attr topic that has been
cooking in 'pu'.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why is my git-http-backend solution using WebDAV on push?

[Luke Madhanga]

Thanks for the response btw

On 25 May 2016 at 22:54, Luke Madhanga  wrote:
> H. Interesting.
>
> When you look at the PHP code, you'll see the following
>
> $res = self::proc_open("{$gitcoredir}/git-http-backend", [],
> $gitdir, true, [...]);
> ...
> $resbits = explode("\n", $res);
> foreach ($resbits as $index => $header) {
> if ($header && strpos($header, ':') !== false) {
> // Headers
> header($header);
> unset($resbits[$index]);
> } else {
> // First blank line is the space between the headers and
> the start of the response from Git
> break;
> }
> }
> echo ltrim(implode("\n", $resbits));
> exit;
>
>
> Everything being returned is from a direct call to the git-http-backend.
>
> A manual CLI call to git-http-backend doesn't include
> 'application/x-git-receive-pack-advertisement'
>
> REQUEST_METHOD=GET GIT_PROJECT_ROOT=/path/to/core/
> PATH_INFO=/repo.git/info/refs /usr/lib/git-core/git-http-backend
>
> The above command outputs
>
> Expires: Fri, 01 Jan 1980 00:00:00 GMT
> Pragma: no-cache
> Cache-Control: no-cache, max-age=0, must-revalidate
> Content-Length: 118
> Content-Type: text/plain
>
> f4648182f5f8eee082c37a83a0072cfc4210e5c5 refs/heads/master
> 8c4efcd77809bc9b94a59cf94653add8007c6b7d refs/heads/zztest



-- 
Yours,
Luke Madhanga
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why is my git-http-backend solution using WebDAV on push?

[Jeff King]

On Wed, May 25, 2016 at 09:28:21PM +0100, Luke Madhanga wrote:

> I've implemented a PHP wrapper for git http backend which works well.
> I've done this to give me advanced control of who has access to
> repositories on my server. You can see the implementation on
> http://stackoverflow.com/questions/36998492/channel-git-on-the-server-calls-through-php/37242591#37242591.
> I can pull from the server okay and all works well. However, I cannot
> push. When I read my trace code to see where it fails, I see that the
> last request is a PROPFIND request. The URL for this request does not
> have any of the usual 'info/refs' etc. that one usually gets on git
> calls.

If the git client detects that the server doesn't implement the
smart-http protocol, it will automatically downgrade to the older,
"dumb" protocol which is less efficient.

For fetching/cloning, this might make your test appear to work, even
though it is downgrading behind the scenes. You should be able to check
GIT_CURL_VERBOSE output to tell the difference; the smart protocol will
POST to .../git-upload-pack, whereas the dumb protocol will download the
individual packfiles and objects directly.

For pushing, the dumb protocol uses WebDAV, which is what you're seeing.

So the question is: why doesn't the client think your server is a smart
server?

Just skimming your output, I'd guess:

> > GET /p/git-backend/run/1/info/refs?service=git-receive-pack HTTP/1.1
> Host: xxx
> User-Agent: git/2.7.4
> Accept: */*
> Accept-Encoding: gzip
> Accept-Language: en-GB, en;q=0.9, *;q=0.8
> Pragma: no-cache
> 
> < HTTP/1.1 200 OK
> < Date: Wed, 25 May 2016 19:00:25 GMT
> < Server: Apache/2.4.18 (Ubuntu)
> < Set-Cookie: PHPSESSID=yyy; path=/
> < Expires: Fri, 01 Jan 1980 00:00:00 GMT
> < Cache-Control: no-cache, max-age=0, must-revalidate
> < Pragma: no-cache
> < Vary: Accept-Encoding
> < Content-Encoding: gzip
> < Content-Length: 109
> < Content-Type: text/plain;charset=UTF-8
> <

The content-type here should be:

  application/x-git-receive-pack-advertisement

The body content should also include "# service=git-receive-pack" on the
first line, but if you are successfully calling into git-http-backend, I
think it should take care of that detail.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [WIP PATCH 00/14] Protocol v2 patches

[Jeff King]

On Tue, May 24, 2016 at 06:46:48PM -0400, David Turner wrote:

> I tried to make libcurl do the receive-before-sending thing, but it
> doesn't seem to be designed for it (even if you prime things by sending
> a "hello" from the client first).  My thought was to hook up
> CURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read
> function return CURL_READFUNC_PAUSE and then have the write (=client
> receiving data ) function unpause the reader (= client sending data)
> once it gets the capabilities.  But apparently pausing only works with
> chunked encoding, which seems to cause Apache's mod_cgi to fail.
> 
> Maybe I'm missing something.  Has anyone else ever made something like
> this work?

I don't think it can work in the general case. HTTP is not full-duplex,
and you have to send off the request and wait for the response. Even if
you could convince the client and git-http-backend to do it, you're
going to get foiled by proxies, web server implementations, and other
middle-men.

> Of course, I could always use CURLOPT_CONNECT_ONLY to write my own HTTP
> client, but that seems pretty unreasonable.
> 
> I also looked to see if libcurl had websockets support, since that's
> one kind of bidirectional conversation over HTTP, but it doesn't seem
> to.

I would love to see us move to a true bidirectional HTTP-based protocol.
It would clear up all of the drawbacks that the current HTTP protocol
has, and I think we could generally recommend it entirely over using
git://. But like you, I haven't figured out an easy way to do it.

I hoped that maybe HTTP/2 would solve some of that if we waited long
enough for it to be adopted, but it doesn't look like there's anything
out of the box. It seems like the recommended solutions still involve
websockets. I might be wrong, though; this is very much outside my area
of expertise.

> Another choice is to make a separate /capabilities endpoint that gets
> hit before /info/refs.  This is a bit bad because:
> (a) it's another HTTP request

Right, this is the extra round-trip I mentioned in:

  http://thread.gmane.org/gmane.comp.version-control.git/291640/focus=291951

I think you could get rid of it by making protocol v2 a true "client
speaks first" protocol, which aligns better with how HTTP works (but if
we do that, it would be nice to do it for _all_ of the transports, so
they stay closer to each other). But...

> (b) it adds implicit state to the HTTP conversation.  If multiple git
> servers were behind a load balancer, you might end up getting server A
> for /capabilities and server B for /info/refs, and those servers might
> have different capabilities.  This is not impossible when testing a git
> server upgrade on one machine before rolling it out to a whole fleet. 
>  Maybe the rule for clients re capabilities is that they can request
> whatever capabilities they want, but the server is free to ignore that
> request and send whatever data it feels like.  That's not great, but it
> should work (I think).

I think this is already the case today. Every non-trivial git-over-http
request requires at least two HTTP requests: one to receive the server
fetch advertisement, and the second to actually do the work (and in the
fetch case, the have/want negotiation in the second one may actually
span several requests).

The capabilities from the server come in the first request, and then the
client sends back its capabilities in the second one. So if you are
hitting multiple incompatible servers, the server may not understand
your request. Likewise, if an upload-pack request takes multiple hits,
we send up the client capabilities in each request.

I don't think quietly ignoring unknown capabilities is a good idea. The
results would range from confusing breakages (e.g., ignored multi-ack or
no-done capabilities) to subtly wrong behavior (e.g., a server which
ignores "atomic" and proceeds with a half-failed push anyway).  Given
the rarity of the situation, it's probably better for the server to barf
with an appropriate error message. That sucks for the user, but it's
probably better than the alternatives.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gitweb: fix link to parent diff with pathinfo

[Jakub Narębski]

On Tue, May 24, 2016 at 8:17 PM, Junio C Hamano  wrote:
> Richard Braun  writes:
>
>> Gitweb, when used with PATH_INFO, shows a link to parent diff
>> like http://somedomain/somerepo.git/commitdiff/somehash?hp=parenthash.
>> That link reports "400 - Invalid hash parameter".

Richard, at which view is this bad link present? Err... never mind, I see that
gitweb uses link to 'commitdiff' view with 'hash_parent' parameter set only
..in two places (well, perhaps there are some which get hash_parent from
-replay, but I doubt it): the "commit" view (link to each parent commit)
and in "commitdiff" view for octopus merges (e.g. "pu" in git.git).

The problem is not with ?hp=parenthash, but with /somehash part, which
somehow got invalid (from the error message). I have checked using
http://repo.or.cz/git.git, and while it has the bug (i.e. uses '?hp=...' instead
of path_info), there is no "400 - Invalid has parameter" error.

>> As I understand it, it should instead directly point to the parent diff,
>> i.e. turn it into http://somedomain/somerepo.git/commitdiff/parenthash,

Actually, the correct path_info based URI would be

  http://somedomain/somerepo.git/commitdiff/parenthash..somehash

Just like href() does with hash_parent_base and hash_base for blobdiff.
Urgh... href() is a bit of mess, now I see it when I am not current.

>> and delete 'hash_parent' element from the %params hash once we used it,
>> otherwise the '?hp=parenthash' string is appended.

That's correct: the unstated rule of href is that if it went into path_info,
it is deleted (not everything can be expressed with path_info), the rest
goes into query parameters. So without deleting the element, it would
be duplicated.

Note that using query parameter when we can use path_info is a minor
error; URL should work anyway (and I don't see why it doesn't - somewhat
the 'hash' parameter got incorrect...).

>>
>> Signed-off-by: Richard Braun 
>> ---
>
> Pinging...

I'm sorry, I didn't notice it was meant for me. Simple "Jakub,..."
would be enough.

On Tue, May 24, 2016 at 8:39 PM, Junio C Hamano  wrote:
> Richard Braun  writes:
>
>> On Tue, May 24, 2016 at 11:17:28AM -0700, Junio C Hamano wrote:
>>> Pinging...
>>
>> Hum, see [1].
>>
>> Tell me if I need to resend.
>
> Sorry, check the "To:" field of the message you are responding to.
> The ping was not meant to (and was not addressed to) you.  It asked
> for comments from an area expert.

Only this made me realize that you are expecting *my* response.

>>  gitweb/gitweb.perl | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 05d7910..f7f7936 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1423,7 +1423,12 @@ sub href {
>>   delete $params{'hash'};
>>   delete $params{'hash_base'};
>>   } elsif (defined $params{'hash'}) {
>> - $href .= esc_path_info($params{'hash'});
>> + if (defined $params{'hash_parent'}) {
>> + $href .= esc_path_info($params{'hash_parent'});
>> + delete $params{'hash_parent'};
>> + } else {
>> + $href .= esc_path_info($params{'hash'});
>> + }

This should read _something_ like this

+ if (defined $params{'hash_parent'}) {
+ $href .=
esc_path_info($params{'hash_parent'}) . '..';
+ delete $params{'hash_parent'};
+ }
+ $href .= esc_path_info($params{'hash'});
+  delete $params{'hash'};

Otherwise you would get correct link in your situation with
bad 'hash' parameter, but not the view that was requested;
it would not be diff between current and given parent, but
commitdiff for parent (to grandparent(s)).

Richard, thanks for finding a problematic thing, but you
need to search more for a true fix.

Regards
-- 
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Why is my git-http-backend solution using WebDAV on push?

[Luke Madhanga]

I've implemented a PHP wrapper for git http backend which works well.
I've done this to give me advanced control of who has access to
repositories on my server. You can see the implementation on
http://stackoverflow.com/questions/36998492/channel-git-on-the-server-calls-through-php/37242591#37242591.
I can pull from the server okay and all works well. However, I cannot
push. When I read my trace code to see where it fails, I see that the
last request is a PROPFIND request. The URL for this request does not
have any of the usual 'info/refs' etc. that one usually gets on git
calls.

During reading of the documentation and reviewing how git-http-backend
should be implemented, I can see that it should not be making a WebDAV
request. I have also come across posts where people implementing
http-backend the 'normal' way have also had this problem, e.g.
http://serverfault.com/questions/390864/git-push-over-http-using-git-http-backend-and-apache-is-not-working



So the question is, why is WebDAV being used on push?




Trace code from the push:

[2016-05-25 09:49:35] ==
[2016-05-25 09:49:35] REQUEST: {
"q": "p\/git-backend\/run\/1\/info\/refs",
"service": "git-receive-pack"
}
[2016-05-25 09:49:35] SERVER: {
"REDIRECT_STATUS": "200",
"HTTP_HOST": "...",
"HTTP_USER_AGENT": "git\/2.7.4",
"HTTP_ACCEPT": "*\/*",
"HTTP_ACCEPT_ENCODING": "gzip",
"HTTP_ACCEPT_LANGUAGE": "en-GB, en;q=0.9, *;q=0.8",
"HTTP_PRAGMA": "no-cache",
"PATH": "...",
"SERVER_SIGNATURE": "Apache\/2.4.18 (Ubuntu) Server at
... Port 80<\/address>\n",
"SERVER_SOFTWARE": "Apache\/2.4.18 (Ubuntu)",
"SERVER_NAME": "...",
"SERVER_ADDR": "...",
"SERVER_PORT": "80",
"REMOTE_ADDR": "...",
"DOCUMENT_ROOT": "...",
"REQUEST_SCHEME": "http",
"CONTEXT_PREFIX": "",
"CONTEXT_DOCUMENT_ROOT": "...",
"SERVER_ADMIN": "...",
"SCRIPT_FILENAME": "...",
"REMOTE_PORT": "49630",
"REDIRECT_URL": "\/p\/git-backend\/run\/1\/info\/refs",
"REDIRECT_QUERY_STRING":
"q=p\/git-backend\/run\/1\/info\/refs=git-receive-pack",
"GATEWAY_INTERFACE": "CGI\/1.1",
"SERVER_PROTOCOL": "HTTP\/1.1",
"REQUEST_METHOD": "GET",
"QUERY_STRING":
"q=p\/git-backend\/run\/1\/info\/refs=git-receive-pack",
"REQUEST_URI":
"\/p\/git-backend\/run\/1\/info\/refs?service=git-receive-pack",
"SCRIPT_NAME": "\/index.php",
"PHP_SELF": "\/index.php",
"REQUEST_TIME_FLOAT": 1464162575.091,
"REQUEST_TIME": 1464162575
}
[2016-05-25 09:49:35] Path: /core.git/info/refs?service=git-receive-pack
[2016-05-25 09:49:35] Cleaned, result only:
[2016-05-25 09:49:35] f4648182f5f8eee082c37a83a0072cfc4210e5c5 refs/heads/master
8c4efcd77809bc9b94a59cf94653add8007c6b7d refs/heads/zztest
[2016-05-25 09:49:35] ==
[2016-05-25 09:49:35] REQUEST: {
"q": "p\/git-backend\/run\/1\/HEAD"
}
[2016-05-25 09:49:35] SERVER: {
"REDIRECT_STATUS": "200",
"HTTP_HOST": "...",
"HTTP_USER_AGENT": "git\/2.7.4",
"HTTP_ACCEPT": "*\/*",
"HTTP_ACCEPT_ENCODING": "gzip",
"HTTP_ACCEPT_LANGUAGE": "en-GB, en;q=0.9, *;q=0.8",
"HTTP_PRAGMA": "no-cache",
"PATH": "...",
"SERVER_SIGNATURE": "Apache\/2.4.18 (Ubuntu) Server at
... Port 80<\/address>\n",
"SERVER_SOFTWARE": "Apache\/2.4.18 (Ubuntu)",
"SERVER_NAME": "...",
"SERVER_ADDR": "...",
"SERVER_PORT": "80",
"REMOTE_ADDR": "...",
"DOCUMENT_ROOT": "...",
"REQUEST_SCHEME": "http",
"CONTEXT_PREFIX": "",
"CONTEXT_DOCUMENT_ROOT": "...",
"SERVER_ADMIN": "...",
"SCRIPT_FILENAME": "...",
"REMOTE_PORT": "49630",
"REDIRECT_URL": "\/p\/git-backend\/run\/1\/HEAD",
"REDIRECT_QUERY_STRING": "q=p\/git-backend\/run\/1\/HEAD",
"GATEWAY_INTERFACE": "CGI\/1.1",
"SERVER_PROTOCOL": "HTTP\/1.1",
"REQUEST_METHOD": "GET",
"QUERY_STRING": "q=p\/git-backend\/run\/1\/HEAD",
"REQUEST_URI": "\/p\/git-backend\/run\/1\/HEAD",
"SCRIPT_NAME": "\/index.php",
"PHP_SELF": "\/index.php",
"REQUEST_TIME_FLOAT": 1464162575.266,
"REQUEST_TIME": 1464162575
}
[2016-05-25 09:49:35] Path: /core.git/HEAD
[2016-05-25 09:49:35] Cleaned, result only:
[2016-05-25 09:49:35] ref: refs/heads/master
[2016-05-25 09:49:35] ==
[2016-05-25 09:49:35] REQUEST: {
"q": "p\/git-backend\/run\/1\/"
}
[2016-05-25 09:49:35] SERVER: {
"REDIRECT_STATUS": "200",
"HTTP_HOST": "...",
"HTTP_USER_AGENT": "git\/2.7.4",
"HTTP_ACCEPT": "*\/*",
"HTTP_DEPTH": "0",
"CONTENT_TYPE": "text\/xml",
"CONTENT_LENGTH": "167",
"HTTP_EXPECT": "100-continue",
"PATH": "...",
"SERVER_SIGNATURE": "Apache\/2.4.18 (Ubuntu) Server at
... Port 80<\/address>\n",
"SERVER_SOFTWARE": "Apache\/2.4.18 (Ubuntu)",
"SERVER_NAME": "...",
"SERVER_ADDR": "...",
"SERVER_PORT": "80",
"REMOTE_ADDR": "...",
"DOCUMENT_ROOT": "...",
"REQUEST_SCHEME": "http",
"CONTEXT_PREFIX": "",
"CONTEXT_DOCUMENT_ROOT": "...",

Re: [WIP PATCH 00/14] Protocol v2 patches

[David Turner]

On Wed, 2016-05-25 at 09:23 -0700, Junio C Hamano wrote:
> David Turner  writes:
> 
> > I was looking at this again today, and noticed that it doesn't
> > really
> > address the HTTP case.
> > 
> > The central problem is that protocol v2 goes like this:
> > server: I have capabilities w,x,y, and z
> > client: I want capabilities x and z.
> > 
> > But HTTP goes like this:
> > client: [request]
> > server: [response]
> 
> I wonder if that can be solved by speculative request?
> 
> Let the connection initiator say "If you can do x and z, please do
> so", and allow the responder to say either "OK, I can do x and z; by
> the way the full capabilites I support are w, x, y and z", or
> "Sorry, can't do that; I have capabilities w, x, and y".

That protocol is somewhat more complicated.  And it's different than
the v2 protocol for the other transports (unless you are thinking that
we should change those too?).  It's actually a tiny bit like what I
originally proposed for HTTP. 

It sounds OK to me, but I want to know what others think before I start
implementing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> This should work, but sounds like too much of overloading of
>> --in-reply-to IMHO: if given a message id, it would only add a reference
>> to this message-id, but if given a file, it would also modify the To:
>> and Cc: list.
>>
>> Not a strong objection, though.
>
> Well, with your "that is the plan indeed", the option would behave
> the same whether given a message ID or a filename, no?

The "fetch message from ID" feature should not be unconditional IMHO. So
it would probably be stg like:

  git send-email --in-reply-to= --fetch

What's a bit counter-intuitive is that --fetch would not only trigger
fetching the complete message, but also populate To/Cc. But thinking
about it, it's not _that_ counter-intuitive, as fetching the message
should be done for a reason, so the user can guess that the message is
going to be used for something.

So, a possible UI would be:

  git send-email --in-reply-to= => just set In-Reply-To: field.

  git send-email --in-reply-to= => set In-Reply-To, To and Cc.

  git send-email --in-reply-to= --cite => in addition, add the
body of the message quoted with '> '.

  git send-email --in-reply-to= --fetch => fetch and do like 
using the default configuration for fetch.

This leaves room for:

  git send-email --in-reply-to= --fetch=gmane => fetch from gmane
(details on how to fetch would be in the config file)

This UI wouldn't allow using a file to get only the message-id. But I'm
not sure this is an interesting use-case.

So, I guess you convinced me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Please add a git config option to make --show-signature the default

[Mehul Jain]

Hi,

On Wed, May 25, 2016 at 9:28 AM, Austin English  wrote:
> I'll try
> to submit my own patch. In the meantime, it seems appropriate to file
> a bug so that others can have the opportunity to solve the problem if
> they're interested.

If you haven't started working on it and if no one else has picked it up
then I would like to try it out and submit a patch.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Junio C Hamano]

Matthieu Moy  writes:

> This should work, but sounds like too much of overloading of
> --in-reply-to IMHO: if given a message id, it would only add a reference
> to this message-id, but if given a file, it would also modify the To:
> and Cc: list.
>
> Not a strong objection, though.

Well, with your "that is the plan indeed", the option would behave
the same whether given a message ID or a filename, no?

But I do agree that those who have accustomed to the behaviour of
--in-reply-to that does not mess with To/Cc:, such a behaviour
change is not desirable.

If we are adding a new --reply-to-email=, it should behave
as a superset of --in-reply-to (i.e. it should set In-Reply-to:
using the message ID of the e-mail we are replying to), though.

>> In the future, you might even teach send-email, perhaps via a user
>> configurable hook, a way to get to the message header and text given a
>> message-id, and when it happens, the same logic can be used when
>> --in-reply-to is given a message-id (i.e. you go from the id to the
>> message and find the addresses you would To/Cc: your message).
>
> That is the plan indeed. Fetching from gmane for example should be
> rather easy in perl, and would be really convenient!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] pull: set-upstream implementation

[Junio C Hamano]

Erwan Mathoniere  writes:

> Subject: Re: [RFC/PATCH] pull: set-upstream implementation

If this were multi-patch series and one of the other patches were
"pull: set-upstream design" or something, then it might have been
understandable, but otherwise the word "implementation" sits rather
strangely in the title of this patch.

> Implements pull [--set-upstream | -u] which sets the remote tracking
> branch to the one the user just pulled from.
>
>   git pull [--set-upstream | -u]  
>
> After successfully fetched from , sets branch..remote to
>  and branch..merge to follow /
>
> Signed-off-by: Erwan Mathoniere 
> Signed-off-by: Jordan De Gea 
> Signed-off-by: Matthieu Moy 
> ---
> Hello,
>
> `git push --set-upstream  ` sets the remote and merge config
> variables for  after successfully pushed.
>
> This patch implements an equivalent for `git pull` or
> `git branch --set-upstream-to=/`.
>
> Difficulties:
>   - I can't figure out what remote branch sould be tracked
>   in that case: `git pull -u origin :master`

What does the command do without "-u"?

>   - Is the result of 'resolve_ref_unsafe' should be freed ?

Check its function signature--it returns "const char *" which is a
sign that the memory does not belong to you (i.e. the caller), and
you should never free it.

>  Documentation/git-pull.txt |  7 +
>  builtin/pull.c | 61 +
>  t/t5544-pull-upstream.sh   | 75 
> ++
>  3 files changed, 143 insertions(+)
>  create mode 100755 t/t5544-pull-upstream.sh
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index d033b25..3a2e0b7 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -93,6 +93,13 @@ OPTIONS
>   has to be called afterwards to bring the work tree up to date with the
>   merge result.
>  
> +-u::
> +--set-upstream::
> + For each branch that is successfully pulled, add
> + upstream (tracking) reference, used by argument-less
> + linkgit:git-push[1] and other commands. For more information,
> + see 'branch..merge' in linkgit:git-config[1].

To me, "For each branch" hints that I could do this:

git pull --set-upstream git://git.kernel.org/r/e/p/o foo bar

and the command would do something to 'foo' and 'bar'.

But I suspect that is not what is going on and that is not what you
wanted to achieve.

A crucial piece of information that is lacking in the above is where
 comes from.  The same issue exists in your proposed commit
log message.

I think that you meant to add a feature to add branch..remote
and branch..merge configuration variables for the current
branch whose name is , and the values to be recorded for these
two configuration variables are the same as those given on the
command line.  For example "git checkout -b topic master && git pull
origin that" would set "branch.topic.remote" and
"branch.topic.merge" to "origin" and "that", respectively.

Without explaining what  is, "For more information" that
refers to 'branch..merge' does not help the readers much.

Side note.  It is an understandable mistake.  After one
spent a lot of effort designing, implementing and debugging
a feature, by the time one describes what it does, some
assumptions one made earlier becomes so fundamental in one's
mind that one forgets that it is not obvious to others.

There a few design decisions you must have made that needs to be
either described fully or at least hinted here, too, such as (not
exhaustive and in random order):

 * What should happen when the current branch already has these two
   configuration variables defined?  Should the user get a warning
   when this changes the setting from what was already there?

 * What should happen when the remote is specified as a Git URL, not
   as a remote nickname?  You want a nickname for the value to place
   in "branch..remote".

- Should Git find an existing remote that points at the URL and
  use it?  If so, and if the value we are about to place in
  "branch..merge" is outside its configured fetch refspec,
  should Git tweak the fetch refspec of the existing remote?

- Should Git create a new remote nickname for the URL?  If so,
  what should the fetch refspec be for the newly created remote?
  Should it fetch only the branch we fetched just now?

 * What should happen when more than one ref is given?
   branch_get_upstream() can return only one ref; should Git choose
   one of them at random?  Should Git warn and turn this into a
   no-op?

 * What should happen when the ref given to pull is not a branch
   (e.g. a tag)?  A tag, meant to be a stable anchoring point, is
   not something suitable to be set as branch..merge, even
   though it might be technically possible to 

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Matthieu Moy]

Junio C Hamano  writes:

> I wonder if we can safely repurpose existing --in-reply-to option?

In theory, obviously no as there can be a file with this name _and_ it
can be a valid message-id. In practice, it is clearly unlikely. The only
use-case I can think of where both would be valid is if the user happens
to have saved the message using the message-id as filename. But then,
the ambiguity would not harm, as the message-id contained in the file
would be the same as the filename.

> That is, if the value of --in-reply-to can be reliably determined as
> a filename that has the message (as opposed to a message-id), we
> read the "Message-Id:" from that file to figuire out what message-id
> to use, and figure out To/Cc: to use for the purpose of your (1) at
> the same time.

This should work, but sounds like too much of overloading of
--in-reply-to IMHO: if given a message id, it would only add a reference
to this message-id, but if given a file, it would also modify the To:
and Cc: list.

Not a strong objection, though.

> In the future, you might even teach send-email, perhaps via a user
> configurable hook, a way to get to the message header and text given a
> message-id, and when it happens, the same logic can be used when
> --in-reply-to is given a message-id (i.e. you go from the id to the
> message and find the addresses you would To/Cc: your message).

That is the plan indeed. Fetching from gmane for example should be
rather easy in perl, and would be really convenient!

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Johannes Schindelin]

Hi Junio,

On Wed, 25 May 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Wed, 25 May 2016, Junio C Hamano wrote:
> >
> >>  * I am not familiar with life on filesystems with core.filemode=0;
> >>do files people would want to be able to "add --chmod=+x" share
> >>common trait that can be expressed with .gitattributes mechanism?
> >
> > I think it is safe to say that the biggest example of core.filemode == 0
> > is Windows. On that platform, there simply is no executable bit in the
> > sense of POSIX permissions. ...
> > ... I still like Ed's idea and would love to have it: it is murky waters to
> > require users to call plumbing only because our porcelain isn't up to par.
> 
> I thought that I made it absolutely clear that I like the addition,
> too.  If it wasn't clear enough, I can say it again, but I do not
> think you need it ;-).

Oh, I understood that you liked it, sorry if my mail looked accusatory.

> The "attribute" thing was an idea that was hoping to make the system
> as a whole even more helpful;

I understood that, too. My first impression was that it would not be.
However, as Git for Windows can set default attributes in
/mingw64/etc/gitconfig, I guess it would actually be helpful. We could
automatically mark all *.exe, *.com, *.bat, *.cmd files as executable. It
would then still be the users' responsibility to add their own attributes
for, say, *.js, *.rb, *.py, *.sh, and whatever else.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [WIP PATCH 00/14] Protocol v2 patches

[David Turner]

On Wed, 2016-05-25 at 06:03 +0700, Duy Nguyen wrote:
> On Wed, May 25, 2016 at 5:46 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > I was looking at this again today, and noticed that it doesn't
> > really
> > address the HTTP case.
> > 
> > The central problem is that protocol v2 goes like this:
> > server: I have capabilities w,x,y, and z
> > client: I want capabilities x and z.
> > 
> > But HTTP goes like this:
> > client: [request]
> > server: [response]
> > 
> > I tried to make libcurl do the receive-before-sending thing, but it
> > doesn't seem to be designed for it (even if you prime things by
> > sending
> > a "hello" from the client first).  My thought was to hook up
> > CURLOPT_READFUNCTION and CURLOPT_WRITEFUNCTION, and have the read
> > function return CURL_READFUNC_PAUSE and then have the write
> > (=client
> > receiving data ) function unpause the reader (= client sending
> > data)
> > once it gets the capabilities.  But apparently pausing only works
> > with
> > chunked encoding, which seems to cause Apache's mod_cgi to fail.
> > 
> > Maybe I'm missing something.  Has anyone else ever made something
> > like
> > this work?
> 
> It simply takes one more round-trip to negotiate. Not the best thing,
> but...

Do you mean that it can be done with libcurl?  Or do you mean that I
should go with the /capabilities endpoint?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [WIP PATCH 00/14] Protocol v2 patches

[Junio C Hamano]

David Turner  writes:

> I was looking at this again today, and noticed that it doesn't really
> address the HTTP case.
>
> The central problem is that protocol v2 goes like this:
> server: I have capabilities w,x,y, and z
> client: I want capabilities x and z.
>
> But HTTP goes like this:
> client: [request]
> server: [response]

I wonder if that can be solved by speculative request?

Let the connection initiator say "If you can do x and z, please do
so", and allow the responder to say either "OK, I can do x and z; by
the way the full capabilites I support are w, x, y and z", or
"Sorry, can't do that; I have capabilities w, x, and y".


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Johannes Schindelin  writes:

> On Wed, 25 May 2016, Junio C Hamano wrote:
>
>>  * I am not familiar with life on filesystems with core.filemode=0;
>>do files people would want to be able to "add --chmod=+x" share
>>common trait that can be expressed with .gitattributes mechanism?
>
> I think it is safe to say that the biggest example of core.filemode == 0
> is Windows. On that platform, there simply is no executable bit in the
> sense of POSIX permissions. ...
> ... I still like Ed's idea and would love to have it: it is murky waters to
> require users to call plumbing only because our porcelain isn't up to par.

I thought that I made it absolutely clear that I like the addition,
too.  If it wasn't clear enough, I can say it again, but I do not
think you need it ;-).

The "attribute" thing was an idea that was hoping to make the system
as a whole even more helpful; if pattern matching with paths is
sufficient for projects to hint desired permission bits per paths,
then those working on such a cross-platform project on Windows do
not have to even worry about "git cmd --chmod=+x", whether cmd is
add or update-index.  If they can just do "git add" and need to use
the new "--chmod=+x" option only when the patterns are not set up
correctly, wouldn't that be even more helpful?  In other words, it
wasn't "with this we can _eliminate_ need for 'add --chmod'".

The only thing I was unsure about that scheme was if "pattern
matching with paths" is sufficiently powerful (if not, such an
addition would not work as a mechanism to reduce the need for the
users to run "git add --chmod=+x").  And that was my inquiry.

Unfortunately, your answer does not help answer that question;
it was a question to Edward, so that's OK anyway.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Junio C Hamano  writes:

>> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char 
>> *path, struct stat *st,
>>  
>>  if (trust_executable_bit && has_symlinks)
>>  ce->ce_mode = create_ce_mode(st_mode);
>> +else if (force_executable)
>> +ce->ce_mode = create_ce_mode(0777);
>> +else if (force_notexecutable)
>> +ce->ce_mode = create_ce_mode(0666);
>
> This is an iffy design decision.
>
> Even when you are in core.filemode=true repository, if you
> explicitly said
>
>   git add --chmod=+x READ.ME
>
> wouldn't you expect that the path would have executable bit in the
> index, whether it has it as executable in the filesystem?  The above
> if/else cascade, because trust-executable-bit is tested first, will
> ignore force_* flags altogether, won't it?  It also is strange that
> the decision to honor or ignore force_* flags is also tied to
> has_symlinks, which is a totally orthogonal concept.

Here is an additional patch to your tests.  It repeats one of the
tests you added, but runs in a repository with core.filemode and
core.symlinks both enabled.  The test fails to force executable bit
on platforms where it runs.

It passes with your patch if you drop core.symlinks, which is a good
demonstration why letting has_symlinks decide if force* is to be
honored is iffy.

 t/t3700-add.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index e551eaf..2afcb74 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -351,4 +351,15 @@ test_expect_success 'git add --chmod=-x stages an 
executable file with -x' '
esac
 '
 
+test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x' '
+   git config core.filemode 1 &&
+   git config core.symlinks 1 &&
+   echo foo >foo2 &&
+   git add --chmod=+x foo2 &&
+   case "$(git ls-files --stage foo2)" in
+   100755" "*foo2) echo pass;;
+   *) echo fail; git ls-files --stage foo2; (exit 1);;
+   esac
+'
+
 test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Junio C Hamano]

Matthieu Moy  writes:

> Actually, I'm not sure what the ideal behavior should be. Perhaps it's
> better to distinguish 1) and 2) above, and have two options
> --reply-to-email= doing 1), and --quote doing 2), implying
> --compose and requiring --reply-to-email.

I tend to agree that sounds like a better way to structure these
features.

I wonder if we can safely repurpose existing --in-reply-to option?
That is, if the value of --in-reply-to can be reliably determined as
a filename that has the message (as opposed to a message-id), we
read the "Message-Id:" from that file to figuire out what message-id
to use, and figure out To/Cc: to use for the purpose of your (1) at
the same time.  In the future, you might even teach send-email,
perhaps via a user configurable hook, a way to get to the message
header and text given a message-id, and when it happens, the same
logic can be used when --in-reply-to is given a message-id (i.e. you
go from the id to the message and find the addresses you would
To/Cc: your message).

> In any case, quoting the message without replying to it does not make
> sense (especially if you add instructions to trim it: the user would not
> even see it). So it its current form, I'd say --quote-email should imply
> --annotate.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH] pull: set-upstream implementation

[Erwan Mathoniere]

Implements pull [--set-upstream | -u] which sets the remote tracking
branch to the one the user just pulled from.

git pull [--set-upstream | -u]  

After successfully fetched from , sets branch..remote to
 and branch..merge to follow /

Signed-off-by: Erwan Mathoniere 
Signed-off-by: Jordan De Gea 
Signed-off-by: Matthieu Moy 
---
Hello,

`git push --set-upstream  ` sets the remote and merge config
variables for  after successfully pushed.

This patch implements an equivalent for `git pull` or
`git branch --set-upstream-to=/`.

Difficulties:
- I can't figure out what remote branch sould be tracked
in that case: `git pull -u origin :master`
- Is the result of 'resolve_ref_unsafe' should be freed ?

What's your opinion ?


 Documentation/git-pull.txt |  7 +
 builtin/pull.c | 61 +
 t/t5544-pull-upstream.sh   | 75 ++
 3 files changed, 143 insertions(+)
 create mode 100755 t/t5544-pull-upstream.sh

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..3a2e0b7 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -93,6 +93,13 @@ OPTIONS
has to be called afterwards to bring the work tree up to date with the
merge result.
 
+-u::
+--set-upstream::
+   For each branch that is successfully pulled, add
+   upstream (tracking) reference, used by argument-less
+   linkgit:git-push[1] and other commands. For more information,
+   see 'branch..merge' in linkgit:git-config[1].
+
 Options related to merging
 ~~
 
diff --git a/builtin/pull.c b/builtin/pull.c
index 1d7333c..e096858 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "branch.h"
 
 enum rebase_type {
REBASE_INVALID = -1,
@@ -109,6 +110,9 @@ static char *opt_unshallow;
 static char *opt_update_shallow;
 static char *opt_refmap;
 
+/* Options about upstream */
+static int opt_set_upstream;
+
 static struct option pull_options[] = {
/* Shared options */
OPT__VERBOSITY(_verbosity),
@@ -116,6 +120,9 @@ static struct option pull_options[] = {
N_("force progress reporting"),
PARSE_OPT_NOARG),
 
+   /* Options about upstream */
+   OPT_BOOL('u', "set-upstream", _set_upstream, N_("set upstream for 
git pull/status")),
+
/* Options passed to git-merge or git-rebase */
OPT_GROUP(N_("Options related to merging")),
{ OPTION_CALLBACK, 'r', "rebase", _rebase,
@@ -829,6 +836,57 @@ static int run_rebase(const unsigned char *curr_head,
return ret;
 }
 
+static int set_pull_upstream(const char *repo, const char **refspecs_name)
+{
+   unsigned char sha1[GIT_SHA1_RAWSZ];
+   const char *local_branch, *remote_branch;
+   struct strbuf remote_name;
+   struct refspec *refspecs;
+   int nr_refspec, i;
+
+   if (repo == NULL) {
+   warning(N_("no remote was specified"));
+   return 1;
+   }
+
+   for (nr_refspec = 0; refspecs_name[nr_refspec] != NULL; nr_refspec++);
+
+   if (nr_refspec == 0) {
+   warning(N_("no refspec was specified"));
+   return 1;
+   }
+
+   refspecs = parse_fetch_refspec(nr_refspec, refspecs_name);
+
+   for (i = 0; i < nr_refspec; i++) {
+   if (refspecs[i].src != NULL && strlen(refspecs[i].src) > 0) {
+   remote_branch = refspecs[i].src;
+   } else {
+   warning(N_("not yet implemented"));
+   continue;
+   }
+
+   if (refspecs[i].dst != NULL && strlen(refspecs[i].dst) > 0) {
+   local_branch = refspecs[i].dst;
+   } else {
+   // TODO : Should it be freed ?
+   local_branch = resolve_ref_unsafe("HEAD", 0, sha1, 
NULL);
+   skip_prefix(local_branch, "refs/heads/", _branch);
+   }
+
+   strbuf_init(_name, strlen(repo) + strlen(remote_branch) 
+ 1);
+   strbuf_addf(_name, "%s/%s", repo, remote_branch);
+
+   create_branch(local_branch, local_branch, remote_name.buf, 0, 
0, 0, opt_verbosity < 0, BRANCH_TRACK_OVERRIDE);
+
+   strbuf_release(_name);
+   }
+
+   free_refspec(nr_refspec, refspecs);
+
+   return 0;
+}
+
 int cmd_pull(int argc, const char **argv, const char *prefix)
 {
const char *repo, **refspecs;
@@ -884,6 +942,9 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
if (opt_dry_run)
return 0;
 
+   if (opt_set_upstream)
+   set_pull_upstream(repo, refspecs);
+
if (get_sha1("HEAD", curr_head))
  

Re: [PATCH v2 15/22] i18n: rebase-interactive: mark here-doc strings for translation

[Vasco Almeida]

Às 19:27 de 23-05-2016, Vasco Almeida escreveu:
> Add eval_ngettext dummy function to be called by tests when running
> under GETTEXT_POISON. Otherwise, tests would fail under gettext poison
> because that function could not be found.
> 
> [...]
>
> diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
> index e6c3116..c14c303 100644
> --- a/git-sh-i18n.sh
> +++ b/git-sh-i18n.sh
> @@ -64,6 +64,10 @@ poison)
>   eval_gettext () {
>   printf "%s" "# GETTEXT POISON #"
>   }
> +
> + eval_ngettext () {
> + printf "%s" "# GETTEXT POISON #"
> + }
>   ;;
>  *)
>   gettext () {

Perhaps, I should have added more eval_ngettext dummy functions in other
places in this file. Not exclusively to the poison part of the switch case.

I see Solaris is a special case (gettext_without_eval_gettext), does it
have eval_ngettext? Probably not, since it does not have
eval_gettext(1). In such case, we would call ngettext like it's done for
eval_gettext, assuming Solaris has ngettext.

The relevant part of git-sh-i18n.sh updated with this patch:

# ... and then follow that decision.
case "$GIT_INTERNAL_GETTEXT_SH_SCHEME" in
gnu)
# Use libintl's gettext.sh, or fall back to English if we can't.
. gettext.sh
;;
gettext_without_eval_gettext)
# Solaris has a gettext(1) but no eval_gettext(1)
eval_gettext () {
gettext "$1" | (
export PATH $(git sh-i18n--envsubst --variables "$1");
git sh-i18n--envsubst "$1"
)
}
;;
poison)
# Emit garbage so that tests that incorrectly rely on translatable
# strings will fail.
gettext () {
printf "%s" "# GETTEXT POISON #"
}

eval_gettext () {
printf "%s" "# GETTEXT POISON #"
}

eval_ngettext () {
printf "%s" "# GETTEXT POISON #"
}
;;
*)
gettext () {
printf "%s" "$1"
}

eval_gettext () {
printf "%s" "$1" | (
export PATH $(git sh-i18n--envsubst --variables "$1");
git sh-i18n--envsubst "$1"
)
}
;;
esac
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Johannes Schindelin]

Hi Junio,

On Wed, 25 May 2016, Junio C Hamano wrote:

>  * I am not familiar with life on filesystems with core.filemode=0;
>do files people would want to be able to "add --chmod=+x" share
>common trait that can be expressed with .gitattributes mechanism?

I think it is safe to say that the biggest example of core.filemode == 0
is Windows. On that platform, there simply is no executable bit in the
sense of POSIX permissions. There are Access Control Lists that let you
permit or deny certain users from executing certain files (and it is files
only, directories are a never "executable" as in POSIX' scheme).

And on Windows, you certainly do not mark a file explicitly as executable
when creating it. The file extension determines whether it is executable
or not, and that's it.

In a sense, it is cleaner than POSIX' permission system: it separates
between the permissions and the classification "is it executable"?

Side note: shell scripts in Git for Windows are a special type of animal.
They *cannot* be made executable by Windows because there is just no
concept of free-form scripts that can choose whatever interpreter they
want to run in. In Git Bash, we have a special hack that is inherited
transitively from Cygwin, where scripts are automatically marked as
executable if their contents start with a shebang. This stops working as
soon as you leave the Bash, of course. Due to Git's heavy dependence on
shell scripting, we had to imitate that same concept in compat/mingw.c. It
is ugly, it is slow, and we have to live with it.

As a consequence of this very different concept of an "executable bit",
you will actually see quite a few Windows-only repositories that *never*
mark their executables with 0755. In Git for Windows' own repositories,
there are a couple of examples where scripts were introduced and only much
later did I realize that they were not marked executable (and then I ran
`git update-index --chmod=+x` on them).

All that means that the `--chmod` option is really useful only to
cross-platform projects, and only with careful developers. (And those
developers could use update-index, as you pointed out.)

I still like Ed's idea and would love to have it: it is murky waters to
require users to call plumbing only because our porcelain isn't up to par.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Odd Difference Between Windows Git and Standard Git

[Johannes Schindelin]

Hi Torsten,

On Wed, 25 May 2016, Torsten Bögershausen wrote:

> On 05/24/2016 01:57 PM, Johannes Schindelin wrote:
> >
> > On Tue, 24 May 2016, Torsten Bögershausen wrote:
> >
> > > if core.filemode is true, Git for Windows could:
> > > a) Behave as today, report changed files (filemode)
> > > b) Give warning to the user (and report changed filemode)
> > > c) Error out, saying misconfigured worktree
> > > d) use core.filemode = false anyway.
> > > e) Give a warning and use core.filemode = false anyway.
> > >
> > > At the moment I tend for c), as it makes it clear what is going wrong,
> > > what do you think ?
> >
> > The problem with that is that we would need to probe again.
>
> The probing for the filemode:
> Wouldn't it be enough to run lstat() on .git/ ?

What about `git diff --no-index`? There is no `.git/` to probe there.

> If the user-execuatable bit is not set, but core.filemode is true, error
> out ?  That would not cost too much.

I think it would cost us a nice and clean logic ;-)

> > Or dictate for all eternity that Git for Windows cannot determine the
> > executable bit (but who knows for certain?)
>
> Can we can limit the eternity until the day when Windows can determine
> the executable bit ?

The point is: I will have forgotten by next week what we talked about
(there are way too many things going on in my life), and if and when
compat/mingw.c will be taught to infer the executable bit, I am prone to
forget that warning (if we introduce it).

Therefore, out of entirely practical considerations, I favor the status
quo.

Ciao,
Dscho

Re: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano  wrote:
> Armin Kunaschik  writes:
>>
>> Ok, how can this be implemented within the test environment?
>
> I actually think an unconditional check like this is sufficient.

Ah, good. My thoughts were a bit more complicated.
Anyway, this works for me.
Thanks!

>  t/t7800-difftool.sh | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 7ce4cd7..f304228 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged 
> files' '
> test_cmp expect actual
>  '
>
> -write_script .git/CHECK_SYMLINKS <<\EOF
> -for f in file file2 sub/sub
> -do
> -   echo "$f"
> -   readlink "$2/$f"
> -done >actual
> -EOF
> -
>  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without 
> unstaged changes' '
> +
> +   write_script .git/CHECK_SYMLINKS <<-\EOF &&
> +   for f in file file2 sub/sub
> +   do
> +   echo "$f"
> +   ls -ld "$2/$f" | sed -e "s/.* -> //"
> +   done >actual
> +   EOF
> +
> cat >expect <<-EOF &&
> file
> $PWD/file
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Johannes Schindelin]

Hi Ed,

On Tue, 24 May 2016, Edward Thomson wrote:

> Users on deficient filesystems that lack an execute bit may still
> wish to add files to the repository with the appropriate execute
> bit set (or not).  Although this can be done in two steps
> (`git add foo && git update-index --chmod=+x foo`), providing the
> `--chmod=+x` option to the add command allows users to set a file
> executable in a single command that they're already familiar with.
> 
> Signed-off-by: Edward Thomson 

I like it! Some comments below:

> diff --git a/builtin/add.c b/builtin/add.c
> index 145f06e..2a9abf7 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, 
> ignore_missing;
>  static int addremove = ADDREMOVE_DEFAULT;
>  static int addremove_explicit = -1; /* unspecified */
>  
> +static char should_chmod = 0;
> +
>  static int ignore_removal_cb(const struct option *opt, const char *arg, int 
> unset)
>  {
>   /* if we are told to ignore, we are not adding removals */
> @@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, 
> const char *arg, int unse
>   return 0;
>  }
>  
> +static int chmod_cb(const struct option *opt, const char *arg, int unset)
> +{
> + char *flip = opt->value;
> + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
> + return error("option 'chmod' expects \"+x\" or \"-x\"");
> + *flip = arg[0];
> + return 0;
> +}
> +
>  static struct option builtin_add_options[] = {
>   OPT__DRY_RUN(_only, N_("dry run")),
>   OPT__VERBOSE(, N_("be verbose")),
> @@ -263,6 +274,9 @@ static struct option builtin_add_options[] = {
>   OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
> index")),
>   OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
> which cannot be added because of errors")),
>   OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
> missing - files are ignored in dry run")),
> + { OPTION_CALLBACK, 0, "chmod", _chmod, N_("(+/-)x"),
> +   N_("override the executable bit of the listed files"),
> +   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb},

I wonder, however, whether it would be "cleaner" to simply make this an
OPT_STRING and perform the validation after the option parsing. Something
like:

const char *chmod_string = NULL;
...
OPT_STRING( 0 , "chmod", _string, N_("( +x | -x )"),
N_("override the executable bit of the listed files")),
...
flags = ...
if (chmod_string) {
if (!strcmp("+x", chmod_string))
flags |= ADD_CACHE_FORCE_EXECUTABLE;
else if (!strcmp("-x", chmod_string))
flags |= ADD_CACHE_FORCE_NOTEXECUTABLE;
else
die(_("invalid --chmod value: %s"), chmod_string);
}

> diff --git a/cache.h b/cache.h
> index 6049f86..da03cd9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -581,6 +581,8 @@ extern int remove_file_from_index(struct index_state *, 
> const char *path);
>  #define ADD_CACHE_IGNORE_ERRORS  4
>  #define ADD_CACHE_IGNORE_REMOVAL 8
>  #define ADD_CACHE_INTENT 16
> +#define ADD_CACHE_FORCE_EXECUTABLE 32
> +#define ADD_CACHE_FORCE_NOTEXECUTABLE 64

Hmm. This change uses up 2 out of 31 available bits. I wonder whether a
better idea would be to extend struct update_callback_data to include a
`force_mode` field, pass a parameter of the same name to
add_files_to_cache() and then handle that in the update_callback().
Something like this:

case DIFF_STATUS_MODIFIED:
-   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_TYPE_CHANGED: {
+   struct stat st;
+   if (lstat(path, ))
+   die_errno("unable to stat '%s'", path);
+   if (S_ISREG(_mode) && data->force_mode)
+   st.st_mode = data->force_mode;
-   if (add_file_to_index(_index, path, data->flags)) {
+   if (add_to_index(_index, path, , data->flags)) {
if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
die(_("updating files failed"));
data->add_errors++;
}
break;
+   }

This would not only contain the changes in builtin/add.c, it would also
force the mode change when core.filemode = true and core.symlinks = true
(which your version would handle in a surprising way, I believe).

> 2.6.4 (Apple Git-63)

Time to upgrade? ;-)

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Edward Thomson  writes:

>   if (trust_executable_bit && has_symlinks)
>   ce->ce_mode = create_ce_mode(st_mode);
> + else if (force_executable)
> + ce->ce_mode = create_ce_mode(0777);
> + else if (force_notexecutable)
> + ce->ce_mode = create_ce_mode(0666);
>   else {
>   /* If there is an existing entry, pick the mode bits and type
>* from it, otherwise assume unexecutable regular file.

I would rather do this part more like:

if (S_ISREG(st_mode) && (force_executable || force_nonexecuable)) {
if (force_executable)
ce->ce_mode = create_ce_mode(0777);
else
ce->ce_mode = create_ce_mode(0666);
} else if (trust_executable_bit && has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
} else {
... carry the existing mode over ...

which would make sure that the new code will not interfere with
symbolic links and that forcing will be honored even on filesystems
whose executable bit can be trusted (i.e. "can be trusted" does not
have to mean "must be trusted").

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Edward Thomson  writes:

> Users on deficient filesystems that lack an execute bit may still
> wish to add files to the repository with the appropriate execute
> bit set (or not).  Although this can be done in two steps
> (`git add foo && git update-index --chmod=+x foo`), providing the
> `--chmod=+x` option to the add command allows users to set a file
> executable in a single command that they're already familiar with.
>
> Signed-off-by: Edward Thomson 
> ---

I think we should tone down the first sentence of the proposed
commit log message.  With "s/deficient //" the paragraph still reads
perfectly well.  Even when an underlying filesystem is capable of
expressing executable bit, we can set core.filemode to false to
emulate the behaviour on DOS, so perhaps

The executable bit will not be set for paths in a repository
with core.filemode set to false, but the users may still wish to
add files to ...

or something like it?

Giving an easy to use single-command short-hand for a common thing
that takes two commands is a worthy goal.  Another way to do the
above is

git update-index --add --chmod=+x foo

and from that point of view, we do not need this patch, but that is
still a mouthful ;-)  I think it is a good idea to teach "git add",
an end-user facing command, to be more helpful.

At the design level, I have a few comments.

 * Unlike the command "chmod", which is _only_ about changing modes,
   "add --chmod" updates both the contents and modes in the index,
   which may invite "I only want to change modes--how?"

Note. We had an ancient regression at 227bdb18 (make
update-index --chmod work with multiple files and --stdin,
2006-04-23), where "update-index --chmod=+x foo" stopped
being "only flip the executable bit without hashing the
contents" and that was done purely by mistake.  There is no
longer a good answer to that question, which makes the above
worry less of an issue.

 * This is about a repository with core.filemode=0; I wonder if
   something for a repository with core.symlinks=0 would also help?
   That is, would it be a big help to users if they can prepare a
   text file that holds symbolic link contents and add it as if it
   were a symlink with "git add", instead of having to run two
   commands, "hash-objects && update-index --cacheinfo"?

 * I am not familiar with life on filesystems with core.filemode=0;
   do files people would want to be able to "add --chmod=+x" share
   common trait that can be expressed with .gitattributes mechanism?

   What I am wondering is if a scheme like the following would work
   well, in addition to your patch:

   1. Have these in .gitattributes:

  * -executable
  *.bat executable text
  *.exe executable binary
  *.com executable binary

  A path with Unset "executable" attribute is explicitly marked
  as "not executable"; a path with Set "executable" attribute is
  marked as "executable", i.e. "needing chmod=+x".

   2. Teach "git add" to take the above hint _only_ in a repository
  where core.filemode is false and _only_ when adding a new path
  to the index.

   If something like this works well enough, users do not have to
   type --chmod=+x too often when doing "git add"; your patch
   becomes an escape hatch that is only needed when the attributes
   system gets it wrong.


Now some comments on the actual code.

> +static int chmod_cb(const struct option *opt, const char *arg, int unset)
> +{
> + char *flip = opt->value;
> + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
> + return error("option 'chmod' expects \"+x\" or \"-x\"");
> + *flip = arg[0];
> + return 0;
> +}

I know you mimicked the command line parser of update-index, but you
didn't have to, and you shouldn't have.

The command line semantics of update-index is largely "we read one
option and prepare to make its effect immediately available", which
predates parse-options where its attitude for command line parsing
is "we first parse all options and figure out what to do, and then
we work on arguments according to these options".  Because of these
vastly different attitudes, the way builtin/update-index.c uses
parse-options API is atypical.  The only reason it uses callback is
because it wants to allow you to say this:

git update-index --chmod=+x foo bar --chmod=-x baz

and register foo and bar as executable, while baz as non-executable.

The way update-index uses parse-options API is not something you
want to mimick when adding a similar option to a more modern command
like "git add", whose attitude toward command line parsing is quite
different.  Modern command line parsing typically takes "the last
one wins" semantics, i.e.

git add --chmod=-x --chmod=+x foo

would make foo executable.

If I were doing this patch, I'd just allocate a file scope 

Re: [PATCH 09/26] upload-pack: move rev-list code out of check_non_tip()

[Eric Sunshine]

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -451,7 +451,7 @@ static int is_our_ref(struct object *o)
> -static void check_non_tip(void)
> +static int check_unreachable(struct object_array *src)
> @@ -461,14 +461,6 @@ static void check_non_tip(void)
> -   /*
> -* In the normal in-process case without
> -* uploadpack.allowReachableSHA1InWant,
> -* non-tip requests can never happen.
> -*/
> -   if (!stateless_rpc && !(allow_unadvertised_object_request & 
> ALLOW_REACHABLE_SHA1))
> -   goto error;
> -
> @@ -476,7 +468,7 @@ static void check_non_tip(void)
> if (start_command())
> -   goto error;
> +   return 0;
> @@ -495,16 +487,16 @@ static void check_non_tip(void)
> if (write_in_full(cmd.in, namebuf, 42) < 0)
> -   goto error;
> +   return 0;
> [...]
> +   for (i = 0; i < src->nr; i++) {
> +   o = src->objects[i].item;
> if (is_our_ref(o))
> continue;
> memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
> if (write_in_full(cmd.in, namebuf, 41) < 0)
> -   goto error;
> +   return 0;
> }
> close(cmd.in);

It's a little bit ugly how this close() becomes disconnected after the
refactoring. In the original code, due to the consistent application
of 'goto error', it's reasonably obvious that skipping the close() is
not harmful since the error case die()'s unconditionally (according to
the comment). However, after the refactoring, the function simply
returns without invoking close(), so there's a disconnect, and it's
not obvious without looking at the caller that the program will die().

Also, if this function later gets a new caller, is that caller going
to need intimate knowledge about this potential descriptor leak?

> @@ -516,7 +508,7 @@ static void check_non_tip(void)
>  */
> i = read_in_full(cmd.out, namebuf, 1);
> if (i)
> -   goto error;
> +   return 0;
> close(cmd.out);

Same observation.

It might be clearer if you retained the 'error' label and used it to
ensure that the descriptors get closed:

cmd.in = -1;
cmd.out = -1;
...
if (...)
goto error;
...
/* All the non-tip ... */
return 1;

error:
if (cmd.in >= 0)
close(cmd.in);
if (cmd.out >= 0)
close(cmd.out);
return 0;

> /*
> @@ -525,15 +517,29 @@ static void check_non_tip(void)
>  * even when it showed no commit.
>  */
> if (finish_command())
> -   goto error;
> +   return 0;

Here too. Does 'cmd' need to be cleaned up if the function bails
before finish_command()?

> /* All the non-tip ones are ancestors of what we advertised */
> -   return;
> +   return 1;
> +}
> +
> +static void check_non_tip(void)
> +{
> +   int i;
> +
> +   /*
> +* In the normal in-process case without
> +* uploadpack.allowReachableSHA1InWant,
> +* non-tip requests can never happen.
> +*/
> +   if (!stateless_rpc && !(allow_unadvertised_object_request & 
> ALLOW_REACHABLE_SHA1))
> +   ;   /* error */
> +   else if (check_unreachable(_obj))
> +   return;

With the loss of the 'error' label (below), this logic becomes a bit
more difficult to follow. I wonder if it would help to invert the
sense of the conditional...

if (stateless_rpc || ...)
if (check_unreachable(...))
return;

Or, perhaps, retain the 'error' label:

if (!stateless_rpc && ...)
goto error:
if (check_unreachable(...))
return;

error:
/* Pick one ... */

I think I might find the latter a bit clearer, but it's highly subjective.

> -error:
> /* Pick one of them (we know there at least is one) */
> for (i = 0; i < want_obj.nr; i++) {
> -   o = want_obj.objects[i].item;
> +   struct object *o = want_obj.objects[i].item;
> if (!is_our_ref(o))
> die("git upload-pack: not our ref %s",
> oid_to_hex(>oid));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/26] upload-pack: use skip_prefix() instead of starts_with()

[Eric Sunshine]

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> upload-pack: use skip_prefix() instead of starts_with()
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -403,8 +405,8 @@ static int get_common_commits(void)
> -   if (starts_with(line, "have ")) {
> -   switch (got_sha1(line+5, sha1)) {
> +   if (skip_prefix(line, "have ", )) {
> +   switch (got_sha1(arg, sha1)) {

There's one more instance of starts_with() in upload-pack.c:main()
which could be given the same treatment:

if (starts_with(arg, "--timeout=")) {
timeout = atoi(arg+10);
daemon_mode = 1;
continue;
}

Was its omission an oversight or intentional since it's not related to
the others?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/26] transport-helper.c: refactor set_helper_option()

[Eric Sunshine]

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> For now we can handle two types, string and boolean, in
> set_helper_option(). Later on we'll add string_list support, which does
> not fit well. The new function strbuf_set_helper_option() can be reused
> for a separate function that handles string-list.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/transport-helper.c b/transport-helper.c
> @@ -260,6 +260,28 @@ static const char *boolean_options[] = {
> +static int strbuf_set_helper_option(struct helper_data *data,
> +   struct strbuf *buf)
> +{
> +   int ret;
> +
> +   sendline(data, buf);
> +   if (recvline(data, buf))
> +   exit(128);
> +
> +   if (!strcmp(buf->buf, "ok"))
> +   ret = 0;
> +   else if (starts_with(buf->buf, "error")) {
> +   ret = -1;
> +   } else if (!strcmp(buf->buf, "unsupported"))

You could use this opportunity to drop the unnecessary braces. (True,
doing so makes it something other than pure code movement, but it's
minor and probably a worthy cleanup.)

> +   ret = 1;
> +   else {
> +   warning("%s unexpectedly said: '%s'", data->name, buf->buf);
> +   ret = 1;
> +   }
> +   return ret;
> +}
> +
>  static int set_helper_option(struct transport *transport,
>   const char *name, const char *value)
>  {
> @@ -291,20 +313,7 @@ static int set_helper_option(struct transport *transport,
> quote_c_style(value, , NULL, 0);
> strbuf_addch(, '\n');
>
> -   sendline(data, );
> -   if (recvline(data, ))
> -   exit(128);
> -
> -   if (!strcmp(buf.buf, "ok"))
> -   ret = 0;
> -   else if (starts_with(buf.buf, "error")) {
> -   ret = -1;
> -   } else if (!strcmp(buf.buf, "unsupported"))
> -   ret = 1;
> -   else {
> -   warning("%s unexpectedly said: '%s'", data->name, buf.buf);
> -   ret = 1;
> -   }
> +   ret = strbuf_set_helper_option(data, );
> strbuf_release();
> return ret;
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/26] remote-curl.c: convert fetch_git() to use argv_array

[Eric Sunshine]

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/remote-curl.c b/remote-curl.c
> @@ -726,37 +726,30 @@ static int fetch_git(struct discovery *heads,
> char *depth_arg = NULL;
> -   int argc = 0, i, err;
> -   const char *argv[17];
> +   int i, err;
> +   struct argv_array args = ARGV_ARRAY_INIT;
> [...]
> if (options.verbosity >= 3) {
> -   argv[argc++] = "-v";
> -   argv[argc++] = "-v";
> +   argv_array_push(, "-v");
> +   argv_array_push(, "-v");

A bit more natural might have been:

argv_array_pushl(, "-v", "-v", NULL);

though the diff becomes noisier when the braces get dropped. Not worth
a re-roll, of course.

> }
> [...]
> -   if (options.depth) {
> -   struct strbuf buf = STRBUF_INIT;
> -   strbuf_addf(, "--depth=%lu", options.depth);
> -   depth_arg = strbuf_detach(, NULL);
> -   argv[argc++] = depth_arg;
> -   }
> -   argv[argc++] = url.buf;
> -   argv[argc++] = NULL;
> +   argv_array_push(, "--no-progress");
> +   if (options.depth)
> +   argv_array_pushf(, "--depth=%lu", options.depth);

'depth_arg' becomes unused with this change and can be retired...

> +   argv_array_push(, url.buf);
> @@ -779,6 +772,7 @@ static int fetch_git(struct discovery *heads,
> strbuf_release();
> strbuf_release();
> free(depth_arg);

...along with this unnecessary free().

> +   argv_array_clear();
> return err;
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Matthieu Moy]

Samuel GROOT  writes:

> On 05/23/2016 10:00 PM, Matthieu Moy wrote:
>
>> Your --quote-mail does two things:
>>
>> 1) Populate the To and Cc field
>>
>> 2) Include the original message body with quotation prefix.
>>

[...]

>> * If --compose is not given, don't send a cover-letter but cite the body
>>   as comment in the first patch.
>
> Then should the option imply `--annotate`, to let the user edit the
> patch containing the quoted email?

Actually, I'm not sure what the ideal behavior should be. Perhaps it's
better to distinguish 1) and 2) above, and have two options
--reply-to-email= doing 1), and --quote doing 2), implying
--compose and requiring --reply-to-email.

That would be more flexible, but that would require two new options, and
I also like to keep things simple.

In any case, quoting the message without replying to it does not make
sense (especially if you add instructions to trim it: the user would not
even see it). So it its current form, I'd say --quote-email should imply
--annotate.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html