Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch
On Tue, Mar 10, 2015 at 6:03 PM, Sudhanshu Shekhar
sudshekha...@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 gits...@pobox.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Sudhanshu Shekhar sudshekha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] reset: enable '-' short-hand for previous branch
'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 gits...@pobox.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Sudhanshu Shekhar sudshekha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch
Sudhanshu Shekhar sudshekha...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch
On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar
sudshekha...@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 gits...@pobox.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Sudhanshu Shekhar sudshekha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] reset: enable '-' short-hand for previous branch
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 gits...@pobox.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Sudhanshu Shekhar sudshekha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] reset: enable '-' short-hand for previous branch
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 gits...@pobox.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Sudhanshu Shekhar sudshekha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
