[PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Search and replace "-" (written in the context of a branch name) in the argument
list with "@{-1}". As per the help text of git rev-list, this includes the following four
cases:

  a. "-"
  b. "^-"
  c. "-..other-branch-name" or "other-branch-name..-"
  d. "-...other-branch-name" or "other-branch-name...-"

(a) and (b) have been implemented as in the previous implementations of
this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
short-hand for "previous branch", 2011-04-07)

(c) and (d) have been implemented by using the strbuf API, growing it to the
right size and placing "@{-1}" instead of "-"

Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
This is a patch for one of the microprojects of SoC 2017. [1]

I have implemented this using multiple methods, that I have re-written again and
again for better versions ([2]). The present version I feel is the closest that
I could get to the existing code in the repository. This patch only uses
functions that are commonly used in the rest of the codebase.

I still have to write tests, as well as update documentation as done in 696acf45
(checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).

I request your comments on this patch. Also, I have the following questions
regarding this patch:

1. Is the approach that I have used to solve this problem fine?
2. Is the code I am writing in the right function? (I have put it right
before the revisions data structure is setup, thus these changes affect only
git-log)

[1]: https://git.github.io/SoC-2017-Microprojects/
[2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
strbufs for the starting 4 characters, and last 4 characters and compares those
to the appropriate strings for case (c) and case (d). I edited this patch to use
strstr instead, which avoids all the strbuf declarations)

 builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..a5aac99 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
 	struct userformat_want w;
-	int quiet = 0, source = 0, mailmap = 0;
+	int quiet = 0, source = 0, mailmap = 0, i = 0;
 	static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};

 	const struct option builtin_log_options[] = {
@@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,

 	if (quiet)
 		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
+
+	/*
+	 * Check if any argument has a "-" in it, which has been referred to as a
+	 * shorthand for @{-1}.  Handles methods that might be used to list commits
+	 * as mentioned in git rev-list --help
+	 */
+
+	for(i = 0; i < argc; ++i) {
+		if (!strcmp(argv[i], "-")) {
+			argv[i] = "@{-1}";
+		} else if (!strcmp(argv[i], "^-")) {
+			argv[i] = "^@{-1}";
+		} else if (strlen(argv[i]) >= 4) {
+
+			if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) {
+				struct strbuf changed_argument = STRBUF_INIT;
+
+				strbuf_addstr(&changed_argument, "@{-1}");
+				strbuf_addstr(&changed_argument, argv[i] + 1);
+
+				strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
+
+				argv[i] = strbuf_detach(&changed_argument, NULL);
+			}
+
+			/*
+			 * Find the first occurence, and add the size to it and proceed if
+			 * the resulting value is NULL
+			 */
+			if (!(strstr(argv[i], "...-") + 4)  ||
+					!(strstr(argv[i], "..-") + 3)) {
+				struct strbuf changed_argument = STRBUF_INIT;
+
+				strbuf_addstr(&changed_argument, argv[i]);
+
+				strbuf_grow(&changed_argument, strlen(argv[i]) + 4);
+				strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
+
+				strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5);
+
+				argv[i] = strbuf_detach(&changed_argument, NULL);
+			}
+		}
+	}
+
 	argc = setup_revisions(argc, argv, rev, opt);

 	/* Any arguments at this point are not recognized */
--
2.1.4

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Pranit Bauva]

Hey Siddharth,

On Sun, Feb 5, 2017 at 6:27 PM, Siddharth Kannan
<kannan.siddharth12@gmail.com> wrote:
> Search and replace "-" (written in the context of a branch name) in the argument
> list with "@{-1}". As per the help text of git rev-list, this includes the following four
> cases:
>
>   a. "-"
>   b. "^-"
>   c. "-..other-branch-name" or "other-branch-name..-"
>   d. "-...other-branch-name" or "other-branch-name...-"
>
> (a) and (b) have been implemented as in the previous implementations of
> this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
> docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
> abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
> short-hand for "previous branch", 2011-04-07)
>
> (c) and (d) have been implemented by using the strbuf API, growing it to the
> right size and placing "@{-1}" instead of "-"
>
> Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
> ---
> This is a patch for one of the microprojects of SoC 2017. [1]
>
> I have implemented this using multiple methods, that I have re-written again and
> again for better versions ([2]). The present version I feel is the closest that
> I could get to the existing code in the repository. This patch only uses
> functions that are commonly used in the rest of the codebase.
>
> I still have to write tests, as well as update documentation as done in 696acf45
> (checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).
>
> I request your comments on this patch. Also, I have the following questions
> regarding this patch:
>
> 1. Is the approach that I have used to solve this problem fine?
> 2. Is the code I am writing in the right function? (I have put it right
> before the revisions data structure is setup, thus these changes affect only
> git-log)
>
> [1]: https://git.github.io/SoC-2017-Microprojects/
> [2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
> strbufs for the starting 4 characters, and last 4 characters and compares those
> to the appropriate strings for case (c) and case (d). I edited this patch to use
> strstr instead, which avoids all the strbuf declarations)
>
>  builtin/log.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 55d20cc..a5aac99 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>                          struct rev_info *rev, struct setup_revision_opt *opt)
>  {
>         struct userformat_want w;
> -       int quiet = 0, source = 0, mailmap = 0;
> +       int quiet = 0, source = 0, mailmap = 0, i = 0;
>         static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
>
>         const struct option builtin_log_options[] = {
> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>
>         if (quiet)
>                 rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> +
> +       /*
> +        * Check if any argument has a "-" in it, which has been referred to as a
> +        * shorthand for @{-1}.  Handles methods that might be used to list commits
> +        * as mentioned in git rev-list --help
> +        */
> +
> +       for(i = 0; i < argc; ++i) {
> +               if (!strcmp(argv[i], "-")) {
> +                       argv[i] = "@{-1}";
> +               } else if (!strcmp(argv[i], "^-")) {
> +                       argv[i] = "^@{-1}";
> +               } else if (strlen(argv[i]) >= 4) {
> +
> +                       if (strstr(argv[i], "-...") == argv[i] || strstr(argv[i], "-..") == argv[i]) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, "@{-1}");
> +                               strbuf_addstr(&changed_argument, argv[i] + 1);
> +
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +
> +                       /*
> +                        * Find the first occurence, and add the size to it and proceed if
> +                        * the resulting value is NULL
> +                        */
> +                       if (!(strstr(argv[i], "...-") + 4)  ||
> +                                       !(strstr(argv[i], "..-") + 3)) {
> +                               struct strbuf changed_argument = STRBUF_INIT;
> +
> +                               strbuf_addstr(&changed_argument, argv[i]);
> +
> +                               strbuf_grow(&changed_argument, strlen(argv[i]) + 4);
> +                               strbuf_setlen(&changed_argument, strlen(argv[i]) + 4);
> +
> +                               strbuf_splice(&changed_argument, strlen(argv[i]) - 1, 5, "@{-1}", 5);
> +
> +                               argv[i] = strbuf_detach(&changed_argument, NULL);
> +                       }
> +               }
> +       }
> +
>         argc = setup_revisions(argc, argv, rev, opt);
>
>         /* Any arguments at this point are not recognized */
> --


It is highly recommended to follow the pre existing style of code and
commits. In the micro project list, I think it is mentioned that this
similar thing is implemented in git-merge so you should try and dig
the commit history of that file to find the similar change.

If you do this, then you will find out that there is a very short and
sweet way to do it. I won't directly point out the commit.

strbuf API should be used when you need to modify the contents of the
string. I think you have a little confusion.

If you declare the string as,

const char *str = "foo";

then, you can also do,

str = "bar";

But you can't do,

str[1] = 'z';

I hope you get what I am saying, if not, search for it.

Regards,
Pranit Bauva

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Junio C Hamano]

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>
>  	if (quiet)
>  		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> +
> +	/*
> +	 * Check if any argument has a "-" in it, which has been referred to as a
> +	 * shorthand for @{-1}.  Handles methods that might be used to list commits
> +	 * as mentioned in git rev-list --help
> +	 */
> +
> +	for(i = 0; i < argc; ++i) {
> +		if (!strcmp(argv[i], "-")) {
> +			argv[i] = "@{-1}";
> +		} else if (!strcmp(argv[i], "^-")) {
> +			argv[i] = "^@{-1}";
> +		} else if (strlen(argv[i]) >= 4) {
> +
> +	...
> +		}
> +	}
> +
>  	argc = setup_revisions(argc, argv, rev, opt);

"Turn '-' to '@{-1}' before we do the real parsing" can never be a
reasonable strategy to implement the desired "'-' means the tip of
the previous branch" correctly.  To understand why, you only need to
imagine what happens to this command:

    $ git log --grep '^-'

Turning it into "git log --grep '^@{-1}'" obviously is not what the
end-users want, so that is an immediate bug in the version of Git
with this patch applied.

Even if this were not a patch for the "log" command but for some
other command, a change with the above approach is very much
unwelcome, even if that other command does not currently have any
option that takes arbitrary string the user may want to specify
(like "find commit with a line that matches this pattern" option
called "--grep" the "log" command has).  That is because it will
make it impossible to enhance the command by adding such an option
in the future.  So it is also adding the problems to future
developers (and users) of Git.

A correct solution needs to know if the argument is at the position
where a revision (or revision range) is expected and then give the
tip of the previous branch when it sees "-" (and other combinations
this patch tries to cover).  In other words, the parser always knows
what it is parsing, and if and only if it is parsing a rev, react to
"-" and think "ah, the user wants me to use the tip of the previous
branch".

But the code that knows that it expects to see a revision already
exists, and it is the real parser.  In the above snippet,
setup_revisions() is the one that does the real parsing of argv[].
The code there knows when it wants to see a rev, and takes argv[i]
and turns into an object to call add_pending_object().  That codepath
may not yet know that "-" means the tip of the previous branch, and
that is where the change needs to go.

Such a properly-done update does not need to textually replace "-"
with "@{-1}" in argv[]; the codepath is where it understands what
any textual representation of a rev the user gave it means, and it
understands "@{-1}" there.  It would be the matter of updating it to
also understand what "-" means.

A correct solution will be a lot more involved, of course, and I
think it will be larger than a reasonable microproject for people
new to the codebase.

I didn't check the microproject ideas page myself; whether it says
that turning "-" unconditionally to "@{-1}" is a good idea, or it
hints that supporting "-" as "the tip of the previous branch" in
more commands is a reasonable byte-sized microproject, I think it is
misleading and misguided.  Can somebody remove that entry so that we
won't waste time of new developers (which would lead to discouraging
them)?  Thanks.

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hey Junio,
On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> 
> > @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
> >
> >  	if (quiet)
> >  		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> > +
> > +	/*
> > +	 * Check if any argument has a "-" in it, which has been referred to as a
> > +	 * shorthand for @{-1}.  Handles methods that might be used to list commits
> > +	 * as mentioned in git rev-list --help
> > +	 */
> > +
> > +	for(i = 0; i < argc; ++i) {
> > +		if (!strcmp(argv[i], "-")) {
> > +			argv[i] = "@{-1}";
> > +		} else if (!strcmp(argv[i], "^-")) {
> > +			argv[i] = "^@{-1}";
> > +		} else if (strlen(argv[i]) >= 4) {
> > +
> > +	...
> > +		}
> > +	}
> > +
> >  	argc = setup_revisions(argc, argv, rev, opt);
> 
> "Turn '-' to '@{-1}' before we do the real parsing" can never be a
> reasonable strategy to implement the desired "'-' means the tip of
> the previous branch" correctly.  To understand why, you only need to
> imagine what happens to this command:
> 
>     $ git log --grep '^-'
> 
> Turning it into "git log --grep '^@{-1}'" obviously is not what the
> end-users want, so that is an immediate bug in the version of Git
> with this patch applied.
> 
> Even if this were not a patch for the "log" command but for some
> other command, a change with the above approach is very much
> unwelcome, even if that other command does not currently have any
> option that takes arbitrary string the user may want to specify
> (like "find commit with a line that matches this pattern" option
> called "--grep" the "log" command has).  That is because it will
> make it impossible to enhance the command by adding such an option
> in the future.  So it is also adding the problems to future
> developers (and users) of Git.

Understood!
> 
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".

Ah, okay. I will do another one of the suggestions as my micro project
but continue to look into this part of the code and try to find the
right place to write the code to implement the present patch.

> 
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.
> 
> Such a properly-done update does not need to textually replace "-"
> with "@{-1}" in argv[]; the codepath is where it understands what
> any textual representation of a rev the user gave it means, and it
> understands "@{-1}" there.  It would be the matter of updating it to
> also understand what "-" means.
> 
> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.
> 
> I didn't check the microproject ideas page myself; whether it says
> that turning "-" unconditionally to "@{-1}" is a good idea, or it
> hints that supporting "-" as "the tip of the previous branch" in
> more commands is a reasonable byte-sized microproject, I think it is
> misleading and misguided.  Can somebody remove that entry so that we
> won't waste time of new developers (which would lead to discouraging
> them)?  Thanks.

Thanks a lot for writing this detailed reply! I will definitely take
into account all of the points mentioned here in the future patches I
send.

- Siddharth Kannan

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hey Junio, I did some more digging into the codepath:

On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> 
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".
> 
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.

Inside setup_revisions, it tries to parse arguments and options. In
there, is this line of code:

    if (*arg == '-') {

Once control enters this branch, it will either parse the argument as
an option / pseudo-option, or simply leave this argument as is in the
argv[] array and move forward with the other arguments.

So, first I need to teach setup_revisions that something starting with
a "-" might be a revision or a revision range.

After this, going further down the codepath, in
revision.c:handle_revision_arg: 

The argument is parsed to find out if it is of the form
rev1...rev2 or rev1..rev2 or just rev1, etc.

(a) -> If `..` or `...` was found, then two pointers "this" and "next"
now hold the from and to revisions, and the function
get_sha1_committish is called on them. In case both were found to be
committish, then the char pointers now hold the sha1 in them, they are
parsed into objects.

(b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To
get r1, again the function get_sha1_committish is called with only r1
as the parameter.

(c) -> Else look for "r1^-"

(d) -> Else look for the argument using the same get_sha1_committish
function (It any "^" was present in it, it has already been noted and
removed)

Cases (a), (b) and (d) can be handled by putting this inside
get_sha1_committish. (Further discussion about that below)

Case (c) is a bit confusing. This could be something like "-^-", and
something like "^-" could mean "Not commits on previous branch" or it
could mean "All commits on this branch except for the parent of HEAD"
Please tell me if this is confusing or undesired altogether.
Personally, I feel that people who have been using "^-" would be
very confused if it's behaviour changed.

So, all the code till now points at adding the patch for "-" = "@{-1}"
inside get_sha1_committish or downstream from there.

get_sha1_committish 
-> get_sha1_with_context 
-> get_sha1_with_context_1
-> get_sha1_1 
  -> peel_onion -> calling get_sha1_basic again with the ref
  only (after peeling) 
  -> get_sha1_basic -> includes parsing of "@{-N}" type revs. So, 
  this indicates that if we can convert the "-" appropriately 
  before this point, then it would be good.
  -> get_short_sha1

So, this patch reduces to the following 2 tasks:

1. Teach setup_revisions that something starting with "-" can be an
argument as well
2. Teach get_sha1_basic that "-" means the tip of the previous branch
perhaps by replacing it with "@{-1}" just before the reflog parsing is
done

> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.

So true :) I had spent a fair bit of time already on my previous patch,
and I thought I might as well complete my research into this, and send
this write-up to the mailing list, so that I could write a patch some
time later. In case you would prefer for me to not work on this
anymore because I am new to the codebase, I will leave it at this.

- Siddharth Kannan

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Junio C Hamano]

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> Hey Junio, I did some more digging into the codepath:
> ...
> In case you would prefer for me to not work on this anymore
> because I am new to the codebase, I will leave it at this.

The above is nicely analized and summarized.

The earlier mention of "those new to the codebase" by me was "this
is an inappropriate topic as a GSoC microproject for people new to
the codebase" and it wasn't meant to say "this part of the code is
too precious to let unknown folks touch it."

The focus of GSoC being mentoring those who are new to the open
source development, and hopefully retain them in the community after
GSoC is over, we do expect microprojects to be suitable for those
who are new to the codebase.

The focus of microprojects are twofold.  It is a way for new people
to learn the way in which they will be interacting with the
community once they become Git developers, sending their patches
(which includes analyzing and explaining the problem they are trying
to solve and their solution to it) and receiving and responding to
review comments.  We also want to find out which candidates are
willing to learn and which ones are difficult to work with during
the process.  And its primary focus is not about solving the real
issues the project has with its code---something "bite-sized" is
sufficient (and desirable) for microprojects for both GSoC student
candidates and GSoC mentors and reviewers to work with.

> (c) -> Else look for "r1^-"
> ...
> Case (c) is a bit confusing. This could be something like "-^-", and
> something like "^-" could mean "Not commits on previous branch" or it
> could mean "All commits on this branch except for the parent of HEAD"

