Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Johannes Schindelin]

Hi Junio,

On Sat, 16 Sep 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Fri, 15 Sep 2017, Junio C Hamano wrote:
> >
> >> --
> >> [Cooking]
> >> 
> >> [...]
> >> 
> >> * mk/diff-delta-uint-may-be-shorter-than-ulong (2017-08-10) 1 commit
> >>  ...
> >>  Dropped, as it was rerolled for review as part of a larger series.
> >>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> >> 
> >> [...]
> >> 
> >> * mk/use-size-t-in-zlib (2017-08-10) 1 commit
> >>  ...
> >>  Dropped, as it was rerolled for review as part of a larger series.
> >>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> >> 
> >> 
> >> * mk/diff-delta-avoid-large-offset (2017-08-11) 1 commit
> >>  ...
> >>  Dropped, as it was rerolled for review as part of a larger series.
> >>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> >> 
> >> 
> >> --
> >> [Discarded]
> >> 
> >> [...]
> >
> > These three topics are still in the wrong category. Please fix.
> 
> Hmph, but did the larger series these refer to actually land?

As I have to read these long mails to keep track of the current status of
some submissions, I do not care. However, in the context of this mail, it
does not make sense to have discarded patch series in the cooking section.
It's simply confusing.

Ciao,
Dscho

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Sahil Dua  writes:

> On Fri, Sep 22, 2017 at 5:39 AM, Junio C Hamano  wrote:
>>
>> So here is such an update.  As the topic is not in 'next' yet, it
>> could also be implemented by replacing patch(es) in the series, but
>> doing it as a follow-up fix made it easier to see what got changed
>> (both in the code and in the tests), so that is how I decided to do
>> this patch.
>>
> Awesome! Thanks for the patch. It was easier than I'd have expected it
> to be. Looks like it fixes the concerns of moving head. Is there
> anythign required from my side on this features / series of patches?

If you now agree with the updated behaviour and think it makes more
sense that "git branch -c new HEAD" does not check out 'new', then
there is nothing required.  Well, other than the usual finding,
reporting and optionally fixing bugs in the documentation and the
code, that is ;-)

Thanks.

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Sahil Dua]

On Fri, Sep 22, 2017 at 5:39 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> My understanding of the next step was for those who are interested
>> in moving this topic forward to update these patches in that
>> direction.
>
> Well, I am one of those who are interested in moving this topic
> forward, not because I'm likely to use it, but because the fewer
> number of topics I have to keep in flight, the easier my work gets.
>
> So here is such an update.  As the topic is not in 'next' yet, it
> could also be implemented by replacing patch(es) in the series, but
> doing it as a follow-up fix made it easier to see what got changed
> (both in the code and in the tests), so that is how I decided to do
> this patch.
>
Awesome! Thanks for the patch. It was easier than I'd have expected it
to be. Looks like it fixes the concerns of moving head. Is there
anythign required from my side on this features / series of patches?
> -- >8 --
> Subject: [PATCH] branch: fix "copy" to never touch HEAD
>
> Probably because "git branch -c A B" piggybacked its implementation
> on "git branch -m A B", when creating a new branch B by copying the
> branch A that happens to be the current branch, it also updated HEAD
> to point at the new branch.
>
> This does not match the usual expectation.  If I were sitting on a
> blue chair, and somebody comes and repaints it to red, I would
> accept ending up sitting on a red chair, but if somebody creates a
> new red chair, modelling it after the blue chair I am sitting on, I
> do not expect to be booted off of the blue chair and ending up on
> sitting on the red one.
>
> Let's fix this strange behaviour before it hits 'next'.  Those who
> want to create a new branch and switch to it can do "git checkout B"
> after creating it by copying the current branch, or if that is so
> useful to deserve a short-hand way to do so, perhaps extend "git
> checkout -b B" to copy configurations while creating the new branch
> B.  A "copy" should remain to be "copy", not "copy and sometimes
> checkout".
>
> Signed-off-by: Junio C Hamano 
> ---
>  builtin/branch.c  |  9 +++--
>  t/t3200-branch.sh | 10 +-
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 89f64f4123..e2e3692838 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -506,12 +506,9 @@ static void copy_or_rename_branch(const char *oldname, 
> const char *newname, int
> oldref.buf + 11);
> }
>
> -   if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf)) {
> -   if (copy)
> -   die(_("Branch copied to %s, but HEAD is not 
> updated!"), newname);
> -   else
> -   die(_("Branch renamed to %s, but HEAD is not 
> updated!"), newname);
> -   }
> +   if (!copy &&
> +   replace_each_worktree_head_symref(oldref.buf, newref.buf, 
> logmsg.buf))
> +   die(_("Branch renamed to %s, but HEAD is not updated!"), 
> newname);
>
> strbuf_release();
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 5d03ad16f6..be9b3784c6 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -422,7 +422,7 @@ test_expect_success 'git branch --copy is a synonym for 
> -c' '
> test_cmp expect actual
>  '
>
> -test_expect_success 'git branch -c ee ef should copy and checkout branch ef' 
> '
> +test_expect_success 'git branch -c ee ef should copy to create branch ef' '
> git checkout -b ee &&
> git reflog exists refs/heads/ee &&
> git config branch.ee.dummy Hello &&
> @@ -431,7 +431,7 @@ test_expect_success 'git branch -c ee ef should copy and 
> checkout branch ef' '
> git reflog exists refs/heads/ef &&
> test $(git config branch.ee.dummy) = Hello &&
> test $(git config branch.ef.dummy) = Hello &&
> -   test $(git rev-parse --abbrev-ref HEAD) = ef
> +   test $(git rev-parse --abbrev-ref HEAD) = ee
>  '
>
>  test_expect_success 'git branch -c f/f g/g should work' '
> @@ -494,12 +494,12 @@ test_expect_success 'git branch -C c1 c2 should succeed 
> when c1 is checked out'
> git checkout -b c1 &&
> git branch c2 &&
> git branch -C c1 c2 &&
> -   test $(git rev-parse --abbrev-ref HEAD) = c2
> +   test $(git rev-parse --abbrev-ref HEAD) = c1
>  '
>
> -test_expect_success 'git branch -C c1 c2 should add entries to 
> .git/logs/HEAD' '
> +test_expect_success 'git branch -C c1 c2 should never touch HEAD' '
> msg="Branch: copied refs/heads/c1 to refs/heads/c2" &&
> -   grep "$msg$" .git/logs/HEAD
> +   ! grep "$msg$" .git/logs/HEAD
>  '
>
>  test_expect_success 'git branch -C master should work when master is checked 
> out' '
> --
> 2.14.1-907-g5aa63875cf
>
>
>



-- 
Regards
Sahil Dua
Software Developer
Booking.com
Connect on LinkedIn
www.sahildua.com

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Junio C Hamano  writes:

> My understanding of the next step was for those who are interested
> in moving this topic forward to update these patches in that
> direction.

Well, I am one of those who are interested in moving this topic
forward, not because I'm likely to use it, but because the fewer
number of topics I have to keep in flight, the easier my work gets.

So here is such an update.  As the topic is not in 'next' yet, it
could also be implemented by replacing patch(es) in the series, but
doing it as a follow-up fix made it easier to see what got changed
(both in the code and in the tests), so that is how I decided to do
this patch.

-- >8 --
Subject: [PATCH] branch: fix "copy" to never touch HEAD

Probably because "git branch -c A B" piggybacked its implementation
on "git branch -m A B", when creating a new branch B by copying the
branch A that happens to be the current branch, it also updated HEAD
to point at the new branch.

This does not match the usual expectation.  If I were sitting on a
blue chair, and somebody comes and repaints it to red, I would
accept ending up sitting on a red chair, but if somebody creates a
new red chair, modelling it after the blue chair I am sitting on, I
do not expect to be booted off of the blue chair and ending up on
sitting on the red one.

Let's fix this strange behaviour before it hits 'next'.  Those who
want to create a new branch and switch to it can do "git checkout B"
after creating it by copying the current branch, or if that is so
useful to deserve a short-hand way to do so, perhaps extend "git
checkout -b B" to copy configurations while creating the new branch
B.  A "copy" should remain to be "copy", not "copy and sometimes
checkout".

Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  |  9 +++--
 t/t3200-branch.sh | 10 +-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 89f64f4123..e2e3692838 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -506,12 +506,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
oldref.buf + 11);
}
 
-   if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf)) {
-   if (copy)
-   die(_("Branch copied to %s, but HEAD is not updated!"), 
newname);
-   else
-   die(_("Branch renamed to %s, but HEAD is not 
updated!"), newname);
-   }
+   if (!copy &&
+   replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf))
+   die(_("Branch renamed to %s, but HEAD is not updated!"), 
newname);
 
strbuf_release();
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5d03ad16f6..be9b3784c6 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -422,7 +422,7 @@ test_expect_success 'git branch --copy is a synonym for -c' 
'
test_cmp expect actual
 '
 
-test_expect_success 'git branch -c ee ef should copy and checkout branch ef' '
+test_expect_success 'git branch -c ee ef should copy to create branch ef' '
git checkout -b ee &&
git reflog exists refs/heads/ee &&
git config branch.ee.dummy Hello &&
@@ -431,7 +431,7 @@ test_expect_success 'git branch -c ee ef should copy and 
checkout branch ef' '
git reflog exists refs/heads/ef &&
test $(git config branch.ee.dummy) = Hello &&
test $(git config branch.ef.dummy) = Hello &&
-   test $(git rev-parse --abbrev-ref HEAD) = ef
+   test $(git rev-parse --abbrev-ref HEAD) = ee
 '
 
 test_expect_success 'git branch -c f/f g/g should work' '
@@ -494,12 +494,12 @@ test_expect_success 'git branch -C c1 c2 should succeed 
when c1 is checked out'
git checkout -b c1 &&
git branch c2 &&
git branch -C c1 c2 &&
-   test $(git rev-parse --abbrev-ref HEAD) = c2
+   test $(git rev-parse --abbrev-ref HEAD) = c1
 '
 
-test_expect_success 'git branch -C c1 c2 should add entries to .git/logs/HEAD' 
'
+test_expect_success 'git branch -C c1 c2 should never touch HEAD' '
msg="Branch: copied refs/heads/c1 to refs/heads/c2" &&
-   grep "$msg$" .git/logs/HEAD
+   ! grep "$msg$" .git/logs/HEAD
 '
 
 test_expect_success 'git branch -C master should work when master is checked 
out' '
-- 
2.14.1-907-g5aa63875cf

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Sahil Dua  writes:

>>
>> * sd/branch-copy (2017-06-18) 3 commits
>>  - branch: add a --copy (-c) option to go with --move (-m)
>>  - branch: add test for -m renaming multiple config sections
>>  - config: create a function to format section headers
>>
>>  "git branch" learned "-c/-C" to create and switch to a new branch
>>  by copying an existing one.
>>
>>  I personally do not think "branch --copy master backup" while on
>>  "master" that switches to "backup" is a good UI, and I *will* say
>>  "I told you so" when users complain after we merge this down to
>>  'master'.
>
> Junio, what's up with this one? It's been a long time for this being
> cooked in next. Do we have a plan/idea going forward or is it just
> waiting for more feedback?

Thanks for pinging.

I was (in a strange way) hoping that it was just me who felt that a
"copy" operation of the current branch that moves me to a new branch
is a design mistake, and I was planning to start merging this down
to 'next' based on that assumption, but IIRC it turned out that it
wasn't just me.  During the renewed discussion I somehow got an
impression that the concensus was for "copy" to just copy without
ever changing what HEAD says, and if an operation that switches to a
new branch based on the current branch is needed, the right place to
do so is to enhance/extend "checkout -b".

My understanding of the next step was for those who are interested
in moving this topic forward to update these patches in that
direction.

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Sahil Dua]

