[PATCH 0/7] Let's get that @{push}!

[PATCH 0/7] Let's get that @{push}!

[Ramkumar Ramachandra]

[7/7] is the meat.  Sorry it's in such a messy state: I was having a
field day tracing what push is actually doing.  Anyway, I wanted to
send out the series now to get early feedback.

In other news: why on earth is push doing _so_ much processing before
pushing?  Is it written very badly, or am I missing something?

Thanks.

(based on rr/die-on-missing-upstream)

Ramkumar Ramachandra (7):
  sha1_name: abstract upstream_mark() logic
  sha1_name: factor out die_no_upstream()
  sha1_name: remove upstream_mark()
  remote: expose parse_push_refspec()
  remote: expose get_ref_match()
  sha1_name: prepare to introduce AT_KIND_PUSH
  sha1_name: implement finding @{push}

 remote.c    |   4 +--
 remote.h    |   4 +++
 sha1_name.c | 111 ++++++++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 88 insertions(+), 31 deletions(-)

-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 1/7] sha1_name: abstract upstream_mark() logic

[Ramkumar Ramachandra]

Currently, the only non-numerical @{...} expression we support is
@{u[pstream]}.  Since we're slowly growing git to support triangular
workflows, it might make sense to have a @{p[ush]} and various other
special @{...} expressions in the future.  To prepare for this, abstract
out the upstream_mark() logic to accept any suffix, while preserving the
upstream_mark() interface.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6928cc7..766e4e9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -23,6 +23,8 @@ struct disambiguate_state {
 	unsigned always_call_fn:1;
 };
 
+#define AT_KIND_UPSTREAM 0
+
 static void update_candidates(struct disambiguate_state *ds, const unsigned char *current)
 {
 	if (ds->always_call_fn) {
@@ -416,20 +418,40 @@ 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, int *kind)
 {
-	const char *suffix[] = { "@{upstream}", "@{u}" };
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
-		int suffix_len = strlen(suffix[i]);
-		if (suffix_len <= len
-		    && !memcmp(string, suffix[i], suffix_len))
-			return suffix_len;
+	int i, j;
+
+	static struct {
+		int kind;
+		const char *suffix[2];
+	} at_form[] = {
+		{ AT_KIND_UPSTREAM, { "@{upstream}", "@{u}" } }
+	};
+
+	for (j = 0; j < ARRAY_SIZE(at_form); j++) {
+		for (i = 0; i < ARRAY_SIZE(at_form[j].suffix); i++) {
+			int suffix_len = strlen(at_form[j].suffix[i]);
+			if (suffix_len <= len
+				&& !memcmp(string, at_form[j].suffix[i], suffix_len)) {
+				if (kind)
+					*kind = at_form[j].kind;
+				return suffix_len;
+			}
+		}
 	}
 	return 0;
 }
 
+static inline int upstream_mark(const char *string, int len)
+{
+	int suffix_len, kind;
+	suffix_len = at_mark(string, len, &kind);
+	if (suffix_len && kind == AT_KIND_UPSTREAM)
+		return suffix_len;
+	return 0;
+}
+
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 2/7] sha1_name: factor out die_no_upstream()

[Ramkumar Ramachandra]

Currently interpret_branch_name() tries to parse various things, and
finally falls back to parsing <branch>@{u[pstream]}.  It dies if the
input string contained an "@{u[pstream]}" but an upstream could not be
found.  The logic can be generalized to check for any branch property
after branch_get().  In preparation for introducing more special @{...}
forms, factor out die_no_upstream().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 766e4e9..7aabd94 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -998,6 +998,24 @@ int get_sha1_mb(const char *name, unsigned char *sha1)
 	return st;
 }
 
