[PATCH v5 1/3] rebase: consistently use branch_name variable

[Kaartic Sivaraam]

The variable "branch_name" holds the  parameter in "git
rebase  ", but one codepath did not use it after
assigning $1 to it (instead it kept using $1).  Make it use the
variable consistently.

Also, update an error message to say there is no such branch or
commit, as we are expecting either of them, and not limiting
ourselves to a branch name.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 git-rebase.sh | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 60b70f3de..a299bcc52 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -518,7 +518,7 @@ case "$onto_name" in
 esac
 
 # If the branch to rebase is given, that is the branch we will rebase
-# $branch_name -- branch being rebased, or HEAD (already detached)
+# $branch_name -- branch/commit being rebased, or HEAD (already detached)
 # $orig_head -- commit object name of tip of the branch before rebasing
 # $head_name -- refs/heads/ or "detached HEAD"
 switch_to=
@@ -528,15 +528,18 @@ case "$#" in
branch_name="$1"
switch_to="$1"
 
-   if git show-ref --verify --quiet -- "refs/heads/$1" &&
-  orig_head=$(git rev-parse -q --verify "refs/heads/$1")
+   # Is it a local branch?
+   if git show-ref --verify --quiet -- "refs/heads/$branch_name" &&
+  orig_head=$(git rev-parse -q --verify "refs/heads/$branch_name")
then
-   head_name="refs/heads/$1"
-   elif orig_head=$(git rev-parse -q --verify "$1")
+   head_name="refs/heads/$branch_name"
+   # If not is it a valid ref (branch or commit)?
+   elif orig_head=$(git rev-parse -q --verify "$branch_name")
then
head_name="detached HEAD"
+
else
-   die "$(eval_gettext "fatal: no such branch: \$branch_name")"
+   die "$(eval_gettext "fatal: no such branch/commit: 
\$branch_name")"
fi
;;
 0)
@@ -547,7 +550,7 @@ case "$#" in
branch_name=$(expr "z$branch_name" : 'zrefs/heads/\(.*\)')
else
head_name="detached HEAD"
-   branch_name=HEAD ;# detached
+   branch_name=HEAD
fi
orig_head=$(git rev-parse --verify HEAD) || exit
;;
-- 
2.15.0.531.g2ccb3012c

[PATCH v5 0/3] rebase: give precise error messages

[Kaartic Sivaraam]

The tip of the v4 of this patch can be found at [1]. It was a revamp
sent by Junio mostly touching [PATCH v2 1/3] of the series. I've updated
it a little to add in something of my taste ;-)

There's only one concern that still bothers me a little. With the current
code you would see the following,

$ git rebase origin/maint 3013dff86
Current branch 3013dff86 is up to date.

That doesn't look good, does it? How about we overcome the issue of
handling this case and the HEAD case done in 3/3 by simplifying the
message as shown in the following diff,

diff --git a/git-rebase.sh b/git-rebase.sh
index 0f379ba2b..4d5400034 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -601,11 +601,11 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date.")"
+   say "$(eval_gettext "\$branch_name is up to date.")"
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date, rebase forced.")"
+   say "$(eval_gettext "\$branch_name is up to date, rebase 
forced.")"
fi
 fi

I guess this one is much better than 3/3 of this series as it handles
any kind of case by making no assumptions.

Thoughts ??


Note: In case you're wondering where's v3 of this series, there wasn't
a v3 series but there was a v3 PATCH of 3/3 [2].

References:

[1]: <xmqq1sjxt3tz@gitster.mtv.corp.google.com>

[2]: <20171201060935.19749-1-kaartic.sivar...@gmail.com>

Here's the interdiff between v4 and v5,

diff --git a/git-rebase.sh b/git-rebase.sh
index f3dd86443..fd72a35c6 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -518,7 +518,7 @@ case "$onto_name" in
 esac
 
 # If the branch to rebase is given, that is the branch we will rebase
-# $branch_name -- branch being rebased, or HEAD (already detached)
+# $branch_name -- branch/commit being rebased, or HEAD (already detached)
 # $orig_head -- commit object name of tip of the branch before rebasing
 # $head_name -- refs/heads/ or "detached HEAD"
 switch_to=
@@ -602,7 +602,7 @@ then
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
if test "$branch_name" = "HEAD" &&
-!(git symbolic-ref -q HEAD)
+! git symbolic-ref -q HEAD
then
say "$(eval_gettext "HEAD is up to date.")"
else
@@ -612,7 +612,7 @@ then
exit 0
else
if test "$branch_name" = "HEAD" &&
-    !(git symbolic-ref -q HEAD)
+! git symbolic-ref -q HEAD
then
say "$(eval_gettext "HEAD is up to date, rebase 
forced.")"
else



Kaartic Sivaraam (3):
  rebase: consistently use branch_name variable
  rebase: distinguish user input by quoting it
  rebase: rebasing can also be done when HEAD is detached

 git-rebase.sh | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

-- 
2.15.0.531.g2ccb3012c

[PATCH v5 0/1] clarify about @{-N} syntax in check-ref-format documentation

[Kaartic Sivaraam]

This was previously [PATCH v4 2/2] of this thread. It has now been
detached from 1/2 as it got merged to 'master'.

This patch applies on top of 'master'.

As this is almost a v2 of the v4 2/2 I'm comparing the changes with
v3 2/2 of the series. Apart from the changes shown by the below interdiff
the commit message got changed,

--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,20 +78,22 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name e.g. when creating a new branch
-(except for one exception related to the previous checkout syntax
-noted below). The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name (e.g. when creating a new
+branch). But be cautious when using the
+previous checkout syntax that may refer to a detached HEAD state.
+The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
 expanded for the ``previous checkout syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
-was checkout using "git checkout" operation. This option should be
+was checked out using "git checkout" operation. This option should be
 used by porcelains to accept this syntax anywhere a branch name is
 expected, so they can act as if you typed the branch name. As an
 exception note that, the ``previous checkout operation'' might result
-in a commit hash when the N-th last thing checked out was not a branch.
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 ---



Kaartic Sivaraam (1):
  Doc/check-ref-format: clarify information about @{-N} syntax

 Documentation/git-check-ref-format.txt | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

-- 
2.15.0.531.g2ccb3012c

[PATCH v5 1/1] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result may not be
the name of a branch that currently exists or ever existed. This
is because @{-N} is used to refer to the N-th last checked out
"thing", which might be a commit object name if the previous check
out was a detached HEAD state; or a branch name, otherwise. The
documentation thus does a wrong thing by promoting it as the
"previous branch syntax".

State that @{-N} is the syntax for specifying "N-th last thing
checked out" and also state that the result of using @{-N} might
also result in an commit object name.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..d9de99258 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -79,16 +79,21 @@ reference name expressions (see linkgit:gitrevisions[7]):
 
 With the `--branch` option, the command takes a name and checks if
 it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+branch). But be cautious when using the
+previous checkout syntax that may refer to a detached HEAD state.
+The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checked out using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 ---
@@ -116,7 +121,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c

Re: What's cooking in git.git (Dec 2017, #03; Wed, 13)

[Kaartic Sivaraam]

On Friday 15 December 2017 02:38 AM, Junio C Hamano wrote:

Junio C Hamano  writes:


I think you only sent 3/3 in newer rounds, which made it not to
apply to my tree.  If you meant to drop changes in 1/3 and 2/3,
perhaps because they were needless churn, then 3/3 needs to be
updated not to depend on the changes these two made.


Here is what I reconstructed to suit my taste better ;-)



Looks good to me except that your v4 3/3 seems to be spawning a subshell 
is an oversight, I guess? Also, I guess it would be better if there was 
comment in the code saying 'branch_nam' might hold a commit/branch name. 
No worries I'll send a v5 to that thread based on your v4.


Thanks,
Kaartic

Re: [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Thursday 14 December 2017 11:32 PM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:

Looks alright.

It was made unnecessarily harder to review because it was marked as
2/2, even though this no longer applies on top of the copy of 1/2
that was merged some time ago.


Sorry about that but I don't remember doing anything that made it not to 
apply on top of 1/2. (I just amended my changes to my topic branch. It 
can be found at [1])




I needed to find that it was rebased
on top of 'master';


I don't remember doing any rebase on top of 'master'. My topic was (and 
still is) based on the 'master' when it was pointing at 89ea799ff (Sync 
with maint, 2017-11-15). Anyways, it's my mistake as I didn't specify 
the branch on which I based this. Sorry about that.





Also re-wrapping the lines only to squeeze in "but be cautious..."
and replace s/branch/checkout/ in a few places did not help to make
it easy to spot what's changed.



I expected this would happen but I thought the line shouldn't grew too 
much so that they have to re-wrapped. Seems it would have been better if 
I did the re-wrapping as a follow-up commit (didn't strike me then).




This part looked a bit strange.


+it can be used as a valid branch name e.g. when creating a new branch
+(but be cautious when using the previous checkout syntax; it may refer
+to a detached HEAD state). The rule `git check-ref-format --branch


I think "e.g. when creating a new branch" is a parenthetical remark,
so it should be inside parenthesis.


It was. I brought them out to introduce the parenthetical warning. I'll 
send a v5 by putting the remark back inside parantheses and bringing the 
warning out. If it's not ok, let me know. I'll also try to do the 
re-wrapping as a separate cleanup patch.




 As the last three lines in the
new text (quoted above) already warns that it may not be a branch name,
I am not sure if the "but be cautious" adds much value, though.



That warning was for the impatient readers, who might want to find quick 
answers as to why they saw an odd behaviour (check-ref-froamt --branch 
not failing for a commit object name) (or) those who would want to use 
'check-ref-format --branch' but do not find time to read the whole 
paragraph.

Re: What's cooking in git.git (Dec 2017, #03; Wed, 13)

[Kaartic Sivaraam]

On Thursday 14 December 2017 10:50 PM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:


It seems my series that fixes an error message in 'git-rebase'[1]
(apart from a little cleanups) is missing. I guess I addressed the
issue that was raised in 3/3 as a v3 for that patch[2]. Are any more
changes needed?

[1]: <20171127172104.5796-1-kaartic.sivar...@gmail.com>
[2]: <20171201060935.19749-1-kaartic.sivar...@gmail.com>


I think you only sent 3/3 in newer rounds, which made it not to
apply to my tree.  


That's surprising, let me check.



If you meant to drop changes in 1/3 and 2/3,
perhaps because they were needless churn, then 3/3 needs to be
updated not to depend on the changes these two made.



2/3 is for sure not needless. But 1/3 might be. Though I think it 
improves the communicativity of the variable name. In case you think 
it's completely useless, then I can drop it as we won't lose much anyway ??



---
Kaartic

Re: What's cooking in git.git (Dec 2017, #03; Wed, 13)

[Kaartic Sivaraam]

It seems my series that fixes an error message in 'git-rebase'[1]
(apart from a little cleanups) is missing. I guess I addressed the
issue that was raised in 3/3 as a v3 for that patch[2]. Are any more
changes needed?

[1]: <20171127172104.5796-1-kaartic.sivar...@gmail.com>
[2]: <20171201060935.19749-1-kaartic.sivar...@gmail.com>


-- 
Kaartic

[PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result may not be
the name of a branch that currently exists or ever existed. This
is because @{-N} is used to refer to the N-th last checked out
"thing", which might be a commit object name if the previous check
out was a detached HEAD state; or a branch name, otherwise. The
documentation thus does a wrong thing by promoting it as the
"previous branch syntax".

State that @{-N} is the syntax for specifying "N-th last thing
checked out" and also state that the result of using @{-N} might
also result in an commit object name.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---

Changes in v4:

- updated the commit message
- made changes suggested by Junio

 Documentation/git-check-ref-format.txt | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..8172a6b9a 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,21 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
-may be stricter than what `git check-ref-format refs/heads/$name`
-says (e.g. a dash may appear at the beginning of a ref component,
-but it is explicitly forbidden at the beginning of a branch name).
-When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+it can be used as a valid branch name e.g. when creating a new branch
+(but be cautious when using the previous checkout syntax; it may refer
+to a detached HEAD state). The rule `git check-ref-format --branch
+$name` implements may be stricter than what `git check-ref-format
+refs/heads/$name` says (e.g. a dash may appear at the beginning of a
+ref component, but it is explicitly forbidden at the beginning of a
+branch name). When run with `--branch` option in a repository, the
+input is first expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checked out using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 ---
@@ -116,7 +120,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c

Re: [PATCH v5 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

On Friday 08 December 2017 04:44 AM, Junio C Hamano wrote:

Junio C Hamano  writes:


Somehow this fell underneath my radar horizon.  I see v4 and v5 of
4/4 but do not seem to find 1-3/4.  Is this meant to be a standalone
patch, or am I expected to already have 1-3 that we already are
committed to take?


Ah, I am guessing that this would apply on top of 1-3/4 in the
thread with <20171118172648.17918-1-kaartic.sivar...@gmail.com>



You guessed right; at the right time. I was about to ask why this got 
"out of your radar" in reply to your recent "What's cooking" email :-)




The base of the series seems to predate 16169285 ("Merge branch
'jc/branch-name-sanity'", 2017-11-28), so let me see how it looks by
applying those three plus this one on top of 'master' before that
point.



Let me know if this has terrible conflicts so that I can rebase the 
series on top of 'master'.



Thanks,
Kaartic

Re: [PATCH v5 2/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Thursday 07 December 2017 10:00 PM, Junio C Hamano wrote:

+
+   if (print_waiting_for_editor) {
+   /*
+* A dumb terminal cannot erase the line later on. Add a
+* newline to separate the hint from subsequent output.
+*




+* In case the editor emits further cruft after what
+* we wrote above, separate it from our message with SP.


I guess this part of the comment could be improved a little. I currently 
interpret it as "See if the editor emits further cruft, print a space in 
that case". Though, it's not what we are doing. Something like the 
following, perhaps?


 In a non-dumb terminal, separate our message from further cruft
 that might be emitted by the editor with SP.




+*/
+   const char term = is_terminal_dumb() ? '\n' : ' ';
+
+   fprintf(stderr,
+   _("hint: Waiting for your editor to close the 
file...%c"),
+   term);
+   fflush(stderr);
+   }
  
  		p.argv = args;

p.env = env;
@@ -63,6 +80,13 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+
+   if (print_waiting_for_editor && !is_terminal_dumb())
+   /*
+* Go back to the beginning and erase the entire line to
+* avoid wasting the vertical space.
+*/
+   fputs("\r\033[K", stderr);
}
  
  	if (!buffer)

Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:



NOTE: Though a commit-hash is a "syntactically" valid branch name,
it is generally not considered as one for the use cases of
"git check-ref-format --branch". That's because a user who does
"git check-ref-format --branch @{-$N}" would except the output
to be a "existing" branch name apart from it being syntactically
valid.


s/except/expect/ I suspect.


Correct suspicion.



 But I do not think this description is
correct.  "check-ref-format --branch @{-1}", when you come from the
detached HEAD state, happily report success so it does not hold that
it is "generally not considered".

Unless you are saying "check-ref-format --branch" is buggy, that is.


I was thinking it was "buggy" from v1 of this patch. The `--branch` 
option is expected to be used by porcelains but they are also wanted to 
be aware that the output might NOT be a branch name when the @{-N} 
syntax is used[1]. This sounds unintuitive given the name of the 
option(`--branch`). No user would expect anything but a branch name from 
such an option, I guess. At least, I don't. So, I thought clarifying the 
Doc was a good "first step" (the Doc guaranteed more than it should).




If so, we shouldn't be casting that buggy behaviour in stone by
documenting, but should be fixing it.



Yes. I wasn't sure how to go about fixing it and thus took the easier 
way of updating the Doc. I also think it would be a good way to trigger 
discussion. Now that the attention has come, any ideas about how it 
could be FIXED? Should we drop support for @{-N} option in 
check-branch-ref (highly backward incompatible)? Should we check if 
@{-$N} is a branch name and fail if it's not(not such a bad thing, I guess)?




And the paragraph that leads to this NOTE and this NOTE itself are
both misleading from that point of view.  The result *is* always a
valid branch name,
I wasn't referring to "syntactic validity" when I mentioned "valid" in 
the commit message. Though, I understand how I wasn't clear by not 
disambiguating.





Taking the above together, perhaps.

 When the N-th previous thing checked out syntax (@{-N}) is used
 with '--branch' option of check-ref-format the result may not be
 the name of a branch that currently exists or ever existed.
 This is because @{-N} is used to refer to the N-th last checked
 out "thing", which might be any commit (sometimes a branch) in
 the detached HEAD state.


I don't get the "... any in the detached HEAD state ..." part. I seem to 
interpret it as "@{-N}" always returns commits from the detached HEAD 
state. How about,


When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result may not be
the name of a branch that currently exists or ever existed. This
is because @{-N} is used to refer to the N-th last checked out
"thing", which might be a commit object name if the previous check
out was a detached HEAD state; or a branch name, otherwise. The
documentation thus does a wrong thing by promoting it as the
"previous branch syntax".




 State that @{-N} is the syntax for specifying "N-th last thing
 checked out" and also state that the result of using @{-N} might
 also result in an commit object name.



This one's nice.



diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
  . at-open-brace `@{` is used as a notation to access a reflog entry.
  
  With the `--branch` option, the command takes a name and checks if

-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements


And "except for" is also wrong here.  40-hex still *IS* a valid
branch name; it is just it may not be what we expect.  So perhaps
rephrase it to something like "(but be cautious when using the
previous checkout syntax that may refer to a detached HEAD state)".



Nice suggestion.



+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be


s/was checkout/was checked out/;



Good catch.



+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the bra

Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Tuesday 05 December 2017 12:14 AM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:


Stepping back a bit, the mild suspicion above says

 $ git checkout HEAD^0
 ... do things ...
 $ git checkout -b temp
 ... do more things ...
 $ git checkout -B @{-1}

that creates a new branch whose name is 40-hex of a commit that
happens to be where we started the whole dance *is* a bug.  No sane
user expects that to happen, and the last step "checkout -B @{-1}"
should result in an error instead [*1*].

I was wondering if "git check-ref-format --branch @{-1}", when used
in place of "checkout -B @{-1}" in the above sequence,


I guess you mean '... "git checkout -B $(git check-ref-format --branch
@{-1}", when used in place of "git checkout -B @{-1}" ...' ?


No you guessed wrong.  I was (and am) wondering if the last step in
the following sequence should fail.

 $ git checkout HEAD^0
 ... do things ...
 $ git checkout -b temp
 ... do more things ...
 $ git check-ref-format --branch @{-1}




Ok. Now I get what you say.



And I am leaning towards saying that it is a bug that it does not
fail; @{-1} is a detached HEAD and not a concrete branch name in
this case, 


It seems your thought is similar to the following thought that I 
expressed in [1],


-- 8< --


I thought this the other way round. Rather than letting the callers
error out when @{-N} didn't expand to a branch name, I thought we
should not be expanding @{-N} syntax for "check-ref-format --branch" at
all to make a "stronger guarantee" that the result is "always" a valid
branch name. Then I thought it might be too restrictive and didn't
mention it. So, I dunno.


-- >8 --




so "check-ref-format --branch" should at least notice
and say that it is a request that may lead to a nonsense next step
(which is to create a branch with that 40-hex name).



Makes sense, this should at least be noted in the Documentation. Is that 
what you had in mind too or do you expect 'check-ref-format' to do 
something else too?



[1]: https://public-inbox.org/git/1511880237.10193.5.ca...@gmail.com/

---
Kaartic

Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Sat, 2017-12-02 at 17:52 -0800, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > > I have a mild suspicion that "git checkout -B @{-1}" would want to
> > > error out instead of creating a valid new branch whose name is
> > > 40-hex that happen to be the name of the commit object you were
> > > detached at previously.
> > 
> > I thought this the other way round. Rather than letting the callers
> > error out when @{-N} didn't expand to a branch name, I thought we
> > should not be expanding @{-N} syntax for "check-ref-format --branch" at
> > all to make a "stronger guarantee" that the result is "always" a valid
> > branch name. Then I thought it might be too restrictive and didn't
> > mention it. So, I dunno.
> > 

OK. Seems I was out of mind in the above reply. I have answered the
question about whether "branch=$(git check-ref-format @{-1}) && git
checkout -B $branch" should fail or not. Sorry, for the confusion in
case you mind it.


> > 
> > > I am not sure if "check-ref-format --branch" should the same; it is
> > > more about the syntax and the 40-hex _is_ valid there, so...
> > 
> > I'm not sure what you were trying to say here, sorry.
> 
> The "am not sure if" comes from this question: should these two be
> equivalent?
> 
> $ git check-ref-format --branch @{-1}
> $ git check-ref-format --branch $(git rev-parse --verify @{-1})
> 

I could see how,

$ git rev-parse --verify @{-1}
$ git rev-parse --verify $(git check-ref-format --branch @{-1})

could be equivalent. I'm not sure how the previous two might be
equivalent except in the case when the previous thing checked out was
not a branch.

1. "git check-ref-format --branch @{-1}" returns the previous thing
checked out which might be a branch name or a commit hash

2. "git check-ref-format --branch $(git rev-parse --verify @{-1})"
returns the commit hash of the previous thing (the hash of the tip if
was a branch). IIUC, this thing never returns a branch name.


> If they should be equivalent (and I think the current implementation
> says they are), then the answer to "if ... should do the same?"
> becomes "no, we should not error out".
> 
> Stepping back a bit, the mild suspicion above says
> 
> $ git checkout HEAD^0
> ... do things ...
> $ git checkout -b temp
> ... do more things ...
> $ git checkout -B @{-1}
> 
> that creates a new branch whose name is 40-hex of a commit that
> happens to be where we started the whole dance *is* a bug.  No sane
> user expects that to happen, and the last step "checkout -B @{-1}"
> should result in an error instead [*1*].
> 
> I was wondering if "git check-ref-format --branch @{-1}", when used
> in place of "checkout -B @{-1}" in the above sequence,

I guess you mean '... "git checkout -B $(git check-ref-format --branch
@{-1}", when used in place of "git checkout -B @{-1}" ...' ?


>  should or
> should not fail.  It really boils down to this question: what @{-1}
> expands to and what the user wants to do with it.
> 
> In the context of getting a revision (i.e. "rev-parse @{-1}") where
> we are asking what the object name is, the value of the detached
> HEAD we were on previously is a valid answer we are "expanding @{-1}
> to".  If we were on a concrete branch and @{-1} yields a concrete
> branch name, then rev-parse would turn that into an object name, and
> in the end, in either case, the object name is what we wanted to
> get.  So we do not want to error this out.
> 

Right.


> But a user of "git check-ref-format --branch" is not asking about
> the object name ("git rev-parse" would have been used otherwise).
> Which argues for erroring out "check-ref-format --branch @{-1}" if
> we were not on a branch in the previous state.
> 

Exactly what I thought.


> And that argues for erroring out "check-ref-format --branch @{-1}"
> in such a case, i.e. declaring that the first two commands in this
> message are not equivalent.
> 

As said before, IIUC, they give equivalent output to stdout only when
the previous thing checked out was not a branch.


-- 
Kaartic

Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Sun, 2017-12-03 at 17:39 +0100, Lars Schneider wrote:
> > On 02 Dec 2017, at 04:45, Kaartic Sivaraam <kaartic.sivar...@gmail.com> 
> > wrote:
> > 
> > On Friday 01 December 2017 11:59 PM, Jeff King wrote:
> > > On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote:
> > > > 
> > > > Thanks for the review :-)
> > > 
> > > Actually, I meant to bikeshed one part but forgot. ;)
> > > > +   fprintf(stderr, _("hint: Waiting for your 
> > > > editor input..."));
> > > 
> > > I found "waiting for editor input" to be a funny way of saying this. I
> > > input to the editor, the editor does not input to Git. :)
> > > Maybe "waiting for your editor finish" or something would make more
> > > sense?
> > 
> > May be the good "Launched editor. Waiting ..." message, that was used in a 
> > previous version, itself makes sense?
> 
> Perfect bikeshed topic :-)
> 

Yep :-)


> I would like to add "for your input" or "for you" to convey 
> that Git is not waiting for the machine but for the user.
> 
> "hint: Launched editor. Waiting for your input..."
> 
> Would that work for you?
> 

Yeah, this one does look fine.

That said, FWIW I don't have strong opinions about the phrase/sentence
except that they should be readable and shouldn't get too verbose.


Thanks, 
Kaartic

Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Friday 01 December 2017 11:59 PM, Jeff King wrote:

On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote:


Thanks for the review :-)


Actually, I meant to bikeshed one part but forgot. ;)


+   fprintf(stderr, _("hint: Waiting for your editor 
input..."));


I found "waiting for editor input" to be a funny way of saying this. I
input to the editor, the editor does not input to Git. :)

Maybe "waiting for your editor finish" or something would make more
sense?


May be the good "Launched editor. Waiting ..." message, that was used in 
a previous version, itself makes sense?





Or given that the goal is really just making it clear that we've spawned
an editor, something like "starting editor %s..." would work.


There was already discussion related to the "continuous tense" used in 
the phrase.


Extract from [1]:

-- 8< --
> fprintf(stderr, "Launching your editor...");

"It takes quite some time to launch this special Git Editor"

As Lars pointed out, the editor may be launched in the background,
that the user would not know, but they might expect a thing to
pop up as a modal dialog as is always with UIs.

So despite it being technically wrong at this point in time,
I would phrase it in past tense or in a way that indicates that the
user needs to take action already.

The "Launching..." sounds as if I need to wait for an event to occur.
-- >8 --

[1]: 
https://public-inbox.org/git/CAGZ79kZbm8SGY4rXKZHV82E-HX9qbQ4iyCbMgJEBFQf4fj3u=q...@mail.gmail.com/




I think
the "waiting for..." pattern is perfectly fine, though.

[PATCH v3 3/3] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

Attempting to rebase when the HEAD is detached and is already
up to date with upstream (so there's nothing to do), the
following message is shown

Current branch HEAD is up to date.

which is clearly wrong as HEAD is not a branch.

Handle the special case of HEAD correctly to give a more precise
error message.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---

Changes in v2:

- avoided unnecesarily spawning a subshell in a conditional


 git-rebase.sh | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 3f8d99e99..1886167e0 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -602,11 +602,23 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date.")"
+   if test "$branch_or_commit" = "HEAD" &&
+! git symbolic-ref -q HEAD
+   then
+   say "$(eval_gettext "HEAD is up to date.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date.")"
+   fi
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date, rebase forced.")"
+   if test "$branch_or_commit" = "HEAD" &&
+! git symbolic-ref -q HEAD
+   then
+   say "$(eval_gettext "HEAD is up to date, rebase 
forced.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date, rebase forced.")"
+   fi
fi
 fi
 
-- 
2.15.0.531.g2ccb3012c

[PATCH v5 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
Sorry, missed a ';' in v4.

The surprising thing I discovered in the TravisCI build for v4
was that apart from the 'Documentation' build the 'Static Analysis'
build passed, with the following output,

-- 
$ ci/run-static-analysis.sh
GIT_VERSION = 2.13.1.1972.g6ced3f745
 SPATCH contrib/coccinelle/array.cocci
 SPATCH result: contrib/coccinelle/array.cocci.patch
 SPATCH contrib/coccinelle/free.cocci
 SPATCH contrib/coccinelle/object_id.cocci
 SPATCH contrib/coccinelle/qsort.cocci
 SPATCH contrib/coccinelle/strbuf.cocci
 SPATCH result: contrib/coccinelle/strbuf.cocci.patch
 SPATCH contrib/coccinelle/swap.cocci
 SPATCH contrib/coccinelle/xstrdup_or_null.cocci

The command "ci/run-static-analysis.sh" exited with 0.
--  --

+Cc: SZEDER
I guess static analysis tools make an assumption that the source
code is syntactically valid for them to work correctly. So, I guess
we should at least make sure the code 'compiles' before running
the static analysis tool even though we don't build it completely.
I'm not sure if it's a bad thing to run the static analysis on code
that isn't syntactically valid, though.


 builtin/branch.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..196d5fe9b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *interpreted_oldname = NULL;
+   const char *interpreted_newname = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,6 +495,11 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   if (!skip_prefix(oldref.buf, "refs/heads/", _oldname) ||
+   !skip_prefix(newref.buf, "refs/heads/", _newname)) {
+   die("BUG: expected prefix missing for refs");
+   }
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
@@ -508,10 +515,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   interpreted_oldname);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   interpreted_oldname);
}
 
if (!copy &&
@@ -520,9 +527,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_oldname);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_newname);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.531.g2ccb3012c

Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Friday 01 December 2017 02:21 AM, Jeff King wrote:


These are obviously the result of devils-advocate poking at the feature.
I doubt any editor would end its output with a CR. But the first case is
probably going to be common, especially for actual graphical editors. We
know that emacsclient prints its own line, and I wouldn't be surprised
if other graphical editors spew some telemetry to stderr (certainly
anything built against GTK tends to do so).



Yeah, at times 'gedit' does do what you say. And if the user 
(surprisingly!) uses an IDE such as "eclipse" or a hackable text editor 
"atom" (of course with the '-f' option) for entering his commit message 
it is likely to happen all the time for him.




I don't think there's a good way around it. Portably saying "delete
_this_ line that I wrote earlier" would probably require libcurses or
similar. So maybe we just live with it. The deletion magic makes the
common cases better (a terminal editor that doesn't print random
lines, or a graphical editor that is quiet), and everyone else can flip
the advice switch if they need to. I dunno.



---
Kaartic

Re: [PATCH v4 0/2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Thu, 2017-11-30 at 16:13 +0100, Andreas Schwab wrote:
> On Nov 30 2017, Thomas Adam  wrote:
> 
> > On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
> > > 
> > > > On 29 Nov 2017, at 19:35, Thomas Adam  wrote:
> > > > 
> > > > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schnei...@autodesk.com 
> > > > wrote:
> > > > > + if (print_waiting_for_editor) {
> > > > > + fprintf(stderr, _("hint: Waiting for your 
> > > > > editor input..."));
> > > > >   fflush(stderr);
> > > > 
> > > > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > > > although the call to fflush() is harmless, I suspect it's not having any
> > > > effect.  That said, there's plenty of other places in Git which seems 
> > > > to think
> > > > fflush()ing stderr actually does something.
> > > 
> > > I agree with the "unbuffered" statement. I am surprised that you expect 
> > > fflush()
> > > to do nothing in that situation... but I am no expert in that area. Can 
> > > you
> > > point me to some documentation?
> > 
> > Because stderr is unbuffered, it will get printed immediately.
> 
> POSIX only requires stderr to be "not fully buffered".  If it is line
> buffered, the message may not appear immediately.
> 

I guess Junio's reply for the same "unbuffered" question I asked for an
earlier version of this patch (now, a series) might be relevant here,

> > Being curious again, is flushing 'stderr' required ? AFAIK, 'stderr'
> > is unbuffered by default and I didn't notice any calls that changed
> > the buffering mode of it along this code path.
> 
> "By default" is the key phrase.  The code is merely being defensive
> to changes in other area of the code.

cf. 


-- 
Kaartic

Re: [PATCH v4 1/2] refactor "dumb" terminal determination

[Kaartic Sivaraam]

On Wednesday 29 November 2017 08:07 PM, lars.schnei...@autodesk.com wrote:

+int is_terminal_dumb(void)
+{
+   const char *terminal = getenv("TERM");
+   return !terminal || !strcmp(terminal, "dumb");


So, IIUC, there terminal is considered to be 'dumb' when the TERM 
environment variable is NOT set or when it is set to 'dumb'.



+}
+
  const char *git_editor(void)
  {
const char *editor = getenv("GIT_EDITOR");
-   const char *terminal = getenv("TERM");
-   int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
+   int terminal_is_dumb = is_terminal_dumb();
  
  	if (!editor && editor_program)

editor = editor_program;
diff --git a/sideband.c b/sideband.c
index 1e4d684d6c..6d7f943e43 100644
--- a/sideband.c
+++ b/sideband.c
@@ -20,13 +20,12 @@
  
  int recv_sideband(const char *me, int in_stream, int out)

  {
-   const char *term, *suffix;
+   const char *suffix;
char buf[LARGE_PACKET_MAX + 1];
struct strbuf outbuf = STRBUF_INIT;
int retval = 0;
  
-	term = getenv("TERM");

-   if (isatty(2) && term && strcmp(term, "dumb"))
+   if (isatty(2) && !is_terminal_dumb())
suffix = ANSI_SUFFIX;
else
suffix = DUMB_SUFFIX;



This one looks good to me if my observation above is correct.

---
Kaartic

[PATCH v4 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 v2 and v3 of this patch got detached to a different thread due
 to a small mistake I made, sorry. You can find it at

 https://public-inbox.org/git/20171121141852.551-1-kaartic.sivar...@gmail.com/

 I've brought v4 back to this thread to bring back the continuity. I guess
 this series gets stabilised with this patch.

 Changes in v4 from v1:

  - updated commit message

  - removed superfluous comment

  - updated variable names

  - checked for the errors pointed out by skip_prefix

 builtin/branch.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..b2f5e4a0c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *interpreted_oldname = NULL;
+   const char *interpreted_newname = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,6 +495,11 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   if (!skip_prefix(oldref.buf, "refs/heads/", _oldname) ||
+   !skip_prefix(newref.buf, "refs/heads/", _newname)) {
+   die("BUG: expected prefix missing for refs")
+   }
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
@@ -508,10 +515,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   interpreted_oldname);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   interpreted_oldname);
}
 
if (!copy &&
@@ -520,9 +527,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_oldname);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_newname);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.531.g2ccb3012c

Re: [PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

On Wed, 2017-11-29 at 09:10 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > If  is the correct substitute , I could try to send a
> > patch that fixes this.
> 
> I do not think the above is a good change in the first place for at
> least two reasons.  By saying , the updated text says "not just
> branches but you can also give tags and remote-tracking branches".

I used  as you could actually use tags, remote-tracking branches
and even "notes" (i.e.) any kind of  for the  part. That's
how the code for rebasing works currently (specifically the elif part),

if git show-ref --verify --quiet -- "refs/heads/$1" &&
   orig_head=$(git rev-parse -q --verify "refs/heads/$1")
then
head_name="refs/heads/$1"
elif orig_head=$(git rev-parse -q --verify "$1")
then
head_name="detached HEAD"
else
die "$(eval_gettext "fatal: no such branch: \$branch_name")"
fi

Which means that you could not only do,

$ git rebase origin/next refs/remotes/origin/maint

You could even get creative and do,

$ git rebase origin/next refs/notes/$something

It just works (of course, I couldn't understand what it did)! I didn't
want to lie to the user. So, I used . So, should we just update
the  part of the doc or should we update the code for 'rebase'?
I'm unsure.

> In reality, however, it can take any commit-ish, as the "no we are
> not rebasing the current checkout" form of the command is merely a
> short-hand to check it out first [*1*].  It gives appearance that
> the change is making it more broad, but not making it broad enough.
> At the same time, more than 90% of the time, people give a branch
> name there.  Saying "branch-or-commit" for a short description of
> the command line (which is what synopsis section is) does not add
> that much value.  The body text of the description where we refer to
> the  parameter of course need to be updated to say "in
> addition, instead of a branch name, you can give a commit-ish that
> is *not* a branch name.  When you do so, instead of checking out the
> branch, the HEAD is detached at that commit before the history
> leading to it is rebased."
> 
> And because we have to say "it can be a non-branch commit-ish and
> here is what happens when you do so" anyway, I think the current
> synopsis as-is would be better than making it less clear and more
> scary by replacing  with other things like  or
> [ | ].
> 
> 
> [Footnote]
> 
> *1* As my "log --first-parent --oneline master..pu" is a strand of
> pearls each of which is a merge of a topic branch,
> 
> $ git rebase -i master pu~$n^2
> 
> can be a handy way to make a trial rebase (after double checking
> the result of "git tbdiff", I may do "git checkout -B topic" to
> overwrite the original or I may discard the result of rebasing).

Re: Feature request: Reduce amount of diff in patch

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 18:09 +0200, KES wrote:
> Hi.
> 
> I get often patches which can be minimized:
> 

I guess the one below can't be (see below).


> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
> -]);
>  
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
> +]);
>  
>  
>  return $users;
> 
> This patch can be minimized to:
> 
> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
>  ]);
>  
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
> 
>  
>  return $users;
> 
> May you please fix the git to generate minimized patches?
> 

You seemed to have overlooked the empty line above the ']);' in the
original patch. So, your minimized version isn't actually equivalent to
the original one. Further, when trying to recreate your patch I get the
following,

diff --git a/diff-test b/diff-test
index 1d5dc1b..f3ec38f 100644
--- a/diff-test
+++ b/diff-test
@@ -1,10 +1,8 @@
 address=>  { -like => \[ '?',  "%$search%" ] },
 company=>  { -like => \[ '?',  "%$search%" ] },
 country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
+
 ]);
 
-$users =  $users->search( $filter, {
-prefetch => { Packages => { Ips => { Subnet => { Server => 'Locality' 
,
-});
 
 return $users;

I use git built from the source (2.15.0.531.g2ccb3012c)

-- 
Kaartic

Re: [PATCH v2 3/3] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:31 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > +   if test "$branch_or_commit" = "HEAD" &&
> > +!(git symbolic-ref -q HEAD)
> 
> Did you need a subshell here? 

No. That's a consequence of me not remembering that I would span a sub-
shell with a simple '()' when I was doing that part. (partial
transition from C to shell)


>  Now with a proper test with
> "symbolic-ref -q HEAD", I wonder if you'd need to check if the
> original was named HEAD in the first place (I do not feel strongly
> enough to say that checking is wrong, at least not yet, but the
> above does make me wonder), and instead something like
> 
>   if ! git symbolic-ref -q HEAD
>   then
>   ...
> 
> might be sufficient.  I dunno.
> 

It does  seem you're right. The only thing we would be losing is the
short-circuiting when $branch_or_commit is not HEAD (which I suspect to
be the case most of the time). So, I'm not sure if I should remove the
check (of course, I'll change the check to avoid spawning a sub-shell).


Thanks, 
Kaartic

Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

On Tuesday 28 November 2017 09:34 AM, Junio C Hamano wrote:

Eric Sunshine  writes:


With this approach, validate_worktree() will print an error message
saying that the worktree directory is missing before the control info
is actually removed. Kaartic's original patch silenced the message
(and _all_ error messages from validate_worktree()) by passing 1 as
the second argument. That seemed heavy-handed, so I suggested keeping
the validate_worktree() call as-is but doing the directory-missing
check first as a special case.

But perhaps that special case isn't necessary.


I do not think the user minds to see "there is no such directory
there"; actually that would be beneficial, even.

But you are right; validate_worktree() would need to become aware of
the possibility that it can be passed such a "corrupt" worktree to
handle if that approach is followed.

The case we are discussing, i.e. the user removed the directory
without telling Git to give it a chance to remove control
information, may be common enough that it becomes a worthwhile
addition to make the "quiet" boolean that occupies a whole int to an
unsigned that is a collection of bits, and pass "missing_ok" bit in
that flag word to the validate_worktree() to tell that the caller
can tolerate the "user removed it while we were looking the other
way" case and can handle it gracefully.  But that would be a lot
larger change, and "the caller checks before calling validate" as
done with this [RFC v2] may be a good way to add the feature with
the least impact to the codebase.


I had envisioned a simple 'goto remove_control_info' rather than extra
nesting or refactoring, but that's a minor point.


Yes, use of goto is also a good way to avoid having to worry about
the future evolution of the codeflow in this function.

So perhaps

if (the directory is no longer there)
goto cleanup;
if (the worktree does not validate)
return -1;
... the original code to (carefully) remove the
... worktree itself

cleanup:

... remove control info
... free resources held in variables
... return from the function

is what we want?



Probably but I'm not interested in going for a v3 that does that as I 
just wanted to show that worktree remove could be enhanced in this 
aspect and show how it could be done. So, I'll leave this in the 
#leftoverbits for the person who would be re-rolling nd/worktree-move.


---
Kaartic

[PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result might not
always be a valid branch name (see NOTE below). This is because
@{-N} is used to refer to the N-th last checked out "thing" which
might be any commit (sometimes a branch). The documentation thus
does a wrong thing by promoting it as the "previous branch syntax".

So, correctly state @{-N} is the syntax for specifying "N-th last
thing checked out" and also state that the result of using @{-N}
might also result in a "commit hash".

NOTE: Though a commit-hash is a "syntactically" valid branch name,
it is generally not considered as one for the use cases of
"git check-ref-format --branch". That's because a user who does
"git check-ref-format --branch @{-$N}" would except the output
to be a "existing" branch name apart from it being syntactically
valid.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit hash when the N-th last thing checked out was not a branch.
 
 OPTIONS
 ---
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c

Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:40 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > When the N-th previous thing checked out sytax is used with
> > '--branch' option of check-ref-format the results might not
> > always be a valid branch name
> 
> I wonder if you want to rephrase this, because 40-hex object name is
> syntactically a valid branch name.  It's (1) cumbersome to type and
> (2) may not be what the user expects.
> 

You're right. Actually a previous draft of that log message did say
something like,

Though a commit-hash might be a valid branch name, it isn't
something that's expected by the users of "check-ref-format".

I removed as I thought it would be unnecessary. It seems I took the
wrong decision. Will fix it. :-)

> I have a mild suspicion that "git checkout -B @{-1}" would want to
> error out instead of creating a valid new branch whose name is
> 40-hex that happen to be the name of the commit object you were
> detached at previously.
> 

I thought this the other way round. Rather than letting the callers
error out when @{-N} didn't expand to a branch name, I thought we
should not be expanding @{-N} syntax for "check-ref-format --branch" at
all to make a "stronger guarantee" that the result is "always" a valid
branch name. Then I thought it might be too restrictive and didn't
mention it. So, I dunno.


> I am not sure if "check-ref-format --branch" should the same; it is
> more about the syntax and the 40-hex _is_ valid there, so...

I'm not sure what you were trying to say here, sorry.


-- 
Kaartic

Re: [PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:25 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > 1. "git rebase  " does nothing
> 
> Not limited to "rebase", you do not muck with remote-tracking branch
> in your local repository, so it would be a bug if the above updated
> where the remote-tracking branch points at.
> 
> The form of "git rebase" with one extra argument (i.e. not rebasing
> the history that leads to the current checkout) is mere shorthand of
> checking that extra thing out before doing the rebase, i.e.
> 
>   $ git rebase origin/next origin/maint
> 
> first checks out origin/maint (you'd get on a detached HEAD) and
> rebase the history leading to the detached HEAD on top of
> origin/next.  If it fast-forwards (and it should if you are talking
> about 'maint' and 'next' I publish), you'll end up sitting on a
> detached HEAD that points at origin/next.
> 
> There is nothing to see here.
> 

You're right. It was my mistake. It seems I didn't notice that I was
already on 'origin/next' before I did,

    $ git rebase origin/next origin/maint

So (obviously) I thought it did nothing, sorry.


> > 2. It's possible to do "git rebase  "
> 
> Again, that's designed behaviour you can trigger by giving 
> (not ).  Very handy when you do not trust your upstream or
> yourself's ability to resolve potential conflicts as a trial run
> before really committing to perform the rebase, e.g.
> 
>   $ git rebase origin master^0
> 

I can't comment about usefulness as I haven't used rebase in this way
but I'm pretty sure that this should be mentioned in the
"Documentation" to help those might be in bare need of syntax like this
to discover it.

Something like the following diff with additional changes to other
places that refer to ,

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e688..ba4a545bf 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -9,9 +9,9 @@ SYNOPSIS
 
 [verse]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
-   [ []]
+   [ []]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
-   --root []
+   --root []
 'git rebase' --continue | --skip | --abort | --quit | --edit-todo
 
 DESCRIPTION


If  is the correct substitute , I could try to send a
patch that fixes this.


-- 
Kaartic

[RFC PATCH v2] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

"git worktree remove" removes both the named worktree
directory and the administrative information for it after
checking that there is no local modifications that would be
lost (which is a handy safety measure). However, due to a
possible oversight, it aborts with an error if the worktree
directory is _already_ removed.

The user could use "git worktree prune" after seeing the
error and realizing the situation, but at that point, there
is nothing gained by leaving only the administrative data
behind. Teach "git worktree remove" to go ahead and remove
the trace of the worktree in such a case.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Changes in v2:

  - incorporated the suggestion to avoid quieting `validate_worktree()`
to detect inexistent directory (thanks, Eric!)

  - used the suggested (much better) commit message

 builtin/worktree.c | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b5afba164..6eab91889 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -605,6 +605,23 @@ static int move_worktree(int ac, const char **av, const 
char *prefix)
return update_worktree_location(wt, dst.buf);
 }
 
+/* Removes the .git/worktrees/worktree_id directory for
+ * the given worktree_id
+ *
+ * Returns 0 on success and non-zero value in case of failure
+ */
+static int remove_worktree_entry(char *worktree_id) {
+   int ret = 0;
+   struct strbuf we_path = STRBUF_INIT;
+   strbuf_addstr(_path, git_common_path("worktrees/%s", worktree_id));
+   if (remove_dir_recursively(_path, 0)) {
+   error_errno(_("failed to delete '%s'"), we_path.buf);
+   ret = -1;
+   }
+   strbuf_release(_path);
+   return ret;
+}
+
 static int remove_worktree(int ac, const char **av, const char *prefix)
 {
int force = 0;
@@ -634,6 +651,16 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
die(_("already locked, reason: %s"), reason);
die(_("already locked, no reason"));
}
+
+   if (!file_exists(wt->path)) {
+   /* There's a worktree entry but the worktree directory
+* doesn't exist. So, just remove the worktree entry.
+*/
+   ret = remove_worktree_entry(wt->id);
+   free_worktrees(worktrees);
+   return ret;
+   }
+
if (validate_worktree(wt, 0))
return -1;
 
@@ -670,13 +697,7 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
error_errno(_("failed to delete '%s'"), sb.buf);
ret = -1;
}
-   strbuf_reset();
-   strbuf_addstr(, git_common_path("worktrees/%s", wt->id));
-   if (remove_dir_recursively(, 0)) {
-   error_errno(_("failed to delete '%s'"), sb.buf);
-   ret = -1;
-   }
-   strbuf_release();
+   ret = remove_worktree_entry(wt->id);
free_worktrees(worktrees);
return ret;
 }
-- 
2.15.0.345.gf926f18f3

[PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out sytax is used with
'--branch' option of check-ref-format the results might not
always be a valid branch name as @{-N} is used to refer to
the N-th last checked out "thing" which might be any commit
(sometimes a branch). The documentation thus does a wrong thing
by promoting it as the "previous branch syntax".

So, correctly state @{-N} is the syntax for specifying "N-th last
thing checked out" and also state that the result of using @{-N}
might also result in a "commit hash".

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit hash when the N-th last thing checked out was not a branch.
 
 OPTIONS
 ---
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.345.gf926f18f3

[PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state

[Kaartic Sivaraam]

@{-N} is a syntax for the N-th last "checkout" and not the N-th
last "branch". Therefore, in some cases using `git checkout @{-$N}`
DOES lead to a "detached HEAD" state. This can also be ensured by
the commit message of 75d6e552a (Documentation: @{-N} can refer to
a commit, 2014-01-19) which clearly specifies how @{-N} can be used
to refer not only to a branch but also to a commit.

Correct the misleading sentence which states that @{-N} doesn't
detach HEAD.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Documentation/git-checkout.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e108b0f74..d5a57ac90 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -272,11 +272,11 @@ section of linkgit:git-add[1] to learn how to operate the 
`--patch` mode.
commit, your HEAD becomes "detached" and you are no longer on
any branch (see below for details).
 +
-As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
-checks out branches (instead of detaching).  You may also specify
-`-` which is synonymous with `"@{-1}"`.
+You can use the `"@{-N}"` syntax to refer to the N-th last
+branch/commit checked out using "git checkout" operation. You may
+also specify `-` which is synonymous to `"@{-1}`.
 +
-As a further special case, you may use `"A...B"` as a shortcut for the
+As a special case, you may use `"A...B"` as a shortcut for the
 merge base of `A` and `B` if there is exactly one merge base. You can
 leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
-- 
2.15.0.345.gf926f18f3

[PATCH v2 2/3] rebase: distinguish user input by quoting it

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index a675cf691..3f8d99e99 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -477,7 +477,7 @@ then
;;
esac
upstream=$(peel_committish "${upstream_name}") ||
-   die "$(eval_gettext "invalid upstream \$upstream_name")"
+   die "$(eval_gettext "invalid upstream '\$upstream_name'")"
upstream_arg="$upstream_name"
 else
if test -z "$onto"
@@ -540,7 +540,7 @@ case "$#" in
head_name="detached HEAD"
 
else
-   die "$(eval_gettext "fatal: no such branch/commit: 
\$branch_or_commit")"
+   die "$(eval_gettext "fatal: no such branch/commit 
'\$branch_or_commit'")"
fi
;;
 0)
-- 
2.15.0.345.gf926f18f3

[PATCH v2 3/3] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

Attempting to rebase when the HEAD is detached and is already
up to date with upstream (so there's nothing to do), the
following message is shown

Current branch HEAD is up to date.

which is clearly wrong as HEAD is not a branch.

Handle the special case of HEAD correctly to give a more precise
error message.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---

 git-rebase.sh | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 3f8d99e99..9cce1483a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -602,11 +602,23 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date.")"
+   if test "$branch_or_commit" = "HEAD" &&
+!(git symbolic-ref -q HEAD)
+   then
+   say "$(eval_gettext "HEAD is up to date.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date.")"
+   fi
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date, rebase forced.")"
+   if test "$branch_or_commit" = "HEAD" &&
+!(git symbolic-ref -q HEAD)
+   then
+   say "$(eval_gettext "HEAD is up to date, rebase 
forced.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_or_commit 
is up to date, rebase forced.")"
+   fi
fi
 fi
 
-- 
2.15.0.345.gf926f18f3

[PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

Junio C Hamano <gits...@pobox.com> writes:
> Perhaps time to learn "git symbolic-ref HEAD" and use it instead of
> depending on the name?

Good point. Helped remove the assumption that there's no branch named HEAD.
(and indirectly led to 2 additional patches and the 3 questions found below ;-)


This started as a small fix to make 'git rebase' give a precise
error message when a rebase was done with a detached HEAD. Now it
includes a few cleanups that I caught while analysing the code.

There were a few weird observations when I was anlaysing the code.
They are listed below. Please give your thoughts about them.

The commands I use below were run on my local clone of 'git' and 'origin'
in this context refers to the git mirror at GitHub.

1. "git rebase  " does nothing

I tried to play around with rebase by trying out various combinations while
analysing and found the following to have not effect even though the output
doesn't say anything about that,

$ git rebase origin/next origin/maint 
First, rewinding head to replay your work on top of it...
Fast-forwarded origin/maint to origin/next.

IOW, updating a remote branch with a remote upstream had no effect.
Though trying to update a remote branch with a remote upstream doesn't
seem to be very meaningful, the output says it HAS updated the remote
which seems to be misleading. What should be done about this?

2. It's possible to do "git rebase  "

$ git origin/next f926f18f3dda0c52e794b2de0911f1b046c7dadf"

This checks out the commit(detaches HEAD) tries to rebase origin/next
from there.

This behaviour doesn't seems to be documented. It says that only a 'branch'
can be specified. (The error message updated in 1/3 previously reported that
the 'branch' name is invalid rather than stating the 'ref (branch/commit) is
invlid')

 git rebase [...] [ []]
 git rebase [...] --root []
 ...

Shouldn't it have said that we can give any  apart from  instead of
saying we could give only a . If intentional, why?

3. "git rebase  " shows misleading message

$ git origin/next f926f18f3dda0c52e794b2de0911f1b046c7dadf"
Current branch f926f18f3dda0c52e794b2de0911f1b046c7dadf is up to date.

As it's clear the commit is not a branch. What should be done to fix this?


Kaartic Sivaraam (3):
  rebase: use a more appropriate variable name
  rebase: distinguish user input by quoting it
  rebase: rebasing can also be done when HEAD is detached

 git-rebase.sh | 46 +++---
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.15.0.345.gf926f18f3

[PATCH v2 1/3] rebase: use a more appropriate variable name

[Kaartic Sivaraam]

The variable name "branch_name" used to indicate the  parameter
in "git rebase  " is misleading as it seems to indicate
that it holds only a "branch name" although at times it might actually hold
any valid  (branch/commit).

So, use a more suitable name for that variable.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 git-rebase.sh | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6344e8d5e..a675cf691 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -518,36 +518,40 @@ case "$onto_name" in
 esac
 
 # If the branch to rebase is given, that is the branch we will rebase
-# $branch_name -- branch being rebased, or HEAD (already detached)
+# $branch_or_commit -- branch/commit being rebased, or HEAD (already detached)
 # $orig_head -- commit object name of tip of the branch before rebasing
 # $head_name -- refs/heads/ or "detached HEAD"
 switch_to=
 case "$#" in
 1)
# Is it "rebase other $branchname" or "rebase other $commit"?
-   branch_name="$1"
+   branch_or_commit="$1"
switch_to="$1"
 
-   if git show-ref --verify --quiet -- "refs/heads/$1" &&
-  orig_head=$(git rev-parse -q --verify "refs/heads/$1")
+   # Is it a local branch?
+   if git show-ref --verify --quiet -- "refs/heads/$branch_or_commit" &&
+  orig_head=$(git rev-parse -q --verify "refs/heads/$branch_or_commit")
then
-   head_name="refs/heads/$1"
-   elif orig_head=$(git rev-parse -q --verify "$1")
+   head_name="refs/heads/$branch_or_commit"
+
+   # If not is it a valid ref (branch or commit)?
+   elif orig_head=$(git rev-parse -q --verify "$branch_or_commit")
then
head_name="detached HEAD"
+
else
-   die "$(eval_gettext "fatal: no such branch: \$branch_name")"
+   die "$(eval_gettext "fatal: no such branch/commit: 
\$branch_or_commit")"
fi
;;
 0)
# Do not need to switch branches, we are already on it.
-   if branch_name=$(git symbolic-ref -q HEAD)
+   if branch_or_commit=$(git symbolic-ref -q HEAD)
then
-   head_name=$branch_name
-   branch_name=$(expr "z$branch_name" : 'zrefs/heads/\(.*\)')
+   head_name=$branch_or_commit
+   branch_or_commit=$(expr "z$branch_or_commit" : 
'zrefs/heads/\(.*\)')
else
head_name="detached HEAD"
-   branch_name=HEAD ;# detached
+   branch_or_commit="HEAD"
fi
orig_head=$(git rev-parse --verify HEAD) || exit
;;
@@ -598,11 +602,11 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date.")"
+   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date.")"
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date, rebase forced.")"
+   say "$(eval_gettext "Current branch \$branch_or_commit is up to 
date, rebase forced.")"
fi
 fi
 
@@ -632,7 +636,7 @@ git update-ref ORIG_HEAD $orig_head
 # we just fast-forwarded.
 if test "$mb" = "$orig_head"
 then
-   say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
+   say "$(eval_gettext "Fast-forwarded \$branch_or_commit to 
\$onto_name.")"
move_to_original_branch
finish_rebase
exit 0
-- 
2.15.0.345.gf926f18f3

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Monday 27 November 2017 07:17 PM, lars.schnei...@autodesk.com wrote:


Show a message in the original terminal and get rid of it when the
editor returns.



"... except in the case when an error occurs." could  be included if needed.

+   static const char *close_notice = NULL;
+


IIRC, this variable is bound to be `static` for sake of future proofing. 
So, I guess you could use the following suggestion of Eric Sunshine in 
the below conditional.


If you reverse this condition to say (!close_notice && isatty(2)),
then you save an isatty() invocation each time if close_notice is
already assigned.


+   if (isatty(2) && !close_notice) {
+   char *term = getenv("TERM");
+
+   if (term && strcmp(term, "dumb"))
+   /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+   close_notice = "\r\033[K";
+   else if (term && strstr(term, "emacsclient"))
+   /*
+* `emacsclient` (or `emacsclientw` on Windows) 
already prints
+* ("Waiting for Emacs...") if a file is opened 
for editing.
+* Therefore, we don't need to print the editor 
launch info.
+*/
+   ;
+   else
+   /* otherwise, complete and waste the line */
+   close_notice = _("done.\n");
+   }
+
+   if (close_notice) {
+   fprintf(stderr, _("Launched editor. Waiting for your 
input... "));
+   fflush(stderr);
+   }

p.argv = args;
p.env = env;
@@ -53,11 +82,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+   if (close_notice)
+   fputs(close_notice, stderr);
}

if (!buffer)
--
2.15.0

Re: [PATCH] git-status.txt: mention --no-optional-locks

[Kaartic Sivaraam]

On Monday 27 November 2017 11:37 AM, Junio C Hamano wrote:

Jeff King  writes:

+using `git --no-optional-locks status` (see linkgit:git[1] for details).


It strikes me just now that `--no-side-effects` might have been a better 
name for the option (of course, iff this avoid all kinds of side 
effects. I'm not sure about the side affects other than index refreshing 
of "git status"). And in case we didn't care about the predictability of 
option names even a little, `--do-what-i-say` might have been a catchy 
alternative ;-)



---
Kaartic

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Wednesday 22 November 2017 10:25 PM, Lars Schneider wrote:



On 20 Nov 2017, at 01:11, Junio C Hamano <gits...@pobox.com> wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:

It might be a good thing to keep the notice but I think it would be
better to have that error message in a "new line". I'm not sure if
it's possible or not.


Of course it is possible, if you really wanted to.  The code knows
if it gave the "we launched and waiting for you" notice, so it can
maintain not just one (i.e. "how I close the notice?") but another
one (i.e. "how I do so upon an error?") and use it in the error
codepath.


I think a newline makes sense. I'll look into this for v3.



If I remember correctly, I don't think it's as simple as printing a 
newline character in case of an error.  That's because the error message 
that shows up in the same line as "Launched your ..." comes from a 
different function (possibly finish_command() though I'm not sure)



Thanks,
Lars

Re: [RFC PATCH] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

Kaartic: Regarding the actual patch, rather than silencing
validate_worktree() (which seems an unfortunate thing to do), isn't it
possible simply to do a quick test to see if the worktree directory
exists before calling validate_worktree()? If it doesn't exist, then
just skip down to the part of the code which does the 'prune'
operation.



Nice suggestion. It seems to be a much better thing to do than silencing 
'validate_worktree' (it was an "ad-hoc" patch, you see). I'll send an 
update incorporating this suggestion (of course, with the suggested 
commit message), when I find time.


Thanks,
Kaartic

Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:

Eric Sunshine <sunsh...@sunshineco.com> writes:


On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
<kaartic.sivar...@gmail.com> wrote:

On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:

The original code unconditionally uses "+ 11", which says that the
prefix is _always_ present. This commit message muddies the waters [...]


That muddiness of that statement is a consequence of my recent encounter[1]
in which the assumption (that the prefix(refs/heads/ always exists) of that
code failed. I had a little suspicion, when I wrote that commit message,
that there might be other cases in which assumption might fail. The issue
has been resolved only in 3/4 of jc/branch-name-sanity but that was only
after I wrote the commit message initially.  So, it does make sense to
remove that muddiness now. Thanks for noting that.

[1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was
a consequence of the assumption that the 'prefix' always existed!
https://public-inbox.org/git/1509209933.2256.4.ca...@gmail.com/


Thanks for the explanation and history reference.


I have a suspicion that it wasn't the case.  The ugly uninitialized
byte sequence came from an error codepath of a function that is asked
to fill a strbuf with "refs/heads/$something" returning early when it
found an error in the input, without realizing that the caller still
looks at the strbuf even when it got an error.  That caller-callee pair
was updated.



You seem to be right when viewing from the perspective of the callee 
(strbuf_check_branch_ref). IOW, you *seem* to be telling that the 
"callee" should have known that the caller was expecting the 'prefix' 
even in case of an error and should have "inserted" it regardless of the 
error (I thought the strbuf was initialized and contained the result of 
strbuf_branchname even in the case of an error) ?


I thought that the 'caller' should have known that the 'callee' would 
not insert the prefix when there was an error in the branch name thus it 
should have anticipated that there would be a case in which the prefix 
(refs/heads/) doesn't exist in the buf (the assumption).




It is just a bug and +11 is always correct; passing the data that
does not begin with "refs/heads/" there, including the case where an
uninitialized buffer is passed, is simply a bug.


Let me see if I could correlate your statement with that error. AFAIK, I 
don't think the buffer was uninitialized in the case of that error. It 
had in it the result of interpretation of 'strbuf_branchname', didn't 
it? So that clears one case and leads me to the interpretation that,


"Passing the data (at an offset of 11 bytes from the beginning of the 
buf) that does not begin with "refs/heads/" is a bug"


Which seems to be in line with my statement that it was wrong (in the 
part of the "caller") to assume that "strbuf_check_branch_ref" always 
returns a buf with the prefix "refs/heads/" ?





In other words, skip_prefix() is a good change, but if we are to use
it, we should also check the result and error out if we do not see
"refs/heads/" there--you already bothered to spend extra cycles for
that.



Makes sense. I should have better done this initially instead of giving 
a muddy justification for not doing it.



Thanks,
Kaartic

Re: [RFC PATCH] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

On Wednesday 22 November 2017 09:25 AM, Junio C Hamano wrote:

Eric Sunshine  writes:

So, Kaatic's patch is intended to address that oversight (though I
haven't examined the implementation closely; I was just trying to
understand the reason for the patch).


OK, so the proposed log message was a bit confusing for those who
are *not* the person who wrote it (who knew why existing behaviour
was inadequate and did not describe how "worktree remove" would fail
under such a scenario to illustrate it, incorrectly assuming that
everybody who reads the proposed log message already *knows* how it
would fail).


I shouldn't have made the log message as 'ad hoc' as I made the patch, 
sorry :-(




"git worktree remove" removes both the named worktree
directory and the administrative information for it after
checking that there is no local modifications that would be
lost (which is a handy safety measure).  It however refuses
to work if the worktree directory is _already_ removed.

The user could use "git worktree prune" after seeing the
error and realizing the situation, but at that point, there
is nothing gained by leaving only the administrative data
behind.  Teach "git worktree remove" to go ahead and remove
the trace of the worktree in such a case.

or soemthing like that?



Much better!

Re: [RFC PATCH] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

On Wednesday 22 November 2017 03:07 AM, Eric Sunshine wrote:

On Tue, Nov 21, 2017 at 10:09 AM, Kaartic Sivaraam
<kaartic.sivar...@gmail.com> wrote:

The new feature to 'remove' worktree was handy to remove specific
worktrees. It didn't cover one particular case of removal. Specifically,
if there is an "entry" (a directory in /.git/worktrees)
for a worktree but the worktree repository itself does not exist then
it means that the "entry" is stale and it could just be removed.

So, in case there's a "worktree entry" but not "worktree direectory"
then just remove the 'stale' entry.


Let me see if I understand. Sometimes you know that you've deleted the
worktree directory, in which case 'git worktree prune' is the obvious
choice. However, there may be cases when you've forgotten that you
deleted the worktree directory (or it got deleted some other way), yet
it still shows up in "git worktree list" output with no indication
that it has been deleted (though, ideally, it should tell you so[1]).
In this case, you see a worktree that you know you no longer want, so
you invoke "git worktree remove" but that errors out because the
actual directory is already gone. This patch make the operation
succeed, despite the missing directory. Is that correct?



Yes. My primary motive for sending this is that, I thought it didn't 
make much sense for 'remove' to fail just because the directory didn't 
exist. Further, I thought it would be more flexible to allow for 
'selective' pruning of worktrees. The use case in which, I thought, this 
flexibility might prove helpful is that of 'scripts'.


Consider a hypothetical script that plays around with a git repository. 
Further, consider that spawns a new worktree for temporary use. It's 
quite natural for the script to consider cleaning up it's own mess and 
thus it might consider removing the worktree it created. With the 
'remove' command it is as easy is,


git worktree remove 

But what if it was the case that the directory might/might not exist 
when that part of the script is reached. In case the directory didn't 
exist the 'remove' command errors out that it didn't exist instead of 
just removing the worktree "entry" for that . I thought it 
wasn't a good thing and thus this script.


After writing this, I feel I haven't justified it well (impostor 
syndrome, possibly). Anyways, at the end of the day this "ad-hoc" patch 
was just a result of my gut feeling that, "It didn't make sense for the 
'remove' command to fail when the directory didn't exist". The 
implementation is just a "sloppy" illustration of how it could be 
achieved and should "better" not be used as such.




[1]: Excerpt from:
https://public-inbox.org/git/capig+cttrv2c7jlu1dr4+n8xo+7yq+deiwlda835wbgd6fh...@mail.gmail.com/

Other information which would be nice to display for each worktree
[by the 'list' command] (possibly controlled by a --verbose flag):

* the checked out branch or detached head
* whether it is locked
 - the lock reason (if available)
 - and whether the worktree is currently accessible
 * whether it can be pruned
 - and the prune reason if so



It would nice to see this information. It would be very useful but 
'list' doesn't seem to be the right sub-sub-command to give such 
information. Maybe there should be a new sub-sub-command named 'info' or 
something to give such information?

[PATCH v3 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Changes in v3:

  - Update commit message

  - Removed superfluous comment

 builtin/branch.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..4ad3b228b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *interpreted_oldname = NULL;
+   const char *interpreted_newname = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,6 +495,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   skip_prefix(oldref.buf, "refs/heads/", _oldname);
+   skip_prefix(newref.buf, "refs/heads/", _newname);
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
@@ -508,10 +513,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   interpreted_oldname);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   interpreted_oldname);
}
 
if (!copy &&
@@ -520,9 +525,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_oldname);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_newname);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.345.gf926f18f3

Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:

On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam
<kaartic.sivar...@gmail.com> wrote:


Though we don't check for the result of verification here as
it's (almost) always the case that the string does start
with "refs/heads", it's just better to avoid hard-coding and
be more communicative.


The original code unconditionally uses "+ 11", which says that the
prefix is _always_ present. This commit message muddies the waters by
saying the prefix might or might not be present. Which is correct? If
the code is correct, then the commit message is misleading; if the
message is correct, then the code is buggy and the commit message
should say that it is fixing a bug.



That muddiness of that statement is a consequence of my recent 
encounter[1] in which the assumption (that the prefix(refs/heads/ always 
exists) of that code failed. I had a little suspicion, when I wrote that 
commit message, that there might be other cases in which assumption 
might fail. The issue has been resolved only in 3/4 of 
jc/branch-name-sanity but that was only after I wrote the commit message 
initially.  So, it does make sense to remove that muddiness now. Thanks 
for noting that.




diff --git a/builtin/branch.c b/builtin/branch.c
@@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
+   /* At this point it should be safe to believe that the refs have the
+  prefix "refs/heads/" */
+   skip_prefix(oldref.buf, "refs/heads/", _oldname);
+   skip_prefix(newref.buf, "refs/heads/", _newname);


 /*
  * Format mult-line comments
  * like this.
  */

However, this in-code comment shares the same problem as the commit
message. It muddies the waters by saying that the prefix may or may
not be present, whereas the original code unconditionally stated that
it was present. Moreover, the comment adds very little or any value
since it's pretty much repeating what the code itself already says.
Consequently, it probably would be best to drop the comment
altogether.


Makes sense. Of course, only if there aren't any code paths that lead to 
a state where the expected prefix doesn't exist. Hope, there's none!






 if (copy)
 strbuf_addf(, "Branch: copied %s to %s",
 oldref.buf, newref.buf);
 else
 strbuf_addf(, "Branch: renamed %s to %s",
 oldref.buf, newref.buf);
-
 if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))


Was the blank line removal intentional?


Nope.


Footnotes:
[1]: Note the 'warning: ' message in the following mail. That ugly '|�?' 
was a consequence of the assumption that the 'prefix' always existed!


https://public-inbox.org/git/1509209933.2256.4.ca...@gmail.com/


---
Kaartic

[PATCH] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

In a repository when attempting to rebase when the HEAD is detached
and it is already up to date with upstream (so there's nothing to do),
the following message is shown

Current branch HEAD is up to date.

which is clearly wrong as HEAD is not a branch.

Handle the special case of HEAD correctly to give a more precise
error message.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 In this patch, I basically assumed that there would be no
 branch named "HEAD". To the cotrary if it did, it would make
 'git' throw spurious ambiguity messages, in general. So, I
 guess it's not worth trying to check if HEAD is a branch or
 not and handle that specially.

 git-rebase.sh | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6344e8d5e..933df832a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -598,11 +598,21 @@ then
test -z "$switch_to" ||
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
git checkout -q "$switch_to" --
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date.")"
+   if test "$branch_name" = "HEAD"
+   then
+   say "$(eval_gettext "HEAD is up to date.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_name is up 
to date.")"
+   fi
finish_rebase
exit 0
else
-   say "$(eval_gettext "Current branch \$branch_name is up to 
date, rebase forced.")"
+   if test "$branch_name" = "HEAD"
+   then
+   say "$(eval_gettext "HEAD is up to date, rebase 
forced.")"
+   else
+   say "$(eval_gettext "Current branch \$branch_name is up 
to date, rebase forced.")"
+   fi
fi
 fi
 
-- 
2.15.0.345.gf926f18f3

[RFC PATCH] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

The new feature to 'remove' worktree was handy to remove specific
worktrees. It didn't cover one particular case of removal. Specifically,
if there is an "entry" (a directory in /.git/worktrees)
for a worktree but the worktree repository itself does not exist then
it means that the "entry" is stale and it could just be removed.

So, in case there's a "worktree entry" but not "worktree direectory"
then just remove the 'stale' entry.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---

Hello Duy,

I noticed that your remove command could be enhanced for a particular
case. So, I made up an ad-hoc patch "illustrating" how it could be done.
I may have broken something by quieting out 'validate_worktree()'
function, so take note of it.

You might add this as a separate commit or just incorporate it into
one of your commits if you re-roll your 'nd/worktree-move' branch.

Thanks,
Kaartic

 builtin/worktree.c | 38 --
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b5afba164..f70bc0bd8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -605,6 +605,22 @@ static int move_worktree(int ac, const char **av, const 
char *prefix)
return update_worktree_location(wt, dst.buf);
 }
 
+/* Removes the .git/worktrees/worktree_id directory for
+   the given worktree_id
+
+   Returns 0 on success and non-zero value in case of failure */
+static int remove_worktree_entry(char *worktree_id) {
+   int ret = 0;
+   struct strbuf we_path = STRBUF_INIT;
+   strbuf_addstr(_path, git_common_path("worktrees/%s", worktree_id));
+   if (remove_dir_recursively(_path, 0)) {
+   error_errno(_("failed to delete '%s'"), we_path.buf);
+   ret = -1;
+   }
+   strbuf_release(_path);
+   return ret;
+}
+
 static int remove_worktree(int ac, const char **av, const char *prefix)
 {
int force = 0;
@@ -634,9 +650,17 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
die(_("already locked, reason: %s"), reason);
die(_("already locked, no reason"));
}
-   if (validate_worktree(wt, 0))
-   return -1;
-
+   if (validate_worktree(wt, 1)) {
+   if (!file_exists(wt->path)) {
+   /* There's a worktree entry but the worktree directory
+  doesn't exist. So, just remove the worktree entry. */
+   ret = remove_worktree_entry(wt->id);
+   free_worktrees(worktrees);
+   return ret;
+   } else {
+   return -1;
+   }
+   }
if (!force) {
struct argv_array child_env = ARGV_ARRAY_INIT;
struct child_process cp;
@@ -670,13 +694,7 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
error_errno(_("failed to delete '%s'"), sb.buf);
ret = -1;
}
-   strbuf_reset();
-   strbuf_addstr(, git_common_path("worktrees/%s", wt->id));
-   if (remove_dir_recursively(, 0)) {
-   error_errno(_("failed to delete '%s'"), sb.buf);
-   ret = -1;
-   }
-   strbuf_release();
+   ret = remove_worktree_entry(wt->id);
free_worktrees(worktrees);
return ret;
 }
-- 
2.15.0.345.gf926f18f3

[PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Though we don't check for the result of verification here as
it's (almost) always the case that the string does start
with "refs/heads", it's just better to avoid hard-coding and
be more communicative.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---

Eric Sunshine <sunsh...@sunshineco.com> writes:
> Perhaps call them 'oldref_bare' and 'newref_bare' or something.

Nice suggestion but I opted for the more communicative (atleast to
me) 'interpreted_*name'. Let me know if they have any issues.

>  It's probably not worth a re-roll,
> though...

It's definitely not worth a re-roll of the series but deserves a re-roll
of this patch. (I fixed a comment in this re-roll, thanks!)

Thanks,
Kaartic

 builtin/branch.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..d3751ca70 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *interpreted_oldname = NULL;
+   const char *interpreted_newname = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   /* At this point it should be safe to believe that the refs have the
+  prefix "refs/heads/" */
+   skip_prefix(oldref.buf, "refs/heads/", _oldname);
+   skip_prefix(newref.buf, "refs/heads/", _newname);
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
else
strbuf_addf(, "Branch: renamed %s to %s",
oldref.buf, newref.buf);
-
if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
@@ -508,10 +514,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   interpreted_oldname);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   interpreted_oldname);
}
 
if (!copy &&
@@ -520,9 +526,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_oldname);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_newname);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.345.gf926f18f3

[PATCH] git-rebase: clean up dashed-usages in messages

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6344e8d5e..2f5d138a0 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -9,7 +9,7 @@ OPTIONS_STUCKLONG=t
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--exec ] [--onto ] [] 
[]
 git rebase [-i] [options] [--exec ] [--onto ] --root []
-git-rebase --continue | --abort | --skip | --edit-todo
+git rebase --continue | --abort | --skip | --edit-todo
 --
  Available options are
 v,verbose! display a diffstat of what changed upstream
@@ -216,7 +216,7 @@ run_pre_rebase_hook () {
 }
 
 test -f "$apply_dir"/applying &&
-   die "$(gettext "It looks like git-am is in progress. Cannot rebase.")"
+   die "$(gettext "It looks like 'git am' is in progress. Cannot rebase.")"
 
 if test -d "$apply_dir"
 then
-- 
2.15.0.291.g0d8980c5d

Re: [PATCH] docs: checking out using @{-N} can lead to detached state

[Kaartic Sivaraam]

On Monday 20 November 2017 07:39 AM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:

After the first paragraph explains what happens during "checkout
" and goes from the normal case where  is really a
branch name to an arbitrary commit (where "detaching" needs to be
mentioned), a commit before 75d6e552a added mention of @{-N} and
made it appear as if it were a reference to a commit (i.e. not a
branch name) and that was why it said "As a special case" and
mentioned "detaching".  The problem lies in a lot older one,
696acf45 ("checkout: implement "-" abbreviation, add docs and
tests", 2009-01-17).



Thanks for the analysis. Just to be sure, I referred to 75d6e552a just 
to back up my claim, was that intention clear in my log message? Also, 
should I mention the old commit (696acf45) also in the log message?





So perhaps we should start from dropping that "As a special case".

 You can also use the `"@{-N}"` syntax to refer to the thing the N-th
 last "git checkout" operation checked out; if it was a branch, that
 branch is checked out, and otherwise the HEAD is detached at the
 commit.  You may also specify `-` which is synonymous to `"@{-1}"`.

or something like that.  If we do so, we'd further need to tweak "As
a further special case", as this rewrite makes it clear that "@{-N}"
is not a special case at all (instead it is merely a different way
to spell  or  that is already covered).


Good point. I did use your rewritten message but with some modification,

You can also use the `@{-N}` syntax to refer to the N-th last
branch/commit checked out using "git checkout" operation. You may
also specify `-` which is synonymous to `@{-1}`.

I tweaked the first part of the first sentence and dropped the last part 
of it just to avoid redundancy with the paragraph above it. Hope that 
sounds good. If it seems to need some modification, let me know.



Thanks,
Kaartic

Can @{-N} always be used where a branch name is expected?

[Kaartic Sivaraam]

I was recently digging to find if there is any special syntax accepted 
for  in "git branch -m  " other than 
the plain branch name. I discovered the @{-N} notation. I was trying to 
play around with it and found that it didn't work as guaranteed by the 
last sentence  of the following paragraph in the "check-ref-format" 
documentation,



With the --branch option, it expands the “previous branch syntax” 
@{-n}. For example, @{-1} is a way to refer the last branch you were on.
This option should be used by porcelains to accept this syntax 
anywhere a branch name is expected, so they can act as if you typed the

branch name.


In particular the following case doesn't work,

git init test &&
cd test &&
echo "Hello" >file &&
git add file &&
git commit -m "Initial commit that will be checked out" &&
echo "Hello world" >file &&
git commit file -m "Second commit" &&
git checkout HEAD^ &&
git checkout - &&
git branch -m @{-1} initial-commit


It failed with an error,

error: refname refs/heads/d21e72600673c670b3ae803488d0cebfa949e4c3 not found
fatal: Branch rename failed


Then I digged into why it didn't work to discover that @{-N}  just 
expands to a valid checkout and not a valid "branch name". So, the 
documentation  guaranteeing that "@{-N} acts as if you typed the branch 
name" seems wrong.  That makes me think that we should avoid misleading 
the user in the "check-ref-format" documentation.


So, should we update the 'check-ref-format' doc or am I missing something?

---
Kaartic

[PATCH] docs: checking out using @{-N} can lead to detached state

[Kaartic Sivaraam]

The commit message of 75d6e552a (Documentation: @{-N} can refer to
a commit, 2014-01-19) clearly specifies how @{-N} can be used to
refer not only to a branch but also to a commit. IOW, @{-N} is a
syntax for the N-th last "checkout" and not the N-th last "branch"
Therefore, in some cases using `git checkout @{-$N}` does lead to a
"detached HEAD" state.

Correct the misleading sentence which states that @{-N} doesn't
detach HEAD.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 Documentation/git-checkout.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e108b0f74..238880f10 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -272,9 +272,8 @@ section of linkgit:git-add[1] to learn how to operate the 
`--patch` mode.
commit, your HEAD becomes "detached" and you are no longer on
any branch (see below for details).
 +
-As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
-checks out branches (instead of detaching).  You may also specify
-`-` which is synonymous with `"@{-1}"`.
+As a special case, the `@{-N}` syntax checks out the N-th last 
branch/commit(checkout).
+You may also specify `-` which is synonymous with `@{-1}`.
 +
 As a further special case, you may use `"A...B"` as a shortcut for the
 merge base of `A` and `B` if there is exactly one merge base. You can
-- 
2.15.0.291.g0d8980c5d

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Saturday 18 November 2017 07:10 AM, Junio C Hamano wrote:

Eric Sunshine  writes:


@@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
+   static const char *close_notice = NULL;
+
+   if (isatty(2) && !close_notice) {


If you reverse this condition to say (!close_notice && isatty(2)),
then you save an isatty() invocation each time if close_notice is
already assigned.

However, it's not clear how much benefit you gain from stashing this
away in a static variable. Premature optimization?


The variable being "static" could be (but it was done primarily
because it allowed me not to worry about freeing),


AFAIK, observing the way the variable 'close_notice' is used, I guess 
you don't need to worry about freeing it up even if it wasn't "static".

That's my interpretation of the following section of the C standard,


6.5.2.5 Compound literals

...

9 EXAMPLE 2
In contrast, in
void f(void)
{
int *p;
/*...*/
p = (int [2]){*p};
/*...*/
}
p is assigned the address of the first element of an array of two ints, 
the first having the value previously pointed to by p and the second, 
zero. The expressions in this compound literal need not be constant. The

unnamed object has automatic storage duration.
=

So the unnamed objects created as a consequence of the string literals 
assigned to 'close_notice' should have "automatic" storage duration 
which means you don't have to worry about allocating memory for them 
which would make you worry about freeing it up. If I'm stating something 
wrong, let me know.


BTW, I find making the variable 'close_notice' to be 'static' unwanted 
as I couldn't find any piece of code that invokes 'launch_editor' more 
than once within a single run. What could be the possible cases in which 
'launch_editor' could be called twice ?




Should printing of close_notice be done before the error()? Otherwise,
you get this:

--- 8< ---
Launched your editor (...) ...There was a problem...
--- 8< ---


In my version with a far shorter message, I deliberately chose not
to clear the notice.  We ran the editor, and we saw a problem.  That
is what happened and that is what will be left on the terminal.



It might be a good thing to keep the notice but I think it would be 
better to have that error message in a "new line". I'm not sure if it's 
possible or not.



---
Kaartic

Re: [PATCH] git-rebase: clean up dashed-usages in messages

[Kaartic Sivaraam]

On Sunday 19 November 2017 07:42 AM, Junio C Hamano wrote:

  test -f "$apply_dir"/applying &&
-   die "$(gettext "It looks like git-am is in progress. Cannot rebase.")"
+   die "$(gettext "It looks like you are in the middle of an am session. Cannot 
rebase.")"


Probably not, as 'am' alone would be confusing.

 "It looks like 'git am' is in progress. Cannot rebase."

may be a more sensible improvement.



Let me guess, 'am' alone would be confusing because it follows 'an' in 
the error message. So, the user might mistake it to be some kind of typo 
in the error message. Is that close to why 'am' alone would be confusing?


Anyways, I stole that from a message shown by 'git status',

"You are in the middle of an am session."

So, does that require a change too?


---
Kaartic

Re: [PATCH 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

On Sunday 19 November 2017 06:34 AM, Eric Sunshine wrote:

On Sat, Nov 18, 2017 at 12:26 PM, Kaartic Sivaraam
<kaartic.sivar...@gmail.com> wrote:

diff --git a/builtin/branch.c b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
  {
 struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
 struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *prefix_free_oldref = NULL;
+   const char *prefix_free_newref = NULL;


A bit of a mouthful.



Quite possibly.


> Perhaps name these 'oldname' and 'newname' or something?

How about the following ?

1) "interpreted_oldname" and "interpreted_newname" or

2) "stripped_oldref" and "stripped_newref"

I couldn't come up with better names for now.


---
Kaartic

Re: [PATCH] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Thursday 16 November 2017 11:34 AM, Junio C Hamano wrote:


  * I tried this with "emacsclient" but it turns out that Emacs folks
have already thought about this issue, and the program says
"Waiting for Emacs..." while it does its thing, so from that
point of view, perhaps as Stefan said originally, the editor Lars
had trouble with is what is at fault and needs fixing?  I dunno.

Also, emacsclient seems to conclude its message, once the editing
is done, by emitting LF _before_ we have a chance to do the "go
back to the beginning and clear" dance, so it probably is not a
very good example to emulate the situation Lars had trouble with.
You cannot observe the nuking of the "launched, waiting..." with
it.



I tried this patch with 'gedit' and it seems to work fine except for one 
particular case. When the launched editor gets killed somehow when 
waiting for user input, the error message shown in the terminal seems to 
be following the "Launched editor..." message immediately, making it 
hard to identify it.


$ GIT_EDITOR=gedit ./git commit --allow-empty
Launched your editor, waiting...error: gedit died of signal 15
error: There was a problem with the editor 'gedit'.
Please supply the message using either -m or -F option.

Though this is not something that's going to happen very often, I'm not 
sure if this is to be considered. Just wanted to note what I observed.




  editor.c | 25 +
  1 file changed, 25 insertions(+)

diff --git a/editor.c b/editor.c
index 7519edecdc..1321944716 100644
--- a/editor.c
+++ b/editor.c
@@ -40,6 +40,28 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+   static const char *close_notice = NULL;


Just a little curious, what's the advantage of making 'close_notice' 
'static' over 'auto' ?




+
+   if (isatty(2) && !close_notice) {
+   char *term = getenv("TERM");
+
+   if (term && strcmp(term, "dumb"))
+   /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+   close_notice = "\r\033[K";
+   else
+   /* otherwise, complete and waste the line */
+   close_notice = "done.\n";
+   }
+
+   if (close_notice) {
+   fprintf(stderr, "Launched your editor, waiting...");
+   fflush(stderr);


Being curious again, is flushing 'stderr' required ? AFAIK, 'stderr' is 
unbuffered by default and I didn't notice any calls that changed the 
buffering mode of it along this code path.




+   }
  
  		p.argv = args;

p.env = env;
@@ -53,11 +75,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+   if (close_notice)
+   fputs(close_notice, stderr);
}
  
  	if (!buffer)

[PATCH 1/4] branch: improve documentation and naming of create_branch() parameters

[Kaartic Sivaraam]

The documentation for 'create_branch()' was incomplete as it didn't say
what certain parameters were used for. Further a parameter name wasn't
very communicative.

So, add missing documentation for the sake of completeness and easy
reference. Also, rename the concerned parameter to make its name more
communicative.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 branch.c | 4 ++--
 branch.h | 7 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 62f7b0d8c..3e8d2f93f 100644
--- a/branch.c
+++ b/branch.c
@@ -228,7 +228,7 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head,
+  int force, int reflog, int clobber_head_ok,
   int quiet, enum branch_track track)
 {
struct commit *commit;
@@ -244,7 +244,7 @@ void create_branch(const char *name, const char *start_name,
 
if (validate_new_branchname(name, , force,
track == BRANCH_TRACK_OVERRIDE ||
-   clobber_head)) {
+   clobber_head_ok)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index b07788558..cb6411f84 100644
--- a/branch.h
+++ b/branch.h
@@ -15,12 +15,17 @@
  *
  *   - reflog creates a reflog for the branch
  *
+ *   - clobber_head_ok allows the currently checked out (hence existing)
+ * branch to be overwritten; without 'force', it has no effect.
+ *
+ *   - quiet suppresses tracking information
+ *
  *   - track causes the new branch to be configured to merge the remote branch
  * that start_name is a tracking branch for (if any).
  */
 void create_branch(const char *name, const char *start_name,
   int force, int reflog,
-  int clobber_head, int quiet, enum branch_track track);
+  int clobber_head_ok, int quiet, enum branch_track track);
 
 /*
  * Validates that the requested branch may be created, returning the
-- 
2.15.0.291.g0d8980c5d

[PATCH 3/4] branch: update warning message shown when copying a misnamed branch

[Kaartic Sivaraam]

When a user tries to rename a branch that has a "bad name" (e.g.,
starts with a '-') then we warn them that the misnamed branch has
been renamed "away". A similar message is shown when trying to create
a copy of a misnamed branch even though it doesn't remove the misnamed
branch. This is not correct and may confuse the user.

So, update the warning message shown to be more precise that only a copy
of the misnamed branch has been created. It's better to show the warning
message than not showing it at all as it makes the user aware of the
presence of a misnamed branch.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4edef5baa..ca9d8abd0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -507,7 +507,7 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
if (recovery) {
if (copy)
-   warning(_("Copied a misnamed branch '%s' away"),
+   warning(_("Created a copy of a misnamed branch '%s'"),
oldref.buf + 11);
else
warning(_("Renamed a misnamed branch '%s' away"),
-- 
2.15.0.291.g0d8980c5d

[PATCH 2/4] branch: group related arguments of create_branch()

[Kaartic Sivaraam]

39bd6f726 (Allow checkout -B  to update the current
branch, 2011-11-26) added 'clobber_head' (now, 'clobber_head_ok')
"before" 'track' as 'track' was closely related 'clobber_head' for
the purpose the commit wanted to achieve. Looking from the perspective
of how the arguments are used it turns out that 'clobber_head' is
more related to 'force' than it is to 'track'.

So, re-order the arguments to keep the related arguments close
to each other.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 branch.c   | 2 +-
 branch.h   | 9 +
 builtin/branch.c   | 2 +-
 builtin/checkout.c | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index 3e8d2f93f..bd607ae97 100644
--- a/branch.c
+++ b/branch.c
@@ -228,7 +228,7 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head_ok,
+  int force, int clobber_head_ok, int reflog,
   int quiet, enum branch_track track)
 {
struct commit *commit;
diff --git a/branch.h b/branch.h
index cb6411f84..f66536a10 100644
--- a/branch.h
+++ b/branch.h
@@ -13,19 +13,20 @@
  *
  *   - force enables overwriting an existing (non-head) branch
  *
- *   - reflog creates a reflog for the branch
- *
  *   - clobber_head_ok allows the currently checked out (hence existing)
  * branch to be overwritten; without 'force', it has no effect.
  *
+ *   - reflog creates a reflog for the branch
+ *
  *   - quiet suppresses tracking information
  *
  *   - track causes the new branch to be configured to merge the remote branch
  * that start_name is a tracking branch for (if any).
+ *
  */
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog,
-  int clobber_head_ok, int quiet, enum branch_track track);
+  int force, int clobber_head_ok,
+  int reflog, int quiet, enum branch_track track);
 
 /*
  * Validates that the requested branch may be created, returning the
diff --git a/builtin/branch.c b/builtin/branch.c
index 33fd5fcfd..4edef5baa 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
die(_("the '--set-upstream' option is no longer 
supported. Please use '--track' or '--set-upstream-to' instead."));
 
create_branch(argv[0], (argc == 2) ? argv[1] : head,
- force, reflog, 0, quiet, track);
+ force, 0, reflog, quiet, track);
 
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7d8bcc383..6068f8d8c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -640,8 +640,8 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
else
create_branch(opts->new_branch, new->name,
  opts->new_branch_force ? 1 : 0,
- opts->new_branch_log,
  opts->new_branch_force ? 1 : 0,
+ opts->new_branch_log,
  opts->quiet,
  opts->track);
new->name = opts->new_branch;
-- 
2.15.0.291.g0d8980c5d

[PATCH 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Though we don't check for the result of verification here as
it's (almost) always the case that the string does start
with "refs/heads", it's just better to avoid hard-coding and
be more communicative.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 builtin/branch.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..8c546a958 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *prefix_free_oldref = NULL;
+   const char *prefix_free_newref = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   /* At this point it should be safe to believe that the refs have the
+  prefix "refs/heads" */
+   skip_prefix(oldref.buf, "refs/heads/", _free_oldref);
+   skip_prefix(newref.buf, "refs/heads/", _free_newref);
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
else
strbuf_addf(, "Branch: renamed %s to %s",
oldref.buf, newref.buf);
-
if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
@@ -508,10 +514,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   prefix_free_oldref);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   prefix_free_oldref);
}
 
if (!copy &&
@@ -520,9 +526,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", prefix_free_oldref);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", prefix_free_newref);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.291.g0d8980c5d

[PATCH 0/4] cleanups surrounding branch

[Kaartic Sivaraam]

On the process of making 'git' give more useful error messages
when trying to rename a branch[0], I found a few things that could
be cleaned up. After noticing that the cleanup commits exceeded
the commits that are related to the series, I thought it would
be better to separate the cleanups into an independent series
to keep that topic focused on improving the error messages.

1/4 and 2/4 were part of that series until v3. The others are new
cleanups.

This series goes on top of 'master' and can be found in my fork
as branch 'work/branch-cleanups'[1].

Note: 1/4 might possibly have some conflicts with jc/branch-name-sanity

Footnotes:

[0]: cf. <20171102065407.25404-1-kaartic.sivar...@gmail.com>

[1]: https://github.com/sivaraam/git/tree/work/branch-cleanups


Kaartic Sivaraam (4):
  branch: improve documentation and naming of create_branch() parameters
  branch: group related arguments of create_branch()
  branch: update warning message shown when copying a misnamed branch
  builtin/branch: strip refs/heads/ using skip_prefix

 branch.c   |  4 ++--
 branch.h   | 10 --
 builtin/branch.c   | 20 +---
 builtin/checkout.c |  2 +-
 4 files changed, 24 insertions(+), 12 deletions(-)

-- 
2.15.0.291.g0d8980c5d

[PATCH] git-rebase: clean up dashed-usages in messages

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 6344e8d5e..42a485aaa 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -9,7 +9,7 @@ OPTIONS_STUCKLONG=t
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--exec ] [--onto ] [] 
[]
 git rebase [-i] [options] [--exec ] [--onto ] --root []
-git-rebase --continue | --abort | --skip | --edit-todo
+git rebase --continue | --abort | --skip | --edit-todo
 --
  Available options are
 v,verbose! display a diffstat of what changed upstream
@@ -216,7 +216,7 @@ run_pre_rebase_hook () {
 }
 
 test -f "$apply_dir"/applying &&
-   die "$(gettext "It looks like git-am is in progress. Cannot rebase.")"
+   die "$(gettext "It looks like you are in the middle of an am session. 
Cannot rebase.")"
 
 if test -d "$apply_dir"
 then
-- 
2.15.0.291.g0d8980c5d

Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}

[Kaartic Sivaraam]

On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:


I guess this series is not yet ready for 'next'. When I tried to apply
this patch it doesn't seem to be applying cleanly. I get some
conflicts in 'sha1_name.c' possibly as a consequence of the changes to
the file that aren't accounted by the patch.


Oh, it is totally expected that this patch (and others) may not
apply on 'next' without conflict resolution, as this topic, as all
the other topics, are designed to apply cleanly to either 'master'
or 'maint' or one of the older 'maint-*' series, if it is a bugfix
topic.  A patch series that only applies cleanly to 'next' would be
useless---it would mean all the topics that are already in 'next'
that interacts with it must graduate first before it can go in.


Thanks for the explanation. Seems I still have to become accustomed to 
the workflow.




Make it a habit to build on 'master' or older and then merge to
higher integration branches to make sure it fits with others.



Though I don't clearly understand how to do that, I'll let experience 
teach me the same (if possible). :-)




What you could do is to inspect origin/pu branch after you fetch,
and look at the commit that merges this topic to learn how the
conflicts are resolved (the contrib/rerere-train.sh script may help
this process).



Sure thing.



+   if (*name == '-' ||
+   !strcmp(sb->buf, "refs/heads/HEAD"))


I guess this check should be made more consistent. Possibly either of,


Among these two, this one


if (starts_with(sb->buf, "refs/heads/-") ||
!strcmp(sb->buf, "refs/heads/HEAD"))


has more chance to be correct.  Also, if we were to check the result
of expansion in sb->buf[], I would think that we should keep a
separate variable that points at >buf[11] and compare the
remainder against fixed strings, as we already know sb->buf[] starts
with "refs/heads/" due to our splicing in the fixed string.

Because the point of using strbuf_branchname() is to allow us expand
funny notations like @{-1} to refs/heads/foo, and the result of
expansion is what eventually matters, checking name[] is wrong, I
would think, even though I haven't thought things through.

In any case, I would say thinking this part through should be left
as a theme for a follow-on patch, and not within the scope of this
one.  After all, checking *name against '-' was part of the original
code, so it is safer to keep the thing we are not touching bug-to-bug
compatible and fixing things one step at a time (the one fix we made
with this patch is to make sure we store refs/heads/-dash in sb when
we reject name=="-dash").



Good point. This is better for a follow-up patch as there's a 
possibility that we might be introducing new bugs which weren't present 
previously as a consequence of changing that conditional (bug-to-bug 
compatibility, good term that (possibly) summarizes my long-winded 
explanation ;-)

Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}

[Kaartic Sivaraam]

On Thursday 16 November 2017 03:44 AM, Junio C Hamano wrote:

 Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:

 >> Are these two patches follow-up fixes (replacement of 3/3 plus an
 >> extra patch) to jc/branch-name-sanity topic?
 >
 > Yes, that's right.
 >
 >> Thanks for working on these.
 >
 > You're welcome. Please do be sure I haven't broken anything in
 > v2. These patches should cleanly apply on 'next', if they don't let me
 > know.

 OK, so here is a replacement for your replacement, based on an
 additional analysis I did while I was reviewing your changes.
 The final 4/4 is what you sent as [v2 2/2] (which was meant to
 be [v2 4/3]).  I think with these updates, the resulting 4-patch
 series is good for 'next'.



I guess this series is not yet ready for 'next'. When I tried to apply 
this patch it doesn't seem to be applying cleanly. I get some conflicts 
in 'sha1_name.c' possibly as a consequence of the changes to the file 
that aren't accounted by the patch. As to which change,


$ git whatchanged  jch/jc/branch-name-sanity..origin/next sha1_name.c

lists at least 5 of them, so there's possibly a lot of change that 
hasn't been taken into account by this patch. Particularly, the function 
'strbuf_check_branch_ref' itself is found at line 1435 in the version 
found in 'next' though this patch expects it to be near line 1332, I guess.


Further comment inline.


  sha1_name.c | 14 --
  t/t1430-bad-ref-name.sh | 43 +++
  2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..67961d6e47 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char 
*name, unsigned allowed)
  int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
  {
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-   if (name[0] == '-')
-   return -1;
+
+   /*
+* This splice must be done even if we end up rejecting the
+* name; builtin/branch.c::copy_or_rename_branch() still wants
+* to see what the name expanded to so that "branch -m" can be
+* used as a tool to correct earlier mistakes.
+*/
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+   if (*name == '-' ||
+   !strcmp(sb->buf, "refs/heads/HEAD"))


I guess this check should be made more consistent. Possibly either of,

if (starts_with(sb->buf, "refs/heads/-") ||
!strcmp(sb->buf, "refs/heads/HEAD"))

or,

if (*name == '-' ||
!strcmp(name, "HEAD"))


might make them consistent (at least from my perspective).


I tried to reproduce this patch manually and other than the above this 
one LGTM. Though I can't be very sure as I couldn't apply it (I did it 
"manually" to some extent, you see ;-)




+   return -1;
+
return check_refname_format(sb->buf, 0);
  }
  
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh

index e88349c8a0..c7878a60ed 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete 
with bad ref name' '
grep "fatal: invalid ref format: ~a" err
  '
  
+test_expect_success 'branch rejects HEAD as a branch name' '

+   test_must_fail git branch HEAD HEAD^ &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+   test_must_fail git checkout -B HEAD HEAD^ &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'update-ref can operate on refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git show-ref refs/heads/HEAD &&
+   git update-ref -d refs/heads/HEAD &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -d can remove refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git branch -d HEAD &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -m can rename refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git branch -m HEAD tail &&
+   test_must_fail git show-ref refs/heads/HEAD &&
+   git show-ref refs/heads/tail
+'
+
+test_expect_success 'branch -d can remove refs/heads/-dash' '
+   git update-ref refs/heads/-dash HEAD^ &&
+   git branch -d -- -dash &&
+   test_must_fail git show-ref refs/heads/-dash
+'
+
+test_expect_success 'branch -m can rename refs/heads/-dash' '
+   git update-ref refs/heads/-dash HEAD^ &&
+   git branch -m -- -dash dash &&
+   test_must_fail git show-ref refs/heads/-dash &&
+   git show-ref refs/heads/dash
+'
+
  test_done

Re: [PATCH] branch doc: remove --set-upstream from synopsis

[Kaartic Sivaraam]

On Thursday 16 November 2017 04:19 PM, Martin Ågren wrote:

On 16 November 2017 at 08:46, Todd Zullinger  wrote:

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index d6587c5e96..159ca388f1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 [(--merged | --no-merged) []]
 [--contains [

Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD

[Kaartic Sivaraam]

On Tuesday 14 November 2017 08:38 PM, Junio C Hamano wrote:

Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:


I should have been a little more clear with the numbering, sorry. The
correct prefix should have been as follows,

* [PATCH v2 1/2] --> [PATCH v2 3/3]

* [PATCH v2 1/2] --> [PATCH v2 4/3]

Sorry for the inconvenience.


I assume that the second one above actually talks about what was
sent as "v2 2/2" (not "v2 1/2") being "4/3"?



Yeah. Copy paste error, sorry.



Are these two patches follow-up fixes (replacement of 3/3 plus an
extra patch) to jc/branch-name-sanity topic?



Yes, that's right.



Thanks for working on these.



You're welcome. Please do be sure I haven't broken anything in v2. These 
patches should cleanly apply on 'next', if they don't let me know.



Thanks,
Kaartic

Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD

[Kaartic Sivaraam]

I should have been a little more clear with the numbering, sorry. The 
correct prefix should have been as follows,


   * [PATCH v2 1/2] --> [PATCH v2 3/3]

   * [PATCH v2 1/2] --> [PATCH v2 4/3]

Sorry for the inconvenience.


---
Kaartic

[PATCH v2 2/2] builtin/branch: remove redundant check for HEAD

[Kaartic Sivaraam]

The lower level code has been made to handle this case for the
sake of consistency. This has made this check redundant.

So, remove the redundant check.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 builtin/branch.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ffd39333a..5fc57cdc9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -793,9 +793,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
} else if (argc > 0 && argc <= 2) {
struct branch *branch = branch_get(argv[0]);
 
-   if (!strcmp(argv[0], "HEAD"))
-   die(_("it does not make sense to create 'HEAD' 
manually"));
-
if (!branch)
die(_("no such branch '%s'"), argv[0]);
 
-- 
2.15.0.rc2.397.geff0134c7.dirty

[PATCH v2 1/2] branch: forbid refs/heads/HEAD

[Kaartic Sivaraam]

From: Junio C Hamano <gits...@pobox.com>

strbuf_check_branch_ref() is the central place where many codepaths
see if a proposed name is suitable for the name of a branch.  It was
designed to allow us to get stricter than the check_refname_format()
check used for refnames in general, and we already use it to reject
a branch whose name begins with a '-'.

Use it to also reject "HEAD" as a branch name.

Helped-by: Jeff King <p...@peff.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
  Changes in v2:

  * Fixed the issue of .git/HEAD being renamed when trying to do,

  $ git branch -m HEAD head

   This also allows to rename a branch named HEAD that was created by accident.

   cf. <1509209933.2256.4.ca...@gmail.com>

  * Added a test to ensure that it is possible to rename a branch named HEAD.

 sha1_name.c |  8 +++-
 t/t1430-bad-ref-name.sh | 29 +
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 9a2d5caf3..657a060cb 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1438,9 +1438,15 @@ int strbuf_check_branch_ref(struct strbuf *sb, const 
char *name)
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
else
strbuf_addstr(sb, name);
-   if (name[0] == '-')
+   if (*name == '-')
return -1;
+
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+   /* HEAD is not to be used as a branch name */
+   if(!strcmp(sb->buf, "refs/heads/HEAD"))
+   return -1;
+
return check_refname_format(sb->buf, 0);
 }
 
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a..421e80a7a 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,33 @@ test_expect_success 'update-ref --stdin -z fails delete 
with bad ref name' '
grep "fatal: invalid ref format: ~a" err
 '
 
+test_expect_success 'branch rejects HEAD as a branch name' '
+   test_must_fail git branch HEAD HEAD^ &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+   test_must_fail git checkout -B HEAD HEAD^ &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'update-ref can operate on refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git show-ref refs/heads/HEAD &&
+   git update-ref -d refs/heads/HEAD &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -d can remove refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git branch -d HEAD &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -m can rename refs/heads/HEAD' '
+   git update-ref refs/heads/HEAD HEAD^ &&
+   git branch -m HEAD head &&
+   test_must_fail git show-ref refs/heads/HEAD
+'
+
 test_done
-- 
2.15.0.rc2.397.geff0134c7.dirty

Re: What's cooking in git.git (Nov 2017, #04; Tue, 14)

[Kaartic Sivaraam]

* jc/branch-name-sanity (2017-10-14) 3 commits
 - branch: forbid refs/heads/HEAD
 - branch: split validate_new_branchname() into two
 - branch: streamline "attr_only" handling in validate_new_branchname()

 "git branch" and "git checkout -b" are now forbidden from creating
 a branch whose name is "HEAD".

 Reported to cause problems when renaming HEAD during a rebase.
 cf. <49563f7c-354e-334e-03a6-c3a40884b...@gmail.com>



Just wanted to note this explicitly. As I'm not aware how the problem 
with above series is going to be resolved, I've decided to stall the v4 
of my series that tries to improve error messages shown when renaming 
the branch[1] until this problem gets resolved. I'm doing this as this 
series and my series touch the same code paths. Furthermore, I based my 
v3 off of 'next' when this series was in there.


I'm not sure if the resolution to the problem might introduce conflicts 
with my series. Hence the stall.


--

[1]: 
https://public-inbox.org/git/20171102065407.25404-1-kaartic.sivar...@gmail.com/



---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Monday 13 November 2017 05:00 PM, Kevin Daudt wrote:

On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote:

That was a little attribution I wanted make to the strbuf API as this was
the first time I leveraged it to this extent and I was surprised by the way
it made string manipulation easier in C. Just documented my excitation. In
case it seems to be noise (?) which should removed, let me know.


I guess that would fit better below the the ---



That's a nice point. Thanks. (Why didn't I think of it before)


---
Kaartic

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

On Mon, 2017-11-13 at 11:32 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > I've tried to improve it, does the following paragraph sound clear
> > enough?
> > 
> > branch: group related arguments of create_branch()
> > 
> > New arguments were added to create_branch() whenever the need
> > arised and they were added to tail of the argument list. This
> > resulted in the related arguments not being close to each other.
> 
> OK, I understand what you wanted to say.  But I do not think that is
> based on a true history.
> 
>  - f9a482e6 ("checkout: suppress tracking message with "-q"",
>2012-03-26) adds 'quiet' just after 'clobber_head', exactly
>because they are related, and leaves 'track' at the end.
> 
>  - 39bd6f72 ("Allow checkout -B  to update the
>current branch", 2011-11-26) adds 'clobber_head' not at the end but
>before 'track', which is left at the end.  
> 
>  - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
>into 'start_name' and 'start_sha1' (the latter was laster removed)
>and this was not a mindless "add at the end", either.
> 
>  - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
>tracking", 2007-03-08) did add track at the end, but that is
>justifiable, as it has no relation to any other parameter.
> 

Seems I wasn't careful enough in noticing how the arguments were added.
I seemed to have overlooked the fact that 39bd6f72 added 'clobber_head'
"before" track which resulted in the vague commit message. Anyways,
thanks for taking the time to dig into this.


> You could call 39bd6f72 somewhat questionable as 'clobber_head' is
> related to 'force' more strongly than it is to 'reflog' [*1*], but
> it is unfair to blame anything else having done a mindless "add at
> the end".
> 

Yep, you're right. How does the following sound?

branch: group related arguments of create_branch()

39bd6f726 (Allow checkout -B  to update the current
branch, 2011-11-26) added 'clobber_head' (now, 'clobber_head_ok')
"before" 'track' as 'track' was closely related 'clobber_head' for
the purpose the commit wanted to achieve. Looking from the perspective
of how the arguments are used it turns out that 'clobber_head' is
more related to 'force' than it is to 'track'.

So, re-order the arguments to keep the related arguments close
to each other.

-- 
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

When trying to rename an inexistent branch to with a name of a branch


This sentence does not read well. Probably s/with a/the/ helps.



Thanks. Seems I missed it somehow. Will fix it.


that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.

[..]

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!


I'm not sure this note adds a lot, since the strbuf API is not that new.



That was a little attribution I wanted make to the strbuf API as this 
was the first time I leveraged it to this extent and I was surprised by 
the way it made string manipulation easier in C. Just documented my 
excitation. In case it seems to be noise (?) which should removed, let 
me know.


---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> No {} around a single statement block of "if", especially when there
> is no "else" that has multi-statement block that needs {}.
> 

The code has changed a little since v3 so this if has been replaced
with a switch case.


> > +   switch (res) {
> > +   case BRANCH_EXISTS_NO_FORCE:
> > +   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
> > connector_string : "");
> > +   strbuf_addf(error_msg,_("branch '%s' already exists"), 
> > newname);
> > +   break;
> 
> The case arms and their statements are indented by one level too much.
> The lines are getting overlong.  Find a good place to split, e.g.
> 
>   strbuf_addf(error_msg, "%s",
>   !old_branch_exists ? connector_string : "");
> 
> Leave a single SP after each "," in an arguments list.
> 

Fixed these.


> As Eric pointed out, this certainly smells like a sentence lego that
> we would be better off without.
> 

As stated in that mail,  I've replaced the connector " and " with "; "
as it seemed to be the most simple way to overcome the issue, at least
in my opinion. In case there's any better way to fix this let me know.


> >  static void copy_or_rename_branch(const char *oldname, const char 
> > *newname, int copy, int force)
> >  {
> > struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
> > STRBUF_INIT;
> > struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
> > int recovery = 0;
> > +   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> > +   enum branch_validation_result res;
> >  
> > if (!oldname) {
> > if (copy)
> > @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char 
> > *oldname, const char *newname, int
> > die(_("cannot rename the current branch while not on 
> > any."));
> > }
> >  
> > -   if (strbuf_check_branch_ref(, oldname)) {
> > +   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
> > +   {
> 
> Opening brace { that begins a block comes at the end of the line
> that closes the condition of "if"; if you found that your line is
> overlong, perhaps do it like so instead:
> 
>   if (strbuf_check_branch_ref(, oldname) &&
>   ref_exists(oldref.buf)) {

This part changed too. Anyways thanks for the detailed review :-)

-- 
Kaartic

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> We usually use the word "gently" to when we enhance an operation
> that used to always die on failure.  When there are tons of callsite
> to the original operation F(), we introduce F_gently() variant and
> do something like
> 
>   F(...)
>   {
>   if (F_gently(...))
>   die(...);
>   }
> 
> so that the callers do not have to change.  If there aren't that
> many, it is OK to change the function signature of F() to tell it
> not to die without adding a new F_gently() function, which is the
> approach more appropriate for this change.  The extra parameter used
> for that purpose should be called "gently", perhaps.
> 

Good suggestion, wasn't aware of it before. Renamed it.


> > +   if(ref_exists(ref->buf))
> > +   return BRANCH_EXISTS;
> > +   else
> > +   return BRANCH_DOESNT_EXIST;
> 
> Always have SP between "if" (and other keyword like "while") and its
> condition.
>
> For this one, however, this might be easier to follow:
> 
>   return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;
> 

Done.


> The names of the enum values may need further thought.  They must
> clearly convey two things, in addition to what kind of status they
> represent; the current names only convey the status.  From the names
> of these values, it must be clear that they are part of the same
> enum (e.g. by sharing a common prefix), and also from the names of
> these values, it must be clear which ones are error conditions and
> which are not, without knowing their actual values.  A reader cannot
> immediately tell from "BRANCH_EXISTS" if that is a good thing or
> not.
> 

I've added the prefix of "VALIDATION_{FAIL,PASS}" as appropriate to the
enum values. This made the names to have the form,

VALIDATION_(kind of status)_(reason)

This made the names a bit long but I couldn't come up with a better
prefix for now. Any suggestions are welcome.


Thanks for the detailed review!

---
Kaartic

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:06 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes:
> 
> > From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
> > 
> > The ad-hoc patches to add new arguments to a function when needed
> > resulted in the related arguments not being close to each other.
> > This misleads the person reading the code to believe that there isn't
> > much relation between those arguments while it's not the case.
> 
> I do not get what this wants to say.  "I am sending this ad-hoc
> patch that scrambles the order of parameters for no real reason" is
> certainly not how you are selling this step.
> 
> > So, re-order the arguments to keep the related arguments close to each
> > other.
> 
> This sentence (without "So,", obviously, because I do not get the
> previous paragraph) I do understand and agree with as a goal.
> 

I've tried to improve it, does the following paragraph sound clear
enough?

branch: group related arguments of create_branch()

New arguments were added to create_branch() whenever the need
arised and they were added to tail of the argument list. This
resulted in the related arguments not being close to each other.
This misleads the person reading the code to believe that
there isn't much relation between those arguments while it is
not the case.

So, re-order the arguments to keep the related arguments close
to each other.


> I think the only two things that should be kept together are "force"
> and "clobber_head_ok" because the previous 1/4 changed the meaning
> of "clobber_head" to "it is OK if I am renaming the currently
> checked-out branch", i.e. closer to what "force" means.
> 
> I certainly would not mind the order used in the result of this
> patch (in other words, if somebody posted a patch to add
> create_branch() function to the codebase that lacked it, with its
> parameters listed in the order this patch uses, I wouldn't
> complain), but it would have equally been OK if "reflog" and "force"
> were swapped without making any other change this patch makes.
> 

Makes sense. The unwanted shuffling was a consequence of my attempt to
see if the signature of the function did change when the position of
the 'enum' was changed. It seems there isn't change in its signature as
it is possible to use integers for enums and vice versa due to liberal
checking for misuses.

I've changed the signature back to keep alone "force" and
"clobber_head_ok" together.


Thanks,
Kaartic

Re: "git bisect" takes exactly one bad commit and one or more good?

[Kaartic Sivaraam]

On Sat, 2017-11-11 at 10:27 -0500, Robert P. J. Day wrote:
> 
>   i realize that one of each commit is the simplest use case, but the
> scenario that occurred to me is a bunch of branches being merged and,
> suddenly, you have a bug, and you're not sure where it came from so
> you identify a number of good commits, one per merged branch, and go
> from there.
> 
> 

Just thinking out loud, couldn't you give the one commit that was the
tip of the branch, to which you merged the branches, before you merged
in the branches as the good commit ?


-- 
Kaartic

Re: "Cannot fetch git.git" ( iworktrees at fault? or origin/HEAD) ?

[Kaartic Sivaraam]

On Fri, 2017-11-03 at 12:11 -0700, Stefan Beller wrote:
> On Fri, Nov 3, 2017 at 2:32 AM, Kaartic Sivaraam
> <kaartic.sivar...@gmail.com> wrote:
> > BTW, this is what I call _way way_ faster. Unfortunately due to the limited
> > configuration of my system, the test suite has following timing
> > 
> > real3m14.482s
> > user2m10.556s
> > sys 1m12.328s
> 
> This sounds to me as if it is running with just one thread
> (because sys+user = real); I usually run with
> 
> cd t
> prove --jobs 25 t[0-8][0-9]*.sh
> 
> The multithreading can be seen in the timing as well
> real 1m9.913s
> user 1m50.796s
> sys 0m54.092s
> as user > real already.
> 
> If you run tests via 'make test' or 'cd t && make', you can also give
> a --jobs 
> to make it faster. I have no good answer for how many, but I have the 
> impression
> overloading the system is no big deal. (I only have a few cores in this 
> machine,
> 4 or 6, but still run with --jobs 25; 'git grep sleep' returns a
> couple of lines,
> and such threads sleeping definitely don't need a CPU)
> 

I usually use the following command to build and run the test suite
from the root of the working directory,

make NO_GETTEXT=1 DEVELOPER=1 DEFAULT_TEST_TARGET=prove 
GIT_PROVE_OPTS="--timer --jobs 16" test

I beleived that the DEFAULT_TST_TARGE and GIT_PROVE_OPTS options would
be seen by the Makefile in t/ but didn't verify it. Your statement made
me suspicious and I wanted to know whether my assumption was correct or
not. So, I did this,


1. I first tried running the above command while seeing how much time
on the 'next' branch when it was pointing at,

9a519c715 (Merge branch 'jk/rebase-i-exec-gitdir-fix' into next, 2017-11-02)

First, I observed that at least part of my assumption held on observing
that the output was similar to that of 'prove'. This shows that
t/Makefile recognizes DEFAULT_TEST_TARGET.

Next, to verify if it recognizes GIT_PROVE_OPTS I ran the above command
with 'time' and it gave back the following statistics,

real4m55.707s
user2m29.924s
sys 1m48.072s

which is in line with the statistics for the spinning disk in the
previous mail. (The time doesn't include the build time).

(real)-(user+sys) = 38s (approx.)


2. Now I switched to the t/ directory, I tried running the following,

make DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="--timer --jobs 16"

'time' gave the statistics for the above command as follows,

real5m19.523s
user2m26.764s
sys 1m45.240s

(real)-(user+sys) = 68s (approx.)
Not a big improvement from the previous case to assure that
GIT_PROVE_OPTS wasn't recognized there.


3. Staying in the t/ directory, I tried running the following just to
be pretty sure that the "--jobs" was indeed sent to 'prove',

prove --jobs 16 t[0-9][0-9]*.sh

'time' gave the statistics for the above command as follows,

real4m49.241s
user2m29.220s
sys 1m47.828s

(real)-(user+sys) = 34s (approx.)

This seems to be identical with the first case.

So, it does seem to be a limitation of my system (Intel i3 with 2
cores, FWIW).


4. To see if things improve with a higher number of jobs I tried,

prove --jobs 25 t[0-9][0-9]*.sh

'time' gave the statistics for the above command as follows,

real5m21.229s
user2m25.704s
sys 1m46.744s

(real)-(user+sys) = 70s (approx.)

This ensured the limitation once more!

Anyways, thanks for the explanation and information which was
enlightening. :-)

-- 
Kaartic

Re: "Cannot fetch git.git" (worktrees at fault? or origin/HEAD) ?

[Kaartic Sivaraam]

On Monday 23 October 2017 11:07 PM, Stefan Beller wrote:

Exactly. By memory I mean volatile RAM (as opposed to
memory on a spinning disk).

Using GIT_TEST_OPTS has had some issues (I remember vaguely
there was an inconsistency between the output of `make test` and prove),
so I put my entire working tree on a tmpfs, I run roughly this script
after booting my computer:

   sudo mount -t tmpfs -o size=16g tmpfs /u
   mkdir /u/git
   echo "gitdir:
/usr/local/google/home/sbeller/OSS/git/.git/worktrees/git"

/u/git/.git

   git -C /u/git checkout -f HEAD

   cat 

Re: [BUG] Incosistent repository state when trying to rename HEAD in the middle of a rebase

[Kaartic Sivaraam]

On Thursday 02 November 2017 01:21 PM, Junio C Hamano wrote:

Kaartic Sivaraam <kaarticsivaraam91...@gmail.com> writes:


I was able to spare some time to dig into this and found a few things.

First, it seems that the issue is more generic and the BUG kicks in
whenever HEAD is not a symbolic ref.


Interesting.



Let me detail a little more about my observations just for the sake of 
completeness. The change that forbid refs/heads/HEAD caused issues only 
when HEAD wasn't a symbolic link because of the following reasons,


1) The change resulted in 'strbuf_check_branch_ref' returning with 
failure when the name it received (sb) was HEAD *without* interpreting 
it as "refs/heads/HEAD" into 'ref'. This resulted in the violation of 
the expectation of it's callers that it would have interpret 'ref' which 
was the major cause of the issue.


It wouldn't have been an issue if we had checked for the existence of a 
"branch" (refs/heads/) rather than checking for the existence of a "ref" 
(which allowed HEAD to pass the test).



2) This did not cause issues when HEAD was a symbolic ref because there 
was a check for attempting to rename in a symbolic ref in 
'files_copy_or_rename_ref'. The check throws an error when trying to 
rename a symbolic ref which resulted in suspicious error message I got.


So, IIUC the issue doesn't occur when 'ref' is intrepreted before the 
check for 'HEAD'. That's possibly why the diff patch I sent in the 
previous mail fixed the issue.


Also, it would be nice if we check for the existence of a "branch" when 
we want to know whether a branch exists rather than simply doing a 
'ref_exists' on the interpreted ref.




Shortly we'll be rewinding the tip of 'next', and it is a good
opportunity to kick any not-so-well-cooked topic back to 'pu',
so let's figure out what is going on after that happens (at which
point let's eject the "branch name sanity" topic out of 'next').


Does the above explanation give an idea about what's happening?


---
Kaartic

[PATCH] mailmap: use Kaartic Sivaraam's new address

[Kaartic Sivaraam]

Map the old address to the new, hopefully more permanent one.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index ab85e0d16..2634baef2 100644
--- a/.mailmap
+++ b/.mailmap
@@ -113,6 +113,7 @@ Junio C Hamano <gits...@pobox.com> <ju...@pobox.com>
 Junio C Hamano <gits...@pobox.com> <ju...@twinsun.com>
 Junio C Hamano <gits...@pobox.com> <jun...@cox.net>
 Junio C Hamano <gits...@pobox.com> <jun...@twinsun.com>
+Kaartic Sivaraam <kaartic.sivar...@gmail.com> <kaarticsivaraam91...@gmail.com>
 Karl Wiberg <k...@treskal.com> Karl  Hasselström
 Karl Wiberg <k...@treskal.com> <k...@yoghurt.hemma.treskal.com>
 Karsten Blees <bl...@dcon.de> <karsten.bl...@dcon.de>
-- 
2.15.0.461.gf957c703b.dirty

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Friday 03 November 2017 12:12 AM, Stefan Beller wrote:

On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam
<kaartic.sivar...@gmail.com> wrote:

On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote:


Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>



I just now saw this small glitch as a consequence of recently
changing my email address. I would prefer to keep the second one
but as the other patches have the first one it's better to keep
the first one for now.


If you prefer one of them, you may have incentive to
add an entry into .mailmap file, otherwise I'd kindly ask you
to. :) (c.f. `git log --no-merges -- .mailmap`)



Sure, I'll do that. My intuition says this doesn't remove the duplicated 
 sign-off line. Anyways, there's for sure a v4 that's going to update 
the connector string in [4/4] and another update. I'll be careful to 
address these issues in v4.


---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Thursday 02 November 2017 07:51 PM, Eric Sunshine wrote:


Nicely explained; easily understood.



Good to hear that.




Translators can correct me, but this smells like "sentence lego"[1],
which we'd like to avoid. Translators lack full context when presented
with bits and pieces of a sentence like this, thus the translation may
be of poor quality; it may even be entirely incorrect since they don't
necessarily know how you will be composing the various pieces.

You _might_ be able to able to resolve this by dropping "and" from:

 "foo is moo, and bar is boo"

to turn the error messages into independent clauses:

 "foo is moo; bar is boo"
> but I'm no translator, and even that may fail the lego sniff test.

Though I can't be very sure that the one you suggested will pass the 
"lego sniff test", its better than the "and" I used. Further, my 
instinct says it wouldn't qualify as sentence lego (it's just a ";").




A sure-fire way to avoid lego construction would be simply to emit
each messages on its own line:

 fatal: branch 'tset' doesn't exist
 fatal: branch 'master' already exists

(though, you might consider that too ugly).



Though it might not look that ugly, I don't know how you could make 
'git' die() twice (or am I missing something)! Of course we could use 
'error()' to report the errors and then 'die()' with a message like 
"Branch rename failed" but I find that to be a little too verbose and 
further using the connector ";" instead of "and" does seem to reduce the 
possibilities for the above program fragment to pass the "lego sniff test".


---
Kaartic

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote:

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>


I just now saw this small glitch as a consequence of recently
changing my email address. I would prefer to keep the second one
but as the other patches have the first one it's better to keep
the first one for now.

But wait, it seems that this commit also has a different author
identity (the email adress part). If this is a big enough issue,
I'll fix that and send a v4 (possibly with any other suggested
changes) else I'll leave it as it is.

BTW, I added the Ccs to this mail which I forgot to do when
sending the patches, hope it's not an issue.

---
Kaartic

Re: [RFC PATCH v3 0/4] give more useful error messages while renaming branch

[Kaartic Sivaraam]

Sorry, ignore this mails. I actually have re-sent it to the correct
thread.
 
-- 
Kaartic

[RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()'

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

The documentation for 'create_branch()' was incomplete as it didn't say
what certain parameters were used for. Further a parameter name wasn't
very communicative.

So, add missing documentation for the sake of completeness and easy
reference. Also, rename the concerned parameter to make its name more
communicative.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 branch.c | 4 ++--
 branch.h | 7 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index fe1e1c367..ea6e2b359 100644
--- a/branch.c
+++ b/branch.c
@@ -244,7 +244,7 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head,
+  int force, int reflog, int clobber_head_ok,
   int quiet, enum branch_track track)
 {
struct commit *commit;
@@ -258,7 +258,7 @@ void create_branch(const char *name, const char *start_name,
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
 
-   if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+   if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
? validate_branchname(name, )
: validate_new_branchname(name, , force)) {
if (!force)
diff --git a/branch.h b/branch.h
index be5e5d130..1512b78d1 100644
--- a/branch.h
+++ b/branch.h
@@ -15,12 +15,17 @@
  *
  *   - reflog creates a reflog for the branch
  *
+ *   - clobber_head_ok allows the currently checked out (hence existing)
+ * branch to be overwritten; without 'force', it has no effect.
+ *
+ *   - quiet suppresses tracking information
+ *
  *   - track causes the new branch to be configured to merge the remote branch
  * that start_name is a tracking branch for (if any).
  */
 void create_branch(const char *name, const char *start_name,
   int force, int reflog,
-  int clobber_head, int quiet, enum branch_track track);
+  int clobber_head_ok, int quiet, enum branch_track track);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

The ad-hoc patches to add new arguments to a function when needed
resulted in the related arguments not being close to each other.
This misleads the person reading the code to believe that there isn't
much relation between those arguments while it's not the case.

So, re-order the arguments to keep the related arguments close to each
other.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 branch.c   |  4 ++--
 branch.h   | 14 +++---
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  6 +++---
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index ea6e2b359..7c8093041 100644
--- a/branch.c
+++ b/branch.c
@@ -244,8 +244,8 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head_ok,
-  int quiet, enum branch_track track)
+  enum branch_track track, int force, int clobber_head_ok,
+  int reflog, int quiet)
 {
struct commit *commit;
struct object_id oid;
diff --git a/branch.h b/branch.h
index 1512b78d1..85052628b 100644
--- a/branch.h
+++ b/branch.h
@@ -11,21 +11,21 @@
  *   - start_name is the name of the existing branch that the new branch should
  * start from
  *
- *   - force enables overwriting an existing (non-head) branch
+ *   - track causes the new branch to be configured to merge the remote branch
+ * that start_name is a tracking branch for (if any).
  *
- *   - reflog creates a reflog for the branch
+ *   - force enables overwriting an existing (non-head) branch
  *
  *   - clobber_head_ok allows the currently checked out (hence existing)
  * branch to be overwritten; without 'force', it has no effect.
  *
- *   - quiet suppresses tracking information
+ *   - reflog creates a reflog for the branch
  *
- *   - track causes the new branch to be configured to merge the remote branch
- * that start_name is a tracking branch for (if any).
+ *   - quiet suppresses tracking information
  */
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog,
-  int clobber_head_ok, int quiet, enum branch_track track);
+  enum branch_track track, int force, int clobber_head_ok,
+  int reflog, int quiet);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index 5be40b384..df06ac968 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -766,7 +766,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 * create_branch takes care of setting up the tracking
 * info and making sure new_upstream is correct
 */
-   create_branch(branch->name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
+   create_branch(branch->name, new_upstream, 
BRANCH_TRACK_OVERRIDE, 0, 0, 0, quiet);
} else if (unset_upstream) {
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
die(_("the '--set-upstream' option is no longer 
supported. Please use '--track' or '--set-upstream-to' instead."));
 
create_branch(argv[0], (argc == 2) ? argv[1] : head,
- force, reflog, 0, quiet, track);
+ track, force, 0, reflog, quiet);
 
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8546d630b..5c34a9a0d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -639,11 +639,11 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
}
else
create_branch(opts->new_branch, new->name,
+ opts->track,
+ opts->new_branch_force ? 1 : 0,
  opts->new_branch_force ? 1 : 0,
  opts->new_branch_log,
- opts->new_branch_force ? 1 : 0,
- opts->quiet,
- opts->track);
+ opts->quiet);
new->name = opts->new_branch;
setup_branch_path(new);
}
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

This parameter allows the branchname validation functions to
optionally return a flag specifying the reason for failure, when
requested. This allows the caller to know why it was about to die.
This allows more useful error messages to be given to the user when
trying to rename a branch.

The flags are specified in the form of an enum and values for success
flags have been assigned explicitly to clearly express that certain
callers rely on those values and they cannot be arbitrary.

Only the logic has been added but no caller has been made to use it, yet.
So, no functional changes.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 branch.c   | 62 +++---
 branch.h   | 60 ++--
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  5 +++--
 4 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/branch.c b/branch.c
index 7c8093041..7db2e3296 100644
--- a/branch.c
+++ b/branch.c
@@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char 
*branch_name)
return 0;
 }
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_branchname(const char *name, struct strbuf *ref)
+enum branch_validation_result validate_branchname(const char *name, struct 
strbuf *ref, unsigned dont_fail)
 {
-   if (strbuf_check_branch_ref(ref, name))
-   die(_("'%s' is not a valid branch name."), name);
+   if (strbuf_check_branch_ref(ref, name)) {
+   if (dont_fail)
+   return INVALID_BRANCH_NAME;
+   else
+   die(_("'%s' is not a valid branch name."), name);
+   }
 
-   return ref_exists(ref->buf);
+   if(ref_exists(ref->buf))
+   return BRANCH_EXISTS;
+   else
+   return BRANCH_DOESNT_EXIST;
 }
 
-/*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+enum branch_validation_result validate_new_branchname(const char *name, struct 
strbuf *ref, int force, unsigned dont_fail)
 {
const char *head;
 
-   if (!validate_branchname(name, ref))
-   return 0;
+   if(dont_fail) {
+   enum branch_validation_result res = validate_branchname(name, 
ref, 1);
+   if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST)
+   return res;
+   } else {
+   if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST)
+   return BRANCH_DOESNT_EXIST;
+   }
 
-   if (!force)
-   die(_("A branch named '%s' already exists."),
-   ref->buf + strlen("refs/heads/"));
+   if (!force) {
+   if (dont_fail)
+   return BRANCH_EXISTS_NO_FORCE;
+   else
+   die(_("A branch named '%s' already exists."),
+   ref->buf + strlen("refs/heads/"));
+   }
 
head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-   if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-   die(_("Cannot force update the current branch."));
+   if (!is_bare_repository() && head && !strcmp(head, ref->buf)) {
+   if (dont_fail)
+   return CANNOT_FORCE_UPDATE_CURRENT_BRANCH;
+   else
+   die(_("Cannot force update the current branch."));
+   }
 
-   return 1;
+   return BRANCH_EXISTS;
 }
 
 static int check_tracking_branch(struct remote *remote, void *cb_data)
@@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name,
explicit_tracking = 1;
 
if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-   ? validate_branchname(name, )
-   : validate_new_branchname(name, , force)) {
+   ? validate_branchname(name, , 0)
+   : validate_new_branchname(name, , force, 0)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index 85052628b..0c178ec5a 100644
--- a/branch.h
+++ b/branch.h
@@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name,
   enum branch_track track, int force, int clobber_head_ok,
   in

[RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

When trying to rename an inexistent branch to with a name of a branch
that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.

$ git branch -m tset master
fatal: A branch named 'master' already exists.

It's conventional to report that 'tset' doesn't exist rather than
reporting that 'master' exists, the same way the 'mv' command does.

(hypothetical)
$ git branch -m tset master
fatal: branch 'tset' doesn't exist.

That has the problem that the error about an existing branch is shown
only after the user corrects the error about inexistent branch.

$ git branch -m test master
fatal: A branch named 'master' already exists.

This isn't useful either because the user would have corrected this error in
a single go if he had been told this alongside the first error. So, give
more useful error messages by giving errors about old branch name and new
branch name at the same time. This is possible as the branch name validation
functions now return the reason they were about to die, when requested.

$ git branch -m tset master
fatal: branch 'tset' doesn't exist, and branch 'master' already exists

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 builtin/branch.c | 49 ++---
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7018e5d75..c2bbf8c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
+static void get_error_msg(struct strbuf* error_msg, const char* oldname, 
unsigned old_branch_exists,
+ const char* newname, enum branch_validation_result 
res)
+{
+   const char* connector_string = _(", and ");
+
+   if (!old_branch_exists) {
+   strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname);
+   }
+
+   switch (res) {
+   case BRANCH_EXISTS_NO_FORCE:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addf(error_msg,_("branch '%s' already exists"), 
newname);
+   break;
+   case CANNOT_FORCE_UPDATE_CURRENT_BRANCH:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addstr(error_msg, _("cannot force update the 
current branch"));
+   break;
+   case INVALID_BRANCH_NAME:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addf(error_msg, _("branch name '%s' is 
invalid"), newname);
+   break;
+   /* not necessary to handle success cases */
+   case BRANCH_EXISTS:
+   case BRANCH_DOESNT_EXIST:
+   break;
+   }
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
+   enum branch_validation_result res;
 
if (!oldname) {
if (copy)
@@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
die(_("cannot rename the current branch while not on 
any."));
}
 
-   if (strbuf_check_branch_ref(, oldname)) {
+   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
+   {
/*
 * Bad name --- this could be an attempt to rename a
 * ref that we used to allow to be created by accident.
 */
-   if (ref_exists(oldref.buf))
-   recovery = 1;
-   else
-   die(_("Invalid branch name: '%s'"), oldname);
+   recovery = 1;
}
 
/*
@@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 * cause the worktree to become inconsistent with HEAD, so allow it.
 */
if (!strcmp(oldname, newname))
-   validate_branchname(newname, , 0);
+   res = validate_branchname(newname, , 1);
else
-   validate_new_branchname(newname, , force

[RFC PATCH v3 0/4] give more useful error messages while renaming branch

[Kaartic Sivaraam]

In builtin/branch, the error messages weren't handled directly by the branch
renaming function and was left to the other function. Though this avoids
redundancy this gave unclear error messages in some cases. So, make 
builtin/branch
give more useful error messages.

Changes in v3:

Incorporated suggestions from v2 to improve code and commit message. To
be more precise about the code part,

In 2/4 slightly re-ordered the parameters to move the flag parameters to
the end.

In 3/4, changed the return type of the branchname validation functions 
to
be the enum (whose values they return) as suggested by Stefan.

Dropped the PATCH 3/5 of v2 as there was another series[1] that did the
refactor and got merged to 'next'. I have now re-rolled the series over
'next' [pointing at 273055501 (Sync with master, 2017-10-24)].
 
This has made the code in 3/4 a little clumsy (at least to me) as I
tried to achieve to achieve what the previous patches did with the new
validate*_branchname functionS. Let me know, if it looks too bad.

So this could go on top of 'next' without any conflicts but in case I
missed something, let me know. The series could be found in my fork[2].


Any feedback welcome.

Thanks,
Kaartic

[1] : https://public-inbox.org/git/20171013051132.3973-1-gits...@pobox.com

[2] : https://github.com/sivaraam/git/tree/work/branch-revamp


Kaartic Sivaraam (4):
  branch: improve documentation and naming of 'create_branch()'
  branch: re-order function arguments to group related arguments
  branch: introduce dont_fail parameter for branchname validation
  builtin/branch: give more useful error messages when renaming

 branch.c   | 63 ++
 branch.h   | 57 ++--
 builtin/branch.c   | 49 ++
 builtin/checkout.c | 11 +-
 4 files changed, 127 insertions(+), 53 deletions(-)

-- 
2.15.0.rc2.401.g3db9995f9

[RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

When trying to rename an inexistent branch to with a name of a branch
that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.

$ git branch -m tset master
fatal: A branch named 'master' already exists.

It's conventional to report that 'tset' doesn't exist rather than
reporting that 'master' exists, the same way the 'mv' command does.

(hypothetical)
$ git branch -m tset master
fatal: branch 'tset' doesn't exist.

That has the problem that the error about an existing branch is shown
only after the user corrects the error about inexistent branch.

$ git branch -m test master
fatal: A branch named 'master' already exists.

This isn't useful either because the user would have corrected this error in
a single go if he had been told this alongside the first error. So, give
more useful error messages by giving errors about old branch name and new
branch name at the same time. This is possible as the branch name validation
functions now return the reason they were about to die, when requested.

$ git branch -m tset master
fatal: branch 'tset' doesn't exist, and branch 'master' already exists

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 builtin/branch.c | 49 ++---
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7018e5d75..c2bbf8c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
+static void get_error_msg(struct strbuf* error_msg, const char* oldname, 
unsigned old_branch_exists,
+ const char* newname, enum branch_validation_result 
res)
+{
+   const char* connector_string = _(", and ");
+
+   if (!old_branch_exists) {
+   strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname);
+   }
+
+   switch (res) {
+   case BRANCH_EXISTS_NO_FORCE:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addf(error_msg,_("branch '%s' already exists"), 
newname);
+   break;
+   case CANNOT_FORCE_UPDATE_CURRENT_BRANCH:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addstr(error_msg, _("cannot force update the 
current branch"));
+   break;
+   case INVALID_BRANCH_NAME:
+   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
connector_string : "");
+   strbuf_addf(error_msg, _("branch name '%s' is 
invalid"), newname);
+   break;
+   /* not necessary to handle success cases */
+   case BRANCH_EXISTS:
+   case BRANCH_DOESNT_EXIST:
+   break;
+   }
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
+   enum branch_validation_result res;
 
if (!oldname) {
if (copy)
@@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
die(_("cannot rename the current branch while not on 
any."));
}
 
-   if (strbuf_check_branch_ref(, oldname)) {
+   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
+   {
/*
 * Bad name --- this could be an attempt to rename a
 * ref that we used to allow to be created by accident.
 */
-   if (ref_exists(oldref.buf))
-   recovery = 1;
-   else
-   die(_("Invalid branch name: '%s'"), oldname);
+   recovery = 1;
}
 
/*
@@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 * cause the worktree to become inconsistent with HEAD, so allow it.
 */
if (!strcmp(oldname, newname))
-   validate_branchname(newname, , 0);
+   res = validate_branchname(newname, , 1);
else
-   validate_new_branchname(newname, , force

[RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

The ad-hoc patches to add new arguments to a function when needed
resulted in the related arguments not being close to each other.
This misleads the person reading the code to believe that there isn't
much relation between those arguments while it's not the case.

So, re-order the arguments to keep the related arguments close to each
other.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 branch.c   |  4 ++--
 branch.h   | 14 +++---
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  6 +++---
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index ea6e2b359..7c8093041 100644
--- a/branch.c
+++ b/branch.c
@@ -244,8 +244,8 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head_ok,
-  int quiet, enum branch_track track)
+  enum branch_track track, int force, int clobber_head_ok,
+  int reflog, int quiet)
 {
struct commit *commit;
struct object_id oid;
diff --git a/branch.h b/branch.h
index 1512b78d1..85052628b 100644
--- a/branch.h
+++ b/branch.h
@@ -11,21 +11,21 @@
  *   - start_name is the name of the existing branch that the new branch should
  * start from
  *
- *   - force enables overwriting an existing (non-head) branch
+ *   - track causes the new branch to be configured to merge the remote branch
+ * that start_name is a tracking branch for (if any).
  *
- *   - reflog creates a reflog for the branch
+ *   - force enables overwriting an existing (non-head) branch
  *
  *   - clobber_head_ok allows the currently checked out (hence existing)
  * branch to be overwritten; without 'force', it has no effect.
  *
- *   - quiet suppresses tracking information
+ *   - reflog creates a reflog for the branch
  *
- *   - track causes the new branch to be configured to merge the remote branch
- * that start_name is a tracking branch for (if any).
+ *   - quiet suppresses tracking information
  */
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog,
-  int clobber_head_ok, int quiet, enum branch_track track);
+  enum branch_track track, int force, int clobber_head_ok,
+  int reflog, int quiet);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index 5be40b384..df06ac968 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -766,7 +766,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 * create_branch takes care of setting up the tracking
 * info and making sure new_upstream is correct
 */
-   create_branch(branch->name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
+   create_branch(branch->name, new_upstream, 
BRANCH_TRACK_OVERRIDE, 0, 0, 0, quiet);
} else if (unset_upstream) {
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
die(_("the '--set-upstream' option is no longer 
supported. Please use '--track' or '--set-upstream-to' instead."));
 
create_branch(argv[0], (argc == 2) ? argv[1] : head,
- force, reflog, 0, quiet, track);
+ track, force, 0, reflog, quiet);
 
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8546d630b..5c34a9a0d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -639,11 +639,11 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
}
else
create_branch(opts->new_branch, new->name,
+ opts->track,
+ opts->new_branch_force ? 1 : 0,
  opts->new_branch_force ? 1 : 0,
  opts->new_branch_log,
- opts->new_branch_force ? 1 : 0,
- opts->quiet,
- opts->track);
+ opts->quiet);
new->name = opts->new_branch;
setup_branch_path(new);
}
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()'

[Kaartic Sivaraam]

From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>

The documentation for 'create_branch()' was incomplete as it didn't say
what certain parameters were used for. Further a parameter name wasn't
very communicative.

So, add missing documentation for the sake of completeness and easy
reference. Also, rename the concerned parameter to make its name more
communicative.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
---
 branch.c | 4 ++--
 branch.h | 7 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index fe1e1c367..ea6e2b359 100644
--- a/branch.c
+++ b/branch.c
@@ -244,7 +244,7 @@ N_("\n"
 "\"git push -u\" to set the upstream config as you push.");
 
 void create_branch(const char *name, const char *start_name,
-  int force, int reflog, int clobber_head,
+  int force, int reflog, int clobber_head_ok,
   int quiet, enum branch_track track)
 {
struct commit *commit;
@@ -258,7 +258,7 @@ void create_branch(const char *name, const char *start_name,
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
 
-   if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+   if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
? validate_branchname(name, )
: validate_new_branchname(name, , force)) {
if (!force)
diff --git a/branch.h b/branch.h
index be5e5d130..1512b78d1 100644
--- a/branch.h
+++ b/branch.h
@@ -15,12 +15,17 @@
  *
  *   - reflog creates a reflog for the branch
  *
+ *   - clobber_head_ok allows the currently checked out (hence existing)
+ * branch to be overwritten; without 'force', it has no effect.
+ *
+ *   - quiet suppresses tracking information
+ *
  *   - track causes the new branch to be configured to merge the remote branch
  * that start_name is a tracking branch for (if any).
  */
 void create_branch(const char *name, const char *start_name,
   int force, int reflog,
-  int clobber_head, int quiet, enum branch_track track);
+  int clobber_head_ok, int quiet, enum branch_track track);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
-- 
2.15.0.461.gf957c703b.dirty

[RFC PATCH v3 0/4] give more useful error messages while renaming branch

[Kaartic Sivaraam]

In builtin/branch, the error messages weren't handled directly by the branch
renaming function and was left to the other function. Though this avoids
redundancy this gave unclear error messages in some cases. So, make 
builtin/branch
give more useful error messages.

Changes in v3:

Incorporated suggestions from v2 to improve code and commit message. To
be more precise about the code part,

In 2/4 slightly re-ordered the parameters to move the flag parameters to
the end.

In 3/4, changed the return type of the branchname validation functions 
to
be the enum (whose values they return) as suggested by Stefan.

Dropped the PATCH 3/5 of v2 as there was another series[1] that did the
refactor and got merged to 'next'. I have now re-rolled the series over
'next' [pointing at 273055501 (Sync with master, 2017-10-24)].
 
This has made the code in 3/4 a little clumsy (at least to me) as I
tried to achieve to achieve what the previous patches did with the new
validate*_branchname functionS. Let me know, if it looks too bad.

So this could go on top of 'next' without any conflicts but in case I
missed something, let me know. The series could be found in my fork[2].


Any feedback welcome.

Thanks,
Kaartic

[1] : https://public-inbox.org/git/20171013051132.3973-1-gits...@pobox.com

[2] : https://github.com/sivaraam/git/tree/work/branch-revamp


Kaartic Sivaraam (4):
  branch: improve documentation and naming of 'create_branch()'
  branch: re-order function arguments to group related arguments
  branch: introduce dont_fail parameter for branchname validation
  builtin/branch: give more useful error messages when renaming

 branch.c   | 63 ++
 branch.h   | 57 ++--
 builtin/branch.c   | 49 ++
 builtin/checkout.c | 11 +-
 4 files changed, 127 insertions(+), 53 deletions(-)

-- 
2.15.0.rc2.401.g3db9995f9

[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

This parameter allows the branchname validation functions to
optionally return a flag specifying the reason for failure, when
requested. This allows the caller to know why it was about to die.
This allows more useful error messages to be given to the user when
trying to rename a branch.

The flags are specified in the form of an enum and values for success
flags have been assigned explicitly to clearly express that certain
callers rely on those values and they cannot be arbitrary.

Only the logic has been added but no caller has been made to use it, yet.
So, no functional changes.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 branch.c   | 62 +++---
 branch.h   | 60 ++--
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  5 +++--
 4 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/branch.c b/branch.c
index 7c8093041..7db2e3296 100644
--- a/branch.c
+++ b/branch.c
@@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char 
*branch_name)
return 0;
 }
 
-/*
- * Check if 'name' can be a valid name for a branch; die otherwise.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_branchname(const char *name, struct strbuf *ref)
+enum branch_validation_result validate_branchname(const char *name, struct 
strbuf *ref, unsigned dont_fail)
 {
-   if (strbuf_check_branch_ref(ref, name))
-   die(_("'%s' is not a valid branch name."), name);
+   if (strbuf_check_branch_ref(ref, name)) {
+   if (dont_fail)
+   return INVALID_BRANCH_NAME;
+   else
+   die(_("'%s' is not a valid branch name."), name);
+   }
 
-   return ref_exists(ref->buf);
+   if(ref_exists(ref->buf))
+   return BRANCH_EXISTS;
+   else
+   return BRANCH_DOESNT_EXIST;
 }
 
-/*
- * Check if a branch 'name' can be created as a new branch; die otherwise.
- * 'force' can be used when it is OK for the named branch already exists.
- * Return 1 if the named branch already exists; return 0 otherwise.
- * Fill ref with the full refname for the branch.
- */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+enum branch_validation_result validate_new_branchname(const char *name, struct 
strbuf *ref, int force, unsigned dont_fail)
 {
const char *head;
 
-   if (!validate_branchname(name, ref))
-   return 0;
+   if(dont_fail) {
+   enum branch_validation_result res = validate_branchname(name, 
ref, 1);
+   if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST)
+   return res;
+   } else {
+   if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST)
+   return BRANCH_DOESNT_EXIST;
+   }
 
-   if (!force)
-   die(_("A branch named '%s' already exists."),
-   ref->buf + strlen("refs/heads/"));
+   if (!force) {
+   if (dont_fail)
+   return BRANCH_EXISTS_NO_FORCE;
+   else
+   die(_("A branch named '%s' already exists."),
+   ref->buf + strlen("refs/heads/"));
+   }
 
head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-   if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-   die(_("Cannot force update the current branch."));
+   if (!is_bare_repository() && head && !strcmp(head, ref->buf)) {
+   if (dont_fail)
+   return CANNOT_FORCE_UPDATE_CURRENT_BRANCH;
+   else
+   die(_("Cannot force update the current branch."));
+   }
 
-   return 1;
+   return BRANCH_EXISTS;
 }
 
 static int check_tracking_branch(struct remote *remote, void *cb_data)
@@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name,
explicit_tracking = 1;
 
if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
-   ? validate_branchname(name, )
-   : validate_new_branchname(name, , force)) {
+   ? validate_branchname(name, , 0)
+   : validate_new_branchname(name, , force, 0)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index 85052628b..0c178ec5a 100644
--- a/branch.h
+++ b/branch.h
@@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name,
   enum branch_track track, int force, int clobber_head_ok,
   in

Re: [BUG] Incosistent repository state when trying to rename HEAD in the middle of a rebase

[Kaartic Sivaraam]

I was able to spare some time to dig into this and found a few things.

First, it seems that the issue is more generic and the BUG kicks in
whenever HEAD is not a symbolic ref. I noticed that because when HEAD
is a symbolic ref there was a change in the error message shown by git.
In (2.11.0) I get this error message,

error: refname refs/heads/HEAD not found
fatal: Branch rename failed

while in the version build from 'next', I get the following error
message,

error: refname HEAD is a symbolic ref, renaming it is not supported
fatal: Branch rename failed

That made me suspicious and I wanted to find where the error message
got changed and bisected this which pointed to,

9416812db (branch: forbid refs/heads/HEAD, 2017-10-13)

This is the same commit which also causes the bug of allowing HEAD to
be renamed when it is not a symbolic ref. I found a way to fix this but
am not so sure if it's the right way to do this. (the diff patch is
found at the end of this mail).

One more observation I made was that without the patch at the end it is
NOT possible to rename a branch named "HEAD" created using the older
version!

On Sat, 2017-10-28 at 22:28 +0530, Kaartic Sivaraam wrote:
> git rebase-i HEAD~
> 

Small correction here. Now you could replace that with the simpler,

git checkout HEAD^



diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376..4345e14c9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1334,7 +1334,13 @@ int strbuf_check_branch_ref(struct strbuf *sb, const 
char *name)
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
if (name[0] == '-')
return -1;
+
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+   /* HEAD is not to be used as a branch name */
+   if(!strcmp(sb->buf, "refs/heads/HEAD"))
+   return -1;
+
return check_refname_format(sb->buf, 0);
 }
 

HTH,
Kaartic

Re: What's cooking in git.git (Oct 2017, #06; Fri, 27)

[Kaartic Sivaraam]

On Fri, 2017-10-27 at 17:32 +0900, Junio C Hamano wrote:
> * jc/branch-name-sanity (2017-10-14) 3 commits
>   (merged to 'next' on 2017-10-16 at 174646d1c3)
>  + branch: forbid refs/heads/HEAD
>  + branch: split validate_new_branchname() into two
>  + branch: streamline "attr_only" handling in validate_new_branchname()
>  "git branch" and "git checkout -b" are now forbidden from creating
>  a branch whose name is "HEAD".
> 
>  Will cook in 'next'.
> 
> 

Good thing this is still cooking in 'next'. I guess there's a bug as a
consequence of this series (though that's just a guess and has not been
confirmed by a bisection). See,

https://public-inbox.org/git/1509209933.2256.4.ca...@gmail.com/T/#u


-- 
Kaartic

[BUG] Incosistent repository state when trying to rename HEAD in the middle of a rebase

[Kaartic Sivaraam]

I just noticed this recently while trying to see if a recent change [1]
that disallowed the possibility of creating HEAD also allowed renaming
branches named "HEAD" that were created using previous versions that
allowed it. Unfortunately (or fortunately (?)), I was in the middle of
an interactive rebase while trying this out and as a consequence
observed weird behaviour as shown in the following output,

$ git branch -m HEAD head-1
warning: Renamed a misnamed branch '|�?' away

The most interesting thing with the above output was that I really
didn't have any branch named "HEAD" while trying this. Further, the
most crucial thing about the above issue is it left the repository in
an inconsistent state by removing ".git/HEAD" and thus leaving the
repository in an inconsistent state. This results in git not
recognizing it as a git repository itself! I had to do a "git init" to
fix this (I guess I lose at least something as a consequence as it
checks out the 'master' by default; though I'm not sure).

git 2.11.0 shows a "branch doesn't exist" error (not exactly) in the
above case,

$ hgit b -m HEAD head-1
error: refname refs/heads/HEAD not found
fatal: Branch rename failed

My reproduction recipe for the above issue is as follows,

git init test &&
cd test &&
echo "Test file" >test &&
git add test &&
git commit -m "Initial commit" & "Test" >test & add test &&
git c -m "Second commit" &&
git rebase-i HEAD~

At this point use the 'edit' command for "Second commit" and close the
editor and try the following,

$ git branch -m HEAD head-1

This should show the issue. Any git operation after this should report
the following error,

fatal: Not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)

Re-initializing the repository should bring the repository back to the
position it was before the issue (to some extent though the staging
area might become cluttered as the 'master' branch is checked out by
default.)

I'm still surprised as to why ".git/HEAD" was treated as a branch. I
thought of digging into this but didn't get the time now so thought of
informing to the people in the mailing list. One last thing, I suspect
this to be a consequence of that change I specified in the beginning,
though I might be just guessing around.

[1]: https://public-inbox.org/git/20171013051132.3973-1-gits...@pobox.com/

-- 
Kaartic

Re: [RFC PATCH v2 5/5] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Mon, 2017-10-23 at 12:44 -0700, Stefan Beller wrote:
> +static void get_error_msg(struct strbuf* error_msg, const char* oldname, 
> unsigned old_branch_exists,
> > + const char* newname, int 
> > new_branch_validation_result)
> 
> nit here and in the return of validate_branch_creation:
> It would be clearer if this is not just 'int', but actually spelling
> out that it is the enum.

Thanks. That's a good suggestion. I'll fix it while dropping [PATCH
3/5] that cleans up the 'validate_new_branchname' function as there's
already another series that refactored the same function and got merged
to 'next',

https://public-inbox.org/git/20171013051132.3973-1-gits...@pobox.com/


-- 
Kaartic

Re: "Cannot fetch git.git" (worktrees at fault? or origin/HEAD) ?

[Kaartic Sivaraam]

On Fri, 2017-10-20 at 13:45 -0700, Stefan Beller wrote:
> 
> The git-test from Michael sounds intriguing. Initially I put off using
> it as I had my main working dir (or rather test dir) on a spinning
> disk, still. Now I test in memory only, which is a lot faster, so I could
> try if git-test can keep up with my local commit pace.
> 

Excuse my ignorance but I don't get your statement correctly. What do
you mean by "I test in memory only"? How do you do that?

---
Kaartic