Re: [PATCH v2 GSoC RFC] diff: allow "-" as a short-hand for "last branch"
On Fri, Mar 10, 2017 at 04:52:07AM +, mash wrote: > Maybe you can reuse the diff tests. I'll do another microproject then. Yeah, definitely. If there are more tests required, then I will reuse your ones! > > mash > > The original message doesn't seem to cc the mailing list: Thanks! It was rather daft of me to not realise this. I was waiting for it to appear on public-inbox. I re-sent it with the CC. The timestamp is a little bit skewed, but I think it should make sense. Thanks, Siddharth.
[PATCH v2 GSoC RFC] diff: allow "-" as a short-hand for "last branch"
Hey, I have already worked on this, and I made the change inside sha1_name.c. The final version of my patch is here[1]. > Handling the dash in sha1_name:get_sha1_basic is not an issue but > git > was designed with the dash in mind for options not for this weird > short-hand so as long as there's no decision made that git should > actually have this short-hand everywhere it does not seem like a > good > idea to change anything in there because it would probably have > unwanted side-effects. Actually, this was discussed even when I was working on this patch. I said [2] > Making a change in sha1_name.c will touch a lot of commands > (setup_revisions is called from everywhere in the codebase), so, I > am > still trying to figure out how to do this such that the rest of the > codepath remains unchanged. Matthieu replied to this [3] > I don't have strong opinion on this: I tend to favor consistency and > supporting "-" everywhere goes in this direction, but I think the > downsides should be considered too. A large part of the exercice > here > is to write a good commit message! >From the discussion over the different versions of my patch, I get the feeling that enabling this shorthand for all the commands is the direction that git wants to move in. Sorry about the time you spent on this patch. [1]: http://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddhart...@gmail.com/ [2]: https://public-inbox.org/git/20170207191450.GA5569@ubuntu-512mb-blr1-01.localdomain/ [3]: https://public-inbox.org/git/vpqh944eof7@anie.imag.fr/ Thanks, Siddharth. P.S. This message was sent _before_ 1cmcxh-nd...@crossperf.com but I didn't CC The mailing list in that message. I am sending it with the mailing list cc-ed to ensure that the conversation makes sense.
Re: [PATCH GSoC] Allow "-" as a short-hand for "@{-1}" in branch deletions
Hey Shuyang, On Thu, Mar 09, 2017 at 09:47:12AM -0800, Stefan Beller wrote: > > The "-" shorthand that stands for "the branch we were previously on", > > like we did for "git merge -" sometime after we introduced "git checkout -". > > Now I am introducing this shorthand to branch delete, i.e. > > "git branch -d -". > > > > More reference: > > https://public-inbox.org/git/7vppuewl6h@alter.siamese.dyndns.org/ > 1. I have already worked on this project, and my patch is in the "Needs review" section in "What's cooking". It implements this change inside sha1_name.c and doesn't touch git branch. So, your patch is mutually exclusive to my previous patch. 2. Matthieu made an argument against enabling commands like "git branch -D -" even by mistake [1]. The way that I have implemented ensured that not a lot of "rm"-like commands were enabled. My patch that would enable this shorthand for other projects is here[2]. [1]: http://public-inbox.org/git/vpqh944eof7@anie.imag.fr/ [2]: http://public-inbox.org/git/1488007487-12965-5-git-send-email-kannan.siddhart...@gmail.com/ Thanks, Siddharth.
[PATCH 1/6 v5] revision.c: do not update argv with unknown option
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.siddhart...@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(>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
[PATCH 1/6 v5] revision.c: do not update argv with unknown option
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.siddhart...@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(>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
[PATCH 2/6 v5] revision.c: swap if/else blocks
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.siddhart...@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(_data, argv + i); break; } - else - got_rev_arg = 1; } if (prune_data.nr) { -- 2.1.4
[PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
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.siddhart...@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(_r_opt, 0, sizeof(s_r_opt)); s_r_opt.assume_dashdash = 1; argc = setup_revisions(argc, argv, opts->revs, _r_opt); diff --git a/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh new file mode 100755 index 000..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
[PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
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.siddhart...@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(); 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 000..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
[PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
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.siddhart...@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 000..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 e
[PATCH 3/6 v5] revision.c: args starting with "-" might be a revision
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.siddhart...@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
[PATCH 0/6 v5] allow "-" as a shorthand for "previous branch"
An updated version of the patch [1]. Discussion here[1] has been taken into account. The test for "-@{yesterday}" is there inside the log-shorthand test, it is commented out for now. I have removed the redundant pieces of code in merge.c and revert.c as mentioned by Matthieu in [2]. As analysed by Junio[3], the same type of code inside checkout.c and worktree.c can not be removed because the appropriate functions inside revision.c are not called in their codepaths. Thanks for your review of the previous versions, Junio and Matthieu! [1]: 1487258054-32292-1-git-send-email-kannan.siddhart...@gmail.com [2]: vpqbmu768on@anie.imag.fr [3]: xmqq1sv1euob@gitster.mtv.corp.google.com Siddharth Kannan (6): revision.c: do not update argv with unknown option revision.c: swap if/else blocks revision.c: args starting with "-" might be a revision sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 revert.c: delegate handling of "-" shorthand to setup_revisions builtin/merge.c | 2 - builtin/revert.c | 2 - revision.c| 15 +++--- sha1_name.c | 5 ++ t/t3035-merge-hyphen-shorthand.sh | 33 t/t3514-revert-shorthand.sh | 25 + t/t4214-log-shorthand.sh | 106 ++ 7 files changed, 178 insertions(+), 10 deletions(-) create mode 100755 t/t3035-merge-hyphen-shorthand.sh create mode 100755 t/t3514-revert-shorthand.sh create mode 100755 t/t4214-log-shorthand.sh -- 2.1.4
Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
On 21 February 2017 at 02:00, Junio C Hamano <gits...@pobox.com> wrote: > Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > > So, is it okay to stop with just supporting "-" and not support things > > like "-@{yesterday}"? > > If the approach to turn "-" into "@{-1}" at that spot you did will > cause "-@{yesterday}" to barf, then I'd say so be it for now ;-). > We can later spread the understanding of "-" to functions deeper in > the callchain and add support for that, no? Yes, this can be done later. I will send these patches again, with only the changes that are discussed here. I will keep the tests for "-@{yesterday}" as failing tests, if that would help in finding this again and fixing it later. Thanks for your review, Junio! -- Best Regards, - Siddharth Kannan.
Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
On 17 February 2017 at 00:38, Junio C Hamano <gits...@pobox.com> wrote: > Having said all that, I do not think the remainder of the code is > prepared to take "-", not yet anyway [*1*], so turning "-" into > "@{-1}" this patch does before it calls get_sha1_basic(), while it > is not an ideal final state, is probably an acceptable milestone to > stop at. So, is it okay to stop with just supporting "-" and not support things like "-@{yesterday}"? Matthieu's comments on the matter: Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > As per Matthieu's comments, I have updated the tests, but there is still one > thing that is not working: log -@{yesterday} or log -@{2.days.ago} Note that I did not request that these things work, just that they seem to be relevant tests: IMHO it's OK to reject them, but for example we don't want them to segfault. And having a test is a good hint that you thought about what could happen and to document it. [Quoted from email <vpqa89mnl4z@anie.imag.fr>] > > It is a separate matter if this patch is sufficient to produce > correct results, though. I haven't studied the callers of this > change to make sure yet, and may find bugs in this approach later. > -- Best Regards, - Siddharth Kannan.
Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
Hey Junio and Matthieu, On 17 February 2017 at 00:19, Junio C Hamano <gits...@pobox.com> wrote: > Matthieu Moy <matthieu@grenoble-inp.fr> writes: > >> Siddharth Kannan <kannan.siddhart...@gmail.com> writes: >> >>> This is as per our discussion[1]. The patches and commit messages are based >>> on >>> Junio's patches that were posted as a reply to >>> <20170212184132.12375-1-gits...@pobox.com>. >>> >>> As per Matthieu's comments, I have updated the tests, but there is still one >>> thing that is not working: log -@{yesterday} or log -@{2.days.ago} >> >> Note that I did not request that these things work, just that they seem >> to be relevant tests: IMHO it's OK to reject them, but for example we >> don't want them to segfault. And having a test is a good hint that you >> thought about what could happen and to document it. > > The branch we were on before would be a ref, and the ref may know > where it was yesterday? If @{-1}@{1.day} works it would be natural > to expect -@{1.day} to, too, but there probably is some disambiguity > or other reasons that they cannot or should not work that way I am > missing, in which case it is fine ("too much work for too obscure > feature that is not expected to be used often" is also an acceptable > reason) to punt or deliberately not support it, as long as it is > explained in the log and/or doc (future developers need to know if > we are simply punting, or if we found a case where it would hurt end > user experience if we supported the feature), and as long as it does > not do a wrong thing (dying with "we do not support it" is OK, > segfaulting or doing random other things is not). > Right now, these commands die with an "fatal: unrecognized argument: -@{yesterday}" or a "fatal: unrecognized argument: -@{2.days.ago}". So, it is definitely not doing anything "random" :) I will wait for consensus on whether these should or should not be supported. -- Best Regards, - Siddharth.
Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
Hey Matthieu, On 16 February 2017 at 23:52, Matthieu Moywrote: > > Indeed, I misread the patch. The explanation could be a little bit more > "tired-reviewer-proof" by not using a past tone, perhaps > > 1. setup_revision, which is changed to ... Oh, okay! Sorry about the confusion! Yes, I used the past perfect tense to refer to changes that were made in this particular patch! I will change the message in the next version to something that's in present tense. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- Best Regards, - Siddharth.
[PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
This is as per our discussion[1]. The patches and commit messages are based on Junio's patches that were posted as a reply to <20170212184132.12375-1-gits...@pobox.com>. As per Matthieu's comments, I have updated the tests, but there is still one thing that is not working: log -@{yesterday} or log -@{2.days.ago} For the other kinds of suffixes, such as -^ or -~ or -~N, the suffix information is first extracted and then, the function get_sha1_1 is called with name="-^" and len=1 (which is the reason for the changed condition inside Patch 4 of this series). For -@{yesterday} kind of queries, the functions dwim_log, interpret_branch_name and interpret_nth_prior_checkout are called. 1. A nice way to solve this would be to extend the replacement of "-" with "@{-1}" one step further. Using strbuf, instead of replacing the whole string with "@{-1}" we would simply replace "-" with "@{-1}" expanding the string appropriately. This will ensure that all the code is inside the function get_sha1_1. The code to do this is in the cover section of the 4th patch in this series. 2. we could go down the dwim_log codepath, and find another suitable place to make the same "-" -> "@{-1}" replacement. In the time that I spent till now, it seems that the suffix information (i.e. @{yesterday} or @{2.days.ago}) is extracted _after_ the branch information has been extracted, so I suspect that we will have to keep that part intact even in this solution. (I am not too sure about this. If this is the preferred solution, then I will dig deeper and find the right place as I did for the first part of this patch) Matthieu: Thanks a lot for your comments on the tests! test_commit has made the tests a lot cleaner! [1]: <xmqqh941ippo@gitster.mtv.corp.google.com> Siddharth Kannan (4): revision.c: do not update argv with unknown option revision.c: swap if/else blocks revision.c: args starting with "-" might be a revision sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" revision.c | 15 --- sha1_name.c | 5 +++ t/t4214-log-shorthand.sh | 106 +++ 3 files changed, 120 insertions(+), 6 deletions(-) create mode 100755 t/t4214-log-shorthand.sh -- 2.1.4
[PATCH 1/4 v4] revision.c: do not update argv with unknown option
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.siddhart...@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(>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
[PATCH 3/4 v4] revision.c: args starting with "-" might be a revision
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.siddhart...@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
[PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
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) Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- Instead of replacing the whole string, we would expand it accordingly using: if (*name == '-') { if (len == 1) { name = "@{-1}"; len = 5; } else { struct strbuf changed_argument = STRBUF_INIT; strbuf_addstr(_argument, "@{-1}"); strbuf_addstr(_argument, name + 1); strbuf_setlen(_argument, strlen(name) + 4); name = strbuf_detach(_argument, NULL); } } Junio's comments on a previous version of the patch which used this same approach but inside setup_revisions [1] [1]: <xmqqtw882n08@gitster.mtv.corp.google.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 000..659b100 --- /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 && +
[PATCH 2/4 v4] revision.c: swap if/else blocks
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.siddhart...@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(_data, argv + i); break; } - else - got_rev_arg = 1; } if (prune_data.nr) { -- 2.1.4
Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
Hey Junio, > > See the 3-patch series I just sent out. I didn't think it through > very carefully (especially the error message the other caller > produces), but the whole thing _smells_ correct to me. Okay, got it! I will write-up those changes, and make sure nothing bad happens. (Also, the one other function that calls handle_revision_opt, parse_revision_opt needs to be fixed for any changes in handle_revision_opt.) I will do all of this in the next week (Unfortunately, exams!) and submit a new version of this patch (Also, I need to update tests, add documentation, and remove code that does this shorthand stuff for other commands as per Matthieu's comments) -- Best Regards, Siddharth Kannan.
Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote: > Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > > > Um, I am sorry, but I feel that decrementing left, and incrementing it > > again is > > also confusing. > > Yes, but it is no more confusing than your original "left--". > ... > > * If it is an option known to us, handle it and go to the next arg. > > * If it is an option that we do not understand, stuff it in >argv[left++] and go to the next arg. > > Because the second step currently is implemented by calling > handle_opt(), which not just tells if it is an option we understand > or not, but also mucks with argv[left++], you need to undo it once > you start making it possible for a valid "rev" to begin with a dash. > That is what your left-- was, and that is what "decrement and then > increment when it turns out it was an unknown option after all" is. So, our problem here is that the function handle_revision_opt is opaquely also incrementing "left", which we need to decrement somehow. Or: we could change the flow of the code so that this incrementing will happen only when we have decided that the argument is not a revision. > > * If it is an option known to us, handle it and go to the next arg. > > * If it is a rev, handle it, and note that fact in got_rev_arg >(this change of order enables you to allow a rev that begins with >a dash, which would have been misrecognised as a possible unknown >option). > > * If it looks like an option (i.e. "begins with a dash"), then we >already know that it is not something we understand, because the >first step would have caught it already. Stuff it in >argv[left++] and go to the next arg. > > * If it is not a rev and we haven't seen dashdash, verify that it >and everything that follows it are pathnames (which is an inexact >but a cheap way to avoid ambiguity), make all them the prune_data >and conclude. This "changing the order" gave me the idea to change the flow. I tried to implement the above steps without touching the function handle_revision_opt. By inserting the handle_revision_arg call just before calling handle_revision_opt. The decrementing then incrementing or "left--" things have now been removed. (But there is still one thing which doesn't look good) diff --git a/revision.c b/revision.c index b37dbec..8c0acea 100644 --- a/revision.c +++ b/revision.c @@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (seen_dashdash) revarg_opt |= REVARG_CANNOT_BE_FILENAME; read_from_stdin = 0; + for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; + int opts; if (*arg == '-') { - int opts; - opts = handle_revision_pseudo_opt(submodule, revs, argc - i, argv + i, ); @@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s read_revisions_from_stdin(revs, _data); continue; } + } + if (!handle_revision_arg(arg, revs, flags, revarg_opt)) + got_rev_arg = 1; + else if (*arg == '-') { opts = handle_revision_opt(revs, argc - i, argv + i, , argv); if (opts > 0) { i += opts - 1; @@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s } if (opts < 0) exit(128); - continue; - } - - - if (handle_revision_arg(arg, revs, flags, revarg_opt)) { + } else { int j; if (seen_dashdash || *arg == '^') die("bad revision '%s'", arg); @@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s append_prune_data(_data, argv + i); break; } - else - got_rev_arg = 1; } if (prune_data.nr) { The "if (*arg =='-')" line is repeated. On analysing the resulting revision.c:setup_revisions function, I feel that the codepath is still as easily followable as it was earlier, and there is definitely no confusion because of a mysterious decrement. Also, the repeated condition doesn't make it any har
Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
Hey Matthieu, On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote: > Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > > > sha1_name.c | 5 > > t/t4214-log-shorthand.sh | 73 > > > > 2 files changed, 78 insertions(+) > > create mode 100755 t/t4214-log-shorthand.sh > > > > diff --git a/sha1_name.c b/sha1_name.c > > index 73a915f..d774e46 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 (!strcmp(name, "-")) { > > + name = "@{-1}"; > > + len = 5; > > + } > > After you do that, the existing "turn - into @{-1}" pieces of code > become useless and you should remove it (probably in a further patch). Yeah, this is currently also implemented in checkout, apart from the grepped list that you have supplied here. I will find all the instances, and ensure that they work, and remove them. (This will require some more digging into the codepath the commands, to ensure that get_sha1_1 is called somewhere down the line) > > > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh > > ... > > +test_expect_success 'setup' ' > > + echo hello >world && > > + git add world && > > + git commit -m initial && > > + echo "hello second time" >>world && > > ... > > You may use test_commit to save a few lines of code. Oh, yeah! I will use that. I need to work on improving the tests, as well as adding the documentation. > > > +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 && > > Nitpick: we stick the > and the filename (as you did in most places > already). Sorry, slipped my mind! > > It may be worth adding tests for more cases like > > * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~. These do not work right now. The first and last cases here are handled by peel_onion, if I remember correctly. I have to find out why exactly these are not working. Thanks for mentioning this! > > * -..- -> to make sure you handle the presence of two - properly. > > * multiple separate arguments to make sure you handle them all, e.g. > "git log - -", "git log HEAD -", "git log - HEAD". Yeah, will add these tests. > > The last two may be overkill, but the first one is probably important. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- Regards, Siddharth Kannan.
Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
) { > + got_rev_arg = 1; > + } else if (maybe_rev) { > + left++; /* it turns out that it was "unknown opt" */ > + continue; > + } else { > int j; > if (seen_dashdash || *arg == '^') > die("bad revision '%s'", arg); > @@ -2255,8 +2261,6 @@ int setup_revisions(int argc, const char **argv, struct > rev_info *revs, struct s > append_prune_data(_data, argv + i); > break; > } > - else > - got_rev_arg = 1; > } > > if (prune_data.nr) { Thanks Junio, for the time you spent analysing and writing the above version of the patch! Regards, - Siddharth Kannan
[PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
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) Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- sha1_name.c | 5 t/t4214-log-shorthand.sh | 73 2 files changed, 78 insertions(+) create mode 100755 t/t4214-log-shorthand.sh diff --git a/sha1_name.c b/sha1_name.c index 73a915f..d774e46 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 (!strcmp(name, "-")) { + 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 000..dec966c --- /dev/null +++ b/t/t4214-log-shorthand.sh @@ -0,0 +1,73 @@ +#!/bin/sh + +test_description='log can show previous branch using shorthand - for @{-1}' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo hello >world && + git add world && + git commit -m initial && + echo "hello second time" >>world && + git add world && + git commit -m second && + echo "hello other file" >>planet && + git add planet && + git commit -m third && + echo "hello yet another file" >>city && + git add city && + git commit -m fourth +' + +test_expect_success '"log -" should not work initially' ' + test_must_fail git log - +' + +test_expect_success '"log -" should work' ' + git checkout -b testing-1 master^ && + git checkout -b testing-2 master~2 && + git checkout master && + + 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_done -- 2.1.4
[PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
setup_revisions used to consider any argument starting with "-" to be either a valid option or nothing at all. This patch will teach it to check if the argument is a revision before declaring that it is nothing at all. Before this patch, handle_revision_arg was not called for arguments starting with "-" and once for arguments that didn't start with "-". Now, it will be called once per argument. This patch prepares the addition of "-" as a shorthand for "previous branch". Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- revision.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index b37dbec..4131ad5 100644 --- a/revision.c +++ b/revision.c @@ -2205,6 +2205,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 handle_rev_arg_called = 0, args; if (*arg == '-') { int opts; @@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s } if (opts < 0) exit(128); - continue; + + args = handle_revision_arg(arg, revs, flags, revarg_opt); + handle_rev_arg_called = 1; + if (args) + continue; + else + --left; } - if (handle_revision_arg(arg, revs, flags, revarg_opt)) { + if ((handle_rev_arg_called && args) || + handle_revision_arg(arg, revs, flags, revarg_opt)) { int j; if (seen_dashdash || *arg == '^') die("bad revision '%s'", arg); -- 2.1.4
[PATCH 0/2 v3] WIP: allow "-" as a shorthand for "previous branch"
Thanks a lot, Matthieu, for your comments on an earlier version of this patch! [1] After the discussion there, I have considered the changes that have been made and I broke them into two separate commits. I have included the list of commands that are touched by this patch series in the second commit message. (idea from the v1 discussion [2]) Reproduced here: * 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. I have added the WIP tag because I am still unsure if the tests that I have added (for git-log) are sufficient for this patch or more comprehensive tests need to be added. So, please help me with some feedback on that. I have removed the "log:" tag from the subject line because this patch now affects commands other than log. I have run the test suite locally and on Travis CI! [3] [1]: https://public-inbox.org/git/vpqh944eof7@anie.imag.fr/#t [2]: https://public-inbox.org/git/can-3qhozn_wyvqbvdu_c1h4vuoat5fobfl7k+femnpqkxjw...@mail.gmail.com/ [3]: https://travis-ci.org/icyflame/git/builds/200431159 Siddharth Kannan (2): revision.c: args starting with "-" might be a revision sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}" revision.c | 12 ++-- sha1_name.c | 5 t/t4214-log-shorthand.sh | 73 3 files changed, 88 insertions(+), 2 deletions(-) create mode 100755 t/t4214-log-shorthand.sh -- 2.1.4
Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
On 9 February 2017 at 17:55, Matthieu Moywrote: > >> [...] >> As you might notice, in this list, most commands are not of the `rm` variety, >> i.e. something that would delete stuff. > > OK, I think I'm convinced. I am glad! :) > > Keep the arguments in mind when polishing the commit message. I will definitely do that. I am working on a good commit message for this by looking at some past changes to sha1_name.c which have affected multiple commands. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- Best Regards, - Siddharth.
Re: GSoC 2017: application open, deadline = February 9, 2017
On 9 February 2017 at 15:45, Matthieu Moywrote: > > A non-quoted but yet important part of my initial email was: > > | So, as much as possible, I'd like to avoid being an org admin this > | year. It's not a lot of work (much, much less than being a mentor!), > | but if I manage to get some time to work for Git, I'd rather do that > | on coding and reviewing this year. > > and for now, no one stepped in to admin. I would like to point everyone to this reply from Jeff King on the original post: [1] (In case this was lost in the midst of other emails) It sounds like Jeff King is okay with taking up the "admin" role. I do not mind doing the administrative stuff. But the real work is in polishing up the ideas list and microprojects page. So I think the first step, if people are interested in GSoC, is not just to say "yes, let's do it", but to actually flesh out these pages: > >> Someone steps in to do what exactly? > > First we need an admin. Then as you said a bit of janitoring work on > the web pages. [1]: https://public-inbox.org/git/20170125204504.ebw2sa4uokfww...@sigill.intra.peff.net/ -- Best Regards, - Siddharth.
Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Hello Matthieu, On 8 February 2017 at 20:10, Matthieu Moywrote: > In a previous discussion, I made an analogy with "cd -" (which is the > source of inspiration of this shorthand AFAIK): "-" did not magically > become "the last visited directory" for all Unix commands, just for > "cd". And in this case, I'm happy with it. For example, I never need > "mkdir -", and I'm happy I can't "rm -fr -" by mistake. > > So, it's debatable whether it's a good thing to have all commands > support "-". For example, forcing users to explicitly type "git branch > -d @{1}" and not providing them with a shortcut might be a good thing. builtin/branch.c does not call setup_revisions and remains unaffected by this patch :) In my original patch post, I was not explicit about what files call setup_revisions. I would like to rectify that with this (grep-ed) list: * 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 only show information, and don't change anything. As you might notice, in this list, most commands are not of the `rm` variety, i.e. something that would delete stuff. In the next version of this patch, I will definitely include the list of commands which are "rm-ish" and affected by this patch. > > I don't have strong opinion on this: I tend to favor consistency and > supporting "-" everywhere goes in this direction, but I think the > downsides should be considered too. A large part of the exercice here is > to write a good commit message! Yes, I prefer consistency very much as well! Having "-" mean the same thing across a lot of commands is better than having that shorthand only in a few commands, as it is now. Unless there is a specific confusion that might arise because of this shorthand inclusion, I think that this shorthand should be supported across the board. (I especially like typing `git checkout - ` which is very handy!) > > Another issue with this is: - is also a common way to say "use stdin > instead of a file", so before enabling - for "previous branch", we need > to make sure it does not introduce any ambiguity. Git does not seem to > use "- for stdin" much (most commands able to read from stdin have an > explicit --stdin option for that), a quick grep in the docs shows only > "git blame --contents -" which is OK because a revision wouldn't make > sense here anyway. Yes, just to jog your memory, this was discussed here [1] Junio said: As long as the addition is carefully prepared so that we know it will not conflict (or be confused by users) with possible other uses of "-", I do not think we would mind "git branch -D -" and other commands to learn "-" as a synonym for @{-1}. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ Thanks a lot for the review on this patch, Matthieu! -- Best Regards, - Siddharth. [1]: https://public-inbox.org/git/7vmwpitb6k@alter.siamese.dyndns.org/
[PATCH/RFC v2] WIP: log: allow "-" as a short-hand for "previous branch"
Teach revision.c:setup_revisions that an argument starting with "-" can be an argument also. `left` variable needs to be incremented only when the supplied arg is neither an argument, nor an option. Teach sha1_name.c:get_sha1_1 that "-" is equivalent to "@{-1}" Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- I have run the test suite locally and on Travis CI. [1] Whenever the argument begins with a "-" then the function "handle_revision_arg" is called twice. I can fix this using a variable that would store whether the function has been called earlier or not. For doing that I have to investigate some more on what the valid return values for "handle_revision_arg" are. (Or a simpler approach would be to use an integer flag, this would also not be affected if in case "handle_revision_arg" is changed in the future) I have also written a very basic test for git-log only. I have based this on the tests that were added in 696acf4 (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17). It tests revisions, revision ranges, and open-ended revision ranges (where the start or the finish argument is inferred) If the code in this patch is okay, or close to okay, then please tell me if further tests need to be added for git-log and/or other commands. This change touches a few commands, other than log. notably, git-format-patch, git-whatchanged and git-show, all of which are defined inside builtin/log.c. In general, it affects commands that call setup_revisions at some point in their codepath. (eg: git-diff-index) Thanks a lot, Junio, for your comments on the previous version of this patch and further discussion on that thread! [1]: https://travis-ci.org/icyflame/git/builds/199350136 revision.c | 9 +-- sha1_name.c | 5 t/t4214-log-shorthand.sh | 62 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100755 t/t4214-log-shorthand.sh diff --git a/revision.c b/revision.c index b37dbec..e14f62c 100644 --- a/revision.c +++ b/revision.c @@ -2206,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; if (*arg == '-') { - int opts; + int opts, args; opts = handle_revision_pseudo_opt(submodule, revs, argc - i, argv + i, @@ -2234,7 +2234,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s } if (opts < 0) exit(128); - continue; + + args = handle_revision_arg(arg, revs, flags, revarg_opt); + if (args) + continue; + else + --left; } diff --git a/sha1_name.c b/sha1_name.c index 73a915f..d774e46 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 (!strcmp(name, "-")) { + 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 000..95cf2d4 --- /dev/null +++ b/t/t4214-log-shorthand.sh @@ -0,0 +1,62 @@ +#!/bin/sh + +test_description='log can show previous branch using shorthand - for @{-1}' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo hello >world && + git add world && + git commit -m initial && + echo "hello second time" >>world && + git add world && + git commit -m second && + echo "hello other file" >>planet && + git add planet && + git commit -m third && + echo "hello yet another file" >>city && + git add city && + git commit -m fourth +' + +test_expect_success '"log -" should not work initially' ' + test_must_fail git log - +' + +test_expect_success '"log -" should work' ' + git checkout -b testing-1 master^ && + git checkout -b testing-2 master~2 && + git checkout master && + + git log testing-2 >expect && + git log - >actual && + test_cmp expect actual +' + +test_expect_success 'revision range should work when one end is left empty' ' + git checkout testi
Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
On Mon, Feb 06, 2017 at 03:09:47PM -0800, Junio C Hamano wrote: > The focus of GSoC being mentoring those who are new to the open > source development, and hopefully retain them in the community after > GSoC is over, we do expect microprojects to be suitable for those > who are new to the codebase. Okay, understood! Since I have spent time here anyway, I guess I will continue on this instead of going over to a new micro project. > > > (c) -> Else look for "r1^-" > > ... > > Case (c) is a bit confusing. This could be something like "-^-", and > > something like "^-" could mean "Not commits on previous branch" or it > > could mean "All commits on this branch except for the parent of HEAD" > > Do you mean: > > "git rev-parse ^-" does not mean "git rev-parse HEAD^-", but we > probably would want to, and if that is what is going to happen, > "^-" should mean "HEAD^-", and cannot be used for "^@{-1}"? > > It's friend "^!" does not mean "HEAD^!", and "^@" does not mean > "HEAD^@", either (the latter is somewhat borked, though, and "^@" > translates to "^HEAD" because confusingly "@" stands for "HEAD" > sometimes). Yes, I meant that whether we should use ^- as ^@{-1} or HEAD^-. Oh! So, that's why running `git log ^@` leads to an empty set! > > So my gut feeling is that it is probably OK to make "^-" mean > "^@{-1}"; it may be prudent to at least initially keep "^-" an error > like it currently is already, though. I agree with your gut feeling, and would like to _not_ exclude only this case. This way, across the code and implementation, there wouldn't be any particular cases which would have to be excluded. > > So, this patch reduces to the following 2 tasks: > > > > 1. Teach setup_revisions that something starting with "-" can be > > an > > argument as well > > 2. Teach get_sha1_basic that "-" means the tip of the previous > > branch > > perhaps by replacing it with "@{-1}" just before the reflog > > parsing is > > done Making a change in sha1_name.c will touch a lot of commands (setup_revisions is called from everywhere in the codebase), so, I am still trying to figure out how to do this such that the rest of the codepath remains unchanged. I hope that you do not mind this side-effect, but rather, you intended for this to happen, right? More commands will start supporting this shorthand, suddenly. (such as format-patch, whatchanged, diff to name a very few). Best Regards, Siddharth.
Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Hey Junio, I did some more digging into the codepath: On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote: > > A correct solution needs to know if the argument is at the position > where a revision (or revision range) is expected and then give the > tip of the previous branch when it sees "-" (and other combinations > this patch tries to cover). In other words, the parser always knows > what it is parsing, and if and only if it is parsing a rev, react to > "-" and think "ah, the user wants me to use the tip of the previous > branch". > > But the code that knows that it expects to see a revision already > exists, and it is the real parser. In the above snippet, > setup_revisions() is the one that does the real parsing of argv[]. > The code there knows when it wants to see a rev, and takes argv[i] > and turns into an object to call add_pending_object(). That codepath > may not yet know that "-" means the tip of the previous branch, and > that is where the change needs to go. Inside setup_revisions, it tries to parse arguments and options. In there, is this line of code: if (*arg == '-') { Once control enters this branch, it will either parse the argument as an option / pseudo-option, or simply leave this argument as is in the argv[] array and move forward with the other arguments. So, first I need to teach setup_revisions that something starting with a "-" might be a revision or a revision range. After this, going further down the codepath, in revision.c:handle_revision_arg: The argument is parsed to find out if it is of the form rev1...rev2 or rev1..rev2 or just rev1, etc. (a) -> If `..` or `...` was found, then two pointers "this" and "next" now hold the from and to revisions, and the function get_sha1_committish is called on them. In case both were found to be committish, then the char pointers now hold the sha1 in them, they are parsed into objects. (b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To get r1, again the function get_sha1_committish is called with only r1 as the parameter. (c) -> Else look for "r1^-" (d) -> Else look for the argument using the same get_sha1_committish function (It any "^" was present in it, it has already been noted and removed) Cases (a), (b) and (d) can be handled by putting this inside get_sha1_committish. (Further discussion about that below) Case (c) is a bit confusing. This could be something like "-^-", and something like "^-" could mean "Not commits on previous branch" or it could mean "All commits on this branch except for the parent of HEAD" Please tell me if this is confusing or undesired altogether. Personally, I feel that people who have been using "^-" would be very confused if it's behaviour changed. So, all the code till now points at adding the patch for "-" = "@{-1}" inside get_sha1_committish or downstream from there. get_sha1_committish -> get_sha1_with_context -> get_sha1_with_context_1 -> get_sha1_1 -> peel_onion -> calling get_sha1_basic again with the ref only (after peeling) -> get_sha1_basic -> includes parsing of "@{-N}" type revs. So, this indicates that if we can convert the "-" appropriately before this point, then it would be good. -> get_short_sha1 So, this patch reduces to the following 2 tasks: 1. Teach setup_revisions that something starting with "-" can be an argument as well 2. Teach get_sha1_basic that "-" means the tip of the previous branch perhaps by replacing it with "@{-1}" just before the reflog parsing is done > A correct solution will be a lot more involved, of course, and I > think it will be larger than a reasonable microproject for people > new to the codebase. So true :) I had spent a fair bit of time already on my previous patch, and I thought I might as well complete my research into this, and send this write-up to the mailing list, so that I could write a patch some time later. In case you would prefer for me to not work on this anymore because I am new to the codebase, I will leave it at this. - Siddharth Kannan
Re: [PATCH v3] parse-remote: remove reference to unused op_prep
Hey Pranit, On Sun, Feb 05, 2017 at 02:45:46AM +0530, Pranit Bauva wrote: > Hey Siddharth, > > On Sat, Feb 4, 2017 at 8:01 PM, Siddharth Kannan > <kannan.siddhart...@gmail.com> wrote: > > The error_on_missing_default_upstream helper function learned to > > take op_prep argument with 15a147e618 ("rebase: use @{upstream} > > if no upstream specified", 2011-02-09), but as of 045fac5845 > > ("i18n: git-parse-remote.sh: mark strings for translation", > > 2016-04-19), the argument is no longer used. Remove it. > > > > Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> > > This looks good to me! Thanks :) > > Regards, > Pranit Bauva Should I send this patch with "To:" set to Junio and "Cc:" set to the mailing list, as mentioend in the SubmittingPatches document? - Siddharth Kannan
Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Hey Junio, On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote: > Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > > > @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char > > **argv, const char *prefix, > > > > if (quiet) > > rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT; > > + > > + /* > > +* Check if any argument has a "-" in it, which has been referred to as > > a > > +* shorthand for @{-1}. Handles methods that might be used to list > > commits > > +* as mentioned in git rev-list --help > > +*/ > > + > > + for(i = 0; i < argc; ++i) { > > + if (!strcmp(argv[i], "-")) { > > + argv[i] = "@{-1}"; > > + } else if (!strcmp(argv[i], "^-")) { > > + argv[i] = "^@{-1}"; > > + } else if (strlen(argv[i]) >= 4) { > > + > > + ... > > + } > > + } > > + > > argc = setup_revisions(argc, argv, rev, opt); > > "Turn '-' to '@{-1}' before we do the real parsing" can never be a > reasonable strategy to implement the desired "'-' means the tip of > the previous branch" correctly. To understand why, you only need to > imagine what happens to this command: > > $ git log --grep '^-' > > Turning it into "git log --grep '^@{-1}'" obviously is not what the > end-users want, so that is an immediate bug in the version of Git > with this patch applied. > > Even if this were not a patch for the "log" command but for some > other command, a change with the above approach is very much > unwelcome, even if that other command does not currently have any > option that takes arbitrary string the user may want to specify > (like "find commit with a line that matches this pattern" option > called "--grep" the "log" command has). That is because it will > make it impossible to enhance the command by adding such an option > in the future. So it is also adding the problems to future > developers (and users) of Git. Understood! > > A correct solution needs to know if the argument is at the position > where a revision (or revision range) is expected and then give the > tip of the previous branch when it sees "-" (and other combinations > this patch tries to cover). In other words, the parser always knows > what it is parsing, and if and only if it is parsing a rev, react to > "-" and think "ah, the user wants me to use the tip of the previous > branch". Ah, okay. I will do another one of the suggestions as my micro project but continue to look into this part of the code and try to find the right place to write the code to implement the present patch. > > But the code that knows that it expects to see a revision already > exists, and it is the real parser. In the above snippet, > setup_revisions() is the one that does the real parsing of argv[]. > The code there knows when it wants to see a rev, and takes argv[i] > and turns into an object to call add_pending_object(). That codepath > may not yet know that "-" means the tip of the previous branch, and > that is where the change needs to go. > > Such a properly-done update does not need to textually replace "-" > with "@{-1}" in argv[]; the codepath is where it understands what > any textual representation of a rev the user gave it means, and it > understands "@{-1}" there. It would be the matter of updating it to > also understand what "-" means. > > A correct solution will be a lot more involved, of course, and I > think it will be larger than a reasonable microproject for people > new to the codebase. > > I didn't check the microproject ideas page myself; whether it says > that turning "-" unconditionally to "@{-1}" is a good idea, or it > hints that supporting "-" as "the tip of the previous branch" in > more commands is a reasonable byte-sized microproject, I think it is > misleading and misguided. Can somebody remove that entry so that we > won't waste time of new developers (which would lead to discouraging > them)? Thanks. Thanks a lot for writing this detailed reply! I will definitely take into account all of the points mentioned here in the future patches I send. - Siddharth Kannan
[PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Search and replace "-" (written in the context of a branch name) in the argument list with "@{-1}". As per the help text of git rev-list, this includes the following four cases: a. "-" b. "^-" c. "-..other-branch-name" or "other-branch-name..-" d. "-...other-branch-name" or "other-branch-name...-" (a) and (b) have been implemented as in the previous implementations of this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a short-hand for "previous branch", 2011-04-07) (c) and (d) have been implemented by using the strbuf API, growing it to the right size and placing "@{-1}" instead of "-" Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- This is a patch for one of the microprojects of SoC 2017. [1] I have implemented this using multiple methods, that I have re-written again and again for better versions ([2]). The present version I feel is the closest that I could get to the existing code in the repository. This patch only uses functions that are commonly used in the rest of the codebase. I still have to write tests, as well as update documentation as done in 696acf45 (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17). I request your comments on this patch. Also, I have the following questions regarding this patch: 1. Is the approach that I have used to solve this problem fine? 2. Is the code I am writing in the right function? (I have put it right before the revisions data structure is setup, thus these changes affect only git-log) [1]: https://git.github.io/SoC-2017-Microprojects/ [2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses strbufs for the starting 4 characters, and last 4 characters and compares those to the appropriate strings for case (c) and case (d). I edited this patch to use strstr instead, which avoids all the strbuf declarations) builtin/log.c | 47 ++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/builtin/log.c b/builtin/log.c index 55d20cc..a5aac99 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, struct rev_info *rev, struct setup_revision_opt *opt) { struct userformat_want w; - int quiet = 0, source = 0, mailmap = 0; + int quiet = 0, source = 0, mailmap = 0, i = 0; static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP}; const struct option builtin_log_options[] = { @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, if (quiet) rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT; + + /* +* Check if any argument has a "-" in it, which has been referred to as a +* shorthand for @{-1}. Handles methods that might be used to list commits +* as mentioned in git rev-list --help +*/ + + for(i = 0; i < argc; ++i) { + if (!strcmp(argv[i], "-")) { + argv[i] = "@{-1}"; + } else if (!strcmp(argv[i], "^-")) { + argv[i] = "^@{-1}"; + } else if (strlen(argv[i]) >= 4) { + + if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) { + struct strbuf changed_argument = STRBUF_INIT; + + strbuf_addstr(_argument, "@{-1}"); + strbuf_addstr(_argument, argv[i] + 1); + + strbuf_setlen(_argument, strlen(argv[i]) + 4); + + argv[i] = strbuf_detach(_argument, NULL); + } + + /* +* Find the first occurence, and add the size to it and proceed if +* the resulting value is NULL +*/ + if (!(strstr(argv[i], "...-") + 4) || + !(strstr(argv[i], "..-") + 3)) { + struct strbuf changed_argument = STRBUF_INIT; + + strbuf_addstr(_argument, argv[i]); + + strbuf_grow(_argument, strlen(argv[i]) + 4); + strbuf_setlen(_argument, strlen(argv[i]) + 4); + + strbuf_splice(_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5); + + argv[i] = strbuf_detach(_argument, NULL); + } + } + } + argc = setup_revisions(argc, argv, rev, opt); /* Any arguments at this point are not recognized */ -- 2.1.4
[PATCH v3] parse-remote: remove reference to unused op_prep
The error_on_missing_default_upstream helper function learned to take op_prep argument with 15a147e618 ("rebase: use @{upstream} if no upstream specified", 2011-02-09), but as of 045fac5845 ("i18n: git-parse-remote.sh: mark strings for translation", 2016-04-19), the argument is no longer used. Remove it. Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- Thanks a lot for the review, Pranit and Junio! I have made the appropriate changes, and the edit to the file inside contrib/examples/ has been removed from this patch. git-parse-remote.sh | 3 +-- git-rebase.sh | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/git-parse-remote.sh b/git-parse-remote.sh index d3c3998..9698a05 100644 --- a/git-parse-remote.sh +++ b/git-parse-remote.sh @@ -56,8 +56,7 @@ get_remote_merge_branch () { error_on_missing_default_upstream () { cmd="$1" op_type="$2" - op_prep="$3" # FIXME: op_prep is no longer used - example="$4" + example="$3" branch_name=$(git symbolic-ref -q HEAD) display_branch_name="${branch_name#refs/heads/}" # If there's only one remote, use that in the suggestion diff --git a/git-rebase.sh b/git-rebase.sh index 04f6e44..b89f960 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -448,7 +448,7 @@ then then . git-parse-remote error_on_missing_default_upstream "rebase" "rebase" \ - "against" "git rebase $(gettext '')" + "git rebase $(gettext '')" fi test "$fork_point" = auto && fork_point=t -- 2.1.4
[PATCH v2] parse-remote: Remove reference to unused op_prep
The error_on_missing_default_upstream helper function learned to take op_prep argument with 15a147e618 ("rebase: use @{upstream} if no upstream specified", 2011-02-09), but as of 045fac5845 ("i18n: git-parse-remote.sh: mark strings for translation", 2016-04-19), the argument is no longer used. Remove it. Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- Thanks a lot, Pranit and Junio for your reviews on the first version of this patch. I have changed the messages accordingly. contrib/examples/git-pull.sh | 2 +- git-parse-remote.sh | 3 +-- git-rebase.sh| 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh index 6b3a03f..1d51dc3 100755 --- a/contrib/examples/git-pull.sh +++ b/contrib/examples/git-pull.sh @@ -267,7 +267,7 @@ error_on_no_merge_candidates () { echo "for your current branch, you must specify a branch on the command line." elif [ -z "$curr_branch" -o -z "$upstream" ]; then . git-parse-remote - error_on_missing_default_upstream "pull" $op_type $op_prep \ + error_on_missing_default_upstream "pull" $op_type \ "git pull " else echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'" diff --git a/git-parse-remote.sh b/git-parse-remote.sh index d3c3998..9698a05 100644 --- a/git-parse-remote.sh +++ b/git-parse-remote.sh @@ -56,8 +56,7 @@ get_remote_merge_branch () { error_on_missing_default_upstream () { cmd="$1" op_type="$2" - op_prep="$3" # FIXME: op_prep is no longer used - example="$4" + example="$3" branch_name=$(git symbolic-ref -q HEAD) display_branch_name="${branch_name#refs/heads/}" # If there's only one remote, use that in the suggestion diff --git a/git-rebase.sh b/git-rebase.sh index 04f6e44..b89f960 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -448,7 +448,7 @@ then then . git-parse-remote error_on_missing_default_upstream "rebase" "rebase" \ - "against" "git rebase $(gettext '')" + "git rebase $(gettext '')" fi test "$fork_point" = auto && fork_point=t -- 2.1.4
[PATCH] git-parse-remote.sh: Remove op_prep argument
- Remove the third argument of error_on_missing_default_upstream that is no longer required - FIXME to remove this argument was added in commit 045fac5845 - Run "grep" on the rest of the codebase to find and remove occurences of the third argument and fix the function calls appropriately Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com> --- The contrib/examples/git-pull.sh file also has a variable op_prep which is used in one of the messages shown the user. Should I remove this variable as well? contrib/examples/git-pull.sh | 2 +- git-parse-remote.sh | 3 +-- git-rebase.sh| 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh index 6b3a03f..1d51dc3 100755 --- a/contrib/examples/git-pull.sh +++ b/contrib/examples/git-pull.sh @@ -267,7 +267,7 @@ error_on_no_merge_candidates () { echo "for your current branch, you must specify a branch on the command line." elif [ -z "$curr_branch" -o -z "$upstream" ]; then . git-parse-remote - error_on_missing_default_upstream "pull" $op_type $op_prep \ + error_on_missing_default_upstream "pull" $op_type \ "git pull " else echo "Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'" diff --git a/git-parse-remote.sh b/git-parse-remote.sh index d3c3998..9698a05 100644 --- a/git-parse-remote.sh +++ b/git-parse-remote.sh @@ -56,8 +56,7 @@ get_remote_merge_branch () { error_on_missing_default_upstream () { cmd="$1" op_type="$2" - op_prep="$3" # FIXME: op_prep is no longer used - example="$4" + example="$3" branch_name=$(git symbolic-ref -q HEAD) display_branch_name="${branch_name#refs/heads/}" # If there's only one remote, use that in the suggestion diff --git a/git-rebase.sh b/git-rebase.sh index 04f6e44..b89f960 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -448,7 +448,7 @@ then then . git-parse-remote error_on_missing_default_upstream "rebase" "rebase" \ - "against" "git rebase $(gettext '')" + "git rebase $(gettext '')" fi test "$fork_point" = auto && fork_point=t -- 2.1.4