Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
- From
Siddharth Kannan <kannan.siddharth12@gmail.com>
- Date
- Feb 12, 2017, 10:42 UTC
- Message-ID
- <20170212104220.GA20317@ubuntu-512mb-blr1-01.localdomain>
- In-Reply-To
- <vpqbmu768on.fsf@anie.imag.fr>
Hey Matthieu, On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:
Show 22 quoted lines
> Siddharth Kannan <kannan.siddharth12@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)
Show 11 quoted lines
> > > 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.
Show 11 quoted lines
>
> > +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!
Show 5 quoted lines
> > * -..- -> 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.
Show 6 quoted lines
> > The last two may be overkill, but the first one is probably important. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/
-- Regards,
Siddharth Kannan.