+static void die_no_upstream(struct branch *upstream, char *name) {
+	/*
+	 * 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'"), name);
+		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);
+	}
+}
+
 /*
  * 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
@@ -1022,7 +1040,7 @@ int get_sha1_mb(const char *name, unsigned char *sha1)
 int interpret_branch_name(const char *name, struct strbuf *buf)
 {
 	char *cp;
-	struct branch *upstream;
+	struct branch *branch;
 	int namelen = strlen(name);
 	int len = interpret_nth_prior_checkout(name, buf);
 	int tmp_len;
@@ -1059,24 +1077,10 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 		return -1;
 	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);
-	}
+	branch = branch_get(*cp ? cp : NULL);
+	die_no_upstream(branch, cp);
 	free(cp);
-	cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
+	cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
 	strbuf_reset(buf);
 	strbuf_addstr(buf, cp);
 	free(cp);
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 3/7] sha1_name: remove upstream_mark()

[Ramkumar Ramachandra]

The first caller get_sha1_basic() just wants to make sure that no
non-numerical @{...} form was matched, so that it can proceed with
processing numerical @{...} forms.  Since we're going to introduce more
non-numerical @{...} forms, replace this upstream_mark() call with a
call to at_mark() passing NULL as the last argument; we don't care what
the kind is: all we need to know is if the return value is zero (parse
failure).

The second caller interpret_branch_name() will be expanded in the future
to handle all possible AT_KIND_* values.  So, replace the
upstream_mark() call with an upstream_mark() call capturing at_kind and
using it in a switch statement to perform the appropriate action.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 7aabd94..106716e 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -443,15 +443,6 @@ static inline int at_mark(const char *string, int len, int *kind)
 	return 0;
 }
 
-static inline int upstream_mark(const char *string, int len)
-{
-	int suffix_len, kind;
-	suffix_len = at_mark(string, len, &kind);
-	if (suffix_len && kind == AT_KIND_UPSTREAM)
-		return suffix_len;
-	return 0;
-}
-
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
@@ -469,7 +460,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && str[len-1] == '}') {
 		for (at = len-2; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
-				if (!upstream_mark(str + at, len - at)) {
+				if (!at_mark(str + at, len - at, NULL)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -1044,6 +1035,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	int namelen = strlen(name);
 	int len = interpret_nth_prior_checkout(name, buf);
 	int tmp_len;
+	int at_kind;
 
 	if (!len)
 		return len; /* syntax Ok, not enough switches */
@@ -1072,15 +1064,21 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	cp = strchr(name, '@');
 	if (!cp)
 		return -1;
-	tmp_len = upstream_mark(cp, namelen - (cp - name));
+	tmp_len = at_mark(cp, namelen - (cp - name), &at_kind);
 	if (!tmp_len)
 		return -1;
 	len = cp + tmp_len - name;
 	cp = xstrndup(name, cp - name);
 	branch = branch_get(*cp ? cp : NULL);
-	die_no_upstream(branch, cp);
-	free(cp);
-	cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+
+	switch (at_kind) {
+	case AT_KIND_UPSTREAM:
+		die_no_upstream(branch, cp);
+		free(cp);
+		cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+		break;
+	}
+
 	strbuf_reset(buf);
 	strbuf_addstr(buf, cp);
 	free(cp);
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 4/7] remote: expose parse_push_refspec()

[Ramkumar Ramachandra]

parse_fetch_refspec() is already available to other callers via
remote.h.  There's no reason why parse_push_refspec() shouldn't be.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 2 +-
 remote.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 68eb99b..99c44da 100644
--- a/remote.c
+++ b/remote.c
@@ -660,7 +660,7 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
 }
 
-static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 {
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
 }
diff --git a/remote.h b/remote.h
index cf56724..2497b93 100644
--- a/remote.h
+++ b/remote.h
@@ -94,6 +94,7 @@ void ref_remove_duplicates(struct ref *ref_map);
 
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);
 
 void free_refspec(int nr_refspec, struct refspec *refspec);
 
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 5/7] remote: expose get_ref_match()

[Ramkumar Ramachandra]

