Re: Implementing reftable in Git

[Carlos Martín Nieto]

On Wed, 2018-05-09 at 10:54 -0700, Jonathan Nieder wrote:
> Carlos Martín Nieto wrote:
> > On Wed, 2018-05-09 at 09:48 -0700, Jonathan Nieder wrote:
> > > If you would like the patches at https://git.eclipse.org/r/q/topi
> > > c:reftable
> > > relicensed for Git's use so that you don't need to include that
> > > license header, let me know.  Separate from any legal concerns,
> > > if
> > > you're doing a straight port, a one-line comment crediting the
> > > JGit
> > > project would still be appreciated, of course.
> 
> [...]
> > Would you expect that this port would keep the Eclipse Distribution
> > License or would it get relicensed to GPLv2?
> 
> I think you're way overcomplicating things.
> 
> The patches are copyright Google.  We can handle issues as they come.

Fair enough. I just wanted to avoid coming back to this in a few months
and realising we can't use it at all.

Cheers,
   cmn

Re: Implementing reftable in Git

[Carlos Martín Nieto]

Hi all,

On Wed, 2018-05-09 at 09:48 -0700, Jonathan Nieder wrote:
> Hi,
> 
> Christian Couder wrote:
> 
> > I might start working on implementing reftable in Git soon.
> 
> Yay!
> 
> [...]
> > So I think the most straightforward and compatible way to do it would
> > be to port the JGit implementation.
> 
> I suspect following the spec[1] would be even more compatible, since it
> would force us to tighten the spec where it is unclear.
> 
> >It looks like the
> > JGit repo and the reftable code there are licensed under the Eclipse
> > Distribution License - v 1.0 [7] which is very similar to the 3-Clause
> > BSD License also called Modified BSD License
> 
> If you would like the patches at https://git.eclipse.org/r/q/topic:reftable
> relicensed for Git's use so that you don't need to include that
> license header, let me know.  Separate from any legal concerns, if
> you're doing a straight port, a one-line comment crediting the JGit
> project would still be appreciated, of course.
> 
> That said, I would not be surprised if going straight from the spec is
> easier than porting the code.

Would you expect that this port would keep the Eclipse Distribution
License or would it get relicensed to GPLv2?

We would also want to have reftable functionality in the libgit2
project, but it has a slightly different license from git (GPLv2 with
linking exception) which requires explicit consent from the authors for
us to port over the code from git with its GPLv2 license.

The libgit2 project does have permission from Shawn to relicense his
git code, but this would presumably not cover this kind of porting. I
don't believe we would have issues if the code remained this BSD-like
license.

Sorry for being difficult, but fewer distinct reimplementations is
probably a good thing overall.

cc the core libgit2 team

Cheers,
   cmn

[PATCH] diff: --indent-heuristic is no longer experimental

[Carlos Martín Nieto]

This heuristic has been the default since 2.14 so we should not confuse our
users by saying that it's experimental and off by default.

Signed-off-by: Carlos Martín Nieto <c...@dwim.me>
---
 Documentation/diff-heuristic-options.txt | 5 -
 Documentation/diff-options.txt   | 7 ++-
 2 files changed, 6 insertions(+), 6 deletions(-)
 delete mode 100644 Documentation/diff-heuristic-options.txt

diff --git a/Documentation/diff-heuristic-options.txt 
b/Documentation/diff-heuristic-options.txt
deleted file mode 100644
index d4f3d95505..00
--- a/Documentation/diff-heuristic-options.txt
+++ /dev/null
@@ -1,5 +0,0 @@
---indent-heuristic::
---no-indent-heuristic::
-   These are to help debugging and tuning experimental heuristics
-   (which are off by default) that shift diff hunk boundaries to
-   make patches easier to read.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index a88c76741e..dd0dba5b1d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -63,7 +63,12 @@ ifndef::git-format-patch[]
Synonym for `-p --raw`.
 endif::git-format-patch[]
 
-include::diff-heuristic-options.txt[]
+--indent-heuristic::
+   Enable the heuristic that shift diff hunk boundaries to make patches
+   easier to read. This is the default.
+
+--no-indent-heuristic::
+   Disable the indent heuristic.
 
 --minimal::
Spend extra time to make sure the smallest possible
-- 
2.15.0.rc2

Re: [RFC PATCH] gpg: add support for gpgsm

[Carlos Martín Nieto]

On Thu, 2016-03-31 at 08:46 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <c...@dwim.me> writes:
> 
> > 
> > Detect the gpgsm block header and run this command instead of gpg.
> > On the signing side, ask gpgsm if it knows the signing key we're
> > trying
> > to use and fall back to gpg if it does not.
> > 
> > This lets the user more easily combine signing and verifying X509
> > and
> > PGP signatures without having to choose a default for a particular
> > repository that may need to be occasionally overridden.
> > 
> > Signed-off-by: Carlos Martín Nieto <c...@dwim.me>
> > 
> > ---
> > 
> > Out there in the so-called "real world", companies like using X509
> > to
> > sign things. Currently you can set 'gpg.program' to gpgsm to get
> > gpg-compatible verification,...
> I notice that you had to add GPGSM_MESSAGE string constant; does the
> current code without any change really work correctly if you set
> 'gpg.program' to gpgsm and do nothing else?

It does work for verify-commit which is what I've been playing around
with since it just sends the contents of the 'gpgsig' header field to
the verification function.

I don't recall testing with verify-tag but there we might indeed have
issues, since we parse the contents to see if we have the signature.

> 
> > 
> > ... but if you're changing it to swap between
> > PGP and X509, it's an extra variable to keep in mind when working
> > with
> > signed commits and tags.
> > 
> > +gpgsm.program::
> > +   Use this custom program instead of "gpgsm" found on $PATH
> > when
> > +   making or verifying a gpsm signature. The program must
> > support the
> gpsm signature, or gpgsm signature?


Nice catch. Thanks.

  cmn

--
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] gpg: add support for gpgsm

[Carlos Martín Nieto]

On Thu, 2016-03-31 at 10:22 -0400, Jeff King wrote:
> On Thu, Mar 31, 2016 at 03:51:44PM +0200, Carlos Martín Nieto wrote:
> 
> > 
> > Detect the gpgsm block header and run this command instead of gpg.
> This part makes sense to me, and is a strict improvement (though
> offhand, I wonder if any other systems use the generic "BEGIN SIGNED
> MESSAGE" header. The obvious option would be PEM from "openssl
> smime",
> but it is "BEGIN PKCS7").

Yep. It's useful when you can tell what generated the signatures.

> 
> > 
> > On the signing side, ask gpgsm if it knows the signing key we're
> > trying
> > to use and fall back to gpg if it does not.
> This part looks like we incur an extra fork/exec each time we sign
> with
> gpg, even if the user doesn't ever want to use gpgsm, or even have it
> installed.

Yes, this is unfortunate as I don't know any other way to tell whether
gpgsm (or whatever else) knows about a key. We could try to find gpgsm
in PATH but I suspect this would be expensive as well.

> 
> I wonder if there are any hints we can use from the key ident, but I
> suppose not. In the default config, it comes straight from
> $GIT_COMMITTER_*, and is just a name/email.

Both gpg and gpgsm accept any string and try to match it against the
information in the keys. So even though gpgsm keys are shown by the
program itself with the "0x" prefix, 'gpgsm -k DigiCert' does show me
the public key I have for them.

> 
> But maybe we could pull this out to a separate config option, like
> "commit.defaultSignatureType", which could be either "gpg", "gpgsm",
> or
> "auto" to enable the behavior you have here.  Then savvy users can
> pick
> the type they plan to use.  We can have a discussion then about
> whether
> to flip the default from "gpg" to "auto", but I'd vote to leave it at
> gpg unless gpgsm gets a huge amount of traction, and it really is
> 50/50
> what people would want.
> 
> And regardless of the default type for creating signatures, we'd
> still
> automatically verify signatures from either type.

This makes sense. It allows for the automatic detection if that's
wanted but it would stop us running gpgsm on each invocation.



> >  /*
> > + * Try to figure out if the given program contains given the key.
> > Both
> > + * gpg and gpgsm have keys in hex format, so we don't necessarily
> > know
> > + * which one to use.
> > + */
> > +static int program_knows_key(const char *program, const char
> > *signing_key)
> > +{
> > +   struct child_process gpg = CHILD_PROCESS_INIT;
> > +   struct strbuf output = STRBUF_INIT;
> > +   const char *args[4];
> > +   size_t len;
> > +
> > +   gpg.argv = args;
> > +   gpg.in = -1;
> > +   gpg.out = -1;
> > +   args[0] = program;
> > +   args[1] = "-K";
> > +   args[2] = signing_key;
> > +   args[3] = NULL;
> I think you'd want to send stderr to /dev/null here, as this is for
> speculatively seeing "does the user even have gpgsm set up?".
> 
> > 
> > +
> > +   if (start_command())
> > +   return error(_("could not run '%s'"), program);
> Likewise, most users would start seeing "could not run 'gpgsm'" if
> they
> do not even have it installed.

Ah yes, I completely forgot to take that into account.

   cmn

--
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 in git diff-index

[Carlos Martín Nieto]

On Thu, 2016-03-31 at 12:39 +, Andy Lowry wrote:
> Following transcript illustrates what I believe to be a bug in git
> diff-
> index. The session used a git built from latest source, located in 
> /tmp/git/git.
> 
> 1. New repo, create empty file A, commit changes.
> 2. touch A
> 3. git diff-index reports A has changed, and reports bogus
> destination 
> SHA
> 4. This is stable behavior until next step

This is expected and matches the documentation. See the bit starting
with

    OPERATING MODES
           You can choose whether you want to trust the index file entirely 
(using the --cached flag) or ask the diff
           logic to show any files that don’t match the stat state as being 
"tentatively changed". Both of these
   operations are very useful indeed.

The next two sections describe what you are seeing. The default is non-
cached mode which also shows files which don't match the stat data in
the index (which you've changed by touching the file).

Cheers,
   cmn

--
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] gpg: add support for gpgsm

[Carlos Martín Nieto]

Detect the gpgsm block header and run this command instead of gpg.
On the signing side, ask gpgsm if it knows the signing key we're trying
to use and fall back to gpg if it does not.

This lets the user more easily combine signing and verifying X509 and
PGP signatures without having to choose a default for a particular
repository that may need to be occasionally overridden.

Signed-off-by: Carlos Martín Nieto <c...@dwim.me>

---

Out there in the so-called "real world", companies like using X509 to
sign things. Currently you can set 'gpg.program' to gpgsm to get
gpg-compatible verification, but if you're changing it to swap between
PGP and X509, it's an extra variable to keep in mind when working with
signed commits and tags.

While this does let us sign and verify, the probing is a bit
awkward. gpgsm returns 0 regardless of whether it found the key, and if
you pass in an id for which you have the public key, it'll still output the
filename as a heading, so we would consider it known. I'm not aware of a
way around that which doesn't involve parsing the output, which would
probably be even more fragile.

 Documentation/config.txt | 11 
 gpg-interface.c  | 65 ++--
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..40f3912 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1484,6 +1484,17 @@ gpg.program::
signed, and the program is expected to send the result to its
standard output.
 
+gpgsm.program::
+   Use this custom program instead of "gpgsm" found on $PATH when
+   making or verifying a gpsm signature. The program must support the
+   same command-line interface as GPG, namely, to verify a detached
+   signature, "gpgsm --verify $file - <$signature" is run, and the
+   program is expected to signal a good signature by exiting with
+   code 0, and to generate an ASCII-armored detached signature, the
+   standard input of "gpgsm -bsau $key" is fed with the contents to be
+   signed, and the program is expected to send the result to its
+   standard output.
+
 gui.commitMsgWidth::
Defines how wide the commit message window is in the
linkgit:git-gui[1]. "75" is the default.
diff --git a/gpg-interface.c b/gpg-interface.c
index 3dc2fe3..194a6c6 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -6,10 +6,13 @@
 
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
+static const char *gpgsm_program = "gpgsm";
 
 #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
 #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
 
+#define GPGSM_MESSAGE "-BEGIN SIGNED MESSAGE-"
+
 void signature_check_clear(struct signature_check *sigc)
 {
free(sigc->payload);
@@ -24,6 +27,20 @@ void signature_check_clear(struct signature_check *sigc)
sigc->key = NULL;
 }
 
+/*
+ * Guess which program this signature was made with based on the block start.
+ * Right now we just detect a gpgsm block and fall back to gpg otherwise.
+ */
+static const char *guess_program(const char *message, size_t message_len)
+{
+   size_t gpgsm_len = strlen(GPGSM_MESSAGE);
+
+   if (message_len > gpgsm_len && !strncmp(message, GPGSM_MESSAGE, 
gpgsm_len))
+   return gpgsm_program;
+
+   return gpg_program;
+}
+
 static struct {
char result;
const char *check;
@@ -131,6 +148,11 @@ int git_gpg_config(const char *var, const char *value, 
void *cb)
return config_error_nonbool(var);
gpg_program = xstrdup(value);
}
+   if (!strcmp(var, "gpgsm.program")) {
+   if (!value)
+   return config_error_nonbool(var);
+   gpgsm_program = xstrdup(value);
+   }
return 0;
 }
 