>
> * sd/branch-copy (2017-06-18) 3 commits
>  - branch: add a --copy (-c) option to go with --move (-m)
>  - branch: add test for -m renaming multiple config sections
>  - config: create a function to format section headers
>
>  "git branch" learned "-c/-C" to create and switch to a new branch
>  by copying an existing one.
>
>  I personally do not think "branch --copy master backup" while on
>  "master" that switches to "backup" is a good UI, and I *will* say
>  "I told you so" when users complain after we merge this down to
>  'master'.

Junio, what's up with this one? It's been a long time for this being
cooked in next. Do we have a plan/idea going forward or is it just
waiting for more feedback?

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Lars Schneider  writes:

> SZEDER noticing a bug in this series that I was about to fix:
> https://public-inbox.org/git/3b175d35-5b1c-43cd-a7e9-85693335b...@gmail.com/
>
> I assume at this point a new patch is better than a re-roll, right?

Thanks, yes indeed.

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Fri, 15 Sep 2017, Junio C Hamano wrote:
>
>> --
>> [Cooking]
>> 
>> [...]
>> 
>> * mk/diff-delta-uint-may-be-shorter-than-ulong (2017-08-10) 1 commit
>>  ...
>>  Dropped, as it was rerolled for review as part of a larger series.
>>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
>> 
>> [...]
>> 
>> * mk/use-size-t-in-zlib (2017-08-10) 1 commit
>>  ...
>>  Dropped, as it was rerolled for review as part of a larger series.
>>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
>> 
>> 
>> * mk/diff-delta-avoid-large-offset (2017-08-11) 1 commit
>>  ...
>>  Dropped, as it was rerolled for review as part of a larger series.
>>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
>> 
>> 
>> --
>> [Discarded]
>> 
>> [...]
>
> These three topics are still in the wrong category. Please fix.