We need it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 2 +-
 remote.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 99c44da..9ae1fc5 100644
--- a/remote.c
+++ b/remote.c
@@ -1168,7 +1168,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 	return errs;
 }
 
-static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
+char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
 		int send_mirror, int direction, const struct refspec **ret_pat)
 {
 	const struct refspec *pat;
diff --git a/remote.h b/remote.h
index 2497b93..5671fe0 100644
--- a/remote.h
+++ b/remote.h
@@ -173,4 +173,7 @@ struct ref *guess_remote_head(const struct ref *head,
 /* Return refs which no longer exist on remote */
 struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map);
 
+char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
+		int send_mirror, int direction, const struct refspec **ret_pat);
+
 #endif
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 6/7] sha1_name: prepare to introduce AT_KIND_PUSH

[Ramkumar Ramachandra]

Introduce an AT_KIND_PUSH to be represented as "@{p[ush]}".  Determine
it using branch.remote.push_refspec.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 106716e..5f6958b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -23,7 +23,7 @@ struct disambiguate_state {
 	unsigned always_call_fn:1;
 };
 
-#define AT_KIND_UPSTREAM 0
+enum at_kind { AT_KIND_UPSTREAM, AT_KIND_PUSH };
 
 static void update_candidates(struct disambiguate_state *ds, const unsigned char *current)
 {
@@ -426,7 +426,8 @@ static inline int at_mark(const char *string, int len, int *kind)
 		int kind;
 		const char *suffix[2];
 	} at_form[] = {
-		{ AT_KIND_UPSTREAM, { "@{upstream}", "@{u}" } }
+		{ AT_KIND_UPSTREAM, { "@{upstream}", "@{u}" } },
+		{ AT_KIND_PUSH, { "@{push}", "@{p}" } }
 	};
 
 	for (j = 0; j < ARRAY_SIZE(at_form); j++) {
@@ -1022,6 +1023,12 @@ static void die_no_upstream(struct branch *upstream, char *name) {
  *   given buf and returns the number of characters parsed if
  *   successful.
  *
+ * - "<branch>@{push}" finds the name of the ref that
+ *   <branch> is configured to push to (missing <branch> defaults
+ *   to the current branch), and places the name of the branch in the
+ *   given buf and returns the number of characters parsed if
+ *   successful.
+ *
  * If the input is not of the accepted format, it returns a negative
  * number to signal an error.
  *
@@ -1077,6 +1084,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 		free(cp);
 		cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
 		break;
+	case AT_KIND_PUSH:
+		break;
 	}
 
 	strbuf_reset(buf);
-- 
1.8.3.rc3.17.gd95ec6c.dirty

Re: [PATCH 6/7] sha1_name: prepare to introduce AT_KIND_PUSH

[Felipe Contreras]

On Thu, May 23, 2013 at 10:12 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Introduce an AT_KIND_PUSH to be represented as "@{p[ush]}".  Determine
> it using branch.remote.push_refspec.

I think the semantics of this don't make any sense.

  git push branch@{upstream}

Is very clear: push upstream of branch.

  git push branch@{push}

Is not clear at all: push push of branch?

Is it a noun, or a verb?

I would expect branch@{X} to be 'the X of branch', and "the push of
branch" doesn't make sense. Whatever X ends up being, it should be a
noun.

-- 
Felipe Contreras

Re: [PATCH 6/7] sha1_name: prepare to introduce AT_KIND_PUSH

[Ramkumar Ramachandra]

Felipe Contreras wrote:
>   git push branch@{push}
>
> Is not clear at all: push push of branch?

We can pick the name later.  I had to pick a name to write code, and
that happens to be @{push}.

[PATCH 7/7] sha1_name: implement finding @{push}

[Ramkumar Ramachandra]

Try this now: configure your current branch's pushremote to push to
"refs/heads/*:refs/heads/rr/*".  Now, type 'git show @{p}'.  Voila!

It currently only works when:

1. remote.<name>.push is explicitly specified.
2. There is a pattern to match (*).

Proof-of-concept only.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/sha1_name.c b/sha1_name.c
index 5f6958b..283d538 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1008,6 +1008,24 @@ static void die_no_upstream(struct branch *upstream, char *name) {
 	}
 }
 