@@ -142,6 +164,41 @@ const char *get_signing_key(void)
 }
 
 /*
+ * Try to figure out if the given program contains given the key. Both
+ * gpg and gpgsm have keys in hex format, so we don't necessarily know
+ * which one to use.
+ */
+static int program_knows_key(const char *program, const char *signing_key)
+{
+   struct child_process gpg = CHILD_PROCESS_INIT;
+   struct strbuf output = STRBUF_INIT;
+   const char *args[4];
+   size_t len;
+
+   gpg.argv = args;
+   gpg.in = -1;
+   gpg.out = -1;
+   args[0] = program;
+   args[1] = "-K";
+   args[2] = signing_key;
+   args[3] = NULL;
+
+   if (start_command())
+   return error(_("could not run '%s'"), program);
+
+   close(gpg.in);
+   len = strbuf_read(, gpg.out, 1024);
+   close(gpg.out);
+
+   /* If the command exits with an error, consider it as not found */
+   if (finish_command(

Re: [PATCH] Disown ssh+git and git+ssh

[Carlos Martín Nieto]

On Wed, 2016-03-09 at 13:56 -0800, Junio C Hamano wrote:
> Eric Sunshine <sunsh...@sunshineco.com> writes:
> 
> > 
> > It might be helpful to cite some reference to support the claim
> > that
> > they are "silly" since it's not necessarily obvious to readers who
> > did
> > not following the discussion.
> > ...
> > > 
> > > || starts_with(url, "ssh://")
> > > +   /*
> > > +* These ways to spell the ssh transport remain
> > > supported for
> > > +* compat but are undocumented and their use is
> > > discouraged
> > > +*/
> > > || starts_with(url, "git+ssh://")
> > > || starts_with(url, "ssh+git://")) {
> > A little "comment" bikeshedding: Aside from undesirably
> > interrupting
> > the code flow, these large comment blocks draw far too much
> > attention
> > from the reader than these deprecated spellings of "ssh" deserve,
> > thus
> > making them seem overly important.
> I've been waiting for an update for it but got tired of it.
> Instead of discarding the topic, let's amend it like so:

Sorry, I missed the call for the rewording. The below looks good to me.
Thanks.

> 
> -- >8 --
> From: Carlos Martín Nieto <c...@dwim.me>
> Date: Mon, 15 Feb 2016 15:29:06 +0100
> Subject: [PATCH] Disown ssh+git and git+ssh
> 
> Some people argue that these were silly from the beginning (see
> http://thread.gmane.org/gmane.comp.version-control.git/285590/focus=2
> 85601
> for example), but we have to support them for compatibility.
> 
> That doesn't mean we have to show them in the documentation.  These
> were already left out of the main list, but a reference in the main
> manpage was left, so remove that.
> 
> Also add a note to discourage their use if anybody goes looking for
> them
> in the source code.
> 
> Signed-off-by: Carlos Martín Nieto <c...@dwim.me>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
>  Documentation/git.txt | 2 +-
>  connect.c | 4 ++--
>  transport.c   | 5 +++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index d987ad2..2f90635 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1122,7 +1122,7 @@ of clones and fetches.
>   connection (or proxy, if configured)
>  
>     - `ssh`: git over ssh (including `host:path` syntax,
> - `git+ssh://`, etc).
> + `ssh://`, etc).
>  
>     - `rsync`: git over rsync
>  
> diff --git a/connect.c b/connect.c
> index fd7ffe1..3babb81 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -267,9 +267,9 @@ static enum protocol get_protocol(const char
> *name)
>   return PROTO_SSH;
>   if (!strcmp(name, "git"))
>   return PROTO_GIT;
> - if (!strcmp(name, "git+ssh"))
> + if (!strcmp(name, "git+ssh")) /* deprecated - do not use */
>   return PROTO_SSH;
> - if (!strcmp(name, "ssh+git"))
> + if (!strcmp(name, "ssh+git")) /* deprecated - do not use */
>   return PROTO_SSH;
>   if (!strcmp(name, "file"))
>   return PROTO_FILE;
> diff --git a/transport.c b/transport.c
> index 67f3666..908e08b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1001,8 +1001,9 @@ struct transport *transport_get(struct remote
> *remote, const char *url)
>   || starts_with(url, "file://")
>   || starts_with(url, "git://")
>   || starts_with(url, "ssh://")
> - || starts_with(url, "git+ssh://")
> - || starts_with(url, "ssh+git://")) {
> + || starts_with(url, "git+ssh://") /* deprecated - do
> not use */
> + || starts_with(url, "ssh+git://") /* deprecated - do
> not use */
> + ) {
>   /*
>    * These are builtin smart transports; "allowed"
> transports
>    * will be checked individually in git_connect.

--
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: Gsoc 16

[Carlos Martín Nieto]

On Tue, 2016-03-15 at 14:33 -0700, Stefan Beller wrote:
> On Tue, Mar 15, 2016 at 2:23 PM, Pranit Bauva  > wrote:
> > 
> > Hey,
> > 
> > Open Source projects run because of people who contribute in their
> > free time (mainly). It might not be possible for someone to be
> > active
> > all times Sometimes it may take around 2-3 days. Give it a little
> > more
> > time.
> Adding to that: Not everybody reads the whole list.
> The first mail you sent, was deep down in the thread for rewriting
> rebase --interactive in C instead of shell. The libgit2 maintainer
> (Carlos)
> is highly unlikely to read that as it doesn't affect libgit2.
> 
> Posting as a separate thread here increases the chance for him to
> spot this
> message.
> 
> Usually you'd cc the people you're addressing as that has highest
> chance that
> the relevant people see your message. (I cc'd Carlos in this message,
> no need
> to resend a third time from you.)

Thanks for the CC. Indeed I generally only look at the git list
sporadically and superficially since most discussions are about the
tool part of git, which doesn't affect the underlying system.

Saurabh, libgit2 development happens on the GitHub repository, so if
you want to contribute, open a pull request there and we'll review the
contributions when we can.

Cheers,
   cmn
--
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: GSoC 2016: applications open, libgit2 and git.git

[Carlos Martín Nieto]

On Fri, 2016-02-19 at 09:06 +0100, Matthieu Moy wrote:
> Carlos Martín Nieto <c...@dwim.me> writes:
> 
> > We still have most of the same things open as for the 2014 list.
> > I'll
> > ask around to see if we have. Last year I wasn't involved in the
> > candidate selection but IIRC we didn't do a project as none of the
> > applications showed the candidates would be capable of doing the
> > project they were applying for.
> 
> OK. It's essentially too late to change this for this year, but next
> time we should discuss earlier about how we want to organize this
> git.git/libgit2 thing. For example, I think it would make little
> sense
> to have a git.git microproject and then apply for a libgit2 project
> since we have very different ways of interacting. And honnestly,
> right
> now the application is really git.git-centric so I don't think it
> attracts students towards libgit2. So, if you want to attract more
> students, we should work on that.
> 
> I tried to clarify the situation with libgit2:
> 
> https://github.com/git/git.github.io/commit/94d1747eb9621b3bc892be2f2
> 32338b7933ac271
> 
> Please say if you're happy/unhappy with what I wrote. PRs are still
> welcome after the deadline.

This is fine. Our projects file should also be noted for the
microprojects, but I'll write that up myself. I'm writing up the two
projects we've thought up as part of the ideas page and will send a PR
today.

   cmn

--
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: GSoC 2016: applications open, deadline = Fri, 19/2

[Carlos Martín Nieto]

On Wed, 2016-02-17 at 13:33 -0800, Junio C Hamano wrote:
> Jeff King  writes:
> 
> > On Wed, Feb 17, 2016 at 09:21:20PM +0100, Matthieu Moy wrote:
> > 
> > > > I am wondering if we heard from libgit2 folks if they want us
> > > > to
> > > > host them (or they want to participate in GSoC at all).
> > > 
> > > The libgit2 mention is left from previous versions of this page.
> > > I left
> > > a message on their IRC channel asking to join this thread if
> > > people were
> > > interested (I don't know the libgit2 community really well, and I
> > > didn't
> > > find a mailing-list to Cc here). 
> > > 
> > > I did not hear anything from them. We should probably remove the
> > > mention
> > > of libgit2. Or, if anyone receiving this message is interested in
> > > having
> > > libgit2 participate, or knows anyone who may be, speak now.
> > 
> > I think they do a lot of their communication via GitHub issues.
> > I've
> > cc'd Carlos, the maintainer, who can ping the rest of the community
> > as
> > appropriate.
> > 
> > I don't think we did a libgit2 project last year, and included the
> > libgit2 references mainly so that we would not drop them with zero
> > warning.
> 
> Understandable.  I do not mind seeing us hosting them if that is
> what they want, but the candidate selection and mentor assignment
> between two more-or-less independent projects would not work very
> well unless there is _some_ degree of coordination ;-)

We still have most of the same things open as for the 2014 list. I'll
ask around to see if we have. Last year I wasn't involved in the
candidate selection but IIRC we didn't do a project as none of the
applications showed the candidates would be capable of doing the
project they were applying for.

I'll ask around to make sure people would be able to be mentors, but I
think that we would still like to put forward a few projects (I can
send a PR with the projects that we would still like to see to the 2016
page).

Cheers,
   cmn

--
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] Disown ssh+git and git+ssh

[Carlos Martín Nieto]

These were silly from the beginning, but we have to support them for
compatibility. That doesn't mean we have to show them in the
documentation. These were already left out of the main list, but a
reference in the main manpage was left, so remove that.

Also add a note to discourage their use if anybody goes looking for them
in the source code.

Signed-off-by: Carlos Martín Nieto <c...@dwim.me>
---

I've updated the wording, so we talk about different ways of spelling
ssh rather than talking about schemes.

 Documentation/git.txt | 2 +-
 connect.c | 4 
 transport.c   | 4 
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d987ad2..2f90635 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1122,7 +1122,7 @@ of clones and fetches.
connection (or proxy, if configured)
 
  - `ssh`: git over ssh (including `host:path` syntax,
-   `git+ssh://`, etc).
+   `ssh://`, etc).
 
  - `rsync`: git over rsync
 
diff --git a/connect.c b/connect.c
index fd7ffe1..d3eaa0e 100644
--- a/connect.c
+++ b/connect.c
@@ -267,6 +267,10 @@ static enum protocol get_protocol(const char *name)
return PROTO_SSH;
if (!strcmp(name, "git"))
return PROTO_GIT;
+   /*
+* These ways to spell the ssh transport remain supported for
+* compat but are undocumented and their use is discouraged
+*/
if (!strcmp(name, "git+ssh"))
return PROTO_SSH;
if (!strcmp(name, "ssh+git"))
diff --git a/transport.c b/transport.c
index 9ae7184..ed61e72 100644
--- a/transport.c
+++ b/transport.c
@@ -1002,6 +1002,10 @@ struct transport *transport_get(struct remote *remote, 
const char *url)
|| starts_with(url, "file://")
|| starts_with(url, "git://")
|| starts_with(url, "ssh://")
+   /*
+* These ways to spell the ssh transport remain supported for
+* compat but are undocumented and their use is discouraged
+*/
|| starts_with(url, "git+ssh://")
|| starts_with(url, "ssh+git://")) {
/*
-- 
2.7.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] Disown ssh+git and git+ssh

[Carlos Martín Nieto]

These were silly from the beginning, but we have to support them for
compatibility. That doesn't mean we have to show them in the
documentation. These were already left out of the main list, but a
reference in the main manpage was left, so remove that.

Also add a note to discourage their use if anybody goes looking for them
in the source code.
---
 Documentation/git.txt | 2 +-
 connect.c | 4 
 transport.c   | 4 
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d987ad2..2f90635 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1122,7 +1122,7 @@ of clones and fetches.
connection (or proxy, if configured)
 
  - `ssh`: git over ssh (including `host:path` syntax,
-   `git+ssh://`, etc).
+   `ssh://`, etc).
 
  - `rsync`: git over rsync
 
diff --git a/connect.c b/connect.c
index fd7ffe1..4f96424 100644
--- a/connect.c
+++ b/connect.c
@@ -267,6 +267,10 @@ static enum protocol get_protocol(const char *name)
return PROTO_SSH;
if (!strcmp(name, "git"))
return PROTO_GIT;
+   /*
+* These ssh schemes remain supported for compat but are
+* undocumented and their use is discouraged
+*/
if (!strcmp(name, "git+ssh"))
return PROTO_SSH;
if (!strcmp(name, "ssh+git"))
diff --git a/transport.c b/transport.c
index 9ae7184..f5ae707 100644
--- a/transport.c
+++ b/transport.c
@@ -1002,6 +1002,10 @@ struct transport *transport_get(struct remote *remote, 
const char *url)
|| starts_with(url, "file://")
|| starts_with(url, "git://")
|| starts_with(url, "ssh://")
+   /*
+* These ssh schemes remain supported for compat but are
+* undocumented and their use is discouraged
+*/
|| starts_with(url, "git+ssh://")
|| starts_with(url, "ssh+git://")) {
/*
-- 
2.7.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

Clarification on the git+ssh and ssh+git schemes

[Carlos Martín Nieto]

Hello gits,

git supports using git+ssh:// and ssh+git:// instead of ssh:// or the 
rsync-style format. The first two are however not documented in the git-clone 
manage as acceptable protocols (which is what I think of as the canonical 
source for what you can use). There are tests to make sure these are supported, 
but even the commit that allows for this (c05186cc; Support git+ssh:// and 
ssh+git:// URL) makes it pretty clear it’s not something that’s considered 
sensible.

But in either case, if we’re going to support it, it should be documented. If 
we don’t want to support it, then we should delete the references to these 
formats along with the tests for this.

I’m happy to write a patch going in either direction, but I’d like some input 
from the community as to what we want to do.

Cheers,
   cmn

--
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: Clarification on the git+ssh and ssh+git schemes

[Carlos Martín Nieto]

> On 05 Feb 2016, at 14:11, Junio C Hamano  wrote:
> 
> Linus Torvalds  writes:
> 
>> On Fri, Feb 5, 2016 at 11:30 AM, Jeff King  wrote:
>>> 
>>> I suspect they were not really documented because nobody wanted to
>>> encourage their use. I don't think it would be wrong to document that
>>> they exist and are deprecated, though.
>> 
>> They exist because some people seemed to think that people shouldn't
>> use "ssh://" since they thought that only ssh should use that.
>> 
>> Which is obviously bullshit, since by that logic all the other formats
>> should have that idiotic "git+" format too ("git+https", anybody?). It
>> doesn't actually help anything, and it only pushes somebodys broken
>> agenda.
>> 
>> So there was a push for that silly thing by a couple of people, but it
>> was always wrong. Don't even document it.
> 
> […]
> 
>> Leave it in the source code as an option, and maybe add a comment
>> about "This is stupid, but we support it for hysterical raisins".
> 
> Sounds good.

OK then, let’s remove the reference from the manpage.

As peff guessed, this query is indeed triggered by having to make a decision 
about whether libgit2 should support them. I suppose we’ll have to go in a 
similar direction. Support them because people are using them (which is why a 
user brought it up) but leave a comment that we don’t like it.

Cheers,
   can--
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: branch --set-upstream-to unexpectedly fails with "starting point ... is no branch"

[Carlos Martín Nieto]

On 23 Nov 2015, at 19:59, Marc Strapetz <marc.strap...@syntevo.com> wrote:

> On 23.11.2015 18:04, Carlos Martín Nieto wrote:
>> Hello Mark,
>> 
>> On 23 Nov 2015, at 12:04, Marc Strapetz <marc.strap...@syntevo.com> wrote:
>> 
>>> There is a strange "branch --set-upstream-to" failure for "clones" which 
>>> haven't been created using "git clone" but constructed using "git init", 
>>> "git remote add" and "git fetch".
>>> 
>>> Following script first creates a "main" repository and then constructs the 
>>> clone. Finally, in the clone branches origin/1 and origin/2 will be 
>>> present, however it's not possible to invoke "git branch --set-upstream-to" 
>>> for origin/2 (it works fine for origin/1).
>>> 
>>> I guess the behavior is related to following line in .git/config:
>>> 
>>> fetch = refs/heads/1:refs/remotes/origin/1
>>> 
>>> However, I don't understand what's the problem for Git here? Definitely the 
>>> error "starting point 'origin/2' is not a branch" is wrong.
>>> 
>> 
>> That is indeed the issue. The configuration which is stored in the 
>> configuration is a remote+branch pair. If there is no fetch refspec 
>> configured which would create the ‘origin/2’ remote-tracking branch, the 
>> command does not know which remote and branch that would correspond to.
> 
> Thanks, Carlos, I understand now.
> 
> My goal is to have a clone which will only fetch specific branches, so I 
> guess I have to stick with "refs/heads/1:refs/remotes/origin/1" for the 
> beginning and for every new branch X add another 
> "refs/heads/X:refs/remotes/origin/X"? Or is there a better way?

If you want fine-grained control over what gets downloaded, you’ll need to 
restrict either the configured refspecs or the ones which git-fetch gets.

You can configure the individual refspecs so a ‘git fetch’ call will download 
the ones you want, giving you the issue you mention here; or you can configure 
the default refspec, but always pass explicit instructions to git-fetch, like 
‘git fetch refs/heads/1 refs/heads/2’. Newer git versions (past 1.9.3 I think) 
will update the remote-tracking bracnhes when you do it this way.

Both of these are annoying in their own way. The second way might be preferable 
if the fetching is done by a script. But if you absent-mindedly run a lone ‘git 
fetch’, then you’ll download all branches.

Cheers,
   cmn

--
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: branch --set-upstream-to unexpectedly fails with "starting point ... is no branch"

[Carlos Martín Nieto]

Hello Mark,

On 23 Nov 2015, at 12:04, Marc Strapetz  wrote:

> There is a strange "branch --set-upstream-to" failure for "clones" which 
> haven't been created using "git clone" but constructed using "git init", "git 
> remote add" and "git fetch".
> 
> Following script first creates a "main" repository and then constructs the 
> clone. Finally, in the clone branches origin/1 and origin/2 will be present, 
> however it's not possible to invoke "git branch --set-upstream-to" for 
> origin/2 (it works fine for origin/1).
> 
> I guess the behavior is related to following line in .git/config:
> 
> fetch = refs/heads/1:refs/remotes/origin/1
> 
> However, I don't understand what's the problem for Git here? Definitely the 
> error "starting point 'origin/2' is not a branch" is wrong.
> 

That is indeed the issue. The configuration which is stored in the 
configuration is a remote+branch pair. If there is no fetch refspec configured 
which would create the ‘origin/2’ remote-tracking branch, the command does not 
know which remote and branch that would correspond to.

If you had ‘refs/heads/2:refs/remotes/origin/2’ then it would know, but other 
remote-tracking branches (e.g. ‘origin/3’) would still have an unknown source.

The error message is indeed bogus; it’s likely one of the functions assuming 
how it’s going to be used.

   cmn

--
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 v2 40/43] refs: allow ref backend to be set for clone

[Carlos Martín Nieto]

On Tue, 2015-10-06 at 14:09 -0400, David Turner wrote:
> On Mon, 2015-10-05 at 21:58 -0400, Jeff King wrote:
> > On Mon, Oct 05, 2015 at 09:29:37PM -0400, David Turner wrote:
> > 
> > > > Therefore, I don't think this can be merged without a bump to
> > > > core.repositoryformatversion. Such a bump will tell well-
> > > > behaved older
> > > > Git clients keep their hands off the repository. (Of course
> > > > repositories
> > > > that use the files backend can continue using
> > > > core.repositoryformatversion 0.)
> > > > 
> > > > I thought Peff proposed a way to do such a bump, including a
> > > > way to
> > > > extend repositories one by one with new features. But that was
> > > > something
> > > > that we were chatting about off-list.
> > > > 
> > > > I haven't reviewed the actual code in this patch yet but I
> > > > wanted to get
> > > > the above comment on your radar.
> > > > 
> > > > Michael
> > > 
> > > I'll fix this to upgrade to v=1 when the lmdb refs backend is in
> > > use,
> > > and to give sensible error messages in a v1 repo if built without
> > > LMDB.
> > 
> > I think the relevant series is:
> > 
> >   http://article.gmane.org/gmane.comp.version-control.git/272447
> > 
> > It did not seem too controversial, but it mostly got dropped amidst
> > the
> > release, and I haven't reposted it yet.
> 
> That patch will work perfectly for this use case.  I'll add it to my
> series when I reroll, and set an extension.

This is something I'm working on right now for libgit2 as well; not
lmdb specifically but allowing user-provided backends, which would
allow built-in ones as well.

Did we ever decide on the format for these extensions? The series
mentioned above has a couple of examples, but doesn't have any testing
for backend stuff. Do we have a concrete proposal for this? I was going
to go for something like

extensions.refbackend = "lmdb"
extensions.odbbackend = "psql"

and have backends register themselves by the "lmdb", "psql" or whatever
format, but if we have already decided something else which I missed,
I'd swap over to that.

Cheers,
   cmn

--
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: curl

[Carlos Martín Nieto]

On Tue, 2015-04-28 at 00:57 -0400, Jeff King wrote:
 On Mon, Apr 27, 2015 at 11:49:51PM -0300, Thiago Farina wrote:
 
  Is it right that git uses libcurl to download while libgit2 does without it?
 
 I'm not sure if you mean right as in this statement is true or as in
 is this a good thing that it is the case.
 
 For the former, yes, libgit2 does not use curl.  On Windows, it can use
 the native http calls (which do nice things like using the system proxy
 and auth systems). On Unix, I think it is a combination of hand-rolled
 code, openssl, and an imported http parser (from nginx).
 
 Whether that is a good idea or not, I can't comment too much. From what
 I have seen discussed in libgit2 issues, the stock http transport is
 meant to be bare-bones (but with minimal dependencies). But it could
 co-exist with a curl transport (just as it does with the WinHTTP
 transport).  Maybe Carlos (cc'd) can say more.

This is accurate, though I'll add that the development version of
libgit2 now uses SecureTransport instead of OpenSSL on Mac OS X.

But this is just the default. You can replace what libgit2 will use as a
transport if you have special needs. Visual Studio use their own network
code, and the cargo package manager uses libcurl. Eventually libcurl
support will likely be added to mainline libgit2, when we find time.

   cmn


--
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] dir: allow a BOM at the beginning of exclude files

[Carlos Martín Nieto]

On Thu, 2015-04-16 at 17:03 +0200, Johannes Schindelin wrote:
 Hi Carlos,
 
 On 2015-04-16 16:05, Carlos Martín Nieto wrote:
  Some text editors like Notepad or LibreOffice write an UTF-8 BOM in
  order to indicate that the file is Unicode text rather than whatever the
  current locale would indicate.
  
  If someone uses such an editor to edit a gitignore file, we are left
  with those three bytes at the beginning of the file. If we do not skip
  them, we will attempt to match a filename with the BOM as prefix, which
  won't match the files the user is expecting.
  
  ---
  
  If you're wondering how I came up with LibreOffice, I was doing a
  workshop recently and one of the participants was not content with the
  choice of vim or nano, so he opened LibreOffice to edit the gitignore
  file with confusing consequences.
  
  This codepath doesn't go as far as the config code in validating that
  we do not have a partial BOM which would mean there's some invalid
  content, but we don't really have invalid content any other way, as
  we're just dealing with a list of paths in the file.
 
 Yeah, users are entertaining!
 
 I agree that this is a good patch. *Maybe* we would need the same handling in 
 more places, in which case it might make sense to refactor the test into its 
 own function.

Yes, this was my train of thought. If we (discover that) need it in a
third place, then we can unify the test and skip. For two places I
reckoned it was fine if we duplicated a bit.

 
 In any case, though, the Git project requires a [Developer's Certificate of 
 Origin](https://github.com/git/git/blob/v2.3.5/Documentation/SubmittingPatches#L234-L277);
  Would you mind adding that?
 

Yeah, sorry. I keep forgetting to do that. I'll reply to my original
e-mail with it.

Cheers,
   cmn


--
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] dir: allow a BOM at the beginning of exclude files

[Carlos Martín Nieto]

On Thu, 2015-04-16 at 16:05 +0200, Carlos Martín Nieto wrote:
 Some text editors like Notepad or LibreOffice write an UTF-8 BOM in
 order to indicate that the file is Unicode text rather than whatever the
 current locale would indicate.
 
 If someone uses such an editor to edit a gitignore file, we are left
 with those three bytes at the beginning of the file. If we do not skip
 them, we will attempt to match a filename with the BOM as prefix, which
 won't match the files the user is expecting.

Signed-off-by: Carlos Martín Nieto c...@elego.de

which I keep forgetting.

 
 ---
 
 If you're wondering how I came up with LibreOffice, I was doing a
 workshop recently and one of the participants was not content with the
 choice of vim or nano, so he opened LibreOffice to edit the gitignore
 file with confusing consequences.
 
 This codepath doesn't go as far as the config code in validating that
 we do not have a partial BOM which would mean there's some invalid
 content, but we don't really have invalid content any other way, as
 we're just dealing with a list of paths in the file.
 
  dir.c  | 8 +++-
  t/t7061-wtstatus-ignore.sh | 2 ++
  2 files changed, 9 insertions(+), 1 deletion(-)
 
 diff --git a/dir.c b/dir.c
 index 0943a81..6368247 100644
 --- a/dir.c
 +++ b/dir.c
 @@ -581,6 +581,7 @@ int add_excludes_from_file_to_list(const char *fname,
   struct stat st;
   int fd, i, lineno = 1;
   size_t size = 0;
 + static const unsigned char *utf8_bom = (unsigned char *) \xef\xbb\xbf;
   char *buf, *entry;
  
   fd = open(fname, O_RDONLY);
 @@ -617,7 +618,12 @@ int add_excludes_from_file_to_list(const char *fname,
   }
  
   el-filebuf = buf;
 - entry = buf;
 +
 + if (size = 3  !memcmp(buf, utf8_bom, 3))
 + entry = buf + 3;
 + else
 + entry = buf;
 +
   for (i = 0; i  size; i++) {
   if (buf[i] == '\n') {
   if (entry != buf + i  entry[0] != '#') {
 diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
 index 460789b..0a06fbf 100755
 --- a/t/t7061-wtstatus-ignore.sh
 +++ b/t/t7061-wtstatus-ignore.sh
 @@ -13,6 +13,8 @@ EOF
  
  test_expect_success 'status untracked directory with --ignored' '
   echo ignored .gitignore 
 + sed -e s/^/\xef\xbb\xbf/ .gitignore .gitignore.new 
 + mv .gitignore.new .gitignore 
   mkdir untracked 
   : untracked/ignored 
   : untracked/uncommitted 



--
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] dir: allow a BOM at the beginning of exclude files

[Carlos Martín Nieto]

Some text editors like Notepad or LibreOffice write an UTF-8 BOM in
order to indicate that the file is Unicode text rather than whatever the
current locale would indicate.

If someone uses such an editor to edit a gitignore file, we are left
with those three bytes at the beginning of the file. If we do not skip
them, we will attempt to match a filename with the BOM as prefix, which
won't match the files the user is expecting.

---

If you're wondering how I came up with LibreOffice, I was doing a
workshop recently and one of the participants was not content with the
choice of vim or nano, so he opened LibreOffice to edit the gitignore
file with confusing consequences.

This codepath doesn't go as far as the config code in validating that
we do not have a partial BOM which would mean there's some invalid
content, but we don't really have invalid content any other way, as
we're just dealing with a list of paths in the file.

 dir.c  | 8 +++-
 t/t7061-wtstatus-ignore.sh | 2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 0943a81..6368247 100644
--- a/dir.c
+++ b/dir.c
@@ -581,6 +581,7 @@ int add_excludes_from_file_to_list(const char *fname,
struct stat st;
int fd, i, lineno = 1;
size_t size = 0;
+   static const unsigned char *utf8_bom = (unsigned char *) \xef\xbb\xbf;
char *buf, *entry;
 
fd = open(fname, O_RDONLY);
@@ -617,7 +618,12 @@ int add_excludes_from_file_to_list(const char *fname,
}
 
el-filebuf = buf;
-   entry = buf;
+
+   if (size = 3  !memcmp(buf, utf8_bom, 3))
+   entry = buf + 3;
+   else
+   entry = buf;
+
for (i = 0; i  size; i++) {
if (buf[i] == '\n') {
if (entry != buf + i  entry[0] != '#') {
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 460789b..0a06fbf 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -13,6 +13,8 @@ EOF
 
 test_expect_success 'status untracked directory with --ignored' '
echo ignored .gitignore 
+   sed -e s/^/\xef\xbb\xbf/ .gitignore .gitignore.new 
+   mv .gitignore.new .gitignore 
mkdir untracked 
: untracked/ignored 
: untracked/uncommitted 
-- 
2.0.0.5.gbce14aa

--
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] dir: allow a BOM at the beginning of exclude files

[Carlos Martín Nieto]

On Thu, 2015-04-16 at 10:16 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Thu, Apr 16, 2015 at 08:39:55AM -0700, Junio C Hamano wrote:
 
test_expect_success 'status untracked directory with --ignored' '
echo ignored .gitignore 
   +sed -e s/^/\xef\xbb\xbf/ .gitignore .gitignore.new 
   +mv .gitignore.new .gitignore 
  
  Is this write literal in \xHEX on the replacement side of sed
  substitution potable?  In any case, replacing the above three with
  something like:
  
 printf bomignored\n .gitignore
  
  may be more sensible, no?
 
  I'm not sure about sed, but I agree it is suspect. And note that printf
  with hex codes is not portable, either You have to use octal:
 
printf '\357\273\277ignored\n' .gitignore
 
  Also, as a nit, I'd much rather see this in its own test rather than
  crammed into another test_expect_success. It's much easier to diagnose
  failures if the test description mentions the goal, and it is not tied
  up with testing other parts that might fail.
 
 Yeah, I totally agree.
 
 Carlos, something like this squashed in, perhaps?
 
  t/t7061-wtstatus-ignore.sh | 11 +--
  1 file changed, 9 insertions(+), 2 deletions(-)
 
 diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
 index 0a06fbf..cdc0747 100755
 --- a/t/t7061-wtstatus-ignore.sh
 +++ b/t/t7061-wtstatus-ignore.sh
 @@ -13,8 +13,6 @@ EOF
  
  test_expect_success 'status untracked directory with --ignored' '
   echo ignored .gitignore 
 - sed -e s/^/\xef\xbb\xbf/ .gitignore .gitignore.new 
 - mv .gitignore.new .gitignore 
   mkdir untracked 
   : untracked/ignored 
   : untracked/uncommitted 
 @@ -22,6 +20,15 @@ test_expect_success 'status untracked directory with 
 --ignored' '
   test_cmp expected actual
  '
  
 +test_expect_success 'same with gitignore starting with BOM' '
 + printf \357\273\277ignored\n .gitignore 
 + mkdir -p untracked 
 + : untracked/ignored 
 + : untracked/uncommitted 
 + git status --porcelain --ignored actual 
 + test_cmp expected actual
 +'
 +
  cat expected \EOF
  ?? .gitignore
  ?? actual
 

Yeah, that makes sense. I had something similar in my patch at one point
before going with modifying the current one.

   cmn


--
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: Returning error message from custom smart http server

[Carlos Martín Nieto]

On Mon, 2014-05-19 at 18:12 +1000, Bryan Turner wrote:
 On Sat, May 17, 2014 at 9:01 AM, brian m. carlson
 sand...@crustytoothpaste.net wrote:
  On Tue, May 13, 2014 at 09:39:59AM +0200, Ákos, Tajti wrote:
  Dear List,
 
  we implemented our own git smart http server to be able to check 
  permissions
  and other thing before pushes. It works fine, however, the error messages 
  we
  generate on the server side are not displayed by the command line client. 
  On
  the server we generate error messages like this:
 
  response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
  response.getWriter().write(msg);
 
  On the command line we get this:
 
  Total 0 (delta 0), reused 0 (delta 0)
  POST git-receive-pack (290 bytes)
  efrror: RPC failed; result=22, HTTP code = 401
  atal: The remote end hung up unexpectedly
  fatal: The remote end hung up unexpectedly
 
  The server message is completely missing. Is there a solution for this?
 
 You should not need a patched git; the wire protocol itself has a
 mechanism for sending smart error messages. It's not particularly
 _obvious_, but it's there.
 
 For starters, to return an error message, your status must be 200 OK.
 You can't return any other status code or Git will interpret your
 error as some form of _HTTP_ error rather than a _git_ error.
 
 In the smart protocol the client sends a service to the server as a
 query parameter, like ?service=git-receive-pack. For such a request,
 you need to:
 - Set the content type to application/x-service-advertisement
 (e.g. application/x-git-receive-pack-advertisement) (Not all command
 line Git versions require this, but JGit does)
 - Set the status code as 200 OK
 - Write back a payload where the first 4 bytes are the hex-encoded
 length of the text (where  is max length for a single packet).
 Note that the 4 bytes for the size are _part_ of that length, so if
 you're writing Test the length is 8, not 4
 - After the size, you write # service=service (e.g. #
 service=git-receive-pack; note the space after the #) This is the
 metadata. For an error, you don't really have much to say.
 - After that, an empty packet, which is  (four zeros) This
 separates the metadata from the ref advertisement
 - After that you can write your message, beginning with ERR  (note
 the trailing space there). The ERR  tells Git what you're writing
 isn't a ref, it's an error. I'd recommend appending a newline (and add
 1 more to your length for it), because when Git echoes your error
 message it doesn't seem to do that
 
 I'm not sure whether there's a document that describes all of this; I
 found it by digging into the Git source code (you can find the ERR
 handling in connect.c, get_remote_heads). This may be exploiting the
 protocol, I'll leave that to someone more knowledgeable on how they
 _intended_ this all to be used, but it works for us.
 
 A full example looks something like this: 0036#
 service=git-receive-packERR This is a test\n

This is indeed documented, namely in 

Documentation/technical/pack-protocol.txt

I guess it could do with an example, but your usage seems correct. There
are two different places where things could go wrong, either in HTTP,
such as authentication, or in the Git part of the request. If you return
an HTTP 404, then all you're telling the client is that you couldn't
find what it asked for, but that could mean either the
receice-pack/upload-pack program or the repository itself. If something
went wrong at the Git level, whether it's a resource problem in the
server or simply that the repo doesn't exist, then ERR is the right
thing to use.

Particularly, we can't rely on the HTTP 404 response being anything
meaningful, as it could simply be the host's default 404 page, and you
don't want html flying through your terminal.

Cheers,
   cmn



--
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: GIT, libcurl and GSS-Negotiate

[Carlos Martín Nieto]

On Sat, 2014-05-10 at 21:01 +, brian m. carlson wrote:
 On Mon, May 05, 2014 at 12:21:33PM +0200, Ivo Bellin Salarin wrote:
  Well, I'm on Windows.
  using `git version 1.9.2.msysgit.0`.
  
  You can find all the exchanges, recorded with wireshark, of the
  following usecases:
  * git vanilla (not working),
  * VisualStudio2013 with libgit (working)
  * curl (--ntlm, working)
  * curl (--negotiate, not working)
 
 Okay, so what it looks like is that for some reason, the server and
 libcurl refuse to connect with Negotiate authentication.  git uses
 CURLAUTH_ANY, and libcurl picks the best choice: Negotiate.  The
 difference between your setup and mine is that I'm using Negotiate with
 Kerberos 5, and you're using Negotiate with NTLM.
 
 What it looks like is happening is that git is offering Negotiate data,
 and then your server is responding with a 401 Unauthorized.  libgit2
 (presumably using WinHTTP) continues in this case, retrying with a
 longer set of credential containing more data, but git gives up.

While libgit2 does use WinHTTP by default on Windows, Visual Studio
overrides this and uses their own HTTP transport (which makes the .NET
stack to handle it) because of the way the prefer to do things, with
just the one persistent connection to TFS.

But details aside, the code Visual Studio uses to do authentication has
nothing to do with any of the others.

   cmn


--
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 2/2] fetch: handle overlaping refspecs on --prune

[Carlos Martín Nieto]

On Thu, 2014-02-27 at 12:19 -0800, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  From: Carlos Martín Nieto c...@dwim.me
 
  We need to consider that a remote-tracking branch may match more than
  one rhs of a fetch refspec.
 
 Hmph, do we *need* to, really?
 
 Do you mean fetching one ref on the remote side and storing that in
 multiple remote-tracking refs on our side?  What benefit does such
 an arrangement give the user?  When we git fetch $there $that_ref

No, I mean a different kind of overlap, where the right-hand side
matches more refs than appear on the left side. In this particular case,
we would have something like

refs/heads/*:refs/remotes/origin/*
refs/pull/*/head:refs/remotes/origin/pr/*

as fetch refspecs. Going remote - remote-tracking branch is not an
issue, as each remote head only matches one refspec. However, we now
have 'origin/master' and 'origin/pr/5' both of which match the
'refs/remotes/origin/*' pattern. The current behaviour is to stop at the
first match, which would mark it as stale as there is no
'refs/heads/pr/5' branch in the remote.

In lieu of real namespacing support for remotes, this seems like a
reasonable way of coalescing the namespaces in the remote repo. I'll
update the commit message with more exact explanation of what kind of
overlap we're dealing with, as it seems it could do with help. Is there
maybe a better word to describe this setup than overlapping?

 to obtain that single ref, do we update both remote-tracking refs?
 When the user asks git log $that_ref@{upstream}, which one of two
 or more remote-tracking refs should we consult?  Should we report
 an error if these remote-tracking refs that are supposed to track
 the same remote ref not all match?  Does git push $there $that_ref
 to update that remote ref update all of these remote-tracking refs
 on our side?  Should it?
 
 My knee-jerk reaction is that it may not be worth supporting such an
 arrangement as broken (we may even want to diagnose it as an error),
 but assuming we do need to, the approach to solve it, i.e. this...
 

For this (other) situation, where you duplicate refs, the issue we're
dealing with in these patches wouldn't arise. I have argued similarly
against built-in support in libgit2 for this kind of shenanigans, but
apparently there's people who use it, though their motivations remain a
mystery to me. Luckily we can support *that* quite well by just going
through the refspecs one by one and applying the rules (both in git and
libgit2).

   cmn

  In such a case, it is not enough to stop at
  the first match but look at all of the matches in order to determine
  whether a head is stale.
 
 ... sounds sensible.




--
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 1/2] fetch: add a failing test for prunning with overlapping refspecs

[Carlos Martín Nieto]

From: Carlos Martín Nieto c...@dwim.me

When a remote has multiple fetch refspecs and these overlap in the
target namespace, fetch may prune a remote-tracking branch which still
exists in the remote. The test uses a popular form of this, by putting
pull requests as stored in a popular hosting platform alongside real
remote-tracking branches.

The fetch command makes a decision of whether to prune based
on the first matching refspec, which in this case is insufficient, as it
covers the pull request names. This pair of refspecs does work as
expected if the more specific refspec is the first in the list.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---

This setup is used by GitHub for Windows, but nobody has noticed this
break because it puts the PR refspec in the system config, which makes
that one the first. I was alerted to this by someone who had done this
setup manually and thus added the PR refspec after the default one.

 t/t5510-fetch.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 1f0f8e6..4949e3d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps 
other namespaces' '
git rev-parse origin/master
 '
 
+test_expect_failure 'fetch --prune handles overlapping refspecs' '
+   cd $D 
+   git update-ref refs/pull/42/head master 
+   git clone . prune-overlapping 
+   cd prune-overlapping 
+   git config --add remote.origin.fetch 
refs/pull/*/head:refs/remotes/origin/pr/* 
+
+   git fetch --prune origin 
+   git rev-parse origin/master 
+   git rev-parse origin/pr/42 
+
+   git config --unset-all remote.origin.fetch
+   git config remote.origin.fetch 
refs/pull/*/head:refs/remotes/origin/pr/* 
+   git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* 

+
+   git fetch --prune origin 
+   git rev-parse origin/master 
+   git rev-parse origin/pr/42
+'
+
 test_expect_success 'fetch --prune --tags does not delete the remote-tracking 
branches' '
cd $D 
git clone . prune-tags 
-- 
1.9.0.rc3.244.g3497008

--
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] fetch: handle overlaping refspecs on --prune

[Carlos Martín Nieto]

From: Carlos Martín Nieto c...@dwim.me

We need to consider that a remote-tracking branch may match more than
one rhs of a fetch refspec. In such a case, it is not enough to stop at
the first match but look at all of the matches in order to determine
whether a head is stale.

To this goal, introduce a variant of query_refspecs which returns all of
the matching refspecs and loop over those answers to check for
staleness.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---

There is an unfortunate duplication of code here, as
query_refspecs_multiple is mostly query_refspecs but we only care
about the other side of matching refspecs and disregard the 'force'
information which query_refspecs does want.

I thought about putting both together via callbacks and having
query_refspecs stop at the first one, but I'm not sure that it would
make it easier to read or manage.

 remote.c | 52 +++-
 t/t5510-fetch.sh |  2 +-
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 9f1a8aa..26140c7 100644
--- a/remote.c
+++ b/remote.c
@@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const 
char *name,
return ret;
 }
 
+static void query_refspecs_multiple(struct refspec *refs, int ref_count, 
struct refspec *query, struct string_list *results)
+{
+   int i;
+   int find_src = !query-src;
+
+   if (find_src  !query-dst)
+   error(query_refspecs_multiple: need either src or dst);
+
+   for (i = 0; i  ref_count; i++) {
+   struct refspec *refspec = refs[i];
+   const char *key = find_src ? refspec-dst : refspec-src;
+   const char *value = find_src ? refspec-src : refspec-dst;
+   const char *needle = find_src ? query-dst : query-src;
+   char **result = find_src ? query-src : query-dst;
+
+   if (!refspec-dst)
+   continue;
+   if (refspec-pattern) {
+   if (match_name_with_pattern(key, needle, value, 
result)) {
+   string_list_append_nodup(results, *result);
+   }
+   } else if (!strcmp(needle, key)) {
+   string_list_append(results, value);
+   }
+   }
+}
+
 static int query_refspecs(struct refspec *refs, int ref_count, struct refspec 
*query)
 {
int i;
@@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
const unsigned char *sha1, int flags, void *cb_data)
 {
struct stale_heads_info *info = cb_data;
+   struct string_list matches = STRING_LIST_INIT_DUP;
struct refspec query;
+   int i, stale = 1;
memset(query, 0, sizeof(struct refspec));
query.dst = (char *)refname;
 
-   if (query_refspecs(info-refs, info-ref_count, query))
+   query_refspecs_multiple(info-refs, info-ref_count, query, matches);
+   if (matches.nr == 0)
return 0; /* No matches */
 
/*
 * If we did find a suitable refspec and it's not a symref and
 * it's not in the list of refs that currently exist in that
-* remote we consider it to be stale.
+* remote we consider it to be stale. In order to deal with
+* overlapping refspecs, we need to go over all of the
+* matching refs.
 */
-   if (!((flags  REF_ISSYMREF) ||
- string_list_has_string(info-ref_names, query.src))) {
+   if (flags  REF_ISSYMREF)
+   return 0;
+
+   for (i = 0; i  matches.nr; i++) {
+   if (string_list_has_string(info-ref_names, 
matches.items[i].string)) {
+   stale = 0;
+   break;
+   }
+   }
+
+   string_list_clear(matches, 0);
+
+   if (stale) {
struct ref *ref = make_linked_ref(refname, 
info-stale_refs_tail);
hashcpy(ref-new_sha1, sha1);
}
 
-   free(query.src);
return 0;
 }
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 4949e3d..a86f6bc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps 
other namespaces' '
git rev-parse origin/master
 '
 
-test_expect_failure 'fetch --prune handles overlapping refspecs' '
+test_expect_success 'fetch --prune handles overlapping refspecs' '
cd $D 
git update-ref refs/pull/42/head master 
git clone . prune-overlapping 
-- 
1.9.0.rc3.244.g3497008

--
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 2/2] fetch: handle overlaping refspecs on --prune

[Carlos Martín Nieto]

On Thu, 2014-02-27 at 11:21 +0100, Michael Haggerty wrote:
 On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote:
  From: Carlos Martín Nieto c...@dwim.me
  
  We need to consider that a remote-tracking branch may match more than
  one rhs of a fetch refspec. In such a case, it is not enough to stop at
  the first match but look at all of the matches in order to determine
  whether a head is stale.
  
  To this goal, introduce a variant of query_refspecs which returns all of
  the matching refspecs and loop over those answers to check for
  staleness.
  
  Signed-off-by: Carlos Martín Nieto c...@elego.de
  ---
  
  There is an unfortunate duplication of code here, as
  query_refspecs_multiple is mostly query_refspecs but we only care
  about the other side of matching refspecs and disregard the 'force'
  information which query_refspecs does want.
  
  I thought about putting both together via callbacks and having
  query_refspecs stop at the first one, but I'm not sure that it would
  make it easier to read or manage.
  
   remote.c | 52 +++-
   t/t5510-fetch.sh |  2 +-
   2 files changed, 48 insertions(+), 6 deletions(-)
  
  diff --git a/remote.c b/remote.c
  index 9f1a8aa..26140c7 100644
  --- a/remote.c
  +++ b/remote.c
  @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, 
  const char *name,
  return ret;
   }
   
  +static void query_refspecs_multiple(struct refspec *refs, int ref_count, 
  struct refspec *query, struct string_list *results)
  +{
  +   int i;
  +   int find_src = !query-src;
  +
  +   if (find_src  !query-dst)
  +   error(query_refspecs_multiple: need either src or dst);
  +
  +   for (i = 0; i  ref_count; i++) {
  +   struct refspec *refspec = refs[i];
  +   const char *key = find_src ? refspec-dst : refspec-src;
  +   const char *value = find_src ? refspec-src : refspec-dst;
  +   const char *needle = find_src ? query-dst : query-src;
  +   char **result = find_src ? query-src : query-dst;
  +
  +   if (!refspec-dst)
  +   continue;
  +   if (refspec-pattern) {
  +   if (match_name_with_pattern(key, needle, value, 
  result)) {
  +   string_list_append_nodup(results, *result);
  +   }
  +   } else if (!strcmp(needle, key)) {
  +   string_list_append(results, value);
  +   }
  +   }
  +}
  +
   static int query_refspecs(struct refspec *refs, int ref_count, struct 
  refspec *query)
   {
  int i;
  @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
  const unsigned char *sha1, int flags, void *cb_data)
   {
  struct stale_heads_info *info = cb_data;
  +   struct string_list matches = STRING_LIST_INIT_DUP;
  struct refspec query;
  +   int i, stale = 1;
  memset(query, 0, sizeof(struct refspec));
  query.dst = (char *)refname;
   
  -   if (query_refspecs(info-refs, info-ref_count, query))
  +   query_refspecs_multiple(info-refs, info-ref_count, query, matches);
  +   if (matches.nr == 0)
  return 0; /* No matches */
   
  /*
   * If we did find a suitable refspec and it's not a symref and
   * it's not in the list of refs that currently exist in that
  -* remote we consider it to be stale.
  +* remote we consider it to be stale. In order to deal with
  +* overlapping refspecs, we need to go over all of the
  +* matching refs.
   */
  -   if (!((flags  REF_ISSYMREF) ||
  - string_list_has_string(info-ref_names, query.src))) {
  +   if (flags  REF_ISSYMREF)
  +   return 0;
  +
  +   for (i = 0; i  matches.nr; i++) {
  +   if (string_list_has_string(info-ref_names, 
  matches.items[i].string)) {
  +   stale = 0;
  +   break;
  +   }
  +   }
  +
  +   string_list_clear(matches, 0);
  +
  +   if (stale) {
  struct ref *ref = make_linked_ref(refname, 
  info-stale_refs_tail);
  hashcpy(ref-new_sha1, sha1);
  }
   
  -   free(query.src);
  return 0;
   }
 
 I didn't have time to review this fully, but I think you are missing
 calls to string_list_clear(matches) on a couple of code paths.

Yep, you're right. I'll fix this and hold off new version for a bit to
see if there's more input. 

   cmn


--
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/2] thin-pack capability for send-pack/receive-pack