Hmph, but did the larger series these refer to actually land?  If I
recall correctly they were too invasive to get merged cleanly to
'next' and 'pu' without disrupting topics in flight and that is why
it is not even listed there.

The only two reasons a topic with a good goal and approach gets
thrown into the Discarded bin are either because it is tentatively
retracted to avoid disrupting other topics in flight (with the
expectation to be redone later) or what it wants to solve is
addressed by another topic and becomes unnecessary.

These three attempt to do good things and I have been hoping that
others can help moving them forward by e.g. reporting that they
still cleanly merge and stand on their own as improvements at a
smaller scale than the larger one that was attempted but was not
queued, or if nobody volunteers, I was guessing that I might end up
doing that myself.  Before that happens, I'd prefer to keep them
listed and topics kept in my tree, even if they are not part of
'pu'.

On the other hand, because many topics have graduated to master
recently, the larger one (possibly in the updated form---I do not
recall what conflicts it had with what other topics) may be able to
cook peacefully together with topics we have that are still in
flight.  If that is the case, then that larger topic will be queued
and these three will truly become material for the Discarded bin.

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Junio C Hamano]

Johannes Schindelin  writes:

> Please stop stating that you expect a reroll for rebase-i-extra when you
> explicitly stated months ago that you would not take my v6. It gets a bit
> annoying.

I already explained to you why I skipped v6, which turned to be
identical to v5 when the unnecessary rebase was undone.  It does not
have anything to do with expecting a v7 or later to fix other things
already pointed out in the review.