+static void find_push_ref(struct branch *branch) {
+	struct remote *remote = pushremote_get(NULL);
+	const struct refspec *pat = NULL;
+	char raw_ref[PATH_MAX];
+	struct ref *this_ref;
+	char *dst_name;
+	int len;
+
+	sprintf(raw_ref, "refs/heads/%s", branch->name);
+	len = strlen(raw_ref) + 1;
+	this_ref = xcalloc(1, sizeof(*this_ref) + len);
+	memcpy(this_ref->name, raw_ref, len);
+
+	dst_name = get_ref_match(remote->push, remote->push_refspec_nr,
+				this_ref, MATCH_REFS_ALL, 0, &pat);
+	printf("dst_name = %s\n", dst_name);
+}
+
 /*
  * 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
@@ -1085,6 +1103,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 		cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
 		break;
 	case AT_KIND_PUSH:
+		find_push_ref(branch);
+		die("Done!");
 		break;
 	}
 
-- 
1.8.3.rc3.17.gd95ec6c.dirty

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Duy Nguyen]

(I haven't caught up with git mails lately, but the @{special}
refactoring caught my eyes..)

On Thu, May 23, 2013 at 10:12 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Try this now: configure your current branch's pushremote to push to
> "refs/heads/*:refs/heads/rr/*".  Now, type 'git show @{p}'.  Voila!

Voila what? Why not avoid guessing game and describe what the patch is for?

> +static void find_push_ref(struct branch *branch) {
> +       struct remote *remote = pushremote_get(NULL);
> +       const struct refspec *pat = NULL;
> +       char raw_ref[PATH_MAX];
> +       struct ref *this_ref;
> +       char *dst_name;
> +       int len;
> +
> +       sprintf(raw_ref, "refs/heads/%s", branch->name);
> +       len = strlen(raw_ref) + 1;
> +       this_ref = xcalloc(1, sizeof(*this_ref) + len);
> +       memcpy(this_ref->name, raw_ref, len);
> +
> +       dst_name = get_ref_match(remote->push, remote->push_refspec_nr,
> +                               this_ref, MATCH_REFS_ALL, 0, &pat);
> +       printf("dst_name = %s\n", dst_name);
> +}
> +

Isn't this an abuse of extended sha-1 syntax? How can I combine this
with other @{}, ^, ~...?
--
Duy

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> On Thu, May 23, 2013 at 10:12 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Try this now: configure your current branch's pushremote to push to
>> "refs/heads/*:refs/heads/rr/*".  Now, type 'git show @{p}'.  Voila!
>
> Voila what? Why not avoid guessing game and describe what the patch is for?

If you're on branch master, it'll output refs/heads/rr/master.  The
topic is about having a @{push} corresponding to @{upstream}

>> +static void find_push_ref(struct branch *branch) {
>> +       struct remote *remote = pushremote_get(NULL);
>> +       const struct refspec *pat = NULL;
>> +       char raw_ref[PATH_MAX];
>> +       struct ref *this_ref;
>> +       char *dst_name;
>> +       int len;
>> +
>> +       sprintf(raw_ref, "refs/heads/%s", branch->name);
>> +       len = strlen(raw_ref) + 1;
>> +       this_ref = xcalloc(1, sizeof(*this_ref) + len);
>> +       memcpy(this_ref->name, raw_ref, len);
>> +
>> +       dst_name = get_ref_match(remote->push, remote->push_refspec_nr,
>> +                               this_ref, MATCH_REFS_ALL, 0, &pat);
>> +       printf("dst_name = %s\n", dst_name);
>> +}
>> +
>
> Isn't this an abuse of extended sha-1 syntax? How can I combine this
> with other @{}, ^, ~...?

I'm unsure what you mean.  How can I be on branch master^1?  Did you
read the cover-letter?

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Duy Nguyen]

On Fri, May 24, 2013 at 11:15 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Duy Nguyen wrote:
>> On Thu, May 23, 2013 at 10:12 PM, Ramkumar Ramachandra
>> <artagnon@gmail.com> wrote:
>>> Try this now: configure your current branch's pushremote to push to
>>> "refs/heads/*:refs/heads/rr/*".  Now, type 'git show @{p}'.  Voila!
>>
>> Voila what? Why not avoid guessing game and describe what the patch is for?
>
> If you're on branch master, it'll output refs/heads/rr/master.  The
> topic is about having a @{push} corresponding to @{upstream}

Then "show @{p}" should show the tip commit of rr/master, not the ref
name. rev-parse (with an option, maybe) may be a better place for
this.

>>> +       dst_name = get_ref_match(remote->push, remote->push_refspec_nr,
>>> +                               this_ref, MATCH_REFS_ALL, 0, &pat);
>>> +       printf("dst_name = %s\n", dst_name);
>>> +}
>>> +
>>
>> Isn't this an abuse of extended sha-1 syntax? How can I combine this
>> with other @{}, ^, ~...?
>
> I'm unsure what you mean.  How can I be on branch master^1?  Did you
> read the cover-letter?

I did not expect @{p} to printf(). If it's part of get_sha1(), how can
it return an sha-1? And the cover letter said "7/7 is the meat". Not
very informative.
--
Duy

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Ramkumar Ramachandra]

Duy Nguyen wrote:
> Then "show @{p}" should show the tip commit of rr/master, not the ref
> name.

Yes, that is correct.

> rev-parse (with an option, maybe) may be a better place for
> this.

Er, no.  I actually want things like diff @{p}..HEAD.  I want it to be
a first-class revision, just like @{u}.

> I did not expect @{p} to printf(). If it's part of get_sha1(), how can
> it return an sha-1? And the cover letter said "7/7 is the meat". Not
> very informative.

I also said sorry for the horrible mess ;)
Yes, it's obviously not supposed to print and die() with a "Done!":
this was a development session, and I hit send-email as soon as I got
the right output.  It's supposed to behave exactly like @{u} (failing
to resolve occasionally).

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Duy Nguyen wrote:
>> Then "show @{p}" should show the tip commit of rr/master, not the ref
>> name.
>
> Yes, that is correct.
>
>> rev-parse (with an option, maybe) may be a better place for
>> this.
>
> Er, no.  I actually want things like diff @{p}..HEAD.  I want it to be
> a first-class revision, just like @{u}.

I think Duy's suggestion makes perfect sense; rev-parse already has
a mechanism to expand @{u} to the full name, so if you are hooking
into the same codepath as @{u} to implement the "I publish there"
information, which I think you should, you already should have it
for free.

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>>> rev-parse (with an option, maybe) may be a better place for
>>> this.
>>
>> Er, no.  I actually want things like diff @{p}..HEAD.  I want it to be
>> a first-class revision, just like @{u}.
>
> I think Duy's suggestion makes perfect sense; rev-parse already has
> a mechanism to expand @{u} to the full name, so if you are hooking
> into the same codepath as @{u} to implement the "I publish there"
> information, which I think you should, you already should have it
> for free.

*scratches head*

Okay, I'm not understanding this.  I've implemented @{push} as a
revision, so all callers who know how to get_sha1_basic() will be able
to resolve this (including rev-parse).  Are you talking about
--symbolic-full-name?  That just calls dwim_ref(), which calls
interpret_branch_name() anyway: except you get a symbolic name instead
of the sha1.  Why do I have to think about rev-parse specifically in
this patch series?  rev-parse has no special logic for @{u} either.

The codepath that resolves @{u} is in interpret_branch_name(): it's
just a matter of reading branch->merge[0]->dst; it's trivial to
determine because read_config() just reads the configuration file and
fills in these values when you get_branch().  How do I get "I publish
there" information for free?  Where is it contained?  In fact, it's so
complicated to get that information that I had to break my head to
even get this far (after mucking around in the transport layer);
that's what I'm trying to show in this series.  Unless I'm very badly
mistaken, it's _impossible_ for any codepath in git to determine where
a push will go, except the one activated by invoking the builtin push.

The long-term impact of this series is not just @{push}, but that
anyone else in git will be able to determine where a push is supposed
to go.  Ultimately, it can lead to very heavy optimizations of the
transport_push() codepath (which is currently super-convoluted unless
I'm missing something).

Re: [PATCH 7/7] sha1_name: implement finding @{push}

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>>> rev-parse (with an option, maybe) may be a better place for
>>>> this.
>>>
>>> Er, no.  I actually want things like diff @{p}..HEAD.  I want it to be
>>> a first-class revision, just like @{u}.
>>
>> I think Duy's suggestion makes perfect sense; rev-parse already has
>> a mechanism to expand @{u} to the full name, so if you are hooking
>> into the same codepath as @{u} to implement the "I publish there"
>> information, which I think you should, you already should have it
>> for free.
>
> *scratches head*
>
> Okay, I'm not understanding this.
> ...  How do I get "I publish
> there" information for free?

That is not what I said.

Let's step back a bit and think what it means that @{u} can be used
to name "The latest, as best of my knowledge, of the possibly moving
ref that my work is based on".  You can do so by

 (1) having a ref that points at such commit and having a mechanism
     to keep the ref up to date;

 (2) having a mechansim to turn @{u} to such a ref; and

 (3) letting get_sha1() machinery to read from that ref when an
     object name is needed for @{u}.

None of the above is what rev-parse does, but because you have (2),
rev-parse --symbolic-full-name can just ask (2) for a refname.

That is what I meant by "for free", and "which I think you should"
refers to all of these three things you would do to keep track of
"The latest, as best of my knowledge, of the possibly moving ref
that points at the commit I pushed the last time" for your @{p} in a
way similar to how @{u} works.

If I understand correctly the discussion so far, for a given branch
B, "git push" (no arguments to specify to which repository to push
nor what branch to update) would decide what the user did not say by
looking at:

    - branch.B.pushremote (and using remote.pushdefault and
      branch.B.remote as fallbacks) to find what repository R to
      push to;

    - branch.B.push (or mapping B through remote.R.push refspecs as
      a fallback logic) to find what destination branch D at R is
      updated by B.

Then after "git push" succeeds, it would look at remote.R.fetch
refspecs to pretend as if we fetched immediately from R, which is
how we keep track of the "last pushed" state, which is (1) of the
necessary three.

It follows that for B@{p} to be "The last pushed" for B, it has to
resolve to a remote tracking branch for D from R.  That (i.e. find R
and D and then map it through R's fetch refspec) is the logic we
need for (2).  That is very similar for B@{u}, which should be to
find the repository O it comes from by looking at branch.B.remote
(or 'origin'), and then where branch.B.merge from O is stored in our
ref namespace (e.g. refs/remotes/origin/topic) by mapping it through
the remote.O.fetch refspec.

Once you have (2), the implementation of (3) is quite obvious.  We
know where in the codeflow @{u} is turned into a ref; we can do the
same for @{p} by calling the logic (2) from the same place to turn
@{p} into a ref.

It is worth noting that @{p} has one error case that @{u} does not,
whose only error case is essentially that the branch you are asking
for @{u} does not have "upstream" configured.

For "git push" to work, you only need to be able to compute R and D
for B.  But R does not necessarily need to have fetch refspec for a
triangular pushing to work, hence we may not be recording what we
pushed the last time, violating (1), which in turn means (2) may not
have an answer.