[Carlos Martín Nieto]

On Wed, 2013-11-06 at 15:42 -0800, Shawn Pearce wrote:
 On Wed, Nov 6, 2013 at 1:41 PM, Carlos Martín Nieto c...@elego.de wrote:
  On Wed, 2013-11-06 at 12:32 -0800, Junio C Hamano wrote:
  I'll queue these for now, but I doubt the wisdom of this series,
  given that the ship has already sailed long time ago.
 
  Currently, no third-party implementation of a receiving end can
  accept thin push, because thin push is not a capability that needs
  to be checked by the current clients.  People will have to wait
  until the clients with 2/2 patch are widely deployed before starting
  to use such a receiving end that is incapable of thin push.
 
  Wouldn't the world be a better place if instead they used that time
  waiting to help such a third-party receiving end to implement thin
  push support?
 
 
  Support in the code isn't always enough. The particular case that
  brought this on is one where the index-pack implementation can deal with
  thin packs just fine.
 
  This particular service takes the pack which the client sent and does
  post-processing on it to store it elsewhere. During the receive-pack
  equivalent, there is no git object db that it can query for the missing
  base objects. I realise this is pretty a unusual situation.
 
 How... odd?
 
 At Google we have made effort to ensure servers can accept thin packs,
 even though its clearly easier to accept non-thin, because clients in
 the wild already send thin packs and changing the deployed clients is
 harder than implementing the existing protocol.

It is harder, but IMO also more correct, as thin packs are an
optimisation that was added somewhat later. Not to say it shouldn't be
something you should attempt to do, but it's a trade-off between the
complexity of the communication between the pieces and the potential
amount of extra data you're willing to put up with.

The Google (Code) servers don't just support thin packs, but for
upload-pack, they force it upon the client, which is quite frustrating
as it won't even tell you why it closes the connection but sends a 500
instead, but that's a different story.

 
 If the server can't complete the pack, I guess this also means the
 client cannot immediately fetch from the server it just pushed to?

Not all the details have been worked out yet, but the new history should
be converted into the target format before reporting success and closing
the connection. The Git frontend/protocol is one way of putting data
into the system, but that's not its native data storage format. The
database where this is getting stored only has very limited knowledge of
git.

I'll reroll the series with no-thin as mentioned elsewhere in this
thread.

   cmn


--
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] send-pack: don't send a thin pack to a server which doesn't support it

[Carlos Martín Nieto]

Up to now git has assumed that all servers are able to fix thin
packs. This is however not always the case.

Document the 'no-thin' capability and prevent send-pack from generating
a thin pack if the server advertises it.
---

This is a re-roll of the series I sent earlier this month, switching
it around by adding the no-thin 

 Documentation/technical/protocol-capabilities.txt | 20 +++-
 send-pack.c   |  2 ++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index fd8ffa5..3a75e79 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -72,15 +72,25 @@ interleaved with S-R-Q.
 thin-pack
 -
 
-This capability means that the server can send a 'thin' pack, a pack
-which does not contain base objects; if those base objects are available
-on client side. Client requests 'thin-pack' capability when it
-understands how to thicken it by adding required delta bases making
-it self-contained.
+A thin pack is one with deltas which reference base objects not
+contained within the pack (but are known to exist at the receiving
+end). This can reduce the network traffic significantly, but it
+requires the receiving end to know how to thicken these packs by
+adding the missing bases to the pack.
+
+The upload-pack server advertises 'thin-pack' when it can generate and
+send a thin pack. The receive-pack server advertises 'no-thin' if
+it does not know how to thicken the pack it receives.
+
+A client requests the 'thin-pack' capability when it understands how
+to thicken it.
 
 Client MUST NOT request 'thin-pack' capability if it cannot turn a thin
 pack into a self-contained pack.
 
+Client MUST NOT send a thin pack if the server advertises the
+'no-thin' capability.
+
 
 side-band, side-band-64k
 
diff --git a/send-pack.c b/send-pack.c
index 7d172ef..9877eb9 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,
quiet_supported = 1;
if (server_supports(agent))
agent_supported = 1;
+   if (server_supports(no-thin))
+   args-use_thin_pack = 0;
 
