[RFC/PATCH] format-patch: introduce branch.*.forkedFrom

classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[RFC/PATCH] format-patch: introduce branch.*.forkedFrom

artagnon
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 <[hidden email]>
Cc: Junio C Hamano <[hidden email]>
Signed-off-by: Ramkumar Ramachandra <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

artagnon
[Fixed typo in Junio's address]

On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra <[hidden email]> 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 <[hidden email]>
> Cc: Junio C Hamano <[hidden email]>
> Signed-off-by: Ramkumar Ramachandra <[hidden email]>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

Junio C Hamano
In reply to this post by artagnon
Ramkumar Ramachandra <[hidden email]> 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 <[hidden email]>
> Cc: Junio C Hamano <[hidden email]>
> Signed-off-by: Ramkumar Ramachandra <[hidden email]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

Jeff King
In reply to this post by artagnon
On Wed, Jan 08, 2014 at 02:00:44AM +0530, Ramkumar Ramachandra wrote:

> On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra <[hidden email]> 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 <[hidden email]>
> > Cc: Junio C Hamano <[hidden email]>
> > Signed-off-by: Ramkumar Ramachandra <[hidden email]>

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

artagnon
In reply to this post by 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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

Junio C Hamano
Ramkumar Ramachandra <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

Junio C Hamano
In reply to this post by Jeff King
Jeff King <[hidden email]> 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".


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

artagnon
In reply to this post by Jeff King
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC/PATCH] format-patch: introduce branch.*.forkedFrom

artagnon
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[RFC/PATCH 0/5] <branch>@{publish} shorthand

Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/5] sha1_name: refactor upstream_mark

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

Signed-off-by: Jeff King <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] interpret_branch_name: factor out upstream handling

Jeff King
In reply to this post by Jeff King
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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5] branch_get: return early on error

Jeff King
In reply to this post by Jeff King
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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5] branch_get: provide per-branch pushremote pointers

Jeff King
In reply to this post by Jeff King
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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] implement @{publish} shorthand

Jeff King
In reply to this post by Jeff King
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 <[hidden email]>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] branch_get: provide per-branch pushremote pointers

Jeff King
In reply to this post by Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH] t5531: further "matching" fixups

Jeff King
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 <[hidden email]>
---
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] branch_get: provide per-branch pushremote pointers

Jeff King
In reply to this post by Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/5] interpret_branch_name: factor out upstream handling

artagnon
In reply to this post by Jeff King
Jeff King wrote:
>  sha1_name.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 31 deletions(-)

Thanks. I applied this to my series as-it-is.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12