Do you mean:

    "git rev-parse ^-" does not mean "git rev-parse HEAD^-", but we
    probably would want to, and if that is what is going to happen,
    "^-" should mean "HEAD^-", and cannot be used for "^@{-1}"?

It's friend "^!" does not mean "HEAD^!", and "^@" does not mean
"HEAD^@", either (the latter is somewhat borked, though, and "^@"
translates to "^HEAD" because confusingly "@" stands for "HEAD"
sometimes).  

So my gut feeling is that it is probably OK to make "^-" mean
"^@{-1}"; it may be prudent to at least initially keep "^-" an error
like it currently is already, though.

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

On Mon, Feb 06, 2017 at 03:09:47PM -0800, Junio C Hamano wrote:
> The focus of GSoC being mentoring those who are new to the open
> source development, and hopefully retain them in the community after
> GSoC is over, we do expect microprojects to be suitable for those
> who are new to the codebase.

Okay, understood! Since I have spent time here anyway, I guess I will
continue on this instead of going over to a new micro project.

> 
> > (c) -> Else look for "r1^-"
> > ...
> > Case (c) is a bit confusing. This could be something like "-^-", and
> > something like "^-" could mean "Not commits on previous branch" or it
> > could mean "All commits on this branch except for the parent of HEAD"
> 
> Do you mean:
> 
>     "git rev-parse ^-" does not mean "git rev-parse HEAD^-", but we
>     probably would want to, and if that is what is going to happen,
>     "^-" should mean "HEAD^-", and cannot be used for "^@{-1}"?
> 
> It's friend "^!" does not mean "HEAD^!", and "^@" does not mean
> "HEAD^@", either (the latter is somewhat borked, though, and "^@"
> translates to "^HEAD" because confusingly "@" stands for "HEAD"
> sometimes).  

Yes, I meant that whether we should use ^- as ^@{-1} or HEAD^-.

Oh! So, that's why running `git log ^@` leads to an empty set!
> 
> So my gut feeling is that it is probably OK to make "^-" mean
> "^@{-1}"; it may be prudent to at least initially keep "^-" an error
> like it currently is already, though.

I agree with your gut feeling, and would like to _not_ exclude only
this case. This way, across the code and implementation, there
wouldn't be any particular cases which would have to be excluded.

> > So, this patch reduces to the following 2 tasks:
> > 
> > 1. Teach setup_revisions that something starting with "-" can be
> > an
> > argument as well
> > 2. Teach get_sha1_basic that "-" means the tip of the previous
> > branch
> > perhaps by replacing it with "@{-1}" just before the reflog
> > parsing is
> > done

Making a change in sha1_name.c will touch a lot of commands
(setup_revisions is called from everywhere in the codebase), so, I am
still trying to figure out how to do this such that the rest of the
codepath remains unchanged.

I hope that you do not mind this side-effect, but rather, you intended
for this to happen, right? More commands will start supporting this
shorthand, suddenly.  (such as format-patch, whatchanged, diff to name
a very few).

Best Regards,

Siddharth.

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Matthieu Moy]

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> Making a change in sha1_name.c will touch a lot of commands
> (setup_revisions is called from everywhere in the codebase), so, I am
> still trying to figure out how to do this such that the rest of the
> codepath remains unchanged.

Changing sha1_name.c is the way to go *if* we want all commands to
support this. Just like other ways to name a revision...

> I hope that you do not mind this side-effect, but rather, you intended
> for this to happen, right? More commands will start supporting this
> shorthand, suddenly.  (such as format-patch, whatchanged, diff to name
> a very few).

... but: the initial implementation of this '-' shorthand was
special-casing a single command (IIRC, "git checkout") for which the
shorthand was useful.

In a previous discussion, I made an analogy with "cd -" (which is the
source of inspiration of this shorthand AFAIK): "-" did not magically
become "the last visited directory" for all Unix commands, just for
"cd". And in this case, I'm happy with it. For example, I never need
"mkdir -", and I'm happy I can't "rm -fr -" by mistake.

So, it's debatable whether it's a good thing to have all commands
support "-". For example, forcing users to explicitly type "git branch
-d @{1}" and not providing them with a shortcut might be a good thing.

I don't have strong opinion on this: I tend to favor consistency and
supporting "-" everywhere goes in this direction, but I think the
downsides should be considered too. A large part of the exercice here is
to write a good commit message!