if (!remote_refs) {
fprintf(stderr, No refs in common and none specified; doing 
nothing.\n
-- 
1.8.5.rc3.362.gdf10213

--
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] send-pack: don't send a thin pack to a server which doesn't support it

[Carlos Martín Nieto]

On Sat, 2013-11-23 at 17:07 +0100, Carlos Martín Nieto wrote:
 Up to now git has assumed that all servers are able to fix thin
 packs. This is however not always the case.
 
 Document the 'no-thin' capability and prevent send-pack from generating
 a thin pack if the server advertises it.

Sorry,

Signed-off-by: Carlos Martín Nieto c...@elego.de

 ---
 
 This is a re-roll of the series I sent earlier this month, switching
 it around by adding the no-thin 
 
  Documentation/technical/protocol-capabilities.txt | 20 +++-
  send-pack.c   |  2 ++
  2 files changed, 17 insertions(+), 5 deletions(-)
 
 diff --git a/Documentation/technical/protocol-capabilities.txt 
 b/Documentation/technical/protocol-capabilities.txt
 index fd8ffa5..3a75e79 100644
 --- a/Documentation/technical/protocol-capabilities.txt
 +++ b/Documentation/technical/protocol-capabilities.txt
 @@ -72,15 +72,25 @@ interleaved with S-R-Q.
  thin-pack
  -
  
 -This capability means that the server can send a 'thin' pack, a pack
 -which does not contain base objects; if those base objects are available
 -on client side. Client requests 'thin-pack' capability when it
 -understands how to thicken it by adding required delta bases making
 -it self-contained.
 +A thin pack is one with deltas which reference base objects not
 +contained within the pack (but are known to exist at the receiving
 +end). This can reduce the network traffic significantly, but it
 +requires the receiving end to know how to thicken these packs by
 +adding the missing bases to the pack.
 +
 +The upload-pack server advertises 'thin-pack' when it can generate and
 +send a thin pack. The receive-pack server advertises 'no-thin' if
 +it does not know how to thicken the pack it receives.
 +
 +A client requests the 'thin-pack' capability when it understands how
 +to thicken it.
  
  Client MUST NOT request 'thin-pack' capability if it cannot turn a thin
  pack into a self-contained pack.
  
 +Client MUST NOT send a thin pack if the server advertises the
 +'no-thin' capability.
 +
  
  side-band, side-band-64k
  
 diff --git a/send-pack.c b/send-pack.c
 index 7d172ef..9877eb9 100644
 --- a/send-pack.c
 +++ b/send-pack.c
 @@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,
   quiet_supported = 1;
   if (server_supports(agent))
   agent_supported = 1;
 + if (server_supports(no-thin))
 + args-use_thin_pack = 0;
  
   if (!remote_refs) {
   fprintf(stderr, No refs in common and none specified; doing 
 nothing.\n



--
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 1/2] receive-pack: advertise thin-pack

[Carlos Martín Nieto]

upload-pack has long advertised thin-pack, letting the clients request
these smaller packs. The client however unconditionally assumes that a
server is able to fix thin packs and there is no way of telling the
client that this is in fact not the case.

Make receive-pack advertise 'thin-pack' in anticipation of the client
toggling the assumption and document this capability when used by
receive-pack.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 Documentation/technical/protocol-capabilities.txt | 20 +++-
 builtin/receive-pack.c|  2 +-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index fd8ffa5..4e96d51 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -72,15 +72,25 @@ interleaved with S-R-Q.
 thin-pack
 -
 
-This capability means that the server can send a 'thin' pack, a pack
-which does not contain base objects; if those base objects are available
-on client side. Client requests 'thin-pack' capability when it
-understands how to thicken it by adding required delta bases making
-it self-contained.
+A thin pack is one with deltas which reference base objects not
+contained within the pack (but are known to exist at the receiving
+end). This can reduce the network traffic significantly, but it
+requires the receiving end to know how to thicken these packs by
+adding the missing bases to the pack.
+
+The upload-pack server advertises 'thin-pack' when it can generate and
+send a thin pack. The receive-pack server advertises 'thin-pack' when
+it knows how to thicken the pack it receives.
+
+Likewise, the client requests the 'thin-pack' capability when it
+understands how to thicken it.
 
 Client MUST NOT request 'thin-pack' capability if it cannot turn a thin
 pack into a self-contained pack.
 
+Client MUST NOT send a thin pack if the server does not advertise this
+capability.
+
 
 side-band, side-band-64k
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..0e35c02 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -132,7 +132,7 @@ static void show_ref(const char *path, const unsigned char 
*sha1)
else
packet_write(1, %s %s%c%s%s agent=%s\n,
 sha1_to_hex(sha1), path, 0,
- report-status delete-refs side-band-64k quiet,
+ report-status delete-refs side-band-64k quiet 
thin-pack,
 prefer_ofs_delta ?  ofs-delta : ,
 git_user_agent_sanitized());
sent_capabilities = 1;
-- 
1.8.4.652.g0d6e0ce

--
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] send-pack: only send a thin pack if the server supports it

[Carlos Martín Nieto]

In combination a the previous patch making receive-pack advertise the
thin-pack capability, this allows git to push to a server in a
constrained environment which is not able to fix thin packs while taking
advantage of the feature for servers which can.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 send-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index 7d172ef..7b88ac8 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,
quiet_supported = 1;
if (server_supports(agent))
agent_supported = 1;
+   if (!server_supports(thin-pack))
+   args-use_thin_pack = 0;
 
if (!remote_refs) {
fprintf(stderr, No refs in common and none specified; doing 
nothing.\n
-- 
1.8.4.652.g0d6e0ce

--
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/2] thin-pack capability for send-pack/receive-pack

[Carlos Martín Nieto]

Hi all,

This comes as a result of the discussion starting at [0] about
git-push assuming that a server will always support thin packs. Most
out there in fact do, but this isn't necessarily the case.

Some implementations may not have support for it yet, or the server
might be running in an environment where it is not feasible for it to
try to fill in the missing objects.

Jonathan and Duy mentioned that separate patches for receive-pack and
send-pack could let us work around adding this at such a late stage,
so here they are. The second patch can maybe lie in waiting for a
while.


[0] http://thread.gmane.org/gmane.comp.version-control.git/235766/focus=236402

Carlos Martín Nieto (2):
  receive-pack: advertise thin-pack
  send-pack: only send a thin pack if the server supports it

 Documentation/technical/protocol-capabilities.txt | 20 +++-
 builtin/receive-pack.c|  2 +-
 send-pack.c   |  2 ++
 3 files changed, 18 insertions(+), 6 deletions(-)

-- 
1.8.4.652.g0d6e0ce

--
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/2] thin-pack capability for send-pack/receive-pack

[Carlos Martín Nieto]

On Wed, 2013-11-06 at 12:32 -0800, Junio C Hamano wrote:
 I'll queue these for now, but I doubt the wisdom of this series,
 given that the ship has already sailed long time ago.
 
 Currently, no third-party implementation of a receiving end can
 accept thin push, because thin push is not a capability that needs
 to be checked by the current clients.  People will have to wait
 until the clients with 2/2 patch are widely deployed before starting
 to use such a receiving end that is incapable of thin push.
 
 Wouldn't the world be a better place if instead they used that time
 waiting to help such a third-party receiving end to implement thin
 push support?
 

Support in the code isn't always enough. The particular case that
brought this on is one where the index-pack implementation can deal with
thin packs just fine.

This particular service takes the pack which the client sent and does
post-processing on it to store it elsewhere. During the receive-pack
equivalent, there is no git object db that it can query for the missing
base objects. I realise this is pretty a unusual situation.

Cheers,
   cmn


--
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] send-pack: don't send a thin pack when the server doesn't support it

[Carlos Martín Nieto]

On Tue, 2013-10-08 at 15:22 -0700, Jonathan Nieder wrote:
 Duy Nguyen wrote:
 
  Or maybe it's not that late. How about you go with your patch and add
  thin-pack capability to receive-pack too?
 
  When new git push is used against old server, thin pack is disabled.
  But that's not a big deal (I hope).
 
 Could we have separate patches to introduce the server-side capability
 and then to request it in the client?  That way, people with old
 servers can apply the patch introducing the capability if they want.

That could work.

 
 The new meaning of the thin-pack capability should also be
 documented in Documentation/technical/protocol-capabilities.txt.

Oh, right; the capability as described is only about the server being
able to generate a thin pack. Wouldn't his mean that git shouldn't
assume that *any* remote can fix thin packs, though? (other than most
servers you do talk to happen to).

Anyway, facts on the ground and all that. I'll prepare some 

 
 Done that way and with enough time between the server advertising the
 capability and the client looking for it, it seems like a good idea.


If such patches would be accepted, that would be great. By the time this
all gets merged, we might have thin pack fixing merged into libgit2, but
there will still be uses where fixing them isn't an issue due to other
constraints.


Cheers,
   cmn


--
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] send-pack: don't send a thin pack when the server doesn't support it

[Carlos Martín Nieto]

Not every server out there supports fixing thin packs, so let's send
them a full pack.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---

It's not always possible to support thin packs (sometimes there isn't
even an object database to grab bases out of). And in any case git
shouldn't create thin packs if the server hasn't said it knows how to
fix them, as per the point of the extension.

 send-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index 7d172ef..7b88ac8 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,
quiet_supported = 1;
if (server_supports(agent))
agent_supported = 1;
+   if (!server_supports(thin-pack))
+   args-use_thin_pack = 0;
 
if (!remote_refs) {
fprintf(stderr, No refs in common and none specified; doing 
nothing.\n
-- 
1.8.4.561.g1c3d45d

--
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] remote: filter out invalid remote configurations

[Carlos Martín Nieto]

On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  In remote's configuration callback, anything that looks like
  'remote.name.*' creates a remote 'name'. This remote may not end
  up having any configuration for a remote, but it's still in the list,
  so 'git remote' shows it, which means something like
 
  [remote bogus]
  hocus = pocus
 
  will show a remote 'bogus' in the listing, even though it won't work
  as a remote name for either git-fetch or git-push.
 
 Isn't this something the user may want to be aware of, though?
 Hiding these would rob a chance for such an entry to be noticed from
 the user---is it a good change?

If we want to help the user know that there's something a bit odd in
their configuration, shouldn't we tell them instead of hoping they
stumble upon it? Otherwise IMO it's more confusing if git-remote does
show the remote when git-fetch is interpreting the argument as a path.

   cmn



--
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] remote: filter out invalid remote configurations

[Carlos Martín Nieto]

In remote's configuration callback, anything that looks like
'remote.name.*' creates a remote 'name'. This remote may not end
up having any configuration for a remote, but it's still in the list,
so 'git remote' shows it, which means something like

[remote bogus]
hocus = pocus

will show a remote 'bogus' in the listing, even though it won't work
as a remote name for either git-fetch or git-push.

Filter out the remotes that we created which have no urls in order to
work around such configuration entries.

Signed-off-by: Carlos Martín Nieto c...@elego.de

---

Due to git's callback-based config, it seemed a lot simpler to let it
do it wrong and then filter out what won't be usable, rather than
delaying the creation of a remote until we're sure we do want it.

The tests that made use of a remote 'existing' with just .fetch seem
to be written that way because they can get away with it, rather than
any assertion that it should be allowed in day-to-day git usage, but
correct me if I'm wrong.

 remote.c  | 17 +
 t/t5505-remote.sh |  2 ++
 t/t7201-co.sh |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 68eb99b..00a1d7a 100644
--- a/remote.c
+++ b/remote.c
@@ -141,6 +141,9 @@ static struct remote *make_remote(const char *name, int len)
int i;
 
for (i = 0; i  remotes_nr; i++) {
+   if (!remotes[i])
+   continue;
+
if (len ? (!strncmp(name, remotes[i]-name, len) 
   !remotes[i]-name[len]) :
!strcmp(name, remotes[i]-name))
@@ -469,6 +472,19 @@ static int handle_config(const char *key, const char 
*value, void *cb)
return 0;
 }
 
+static void filter_valid_remotes(void)
+{
+   int i;
+   for (i = 0; i  remotes_nr; i++) {
+   if (!remotes[i])
+   continue;
+
+   /* It's not a remote unless it has at least one url */
+   if (remotes[i]-url_nr == 0  remotes[i]-pushurl_nr == 0)
+   remotes[i] = NULL;
+   }
+}
+
 static void alias_all_urls(void)
 {
int i, j;
@@ -504,6 +520,7 @@ static void read_config(void)
make_branch(head_ref + strlen(refs/heads/), 0);
}
git_config(handle_config, NULL);
+   filter_valid_remotes();
alias_all_urls();
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..848e7b7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -130,9 +130,11 @@ to delete them, use:
 EOF
} 
git tag footag 
+   git remote add oops .
git config --add remote.oops.fetch +refs/*:refs/* 
git remote remove oops 2actual1 
git branch foobranch 
+   git remote add oops .
git config --add remote.oops.fetch +refs/*:refs/* 
git remote rm oops 2actual2 
git branch -d foobranch 
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 0c9ec0a..4647f1c 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -431,7 +431,7 @@ test_expect_success 'detach a symbolic link HEAD' '
 
 test_expect_success \
 'checkout with --track fakes a sensible -b name' '
-git config remote.origin.fetch +refs/heads/*:refs/remotes/origin/* 
+git remote add origin . 
 git update-ref refs/remotes/origin/koala/bear renamer 
 
 git checkout --track origin/koala/bear 
-- 
1.8.4.561.g1c3d45d

--
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] branch: make sure the upstream remote is configured

[Carlos Martín Nieto]

A command of e.g.

git push --set-upstream /tmp/t master

will call install_branch_config() with a remote name of /tmp/t. This
function will set the 'branch.master.remote' key to, which is
nonsensical as there is no remote by that name.

Instead, make sure that the remote given does exist when writing the
configuration and warn if it does not. In order to distinguish named
remotes, introduce REMOTE_NONE as the default origin for remotes,
which the functions reading from the different sources will
overwrite. Thus, an origin of REMOTE_NONE means it has been created at
run-time in order to push to it.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---

It's somewhat surprising that there didn't seem to be a way to
distinguish named remotes from those created from a command-line path,
but I guess nobody needed to.

 branch.c | 11 +++
 remote.h |  1 +
 t/t5523-push-upstream.sh |  5 +
 3 files changed, 17 insertions(+)

diff --git a/branch.c b/branch.c
index c5c6984..cefb8f6 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,7 @@ void install_branch_config(int flag, const char *local, const 
char *origin, cons
int remote_is_branch = !prefixcmp(remote, refs/heads/);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
+   struct remote *r = remote_get(origin);
 
if (remote_is_branch
 !strcmp(local, shortname)
@@ -62,6 +63,16 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
return;
}
 
+   /*
+* Make sure that the remote passed is a configured remote, or
+* we end up setting 'branch.foo.remote = /tmp/t' which is
+* nonsensical.
+*/
+   if (origin  strcmp(origin, .)  r-origin == REMOTE_NONE) {
+   warning(_(there is no remote named '%s', no upstream 
configuration will be set.), origin);
+   return;
+   }
+
strbuf_addf(key, branch.%s.remote, local);
git_config_set(key.buf, origin ? origin : .);
 
diff --git a/remote.h b/remote.h
index cf56724..92f6e33 100644
--- a/remote.h
+++ b/remote.h
@@ -2,6 +2,7 @@
 #define REMOTE_H
 
 enum {
+   REMOTE_NONE,
REMOTE_CONFIG,
REMOTE_REMOTES,
REMOTE_BRANCHES
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 3683df1..e84c2f8 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -71,6 +71,11 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
 '
 
+test_expect_success 'push -u url' '
+git push -u parent HEAD 2err 
+grep no upstream configuration will be set err
+'
+
 test_expect_success TTY 'progress messages go to tty' '
ensure_fresh_upstream 
 
-- 
1.8.3

--
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] branch: make sure the upstream remote is configured

[Carlos Martín Nieto]

On Fri, 2013-07-26 at 14:43 -0400, Jeff King wrote:
 On Fri, Jul 26, 2013 at 07:39:37PM +0200, Carlos Martín Nieto wrote:
 
  A command of e.g.
  
  git push --set-upstream /tmp/t master
  
  will call install_branch_config() with a remote name of /tmp/t. This
  function will set the 'branch.master.remote' key to, which is
  nonsensical as there is no remote by that name.
 
 Is it nonsensical? It does not make sense for the @{upstream} magic
 token, because we will not have a branch in tracking branch refs/remotes

This was the main point, yes; the only time I've seen it used is by
mistake/misunderstanding, and thinking that you wouldn't want to do
something like what's below.

You are also unable to do this kind of thing through git-branch, and as
it seemed to be an oversight, I wanted to tighten it up.

 to point to. But the configuration would still affect how git pull
 chooses a branch to fetch and merge.
 
 I.e., you can currently do:
 
   git push --set-upstream /tmp/t master
   git pull ;# pulls from /tmp/t master

Interestingly, this actually fetches the right branch from the remote. I
wasn't expecting something like this to work at all.

Somewhat doubtful that this usage is something you'd really want to do,
I see that it does behave properly.

   cmn


--
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: Basic git-archive --remote question

[Carlos Martín Nieto]

On Mon, 2013-06-24 at 15:53 -0400, Greg Freemyer wrote:
 I'm trying to create a tarball from a git tag and I can't get the
 syntax right.  The documentation is not very clear.
 
 Can someone help me?
 
 == details
 
 git v1.8.1.4
 
 The upstream git repo is at: https://github.com/dkovar/analyzeMFT
 
 Here's a few attempts using git as the protocol:
 
  git archive --format=tar --remote=github.com:dkovar/analyzeMFT.git v2.0.4
 
 Permission denied (publickey).
 fatal: The remote end hung up unexpectedly

Assuming you haven't set up any ssh rules for the github.com host,
you're trying to log in with ssh with your local username, which isn't
going to work.

 
  git archive --format=tar --remote=git://github.com/dkovar/analyzeMFT v2.0.4
 
 fatal: remote error
Your Git client has made an invalid request:
003agit-upload-archive /dkovar/analyzeMFT
 

This is the right format. GitHub doesn't allow remote archive requests,
which is why it's complaining.

If you want a tarball from GitHub, you need to download over HTTP from
the links they provide (which you can find e.g. through the web UI).

 
 The github page also says I can use ssh with git as the user, but that
 complains I don't have the private key (which I don't):
 
 git archive --format=tar
 --remote=ssh://git@github/com/dkovar/analyzeMFT.git v2.0.4

Using git as the ssh user is the right thing (if you want to talk git
over ssh) with GitHub and a few other sites/hosting programs, as your
public key is used to determine which user is trying to connect without
giving each user an account on the underlying OS.

   cmn


--
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] pull: fail early if we know we can't merge from upstream

[Carlos Martín Nieto]

On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  I can't quite decide whether the behaviour of 'git pull' with no
  upstream configured but a default remote with no fetch refspecs
  merging the remote's HEAD is a feature, a bug or something in between,
  but it's used by t7409 so maybe someone else is using it and we
  shouldn't break it.
 
 Isn't it the simplest works without any configuration from the
 original days? 

I don't recall remotes not having refspecs when they're int he config,
though I guess it's equivalent to running 'git pull
git://example.org/myrepo.git'.

 
  There's another check that could be made earlier ('git pull
  someremote' when that's not the branch's upstream remote), but then
  you have to start figuring out what the flags to fetch are.
 
 When the user gave us explicitly the name of the remote, it does not
 sound too bad to fetch from there.  git pull someremote thatbranch
 can be given after seeing a failure and succeed without retransfer,
 no?

It's not too bad, though you're paying for connection and ref
advertisement twice which breaks the otherwise quick pace of git
commands.

