* [PATCH v3 1/2] reset: enable '-' short-hand for previous branch @ 2015-03-10 10:52 Sudhanshu Shekhar 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 10:52 UTC (permalink / raw) To: git; +Cc: Matthieu.Moy, gitster, davvid, sunshine, Sudhanshu Shekhar 'git reset -' will reset to the previous branch. It will behave similar to @{-1} except when a file named '@{-1}' is present. To refer to a file named '-', use ./- or the -- flag. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> --- Thank you all for your feedback. I have made changes and I hope this patch meets community standards. Please let me know if any further changes are required. Regards, Sudhanshu builtin/reset.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4c08ddc..8abd300 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec, { const char *rev = "HEAD"; unsigned char unused[20]; + int substituted_minus = 0; /* * Possible arguments are: * @@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec, */ if (argv[0]) { + if (!strcmp(argv[0], "-")) { + argv[0] = "@{-1}"; + substituted_minus = 1; + } if (!strcmp(argv[0], "--")) { argv++; /* reset to HEAD, possibly with paths */ } else if (argv[1] && !strcmp(argv[1], "--")) { @@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec, * Ok, argv[0] looks like a commit/tree; it should not * be a filename. */ + if (substituted_minus) + argv[0] = "-"; verify_non_filename(prefix, argv[0]); + if (substituted_minus) + argv[0] = "@{-1}"; rev = *argv++; } else { + /* We were treating "-" as a commit and not a file */ + if (substituted_minus) + argv[0] = "-"; /* Otherwise we treat this as a filename */ verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; - if (read_cache() < 0) die(_("index file corrupt")); -- 2.3.1.279.gd534259 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] Added tests for reset - 2015-03-10 10:52 [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar @ 2015-03-10 10:52 ` Sudhanshu Shekhar 2015-03-10 13:26 ` Matthieu Moy 2015-03-10 18:29 ` Eric Sunshine 2015-03-10 13:17 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Matthieu Moy 2015-03-10 19:18 ` Eric Sunshine 2 siblings, 2 replies; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 10:52 UTC (permalink / raw) To: git; +Cc: Matthieu.Moy, gitster, davvid, sunshine, Sudhanshu Shekhar Added the following test cases: 1) Confirm error message when git reset is used with no previous branch 2) Confirm git reset - works like git reset @{-1} 3) Confirm "-" is always treated as a commit unless the -- file option is specified 4) Confirm "git reset -" works normally even when a file named @{-1} is present Helped-by: David Aguilar <davvid@gmail.com> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> --- I have tried to keep each test self sufficient. Please let me know if any changes are required. Thank you! t/t7102-reset.sh | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 98bcfe2..0faf241 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -568,4 +568,143 @@ test_expect_success 'reset --mixed sets up work tree' ' test_cmp expect actual ' +test_expect_success 'reset - with no previous branch' ' + git init no_previous && + ( + cd no_previous && + test_must_fail git reset - 2>output + ) && + test_i18ngrep "bad flag" no_previous/output +' + +test_expect_success 'reset - while having file named - and no previous branch' ' + git init no_previous && + ( + cd no_previous && + >./- && + test_must_fail git reset - 2>output + ) && + test_i18ngrep "bad flag" no_previous/output +' + + +test_expect_success 'reset - in the presence of file named - with previous branch' ' + echo "Unstaged changes after reset:" >expect && + echo "M -" >>expect && + echo "M 1" >>expect && + git init no_previous && + ( + cd no_previous && + >./- && + >1 && + git add 1 - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >./- && + echo "wow" >1 && + git add 1 - && + git reset - >../output + ) && + rm -rf no_previous && + test_cmp output expect +' +test_expect_success 'reset - in the presence of file named - with -- option' ' + echo "Unstaged changes after reset:" >expect && + echo "M -" >>expect && + echo "M 1" >>expect && + git init no_previous && + ( + cd no_previous && + >./- && + >1 && + git add 1 - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >./- && + echo "wow" >1 && + git add 1 - && + git reset - -- >../output + ) && + rm -rf no_previous && + test_cmp output expect +' + +test_expect_success 'reset - in the presence of file named - with -- file option' ' + echo "Unstaged changes after reset:" >expect && + echo "M -" >>expect && + git init no_previous && + ( + cd no_previous && + >./- && + >1 && + git add 1 - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >./- && + echo "wow" >1 && + git add 1 - && + git reset -- - >../output + ) && + rm -rf no_previous + test_cmp output expect +' +test_expect_success 'reset - in the presence of file named - with both pre and post -- option' ' + echo "Unstaged changes after reset:" >expect && + echo "M -" >>expect && + git init no_previous && + ( + cd no_previous && + >./- && + >1 && + git add 1 - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >./- && + echo "wow" >1 && + git add 1 - && + git reset - -- - >../output + ) && + rm -rf no_previous + test_cmp output expect +' + +test_expect_success 'reset - works same as reset @{-1}' ' + git init no_previous && + ( + cd no_previous && + echo "random" >random && + git add random && + git commit -m "base commit" && + git checkout -b temp && + echo new-file >new-file && + git add new-file && + git commit -m "added new-file" && + git reset - && + git status --porcelain >../first && + git add new-file && + git commit -m "added new-file" && + git reset @{-1} && + git status --porcelain >../second + ) && + test_cmp first second +' + +test_expect_success 'reset - with file named @{-1}' ' + echo "Unstaged changes after reset:" >expect && + echo "M @{-1}" >>expect && + git init no_previous && + ( + cd no_previous && + echo "random" >./@{-1} && + git add ./@{-1} && + git commit -m "base commit" && + git checkout -b new_branch && + echo "additional stuff" >>./@{-1} && + git add ./@{-1} && + git reset - >../output + ) && + rm -rf no_previous && + test_cmp output expect +' + test_done -- 2.3.1.279.gd534259 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] Added tests for reset - 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar @ 2015-03-10 13:26 ` Matthieu Moy 2015-03-10 17:52 ` Sudhanshu Shekhar 2015-03-10 18:29 ` Eric Sunshine 1 sibling, 1 reply; 13+ messages in thread From: Matthieu Moy @ 2015-03-10 13:26 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: git, gitster, davvid, sunshine Sudhanshu Shekhar <sudshekhar02@gmail.com> writes: > +test_expect_success 'reset - while having file named - and no previous branch' ' I like having the expected behavior in the test name too. e.g. add "fails" at the end of the sentence. > +test_expect_success 'reset - in the presence of file named - with previous branch' ' > + echo "Unstaged changes after reset:" >expect && > + echo "M -" >>expect && > + echo "M 1" >>expect && Here and elsewhere: why not cat >expect <<-EOF Unstaged changes after reset: M - M 1 ? > + rm -rf no_previous && That would be best done in a test_when_finished, so that the directory is removed regardless of whether the test failed before this line or not. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] Added tests for reset - 2015-03-10 13:26 ` Matthieu Moy @ 2015-03-10 17:52 ` Sudhanshu Shekhar 2015-03-10 18:05 ` Eric Sunshine 0 siblings, 1 reply; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 17:52 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, Junio C Hamano, David Aguilar, Eric Sunshine Hi, On Tue, Mar 10, 2015 at 6:56 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > Sudhanshu Shekhar <sudshekhar02@gmail.com> writes: > >> +test_expect_success 'reset - while having file named - and no previous branch' ' > > I like having the expected behavior in the test name too. e.g. add > "fails" at the end of the sentence. > Sure. I will update this in the next patch. >> +test_expect_success 'reset - in the presence of file named - with previous branch' ' >> + echo "Unstaged changes after reset:" >expect && >> + echo "M -" >>expect && >> + echo "M 1" >>expect && > > Here and elsewhere: why not > > cat >expect <<-EOF > Unstaged changes after reset: > M - > M 1 > > ? I was confused whether to use cat or echo. I thought using cat will disrupt the indentation as the EOF needs to be on a single line. This is why I chose echo. Please let me know your thoughts on this. >> + rm -rf no_previous && > > That would be best done in a test_when_finished, so that the directory > is removed regardless of whether the test failed before this line or > not. Thanks for pointing this out. I will update this accordingly. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ Regards, Sudhanshu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] Added tests for reset - 2015-03-10 17:52 ` Sudhanshu Shekhar @ 2015-03-10 18:05 ` Eric Sunshine 0 siblings, 0 replies; 13+ messages in thread From: Eric Sunshine @ 2015-03-10 18:05 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: Matthieu Moy, Git List, Junio C Hamano, David Aguilar On Tue, Mar 10, 2015 at 1:52 PM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote: > On Tue, Mar 10, 2015 at 6:56 PM, Matthieu Moy > <Matthieu.Moy@grenoble-inp.fr> wrote: >> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes: >>> +test_expect_success 'reset - in the presence of file named - with previous branch' ' >>> + echo "Unstaged changes after reset:" >expect && >>> + echo "M -" >>expect && >>> + echo "M 1" >>expect && >> >> Here and elsewhere: why not >> >> cat >expect <<-EOF >> Unstaged changes after reset: >> M - >> M 1 >> >> ? > I was confused whether to use cat or echo. I thought using cat will > disrupt the indentation as the EOF needs to be on a single line. This > is why I chose echo. Please let me know your thoughts on this. Here-docs are easier to compose and read than individual 'echo' statements for multi-line content. The '-' in front of EOF allows you to indent the entire body. Even better, use -\EOF to signify that you don't expect any interpolation to occur within the body. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] Added tests for reset - 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar 2015-03-10 13:26 ` Matthieu Moy @ 2015-03-10 18:29 ` Eric Sunshine 2015-03-10 22:03 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar 2015-03-10 22:10 ` [PATCH v3 2/2] t7102: add 'reset -' tests Sudhanshu Shekhar 1 sibling, 2 replies; 13+ messages in thread From: Eric Sunshine @ 2015-03-10 18:29 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: Git List, Matthieu Moy, Junio C Hamano, David Aguilar On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote: > Added tests for reset - Mention the area of the project you are changing, followed by a colon, followed by a short summary of the change. Drop capitalization. Write in imperative mood. t7102: add 'reset -' tests > Added the following test cases: Imperative mood: "Add test cases:" > 1) Confirm error message when git reset is used with no previous branch > 2) Confirm git reset - works like git reset @{-1} > 3) Confirm "-" is always treated as a commit unless the -- file option > is specified > 4) Confirm "git reset -" works normally even when a file named @{-1} is > present > > Helped-by: David Aguilar <davvid@gmail.com> > Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> > --- > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh > index 98bcfe2..0faf241 100755 > --- a/t/t7102-reset.sh > +++ b/t/t7102-reset.sh > @@ -568,4 +568,143 @@ test_expect_success 'reset --mixed sets up work tree' ' > test_cmp expect actual > ' > > +test_expect_success 'reset - in the presence of file named - with previous branch' ' > + echo "Unstaged changes after reset:" >expect && > + echo "M -" >>expect && > + echo "M 1" >>expect && Mentioned by Matthieu: Use "cat <<-\EOF". > + git init no_previous && > + ( > + cd no_previous && > + >./- && Unnecessarily complex ">./-" when ">-" will suffice. > + >1 && > + git add 1 - && > + git commit -m "add base files" && > + git checkout -b new_branch && > + echo "random" >./- && > + echo "wow" >1 && > + git add 1 - && > + git reset - >../output Since you will be comparing this output with "expected" output, it is customary to name this file "actual". > + ) && > + rm -rf no_previous && Mentioned by Matthieu: Use test_when_finished(). You want this cleanup action invoked even if any part of the test fails, so register it as early as possible for it to be effective; either just before or after git-init. > + test_cmp output expect When test_cmp() detects a difference in the expected and actual output, it dumps the difference (in 'diff' format) as debugging output. It's easier to read 'diff' output, and matches our expectations more closely, if 'expect' is mentioned before 'actual', so: test_cmp expect actual > +' > +test_expect_success 'reset - in the presence of file named - with -- file option' ' > + echo "Unstaged changes after reset:" >expect && > + echo "M -" >>expect && > + git init no_previous && > + ( > + cd no_previous && > + >./- && > + >1 && > + git add 1 - && > + git commit -m "add base files" && > + git checkout -b new_branch && > + echo "random" >./- && > + echo "wow" >1 && > + git add 1 - && > + git reset -- - >../output > + ) && > + rm -rf no_previous Broken &&-chain. > + test_cmp output expect > +' > +test_expect_success 'reset - in the presence of file named - with both pre and post -- option' ' > + echo "Unstaged changes after reset:" >expect && > + echo "M -" >>expect && > + git init no_previous && > + ( > + cd no_previous && > + >./- && > + >1 && > + git add 1 - && > + git commit -m "add base files" && > + git checkout -b new_branch && > + echo "random" >./- && > + echo "wow" >1 && > + git add 1 - && > + git reset - -- - >../output > + ) && > + rm -rf no_previous Broken &&-chain. > + test_cmp output expect > +' > + > +test_expect_success 'reset - works same as reset @{-1}' ' > + git init no_previous && > + ( > + cd no_previous && > + echo "random" >random && > + git add random && > + git commit -m "base commit" && > + git checkout -b temp && > + echo new-file >new-file && > + git add new-file && > + git commit -m "added new-file" && > + git reset - && > + git status --porcelain >../first && > + git add new-file && > + git commit -m "added new-file" && > + git reset @{-1} && > + git status --porcelain >../second > + ) && In other tests, you performed "rm -rf no_previous &&" cleanup at this point, but it's missing here. > + test_cmp first second > +' > + > +test_expect_success 'reset - with file named @{-1}' ' > + echo "Unstaged changes after reset:" >expect && > + echo "M @{-1}" >>expect && > + git init no_previous && > + ( > + cd no_previous && > + echo "random" >./@{-1} && > + git add ./@{-1} && > + git commit -m "base commit" && > + git checkout -b new_branch && > + echo "additional stuff" >>./@{-1} && > + git add ./@{-1} && > + git reset - >../output > + ) && > + rm -rf no_previous && > + test_cmp output expect > +' > + > test_done > -- > 2.3.1.279.gd534259 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] reset: enable '-' short-hand for previous branch 2015-03-10 18:29 ` Eric Sunshine @ 2015-03-10 22:03 ` Sudhanshu Shekhar 2015-03-13 10:11 ` Eric Sunshine 2015-03-10 22:10 ` [PATCH v3 2/2] t7102: add 'reset -' tests Sudhanshu Shekhar 1 sibling, 1 reply; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 22:03 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, davvid, Matthieu.Moy, Sudhanshu Shekhar git reset -' will reset to the previous branch. It will behave similar to @{-1} except when a file named '@{-1}' is present. To refer to a file named '-', use ./- or the -- flag. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> --- Eric, I have added a user_input variable to record the input entered by the user. This way I can avoid the multiple 'if' clauses. Thank you for the suggestion. I have also removed the unrelated change that I had unintentionally committed. I am sending this patch on the thread for further review. Once both the patches are reviewed and accepted, I will create a new mail for it. Hope that is okay. Regards, Sudhanshu builtin/reset.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4c08ddc..b428241 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec, { const char *rev = "HEAD"; unsigned char unused[20]; + int substituted_minus = 0; + char *user_input = argv[0]; /* * Possible arguments are: * @@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec, */ if (argv[0]) { + if (!strcmp(argv[0], "-")) { + argv[0] = "@{-1}"; + substituted_minus = 1; + } if (!strcmp(argv[0], "--")) { argv++; /* reset to HEAD, possibly with paths */ } else if (argv[1] && !strcmp(argv[1], "--")) { @@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec, * Ok, argv[0] looks like a commit/tree; it should not * be a filename. */ - verify_non_filename(prefix, argv[0]); + verify_non_filename(prefix, user_input); rev = *argv++; } else { + /* We were treating "-" as a commit and not a file */ + if (substituted_minus) + argv[0] = "-"; /* Otherwise we treat this as a filename */ verify_filename(prefix, argv[0], 1); } -- 2.3.1.278.ge5c7b1f.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch 2015-03-10 22:03 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar @ 2015-03-13 10:11 ` Eric Sunshine 0 siblings, 0 replies; 13+ messages in thread From: Eric Sunshine @ 2015-03-13 10:11 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: Git List, Junio C Hamano, David Aguilar, Matthieu Moy On Tue, Mar 10, 2015 at 6:03 PM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote: > [PATCH v3 1/2] reset: enable '-' short-hand for previous branch This should be v4, I think, not v3. > git reset -' will reset to the previous branch. It will behave similar > to @{-1} except when a file named '@{-1}' is present. The way this is phrased, the reader is left hanging: "What happens when a file named @{-1} is present?" > To refer to a file named '-', use ./- or the -- flag. A documentation update to reflect this change would be appropriate. See, for instance, 696acf45 (checkout: implement "-" abbreviation, add docs and tests; 2009-01-17). > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> > --- > Eric, I have added a user_input variable to record the input entered > by the user. This way I can avoid the multiple 'if' clauses. Thank you > for the suggestion. > > I have also removed the unrelated change that I had unintentionally > committed. I am sending this patch on the thread for further review. > Once both the patches are reviewed and accepted, I will create a new > mail for it. Hope that is okay. Please wrap commentary to about 72 columns, just as you would the commit message, rather than typing it all on a single line. (I wrapped it manually here in order to reply to it.) > diff --git a/builtin/reset.c b/builtin/reset.c > index 4c08ddc..b428241 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec, > { > const char *rev = "HEAD"; > unsigned char unused[20]; > + int substituted_minus = 0; > + char *user_input = argv[0]; You're assigning a 'const char *' to a 'char *'. The compiler should have warned about it. This variable name is not as expressive as it could be. Try to name the variable after what you expect it to represent, for instance "maybe_rev" or "rev_or_path" or something more suitable. Even "orig_argv0" would be more informative than "user_input". > /* > * Possible arguments are: > * > @@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec, > */ > > if (argv[0]) { > + if (!strcmp(argv[0], "-")) { > + argv[0] = "@{-1}"; > + substituted_minus = 1; > + } > if (!strcmp(argv[0], "--")) { > argv++; /* reset to HEAD, possibly with paths */ > } else if (argv[1] && !strcmp(argv[1], "--")) { > @@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec, > * Ok, argv[0] looks like a commit/tree; it should not > * be a filename. > */ > - verify_non_filename(prefix, argv[0]); > + verify_non_filename(prefix, user_input); > rev = *argv++; > } else { > + /* We were treating "-" as a commit and not a file */ > + if (substituted_minus) > + argv[0] = "-"; In my review of the previous version[1], I mentioned that it was ugly to muck with argv[0]; moreover that it was particularly ugly to have to muck with it multiple times, undoing and redoing assignments. Although you've eliminated some reassignments via 'user_input', my intent, by asking if you could rework the logic, was to prompt you to take a couple steps back and examine the overall logic of the function more closely. The munging of argv[0] is effectively a fragile and undesirable band-aid approach. It is possible to support '-' as an alias for '@{-1}' without having to resort to such kludges at all. One other concern: When there is no previous branch, and no file named "-", then 'git reset -' misleadingly complains "bad flag '-' used after filename", rather than the more appropriate "ambiguous argument '-': unknown revision or path". [1]: http://article.gmane.org/gmane.comp.version-control.git/265255 > /* Otherwise we treat this as a filename */ > verify_filename(prefix, argv[0], 1); > } > -- > 2.3.1.278.ge5c7b1f.dirty ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] t7102: add 'reset -' tests 2015-03-10 18:29 ` Eric Sunshine 2015-03-10 22:03 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar @ 2015-03-10 22:10 ` Sudhanshu Shekhar 2015-03-13 10:27 ` Eric Sunshine 1 sibling, 1 reply; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 22:10 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, davvid, Matthieu.Moy, Sudhanshu Shekhar Add following test cases: 1) Confirm error message when git reset is used with no previous branch 2) Confirm git reset - works like git reset @{-1} 3) Confirm "-" is always treated as a commit unless the -- file option is specified 4) Confirm "git reset -" works normally even when a file named @{-1} is present Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Helped-by: David Aguilar <davvid@gmail.com> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> --- First of all, thank you for being so incredibly patient and helpful. I am very grateful for your remarks and reviews. I have implemented your suggestions in this patch. Please let me know if I have missed out on anything else. Also, sorry for sending PATCH 1/2 on the wrong thread, I entered the Message-ID incorrectly (still getting used to send-email :/ ). Regards, Sudhanshu t/t7102-reset.sh | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 98bcfe2..d3a5874 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -568,4 +568,163 @@ test_expect_success 'reset --mixed sets up work tree' ' test_cmp expect actual ' +test_expect_success 'reset - with no previous branch fails' ' + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "bad flag" no_previous/actual +' + +test_expect_success 'reset - while having file named - and no previous branch fails' ' + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + >- && + test_must_fail git reset - 2>actual + ) && + test_i18ngrep "bad flag" no_previous/actual +' + +test_expect_success \ + 'reset - in the presence of file named - with previous branch resets commit' ' + cat >expect <<-\EOF + Unstaged changes after reset: + M - + M file + EOF && + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + >- && + >file && + git add file - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >- && + echo "wow" >file && + git add file - && + git reset - >../actual + ) && + test_cmp expect actual +' + +test_expect_success \ + 'reset - in the presence of file named - with -- option resets commit' ' + cat >expect <<-\EOF + Unstaged changes after reset: + M - + M file + EOF && + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + >- && + >file && + git add file - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >- && + echo "wow" >file && + git add file - && + git reset - -- >../actual + ) && + test_cmp expect actual +' + +test_expect_success 'reset - in the presence of file named - with -- file option resets file' ' + cat >expect <<-\EOF + Unstaged changes after reset: + M - + M file + EOF && + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + >- && + >file && + git add file - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >- && + echo "wow" >file && + git add file - && + git reset -- - >../actual + ) && + test_cmp expect actual +' + +test_expect_success \ + 'reset - in the presence of file named - with both pre and post -- option resets file' ' + cat >expect <<-\EOF + Unstaged changes after reset:" >expect && + M - + EOF && + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + >- && + >file && + git add file - && + git commit -m "add base files" && + git checkout -b new_branch && + echo "random" >- && + echo "wow" >file && + git add file - && + git reset - -- - >../actual + ) && + test_cmp expect actual +' + +test_expect_success 'reset - works same as reset @{-1}' ' + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + echo "file1" >file1 && + git add file1 && + git commit -m "base commit" && + git checkout -b temp && + echo "new file" >file && + git add file && + git commit -m "added file" && + git reset - && + git status --porcelain >../actual && + git add file && + git commit -m "added file" && + git reset @{-1} && + git status --porcelain >../expect + ) && + test_cmp expect actual +' + +test_expect_success 'reset - with file named @{-1} succeeds' ' + cat >expect <<EOF + Unstaged changes after reset: + M file + M @{-1} + EOF && + git init no_previous && + test_when_finished rm -rf no_previous && + ( + cd no_previous && + echo "random" >@{-1} && + echo "random" >file && + git add @{-1} file && + git commit -m "base commit" && + git checkout -b new_branch && + echo "additional stuff" >>file && + echo "additional stuff" >>@{-1} && + git add file @{-1} && + git reset - >../actual + ) && + test_cmp expect actual +' + test_done -- 2.3.1.278.ge5c7b1f.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] t7102: add 'reset -' tests 2015-03-10 22:10 ` [PATCH v3 2/2] t7102: add 'reset -' tests Sudhanshu Shekhar @ 2015-03-13 10:27 ` Eric Sunshine 0 siblings, 0 replies; 13+ messages in thread From: Eric Sunshine @ 2015-03-13 10:27 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: Git List, Junio C Hamano, David Aguilar, Matthieu Moy On Tue, Mar 10, 2015 at 6:10 PM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote: > Add following test cases: > > 1) Confirm error message when git reset is used with no previous branch > 2) Confirm git reset - works like git reset @{-1} > 3) Confirm "-" is always treated as a commit unless the -- file option is specified > 4) Confirm "git reset -" works normally even when a file named @{-1} is present Does it concern you that all new tests pass except the last one even when patch 1/2 is not applied? I would have expected most or all of these tests to fail without patch 1/2. > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > Helped-by: David Aguilar <davvid@gmail.com> > Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> > --- > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh > index 98bcfe2..d3a5874 100755 > --- a/t/t7102-reset.sh > +++ b/t/t7102-reset.sh > @@ -568,4 +568,163 @@ test_expect_success 'reset --mixed sets up work tree' ' > test_cmp expect actual > ' > > +test_expect_success 'reset - with no previous branch fails' ' > + git init no_previous && I understand the intention of the name "no_previous" in tests for which there is no @{-1}, however... > + test_when_finished rm -rf no_previous && > + ( > + cd no_previous && > + test_must_fail git reset - 2>actual > + ) && > + test_i18ngrep "bad flag" no_previous/actual > +' > + > +test_expect_success 'reset - while having file named - and no previous branch fails' ' > + git init no_previous && > + test_when_finished rm -rf no_previous && > + ( > + cd no_previous && > + >- && > + test_must_fail git reset - 2>actual > + ) && > + test_i18ngrep "bad flag" no_previous/actual > +' > + > +test_expect_success \ > + 'reset - in the presence of file named - with previous branch resets commit' ' > + cat >expect <<-\EOF > + Unstaged changes after reset: > + M - > + M file > + EOF && > + git init no_previous && I don't understand the name "no_previous" in tests for which @{-1} can resolve. It probably would be better to choose a more generic name and use it for all these tests. For instance, "resetdash" or just "dash" or something better. > + test_when_finished rm -rf no_previous && > + ( > + cd no_previous && > + >- && > + >file && > + git add file - && > + git commit -m "add base files" && > + git checkout -b new_branch && > + echo "random" >- && > + echo "wow" >file && > + git add file - && > + git reset - >../actual > + ) && > + test_cmp expect actual > +' > + > +test_expect_success 'reset - with file named @{-1} succeeds' ' I may be missing something obvious, but why is it necessary to single out a file named @{-1} at all, particuarly when there are so many other ways to specify revisions, and there may be files mirroring those spellings, as well? > + cat >expect <<EOF > + Unstaged changes after reset: > + M file > + M @{-1} > + EOF && > + git init no_previous && > + test_when_finished rm -rf no_previous && > + ( > + cd no_previous && > + echo "random" >@{-1} && > + echo "random" >file && > + git add @{-1} file && > + git commit -m "base commit" && > + git checkout -b new_branch && > + echo "additional stuff" >>file && > + echo "additional stuff" >>@{-1} && > + git add file @{-1} && > + git reset - >../actual > + ) && > + test_cmp expect actual > +' > + > test_done > -- > 2.3.1.278.ge5c7b1f.dirty ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch 2015-03-10 10:52 [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar @ 2015-03-10 13:17 ` Matthieu Moy 2015-03-10 19:18 ` Eric Sunshine 2 siblings, 0 replies; 13+ messages in thread From: Matthieu Moy @ 2015-03-10 13:17 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: git, gitster, davvid, sunshine Sudhanshu Shekhar <sudshekhar02@gmail.com> writes: > *rev_ret = rev; > - > if (read_cache() < 0) Please don't make whitespace-only changes like this in your patches. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch 2015-03-10 10:52 [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar 2015-03-10 13:17 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Matthieu Moy @ 2015-03-10 19:18 ` Eric Sunshine 2015-03-10 22:12 ` Sudhanshu Shekhar 2 siblings, 1 reply; 13+ messages in thread From: Eric Sunshine @ 2015-03-10 19:18 UTC (permalink / raw) To: Sudhanshu Shekhar; +Cc: Git List, Matthieu Moy, Junio C Hamano, David Aguilar On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote: > 'git reset -' will reset to the previous branch. It will behave similar > to @{-1} except when a file named '@{-1}' is present. To refer to a file > named '-', use ./- or the -- flag. > > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> > --- > diff --git a/builtin/reset.c b/builtin/reset.c > index 4c08ddc..8abd300 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec, > */ > > if (argv[0]) { > + if (!strcmp(argv[0], "-")) { > + argv[0] = "@{-1}"; It's somewhat ugly to modify an element of argv[] since you don't own the array and the contract does not particularly suggest that is modifiable by you. A more serious concern is that doing so creates a tighter binding between parse_args() and its caller since parse_args() doesn't know how the caller will be disposing of argv[] when finished with it. Say, for instance, that the caller disposes of each argv[] element by invoking free(argv[n]). The literal "@{-1}" you've assigned here is not allocated on the heap, so free()ing it would be wrong. However, some of the other commands which alias "-" to "@{-1}" also modify argv[] in this way, so we'll let it slide for the moment. > + substituted_minus = 1; > + } > if (!strcmp(argv[0], "--")) { > argv++; /* reset to HEAD, possibly with paths */ > } else if (argv[1] && !strcmp(argv[1], "--")) { > @@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec, > * Ok, argv[0] looks like a commit/tree; it should not > * be a filename. > */ > + if (substituted_minus) > + argv[0] = "-"; > verify_non_filename(prefix, argv[0]); > + if (substituted_minus) > + argv[0] = "@{-1}"; Do you find it ugly that you have to undo and then redo your argv[] change from a few lines above? Rather than jumping through such hoops, can't you instead define a new variable which holds the appropriate value ("@{-1}"), or rework the logic to avoid such kludges altogether? > rev = *argv++; > } else { > + /* We were treating "-" as a commit and not a file */ > + if (substituted_minus) > + argv[0] = "-"; > /* Otherwise we treat this as a filename */ > verify_filename(prefix, argv[0], 1); > } > } > *rev_ret = rev; > - Mentioned by Matthieu: Don't sneak in unrelated changes. This change was probably unintentional, but serves as a good reminder that you should review the patch yourself before sending it. (And, if you do have a need to make cleanups, it's typically best to do so as a separate preparatory patch.) > if (read_cache() < 0) > die(_("index file corrupt")); > > -- > 2.3.1.279.gd534259 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] reset: enable '-' short-hand for previous branch 2015-03-10 19:18 ` Eric Sunshine @ 2015-03-10 22:12 ` Sudhanshu Shekhar 0 siblings, 0 replies; 13+ messages in thread From: Sudhanshu Shekhar @ 2015-03-10 22:12 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, davvid, Matthieu.Moy, Sudhanshu Shekhar git reset -' will reset to the previous branch. It will behave similar to @{-1} except when a file named '@{-1}' is present. To refer to a file named '-', use ./- or the -- flag. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com> --- Eric, I have added a user_input variable to record the input entered by the user. This way I can avoid the multiple 'if' clauses. Thank you for the suggestion. I have also removed the unrelated change that I had unintentionally committed. I am sending this patch on the thread for further review. Once both the patches are reviewed and accepted, I will create a new mail for it. Hope that is okay. Regards, Sudhanshu builtin/reset.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index 4c08ddc..b428241 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec, { const char *rev = "HEAD"; unsigned char unused[20]; + int substituted_minus = 0; + char *user_input = argv[0]; /* * Possible arguments are: * @@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec, */ if (argv[0]) { + if (!strcmp(argv[0], "-")) { + argv[0] = "@{-1}"; + substituted_minus = 1; + } if (!strcmp(argv[0], "--")) { argv++; /* reset to HEAD, possibly with paths */ } else if (argv[1] && !strcmp(argv[1], "--")) { @@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec, * Ok, argv[0] looks like a commit/tree; it should not * be a filename. */ - verify_non_filename(prefix, argv[0]); + verify_non_filename(prefix, user_input); rev = *argv++; } else { + /* We were treating "-" as a commit and not a file */ + if (substituted_minus) + argv[0] = "-"; /* Otherwise we treat this as a filename */ verify_filename(prefix, argv[0], 1); } -- 2.3.1.278.ge5c7b1f.dirty ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-03-13 10:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-10 10:52 [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar 2015-03-10 10:52 ` [PATCH v3 2/2] Added tests for reset - Sudhanshu Shekhar 2015-03-10 13:26 ` Matthieu Moy 2015-03-10 17:52 ` Sudhanshu Shekhar 2015-03-10 18:05 ` Eric Sunshine 2015-03-10 18:29 ` Eric Sunshine 2015-03-10 22:03 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Sudhanshu Shekhar 2015-03-13 10:11 ` Eric Sunshine 2015-03-10 22:10 ` [PATCH v3 2/2] t7102: add 'reset -' tests Sudhanshu Shekhar 2015-03-13 10:27 ` Eric Sunshine 2015-03-10 13:17 ` [PATCH v3 1/2] reset: enable '-' short-hand for previous branch Matthieu Moy 2015-03-10 19:18 ` Eric Sunshine 2015-03-10 22:12 ` Sudhanshu Shekhar
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.