All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
@ 2014-01-07 20:29 Ramkumar Ramachandra
  2014-01-07 20:30 ` Ramkumar Ramachandra
  2014-01-07 20:36 ` [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Junio C Hamano
  0 siblings, 2 replies; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-07 20:29 UTC (permalink / raw)
  To: Git List; +Cc: Jeff King, Junio C Hamano

A very common workflow for preparing patches involves working off a
topic branch and generating patches against 'master' to send off to the
maintainer. However, a plain

  $ git format-patch -o outgoing

is a no-op on a topic branch, and the user has to remember to specify
'master' explicitly everytime. This problem is not unique to
format-patch; even a

  $ git rebase -i

is a no-op because the branch to rebase against isn't specified.

To tackle this problem, introduce branch.*.forkedFrom which can specify
the parent branch of a topic branch. Future patches will build
functionality around this new configuration variable.

Cc: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gister@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Since -M, -C, -D are left in the argc, checking argc < 2 isn't
 sufficient.

 I wanted to get an early reaction before wiring up checkout and
 rebase.

 But I wanted to discuss the overall idea of the patch.
 builtin/log.c           | 21 +++++++++++++++++++++
 t/t4014-format-patch.sh | 20 ++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index b97373d..525e696 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -674,6 +674,7 @@ static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
 static int config_cover_letter;
+static const char *config_base_branch;
 
 enum {
 	COVER_UNSET,
@@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
 		return 0;
 	}
+	if (starts_with(var, "branch.")) {
+		const char *name = var + 7;
+		const char *subkey = strrchr(name, '.');
+		struct strbuf buf = STRBUF_INIT;
+
+		if (!subkey)
+			return 0;
+		strbuf_add(&buf, name, subkey - name);
+		if (branch_get(buf.buf) != branch_get(NULL))
+			return 0;
+		strbuf_release(&buf);
+		if (!strcmp(subkey, ".forkedfrom")) {
+			if (git_config_string(&config_base_branch, var, value))
+				return -1;
+		}
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die (_("--subject-prefix and -k are mutually exclusive."));
 	rev.preserve_subject = keep_subject;
 
+	if (argc < 2 && config_base_branch) {
+		argv[1] = config_base_branch;
+		argc++;
+	}
 	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
 	if (argc > 1)
 		die (_("unrecognized argument: %s"), argv[1]);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 73194b2..2ea94af 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
 	test_line_count = 2 list
 '
 
+test_expect_success 'branch.*.forkedFrom matches' '
+	mkdir -p tmp &&
+	test_when_finished "rm -rf tmp;
+		git config --unset branch.rebuild-1.forkedFrom" &&
+
+	git config branch.rebuild-1.forkedFrom master &&
+	git format-patch -o tmp >list &&
+	test_line_count = 2 list
+'
+
+test_expect_success 'branch.*.forkedFrom does not match' '
+	mkdir -p tmp &&
+	test_when_finished "rm -rf tmp;
+		git config --unset branch.foo.forkedFrom" &&
+
+	git config branch.foo.forkedFrom master &&
+	git format-patch -o tmp >list &&
+	test_line_count = 0 list
+'
+
 test_done
-- 
1.8.5.2.234.gba2dde8.dirty

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:29 [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Ramkumar Ramachandra
@ 2014-01-07 20:30 ` Ramkumar Ramachandra
  2014-01-07 20:40   ` Jeff King
  2014-01-07 20:36 ` [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Junio C Hamano
  1 sibling, 1 reply; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-07 20:30 UTC (permalink / raw)
  To: Git List; +Cc: Jeff King, Junio C Hamano

[Fixed typo in Junio's address]

On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> A very common workflow for preparing patches involves working off a
> topic branch and generating patches against 'master' to send off to the
> maintainer. However, a plain
>
>   $ git format-patch -o outgoing
>
> is a no-op on a topic branch, and the user has to remember to specify
> 'master' explicitly everytime. This problem is not unique to
> format-patch; even a
>
>   $ git rebase -i
>
> is a no-op because the branch to rebase against isn't specified.
>
> To tackle this problem, introduce branch.*.forkedFrom which can specify
> the parent branch of a topic branch. Future patches will build
> functionality around this new configuration variable.
>
> Cc: Jeff King <peff@peff.net>
> Cc: Junio C Hamano <gister@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Since -M, -C, -D are left in the argc, checking argc < 2 isn't
>  sufficient.
>
>  I wanted to get an early reaction before wiring up checkout and
>  rebase.
>
>  But I wanted to discuss the overall idea of the patch.
>  builtin/log.c           | 21 +++++++++++++++++++++
>  t/t4014-format-patch.sh | 20 ++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b97373d..525e696 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -674,6 +674,7 @@ static int thread;
>  static int do_signoff;
>  static const char *signature = git_version_string;
>  static int config_cover_letter;
> +static const char *config_base_branch;
>
>  enum {
>         COVER_UNSET,
> @@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char *value, void *cb)
>                 config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
>                 return 0;
>         }
> +       if (starts_with(var, "branch.")) {
> +               const char *name = var + 7;
> +               const char *subkey = strrchr(name, '.');
> +               struct strbuf buf = STRBUF_INIT;
> +
> +               if (!subkey)
> +                       return 0;
> +               strbuf_add(&buf, name, subkey - name);
> +               if (branch_get(buf.buf) != branch_get(NULL))
> +                       return 0;
> +               strbuf_release(&buf);
> +               if (!strcmp(subkey, ".forkedfrom")) {
> +                       if (git_config_string(&config_base_branch, var, value))
> +                               return -1;
> +               }
> +       }
>
>         return git_log_config(var, value, cb);
>  }
> @@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>                 die (_("--subject-prefix and -k are mutually exclusive."));
>         rev.preserve_subject = keep_subject;
>
> +       if (argc < 2 && config_base_branch) {
> +               argv[1] = config_base_branch;
> +               argc++;
> +       }
>         argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>         if (argc > 1)
>                 die (_("unrecognized argument: %s"), argv[1]);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 73194b2..2ea94af 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
>         test_line_count = 2 list
>  '
>
> +test_expect_success 'branch.*.forkedFrom matches' '
> +       mkdir -p tmp &&
> +       test_when_finished "rm -rf tmp;
> +               git config --unset branch.rebuild-1.forkedFrom" &&
> +
> +       git config branch.rebuild-1.forkedFrom master &&
> +       git format-patch -o tmp >list &&
> +       test_line_count = 2 list
> +'
> +
> +test_expect_success 'branch.*.forkedFrom does not match' '
> +       mkdir -p tmp &&
> +       test_when_finished "rm -rf tmp;
> +               git config --unset branch.foo.forkedFrom" &&
> +
> +       git config branch.foo.forkedFrom master &&
> +       git format-patch -o tmp >list &&
> +       test_line_count = 0 list
> +'
> +
>  test_done
> --
> 1.8.5.2.234.gba2dde8.dirty
>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:29 [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Ramkumar Ramachandra
  2014-01-07 20:30 ` Ramkumar Ramachandra
@ 2014-01-07 20:36 ` Junio C Hamano
  2014-01-07 20:40   ` Ramkumar Ramachandra
  1 sibling, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2014-01-07 20:36 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Jeff King, Junio C Hamano

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> A very common workflow for preparing patches involves working off a
> topic branch and generating patches against 'master' to send off to the
> maintainer. However, a plain
>
>   $ git format-patch -o outgoing
>
> is a no-op on a topic branch, and the user has to remember to specify
> 'master' explicitly everytime. This problem is not unique to
> format-patch; even a
>
>   $ git rebase -i
>
> is a no-op because the branch to rebase against isn't specified.
>
> To tackle this problem, introduce branch.*.forkedFrom which can specify
> the parent branch of a topic branch. Future patches will build
> functionality around this new configuration variable.
>

I do not mind allowing laziness by defaulting to something, but I am
not enthused by an approach that adds the new variable whose value
is questionable.  The description does not justify at all why
@{upstream} is not a good default (unless the workflow is screwed up
and @{upstream} is set to point at somewhere that is _not_ a true
upstream, that is).

> Cc: Jeff King <peff@peff.net>
> Cc: Junio C Hamano <gister@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Since -M, -C, -D are left in the argc, checking argc < 2 isn't
>  sufficient.
>
>  I wanted to get an early reaction before wiring up checkout and
>  rebase.
>
>  But I wanted to discuss the overall idea of the patch.
>  builtin/log.c           | 21 +++++++++++++++++++++
>  t/t4014-format-patch.sh | 20 ++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b97373d..525e696 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -674,6 +674,7 @@ static int thread;
>  static int do_signoff;
>  static const char *signature = git_version_string;
>  static int config_cover_letter;
> +static const char *config_base_branch;
>  
>  enum {
>  	COVER_UNSET,
> @@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char *value, void *cb)
>  		config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
>  		return 0;
>  	}
> +	if (starts_with(var, "branch.")) {
> +		const char *name = var + 7;
> +		const char *subkey = strrchr(name, '.');
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (!subkey)
> +			return 0;
> +		strbuf_add(&buf, name, subkey - name);
> +		if (branch_get(buf.buf) != branch_get(NULL))
> +			return 0;
> +		strbuf_release(&buf);
> +		if (!strcmp(subkey, ".forkedfrom")) {
> +			if (git_config_string(&config_base_branch, var, value))
> +				return -1;
> +		}
> +	}
>  
>  	return git_log_config(var, value, cb);
>  }
> @@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die (_("--subject-prefix and -k are mutually exclusive."));
>  	rev.preserve_subject = keep_subject;
>  
> +	if (argc < 2 && config_base_branch) {
> +		argv[1] = config_base_branch;
> +		argc++;
> +	}
>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>  	if (argc > 1)
>  		die (_("unrecognized argument: %s"), argv[1]);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 73194b2..2ea94af 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
>  	test_line_count = 2 list
>  '
>  
> +test_expect_success 'branch.*.forkedFrom matches' '
> +	mkdir -p tmp &&
> +	test_when_finished "rm -rf tmp;
> +		git config --unset branch.rebuild-1.forkedFrom" &&
> +
> +	git config branch.rebuild-1.forkedFrom master &&
> +	git format-patch -o tmp >list &&
> +	test_line_count = 2 list
> +'
> +
> +test_expect_success 'branch.*.forkedFrom does not match' '
> +	mkdir -p tmp &&
> +	test_when_finished "rm -rf tmp;
> +		git config --unset branch.foo.forkedFrom" &&
> +
> +	git config branch.foo.forkedFrom master &&
> +	git format-patch -o tmp >list &&
> +	test_line_count = 0 list
> +'
> +
>  test_done

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:30 ` Ramkumar Ramachandra
@ 2014-01-07 20:40   ` Jeff King
  2014-01-07 20:48     ` Junio C Hamano
  2014-01-07 21:02     ` Ramkumar Ramachandra
  0 siblings, 2 replies; 37+ messages in thread
From: Jeff King @ 2014-01-07 20:40 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

On Wed, Jan 08, 2014 at 02:00:44AM +0530, Ramkumar Ramachandra wrote:

> On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> > A very common workflow for preparing patches involves working off a
> > topic branch and generating patches against 'master' to send off to the
> > maintainer. However, a plain
> >
> >   $ git format-patch -o outgoing
> >
> > is a no-op on a topic branch, and the user has to remember to specify
> > 'master' explicitly everytime. This problem is not unique to
> > format-patch; even a
> >
> >   $ git rebase -i
> >
> > is a no-op because the branch to rebase against isn't specified.
> >
> > To tackle this problem, introduce branch.*.forkedFrom which can specify
> > the parent branch of a topic branch. Future patches will build
> > functionality around this new configuration variable.
> >
> > Cc: Jeff King <peff@peff.net>
> > Cc: Junio C Hamano <gister@pobox.com>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

I have not carefully read some of the later bits of the discussion from
last night / this morning, so maybe I am missing something, but this
seems backwards to me from what Junio and I were discussing earlier.

The point was that the meaning of "@{upstream}" (and "branch.*.merge")
is _already_ "forked-from", and "push -u" and "push.default=upstream"
are the odd men out. If we are going to add an option to distinguish the
two branch relationships:

  1. Where you forked from

  2. Where you push to

we should leave @{upstream} as (1), and add a new option to represent
(2). Not the other way around.

Am I missing something?

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:36 ` [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Junio C Hamano
@ 2014-01-07 20:40   ` Ramkumar Ramachandra
  2014-01-07 20:42     ` Junio C Hamano
  0 siblings, 1 reply; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-07 20:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Jeff King, Junio C Hamano

Junio C Hamano wrote:
> I do not mind allowing laziness by defaulting to something, but I am
> not enthused by an approach that adds the new variable whose value
> is questionable.  The description does not justify at all why
> @{upstream} is not a good default (unless the workflow is screwed up
> and @{upstream} is set to point at somewhere that is _not_ a true
> upstream, that is).

Did you find the explanation I gave in
http://article.gmane.org/gmane.comp.version-control.git/240077
reasonable? I don't know why label the respin-workflow as being
"screwed up".

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:40   ` Ramkumar Ramachandra
@ 2014-01-07 20:42     ` Junio C Hamano
  0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-01-07 20:42 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Jeff King, Junio C Hamano

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> I do not mind allowing laziness by defaulting to something, but I am
>> not enthused by an approach that adds the new variable whose value
>> is questionable.  The description does not justify at all why
>> @{upstream} is not a good default (unless the workflow is screwed up
>> and @{upstream} is set to point at somewhere that is _not_ a true
>> upstream, that is).
>
> Did you find the explanation I gave in
> http://article.gmane.org/gmane.comp.version-control.git/240077
> reasonable?

No.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:40   ` Jeff King
@ 2014-01-07 20:48     ` Junio C Hamano
  2014-01-07 21:02     ` Ramkumar Ramachandra
  1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-01-07 20:48 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

> The point was that the meaning of "@{upstream}" (and "branch.*.merge")
> is _already_ "forked-from", and "push -u" and "push.default=upstream"
> are the odd men out. If we are going to add an option to distinguish the
> two branch relationships:
>
>   1. Where you forked from
>
>   2. Where you push to
>
> we should leave @{upstream} as (1), and add a new option to represent
> (2). Not the other way around.

That matches my feeling as well.

I am not sure if "push -u" is truly odd man out, though.  It was an
invention back in the "you fetch from and push to the same place and
there is no other workflow support" days, and in that context, the
"upstream" meant just that: the place you fetch from, which happens
to be the same as where you are pushing to right now.  If "push -u"
suddenly stopped setting the configuration to merge back from where
it is pushing, that would regress for centralized folks, so I am not
sure how it could be extended to also support triangular folks, but
I do think @{upstream} should mean "this is where I sync with to
stay abreast with others".

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 20:40   ` Jeff King
  2014-01-07 20:48     ` Junio C Hamano
@ 2014-01-07 21:02     ` Ramkumar Ramachandra
  2014-01-07 21:16       ` Jeff King
  1 sibling, 1 reply; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-07 21:02 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

Jeff King wrote:
> I have not carefully read some of the later bits of the discussion from
> last night / this morning, so maybe I am missing something, but this
> seems backwards to me from what Junio and I were discussing earlier.
>
> The point was that the meaning of "@{upstream}" (and "branch.*.merge")
> is _already_ "forked-from", and "push -u" and "push.default=upstream"
> are the odd men out. If we are going to add an option to distinguish the
> two branch relationships:
>
>   1. Where you forked from
>
>   2. Where you push to
>
> we should leave @{upstream} as (1), and add a new option to represent
> (2). Not the other way around.

I have a local branch 'forkedfrom' that has two "sources": 'master'
and 'ram/forkedfrom'. 'ram/forkedfrom' isn't a "dumb" publish-point:
the relationship information I get between 'forkedfrom' and
'ram/forkedfrom' is very useful; it's what helps me tell how my
re-roll is doing with respect to the original series; I'd often want
to cherry-pick commits/ messages from my original series to prepare
the re-roll, so interaction with this source is quite high. On the
other hand, the relationship information I get between 'forkedfrom'
and 'master' is practically useless: 'forkedfrom' is always ahead of
'master', and a divergence indicates that I need to rebase; I'll never
really need to interact with this source.

I'm only thinking in terms of what infrastructure we've already built:
if @{u} is set to 'ram/forkedfrom', we get a lot of information for
free _now_. If @{u} is set to 'master', the current git-status is
unhelpful.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 21:02     ` Ramkumar Ramachandra
@ 2014-01-07 21:16       ` Jeff King
  2014-01-07 21:35         ` Ramkumar Ramachandra
  0 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2014-01-07 21:16 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

On Wed, Jan 08, 2014 at 02:32:10AM +0530, Ramkumar Ramachandra wrote:

> > we should leave @{upstream} as (1), and add a new option to represent
> > (2). Not the other way around.
> 
> I have a local branch 'forkedfrom' that has two "sources": 'master'
> and 'ram/forkedfrom'. 'ram/forkedfrom' isn't a "dumb" publish-point:
> the relationship information I get between 'forkedfrom' and
> 'ram/forkedfrom' is very useful; it's what helps me tell how my
> re-roll is doing with respect to the original series; I'd often want
> to cherry-pick commits/ messages from my original series to prepare
> the re-roll, so interaction with this source is quite high. On the
> other hand, the relationship information I get between 'forkedfrom'
> and 'master' is practically useless: 'forkedfrom' is always ahead of
> 'master', and a divergence indicates that I need to rebase; I'll never
> really need to interact with this source.

Thanks for a concrete example.

I definitely respect the desire to reuse the existing tooling we have
for @{u}. At the same time, I think you are warping the meaning of
@{u} somewhat. It is _not_ your upstream here, but rather another
version of the branch that has useful changes in it. That might be
splitting hairs a bit, but I think you will find that the differences
leak through in inconvenient spots (like format-patch, where you really
_do_ want to default to the true upstream).

If we add "@{publish}" (and "@{pu}"), then it becomes very convenient to
refer to the ram/ version of your branch. That seems like an obvious
first step to me. We don't have to add new config, because
"branch.*.pushremote" already handles this.

Now you can do "git rebase @{pu}" which is nice, but not _quite_ as nice
as "git rebase", which defaults to "@{u}". That first step might be
enough, and I'd hold off there and try it out for a few days or weeks
first. But if you find in your workflow that you are having to specify
"@{pu}" a lot, then maybe it is worth adding an option to default rebase
to "@{pu}" instead of "@{u}".

You end up in the same place ("git rebase" without options does what you
want), but I think the underlying data more accurately represents what
is going on (and there is no need to teach "format-patch" anything
special).

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom
  2014-01-07 21:16       ` Jeff King
@ 2014-01-07 21:35         ` Ramkumar Ramachandra
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
  0 siblings, 1 reply; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-07 21:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

Jeff King wrote:
> I definitely respect the desire to reuse the existing tooling we have
> for @{u}. At the same time, I think you are warping the meaning of
> @{u} somewhat. It is _not_ your upstream here, but rather another
> version of the branch that has useful changes in it. That might be
> splitting hairs a bit, but I think you will find that the differences
> leak through in inconvenient spots (like format-patch, where you really
> _do_ want to default to the true upstream).

Thanks for the clear reasoning.

> If we add "@{publish}" (and "@{pu}"), then it becomes very convenient to
> refer to the ram/ version of your branch. That seems like an obvious
> first step to me. We don't have to add new config, because
> "branch.*.pushremote" already handles this.

Agreed. I'll start working on @{publish}. It's going to take quite a
bit of effort, because I won't actually start using it until my prompt
is @{publish}-aware.

> Now you can do "git rebase @{pu}" which is nice, but not _quite_ as nice
> as "git rebase", which defaults to "@{u}". That first step might be
> enough, and I'd hold off there and try it out for a few days or weeks
> first. But if you find in your workflow that you are having to specify
> "@{pu}" a lot, then maybe it is worth adding an option to default rebase
> to "@{pu}" instead of "@{u}".

Actually, I'm not sure I'd use "git rebase @{pu}"; for me @{pu} is
mainly a source of information for taking apart to build a new series.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [RFC/PATCH 0/5] <branch>@{publish} shorthand
  2014-01-07 21:35         ` Ramkumar Ramachandra
@ 2014-01-08  9:33           ` Jeff King
  2014-01-08  9:34             ` [PATCH 1/5] sha1_name: refactor upstream_mark Jeff King
                               ` (5 more replies)
  0 siblings, 6 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:33 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

On Wed, Jan 08, 2014 at 03:05:48AM +0530, Ramkumar Ramachandra wrote:

> Agreed. I'll start working on @{publish}. It's going to take quite a
> bit of effort, because I won't actually start using it until my prompt
> is @{publish}-aware.

There's a fair bit of refactoring involved. I took a stab at it and came
up with the series below. No docs or tests, and some of the refactoring
in remote.c feels a little weird. I can't help but feel more of the
logic from "git push" should be shared here.

But it at least works with my rudimentary examples. I'm hoping it will
make a good starting point for you to build on. Otherwise, I may get to
it eventually, but it's not a high priority for me right now.

> Actually, I'm not sure I'd use "git rebase @{pu}"; for me @{pu} is
> mainly a source of information for taking apart to build a new series.

Ah, that's how I'd probably use it, too. :)

  [1/5]: sha1_name: refactor upstream_mark
  [2/5]: interpret_branch_name: factor out upstream handling
  [3/5]: branch_get: return early on error
  [4/5]: branch_get: provide per-branch pushremote pointers
  [5/5]: implement @{publish} shorthand

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH 1/5] sha1_name: refactor upstream_mark
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
@ 2014-01-08  9:34             ` Jeff King
  2014-01-08  9:34             ` [PATCH 2/5] interpret_branch_name: factor out upstream handling Jeff King
                               ` (4 subsequent siblings)
  5 siblings, 0 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:34 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

We will be adding new mark types in the future, so separate
the suffix data from the logic.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_name.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index b1873d8..0c50801 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -415,12 +415,12 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
-static inline int upstream_mark(const char *string, int len)
+static inline int at_mark(const char *string, int len,
+			  const char **suffix, int nr)
 {
-	const char *suffix[] = { "@{upstream}", "@{u}" };
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
+	for (i = 0; i < nr; i++) {
 		int suffix_len = strlen(suffix[i]);
 		if (suffix_len <= len
 		    && !memcmp(string, suffix[i], suffix_len))
@@ -429,6 +429,12 @@ static inline int upstream_mark(const char *string, int len)
 	return 0;
 }
 
+static inline int upstream_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{upstream}", "@{u}" };
+	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
 
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 2/5] interpret_branch_name: factor out upstream handling
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
  2014-01-08  9:34             ` [PATCH 1/5] sha1_name: refactor upstream_mark Jeff King
@ 2014-01-08  9:34             ` Jeff King
  2014-01-08 12:37               ` Ramkumar Ramachandra
  2014-01-08  9:35             ` [PATCH 3/5] branch_get: return early on error Jeff King
                               ` (3 subsequent siblings)
  5 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:34 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

This function checks a few different @{}-constructs. The
early part checks for and dispatches us to helpers for each
construct, but the code for handling @{upstream} is inline.

Let's factor this out into its own function. This makes
interpret_branch_name more readable, and will make it much
simpler to add more constructs in future patches.

While we're at it, let's also break apart the refactored
code into a few helper functions. These will be useful when
we implement similar @{upstream}-like constructs.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_name.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 52 insertions(+), 31 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 0c50801..50df5d4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1052,6 +1052,54 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 	return ret - used + len;
 }
 
+static void set_shortened_ref(struct strbuf *buf, const char *ref)
+{
+	char *s = shorten_unambiguous_ref(ref, 0);
+	strbuf_reset(buf);
+	strbuf_addstr(buf, s);
+	free(s);
+}
+
+static const char *get_upstream_branch(const char *branch_buf, int len)
+{
+	char *branch = xstrndup(branch_buf, len);
+	struct branch *upstream = branch_get(*branch ? branch : NULL);
+
+	/*
+	 * Upstream can be NULL only if branch refers to HEAD and HEAD
+	 * points to something different than a branch.
+	 */
+	if (!upstream)
+		die(_("HEAD does not point to a branch"));
+	if (!upstream->merge || !upstream->merge[0]->dst) {
+		if (!ref_exists(upstream->refname))
+			die(_("No such branch: '%s'"), branch);
+		if (!upstream->merge) {
+			die(_("No upstream configured for branch '%s'"),
+				upstream->name);
+		}
+		die(
+			_("Upstream branch '%s' not stored as a remote-tracking branch"),
+			upstream->merge[0]->src);
+	}
+	free(branch);
+
+	return upstream->merge[0]->dst;
+}
+
+static int interpret_upstream_mark(const char *name, int namelen,
+				   int at, struct strbuf *buf)
+{
+	int len;
+
+	len = upstream_mark(name + at, namelen - at);
+	if (!len)
+		return -1;
+
+	set_shortened_ref(buf, get_upstream_branch(name, at));
+	return len + at;
+}
+
 /*
  * This reads short-hand syntax that not only evaluates to a commit
  * object name, but also can act as if the end user spelled the name
@@ -1076,9 +1124,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 {
 	char *cp;
-	struct branch *upstream;
 	int len = interpret_nth_prior_checkout(name, buf);
-	int tmp_len;
 
 	if (!namelen)
 		namelen = strlen(name);
@@ -1100,36 +1146,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	if (len > 0)
 		return reinterpret(name, namelen, len, buf);
 
-	tmp_len = upstream_mark(cp, namelen - (cp - name));
-	if (!tmp_len)
-		return -1;
+	len = interpret_upstream_mark(name, namelen, cp - name, buf);
+	if (len > 0)
+		return len;
 
-	len = cp + tmp_len - name;
-	cp = xstrndup(name, cp - name);
-	upstream = branch_get(*cp ? cp : NULL);
-	/*
-	 * Upstream can be NULL only if cp refers to HEAD and HEAD
-	 * points to something different than a branch.
-	 */
-	if (!upstream)
-		die(_("HEAD does not point to a branch"));
-	if (!upstream->merge || !upstream->merge[0]->dst) {
-		if (!ref_exists(upstream->refname))
-			die(_("No such branch: '%s'"), cp);
-		if (!upstream->merge) {
-			die(_("No upstream configured for branch '%s'"),
-				upstream->name);
-		}
-		die(
-			_("Upstream branch '%s' not stored as a remote-tracking branch"),
-			upstream->merge[0]->src);
-	}
-	free(cp);
-	cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
-	strbuf_reset(buf);
-	strbuf_addstr(buf, cp);
-	free(cp);
-	return len;
+	return -1;
 }
 
 int strbuf_branchname(struct strbuf *sb, const char *name)
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 3/5] branch_get: return early on error
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
  2014-01-08  9:34             ` [PATCH 1/5] sha1_name: refactor upstream_mark Jeff King
  2014-01-08  9:34             ` [PATCH 2/5] interpret_branch_name: factor out upstream handling Jeff King
@ 2014-01-08  9:35             ` Jeff King
  2014-01-08  9:35             ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
                               ` (2 subsequent siblings)
  5 siblings, 0 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:35 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

Right now we simply check if "ret" is valid before doing
further processing. As we add more processing, this will
become more and more cumbersome. Instead, let's just check
whether "ret" is invalid and return early with the error.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index a89efab..a773004 100644
--- a/remote.c
+++ b/remote.c
@@ -1543,7 +1543,10 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
-	if (ret && ret->remote_name) {
+	if (!ret)
+		return NULL;
+
+	if (ret->remote_name) {
 		ret->remote = remote_get(ret->remote_name);
 		if (ret->merge_nr) {
 			int i;
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 4/5] branch_get: provide per-branch pushremote pointers
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
                               ` (2 preceding siblings ...)
  2014-01-08  9:35             ` [PATCH 3/5] branch_get: return early on error Jeff King
@ 2014-01-08  9:35             ` Jeff King
  2014-01-08 10:27               ` Jeff King
  2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
  2014-01-08 12:40             ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Ramkumar Ramachandra
  5 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:35 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

When a caller uses branch_get to retrieve a "struct branch",
they get the per-branch remote name and a pointer to the
remote struct. However, they have no way of knowing about
the per-branch pushremote from this interface.

Let's expose that information via fields similar to
"remote" and "remote_name".

We have to do a little refactoring around the configuration
reading here. Instead of pushremote_name being its own
allocated string, it instead becomes a pointer to one of:

  1. The pushremote_name of the current branch, if
     configured.

  2. The globally configured remote.pushdefault, which we
     store separately as pushremote_config_default.

We can then set the branch's "pushremote" field by doing the
normal sequence of config fallback.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 23 +++++++++++++++++++----
 remote.h |  2 ++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index a773004..53e40e0 100644
--- a/remote.c
+++ b/remote.c
@@ -50,6 +50,7 @@ static int branches_nr;
 static struct branch *current_branch;
 static const char *default_remote_name;
 static const char *pushremote_name;
+static const char *pushremote_config_default;
 static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
@@ -351,9 +352,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 				explicit_default_remote_name = 1;
 			}
 		} else if (!strcmp(subkey, ".pushremote")) {
+			if (git_config_string(&branch->pushremote_name, key, value))
+				return -1;
 			if (branch == current_branch)
-				if (git_config_string(&pushremote_name, key, value))
-					return -1;
+				pushremote_name = branch->pushremote_name;
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
@@ -385,8 +387,11 @@ static int handle_config(const char *key, const char *value, void *cb)
 	name = key + 7;
 
 	/* Handle remote.* variables */
-	if (!strcmp(name, "pushdefault"))
-		return git_config_string(&pushremote_name, key, value);
+	if (!strcmp(name, "pushdefault")) {
+		if (git_config_string(&pushremote_config_default, key, value) < 0)
+			return -1;
+		pushremote_name = pushremote_config_default;
+	}
 
 	/* Handle remote.<name>.* variables */
 	if (*name == '/') {
@@ -1560,6 +1565,16 @@ struct branch *branch_get(const char *name)
 			}
 		}
 	}
+
+	if (ret->pushremote_name)
+		ret->pushremote = remote_get(ret->pushremote_name);
+	else if (pushremote_config_default)
+		ret->pushremote = remote_get(pushremote_config_default);
+	else if (ret->remote_name)
+		ret->pushremote = remote_get(ret->remote_name);
+	else
+		ret->pushremote = remote_get("origin");
+
 	return ret;
 }
 
diff --git a/remote.h b/remote.h
index 00c6a76..e5beb30 100644
--- a/remote.h
+++ b/remote.h
@@ -200,6 +200,8 @@ struct branch {
 
 	const char *remote_name;
 	struct remote *remote;
+	const char *pushremote_name;
+	struct remote *pushremote;
 
 	const char **merge_name;
 	struct refspec **merge;
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 5/5] implement @{publish} shorthand
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
                               ` (3 preceding siblings ...)
  2014-01-08  9:35             ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
@ 2014-01-08  9:37             ` Jeff King
  2014-01-08 23:42               ` Junio C Hamano
                                 ` (2 more replies)
  2014-01-08 12:40             ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Ramkumar Ramachandra
  5 siblings, 3 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08  9:37 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

In a triangular workflow, you may have a distinct
@{upstream} that you pull changes from, but publish by
default (if you typed "git push") to a different remote (or
a different branch on the remote). It may sometimes be
useful to be able to quickly refer to that publishing point
(e.g., to see which changes you have that have not yet been
published).

This patch introduces the <branch>@{publish} shorthand (or
"@{pu}" to be even shorter). It refers to the tracking
branch of the remote branch to which you would push if you
were to push the named branch. That's a mouthful to explain,
so here's an example:

  $ git checkout -b foo origin/master
  $ git config remote.pushdefault github
  $ git push

Signed-off-by: Jeff King <peff@peff.net>
---
The implementation feels weird, like the "where do we push to" code
should be factored out from somewhere else. I think what we're doing
here is not _wrong_, but I don't like repeating what "git push" is doing
elsewhere. And I just punt on "simple" as a result. :)

 sha1_name.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 50df5d4..59ffa93 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -435,6 +435,12 @@ static inline int upstream_mark(const char *string, int len)
 	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
+static inline int publish_mark(const char *string, int len)
+{
+	const char *suffix[] = { "@{publish}" };
+	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
 
@@ -481,7 +487,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 					nth_prior = 1;
 					continue;
 				}
-				if (!upstream_mark(str + at, len - at)) {
+				if (!upstream_mark(str + at, len - at) &&
+				    !publish_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -1100,6 +1107,69 @@ static int interpret_upstream_mark(const char *name, int namelen,
 	return len + at;
 }
 
+static const char *get_publish_branch(const char *name_buf, int len)
+{
+	char *name = xstrndup(name_buf, len);
+	struct branch *b = branch_get(*name ? name : NULL);
+	struct remote *remote = b->pushremote;
+	const char *dst;
+	const char *track;
+
+	free(name);
+
+	if (!remote)
+		die(_("branch '%s' has no remote for pushing"), b->name);
+
+	/* Figure out what we would call it on the remote side... */
+	if (remote->push_refspec_nr)
+		dst = apply_refspecs(remote->push, remote->push_refspec_nr,
+				     b->refname);
+	else
+		dst = b->refname;
+	if (!dst)
+		die(_("unable to figure out how '%s' would be pushed"),
+		    b->name);
+
+	/* ...and then figure out what we would call that remote here */
+	track = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, dst);
+	if (!track)
+		die(_("%s@{publish} has no tracking branch for '%s'"),
+		    b->name, dst);
+
+	return track;
+}
+
+static int interpret_publish_mark(const char *name, int namelen,
+				  int at, struct strbuf *buf)
+{
+	int len;
+
+	len = publish_mark(name + at, namelen - at);
+	if (!len)
+		return -1;
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		die(_("cannot use @{publish} with push.default of 'nothing'"));
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		set_shortened_ref(buf, get_publish_branch(name, at));
+		break;
+
+	case PUSH_DEFAULT_UPSTREAM:
+		set_shortened_ref(buf, get_upstream_branch(name, at));
+		break;
+
+	case PUSH_DEFAULT_SIMPLE:
+		/* ??? */
+		die("@{publish} with simple unimplemented");
+	}
+
+	return at + len;
+}
+
 /*
  * This reads short-hand syntax that not only evaluates to a commit
  * object name, but also can act as if the end user spelled the name
@@ -1150,6 +1220,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	if (len > 0)
 		return len;
 
+	len = interpret_publish_mark(name, namelen, cp - name, buf);
+	if (len > 0)
+		return len;
+
 	return -1;
 }
 
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/5] branch_get: provide per-branch pushremote pointers
  2014-01-08  9:35             ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
@ 2014-01-08 10:27               ` Jeff King
  2014-01-08 10:47                 ` [PATCH] t5531: further "matching" fixups Jeff King
  2014-01-08 11:09                 ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
  0 siblings, 2 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08 10:27 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

On Wed, Jan 08, 2014 at 04:35:31AM -0500, Jeff King wrote:

> @@ -385,8 +387,11 @@ static int handle_config(const char *key, const char *value, void *cb)
>  	name = key + 7;
>  
>  	/* Handle remote.* variables */
> -	if (!strcmp(name, "pushdefault"))
> -		return git_config_string(&pushremote_name, key, value);
> +	if (!strcmp(name, "pushdefault")) {
> +		if (git_config_string(&pushremote_config_default, key, value) < 0)
> +			return -1;
> +		pushremote_name = pushremote_config_default;
> +	}

This needs "return 0" squashed in at the end of the conditional, of
course, to match the old behavior.

This patch passes the test suite by itself (with or without that fixup).
But oddly, it seems to fail t5531 when merged with 'next'. I can't
figure out why, though. It shouldn't affect any code that doesn't look
at branch->pushremote.

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH] t5531: further "matching" fixups
  2014-01-08 10:27               ` Jeff King
@ 2014-01-08 10:47                 ` Jeff King
  2014-01-10 23:34                   ` Junio C Hamano
  2014-01-08 11:09                 ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
  1 sibling, 1 reply; 37+ messages in thread
From: Jeff King @ 2014-01-08 10:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List

Commit 43eb920 switched one of the sub-repository in this
test to matching to prepare for a world where the default
becomes "simple". However, the main repository needs a
similar change.

We did not notice any test failure when merged with b2ed944
(push: switch default from "matching" to "simple", 2013-01-04)
because t5531.6 is trying to provoke a failure of "git push"
due to a submodule check. When combined with b2ed944 the
push still fails, but for the wrong reason (because our
upstream setup does not exist, not because of the submodule).

Signed-off-by: Jeff King <peff@peff.net>
---
On Wed, Jan 08, 2014 at 05:27:07AM -0500, Jeff King wrote:

> This patch passes the test suite by itself (with or without that fixup).
> But oddly, it seems to fail t5531 when merged with 'next'. I can't
> figure out why, though. It shouldn't affect any code that doesn't look
> at branch->pushremote.

I still don't understand the full reason for this interaction, but the
failing test is actually somewhat broken in 'next' already. This patch
fixes it, and should be done regardless of the other series.

 t/t5531-deep-submodule-push.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 8c16e04..445bb5f 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -12,6 +12,7 @@ test_expect_success setup '
 	(
 		cd work &&
 		git init &&
+		git config push.default matching &&
 		mkdir -p gar/bage &&
 		(
 			cd gar/bage &&
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 4/5] branch_get: provide per-branch pushremote pointers
  2014-01-08 10:27               ` Jeff King
  2014-01-08 10:47                 ` [PATCH] t5531: further "matching" fixups Jeff King
@ 2014-01-08 11:09                 ` Jeff King
  1 sibling, 0 replies; 37+ messages in thread
From: Jeff King @ 2014-01-08 11:09 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

On Wed, Jan 08, 2014 at 05:27:07AM -0500, Jeff King wrote:

> This patch passes the test suite by itself (with or without that fixup).
> But oddly, it seems to fail t5531 when merged with 'next'. I can't
> figure out why, though. It shouldn't affect any code that doesn't look
> at branch->pushremote.

OK, I figured it out. My patch calls:

  remote_get("origin")

which creates an origin remote, even if one does not exist (it assumes
it to be a URL "origin"). Later, when we want to decide if the push is
triangular or not, we ask for:

  remote_get(NULL);

which will internally look for a remote called "origin". Before my patch
there was not such a remote, and so the push could not be triangular.
After my patch, it finds the bogus remote and says "this thing exists,
and is not what we are pushing to; therefore the push is triangular".

The solution is that I should not be passing the term "origin" to
remote_get, but rather passing NULL and relying on it to figure out the
default remote correctly. I.e.:

diff --git a/remote.c b/remote.c
index 8724388..d214fa2 100644
--- a/remote.c
+++ b/remote.c
@@ -1574,7 +1574,7 @@ struct branch *branch_get(const char *name)
 	else if (ret->remote_name)
 		ret->pushremote = remote_get(ret->remote_name);
 	else
-		ret->pushremote = remote_get("origin");
+		ret->pushremote = remote_get(NULL);
 
 	return ret;
 }

-Peff

^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 2/5] interpret_branch_name: factor out upstream handling
  2014-01-08  9:34             ` [PATCH 2/5] interpret_branch_name: factor out upstream handling Jeff King
@ 2014-01-08 12:37               ` Ramkumar Ramachandra
  0 siblings, 0 replies; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-08 12:37 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

Jeff King wrote:
>  sha1_name.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 31 deletions(-)

Thanks. I applied this to my series as-it-is.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [RFC/PATCH 0/5] <branch>@{publish} shorthand
  2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
                               ` (4 preceding siblings ...)
  2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
@ 2014-01-08 12:40             ` Ramkumar Ramachandra
  5 siblings, 0 replies; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-08 12:40 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

Jeff King wrote:
> There's a fair bit of refactoring involved. I took a stab at it and came
> up with the series below. No docs or tests, and some of the refactoring
> in remote.c feels a little weird. I can't help but feel more of the
> logic from "git push" should be shared here.
>
> But it at least works with my rudimentary examples. I'm hoping it will
> make a good starting point for you to build on. Otherwise, I may get to
> it eventually, but it's not a high priority for me right now.

Thanks; I'll see what I can do about getting it to share code with 'git push'.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
@ 2014-01-08 23:42               ` Junio C Hamano
  2014-01-09 18:20                 ` Jeff King
  2014-01-09  8:39               ` Philip Oakley
  2014-01-24  0:16               ` Junio C Hamano
  2 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2014-01-08 23:42 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

> In a triangular workflow, you may have a distinct
> @{upstream} that you pull changes from, but publish by
> default (if you typed "git push") to a different remote (or
> a different branch on the remote). It may sometimes be
> useful to be able to quickly refer to that publishing point
> (e.g., to see which changes you have that have not yet been
> published).
>
> This patch introduces the <branch>@{publish} shorthand (or
> "@{pu}" to be even shorter). It refers to the tracking

If @{u} can already be used for upstream, why not allow @{p} but
require two letters @{pu}?  Just being curious---I am not advocating
strongly for a shorter short-hand.

Or is @{p} already taken by something and my memory is not
functioning well?

> branch of the remote branch to which you would push if you
> were to push the named branch. That's a mouthful to explain,
> so here's an example:
>
>   $ git checkout -b foo origin/master
>   $ git config remote.pushdefault github
>   $ git push
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The implementation feels weird, like the "where do we push to" code
> should be factored out from somewhere else. I think what we're doing
> here is not _wrong_, but I don't like repeating what "git push" is doing
> elsewhere. And I just punt on "simple" as a result. :)
>
>  sha1_name.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 50df5d4..59ffa93 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -435,6 +435,12 @@ static inline int upstream_mark(const char *string, int len)
>  	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
>  }
>  
> +static inline int publish_mark(const char *string, int len)
> +{
> +	const char *suffix[] = { "@{publish}" };
> +	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
> +}
> +
>  static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
>  static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
>  
> @@ -481,7 +487,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  					nth_prior = 1;
>  					continue;
>  				}
> -				if (!upstream_mark(str + at, len - at)) {
> +				if (!upstream_mark(str + at, len - at) &&
> +				    !publish_mark(str + at, len - at)) {
>  					reflog_len = (len-1) - (at+2);
>  					len = at;
>  				}
> @@ -1100,6 +1107,69 @@ static int interpret_upstream_mark(const char *name, int namelen,
>  	return len + at;
>  }
>  
> +static const char *get_publish_branch(const char *name_buf, int len)
> +{
> +	char *name = xstrndup(name_buf, len);
> +	struct branch *b = branch_get(*name ? name : NULL);
> +	struct remote *remote = b->pushremote;
> +	const char *dst;
> +	const char *track;
> +
> +	free(name);
> +
> +	if (!remote)
> +		die(_("branch '%s' has no remote for pushing"), b->name);
> +
> +	/* Figure out what we would call it on the remote side... */
> +	if (remote->push_refspec_nr)
> +		dst = apply_refspecs(remote->push, remote->push_refspec_nr,
> +				     b->refname);
> +	else
> +		dst = b->refname;
> +	if (!dst)
> +		die(_("unable to figure out how '%s' would be pushed"),
> +		    b->name);
> +
> +	/* ...and then figure out what we would call that remote here */
> +	track = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, dst);
> +	if (!track)
> +		die(_("%s@{publish} has no tracking branch for '%s'"),
> +		    b->name, dst);
> +
> +	return track;
> +}
> +
> +static int interpret_publish_mark(const char *name, int namelen,
> +				  int at, struct strbuf *buf)
> +{
> +	int len;
> +
> +	len = publish_mark(name + at, namelen - at);
> +	if (!len)
> +		return -1;
> +
> +	switch (push_default) {
> +	case PUSH_DEFAULT_NOTHING:
> +		die(_("cannot use @{publish} with push.default of 'nothing'"));
> +
> +	case PUSH_DEFAULT_UNSPECIFIED:
> +	case PUSH_DEFAULT_MATCHING:
> +	case PUSH_DEFAULT_CURRENT:
> +		set_shortened_ref(buf, get_publish_branch(name, at));
> +		break;
> +
> +	case PUSH_DEFAULT_UPSTREAM:
> +		set_shortened_ref(buf, get_upstream_branch(name, at));
> +		break;
> +
> +	case PUSH_DEFAULT_SIMPLE:
> +		/* ??? */
> +		die("@{publish} with simple unimplemented");
> +	}
> +
> +	return at + len;
> +}
> +
>  /*
>   * This reads short-hand syntax that not only evaluates to a commit
>   * object name, but also can act as if the end user spelled the name
> @@ -1150,6 +1220,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
>  	if (len > 0)
>  		return len;
>  
> +	len = interpret_publish_mark(name, namelen, cp - name, buf);
> +	if (len > 0)
> +		return len;
> +
>  	return -1;
>  }

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
  2014-01-08 23:42               ` Junio C Hamano
@ 2014-01-09  8:39               ` Philip Oakley
  2014-01-09 22:03                 ` Jeff King
  2014-01-09 22:24                 ` Junio C Hamano
  2014-01-24  0:16               ` Junio C Hamano
  2 siblings, 2 replies; 37+ messages in thread
From: Philip Oakley @ 2014-01-09  8:39 UTC (permalink / raw)
  To: Jeff King, Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

From: "Jeff King" <peff@peff.net>
Sent: Wednesday, January 08, 2014 9:37 AM
> In a triangular workflow, you may have a distinct
> @{upstream} that you pull changes from, but publish by
> default (if you typed "git push") to a different remote (or
> a different branch on the remote).

One of the broader issues is the lack of _documenation_ about what the 
'normal' naming convention is for the uspstream remote. Especially the 
implicit convention used within our documentation (and workflow).

This is especially true for github users who will normally fork a repo 
of interest and then clone it from their own copy/fork. This means that 
the 'origin' remote is _not_ the upstream. See 
https://help.github.com/articles/fork-a-repo In my case 'origin' is my 
publish repo (as suggested by Github) while 'junio' is the upstream (as 
do some others). There are similar results from the likes of 
Stackoverflow.

Much of the earlier discussion did appear to be as much a confusion over 
terminology as that of coding a suitable solution ro Ram's original 
forked-from issue.

I know it's been an issue I've had for some while 
http://thread.gmane.org/gmane.comp.version-control.git/194175/focus=195385

Philip

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-08 23:42               ` Junio C Hamano
@ 2014-01-09 18:20                 ` Jeff King
  2014-01-09 21:24                   ` Junio C Hamano
  0 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2014-01-09 18:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List

On Wed, Jan 08, 2014 at 03:42:09PM -0800, Junio C Hamano wrote:

> > This patch introduces the <branch>@{publish} shorthand (or
> > "@{pu}" to be even shorter). It refers to the tracking
> 
> If @{u} can already be used for upstream, why not allow @{p} but
> require two letters @{pu}?  Just being curious---I am not advocating
> strongly for a shorter short-hand.
> 
> Or is @{p} already taken by something and my memory is not
> functioning well?

It is my brain that was not functioning well. I somehow thought "well,
@{u} is already taken, so we must use "@{pu}". Which of course makes no
sense, unless you are middle-endian. :)

We may want to be cautious about giving up a short-and-sweet
single-letter, though, until the feature has proved itself. We could
also teach upstream_mark and friends to match unambiguous prefixes (so
"@{u}, "@{up}", "@{upst}", etc). That means "@{p}" would work
immediately, but scripts should use "@{publish}" for future-proofing.

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-09 18:20                 ` Jeff King
@ 2014-01-09 21:24                   ` Junio C Hamano
  0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-01-09 21:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

>> Or is @{p} already taken by something and my memory is not
>> functioning well?
>
> It is my brain that was not functioning well. I somehow thought "well,
> @{u} is already taken, so we must use "@{pu}". Which of course makes no
> sense, unless you are middle-endian. :)
>
> We may want to be cautious about giving up a short-and-sweet
> single-letter, though, until the feature has proved itself. We could
> also teach upstream_mark and friends to match unambiguous prefixes (so
> "@{u}, "@{up}", "@{upst}", etc). That means "@{p}" would work
> immediately, but scripts should use "@{publish}" for future-proofing.

I recall we wanted to start only with "@{upstream}" without "@{u}";
justification being "if the concept is solid and useful enough, the
latter will come later as a natural user-desire", during the
discussion that ended up introducing them.

I am OK with the "unambigous prefix string".

Thanks for sanity-checking.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-09  8:39               ` Philip Oakley
@ 2014-01-09 22:03                 ` Jeff King
  2014-01-09 22:24                 ` Junio C Hamano
  1 sibling, 0 replies; 37+ messages in thread
From: Jeff King @ 2014-01-09 22:03 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Ramkumar Ramachandra, Git List, Junio C Hamano

On Thu, Jan 09, 2014 at 08:39:44AM -0000, Philip Oakley wrote:

> From: "Jeff King" <peff@peff.net>
> Sent: Wednesday, January 08, 2014 9:37 AM
> >In a triangular workflow, you may have a distinct
> >@{upstream} that you pull changes from, but publish by
> >default (if you typed "git push") to a different remote (or
> >a different branch on the remote).
> 
> One of the broader issues is the lack of _documenation_ about what
> the 'normal' naming convention is for the uspstream remote.
> Especially the implicit convention used within our documentation (and
> workflow).
> 
> This is especially true for github users who will normally fork a
> repo of interest and then clone it from their own copy/fork. This
> means that the 'origin' remote is _not_ the upstream. See
> https://help.github.com/articles/fork-a-repo In my case 'origin' is
> my publish repo (as suggested by Github) while 'junio' is the
> upstream (as do some others). There are similar results from the
> likes of Stackoverflow.

Sure, and I have done the same thing (though I tend to clone from the
other person as "origin", and only fork my own repo when I am ready to
push). But it shouldn't matter, should it? The whole point of the
upstream config is that "git checkout -b topic junio/master" does the
right thing, without caring about your naming convention.

So I'm not sure what you think should be said (or where). Telling me in
patch form is preferred. :)

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-09  8:39               ` Philip Oakley
  2014-01-09 22:03                 ` Jeff King
@ 2014-01-09 22:24                 ` Junio C Hamano
  1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-01-09 22:24 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Jeff King, Ramkumar Ramachandra, Git List

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Jeff King" <peff@peff.net>
> Sent: Wednesday, January 08, 2014 9:37 AM
>> In a triangular workflow, you may have a distinct
>> @{upstream} that you pull changes from, but publish by
>> default (if you typed "git push") to a different remote (or
>> a different branch on the remote).
>
> One of the broader issues is the lack of _documenation_ about what the
> normal' naming convention is for the uspstream remote. Especially the
> implicit convention used within our documentation (and workflow).

Sure, let's start trying to come up with what the eventual
documentation patch may want to say.

 * The "upstream" is the place the updates by the project-as-a-whole
   (including others' work but also your previous work) come from.
   It is what you use "git pull [--rebase]" to integrate the work on
   your current branch with in order to keep it in sync with the
   outside world.  Such a repository (often called "origin", and
   "git clone" sets it up for you) may be called "upstream
   repository".

   Each of your branch would often have a single branch in that
   repository (e.g. "master", which you locally use the
   "origin/master" remote-tracking branch to keep track of its most
   recently observed state).  In the simplest case, you clone from
   your "origin", you get your own "master" branch, which is set to
   integrate with the "master" branch at the "origin".  Their
   "master" (i.e. what you view as "origin/master") would be the
   "upstream branch" for your "master" branch.

   For a branch B, B@{upstream} names the remote-tracking branch
   used for the upstream branch of B.  For example, to fork a new
   branch 'foo' that has the same upstream branch as your branch
   'master' does, "git checkout -t -b foo master@{upstream}" can be
   used.

 * If you and others are using the same repository to advance the
   project, the repository you cloned from, i.e. your "upstream
   repository", is the same repository you push your changes back
   to.  There is no other repository you have to worry about.

   In such a "centralized" setting, it is likely that you may want
   to update one of three possible branches at the upstream
   repository when you push your changes back, if your local branch
   is named differently from its upstream branch.  Either:

   (1) You started working on a topic (e.g. your "fix-bug-2431"
       branch) based on an integration branch (e.g. "master" at the
       upstream, i.e. "origin/master" to you), and you want to
       publish it so that others can take a look at it and help you
       polish it while it is still not suitable for the integration
       branch.  As long as you gave a name to that topic branch that
       is descriptive and good enough for public consumption, you
       would want it to go to the same name (e.g. you would want to
       push to "fix-bug-2431" branch at the upstream repository from
       your "fix-bug-2431" branch); or

   (2) You are working on your copy (e.g. your "master" branch) of
       an integration branch (e.g. "origin/master" to you), and you
       want to update the "master" branch at the upstream
       repository.

   (3) There is another possibilty, in which you are working on a
       topic forked from an integration branch (as in (1)), and are
       done with the topic and want to push the result out directly
       to the integration branch.  Your "fix-bug-2431" branch may
       have started from "origin/master" and "git pull [--rebase]"
       on the branch would integrate with "master" branch at the
       upstream repository, and your "git push" on the
       "fix-bug-2431" branch will update that "master" branch at the
       upstream repository, which makes it look symmetric.

    The default in Git 2.0 will allow you to do (2) without any
    further set-up, and you can start living in the future by
    setting push.default to "simple".  Your current branch, when you
    run "git push", and its upstream branch must share the same
    name.

    If you want to do (1), you would want to set push.default to
    "current".  Your current branch, when you run "git push" may not
    have an explicit upstream branch (hence "git pull" without any
    other argument may fail), but the work on your branch will be
    pushed to the branch of the same name at the upstream
    repository.

    For (3), you would set push.default to "upstream".  Your current
    branch, when you run "git push", must have an explicit upstream
    branch specified and you must be pushing to the upstream
    repository for this to work for obvious reasons.

 * If you originally clone from somewhere you cannot (or do not want
   to even if you could) push to, you would want your "git push" to
   go to a repository that is different from your "upstream".  In
   such a "triangular" setting, the result of your work is published
   to your own repository (we'd call it "publish"), and others
   interested in your work would pull from there to integrate it to
   their work.  Among these other people there may be somebody who
   integrates work by all relevant people to the project mainline
   and updates the repository that you and other project participant
   all call their "upstream", and that is how you see your own work
   back in your "upstream".

   Set remote.pushdefault to name the repository that is your
   "publish" repository if you are using such a triangular workflow
   (you could use branch.*.pushremote to publish to different
   repositories per branch).

   Your local branch in such a triangular setting will have its
   "upstream" (the repository your "git pull" goes to and one of its
   branches it integrates with) and its "publish" (the repository
   and one of its branches your "git push" updates).  Like the way
   B@{upstream} can be used to refer to the former for your local
   branch B, B@{publish} can be used to refer to the latter.

   In such a "triangular" setting, it is likely that you may want to
   update the branch of the same name in your "publish" repository.
   If you have been working on your "fix-bug-2431" branch, you would
   want the result to go to "fix-bug-2431" branch there.

   The default in Git 2.0, when a triangular workflow is used by
   setting remote.pushdefault (or branch.*.pushremote), will push
   the current branch to the branch of the same name, so you do not
   have to do anything further.  You can start living in the future
   by setting push.default to "simple".

   The "upstream" setting of push.default would not make any sense
   in such a triangular workflow, so your "git push" will error out
   when you push to a repository that is not your "upstream" while
   the push.default is set to "upstream".

 * At the conceptual level, anybody who treats the work you publish
   as his or her "upstream" is your downstream, but because you do
   not control and keep track of who clones and pulls from you,
   there is no such notation as @{downstream}.

It is unfortunate that GitHub worked aroud the lack of "publish"
concept not by adding it to Git, but by introducing "fork" at the
server side, which ends up twisting the concept of "upstream" that
is sets up by "git clone".

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH] t5531: further "matching" fixups
  2014-01-08 10:47                 ` [PATCH] t5531: further "matching" fixups Jeff King
@ 2014-01-10 23:34                   ` Junio C Hamano
  2014-01-11  4:22                     ` Jeff King
  0 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2014-01-10 23:34 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

> ... but the
> failing test is actually somewhat broken in 'next' already.

Hmph, in what way?  I haven't seen t5531 breakage on 'next', with or
without your series...

> fixes it, and should be done regardless of the other series.
>
>  t/t5531-deep-submodule-push.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
> index 8c16e04..445bb5f 100755
> --- a/t/t5531-deep-submodule-push.sh
> +++ b/t/t5531-deep-submodule-push.sh
> @@ -12,6 +12,7 @@ test_expect_success setup '
>  	(
>  		cd work &&
>  		git init &&
> +		git config push.default matching &&
>  		mkdir -p gar/bage &&
>  		(
>  			cd gar/bage &&

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH] t5531: further "matching" fixups
  2014-01-10 23:34                   ` Junio C Hamano
@ 2014-01-11  4:22                     ` Jeff King
  0 siblings, 0 replies; 37+ messages in thread
From: Jeff King @ 2014-01-11  4:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List

On Fri, Jan 10, 2014 at 03:34:59PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... but the
> > failing test is actually somewhat broken in 'next' already.
> 
> Hmph, in what way?  I haven't seen t5531 breakage on 'next', with or
> without your series...

The test still passes, but it is not testing the right thing anymore.

On 'next', run t5531. Test 6 is "push fails when commit on
multiple branches if one branch has no remote" and ends with:

  test_must_fail git push --recurse-submodules=check ../pub.git

But the output ends with:

  warning: push.default is unset; its implicit value has changed in
  Git 2.0 from 'matching' to 'simple'. To squelch this message
  [...]

  fatal: The current branch branch2 has no upstream branch.
  To push the current branch and set the remote as upstream, use

      git push --set-upstream ../pub.git branch2

When not merged with b2ed944 (push: switch default from "matching" to
"simple"), or with my patch to set push.default=matching explicitly, the
output is:

  The following submodule paths contain changes that can
  not be found on any remote:
    gar/bage

  Please try

          git push --recurse-submodules=on-demand

  or cd to the path and use

          git push

  to push them to a remote.

  fatal: Aborting.

which is what the test is actually trying to check. So the push fails,
as we expect, but not for the right reason.

My other series for @{publish} had a bug that caused the push to
succeed. So that series was buggy (and I posted the fix already), but we
only noticed it because this test was not working (it should not care
about upstream/triangular config at all, but it accidentally did).

Does that clarify the situation?

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
  2014-01-08 23:42               ` Junio C Hamano
  2014-01-09  8:39               ` Philip Oakley
@ 2014-01-24  0:16               ` Junio C Hamano
  2014-01-24 21:35                 ` Jeff King
  2 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2014-01-24  0:16 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

> In a triangular workflow, you may have a distinct
> @{upstream} that you pull changes from, but publish by
> default (if you typed "git push") to a different remote (or
> a different branch on the remote). It may sometimes be
> useful to be able to quickly refer to that publishing point
> (e.g., to see which changes you have that have not yet been
> published).
>
> This patch introduces the <branch>@{publish} shorthand (or
> "@{pu}" to be even shorter). It refers to the tracking
> branch of the remote branch to which you would push if you
> were to push the named branch. That's a mouthful to explain,
> so here's an example:
>
>   $ git checkout -b foo origin/master
>   $ git config remote.pushdefault github
>   $ git push
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

As there is no @{pu} in publish_mark() as far as I can see, and also
I found it is a bit unclear what the example in the last paragraph
wants to illustrate, I'll reword the above as the following before
merging it to 'next'.

    This patch introduces the <branch>@{publish} shorthand that
    refers to the tracking branch of the remote branch to which
    you would push if you were to push the named branch.
    
    That's a mouthful to explain, so here's an example:
    
      $ git checkout -b foo origin/master
      $ git config remote.pushdefault github
      $ git push
    
    With this, foo@{upstream} and foo@{publish} would be origin/master
    and github/foo, respectively (assuming that "git fetch github" is
    configured to use refs/remotes/github/* remote-tracking branches).

> The implementation feels weird, like the "where do we push to" code
> should be factored out from somewhere else. I think what we're doing
> here is not _wrong_, but I don't like repeating what "git push" is doing
> elsewhere. And I just punt on "simple" as a result. :)

I think we can polish that in-tree.

Thanks.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-24  0:16               ` Junio C Hamano
@ 2014-01-24 21:35                 ` Jeff King
  2014-01-24 22:05                   ` Ramkumar Ramachandra
  2014-02-15 11:50                   ` Philip Oakley
  0 siblings, 2 replies; 37+ messages in thread
From: Jeff King @ 2014-01-24 21:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Git List

On Thu, Jan 23, 2014 at 04:16:06PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > In a triangular workflow, you may have a distinct
> > @{upstream} that you pull changes from, but publish by
> > default (if you typed "git push") to a different remote (or
> > a different branch on the remote). It may sometimes be
> > useful to be able to quickly refer to that publishing point
> > (e.g., to see which changes you have that have not yet been
> > published).
> >
> > This patch introduces the <branch>@{publish} shorthand (or
> > "@{pu}" to be even shorter). It refers to the tracking
> > branch of the remote branch to which you would push if you
> > were to push the named branch. That's a mouthful to explain,
> > so here's an example:
> >
> >   $ git checkout -b foo origin/master
> >   $ git config remote.pushdefault github
> >   $ git push
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> 
> As there is no @{pu} in publish_mark() as far as I can see, and also
> I found it is a bit unclear what the example in the last paragraph
> wants to illustrate, I'll reword the above as the following before
> merging it to 'next'.

Yeah, I think the @{pu} was just a silly omission from the code, though
I agree after our discussion that we should just stick with "@{publish}"
for now.

I am not sure why I said "git push" at the end. I would have thought
that:

  $ git rev-parse --symbolic-full-name @{publish}
  refs/remotes/github/foo

would have been the right command to demonstrate. The text you suggested
is fine, though I think you can simply drop the "git push", as it does
not add anything.

As far as merging it to 'next', I had not really intended it to go that
far. :) It was more for Ram to use as a base. I find some of the
refactoring questionable, including:

  1. The meaning of branch->pushremote is subtly different from that of
     branch->remote. Ram's followup refactoring did a better job of
     that (but he is missing the patches on top to finish out the
     feature).

  2. We are duplicating the "where to push" logic here. That should
     probably be factored out so that "git push" and "@{publish}" use
     the same logic.

And of course there are no tests or documentation. It might work,
though.

I don't mind if you want to merge it and do more work in-tree, but I do
not think it should graduate as-is. And you may want check from Ram that
he is not in the middle of his own version based on the patches he sent
earlier, as reworking them on top of mine would probably just be
needless extra work.

Are you planning on having request-pull use @{publish} as a default? I
saw you cc'd me on that thread, but I didn't have any opinion besides
"sounds like you could use it here".

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-24 21:35                 ` Jeff King
@ 2014-01-24 22:05                   ` Ramkumar Ramachandra
  2014-01-24 23:12                     ` Junio C Hamano
  2014-02-15 11:50                   ` Philip Oakley
  1 sibling, 1 reply; 37+ messages in thread
From: Ramkumar Ramachandra @ 2014-01-24 22:05 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Git List

Jeff King wrote:
> As far as merging it to 'next', I had not really intended it to go that
> far. :) It was more for Ram to use as a base.

Sorry about not having posted a follow-up yet; I'm adjusting to a new
timezone and environment.

> I find some of the
> refactoring questionable, including:
>
>   1. The meaning of branch->pushremote is subtly different from that of
>      branch->remote. Ram's followup refactoring did a better job of
>      that (but he is missing the patches on top to finish out the
>      feature).
>
>   2. We are duplicating the "where to push" logic here. That should
>      probably be factored out so that "git push" and "@{publish}" use
>      the same logic.
>
> And of course there are no tests or documentation. It might work,
> though.

Actually, task (2) is somewhat involved: I still haven't figured out
how to share code with 'git push'.

> I don't mind if you want to merge it and do more work in-tree, but I do
> not think it should graduate as-is. And you may want check from Ram that
> he is not in the middle of his own version based on the patches he sent
> earlier, as reworking them on top of mine would probably just be
> needless extra work.

On that note, can you hold off graduating
jk/branch-at-publish-rebased, Junio? Hopefully, I'll come up with a
replacement over the weekend.

Thanks.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-24 22:05                   ` Ramkumar Ramachandra
@ 2014-01-24 23:12                     ` Junio C Hamano
  0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-01-24 23:12 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Jeff King, Git List

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> On that note, can you hold off graduating
> jk/branch-at-publish-rebased, Junio? Hopefully, I'll come up with a
> replacement over the weekend.

Sure.

This close to the feature freeze, I'd rather see all contributors,
not limited to you, not rush on new and shiny things, but instead
spend time looking at bugs and fixes proposed for the upcoming
release in the codepaths they were involved.

The send-email SSL issue $gmane/240479 is one of the things I'd like
to see your sanity-checking ;-)

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-01-24 21:35                 ` Jeff King
  2014-01-24 22:05                   ` Ramkumar Ramachandra
@ 2014-02-15 11:50                   ` Philip Oakley
  2014-02-18  8:52                     ` Jeff King
  1 sibling, 1 reply; 37+ messages in thread
From: Philip Oakley @ 2014-02-15 11:50 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramkumar Ramachandra, Git List, Junio C Hamano

From: "Jeff King" <peff@peff.net>
Sent: Friday, January 24, 2014 9:35 PM
> On Thu, Jan 23, 2014 at 04:16:06PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > In a triangular workflow, you may have a distinct
>> > @{upstream} that you pull changes from, but publish by
>> > default (if you typed "git push") to a different remote (or
>> > a different branch on the remote). It may sometimes be
>> > useful to be able to quickly refer to that publishing point
>> > (e.g., to see which changes you have that have not yet been
>> > published).
>> >
>> > This patch introduces the <branch>@{publish} shorthand (or
>> > "@{pu}" to be even shorter).

Just to say that I'm not sure that "publish" is the best word for this 
concept.

To my mind something is published when some form of editorial oversight 
has been applied to the works. Such an understanding would better match 
the 'upstream' concept (e.g. $gmane/240230 jch/9Jan14). This should be 
distinguished from 'self-publishing', and again from 
'vanity-publishing'.

In terms of the triangular work-flow such a 'publish' repo is somewhere 
between a vanity publishing, and self publishing (depending on the level 
of code cleanliness;-)

One of the problems, just like the 'staging/index' discussions, is the
lack of suitable word for the new working concepts that perfect
reproduction has brought.

At best, I considered terms such as "home" repo, "depository" repo and 
"self" repo, and the many synonyms of publish, store, copy, duplicate, 
etc. without much success.

Finding a right term or phrase for the concept will be important to 
ensure some clarity and distinction between the upstream and one's 
'home' repo. Perhaps "self-storage" might be borrowed from that 
new(-ish) industry.

>> >                                            It refers to the 
>> > tracking
>> > branch of the remote branch to which you would push if you
>> > were to push the named branch. That's a mouthful to explain,
>> > so here's an example:
>> >
>> >   $ git checkout -b foo origin/master
>> >   $ git config remote.pushdefault github
>> >   $ git push
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>>
>> As there is no @{pu} in publish_mark() as far as I can see, and also
>> I found it is a bit unclear what the example in the last paragraph
>> wants to illustrate, I'll reword the above as the following before
>> merging it to 'next'.
>
> Yeah, I think the @{pu} was just a silly omission from the code, 
> though
> I agree after our discussion that we should just stick with 
> "@{publish}"
> for now.
>
> I am not sure why I said "git push" at the end. I would have thought
> that:
>
>  $ git rev-parse --symbolic-full-name @{publish}
>  refs/remotes/github/foo
>
> would have been the right command to demonstrate. The text you 
> suggested
> is fine, though I think you can simply drop the "git push", as it does
> not add anything.
>
> As far as merging it to 'next', I had not really intended it to go 
> that
> far. :) It was more for Ram to use as a base. I find some of the
> refactoring questionable, including:
>
>  1. The meaning of branch->pushremote is subtly different from that of
>     branch->remote. Ram's followup refactoring did a better job of
>     that (but he is missing the patches on top to finish out the
>     feature).
>
>  2. We are duplicating the "where to push" logic here. That should
>     probably be factored out so that "git push" and "@{publish}" use
>     the same logic.
>
> And of course there are no tests or documentation. It might work,
> though.
>
> I don't mind if you want to merge it and do more work in-tree, but I 
> do
> not think it should graduate as-is. And you may want check from Ram 
> that
> he is not in the middle of his own version based on the patches he 
> sent
> earlier, as reworking them on top of mine would probably just be
> needless extra work.
>
> Are you planning on having request-pull use @{publish} as a default? I
> saw you cc'd me on that thread, but I didn't have any opinion besides
> "sounds like you could use it here".
>
> -Peff
> --
Philip 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-02-15 11:50                   ` Philip Oakley
@ 2014-02-18  8:52                     ` Jeff King
  2014-02-18 13:10                       ` Johan Herland
  2014-02-18 19:52                       ` Junio C Hamano
  0 siblings, 2 replies; 37+ messages in thread
From: Jeff King @ 2014-02-18  8:52 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Ramkumar Ramachandra, Git List, Junio C Hamano

On Sat, Feb 15, 2014 at 11:50:10AM -0000, Philip Oakley wrote:

> >>> This patch introduces the <branch>@{publish} shorthand (or
> >>> "@{pu}" to be even shorter).
> 
> Just to say that I'm not sure that "publish" is the best word for
> this concept.
> 
> To my mind something is published when some form of editorial
> oversight has been applied to the works. Such an understanding would
> better match the 'upstream' concept (e.g. $gmane/240230 jch/9Jan14).
> This should be distinguished from 'self-publishing', and again from
> 'vanity-publishing'.
> 
> In terms of the triangular work-flow such a 'publish' repo is
> somewhere between a vanity publishing, and self publishing (depending
> on the level of code cleanliness;-)

I would much rather have a name that describes what the thing _is_, then
how it is meant to be used. The concept of @{publish} is a shorthand for
"where would I push if I typed git push on this branch". In a
non-triangular workflow, that means sharing your commits with others on
the main branch. In a triangular workflow, it means sharing your commits
with a publishing point so that others can see them. If your default
push goes to a backup repo, it does not mean publishing at all, but
rather syncing the backup.

So I do not think any one word can describe all of those use cases; they
are orthogonal to each other, and it depends on your workflow.

In that sense, "publish" is not the best word, either, as it describes
only the first two, but not the third case (and those are just examples;
there may be other setups beyond that, even).

Perhaps "@{push}" would be the most direct word.

-Peff

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-02-18  8:52                     ` Jeff King
@ 2014-02-18 13:10                       ` Johan Herland
  2014-02-18 19:52                       ` Junio C Hamano
  1 sibling, 0 replies; 37+ messages in thread
From: Johan Herland @ 2014-02-18 13:10 UTC (permalink / raw)
  To: Jeff King; +Cc: Philip Oakley, Ramkumar Ramachandra, Git List, Junio C Hamano

On Tue, Feb 18, 2014 at 9:52 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 15, 2014 at 11:50:10AM -0000, Philip Oakley wrote:
>> >>> This patch introduces the <branch>@{publish} shorthand (or
>> >>> "@{pu}" to be even shorter).
>>
>> Just to say that I'm not sure that "publish" is the best word for
>> this concept.
>> [...]
>
> I would much rather have a name that describes what the thing _is_, then
> how it is meant to be used. The concept of @{publish} is a shorthand for
> "where would I push if I typed git push on this branch". In a
> non-triangular workflow, that means sharing your commits with others on
> the main branch. In a triangular workflow, it means sharing your commits
> with a publishing point so that others can see them. If your default
> push goes to a backup repo, it does not mean publishing at all, but
> rather syncing the backup.
>
> So I do not think any one word can describe all of those use cases; they
> are orthogonal to each other, and it depends on your workflow.
>
> In that sense, "publish" is not the best word, either, as it describes
> only the first two, but not the third case (and those are just examples;
> there may be other setups beyond that, even).
>
> Perhaps "@{push}" would be the most direct word.

I agree that we want a more general (i.e. workflow-agnostic) term to
differentiate between "where we pull from", and "where we push to". As
such, "@{push}" should have a corresponding "@{pull}" (which I believe
should function as an alias of "@{upstream}"). [1]


...Johan

[1]: I don't think there is a reason not to reuse the "push"/"pull"
terminology for these concepts, but if there is, I guess we could
instead call them "@{source}"/"@{destination}", "@{src}/@{dst}", or
"@{from}/@{to}", or somesuch...

-- 
Johan Herland, <johan@herland.net>
www.herland.net

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 5/5] implement @{publish} shorthand
  2014-02-18  8:52                     ` Jeff King
  2014-02-18 13:10                       ` Johan Herland
@ 2014-02-18 19:52                       ` Junio C Hamano
  1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2014-02-18 19:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Philip Oakley, Ramkumar Ramachandra, Git List

Jeff King <peff@peff.net> writes:

> In that sense, "publish" is not the best word, either, as it describes
> only the first two, but not the third case (and those are just examples;
> there may be other setups beyond that, even).
>
> Perhaps "@{push}" would be the most direct word.

Hmph, then the other one would be @{pull}.

Which does not sound too bad, IMHO.

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2014-02-18 19:53 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-07 20:29 [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Ramkumar Ramachandra
2014-01-07 20:30 ` Ramkumar Ramachandra
2014-01-07 20:40   ` Jeff King
2014-01-07 20:48     ` Junio C Hamano
2014-01-07 21:02     ` Ramkumar Ramachandra
2014-01-07 21:16       ` Jeff King
2014-01-07 21:35         ` Ramkumar Ramachandra
2014-01-08  9:33           ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Jeff King
2014-01-08  9:34             ` [PATCH 1/5] sha1_name: refactor upstream_mark Jeff King
2014-01-08  9:34             ` [PATCH 2/5] interpret_branch_name: factor out upstream handling Jeff King
2014-01-08 12:37               ` Ramkumar Ramachandra
2014-01-08  9:35             ` [PATCH 3/5] branch_get: return early on error Jeff King
2014-01-08  9:35             ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
2014-01-08 10:27               ` Jeff King
2014-01-08 10:47                 ` [PATCH] t5531: further "matching" fixups Jeff King
2014-01-10 23:34                   ` Junio C Hamano
2014-01-11  4:22                     ` Jeff King
2014-01-08 11:09                 ` [PATCH 4/5] branch_get: provide per-branch pushremote pointers Jeff King
2014-01-08  9:37             ` [PATCH 5/5] implement @{publish} shorthand Jeff King
2014-01-08 23:42               ` Junio C Hamano
2014-01-09 18:20                 ` Jeff King
2014-01-09 21:24                   ` Junio C Hamano
2014-01-09  8:39               ` Philip Oakley
2014-01-09 22:03                 ` Jeff King
2014-01-09 22:24                 ` Junio C Hamano
2014-01-24  0:16               ` Junio C Hamano
2014-01-24 21:35                 ` Jeff King
2014-01-24 22:05                   ` Ramkumar Ramachandra
2014-01-24 23:12                     ` Junio C Hamano
2014-02-15 11:50                   ` Philip Oakley
2014-02-18  8:52                     ` Jeff King
2014-02-18 13:10                       ` Johan Herland
2014-02-18 19:52                       ` Junio C Hamano
2014-01-08 12:40             ` [RFC/PATCH 0/5] <branch>@{publish} shorthand Ramkumar Ramachandra
2014-01-07 20:36 ` [RFC/PATCH] format-patch: introduce branch.*.forkedFrom Junio C Hamano
2014-01-07 20:40   ` Ramkumar Ramachandra
2014-01-07 20:42     ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.