What I find bad from a UI point of view is that after fetching (which
could even be from the wrong remote for 'git pull' w/o upstream info)
git turns around and says I was never going to merge/rebase that for
things that we can know before fetching because they depend solely on
the configuration.

 
 I am not sure if it is worth the added complexity and potential to
 introduce new bugs in general by trying to outsmart the for-merge
 logic that kicks in only after we learn what the other side offers
 and fetch from it, but anyway, let's see what we got here...
 
  diff --git a/git-pull.sh b/git-pull.sh
  index 266e682..b62f5d3 100755
  --- a/git-pull.sh
  +++ b/git-pull.sh
  @@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=
   merge_args= edit=
   curr_branch=$(git symbolic-ref -q HEAD)
   curr_branch_short=${curr_branch#refs/heads/}
  +upstream=$(git config branch.$curr_branch_short.merge)
  +remote=$(git config branch.$curr_branch_short.remote)
   rebase=$(git config --bool branch.$curr_branch_short.rebase)
 
 Learning these upfront sounds sensible.
 
   if test -z $rebase
   then
  @@ -138,6 +140,47 @@ do
  esac
  shift
   done
  +if test true = $rebase
  +then
  +op_type=rebase
  +op_prep=against
  +else
  +op_type=merge
  +op_prep=with
  +fi
  +
  +check_args_against_config () {
  +   # If fetch gets user-provided arguments, the user is
  +   # overriding the upstream configuration, so we have to wait
  +   # for fetch to do its work to know if we can merge.
  +   if [ $# -gt 0 ]; then
  +   return
  +   fi
 
  +   # Figure out what remote we're going to be fetching from
  +   use_remote=origin
  +   if [ -n $remote ]; then
  +   use_remote=$remote
  +   fi
  +
  +   # If the remote doesn't have a fetch refspec, then we'll merge
  +   # whatever fetch marks for-merge, same as above.
 
 The above in this sentence refers to...?
 
 I guess we have to wait, but it wasn't very clear.
 

Yes, it refers to having to wait for fetch to complete before we can
know if we'll be able to merge.

  +   fetch=$(git config --get-all remote.$use_remote.fetch)
  +   if [ -z $fetch ]; then
  +   return
  +   fi
 
 Hmm, it is probably correct to punt on this case, but it defeats
 large part of the effect of your effort, doesn't it? We fetch what
 is covered by remote.$name.fetch _and_ what need to complete the
 merge operation (otherwise branch.$name.merge that is not covered by
 remote.$there.fetch will not work).  So
 
 [remote origin]
 url = $over_there
 [branch master]
 remote = origin
 merge = refs/heads/master
 
 would still fetch refs/heads/master from there and merge it.

If you run 'git pull' in this situation, then everything's fine and the
right thing gets merged.

 
  +   # The typical 'git pull' case where it should merge from the
  +   # current branch's upstream. We can already check whether we
  +   # we can do it. If HEAD is detached or there is no upstream
  +   # branch, complain now.
 
 Drop typical, and rephrase merge from to also cover rebase (I
 often say integrate with).

Sounds good.

 
 To return to your original description:
 
 A 'git pull' without specifying a remote is asked to take the
 current branch's upstream as the branch to merge from. This
 cannot work without an upstream configuration nor with HEAD
 detached, but we only check for this after fetching.
 
 Wouldn't it be sufficient to add something like this before fetch
 happens:
 
   if test $# != 0 || # args explicitly specified
test -n $curr_branch || # not detached
  test -n $upstream # what to integrate with is known
   then
   return ;# then no problem
   fi
   die underspecified 'git pull

[PATCH] pull: fail early if we know we can't merge from upstream

[Carlos Martín Nieto]

A 'git pull' without specifying a remote is asked to take the current
branch's upstream as the branch to merge from. This cannot work
without an upstream configuration nor with HEAD detached, but we only
check for this after fetching.

Perform the check beforehand, as we already know whether we have
enough information to merge and can fail immediately otherwise.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 git-pull.sh | 62 -
 1 file changed, 45 insertions(+), 17 deletions(-)

I can't quite decide whether the behaviour of 'git pull' with no
upstream configured but a default remote with no fetch refspecs
merging the remote's HEAD is a feature, a bug or something in between,
but it's used by t7409 so maybe someone else is using it and we
shouldn't break it.

There's another check that could be made earlier ('git pull
someremote' when that's not the branch's upstream remote), but then
you have to start figuring out what the flags to fetch are. I'll
revisit this at some point, but I wanted to get this out since it's
working.

diff --git a/git-pull.sh b/git-pull.sh
index 266e682..b62f5d3 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=
 merge_args= edit=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=${curr_branch#refs/heads/}
+upstream=$(git config branch.$curr_branch_short.merge)
+remote=$(git config branch.$curr_branch_short.remote)
 rebase=$(git config --bool branch.$curr_branch_short.rebase)
 if test -z $rebase
 then
@@ -138,6 +140,47 @@ do
esac
shift
 done
+if test true = $rebase
+then
+op_type=rebase
+op_prep=against
+else
+op_type=merge
+op_prep=with
+fi
+
+check_args_against_config () {
+   # If fetch gets user-provided arguments, the user is
+   # overriding the upstream configuration, so we have to wait
+   # for fetch to do its work to know if we can merge.
+   if [ $# -gt 0 ]; then
+   return
+   fi
+
+   # Figure out what remote we're going to be fetching from
+   use_remote=origin
+   if [ -n $remote ]; then
+   use_remote=$remote
+   fi
+
+   # If the remote doesn't have a fetch refspec, then we'll merge
+   # whatever fetch marks for-merge, same as above.
+   fetch=$(git config --get-all remote.$use_remote.fetch)
+   if [ -z $fetch ]; then
+   return
+   fi
+
+   # The typical 'git pull' case where it should merge from the
+   # current branch's upstream. We can already check whether we
+   # we can do it. If HEAD is detached or there is no upstream
+   # branch, complain now.
+   if [ -z $curr_branch_short -o -z $upstream ]; then
+   . git-parse-remote
+   error_on_missing_default_upstream pull $op_type $op_prep \
+   git pull remote branch
+   exit 1
+   fi
+}
 
 error_on_no_merge_candidates () {
exec 2
@@ -151,19 +194,6 @@ error_on_no_merge_candidates () {
esac
done
 
-   if test true = $rebase
-   then
-   op_type=rebase
-   op_prep=against
-   else
-   op_type=merge
-   op_prep=with
-   fi
-
-   curr_branch=${curr_branch#refs/heads/}
-   upstream=$(git config branch.$curr_branch.merge)
-   remote=$(git config branch.$curr_branch.remote)
-
if [ $# -gt 1 ]; then
if [ $rebase = true ]; then
printf There is no candidate for rebasing against 
@@ -177,10 +207,6 @@ error_on_no_merge_candidates () {
echo You asked to pull from the remote '$1', but did not 
specify
echo a branch. Because this is not the default configured 
remote
echo for your current branch, you must specify a branch on the 
command line.
-   elif [ -z $curr_branch -o -z $upstream ]; then
-   . git-parse-remote
-   error_on_missing_default_upstream pull $op_type $op_prep \
-   git pull remote branch
else
echo Your configuration specifies to $op_type $op_prep the ref 
'${upstream#refs/heads/}'
echo from the remote, but no such ref was fetched.
@@ -213,6 +239,8 @@ test true = $rebase  {
fi
done
 }
+
+check_args_against_config $@
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok 
$@ || exit 1
 test -z $dry_run || exit 0
-- 
1.8.2.524.g8f8def7

--
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] Documentation/git-commit: reword the --amend explanation

[Carlos Martín Nieto]

On Thu, 2013-04-04 at 10:04 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  On Wed, 2013-04-03 at 23:25 +0100, Philip Oakley wrote:
 
   + Replace the tip of the current branch with a fresh commit.
  [or updated commit, or new commit, or ...]
 
  Ack, we should lead with the goal, I'd go for the
 
  Replace the tip of the current branch with a new commit
 
  wording.
 
 We would want to be careful to make sure that the reader understands
 that the new commit is created by running this command (i.e. it is
 not like git branch -f $current_branch $new_commit), but other
 than that, sounds sensible.
 
 Perhaps like this?
 
  Documentation/git-commit.txt | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
 index bc919ac..61266d8 100644
 --- a/Documentation/git-commit.txt
 +++ b/Documentation/git-commit.txt
 @@ -190,8 +190,8 @@ OPTIONS
   without changing its commit message.
  
  --amend::
 - Create a new commit and replace the tip of the current
 - branch. The recorded tree is prepared as usual (including
 + Replace the tip of the current branch by creating a new
 + commit. The recorded tree is prepared as usual (including
   the effect of the `-i` and `-o` options and explicit
   pathspec), and the message from the original commit is used
   as the starting point, instead of an empty message, when no
 

Looks good, yeah. This should stop anybody thinking that they can
replace the tip with an arbitrary commit.

   cmn


--
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] Documentation/git-commit: reword the --amend explanation

[Carlos Martín Nieto]

On Wed, 2013-04-03 at 23:25 +0100, Philip Oakley wrote:
 Sent: Wednesday, April 03, 2013 9:04 PM
  Junio C Hamano gits...@pobox.com writes:
 
  Yes, and since then we gained --no-edit option and such, so editor
  starts off also needs to be rethought, no?  The original wording
  with seeded may have a better chance of survival, I suspect, but
  still needs some adjustment.
 
  So here is my attempt.  We still need a sign-off from you even if we
  decide to use this version.  Relative to your original patch:


Sorry I keep forgetting lately, it seems I've been away from core git
too long.

Signed-off-by: Carlos Martín Nieto c...@elego.de

  -- 8 --
  From: Carlos Martín Nieto c...@elego.de
 
  The explanation for 'git commit --amend' talks about preparing a tree
  object, which shouldn't be how user-facing documentation talks about
  commit.
 
  Reword it to say it works as usual, but replaces the current commit.
 
  ---
  Documentation/git-commit.txt | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)
 
  diff --git a/Documentation/git-commit.txt 
  b/Documentation/git-commit.txt
  index 19cbb90..bc919ac 100644
  --- a/Documentation/git-commit.txt
  +++ b/Documentation/git-commit.txt
  @@ -190,14 +190,15 @@ OPTIONS
   without changing its commit message.
 
  --amend::
  - Used to amend the tip of the current branch. Prepare the tree
  - object you would want to replace the latest commit as usual
  - (this includes the usual -i/-o and explicit paths), and the
  - commit log editor is seeded with the commit message from the
  - tip of the current branch. The commit you create replaces the
  - current tip -- if it was a merge, it will have the parents of
  - the current tip as parents -- so the current top commit is
  - discarded.
  + Create a new commit and replace the tip of the current
  + branch.
 
 I don't think we should say Create New at the start of the sentence,
 which may confuse some, rather we should start with the key
 'Replace' verb, essentially swapping the parts to say:
 
  + Replace the tip of the current branch with a fresh commit.
 [or updated commit, or new commit, or ...]

Ack, we should lead with the goal, I'd go for the

Replace the tip of the current branch with a new commit

wording.


 
   The recorded tree is prepared as usual (including
  + the effect of the `-i` and `-o` options and explicit

Is recorded tree what we want to say at porcelain level? I'd go for
The commit as in my version, but maybe it's just the way I think about
it. I don't feel too strongly about changing it, though.

  + pathspec), and the message from the original commit is used
  + as the starting point, instead of an empty message, when no
  + other message is specified from the command line via options
  + such as `-m`, `-F`, `-c`, etc.  The new commit has the same
  + parents and author as the current one (the `--reset-author`
  + option can countermand this).
  +

The rest looks great.

   cmn


--
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] Documentation/git-commit: reword the --amend explanation

[Carlos Martín Nieto]

The explanation for 'git commit --amend' talks about preparing a tree
object, which shouldn't be how user-facing documentation talks about
commit.

Reword it to say it works as usual, but replaces the current commit.
---

The current text is from 2006, which I guess explains the wording.

 Documentation/git-commit.txt | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 42c22bb..48dac29 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -198,14 +198,11 @@ OPTIONS
without changing its commit message.
 
 --amend::
-   Used to amend the tip of the current branch. Prepare the tree
-   object you would want to replace the latest commit as usual
-   (this includes the usual -i/-o and explicit paths), and the
-   commit log editor is seeded with the commit message from the
-   tip of the current branch. The commit you create replaces the
-   current tip -- if it was a merge, it will have the parents of
-   the current tip as parents -- so the current top commit is
-   discarded.
+   Amend the tip of the current branch. The commit is prepared as
+   usual (including -i/-o and explicit paths) and the editor
+   starts off with the current tip's commit message. The new
+   commit has the same parents and author as the current one and
+   replaces it as the tip.
 +
 --
 It is a rough equivalent for:
-- 
1.8.2.524.g8f8def7

--
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: question re tags

[Carlos Martín Nieto]

On Fri, 2013-03-08 at 15:16 +, John Stean wrote:
 Ive been tagging some commits using tortoise git , for example with
 v1.0, v1.1 etc. In tortoise git log the tag sits alongside the
 commit as I expect.
 But when I do a git describe it outputs the first tag along with the
 latest commit. What am I doing wrong?

Those tags are probably lightweight tags, so by default git-describe
doesn't take them into account. You can pass the --tags flag to tell it
to consider lightweight tags as well.


   cmn


--
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: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 13:16 -0800, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  As packed-refs file is expected to be a text file, it is not
  surprising to get an undefined result if the it ends with an
  incomplete line.
 
  I guess that depends on what you mean by incomplete.
 
 I used that word in the POSIX sense, i.e.
 
   
 http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67

Huh, I must revise my POSIX. Sure, in that sense it's incomplete.

 
 Unless the user edited the file, an incomplete line may indicate
 that the file has been truncated when (or after) it was written, and
 we have to suspect not just that the last line may have been
 truncated (in this case, not having the full 40-hex object name),
 but other records that should have been after that line were lost.
 
 We may want to detect such corruption at runtime, at least at
 strategic places; making it a hard runtime error will make it
 difficult to use Git itself to recover from such an corruption
 (e.g. you may have a healty mirror from which you can recover refs
 with fetch --mirror).

Since the libgit2 parser seems to work with it, it's perfectly possible
I did mess about with the file and then promptly forgot. An error would
definitely not help here, but I do think a warning should be issued if
the file isn't quite as it should be. It seems the parser can already
detect this, so it could be as easy as adding a fprintf(stderr, ...).

 
 We should at least refrain from running repack/gc to make things
 worse, for example.
 

Sounds sensible, yep.

   cmn


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

Possible regression in ref advertisement

[Carlos Martín Nieto]

Hi all,

When testing to see if a different implementation was in shape, I came
across something odd where newer git doesn't advertise one of the refs
in the git repo.

Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git
versions omit peeling the v1.8.0-rc3 tag.

The diff between the command above when ran with 1.7.10.4 (from Debian)
and current 'master'

--- old 2013-02-25 17:31:29.583526606 +0100
+++ new 2013-02-25 17:31:36.783526559 +0100
@@ -1379,7 +1379,6 @@
 c15295d7477ccec489953299bd03a8e62f86e611   refs/tags/v1.8.0-rc2
 cd46259ebf2e624bcee2aaae05c36663d414e1a2   refs/tags/v1.8.0-rc2^{}
 22ed067acc84eac8a0a72d20478a18aee4e25571   refs/tags/v1.8.0-rc3
-87a5461fa7b30f7b7baf27204f10219d61500fbf   refs/tags/v1.8.0-rc3^{}
 bfeb8b9ae0012cb61e026cbcd29664876abf5389   refs/tags/v1.8.0.1
 ed9fe755130891fc878bb2433204faffb534697b   refs/tags/v1.8.0.1^{}
 63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e   refs/tags/v1.8.0.2

Diffing with the output from next, diff tells me it's binary for some
reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there
either. I haven't had time to bisect this, so I'm putting it out there
in case anybody wants to investigate before I have time to dig into it.


   cmn


--
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: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  Hi all,
 
  When testing to see if a different implementation was in shape, I came
  across something odd where newer git doesn't advertise one of the refs
  in the git repo.
 
  Running `git ls-remote .` or `git-upload-pack` in my git repo, newer git
  versions omit peeling the v1.8.0-rc3 tag.
 
  The diff between the command above when ran with 1.7.10.4 (from Debian)
  and current 'master'
 
  --- old 2013-02-25 17:31:29.583526606 +0100
  +++ new 2013-02-25 17:31:36.783526559 +0100
  @@ -1379,7 +1379,6 @@
   c15295d7477ccec489953299bd03a8e62f86e611   refs/tags/v1.8.0-rc2
   cd46259ebf2e624bcee2aaae05c36663d414e1a2   refs/tags/v1.8.0-rc2^{}
   22ed067acc84eac8a0a72d20478a18aee4e25571   refs/tags/v1.8.0-rc3
  -87a5461fa7b30f7b7baf27204f10219d61500fbf   refs/tags/v1.8.0-rc3^{}
   bfeb8b9ae0012cb61e026cbcd29664876abf5389   refs/tags/v1.8.0.1
   ed9fe755130891fc878bb2433204faffb534697b   refs/tags/v1.8.0.1^{}
   63add1fb45e1ab7a76bb38bbb9467c91fdfaaa7e   refs/tags/v1.8.0.2
 
  Diffing with the output from next, diff tells me it's binary for some
  reason, but looking manually, the peeled v1.8.0-rc3 tag isn't there
  either. I haven't had time to bisect this, so I'm putting it out there
  in case anybody wants to investigate before I have time to dig into it.
 
 Interesting.  git ls-remote . | grep 1.8.0- for maint, master,
 next and pu produce identical results for me, all showing peeled
 ones correctly.

Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use
peel_ref for ref advertisements) and reverting that commit brings the
-rc3^{} back.

   cmn

--
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: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 11:27 -0800, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  On Mon, 2013-02-25 at 10:31 -0800, Junio C Hamano wrote:
  ...
  Interesting.  git ls-remote . | grep 1.8.0- for maint, master,
  next and pu produce identical results for me, all showing peeled
  ones correctly.
 
  Bisection leads me to Peff's 435c8332 (2012-10-04; upload-pack: use
  peel_ref for ref advertisements) and reverting that commit brings the
  -rc3^{} back.
 
 A shot in the dark, as I do not seem to be able to reproduce the issue
 with anything that contains the commit.  Perhaps your .git/packed-refs
 is corrupt?

My packed-refs file did not end with LF. It seems it must or the parser
won't consider the last tag in the file to have a peeled target and mine
didn't. I don't how the ended up this way but it looks like there's is a
bug in the parser (or does the format force you to have LF at the end?)

   cmn


--
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: Possible regression in ref advertisement

[Carlos Martín Nieto]

On Mon, 2013-02-25 at 12:07 -0800, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  A shot in the dark, as I do not seem to be able to reproduce the issue
  with anything that contains the commit.  Perhaps your .git/packed-refs
  is corrupt?
 
  My packed-refs file did not end with LF. It seems it must or the parser
  won't consider the last tag in the file to have a peeled target and mine
  didn't. I don't how the ended up this way but it looks like there's is a
  bug in the parser (or does the format force you to have LF at the end?)
 
 It is unclear what kind of corruption your packed-refs file had, as
 there are multiple ways for a file not to end with LF, but anyway.

I mean that the file ended at the end of the peeled id. It was missing
the last empty line.

 
 As packed-refs file is expected to be a text file, it is not
 surprising to get an undefined result if the it ends with an
 incomplete line.

I guess that depends on what you mean by incomplete. All the data is
there, just that it had

tag id SP refname LF ^peeled id

as the last entry instead of

tag id SP refname LF ^peeled id LF

The parser skips the last entry if there is no trailing LF (the same
happens for lightweight tags, though here it simply doesn't report
them).

 
 I do not offhand recall if we tolerate lines with CRLF endings; as
 far as I know we do not _write_ CRLF out, and because we do not
 expect people to muck directly with editor bypassing pack-refs, I
 would not be surprised if we didn't do anything special for people
 who make the lines end with with CRLF the file, either.

I wouldn't expect it to work. It would probably skip over those entries.

 
 I'd be more worried about the possibly lost entries after that
 incomplete line (and also possibly truncated end part of the tagname
 on the last, incomplete line).  Perhaps fsck should diagnose such an
 anomaly as repository corruption?  Perhaps we should enhance the
 file format a bit (right now, the first capability line should say
 something like # pack-refs with: peeled to say it was created with
 the version of pack-refs that can record peeled tags, but we can add
 other capabilities to extend the format) to add checksums to detect
 corruption?
 

I just tested and the parser ignores any malformed lines. The ones after
it are fine. Nothing complains though, ls-remote just shows the next
entry. I'd expect at least fsck to complain, but it doesn't say
anything. Presumably they all use the same parser that just ignores
anything it doesn't like.

   cmn


--
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: Google Summer of Code 2013 (GSoC13)

[Carlos Martín Nieto]

Michael Schubert s...@schu.io writes:

 On 02/18/2013 06:42 PM, Jeff King wrote:
 
 I will do it again, if people feel strongly about Git being a part of
 it. However, I have gotten a little soured on the GSoC experience. Not
 because of anything Google has done; it's a good idea, and I think they
 do a fine of administering the program. But I have noticed that the work
 that comes out of GSoC the last few years has quite often not been
 merged, or not made a big impact in the codebase, and nor have the
 participants necessarily stuck around.
 
 And I do not want to blame the students here (some of whom are on the cc
 list :) ). They are certainly under no obligation to stick around after
 GSoC ends, and I know they have many demands on their time. But I am
 also thinking about what Git wants to get out of GSoC (and to my mind,
 the most important thing is contributors).

 Speaking of libgit2:

 Git provided the libgit2 project with a slot each of the last three GSOC.
 The contributions made by the former students (Disclaimer: one of them
 speaking) have been quite important for libgit2 and all three students
 are still involved. Each project was an important push towards building
 a new, feature complete Git library.

Right, speaking of libgit2. GSoC has been very successful (as Michael,
I'm also somewhat biased) for libgit2. This happens outside of the git
ML so it probably hasn't gotten as much visibility here.

I believe it's partly because there were still larger parts where most
of the work was technical and the goal was quite clear, as git had
already set the standard and expectations and the decisions had to be
mostly about how to implement it in a way that makes sense for a
library, rather than it living inside of git, which is not always easy,
but you can experiment with different uses of it.

It's also possible that part of the success was the fact that we were
already acquainted with the release often and early policy, as we'd
been involved with FLOSS for a while already.

The current gaping hole in libgit2 is the lack of merge support, which
is the last hurdle to a stable 1.0 release. There is already some work
by Edward Thomson that needs to be reviewed and merged. I'm not sure
that there's enough for a whole summer there, but you could throw in the
review and merge of another missing feature, which is making the
reference storage generic, as it currently only supports the
git-compatible file-based one. There's other nice-to-have things like
thin-pack support that you could use to fill up a summer, though I'm not
sure that goes with the spirit of the programme.

Something else that needs love is Git for Windows. I believe both git
and libgit2 would benefit a lot from a project to take some parts of git
that are implemented in a scripting language and port them to use
libgit2. As Git for Windows needs to ship a ton of dependencies anyway,
using a pre-1.0 library wouldn't be an issue and it can be used to
experiment with an eventual porting of git to be one user of libgit2
rather than a completely different implementation. The more immediate
benefit for Git for Windows would be less reliance on languages that are
awkward to use on Windows and need their own environment. Mentoring from
the libgit2 probably wouldn't be much of an issue to organise, though
I'm not sure if the GfW team would have time for the part that involves
its peculiarities.

So there's a couple of projects that could be done with some realistic
chance of being merged upstream, as they'd be technical, as long as we
do tell the student to send small units of work to be reviewed often.

Cheers,
   cmn
--
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: Inconsistency in messages about --set-upstream from git pull and git branch

[Carlos Martín Nieto]

Dan Rosén d...@student.chalmers.se writes:


 git branch --set-upstream master origin/branch


This has been fixed already in 1.8.0.1

   cmn
--
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] config: don't segfault when given --path with a missing value

[Carlos Martín Nieto]

When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.

Detect the error and return immediately to avoid freeing or accessing
the uninitialed memory in the stack.

Signed-off-by: Carlos Martín Nieto c...@elego.de

---

On Thu, Nov 15, 2012 at 08:11:50AM -0800, Jeff King wrote:

 Hmm, actually, we should probably propagate the error (I was thinking
 for some reason this was in the listing code, but it is really about
 getting a specific variable, and that variable does not have a sane
 format. We'll already have printed the non-bool error, so we should
 probably die. So more like:
 
   if (git_config_pathname(vptr, key_, value_)  0)
   return -1;
   must_free_vptr = 1;

Yeah, that's more sensible. I didn't notice that the buffer never gets
written to in this codepath, and the trying to print it out is silly
when we know that there is nothing valid to print. Thanks for the
review. I've included your test as well, which really makes all of
this your code. Do we have some equivalent of a Basically-writen-by
line?

 builtin/config.c   | 3 ++-
 t/t1300-repo-config.sh | 5 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 442ccc2..4dc5ffa 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -129,7 +129,8 @@ static int show_config(const char *key_, const char 
*value_, void *cb)
else
sprintf(value, %d, v);
} else if (types == TYPE_PATH) {
-   git_config_pathname(vptr, key_, value_);
+   if (git_config_pathname(vptr, key_, value_)  0)
+   return -1;
must_free_vptr = 1;
} else if (value_) {
vptr = value_;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a477453..17272e0 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -803,6 +803,11 @@ test_expect_success NOT_MINGW 'get --path copes with unset 
$HOME' '
test_cmp expect result
 '
 
+test_expect_success 'get --path barfs on boolean variable' '
+   echo [path]bool .git/config 
+   test_must_fail git config --get --path path.bool
+'
+
 cat  expect  EOF
 [quote]
leading =  test
-- 
1.8.0.316.g291341c

--
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] config: don't segfault when given --path with a missing value

[Carlos Martín Nieto]

When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.

Set the must_free_vptr flag depending on the return value of
git_config_pathname so it's accurate.
---
 builtin/config.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 442ccc2..60220d5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -129,8 +129,7 @@ static int show_config(const char *key_, const char 
*value_, void *cb)
else
sprintf(value, %d, v);
} else if (types == TYPE_PATH) {
-   git_config_pathname(vptr, key_, value_);
-   must_free_vptr = 1;
+   must_free_vptr = !git_config_pathname(vptr, key_, value_);
} else if (value_) {
vptr = value_;
} else {
-- 
1.8.0.316.g291341c

--
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: Git ~unusable on slow lines :,'C

[Carlos Martín Nieto]

Marcel Partap mpar...@gmx.net writes:

 Bam, the server kicked me off after taking to long to sync my copy.
 This is unrelated to git. The HTTP server's configuration is too
 impatient.
 Yes. How does that mean it is unrelated to git?

 - git fetch should show the total amount of data it is about to
 transfer!
 It can't, because it doesn't know.
 The server side doesn't know at how much the objects *it just repacked
 for transfer* weigh in?
 If that truly is the case, wouldn't it make sense to make git a little
 more introspective? f.e.

It sends you more objects than the ones it just repacked in the normal
case. It could tell you, but it would have to keep track of more
information (which would make it take longer for the first bytes to get
to you) for little gain. The only thing you'd be able to do is to
abort the transfer immediately, but you can do that anyway, and waiting
is only going to add history to download.

 # git info git://foo.org/bar.git
 .. [server generating figures] ..
 URL: git://foo.org/bar.git
 Created/Earliest commit: ...
 Last modified/Latest commit: ...
 Total object count:  (..commits, ..files, .. directories)
 Total repository size (compressed): ... MiB
 Branches:
 [git branch -va] + branch size

 The error message doesn't really know whether it is going to overwrite
 it (the CR comes from the server), though I suppose an extra LF wouldn't
 hurt there.
 Definitely wouldn't hurt.

 - would be nice to be able to tell git fetch to get the next chunk of
 say 500 commits instead of trying to receive ALL commits, then b0rking
 after umpteen percent on server timeout. Not?
 You asked for the current state of the repository, and that's what its
 giving you.
 And instead, I would rather like to ask for the next 500 commits. No way
 to do it.

Do you mean that there are no tags in between your current state and the
one you want to be at?


 The timeout has nothing to do with git, if you can't
 convince the admins to increase it, you can try using another transport
 which doesn't suffer from HTTP, as it's most likely an anti-DoS measure.
 See, I probably can't convince the admins to drop their anti-dos measures.
 And they (drupal.org admins) probably will not change their allowed
 protocol policies.

Switch to using the raw git protocol, which is much less likely to have
this sort of measure.

 Despite that, i've had timeouts or simply stale connections dying down
 before with other repositories and various transport modes.
 The easiest fix would be an option to tell git to not fetch everything...

 If you want to download it bit by bit, you can tell fetch to download
 particular tags.
 ..without specifying specific commit tags.
 Browsing gitweb sites to find a tag for which the fetch doesn't time out
 is hugely inconvenient, especially on a slow line.

Don't use the web then. Use ls-remote to see what's at the other end.


 Doing this automatically for this would be working
 around a configuration issue for a particular server, which is generally
 better fixed in other ways.
 It is not only a configuration issue for one particular server. Git in
 general is hardly usable on slow lines because
 - it doesn't show the volume of data that is to be downloaded!

How would showing the amount of data help your connection?

 - it doesn't allow the user to sync up in steps the circumstances will
 allow to succeed.

This is unfortunate is some circunstances, but you haven't shown that
yours is one of these.


   cmn
--
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: Git ~unusable on slow lines :,'C

[Carlos Martín Nieto]

Marcel Partap mpar...@gmx.net writes:

 Dear Git Devs,
 I love GIT, but since a couple of months I'm on 3G and after my traffic
 limit is transcended, things slow down to a feeble 8KiB/s. Jst like
 back then - things moved somewhat slower. And I'm fine with that - as
 long as things just keep moving.
 Unfortunately, git does not scale down very well, so for ten more days I
 will be unable to get the newest commits onto my machine. Which is very,
 very sad :/
 git fetch --verbose --all 
 Fetching origin
 POST git-upload-pack (1023 bytes)
 POST git-upload-pack (gzip 1123 to 614 bytes)
 POST git-upload-pack (gzip 1973 to 1030 bytes)
 POST git-upload-pack (gzip 5173 to 2639 bytes)
 POST git-upload-pack (gzip 7978 to 4042 bytes)
 remote: Counting objects: 24504, done.
 remote: Compressing objects: 100% (10705/10705), done.
 error: RPC failed; result=56, HTTP code = 200iB | 10 KiB/s   
 fatal: The remote end hung up unexpectedly
 fatal: early EOF
 fatal: index-pack failed
 error: Could not fetch origin
 Bam, the server kicked me off after taking to long to sync my copy.

This is unrelated to git. The HTTP server's configuration is too
impatient.

 Multiple potential points of action:
 - git fetch should show the total amount of data it is about to
 transfer!

It can't, because it doesn't know.

 - when ab^H^Horting, the cursor should be moved down (tput cud1) to not
 overwrite previous output

The error message doesn't really know whether it is going to overwrite
it (the CR comes from the server), though I suppose an extra LF wouldn't
hurt there.

 - would be nice to be able to tell git fetch to get the next chunk of
 say 500 commits instead of trying to receive ALL commits, then b0rking
 after umpteen percent on server timeout. Not?

You asked for the current state of the repository, and that's what its
giving you. The timeout has nothing to do with git, if you can't
convince the admins to increase it, you can try using another transport
which doesn't suffer from HTTP, as it's most likely an anti-DoS measure.

If you want to download it bit by bit, you can tell fetch to download
particular tags. Doing this automatically for this would be working
around a configuration issue for a particular server, which is generally
better fixed in other ways.


   cmn
--
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 report

[Carlos Martín Nieto]

Konstantin Khomoutov flatw...@users.sourceforge.net writes:

 On Fri, 5 Oct 2012 14:13:49 +0400
 Муковников Михаил m.mukovni...@gmail.com wrote:

 There's a problem using git with files having cyrillic 'й' in their
 name, git just can't track them.
 
 uname: Darwin 12.2.0 Darwin Kernel Version 12.2.0: Sat Aug 25
 00:48:52 PDT 2012; root:xnu-2050.18.24~1/RELEASE_X86_64 x86_64 git
 version: 1.7.12.1 (from macports)
 
 Steps to reproduce:
 - git init
 - touch test_й
 - git status (should be untracked files present)
 - git add test_й
 - git status
 
 # Untracked files:
 #   (use git add file... to include in what will be committed)
 #
 #test_\320\270\314\206
 nothing added to commit but untracked files present (use git add to
 track)
 
 Could this be helped somehow?--

 What this?  If you mean displaying escapes for UTF-8 bytes
 representing that letter й, then try setting core.quotepath to false
 for this repository and see if that helps.

Notice the 'git add test_й'. The problem is that git reports it as untracked.

Михаил, is this the whole output or do you also see a
differently-escaped version of the filename under tracked files? Does
this problem not show up if you use 'git add -A' or 'git add .' instead
of typing the name? If so, this happens because HFS+ stores and reports
names differently than the way we told it to store it, so git sees a
different set of bytes than what it's expecting and considers it a
different file.

With a recent version of git, you can set the core.precomposeunicode
config setting to true, which deals with this situation. This tells git
to transform the data it gets from the filesystem to the format that
everyone else uses, which helps not only this, but also the 'git add .'
case, so git stores the filename in the format the same way that other
OSs expect to find.

   cmn
--
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] run-command: don't try to execute directories

[Carlos Martín Nieto]

When looking through $PATH to try to find an external command,
locate_in_PATH doesn't check that it's trying to execute a file. Add a
check to make sure we won't try to execute a directory.

This also stops us from looking further and maybe finding that the
user meant an alias, as in the case where the user has
/home/user/bin/git-foo/git-foo.pl and an alias

[alias] foo = !/home/user/bin/git-foo/git-foo.pl

Running 'git foo' will currently will try to execute ~/bin/git-foo and
fail because you can't execute a directory. By making sure we don't do
that, we realise that it's an alias and do the right thing

Signed-off-by: Carlos Martín Nieto c...@elego.de

---

This comes from a case in #git. Not sure if this is worth it, or the
better solution is just to say no to dirs in $PATH.

After writing all of that, I thought to check the shell, and indeed

% git-foo
zsh: permission denied: git-foo

so if the shell doesn't do it, the benefits probably don't outweigh
having a dozen stat instead of access calls. strace reveals that zsh
does what git currently does. bash uses stat and says 'command not
found'.

Sending in case someone finds it useful or interesting. Feel free to
ignore it or make fun of it if you want.

 run-command.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 1101ef7..97e6960 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)
 {
const char *p = getenv(PATH);
struct strbuf buf = STRBUF_INIT;
+   struct stat st;
 
if (!p || !*p)
return NULL;
@@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)
}
strbuf_addstr(buf, file);
 
-   if (!access(buf.buf, F_OK))
+   if (!stat(buf.buf, st)  !S_ISDIR(st.st_mode)) {
return strbuf_detach(buf, NULL);
+   }
 
if (!*end)
break;
-- 
1.8.0.rc0.175.g59a8d0e

--
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] run-command: don't try to execute directories