Another issue with this is: - is also a common way to say "use stdin
instead of a file", so before enabling - for "previous branch", we need
to make sure it does not introduce any ambiguity. Git does not seem to
use "- for stdin" much (most commands able to read from stdin have an
explicit --stdin option for that), a quick grep in the docs shows only
"git blame --contents -" which is OK because a revision wouldn't make
sense here anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hello Matthieu,

On 8 February 2017 at 20:10, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> In a previous discussion, I made an analogy with "cd -" (which is the
> source of inspiration of this shorthand AFAIK): "-" did not magically
> become "the last visited directory" for all Unix commands, just for
> "cd". And in this case, I'm happy with it. For example, I never need
> "mkdir -", and I'm happy I can't "rm -fr -" by mistake.
>
> So, it's debatable whether it's a good thing to have all commands
> support "-". For example, forcing users to explicitly type "git branch
> -d @{1}" and not providing them with a shortcut might be a good thing.

builtin/branch.c does not call setup_revisions and remains unaffected
by this patch :)

In my original patch post, I was not explicit about what files call
setup_revisions.
I would like to rectify that with this (grep-ed) list:

* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c

* marked commands only show information, and don't change anything.

As you might notice, in this list, most commands are not of the `rm` variety,
i.e. something that would delete stuff.

In the next version of this patch, I will definitely include the list
of commands
which are "rm-ish" and affected by this patch.

>
> I don't have strong opinion on this: I tend to favor consistency and
> supporting "-" everywhere goes in this direction, but I think the
> downsides should be considered too. A large part of the exercice here is
> to write a good commit message!

Yes, I prefer consistency very much as well! Having "-" mean the same thing
across a lot of commands is better than having that shorthand only in a few
commands, as it is now. Unless there is a specific confusion that might arise
because of this shorthand inclusion, I think that this shorthand
should be supported
across the board.
(I especially like typing `git checkout - <filename>` which is very handy!)

>
> Another issue with this is: - is also a common way to say "use stdin
> instead of a file", so before enabling - for "previous branch", we need
> to make sure it does not introduce any ambiguity. Git does not seem to
> use "- for stdin" much (most commands able to read from stdin have an
> explicit --stdin option for that), a quick grep in the docs shows only
> "git blame --contents -" which is OK because a revision wouldn't make
> sense here anyway.

Yes, just to jog your memory, this was discussed here [1]

Junio said:

    As long as the addition is carefully prepared so that we know it
    will not conflict (or be confused by users) with possible other uses
    of "-", I do not think we would mind "git branch -D -" and other
    commands to learn "-" as a synonym for @{-1}.

>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Thanks a lot for the review on this patch, Matthieu!

-- 

Best Regards,

- Siddharth.

[1]: https://public-inbox.org/git/7vmwpitb6k.fsf@alter.siamese.dyndns.org/

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Matthieu Moy]

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> Hello Matthieu,
>
> On 8 February 2017 at 20:10, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> In a previous discussion, I made an analogy with "cd -" (which is the
>> source of inspiration of this shorthand AFAIK): "-" did not magically
>> become "the last visited directory" for all Unix commands, just for
>> "cd". And in this case, I'm happy with it. For example, I never need
>> "mkdir -", and I'm happy I can't "rm -fr -" by mistake.
>>
>> So, it's debatable whether it's a good thing to have all commands
>> support "-". For example, forcing users to explicitly type "git branch
>> -d @{1}" and not providing them with a shortcut might be a good thing.
>
> builtin/branch.c does not call setup_revisions and remains unaffected
> by this patch :)

Right, I forgot this: in some place we need any revspec, but "branch -d"
needs a branch name explicitly.

> [...]
> As you might notice, in this list, most commands are not of the `rm` variety,
> i.e. something that would delete stuff.

OK, I think I'm convinced.

Keep the arguments in mind when polishing the commit message.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

On 9 February 2017 at 17:55, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> [...]
>> As you might notice, in this list, most commands are not of the `rm` variety,
>> i.e. something that would delete stuff.
>
> OK, I think I'm convinced.

I am glad! :)

>
> Keep the arguments in mind when polishing the commit message.

I will definitely do that. I am working on a good commit message for
this by looking at some past changes to sha1_name.c which have
affected multiple commands.

>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

-- 

Best Regards,

- Siddharth.