* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-02-25 7:24 ` [PATCH 2/6 v5] revision.c: swap if/else blocks Siddharth Kannan
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
increment of unkc causes the variable in the caller to change.
Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.
There are two callers of this function:
1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv
2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg is an unknown option */
+ argv[left++] = arg;
continue;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/6 v5] revision.c: swap if/else blocks
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
2017-02-25 7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-02-25 7:24 ` [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision Siddharth Kannan
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.
No behaviour change is intended in this step.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/revision.c b/revision.c
index 5674a9a..8d4ddae 100644
--- a/revision.c
+++ b/revision.c
@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+ got_rev_arg = 1;
+ else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2257,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
append_prune_data(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
2017-02-25 7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
2017-02-25 7:24 ` [PATCH 2/6 v5] revision.c: swap if/else blocks Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-02-25 7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
setup_revisions used to consider any argument starting with "-" to be either a
valid or an unknown option.
Teach setup_revisions to check if an argument is a revision before adding it as
an unknown option (something that setup_revisions didn't understand) to argv,
and moving on to the next argument.
This patch prepares the addition of "-" as a shorthand for "previous branch".
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index 8d4ddae..5470c33 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+ int maybe_opt = 0;
if (*arg == '-') {
int opts;
@@ -2232,15 +2233,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- /* arg is an unknown option */
- argv[left++] = arg;
- continue;
+ maybe_opt = 1;
}
if (!handle_revision_arg(arg, revs, flags, revarg_opt))
got_rev_arg = 1;
- else {
+ else if (maybe_opt) {
+ /* arg is an unknown option */
+ argv[left++] = arg;
+ continue;
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
` (2 preceding siblings ...)
2017-02-25 7:24 ` [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-03-14 2:10 ` [PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily mash
2017-02-25 7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
` (2 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
This patch introduces "-" as a method to refer to a revision, and adds tests to
test that git-log works with this shorthand.
This change will touch the following commands (through
revision.c:setup_revisions):
* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c
* marked commands are information-only.
As most commands in this list are not of the rm-variety, (i.e a command that
would delete something), this change does not make it easier for people to
delete. (eg: "git branch -d -" is *not* enabled by this patch)
Suffixes like "-@{yesterday}" and "-@{2.days.ago}" are not enabled by this
patch. This is something that needs to be fixed later by making changes deeper
down the callchain.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
sha1_name.c | 5 +++
t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)
create mode 100755 t/t4214-log-shorthand.sh
diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..2f86bc9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
if (!ret)
return 0;
+ if (*name == '-' && len == 1) {
+ name = "@{-1}";
+ len = 5;
+ }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 0000000..8be2de1
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ test_commit fourth &&
+ test_commit fifth &&
+ test_commit sixth &&
+ test_commit seventh
+'
+
+test_expect_success '"log -" should not work initially' '
+ test_must_fail git log -
+'
+
+test_expect_success 'setup branches for testing' '
+ git checkout -b testing-1 master^ &&
+ git checkout -b testing-2 master~2 &&
+ git checkout master
+'
+
+test_expect_success '"log -" should work' '
+ git log testing-2 >expect &&
+ git log - >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ...@{-1} >expect.first_empty &&
+ git log @{-1}... >expect.last_empty &&
+ git log ...- >actual.first_empty &&
+ git log -... >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ..@{-1} >expect.first_empty &&
+ git log @{-1}.. >expect.last_empty &&
+ git log ..- >actual.first_empty &&
+ git log -.. >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -...testing-1 >expect &&
+ git log testing-2...testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -..testing-1 >expect &&
+ git log testing-2..testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple separate arguments should be handled properly' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log - - >expect.1 &&
+ git log @{-1} @{-1} >actual.1 &&
+ git log - HEAD >expect.2 &&
+ git log @{-1} HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+test_expect_success 'revision ranges with same start and end should be empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ test 0 -eq $(git log -...- | wc -l) &&
+ test 0 -eq $(git log -..- | wc -l)
+'
+
+test_expect_success 'suffixes to - should work' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -~ >expect.1 &&
+ git log @{-1}~ >actual.1 &&
+ git log -~2 >expect.2 &&
+ git log @{-1}~2 >actual.2 &&
+ git log -^ >expect.3 &&
+ git log @{-1}^ >actual.3 &&
+ # git log -@{yesterday} >expect.4 &&
+ # git log @{-1}@{yesterday} >actual.4 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2 &&
+ test_cmp expect.3 actual.3
+ # test_cmp expect.4 actual.4
+'
+
+test_done
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily
2017-02-25 7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
@ 2017-03-14 2:10 ` mash
0 siblings, 0 replies; 12+ messages in thread
From: mash @ 2017-03-14 2:10 UTC (permalink / raw)
To: Siddharth Kannan
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan, Git Mailing List, Stefan Beller
Move dash is previous branch check to get_sha1_basic.
Introduce helper function that gets nth prior branch switch from reflog.
Signed-off-by: mash <mash+git@crossperf.com>
---
RE: [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
> + if (*name == '-' && len == 1) {
> + name = "@{-1}";
> + len = 5;
> + }
We could avoid parsing @{-1} unnecessarily with something like this patch.
Forgive me I don't understand how the patch numbering works just yet. This is
6/6 because format-patch made it 6/6 with however I got the patches applied on
my end. This should apply cleanly on pu anyways.
Thanks to Stefan since he suggested that I might want to review this.
mash
sha1_name.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 2f86bc9..363bbe7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -568,6 +568,7 @@ static inline int push_mark(const char *string, int len)
}
static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
+static int get_branch_switch(int nth, struct strbuf *buf);
static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
@@ -628,11 +629,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
if (len && ambiguous_path(str, len))
return -1;
- if (nth_prior) {
+ if (nth_prior || !strcmp(str, "-")) {
struct strbuf buf = STRBUF_INIT;
int detached;
- if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
+ if (nth_prior ? interpret_nth_prior_checkout(str, len, &buf) > 0
+ : get_branch_switch(1, &buf) > 0) {
detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
strbuf_release(&buf);
if (detached)
@@ -1078,6 +1080,25 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
return 0;
}
+static int get_branch_switch(int nth, struct strbuf *buf)
+{
+ int retval;
+ struct grab_nth_branch_switch_cbdata cb;
+
+ cb.remaining = nth;
+ strbuf_init(&cb.buf, 20);
+
+ retval = for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch,
+ &cb);
+ if (0 < retval) {
+ strbuf_reset(buf);
+ strbuf_addbuf(buf, &cb.buf);
+ }
+
+ strbuf_release(&cb.buf);
+ return retval;
+}
+
/*
* Parse @{-N} syntax, return the number of characters parsed
* if successful; otherwise signal an error with negative value.
@@ -1086,8 +1107,6 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
struct strbuf *buf)
{
long nth;
- int retval;
- struct grab_nth_branch_switch_cbdata cb;
const char *brace;
char *num_end;
@@ -1103,18 +1122,8 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
return -1;
if (nth <= 0)
return -1;
- cb.remaining = nth;
- strbuf_init(&cb.buf, 20);
- retval = 0;
- if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
- strbuf_reset(buf);
- strbuf_addbuf(buf, &cb.buf);
- retval = brace - name + 1;
- }
-
- strbuf_release(&cb.buf);
- return retval;
+ return 0 < get_branch_switch(nth, buf) ? brace - name + 1 : 0;
}
int get_oid_mb(const char *name, struct object_id *oid)
--
2.9.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
` (3 preceding siblings ...)
2017-02-25 7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-03-01 22:49 ` Junio C Hamano
2017-02-25 7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
2017-02-25 7:32 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
The callchain for handling each argument contains the function
revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
implemented in a previous patch; the complete callchain leading to that
function is:
1. merge.c:collect_parents
2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
This patch also adds a test for checking that the shorthand works properly
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
builtin/merge.c | 2 --
t/t3035-merge-hyphen-shorthand.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
create mode 100755 t/t3035-merge-hyphen-shorthand.sh
diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb..36ff420 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1228,8 +1228,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
argc = setup_with_upstream(&argv);
else
die(_("No commit specified and merge.defaultToUpstream not set."));
- } else if (argc == 1 && !strcmp(argv[0], "-")) {
- argv[0] = "@{-1}";
}
if (!argc)
diff --git a/t/t3035-merge-hyphen-shorthand.sh b/t/t3035-merge-hyphen-shorthand.sh
new file mode 100755
index 0000000..fd37ff9
--- /dev/null
+++ b/t/t3035-merge-hyphen-shorthand.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='merge uses the shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ test_commit fourth &&
+ test_commit fifth &&
+ test_commit sixth &&
+ test_commit seventh
+'
+
+test_expect_success 'setup branches' '
+ git checkout master &&
+ git checkout -b testing-2 &&
+ git checkout -b testing-1 &&
+ test_commit eigth &&
+ test_commit ninth
+'
+
+test_expect_success 'merge - should work' '
+ git checkout testing-2 &&
+ git merge - &&
+ git rev-parse HEAD HEAD^^ | sort >actual &&
+ git rev-parse master testing-1 | sort >expect &&
+ test_cmp expect actual
+'
+
+test_done
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
2017-02-25 7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
@ 2017-03-01 22:49 ` Junio C Hamano
2017-03-01 23:05 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 22:49 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> The callchain for handling each argument contains the function
> revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
> implemented in a previous patch; the complete callchain leading to that
> function is:
>
> 1. merge.c:collect_parents
> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
>
> This patch also adds a test for checking that the shorthand works properly
This breaks "git merge".
> +test_expect_success 'merge - should work' '
> + git checkout testing-2 &&
> + git merge - &&
> + git rev-parse HEAD HEAD^^ | sort >actual &&
> + git rev-parse master testing-1 | sort >expect &&
> + test_cmp expect actual
This test is not sufficient to catch a regression I seem to be
seeing.
$ git checkout side
$ git checkout pu
$ git merge -
used to say "Merge branch 'side' into pu". With this series merged,
I seem to be getting "Merge commit '-' into pu".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
2017-03-01 22:49 ` Junio C Hamano
@ 2017-03-01 23:05 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 23:05 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
Junio C Hamano <gitster@pobox.com> writes:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> The callchain for handling each argument contains the function
>> revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
>> implemented in a previous patch; the complete callchain leading to that
>> function is:
>>
>> 1. merge.c:collect_parents
>> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
>>
>> This patch also adds a test for checking that the shorthand works properly
>
> This breaks "git merge".
>
>> +test_expect_success 'merge - should work' '
>> + git checkout testing-2 &&
>> + git merge - &&
>> + git rev-parse HEAD HEAD^^ | sort >actual &&
>> + git rev-parse master testing-1 | sort >expect &&
>> + test_cmp expect actual
>
> This test is not sufficient to catch a regression I seem to be
> seeing.
>
> $ git checkout side
> $ git checkout pu
> $ git merge -
>
> used to say "Merge branch 'side' into pu". With this series merged,
> I seem to be getting "Merge commit '-' into pu".
You stopped at get_sha1_1() in your 3817cebabc ("sha1_name.c: teach
get_sha1_1 "-" shorthand for "@{-1}"", 2017-02-25), instead of going
down to get_sha1_basic() and teaching it that "-" means the same
thing as "@{-1}", which would in turn require you to teach
dwim_ref() that "-" is the same thing as "@{-1}". As dwim_ref()
does not know about "-" and does not expand it to the refname like
it expands "@{-1}", it would break and that is why 3817cebabc punts
at a bit higher in the callchain.
The breakage by this patch to "git merge" happens for the same
reason. cmd_merge() calls collect_parents() which annotates the
commits that are merged with their textual name, which used to be
"@{-1}" without this patch but now "-" is passed as-is. This
annotation will be given to merge_name(), and the first thing it
does is dwim_ref(). The function knows what to do with "@{-1}",
but it does not know what to do with "-", and that is why you end up
producing "Merge commit '-' into ...".
Dropping this patch from the series would make things consistent
with what was done in 3817cebabc and I think that is a sensible
place to stop. After the dust settles, We _can_ later dig further
by teaching dwim_ref() and friends what "-" means, and when it is
done, this patch would become useful.
Thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
` (4 preceding siblings ...)
2017-02-25 7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
@ 2017-02-25 7:24 ` Siddharth Kannan
2017-03-01 23:18 ` Junio C Hamano
2017-02-25 7:32 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
revert.c:run_sequencer calls setup_revisions right after replacing "-" with
"@{-1}" for this shorthand. A previous patch taught setup_revisions to handle
this shorthand by doing the required replacement inside revision.c:get_sha1_1.
Hence, the code here is redundant and has been removed.
This patch also adds a test to check that revert recognizes the "-" shorthand.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
builtin/revert.c | 2 --
t/t3514-revert-shorthand.sh | 25 +++++++++++++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
create mode 100755 t/t3514-revert-shorthand.sh
diff --git a/builtin/revert.c b/builtin/revert.c
index 4ca5b51..0bc6657 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -155,8 +155,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
if (argc < 2)
usage_with_options(usage_str, options);
- if (!strcmp(argv[1], "-"))
- argv[1] = "@{-1}";
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
diff --git a/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh
new file mode 100755
index 0000000..51f8c81d
--- /dev/null
+++ b/t/t3514-revert-shorthand.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first
+'
+
+test_expect_success 'setup branches' '
+ echo "hello" >hello &&
+ cat hello >expect &&
+ git add hello &&
+ git commit -m "hello first commit" &&
+ echo "world" >>hello &&
+ git commit -am "hello second commit" &&
+ git checkout -b testing-1 &&
+ git checkout master &&
+ git revert --no-edit - &&
+ cat hello >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
2017-02-25 7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
@ 2017-03-01 23:18 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 23:18 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> revert.c:run_sequencer calls setup_revisions right after replacing "-" with
> "@{-1}" for this shorthand. A previous patch taught setup_revisions to handle
> this shorthand by doing the required replacement inside revision.c:get_sha1_1.
>
> Hence, the code here is redundant and has been removed.
>
> This patch also adds a test to check that revert recognizes the "-" shorthand.
Unlike "merge" [*1*], I think this one is OK because "git revert
$commit" does not try to say _how_ the commit was given, and most
importantly, it does not say what branch the reverted thing was.
Thanks.
[Footnote]
*1* Probably "checkout" would exhibit the same issue as we saw in
5/6 for "git merge" if you remove the "- to @{-1}" conversion
from it.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
2017-02-25 7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
` (5 preceding siblings ...)
2017-02-25 7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
@ 2017-02-25 7:32 ` Siddharth Kannan
6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25 7:32 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
increment of unkc causes the variable in the caller to change.
Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.
There are two callers of this function:
1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv
2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg is an unknown option */
+ argv[left++] = arg;
continue;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread