threads / rfc / 45053

RFC patchWIP: log: allow "-" as a short-hand for "previous branch"

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

## tl;dr

11 messages between Feb 5, 2017 and Feb 9, 2017. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Siddharth Kannan· Feb 5, 2017, 12:57 UTC · lore

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(-)
Show changes to builtin/log.c +46 −2
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
Pranit Bauva· Feb 5, 2017, 14:55 UTC · re: Siddharth Kannan · lore

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

Hey Siddharth,

On Sun, Feb 5, 2017 at 6:27 PM, Siddharth Kannan <kannan.siddharth12@gmail.com> wrote:

Show 113 quoted lines
> 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

Junio C Hamano· Feb 6, 2017, 00:15 UTC · re: Siddharth Kannan · lore

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

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
Show 23 quoted lines
> @@ -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.

Siddharth Kannan· Feb 6, 2017, 02:27 UTC · re: Junio C Hamano · lore

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

Hey Junio, On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:

Show 46 quoted lines
> 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!
Show 8 quoted lines
> 
> 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.

Show 26 quoted lines
> 
> 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
Siddharth Kannan· Feb 6, 2017, 18:10 UTC · re: Junio C Hamano · lore

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

Hey Junio, I did some more digging into the codepath:
On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> 
> 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
Junio C Hamano· Feb 6, 2017, 23:09 UTC · re: Siddharth Kannan · lore

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

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.

Show 5 quoted lines
> (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.

Siddharth Kannan· Feb 7, 2017, 19:14 UTC · re: Junio C Hamano · lore

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

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.

Show 17 quoted lines
> 
> > (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.

Show 10 quoted lines
> > 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.
Matthieu Moy· Feb 8, 2017, 14:40 UTC · re: Siddharth Kannan · lore

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

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/
Siddharth Kannan· Feb 8, 2017, 17:23 UTC · re: Matthieu Moy · lore

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

Hello Matthieu,
On 8 February 2017 at 20:10, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
Show 9 quoted lines
> 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.

Show 5 quoted lines
>
> 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!)

Show 8 quoted lines
>
> 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/
Matthieu Moy· Feb 9, 2017, 12:25 UTC · re: Siddharth Kannan · lore

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

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
Show 15 quoted lines
> 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/
Siddharth Kannan· Feb 9, 2017, 18:21 UTC · re: Matthieu Moy · lore

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

On 9 February 2017 at 17:55, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
Show 6 quoted lines
>
>> [...]
>> 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.

← back to recent threads