[Carlos Martín Nieto]

Junio C Hamano gits...@pobox.com writes:

 Carlos Martín Nieto c...@elego.de writes:

 When looking through $PATH to try to find an external command,
 locate_in_PATH doesn't check that it's trying to execute a file. Add a
 check to make sure we won't try to execute a directory.

 This also stops us from looking further and maybe finding that the
 user meant an alias, as in the case where the user has
 /home/user/bin/git-foo/git-foo.pl and an alias

 [alias] foo = !/home/user/bin/git-foo/git-foo.pl

 Running 'git foo' will currently will try to execute ~/bin/git-foo and
 fail because you can't execute a directory. By making sure we don't do
 that, we realise that it's an alias and do the right thing

 Signed-off-by: Carlos Martín Nieto c...@elego.de

 ---

 This comes from a case in #git. Not sure if this is worth it, or the
 better solution is just to say no to dirs in $PATH.

 After writing all of that, I thought to check the shell, and indeed

 % git-foo
 zsh: permission denied: git-foo

 so if the shell doesn't do it, the benefits probably don't outweigh
 having a dozen stat instead of access calls. strace reveals that zsh
 does what git currently does. bash uses stat and says 'command not
 found'.

 Hrm, I do not use zsh but it does not seem to reproduce for me.

   $ mkdir -p /var/tmp/xx/git
 $ zsh
 % PATH=/var/tmp/xx:$PATH
 % type git
 git is /home/junio/bin/git
 % git version
 git version 1.8.0.rc0.45.g7ce8dc5
   % zsh --version
   zsh 4.3.10 (x86_64-unknown-linux-gnu)

zsh has some quite aggressive PATH caching. I did this with git-foo in
the path so it didn't already know what to look for. I can reproduce
what you saw, but also consider this:

% /var/tmp/xx/git
zsh: permission denied: /var/tmp/xx/git
% zsh --version
zsh 4.3.17 (x86_64-unknown-linux-gnu)

If you change your test to use git-foo instead of just git, you should
see what I wrote in the message.

bash rightfully complains that it's a stupid thing to do.

$ /var/tmp/xx/git
bash: /var/tmp/xx/git: Is a directory


 @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)
  }
  strbuf_addstr(buf, file);
  
 -if (!access(buf.buf, F_OK))
 +if (!stat(buf.buf, st)  !S_ISDIR(st.st_mode)) {
  return strbuf_detach(buf, NULL);
 +}

 So we used to say if it exists and accessible, return that.  Now
 we say if it exists and is not a directory, return that.

 I have to wonder what would happen if it exists as a non-directory
 but we cannot access it.  Is that a regression?

I guess it would be, yeah. Would this be related to tha situation where
the user isn't allowed to access something in their PATH?

How about something like this instead? We keep the access check and only
do the stat call when we have found something we want to look at.

   cmn

---8---

diff --git a/run-command.c b/run-command.c
index 1101ef7..fb8a93c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)
 {
const char *p = getenv(PATH);
struct strbuf buf = STRBUF_INIT;
+   struct stat st;
 
if (!p || !*p)
return NULL;
@@ -101,7 +102,8 @@ static char *locate_in_PATH(const char *file)
}
strbuf_addstr(buf, file);
 
-   if (!access(buf.buf, F_OK))
+   if (!access(buf.buf, F_OK) 
+   !stat(buf.buf, st)  !S_ISDIR(st.st_mode))
return strbuf_detach(buf, NULL);
 
if (!*end)
--
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: checkout extra files

[Carlos Martín Nieto]

Angelo Borsotti angelo.borso...@gmail.com writes:

 Hello,

 the man page of git checkout states:

 git checkout [-p|--patch] [tree-ish] [--] pathspec...

 It updates the named paths in the working tree from the index file or
 from a named tree-ish ...

 This means that for each file denoted by pathspec, git tries to
 restore it from the tree-ish.
 However, it seems that git does more than this: it restores also files
 that are not denoted
 by pathspec.
 This sequence of commands shows it:

 $ mkdir gittest
 $ cd gittest
 $ git init
 Initialized empty Git repository in d:/gittest/.git/
 $ touch f1
 $ git add f1
 $ git commit commit -m first commit
 [master (root-commit) 94d882a] first commit
  0 files changed
  create mode 100644 f1
 $ rm f1
 $ git checkout 94d8 -- *
 $ ls
 f1

 Note that the work directory is empty when the checkout is done, and
 that the checkout restores f1
 in it, a file that is not denoted by the * pathspec.

The '*' pathspec refers to every file. There are no files in the current
directory, so your shell can't expand the glob and passes it to git,
which then looks and sees that the glob expands to f1 in the given
commit which it then checks out.

   cmn
--
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: checkout extra files

[Carlos Martín Nieto]

Angelo Borsotti angelo.borso...@gmail.com writes:

[please keep it in the list]

 Hi Carlos,

 the behavior is quite clear, but the man pages do not describe it properly.
 The man pages state:

 It updates the named paths in the working tree from the index file or
 from a named tree-ish 

 In my example, the named paths in the working tree is '*', which
 denotes

That grouping is not what it's saying. It doesn't update the files that
exist in the working tree matching some glob. It updates the files in
the working tree from either the index or a treeish. The pathspec
refers, as always, to the data source, and '*' matches all files.

It puts the named paths on to the working tree. Is that clearer?

 no files. So, the man pages are telling that the command updates nothing.
 The man pages should state that it copies from the index file of the names
 tree-ish the files that match the names paths and that are not present
 in the working tree.

Whether they are missing doesn't make any difference. It updates the
files you tell it from the index/tree. You told it to update all of
them.

   cmn
--
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: checkout extra files

[Carlos Martín Nieto]

Keep it in the list.

Angelo Borsotti angelo.borso...@gmail.com writes:

 Hi Carlos,

 That grouping is not what it's saying. It doesn't update the files that
 exist in the working tree matching some glob. It updates the files in
 the working tree from either the index or a treeish. The pathspec
 refers, as always, to the data source, and '*' matches all files.

 It puts the named paths on to the working tree. Is that clearer?

 This was mi first understanding, until one day I had in the working directory
 a file that matched the path (the path was '*') and that was NOT in the
 index or a treeish. The git checkout command tried to copy it and
 complained that there was no such file to restore.

So you're saying that you ran

git checkout tree-ish -- *

and git complained that there was no such file? This is because the
shell expanded the glob and gave git a list of files.

 Then I thought that it visited the working directory and tried to restore
 each file it matched and at the end restored also the ones that were not
 there.

I can't quite parse this. Git will restore whichever files you tell it
to. If you use an asterisk, then your shell will usually expand it. In
the case you posted to the list there were no files, so there was
nothing to expand it to. Some shells complain in this case by default,
some don't and just pass the asterisk to the program and let it figure
out what to do with it. This was the case in your example. You told git
to expand all the files it found in that tree-ish.

   cmn
--
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] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

Ralf Thielow ralf.thie...@gmail.com writes:

 On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto c...@elego.de wrote:
 behaviour. To work around this, introduce --set-upstream-to which
 accepts a compulsory argument indicating what the new upstream branch
 should be and one optinal argument indicating which branch to change,
 defaulting to HEAD.


 Could you please also add this new option to the
 contrib/completion/git-completion.bash
 script?

If I knew how those things work... Is this enough?

   cmn

--8--
Subject: [PATCH] completion: add --set-upstream-to and --unset-upstream

---
 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index ffedce7..4f46357 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -880,6 +880,7 @@ _git_branch ()
--color --no-color --verbose --abbrev= --no-abbrev
--track --no-track --contains --merged --no-merged
--set-upstream --edit-description --list
+   --unset-upstream --set-upstream-to=

;;
*)
-- 
1.7.12.3.g0dd8ef6
--
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/3] Improve branch UI for setting upstream information

[Carlos Martín Nieto]

Hi all,

As a result of making --unset-upstream fail if the given branch
doesn't exist, I discovered a copy-paste error in on the the tests in
the patch after it, so I'm resending the whole thing.

The changes from the last reroll are the tightening of the situations
where git will show an error message (not it's just if the branch is
new and exists as remote-tracking) which I already sent as a reply in
the other thread; and making --unset-upstream error out on bad input,
which I already mentioned above.

   cmn

Carlos Martín Nieto (3):
  branch: introduce --set-upstream-to
  branch: add --unset-upstream option
  branch: deprecate --set-upstream and show help if we detect possible
mistaken use

 Documentation/git-branch.txt | 14 -
 builtin/branch.c | 60 +--
 t/t3200-branch.sh| 67 
 3 files changed, 137 insertions(+), 4 deletions(-)

-- 
1.7.12.3.g0dd8ef6

--
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] branch: deprecate --set-upstream and show help if we detect possible mistaken use

[Carlos Martín Nieto]

This interface is error prone, and a better one (--set-upstream-to)
exists. Add a message listing the alternatives and suggest how to fix
a --set-upstream invocation in case the user only gives one argument
which causes a local branch with the same name as a remote-tracking
one to be created. The typical case is

git branch --set-upstream origin/master

when the user meant

git branch --set-upstream master origin/master

assuming that the current branch is master. Show a message telling the
user how to undo their action and get what they wanted. For the
command above, the message would be

The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
Branch origin/master set up to track local branch master.

If you wanted to make 'master' track 'origin/master', do this:

git branch -d origin/master
git branch --set-upstream-to origin/master

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 builtin/branch.c  | 26 ++
 t/t3200-branch.sh | 34 ++
 2 files changed, 60 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 557995d..5e95e35 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -881,10 +881,36 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
git_config_set_multivar(buf.buf, NULL, NULL, 1);
strbuf_release(buf);
} else if (argc  0  argc = 2) {
+   struct branch *branch = branch_get(argv[0]);
+   int branch_existed = 0, remote_tracking = 0;
+   struct strbuf buf = STRBUF_INIT;
+
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
+
+   if (track == BRANCH_TRACK_OVERRIDE)
+   fprintf(stderr, _(The --set-upstream flag is 
deprecated and will be removed. Consider using --track or 
--set-upstream-to\n));
+
+   strbuf_addf(buf, refs/remotes/%s, branch-name);
+   remote_tracking = ref_exists(buf.buf);
+   strbuf_release(buf);
+
+   branch_existed = ref_exists(branch-refname);
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
  force_create, reflog, 0, quiet, track);
+
+   /*
+* We only show the instructions if the user gave us
+* one branch which doesn't exist locally, but is the
+* name of a remote-tracking branch.
+*/
+   if (argc == 1  track == BRANCH_TRACK_OVERRIDE 
+   !branch_existed  remote_tracking) {
+   fprintf(stderr, _(\nIf you wanted to make '%s' track 
'%s', do this:\n\n), head, branch-name);
+   fprintf(stderr, _(git branch -d %s\n), 
branch-name);
+   fprintf(stderr, _(git branch --set-upstream-to 
%s\n), branch-name);
+   }
+
} else
usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 1018e8b..f2a076c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -402,6 +402,40 @@ test_expect_success 'test --unset-upstream on a particular 
branch' \
  test_must_fail git config branch.my14.remote 
  test_must_fail git config branch.my14.merge'
 
+test_expect_success '--set-upstream shows message when creating a new branch 
that exists as remote-tracking' \
+'git update-ref refs/remotes/origin/master HEAD 
+ git branch --set-upstream origin/master 2actual 
+ test_when_finished git update-ref -d refs/remotes/origin/master 
+ test_when_finished git branch -d origin/master 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+
+If you wanted to make ''master'' track ''origin/master'', do this:
+
+git branch -d origin/master
+git branch --set-upstream-to origin/master
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with two args only shows the deprecation 
message' \
+'git branch --set-upstream master my13 2actual 
+ test_when_finished git branch --unset-upstream master 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with one arg only shows the deprecation 
message if the branch existed' \
+'git branch --set-upstream my13 2actual 
+ test_when_finished git branch --unset-upstream my13 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+EOF
+ test_cmp expected actual
+'
+
 # Keep this test last, as it changes the current branch
 cat expect EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL

[PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

We have ways of setting the upstream information, but if we want to
unset it, we need to resort to modifying the configuration manually.

Teach branch an --unset-upstream option that unsets this information.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 Documentation/git-branch.txt |  5 +
 builtin/branch.c | 21 ++---
 t/t3200-branch.sh| 19 +++
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e41c4b5..9c1d2f1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
[(--merged | --no-merged | --contains) [commit]] [pattern...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] branchname 
[start-point]
 'git branch' (--set-upstream-to=upstream | -u upstream) [branchname]
+'git branch' --unset-upstream [branchname]
 'git branch' (-m | -M) [oldbranch] newbranch
 'git branch' (-d | -D) [-r] branchname...
 'git branch' --edit-description [branchname]
@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.
considered branchname's upstream branch. If no branchname
is specified, then it defaults to the current branch.
 
+--unset-upstream::
+   Remove the upstream information for branchname. If no branch
+   is specified it defaults to the current branch.
+
 --edit-description::
Open an editor and edit the text to explain what the branch is
for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 3c978eb..557995d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
int delete = 0, rename = 0, force_create = 0, list = 0;
int verbose = 0, abbrev = -1, detached = 0;
int reflog = 0, edit_description = 0;
-   int quiet = 0;
+   int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
enum branch_track track;
int kinds = REF_LOCAL_BRANCH;
@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_SET_INT( 0, set-upstream,  track, change upstream info,
BRANCH_TRACK_OVERRIDE),
OPT_STRING('u', set-upstream-to, new_upstream, upstream, 
change the upstream info),
+   OPT_BOOLEAN(0, unset-upstream, unset_upstream, Unset the 
upstream info),
OPT__COLOR(branch_use_color, use colored output),
OPT_SET_INT('r', remotes, kinds, act on remote-tracking 
branches,
REF_REMOTE_BRANCH),
@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 0);
 
-   if (!delete  !rename  !edit_description  !new_upstream  argc == 
0)
+   if (!delete  !rename  !edit_description  !new_upstream  
!unset_upstream  argc == 0)
list = 1;
 
-   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream  1)
+   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
!!unset_upstream  1)
usage_with_options(builtin_branch_usage, options);
 
if (abbrev == -1)
@@ -865,6 +866,20 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 * info and making sure new_upstream is correct
 */
create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
+   } else if (unset_upstream) {
+   struct branch *branch = branch_get(argv[0]);
+   struct strbuf buf = STRBUF_INIT;
+
+   if (!branch_has_merge_config(branch)) {
+   die(_(Branch '%s' has no upstream information), 
branch-name);
+   }
+
+   strbuf_addf(buf, branch.%s.remote, branch-name);
+   git_config_set_multivar(buf.buf, NULL, NULL, 1);
+   strbuf_reset(buf);
+   strbuf_addf(buf, branch.%s.merge, branch-name);
+   git_config_set_multivar(buf.buf, NULL, NULL, 1);
+   strbuf_release(buf);
} else if (argc  0  argc = 2) {
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..1018e8b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,25 @@ test_expect_success 'use --set-upstream-to modify a 
particular branch' \
  test $(git config branch.my13.remote) = . 
  test $(git config branch.my13.merge) = refs/heads/master'
 
+test_expect_success '--unset-upstream should fail if given a non-existent 
branch' \
+'test_must_fail git branch --unset-upstream

Re: [PATCHv2 0/3] Improve branch UI for setting upstream information

[Carlos Martín Nieto]

Junio C Hamano gits...@pobox.com writes:

 Carlos Martín Nieto c...@elego.de writes:

 As a result of making --unset-upstream fail if the given branch
 doesn't exist, I discovered a copy-paste error in on the the tests in
 the patch after it, so I'm resending the whole thing.

 The changes from the last reroll are the tightening of the situations
 where git will show an error message (not it's just if the branch is
 new and exists as remote-tracking) which I already sent as a reply in
 the other thread; and making --unset-upstream error out on bad input,
 which I already mentioned above.

 Thanks.

 In addition to --unset-upstream must fail on i-dont-exist branch
 in [2/3], I am wondering if we would want to also make sure the
 command fails when the upstream information is not set for the
 branch, i.e. something like the following on top.

 What do you think?

  t/t3200-branch.sh | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git i/t/t3200-branch.sh w/t/t3200-branch.sh
 index 1018e8b..a0aaedd 100755
 --- i/t/t3200-branch.sh
 +++ w/t/t3200-branch.sh
 @@ -393,7 +393,9 @@ test_expect_success 'test --unset-upstream on HEAD' \
   git branch --set-upstream-to my14 
   git branch --unset-upstream 
   test_must_fail git config branch.master.remote 
 - test_must_fail git config branch.master.merge'
 + test_must_fail git config branch.master.merge 
 + test_must_fail git branch --unset-upstream
 +'

Yeah, this looks good, makes sure that it will still behave correctly
even if the code path for these two situations diverges.

   cmn
--
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 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

Junio C Hamano gits...@pobox.com writes:

 Junio C Hamano gits...@pobox.com writes:

 c...@elego.de (Carlos Martín Nieto) writes:

 Junio C Hamano gits...@pobox.com writes:

 Carlos Martín Nieto c...@elego.de writes:

 diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
 index e9019ac..93e5d6e 100755
 --- a/t/t3200-branch.sh
 +++ b/t/t3200-branch.sh
 @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a 
 particular branch' \
   test $(git config branch.my13.remote) = . 
   test $(git config branch.my13.merge) = refs/heads/master'
  
 +test_expect_success 'test --unset-upstream on HEAD' \
 +'git branch my14
 + test_config branch.master.remote foo 
 + test_config branch.master.merge foo 
 + git branch --set-upstream-to my14 
 + git branch --unset-upstream 
 + test_must_fail git config branch.master.remote 
 + test_must_fail git config branch.master.merge'
 +
 +test_expect_success 'test --unset-upstream on a particular branch' \
 +'git branch my15
 + git branch --set-upstream-to master my14 
 + git branch --unset-upstream my14 
 + test_must_fail git config branch.my14.remote 
 + test_must_fail git config branch.my14.merge'
 +

 What should happen when you say --unset-upstream on a branch B
 that does not have B@{upstream}?  Should it fail?  Should it be
 silently ignored?  Is it undefined that we do not want to test?

 I'd say it should be ignored, as the end result we want is for there to
 be no upstream information. What we do underneath is remove a couple of
 config options, wich doesn't fail if they didn't insist in the first
 place.

 I am not sure about that reasoning.

 $ git config --unset core.nosuchvariable ; echo $?
 5

 looks like a failure to me.

 More importantly, wouldn't we want to catch typo in

  git branch --unset-upstream mext

 which admittedly should come from a different codepath (I do not
 think your patch checks if mext branch exists before going ahead
 to the config--unset dance)?

 Sorry, the last paragraph was incomplete.

 In general, we should detect errors and allow the user to choose to
 ignore.

 A script that wants to make sure that B does not have an upstream,
 without knowing if it already has one, can say --unset-upstream B
 and choose to ignore the error if B does not have an upstream.  

 If the script is carefully written, it would try to check if B has
 one and call --unset-upstream B only when it doesn't.  A casually
 written loose script would say --unset-upstream B 2/dev/null
 and keep going (it would not notice other kinds of errors, but that
 is what makes it casual and loose).

OK, that's a good point, I'll update the patch.

   cmn
--
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: libgit2 status

[Carlos Martín Nieto]

Nicolas Sebrecht nicolas.s@gmx.fr writes:

 The 25/08/12, Vicent Marti wrote:

 The development of libgit2 happens 100% in the open. I don't know what
 commercial entity are you talking about, but there are several
 companies and independent contributors working on the Library at the
 moment.

 Right but as far as I'm aware of Junio had reserves about libgit2
 integration into git due to issues making repositories broken. Though,

The comment I saw about that was that at one point libgit2 had produced
broken trees; which is true, the algorithm for the almost-alphanumeric
sorting was slightly broken. This was fixed quite some time ago, which
he also mentioned in the same message.

 [ I'm somewhat in the same situation of OP. ]

If you wait for it to be perfect, it's never going to happen. If your
application would benefit, port it to libgit2 and report the issues you
find. That's the only way we can know of the odd edge cases and
improvements that we should make.

Note that the GitHub apps for Mac and Windows both use the Library to
perform parts of their job. Their new backend for the website is also
(going to be) based on libgit2.

I am also working on a project for a client involving the Library for
importing data and the only problem we've had is that we discovered an
edge case regarding symlinks and an assumption that one of the bindings
made wrt diffs, which is getting fixed.

   cmn
--
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 3/3] branch: deprecate --set-upstream and show help if we detect possible mistaken use

[Carlos Martín Nieto]

This interface is error prone, and a better one (--set-upstream-to)
exists. Add a message listing the alternatives and suggest how to fix
a --set-upstream invocation in case the user only gives one argument
which causes a local branch with the same name as a remote-tracking
one to be created. The typical case is

git branch --set-upstream origin/master

when the user meant

git branch --set-upstream master origin/master

assuming that the current branch is master. Show a message telling the
user how to undo their action and get what they wanted. For the
command above, the message would be

The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
Branch origin/master set up to track local branch master.

If you wanted to make 'master' track 'origin/master', do this:

git branch -d origin/master
git branch --set-upstream-to origin/master

Signed-off-by: Carlos Martín Nieto c...@elego.de
---

The 'track' in the message is still not great, but it does fit with
the one above. Maybe if we make it say If youw wanted [...] track the
remote-tracking branch 'origin/master' it would be clearer?

I've simplified and tightened the logic. Now it will only show the
undo message if the branch didn't exist locally and there is a
remote-tracking branch of the same name.

 builtin/branch.c  | 26 ++
 t/t3200-branch.sh | 34 ++
 2 files changed, 60 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 08068f7..a0302a2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -877,10 +877,36 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
git_config_set_multivar(buf.buf, NULL, NULL, 1);
strbuf_release(buf);
} else if (argc  0  argc = 2) {
+   struct branch *branch = branch_get(argv[0]);
+   int branch_existed = 0, remote_tracking = 0;
+   struct strbuf buf = STRBUF_INIT;
+
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
+
+   if (track == BRANCH_TRACK_OVERRIDE)
+   fprintf(stderr, _(The --set-upstream flag is 
deprecated and will be removed. Consider using --track or 
--set-upstream-to\n));
+
+   strbuf_addf(buf, refs/remotes/%s, branch-name);
+   remote_tracking = ref_exists(buf.buf);
+   strbuf_release(buf);
+
+   branch_existed = ref_exists(branch-refname);
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
  force_create, reflog, 0, quiet, track);
+
+   /*
+* We only show the instructions if the user gave us
+* one branch which doesn't exist locally, but is the
+* name of a remote-tracking branch.
+*/
+   if (argc == 1  track == BRANCH_TRACK_OVERRIDE 
+   !branch_existed  remote_tracking) {
+   fprintf(stderr, _(\nIf you wanted to make '%s' track 
'%s', do this:\n\n), head, branch-name);
+   fprintf(stderr, _(git branch -d %s\n), 
branch-name);
+   fprintf(stderr, _(git branch --set-upstream-to 
%s\n), branch-name);
+   }
+
} else
usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 93e5d6e..e9e11cf 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -399,6 +399,40 @@ test_expect_success 'test --unset-upstream on a particular 
branch' \
  test_must_fail git config branch.my14.remote 
  test_must_fail git config branch.my14.merge'
 
+test_expect_success '--set-upstream shows message when creating a new branch 
that exists as remote-tracking' \
+'git update-ref refs/remotes/origin/master HEAD 
+ git branch --set-upstream origin/master 2actual 
+ test_when_finished git update-ref -d refs/remotes/origin/master 
+ test_when_finished git branch -d origin/master 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+
+If you wanted to make ''master'' track ''origin/master'', do this:
+
+git branch -d origin/master
+git branch --set-upstream-to origin/master
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with two args only shows the deprecation 
message' \
+'git branch --set-upstream master my13 2actual 
+ test_when_finished git branch --unset-upstream master 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with one arg only shows the deprecation 
message if the branch existed' \
+'git branch --set

Re: Git to use XDG Base Directory Standard

[Carlos Martín Nieto]

Lanoxx lan...@gmx.net writes:

 Hi,

 Git and Gitk are currently using the ~/.gitconfig and ~/.gitk files in
 the $HOME directory. It would be nice to use the XDG Base Directory
 standard for the location instead, see [1] and [2]. Are there any
 plans regarding this standard?

Git does this starting at 1.7.12. See the release notes, e.g. at
https://github.com/git/git/blob/master/Documentation/RelNotes/1.7.12.txt#L18-23

   cmn
--
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 3/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Mon, 2012-08-20 at 11:50 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  This interface is error prone, and a better one (--set-upstream-to)
  exists. Suggest how to fix a --set-upstream invocation in case the
  user only gives one argument, which makes it likely that he meant to
  do the opposite, like with
 
  git branch --set-upstream origin/master
 
  when they meant one of
 
  git branch --set-upstream origin/master master
  git branch --set-upstream-to origin/master
 
 In git branch --set-upstream $A, if $A did not exist (i.e. we
 ended up creating $A that is forked from the current branch), and if
 refs/remotes/$A exists, I agree it is very likely that the user
 wanted to set the upstream of the current branch to remotes/$A
 instead.
 
 But I am not sure about other cases.  If $A already existed, it is
 equally likely that git branch --set-upstream $A wanted to make
 the existing $A track our current branch, or make our branch track
 that existing $A, isn't it?  We would end up giving unwanted advice
 that is *wrong* for the user's situation 50% of the time, which does
 not sound like an acceptable thing to do.
 
 So I would really prefer to see the condition this advice is offered
 limited to very specific cases (namely, only one parameter $A was
 given to cause us use HEAD as the other branch, refs/heads/$A did
 not exist and refs/remotes/$A did; there may be others but I think
 this is the one we most care about) in which we know the advice is
 correct with certainty.

I'm not sure about the 50%, I've personally never used the one-argument
version (correctly), but the restriction does make sense as
--set-upstream origin/master is the main thing we want to protect
against.

 
  While we're at it, add a notice that the --set-upstream flag is
  deprecated and will be removed at some point.
 
 This part is unquestionably good.
 
  Signed-off-by: Carlos Martín Nieto c...@elego.de
 
  ---
 
  This produces suboptimal output in case that A tracks B and we do
 
  git checkout B
  git branch --set-upstream A
 
  as it will suggest
 
  git branch --set-upstream A B
 
  as a way of undoing what we just did.
 
 The literal meaning of what the user did was to create B and then
 made A that existed (and used to build upon B) to build upon B,
 which is a no-op.  And undoing can be done with the same command.
 
 Which is funny.
 
 I am sure you will get complaint from the real users.  To avoid it,
 you would need to know what the old value was (if any), and skip the
 undo part, but I think it is probably worth it.  You already have
 code to learn what the old value was anyway, so the ideal is just a
 single comparison away, no?

Yes, but this comparison is already two or three levels deep in ifs,
which is why I didn't put it in this version, as it was unlikely to be
triggered anyway. As per the next paragraph, this probably won't be an
issue.

 
 By the way, are you assuming that what the user wanted to do was to
 make B build on top of A (i.e. set branch.B.up to A) in this
 example?  For that setting to make sense, perhaps B should already
 be a descendant of A, shouldn't it?  If that is the case, that is
 another clue you can use in your logic to decide if you want to tell
 the user you told me to make $A build on the current branch, but I
 think you meant the other way around with more confidence.

I am pretty much assuming that every use of --set-upstream with a single
argument was meant to use the --set-upstream-to semantics. This
particular examples is an edge-case I found when trying to figure out
the possible combinations, not a sane one I expect to see in the wild.

 
  diff --git a/builtin/branch.c b/builtin/branch.c
  index 08068f7..33641d9 100644
  --- a/builtin/branch.c
  +++ b/builtin/branch.c
  @@ -707,6 +707,21 @@ static int edit_branch_description(const char 
  *branch_name)
  return status;
   }
   
  +static void print_set_upstream_warning(const char *branch, int 
  branch_existed, const char *old_upstream)
  +{
  +   fprintf(stderr, _(If you wanted to make '%s' track '%s', do 
  this:\n\n), head, branch);
 
 I would suggest strongly against using the verb track here, as it
 is overused and has caused confusion is it tracking branch, remote
 tracking branch, do I merge in one but fetch to update the other?.
 
 Regardless of the verb, I think there should be a line _before_ the
 above to tell the user what the command actually _did_, to make the
 distinction stand out to help the user decide.

I don't like the word either, but that's what 'branch --set-upstream'
and 'checkout -t' use (or anything that ends up calling
install_branch_config()). There is such a message (see below) and that's
how it says it. I worry that changing the wording might be even more
confusing, but I'll play with some wording (unless you want to change
the message in install_branch_config()).

 
 That is, tell the user I set upstream for that other branch

[PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

This interface is error prone, and a better one (--set-upstream-to)
exists. Suggest how to fix a --set-upstream invocation in case the
user only gives one argument, which makes it likely that he meant to
do the opposite, like with

git branch --set-upstream origin/master

when they meant one of

git branch --set-upstream origin/master master
git branch --set-upstream-to origin/master

While we're at it, add a notice that the --set-upstream flag is
deprecated and will be removed at some point.

Signed-off-by: Carlos Martín Nieto c...@elego.de

---

This produces suboptimal output in case that A tracks B and we do

git checkout B
git branch --set-upstream A

as it will suggest

git branch --set-upstream A B

as a way of undoing what we just did. Avoiding it becomes a bit messy
(yet another layer of ifs), so I've left it out. Anybody reckon it's
worth recognising this?
---
 builtin/branch.c  | 35 +++
 t/t3200-branch.sh | 52 
 2 files changed, 87 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 08068f7..33641d9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -707,6 +707,21 @@ static int edit_branch_description(const char *branch_name)
return status;
 }
 
+static void print_set_upstream_warning(const char *branch, int branch_existed, 
const char *old_upstream)
+{
+   fprintf(stderr, _(If you wanted to make '%s' track '%s', do 
this:\n\n), head, branch);
+   if (branch_existed) {
+   if (old_upstream)
+   fprintf(stderr, _(git branch --set-upstream %s 
%s\n), old_upstream, branch);
+   else
+   fprintf(stderr, _(git branch --unset-upstream 
%s\n), branch);
+   } else {
+   fprintf(stderr, _(git branch -d %s\n), branch);
+   }
+
+   fprintf(stderr, _(git branch --set-upstream-to %s\n), branch);
+}
+
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
int delete = 0, rename = 0, force_create = 0, list = 0;
@@ -877,10 +892,30 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
git_config_set_multivar(buf.buf, NULL, NULL, 1);
strbuf_release(buf);
} else if (argc  0  argc = 2) {
+   struct branch *branch = branch_get(argv[0]);
+   const char *old_upstream = NULL;
+   int branch_existed = 0;
+
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
+
+   if (track == BRANCH_TRACK_OVERRIDE)
+   fprintf(stderr, _(The --set-upstream flag is 
deprecated and will be removed. Consider using --track or 
--set-upstream-to\n));
+
+   /*
+* Save what argv[0] was pointing to so we can give
+* the --set-upstream-to hint
+*/
+   if (branch_has_merge_config(branch))
+   old_upstream = 
shorten_unambiguous_ref(branch-merge[0]-dst, 0);
+
+   branch_existed = ref_exists(branch-refname);
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
  force_create, reflog, 0, quiet, track);
+
+   if (argc == 1  track == BRANCH_TRACK_OVERRIDE)
+   print_set_upstream_warning(argv[0], branch_existed, 
old_upstream);
+
} else
usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 93e5d6e..702bffa 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -399,6 +399,58 @@ test_expect_success 'test --unset-upstream on a particular 
branch' \
  test_must_fail git config branch.my14.remote 
  test_must_fail git config branch.my14.merge'
 
+test_expect_success 'test --set-upstream help message with one arg' \
+'git branch --set-upstream origin/master 2actual 
+ test_when_finished git branch -d origin/master 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+If you wanted to make ''master'' track ''origin/master'', do this:
+
+git branch -d origin/master
+git branch --set-upstream-to origin/master
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with a branch that already has an 
upstream' \
+'git branch --set-upstream-to my12 master 
+ git branch --set-upstream-to my13 my12 
+ test_when_finished git branch --unset-upstream my12 
+ test_when_finished git branch --unset-upstream my13 
+ git branch --set-upstream my12 2actual 
+ cat actual 
+ cat expected EOF 
+The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
+If you wanted to make ''master'' track ''my12'', do this:
+
+git branch --set

[PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

We have ways of setting the upstream information, but if we want to
unset it, we need to resort to modifying the configuration manually.

Teach branch an --unset-upstream option that unsets this information.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 Documentation/git-branch.txt |  5 +
 builtin/branch.c | 17 ++---
 t/t3200-branch.sh| 16 
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e41c4b5..9c1d2f1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
[(--merged | --no-merged | --contains) [commit]] [pattern...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] branchname 
[start-point]
 'git branch' (--set-upstream-to=upstream | -u upstream) [branchname]
+'git branch' --unset-upstream [branchname]
 'git branch' (-m | -M) [oldbranch] newbranch
 'git branch' (-d | -D) [-r] branchname...
 'git branch' --edit-description [branchname]
@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.
considered branchname's upstream branch. If no branchname
is specified, then it defaults to the current branch.
 
+--unset-upstream::
+   Remove the upstream information for branchname. If no branch
+   is specified it defaults to the current branch.
+
 --edit-description::
Open an editor and edit the text to explain what the branch is
for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 3c978eb..08068f7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
int delete = 0, rename = 0, force_create = 0, list = 0;
int verbose = 0, abbrev = -1, detached = 0;
int reflog = 0, edit_description = 0;
-   int quiet = 0;
+   int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
enum branch_track track;
int kinds = REF_LOCAL_BRANCH;
@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_SET_INT( 0, set-upstream,  track, change upstream info,
BRANCH_TRACK_OVERRIDE),
OPT_STRING('u', set-upstream-to, new_upstream, upstream, 
change the upstream info),
+   OPT_BOOLEAN(0, unset-upstream, unset_upstream, Unset the 
upstream info),
OPT__COLOR(branch_use_color, use colored output),
OPT_SET_INT('r', remotes, kinds, act on remote-tracking 
branches,
REF_REMOTE_BRANCH),
@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 0);
 
-   if (!delete  !rename  !edit_description  !new_upstream  argc == 
0)
+   if (!delete  !rename  !edit_description  !new_upstream  
!unset_upstream  argc == 0)
list = 1;
 
-   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream  1)
+   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
!!unset_upstream  1)
usage_with_options(builtin_branch_usage, options);
 
if (abbrev == -1)
@@ -865,6 +866,16 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 * info and making sure new_upstream is correct
 */
create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
+   } else if (unset_upstream) {
+   struct branch *branch = branch_get(argv[0]);
+   struct strbuf buf = STRBUF_INIT;
+
+   strbuf_addf(buf, branch.%s.remote, branch-name);
+   git_config_set_multivar(buf.buf, NULL, NULL, 1);
+   strbuf_reset(buf);
+   strbuf_addf(buf, branch.%s.merge, branch-name);
+   git_config_set_multivar(buf.buf, NULL, NULL, 1);
+   strbuf_release(buf);
} else if (argc  0  argc = 2) {
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..93e5d6e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a 
particular branch' \
  test $(git config branch.my13.remote) = . 
  test $(git config branch.my13.merge) = refs/heads/master'
 
+test_expect_success 'test --unset-upstream on HEAD' \
+'git branch my14
+ test_config branch.master.remote foo 
+ test_config branch.master.merge foo 
+ git branch --set-upstream-to my14 
+ git branch --unset-upstream 
+ test_must_fail git config branch.master.remote

[PATCH 0/3] Improve branch UI for setting upstream information

[Carlos Martín Nieto]

Hi all,

After way too long, here's the next iteration of the concept that
began with swapping arguments in --set-upstream like -m does.

After the feedback from the list, --set-upstream-to was born and
--set-upstream is being deprecated in favour of either --track or
--set-upstream-to depening on which of the behaviours the user wants
to have.

Using --set-upsteam with one argument now also leads to a message
explaining how to undo it. For that, branch has learnt
--unset-upstream which will remove the upstream information for the
given branch (or HEAD).

Carlos Martín Nieto (3):
  branch: introduce --set-upstream-to
  branch: add --unset-upstream option
  branch: suggest how to undo a --set-upstream when given one branch

 Documentation/git-branch.txt | 14 +++-
 builtin/branch.c | 65 +--
 t/t3200-branch.sh| 82 
 3 files changed, 157 insertions(+), 4 deletions(-)

-- 
1.7.11.1.104.ge7b44f1

--
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 1/3] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

The existing --set-uptream option can cause confusion, as it uses the
usual branch convention of assuming a starting point of HEAD if none
is specified, causing

git branch --set-upstream origin/master

to create a new local branch 'origin/master' that tracks the current
branch. As --set-upstream already exists, we can't simply change its
behaviour. To work around this, introduce --set-upstream-to which
accepts a compulsory argument indicating what the new upstream branch
should be and one optinal argument indicating which branch to change,
defaulting to HEAD.

The new options allows us to type

git branch --set-upstream-to origin/master

to set the current branch's upstream to be origin's master.

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 Documentation/git-branch.txt |  9 -
 builtin/branch.c | 17 +++--
 t/t3200-branch.sh| 14 ++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 47235be..e41c4b5 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -13,6 +13,7 @@ SYNOPSIS
[--column[=options] | --no-column]
[(--merged | --no-merged | --contains) [commit]] [pattern...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] branchname 
[start-point]
+'git branch' (--set-upstream-to=upstream | -u upstream) [branchname]
 'git branch' (-m | -M) [oldbranch] newbranch
 'git branch' (-d | -D) [-r] branchname...
 'git branch' --edit-description [branchname]
@@ -48,7 +49,7 @@ branch so that 'git pull' will appropriately merge from
 the remote-tracking branch. This behavior may be changed via the global
 `branch.autosetupmerge` configuration flag. That setting can be
 overridden by using the `--track` and `--no-track` options, and
-changed later using `git branch --set-upstream`.
+changed later using `git branch --set-upstream-to`.
 
 With a `-m` or `-M` option, oldbranch will be renamed to newbranch.
 If oldbranch had a corresponding reflog, it is renamed to match
@@ -173,6 +174,12 @@ start-point is either a local or remote-tracking branch.
like `--track` would when creating the branch, except that where
branch points to is not changed.
 
+-u upstream::
+--set-upstream-to=upstream::
+   Set up branchname's tracking information so upstream is
+   considered branchname's upstream branch. If no branchname
+   is specified, then it defaults to the current branch.
+
 --edit-description::
Open an editor and edit the text to explain what the branch is
for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..3c978eb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -713,6 +713,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
int verbose = 0, abbrev = -1, detached = 0;
int reflog = 0, edit_description = 0;
int quiet = 0;
+   const char *new_upstream = NULL;
enum branch_track track;
int kinds = REF_LOCAL_BRANCH;
struct commit_list *with_commit = NULL;
@@ -726,6 +727,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
BRANCH_TRACK_EXPLICIT),
OPT_SET_INT( 0, set-upstream,  track, change upstream info,
BRANCH_TRACK_OVERRIDE),
+   OPT_STRING('u', set-upstream-to, new_upstream, upstream, 
change the upstream info),
OPT__COLOR(branch_use_color, use colored output),
OPT_SET_INT('r', remotes, kinds, act on remote-tracking 
branches,
REF_REMOTE_BRANCH),
@@ -794,10 +796,10 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 0);
 
-   if (!delete  !rename  !edit_description  argc == 0)
+   if (!delete  !rename  !edit_description  !new_upstream  argc == 
0)
list = 1;
 
-   if (!!delete + !!rename + !!force_create + !!list  1)
+   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream  1)
usage_with_options(builtin_branch_usage, options);
 
if (abbrev == -1)
@@ -852,6 +854,17 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
rename_branch(argv[0], argv[1], rename  1);
else
usage_with_options(builtin_branch_usage, options);
+   } else if (new_upstream) {
+   struct branch *branch = branch_get(argv[0]);
+
+   if (!ref_exists(branch-refname))
+   die(_(branch '%s' does not exist), branch-name);
+
+   /*
+* create_branch takes care of setting up the tracking
+* info and making sure new_upstream is correct

Re: inconsistent logs when displayed on screen / piped to a file

[Carlos Martín Nieto]

On Mon, 2012-07-30 at 15:39 +0200, Michael J Gruber wrote:
 a) probes your terminal for the number of columns and uses all available
 space.
 
 b) goes to a file and has no connected terminal, thus uses a default
 column number. You can change that number using
 
 COLUMNS=YourNumber git log YourArgs  YourFile

You can also pass a width to --stat. See the git log manpage for details
about which widths you can override.

   cmn


--
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] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

On Tue, 2012-07-17 at 22:56 -0700, Junio C Hamano wrote:
 Ping on a seemingly stalled discussion (no need to rush but just
 checking).

I've implemented the feedback, but been slacking on writing the tests
which is what's stopped me from re-sending the series.


   cmn


--
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: git rebase -i does not rebase if all lines are removed

[Carlos Martín Nieto]

On Tue, 2012-07-17 at 13:46 +0300, Orgad and Raizel Shaneh wrote:
 Make a commit on top of master.
 
 git rebase -i origin/master
 
 Remove the commit.
 
 Git prints Nothing to do and does not rebase.
 
 Running 'git rebase -i' when there are no local commits has 'noop' in
 the first line, and with it the rebase is successful. Why is this
 'noop' mandatory?

If you read the instructions, the last line says

# However, if you remove everything, the rebase will be aborted

so if you want to do a no-op, then you need to tell it. This is the same
way you abort a commit, by providing it with an empty message.

But more important would be /why/ you feel that rebase -i is the tool
you should be using. If you'd like to move the branch pointer back,
that's what the reset command is for. rebase deals with moving commits
from one base to another and optionally reordering, squashing or
removing some of them.

   cmn


--
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 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 19:20 +0200, Matthieu Moy wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  --- a/builtin/branch.c
  +++ b/builtin/branch.c
  @@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const 
  char *prefix)
 info and making sure new_upstream is correct */
  create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
  BRANCH_TRACK_OVERRIDE);
  } else if (argc  0  argc = 2) {
  +   struct branch *branch = branch_get(argv[0]);
  +   const char *old_upstream = NULL;
  +   int branch_existed = 0;
  +
  if (kinds != REF_LOCAL_BRANCH)
  die(_(-a and -r options to 'git branch' do not make 
  sense with a branch name));
  +
  +   /* Save what argv[0] was pointing to so we can give
  +  the --set-upstream-to hint */
 
 Multi-line comments are usually written in Git as
 
 /*
  * multi-line
  * comment
  */

I've seen this style often, but sure.

 
  +   if (branch_has_merge_config(branch))
  + old_upstream = shorten_unambiguous_ref(branch-merge[0]-dst, 
  0);
 
 Broken indentation.

Yeah, sorry. New laptop, hadn't got the default style fixed in the
config.

 
  +   if (argc == 1) {
  +   printf(If you wanted to make '%s' track '%s', do 
  this:\n, head, argv[0]);
 
 Could be marked for translation with _(...).

Done.

 
  +   if (branch_existed)
  +   printf( $ git branch --set-upstream '%s' 
  '%s'\n, argv[0], old_upstream);
 
 old_upstream may be NULL at this point. I guess you want to skip this
 line if old_upsteam is NULL.

We've just set up tracking for it, so we'd want to undo that. Which
means --unset-upstream would have to move earlier in the series so we
can suggest that.

 
 The fact that I could find this bug suggests that this lacks a few new
 tests too ;-).

Indeed :) the next round will have them.

 
  +   else
  +   printf( $ git branch -d '%s'\n, argv[0]);
  +
  +   printf( $ git branch --set-upstream-to '%s'\n, 
  argv[0]);
 
 For the 3 printf()s: we usually display commands without the $, and
 separate them from text with a blank line. See for example what git
 commit says when you didn't provide authorship:

Yeah, I was going by what Junio wrote in his mail. We should probably
have a double-LF as well, like in the message below.

 
 You can suppress this message by setting them explicitly:
 
 git config --global user.name Your Name
 git config --global user.email y...@example.com
 
 After doing this, you may fix the identity used for this commit with:
 
 git commit --amend --reset-author
 
 (the absence of $ sign avoids the temptation to cut-and-paste it)
 



--
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 3/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 11:02 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  We have ways of setting the upstream information, but if we want to
  unset it, we need to resort to modifying the configuration manually.
 
  Teach branch an --unset-upstream option that unsets this information.
 
  ---
 
  create_branch() uses install_branch_config() which may also set
  branch.foo.rebase, so this version might leave some configuration
  laying around.
 
  I wonder if deleting the whole branch.foo section would be better. Can
  we be sure that nothing else shows up there?
 
 branch.foo.$unknown may not be related to upstream at all, so that
 will not fly.  Besides, we already have unknown=description, no?

Ah yes, that exists. I've added a bit of code to also remove
branch.foo.rebase, which I'd also consider to be part of the upstream
information.

   cmn


--
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 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 10:40 -0700, Junio C Hamano wrote:
 Carlos Martín Nieto c...@elego.de writes:
 
  This interface is error prone, and a better one (--set-upstream-to)
  exists. Suggest how to fix a --set-upstream invocation in case the
  user only gives one argument, which makes it likely that he meant to
  do the opposite, like with
 
  git branch --set-upstream origin/master
 
  when they meant one of
 
  git branch --set-upstream origin/master master
  git branch --set-upstream-to origin/master
 
  Signed-off-by: Carlos Martín Nieto c...@elego.de
 
 The new code does not seem to depend on the value of track (which
 is set by either -t or --set-upstream) in any way.  Shouldn't it be
 done only when it is set to track-override?

Yes, yes it should.

 
 Doesn't git branch [-f] frotz without any other argument trigger
 the warning?

It does. Oops. Fixed.

   cmn


--
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 2/3] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

On Tue, 2012-07-10 at 18:00 -0500, Jonathan Nieder wrote:
 Junio C Hamano wrote:
 
I think it
  is better to leave them emitted unconditionally to the standard
  error stream, in order to train users away from using the old option
  that has its arguments wrong (the option does not take an argument
  it should, and makes the command line to look as if it takes two
  branch arguments in the wrong order).
 
 I thought we already discussed that that is a side-issue?

The current --set-upstream is the whole reason for this series existing.

 
 The option is a mode option for the command, like -m, -d, or
 --edit-description.  I genuinely don't think the order of options it
 takes is counter-intuitive.  The second argument defaulting to HEAD
 and the behavior of creating the branch named by the first argument
 when it does not exist are quite counter-intuitive.

This is confusing. First you say that you don't think it's
counter-intuitive but then you say it is? Or is the first part about -m
and -d?

The second part of the paragraph is indeed what I'm trying to solve with
this series. If you want to create a new branch, you should be using -t.

 
 Transitioning to a different argument order seems like it would just
 make the command more complicated.  After the transition, there are
 two options to explain, and during the transition, it is easy to make
 scripts with gratuitous incompatibilities that won't work on older
 systems.
 
 Where is my thinking going wrong?

We're not transitioning to a new order as such, particularly not with
the same option name. The incompatibilities would ensue with the other
patch I send which did change the order for --set-upstream, but what
this does is _add_ --set-upstream-to=upstream such that the option
takes one argument and the command takes one optional argument, which
makes it closer to what one would expect, specially as changing the
upstream information is something you're most likely to do on the
current branch.

   cmn


--
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] A better way of handling upstream information in git-branch

[Carlos Martín Nieto]

Hello all,

This stems from comments made by Junio and Jonathan about my proposed
changes to --set-upstream.

This should probably have a few tests, but I'd like to hear comments
about the code and documentation first. The third patch is the one I'm
not so confident about. It would be simpler to remove the whole
branch.foo configuration, but that wouldn't be very safe, as we may
have more things there (either the future git or some external tool).

Carlos Martín Nieto (3):
  branch: introduce --set-upstream-to
  branch: suggest how to undo a --set-upstream when given one branch
  branch: add --unset-upstream option

 Documentation/git-branch.txt |   14 +++-
 builtin/branch.c |   50 +++---
 2 files changed, 60 insertions(+), 4 deletions(-)

-- 
1.7.10.2.1.g8c77c3c

--
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] branch: suggest how to undo a --set-upstream when given one branch

[Carlos Martín Nieto]

This interface is error prone, and a better one (--set-upstream-to)
exists. Suggest how to fix a --set-upstream invocation in case the
user only gives one argument, which makes it likely that he meant to
do the opposite, like with

git branch --set-upstream origin/master

when they meant one of

git branch --set-upstream origin/master master
git branch --set-upstream-to origin/master

Signed-off-by: Carlos Martín Nieto c...@elego.de
---
 builtin/branch.c |   22 ++
 1 file changed, 22 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index c886fc0..5551227 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
   info and making sure new_upstream is correct */
create_branch(head, branch-name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
} else if (argc  0  argc = 2) {
+   struct branch *branch = branch_get(argv[0]);
+   const char *old_upstream = NULL;
+   int branch_existed = 0;
+
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
+
+   /* Save what argv[0] was pointing to so we can give
+  the --set-upstream-to hint */
+   if (branch_has_merge_config(branch))
+ old_upstream = shorten_unambiguous_ref(branch-merge[0]-dst, 
0);
+
+   branch_existed = ref_exists(branch-refname);
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
  force_create, reflog, 0, quiet, track);
+
+   if (argc == 1) {
+   printf(If you wanted to make '%s' track '%s', do 
this:\n, head, argv[0]);
+   if (branch_existed)
+   printf( $ git branch --set-upstream '%s' 
'%s'\n, argv[0], old_upstream);
+   else
+   printf( $ git branch -d '%s'\n, argv[0]);
+
+   printf( $ git branch --set-upstream-to '%s'\n, 
argv[0]);
+   }
+
} else
usage_with_options(builtin_branch_usage, options);
 
-- 
1.7.10.2.1.g8c77c3c

--
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] branch: add -u as a shortcut for --set-upstream

[Carlos Martín Nieto]

On Fri, 2012-07-06 at 10:32 -0500, Jonathan Nieder wrote:
 Hi Carlos,
 
 Carlos Martín Nieto wrote:
 
  Add this shortcut just like git-push has it.
 [...]
  --- a/builtin/branch.c
  +++ b/builtin/branch.c
  @@ -724,7 +724,7 @@ int cmd_branch(int argc, const char **argv, const char 
  *prefix)
  OPT__QUIET(quiet, suppress informational messages),
  OPT_SET_INT('t', track,  track, set up tracking mode (see 
  git-pull(1)),
  BRANCH_TRACK_EXPLICIT),
  -   OPT_SET_INT( 0, set-upstream,  track, change upstream info,
  +   OPT_SET_INT('u', set-upstream,  track, change upstream 
  info,
  BRANCH_TRACK_OVERRIDE),
 
 I think this is a bad idea.  The --set-upstream option is confusing:
 
   $ git branch --set-upstream=foo
   error: option 'set-upstream' takes no value
   $ ???

It is confusing, see the other thread (about making --set-upstream
behave more sanely) for the second part of this. I wanted to add a hack
so

git branch --set-upstream foo

would set the current branch to track foo.

 
 -u to set the corresponding upstream branch at the same time as
 creating a branch, analagous to git push -u might seem intuitive:
 
   # create foo branch, setting its upstream at the same time
   git branch -u foo origin/foo
 
 But that is what --track does and is not what --set-upstream is for.

Right, it's for altering the configuration after the branch has already
been created. I wasn't thinking about creating the branch. That usage of
--set-upstream seems unintuitive to me, but changing the upstream for a
branch that already exists is quite intuitive and what push -u does.

 
 Unlike --track, I don't think --set-upstream is a common enough
 operation to deserve a one-letter synonym.

I find myself using it surprisingly often, though it's certainly still
not in the top-10.

 
 Instead, I'd suggest the following changes:
 
  1) Add support for
 
   # change upstream info
   git branch --set-upstream=origin/foo foo
 
 for existing branches only.

I submitted the patch I mentioned above which changes --set-upstream to
something closer to what the user probably expects, which behaves mostly
like this, except that you'd have to omit the '=' as it still expected
the branch as an argument to the command. Not the cleanest wrt how
options should take arguments, but it got the job done without much code
churn.

However, Junio doesn't like that it changes existing behaviour which is
even consistent (as long as you know that --set-upstream is a flag,
rather than an option with an argument) and some people might depend on
it behaving like it does with a single argument. He'd accept a
--set-upstream-to that behaves just like you describe. This could do
with having a short alias, as the name is quite long-winded. Another
reason for adding -u is that it would add confusion if --set-upstream
has the short alias -u in push, but it means something else in branch,
even though they have the same option (though it would be
--set-upstream-to here, but we'll end up deprecating --set-upstream, so
it works out in the long run).

 
  2) Introduce an --unset-upstream option which removes the
 corresponding upstream branch configuration for an existing
 branch.

This is a good idea. I'll add it to the patch series.

 
  3) Warn on
 
   # acts just like --track
   git branch --set-upstream foo origin/foo
 
 for branches that do not exist yet.  Plan to make this a hard
 error in the future.
 
  4) Warn on
 
   # sets upstream for foo to the current branch
   git branch --set-upstream foo
 
 and plan to make it a hard error in the future.
 
  5) Warn on
 
   git branch --set-upstream origin/foo foo
 
   which is almost certainly a typo for
 
   git branch --set-upstream=origin/foo foo

This is roughly what Junio suggested. His proposal was to also show how
to undo the --set-upstream if it was invoked the wrong way.

 
  6) Perhaps, make -u a synonym for -t for consistency with git push.

I don't really see -u and -t being consistent with push -u. The push
might create a branch, but that would be in another repository. I look
at it as modifying the upstream information for an existing branch in
the local repository, and -t does not do that, --set-upstream(-to) does
that. It can also create a new local branch, but that's another bug in
the interface.

   cmn


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