Having said that, I am getting sick of your constant hostile
whining, and am inclined to merge it as-is to 'next' and advance it
from there, if only to reduce these noises from you.  Even if you
are unwilling to, you do not have the sole possession and veto power
over the area of the code (neither do I) to prevent further
improvements, so I can expect others with good design taste
(hopefully not me) to fix up the misuse of revision traversal
implementation detail with follow-up patches.

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Johannes Schindelin]

Hi Junio,

On Fri, 15 Sep 2017, Junio C Hamano wrote:

> --
> [Cooking]
> 
> [...]
> 
> * mk/diff-delta-uint-may-be-shorter-than-ulong (2017-08-10) 1 commit
>  . diff-delta: fix encoding size that would not fit in "unsigned int"
> 
>  The machinery to create xdelta used in pack files received the
>  sizes of the data in size_t, but lost the higher bits of them by
>  storing them in "unsigned int" during the computation, which is
>  fixed.
> 
>  Dropped, as it was rerolled for review as part of a larger series.
>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> 
> [...]
> 
> * mk/use-size-t-in-zlib (2017-08-10) 1 commit
>  . zlib.c: use size_t for size
> 
>  The wrapper to call into zlib followed our long tradition to use
>  "unsigned long" for sizes of regions in memory, which have been
>  updated to use "size_t".
> 
>  Dropped, as it was rerolled for review as part of a larger series.
>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> 
> 
> * mk/diff-delta-avoid-large-offset (2017-08-11) 1 commit
>  . diff-delta: do not allow delta offset truncation
> 
>  The delta format used in the packfile cannot reference data at
>  offset larger than what can be expressed in 4-byte, but the
>  generator for the data failed to make sure the offset does not
>  overflow.  This has been corrected.
> 
>  Dropped, as it was rerolled for review as part of a larger series.
>  cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause>
> 
> 
> --
> [Discarded]
> 
> [...]

These three topics are still in the wrong category. Please fix.

Ciao,
Dscho

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Johannes Schindelin]

Hi Junio,

On Fri, 15 Sep 2017, Junio C Hamano wrote:

> * js/rebase-i-final (2017-07-27) 10 commits
>  - rebase -i: rearrange fixup/squash lines using the rebase--helper
>  - t3415: test fixup with wrapped oneline
>  - rebase -i: skip unnecessary picks using the rebase--helper
>  - rebase -i: check for missing commits in the rebase--helper
>  - t3404: relax rebase.missingCommitsCheck tests
>  - rebase -i: also expand/collapse the SHA-1s via the rebase--helper
>  - rebase -i: do not invent onelines when expanding/collapsing SHA-1s
>  - rebase -i: remove useless indentation
>  - rebase -i: generate the script via rebase--helper
>  - t3415: verify that an empty instructionFormat is handled as before
> 
>  The final batch to "git rebase -i" updates to move more code from
>  the shell script to C.
> 
>  Expecting a reroll.

Please stop stating that you expect a reroll for rebase-i-extra when you
explicitly stated months ago that you would not take my v6. It gets a bit
annoying.

Thanks,
Dscho

Re: What's cooking in git.git (Sep 2017, #03; Fri, 15)

[Lars Schneider]

On 15 Sep 2017, at 07:58, Junio C Hamano  wrote:

> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.  The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.

> ...

> * ls/travis-scriptify (2017-09-11) 3 commits
>  (merged to 'next' on 2017-09-14 at 8fa685d3b7)
> + travis: dedent a few scripts that are indented overly deeply
> + travis-ci: skip a branch build if equal tag is present
> + travis-ci: move Travis CI code into dedicated scripts
> 
> The scripts to drive TravisCI has been reorganized and then an
> optimization to avoid spending cycles on a branch whose tip is
> tagged has been implemented.
> 
> Will merge to 'master'.

SZEDER noticing a bug in this series that I was about to fix:
https://public-inbox.org/git/3b175d35-5b1c-43cd-a7e9-85693335b...@gmail.com/

I assume at this point a new patch is better than a re-roll, right?

Thanks,
Lars