threads / patch / 38737

patch[GSoC Microproject]Adding "-" shorthand for "@{-1}" in RESET command

Subject: [PATCH] [GSoC Microproject]Adding "-" shorthand for "@{-1}" in RESET command

## tl;dr

2 messages between Mar 7, 2015 and Mar 8, 2015. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Sundararajan R· Mar 7, 2015, 01:57 UTC · lore
Hi all, I am a GSoC '15 aspirant for git.
In this commit I have directly associated "-" to "@{-1}" except when it refers to a filename. 
All the given tests pass(except those which shouldn't).
I have to add a failsafe for the case in when there is no branch as "@{-1}". For this I have a 
rough idea that I would have to call get-sha1() on @{-1} to check if there is an object matching 
with it. But I am not able to think of the details.
Please guide me with that and give feedback for this patch.
Signed-off-by: Sundararajan R <dyoucme@gmail.com>
---
 builtin/reset.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
Show changes to builtin/reset.c +11 −1
diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..62764d4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -203,8 +203,16 @@ static void parse_args(struct pathspec *pathspec,
 	 *
 	 * At this point, argv points immediately after [-opts].
 	 */
-
+	int flag=0; /* 
+		     *  "-" may refer to filename in which case we should be giving more precedence 
+		     *  to filename than equating argv[0] to "@{-1}" 
+		     */
 	if (argv[0]) {
+		if (!strcmp(argv[0], "-") && !argv[1])  /* "-" is the only argument */
+		{
+			argv[0]="@{-1}";
+			flag=1;
+		}
 		if (!strcmp(argv[0], "--")) {
 			argv++; /* reset to HEAD, possibly with paths */
 		} else if (argv[1] && !strcmp(argv[1], "--")) {
@@ -226,6 +234,8 @@ static void parse_args(struct pathspec *pathspec,
 			rev = *argv++;
 		} else {
 			/* Otherwise we treat this as a filename */
+			if(flag)
+				argv[0]="-";
 			verify_filename(prefix, argv[0], 1);
 		}
 	}

-- 
2.1.0
Junio C Hamano· Mar 8, 2015, 07:34 UTC · re: Sundararajan R · lore

Re: [PATCH] [GSoC Microproject]Adding "-" shorthand for "@{-1}" in RESET command

Sundararajan R <dyoucme@gmail.com> writes:
Show 13 quoted lines
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..62764d4 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -203,8 +203,16 @@ static void parse_args(struct pathspec *pathspec,
>  	 *
>  	 * At this point, argv points immediately after [-opts].
>  	 */
> -
> +	int flag=0; /* 
> +		     *  "-" may refer to filename in which case we should be giving more precedence 
> +		     *  to filename than equating argv[0] to "@{-1}" 
> +		     */

Comment on a separate line. More importantly, think if you can give the variable a more meaningful name so that you do not have to explain.

You are missing SPs requested by the coding guideline everywhere in your patch.

Show 10 quoted lines
>  	if (argv[0]) {
> +		if (!strcmp(argv[0], "-") && !argv[1])  /* "-" is the only argument */
> +		{
> +			argv[0]="@{-1}";
> +			flag=1;
> +		}
>  		if (!strcmp(argv[0], "--")) {
>  			argv++; /* reset to HEAD, possibly with paths */
>  		} else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -226,6 +234,8 @@ static void parse_args(struct pathspec *pathspec,

Around here not shown by this patch there are a few uses of argv[0], and the most important one is

			verify_non_filename(prefix, argv[0]);
just before the line below (see below).
Show 8 quoted lines
>  			rev = *argv++;
>  		} else {
>  			/* Otherwise we treat this as a filename */
> +			if(flag)
> +				argv[0]="-";
>  			verify_filename(prefix, argv[0], 1);
>  		}
>  	}

By the way, do you understand the intent of the existing checks in this codepath that uses verify_filename() and verify_non_filename()?

The idea is to allow users to write "git reset X" and "git reset Y Z" safely in an unambiguous way.

 * X could be a commit (e.g. "git reset master"), to update the
   current branch to point at the same commit as 'master' and update
   the index to match.
 * X could be a pathspec (e.g. "git reset hello.c"), to grab the
   blob object for X out of the HEAD and put it in the index.
 * Y could be a tree-ish and Z a pathspec (e.g. "git reset HEAD^
   hello.c"), to grab the blob object for Z out of tree-ish Y and
   put it to the index.
 * Both Y and Z could be pathspecs (e.g. "git reset hello.c
   goodbye.c"), to revert the index entries for these two paths to
   what the HEAD records.

If you happen to have a file whose name is 'master', and if you are working on your 'topic' branch, what would this do?

    $ git reset master

Is this a request to revert the index entry for path 'master' from the HEAD? Or is it a request to update the current branch to be the same as the 'master' branch and repopulate the index from there?

What does the existing code try to do, and how does it do it? It detects the ambiguity and refuses to do either, to make sure it causes no harm.

Now, with your change, does the result still honor this "when ambiguous, stop without causing harm to the user" principle? What happens when your user has a file whose name is "-" in the working tree? What happens when your user has a file whose name is "@{-1}" in the working tree?

← back to recent threads