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

On Sun, Mar 8, 2015 at 1:04 PM Junio C Hamano <gitster@pobox.com> wrote: Sundararajan R <dyoucme@gmail.com> writes:

giving more precedence

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.

argument */

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).

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?

-------------------------------------------------------------------------- Hi all,

I am sorry for the mistakes in the code formatting. It was because I was in a hurry that day and I wanted to submit a working patch. In the new patch I am making, I am using check_filename() to see if there are files named "-" and "@{-1}" in the working tree . Is this an appropriate way to check or is there something else suggested?

Thanks a lot. R Sundararajan.

Sundararajan R <dyoucme@gmail.com> writes:

No need to apologize for mistakes. Mistakes are expected part of being human and the review process is designed to catch exactly that.

The development is not a race to see who gets there first. It is a collaborative process to get to a better place together. Once you got something "working", stop and review your work to see if your definition of "working" is sensible. Is there a corner case you missed? Is the code formatted in a similar way as the existing code around the area you are touching? Are there better ways to do what you did? Take your time to make sure you would be happy with what you are sending out.

I think you are making it unnecessarily hard. With your patch, the code would look like this:

 	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], "--")) {
 			rev = argv[0];
 			argv += 2;
 		}
 		/*
 		 * Otherwise, argv[0] could be either <rev> or <paths> and
 		 * has to be unambiguous. If there is a single argument, it
 		 * can not be a tree
 		 */
 		else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||
 			 (argv[1] && !get_sha1_treeish(argv[0], unused))) {
 			/*
 			 * Ok, argv[0] looks like a commit/tree; it should not
 			 * be a filename.
 			 */
 			verify_non_filename(prefix, argv[0]);
 			rev = *argv++;
 		} else {
 			/* Otherwise we treat this as a filename */
+			if(flag)
+				argv[0]="-";
 			verify_filename(prefix, argv[0], 1);
 		}
 	}

I was wondering what you are passing to verify_non_filename() that you did not touch. It would see "@{-1}", try to make sure that the working tree does not have a file with that name, and if there is the end user would be warned about ambiguity.

If the user typed "git reset @{-1}", then that warning is very sensible, but when the end user only typed "git reset -", is there any ambiguity?