Re: [PATCH v4 1/1] Make gitdir work with worktrees, respect core.hooksPath, etc

[Johannes Schindelin]

Hi Pratyush,

On Mon, 14 Oct 2019, Pratyush Yadav wrote:

> On 12/10/19 11:24PM, Johannes Schindelin wrote:
> >
> > On Sat, 12 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 08/10/19 04:33AM, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > @@ -1453,10 +1501,16 @@ proc rescan {after {honor_trustmtime 1}} {
> > > > global HEAD PARENT MERGE_HEAD commit_type
> > > > global ui_index ui_workdir ui_comm
> > > > global rescan_active file_states
> > > > -   global repo_config
> > > > +   global repo_config _gitdir_cache
> > > >
> > > > if {$rescan_active > 0 || ![lock_index read]} return
> > > >
> > > > +   # Only re-prime gitdir cache on a full rescan
> > > > +   if {$after ne "ui_ready"} {
> > >
> > > What do you mean by a "full rescan"? I assume you use it as the
> > > differentiator between `ui_do_rescan` (called when you hit F5 or choose
> > > rescan from the menu) and `do_rescan` (called when you revert a line or
> > > hunk), and a "full rescan" refers to `ui_do_rescan`.
> > >
> > > Well in that case, this check is incorrect. `do_rescan` passes only
> > > "ui_ready" and `ui_do_rescan` passes "force_first_diff ui_ready".
> > >
> > > But either way, I'm not a big fan of this. This check makes assumptions
> > > about the behaviour of its callers based on what they pass to $after.
> > > The way I see it, $after should be a black box to `rescan`, and it
> > > should make absolutely no assumptions about it.
> > >
> > > Doing it this way is really brittle, and would break as soon as someone
> > > changes the behaviour of `ui_do_rescan`. If someone in the future passes
> > > a different value in $after, this would stop working as intended and
> > > would not refresh the cached list on a rescan.
> > >
> > > So, I think a better place for this if statement would be in
> > > `ui_do_rescan`. This would mean adding a new function that does this.
> > > But if we unset _gitdir_cache in prime_gitdir_cache (I see no reason not
> > > to), we can get away with just something like:
> > >
> > >   proc ui_do_rescan {} {
> > >   rescan {prime_gitdir_cache; ui_ready}
> > >   }
> > >
> > > Though since `prime_gitdir_cache` does not really depend on the rescan
> > > being finished, something like this would also work fine:
> > >
> > >   proc ui_do_rescan {} {
> > >   rescan ui_ready
> > >   prime_gitdir_cache
> > >   }
> >
> > That was my first attempt. However, there is a very important piece of
> > code that is even still quoted above: that `if {$rescan_active > 0 ||
> > ![lock_index read]} return` part.
> >
> > I do _not_ want to interfere with an actively-going-on rescan. If there
> > is an active one, I don't want to re-prime the `_gitdir` cache.
>
> Good catch! In that case I suppose refreshing the cache in $after would
> be the way to go (IOW, the former of my two suggestions). Anything
> $after won't get executed if we return early from that check.

But it also won't get executed before the actual rescan. Which is
precisely what I tried to ensure.

Ciao,
Johannes

>
> > That was the reason why I put the additional code into `rescan` rather
> > than into `ui_do_rescan()`.
> >
> > Ciao,
> > Johannes
> >
> > >
> > > This would allow us to do these two things in parallel since `rescan` is
> > > asynchronous. But that would also mean it is possible that the status
> > > bar would show "Ready" while `prime_gitdir_cache` is still executing.
> > >
> > > I can't really make up my mind on what is better. I'm inclining on using
> > > the latter way, effectively trading a bit of UI inconsistency for
> > > performance (at least in theory).
> > >
> > > Thoughts?
> > >
> > > > +   array unset _gitdir_cache
> > > > +   prime_gitdir_cache
> > > > +   }
> > > > +
> > > > repository_state newType newHEAD newMERGE_HEAD
> > > > if {[string match amend* $commit_type]
> > > > && $newType eq {normal}
>
> --
> Regards,
> Pratyush Yadav
>

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Sat, 12 Oct 2019, Elijah Newren wrote:

> On Sat, Oct 12, 2019 at 1:37 PM Johannes Schindelin
>  wrote:
> >
> [...]
> > For the record: I am still a huge anti-fan of splitting `setup` test
> > cases from the test cases that do actual things, _unless_ it is
> > _one_, and _only one_, big, honking `setup` test case that is the
> > very first one in the test script.
> >
> > Splitting things into two inevitably leads to unnecessary churn when
> > investigating test failures.
> >
> > And that's really what test cases need to be optimized for: when
> > they report breakages. They need to help as much as they can to
> > investigate why things break. Nobody cares when test cases succeed.
> > The ~150k test cases that pass on every CI build: nobody is
> > interested. When a test case reports failure, that's when people pay
> > attention. At least some, including me.
> >
> > The most common case for me (and every other lazy person who relies
> > on CI/PR builds) is when a build breaks, and then I usually get to
> > the trace of the actually failing test case very quickly. The
> > previous test case's trace, not so easy. Clicks involved. Now I lose
> > context. Not good.
> >
> > A less common case for me is when I run test scripts locally, with
> > `-i -v -x`. Still, I need to scroll back to get context. And then,
> > really, I already lost context.
>
> I guess we have some strongly differing opinions here.

And probably experiences, too.

I literally debug something like a dozen per week test failures that are
reported via Azure Pipelines. And yes, I ran into some snags even with
your two-parter test cases in the past, and they did not help me. They
increased my burden of figuring out what is going wrong.

> The one thing I do agree with you on is test cases need to be
> optimized for when they report breakages, but that is precisely what
> led me to splitting setup and testing.

To me, it is so not helpful _not_ to see the output of a `setup` that
succeeded, and only the output of the actual test that actually failed.

It removes context.

I need to understand the scenario where the breakage happens, and the
only way I can understand is when I understand the context.

So the context needs to be as close as possible.

> Way too many tests in the testsuite intersperse several setup and test
> cases together making it really hard to disentangle, understand what
> is going on, or even reverse engineer what is relevant.  The absolute
> worst tests are the ones which just keep making additional changes to
> some existing repo to provide extra setup, causing all sorts of
> problems for skipping and resuming and understanding (to say nothing
> of test prerequisites that aren't always met).

I agree with this sentiment, and have to point out that this is yet
another fallout of the way our test suite is implemented. If you look
e.g. at JUnit, there are no "setup test cases". There are specific setup
steps that you can define, there is even a teardown step you can define,
and those individual test cases? They can run in parallel, or
randomized, and they run in their own sandbox, to make sure that they
don't rely on side effects of unrelated test cases.

We don't do that. We don't enforce the absence of side effects, and
therefore we have a megaton of them.

But note how this actually speaks _against_ separating the setup from
the test? Because then you _explicitly_ want those test cases to rely on
one another. Which flies in the _face_ of trying to disentangling them.

> But the ones with one big honking `setup` test case that is the very
> first one in the test script can often be pretty bad too when you've
> found a bug in testcase 82 and want to have a simple way to reproduce.

Indeed. One of the first things I try when `--run=82` fails is
`--run=1,82`. That's the best we can do in the absence of a clean design
like JUnit and its `@Before` method.

> It's awesome when someone can just run ./testcase.sh --ver --imm -x
> --run=86,87 and reproduce the problem.

But of course, often you would need `--run=1,86,87`. And it is totally
not obvious to _anybody_ who wants to contribute a new feature and whose
CI/PR build fails in one of your test cases.

> It feels sadly rare to be able to do that in much of git.git's
> testsuite.  Not accreting ever more changes into a repo to setup
> subsequent problems, using entirely separate repos for different cases
> where testing makes any changes, making it clear what is setup, making
> it clear what's the command being tested, and making it clear what all
> the commands are that are testing that the original test command
> produced the expected behavior all go a long way to making things way
> easier to investigate.

Sorry, I disagree. By squirreling away your setup phase into what looks
like an independent test case, the code is not more obvious, but less
so.

> Now, I will re-use a setup case for multiple tests and even did so in
> t6043, but only when the setup really is 

Re: [PATCH] column: use utf8_strnwidth() to strip out ANSI color escapes

[Johannes Schindelin]

Hi René,

On Sun, 13 Oct 2019, René Scharfe wrote:

> Make use of utf8_strnwidth()'s feature to skip ANSI escape sequences
> instead of open-coding it.  This shortens the code and makes it more
> consistent.

Sounds good.

> This changes the behavior, though: The old code skips all kinds of
> Control Sequence Introducer sequences, while utf8_strnwidth() only skips
> the Select Graphic Rendition kind, i.e. those ending with "m".  They are
> used for specifying color and font attributes like boldness.  The only
> other kind of escape sequence we print in Git is Erase in Line, ending
> with "K".  That's not used for columnar output, so this difference
> actually doesn't matter here.

Arguably, the "Erase in Line" thing should re-set the width to 0, no?
But as you say, this is not needed for this patch.

Thanks,
Dscho

>
> Signed-off-by: René Scharfe 
> ---
>  column.c | 13 +
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/column.c b/column.c
> index 7a17c14b82..4a38eed322 100644
> --- a/column.c
> +++ b/column.c
> @@ -23,18 +23,7 @@ struct column_data {
>  /* return length of 's' in letters, ANSI escapes stripped */
>  static int item_length(const char *s)
>  {
> - int len, i = 0;
> - struct strbuf str = STRBUF_INIT;
> -
> - strbuf_addstr(&str, s);
> - while ((s = strstr(str.buf + i, "\033[")) != NULL) {
> - int len = strspn(s + 2, "0123456789;");
> - i = s - str.buf;
> - strbuf_remove(&str, i, len + 3); /* \033[ */
> - }
> - len = utf8_strwidth(str.buf);
> - strbuf_release(&str);
> - return len;
> + return utf8_strnwidth(s, -1, 1);
>  }
>
>  /*
> --
> 2.23.0
>

Re: [PATCH 1/1] Improve Japanese translation

[Johannes Schindelin]

[Cc:ing the Git GUI maintainer]


On Mon, 14 Oct 2019, kdnakt via GitGitGadget wrote:

> From: kdnakt 
>
> Signed-off-by: kdnakt 
> ---
>  git-gui/po/ja.po | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/git-gui/po/ja.po b/git-gui/po/ja.po
> index 208651c1af..2f61153ab9 100644
> --- a/git-gui/po/ja.po
> +++ b/git-gui/po/ja.po
> @@ -4,14 +4,15 @@
>  #
>  # しらいし ななこ , 2007.
>  # Satoshi Yasushima , 2016.
> +# KIDANI Akito , 2019.
>  #
>  msgid ""
>  msgstr ""
>  "Project-Id-Version: git-gui\n"
>  "Report-Msgid-Bugs-To: \n"
>  "POT-Creation-Date: 2016-05-27 17:52+0900\n"
> -"PO-Revision-Date: 2016-06-22 12:50+0900\n"
> -"Last-Translator: Satoshi Yasushima \n"
> +"PO-Revision-Date: 2019-10-13 23:20+0900\n"
> +"Last-Translator: KIDANI Akito \n"
>  "Language-Team: Japanese\n"
>  "Language: ja\n"
>  "MIME-Version: 1.0\n"
> @@ -661,7 +662,7 @@ msgstr ""
>  #: lib/merge.tcl:108
>  #, tcl-format
>  msgid "%s of %s"
> -msgstr "%s の %s ブランチ"
> +msgstr "%2$s の %1$s ブランチ"
>
>  #: lib/merge.tcl:122
>  #, tcl-format
> @@ -956,7 +957,7 @@ msgstr "エラー: コマンドが失敗しました"
>  #: lib/checkout_op.tcl:85
>  #, tcl-format
>  msgid "Fetching %s from %s"
> -msgstr "%s から %s をフェッチしています"
> +msgstr "%2$s から %1$s をフェッチしています"
>
>  #: lib/checkout_op.tcl:133
>  #, tcl-format
> --
> gitgitgadget
>

Re: [PATCH 3/3] mingw: bump the minimum Windows version to Vista

[Johannes Schindelin]

Hi,

[sorry about the blast from the past, I just noticed that I had not
answered]

On Thu, 11 Oct 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
>
> >> It also means that we no longer need the inet_pton() and inet_ntop()
> >> emulation, which is nice.
> >
> > Earlier in this series you add a:
> >
> > #if defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x600
> > ...
> > #endif
> >
> > Shouldn't that now be something like:
> >
> > #if defined(_WIN32_WINNT)
> > #if _WIN32_WINNT >= 0x600
> > ...
> > #else
> > #error "You need at least Windows Vista to build Git!"
> > #endif
> > #endif
> >
> > Or do we catch users building on non-supported versions earlier somehow
> > (i.e. not just with a flood of compilation errors).
>
> That sounds like a reasonable thing to be curious about.

I specifically wanted to leave the door open for people who might want
to put in the effort of keeping Windows XP support alive.

But I guess this does not make any sense anymore, what with Windows
_Vista_ being at its end of its life in 3 months.

Thanks! Will prepare a patch,
Dscho

>
> > Both of the above are just questions I was curious about since I saw
> > your , and
> > shouldn't bee seen as bumping this to "this needs a re-roll" or it
> > should be delayed in getting to master.
>

Re: [PATCH 3/3] mingw: bump the minimum Windows version to Vista

[Johannes Schindelin]

Hi Ævar,

[sorry about the blast from the past, I forgot to answer back then.]

On Wed, 10 Oct 2018, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Oct 03 2018, Johannes Schindelin via GitGitGadget wrote:
>
> > diff --git a/config.mak.uname b/config.mak.uname
> > index e47af72e01..8acdeb71fd 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -381,8 +381,6 @@ ifeq ($(uname_S),Windows)
> > NO_PYTHON = YesPlease
> > BLK_SHA1 = YesPlease
> > ETAGS_TARGET = ETAGS
> > -   NO_INET_PTON = YesPlease
> > -   NO_INET_NTOP = YesPlease
> > NO_POSIX_GOODIES = UnfortunatelyYes
> > NATIVE_CRLF = YesPlease
> > DEFAULT_HELP_FORMAT = html
> > @@ -529,8 +527,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
> > NO_REGEX = YesPlease
> > NO_PYTHON = YesPlease
> > ETAGS_TARGET = ETAGS
> > -   NO_INET_PTON = YesPlease
> > -   NO_INET_NTOP = YesPlease
> > NO_POSIX_GOODIES = UnfortunatelyYes
> > DEFAULT_HELP_FORMAT = html
> > COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32
>
> So before we were defining NO_INET_{PTON,NTOP} because XP needed it, but
> doing so on all Windows versions, is there other compat stuff there
> that's just catering to the lowest common denominator, and if so
> shouldn't that be version checked against the NT version?

I meant to reply much earlier: When I worked on this, I essentially went
through all of the functions that we load dynamically on Windows, and
I don't think I found any other pre-Vista support code to get rid of.

Thanks,
Dscho

Re: [PATCH 3/3] mingw: bump the minimum Windows version to Vista

[Johannes Schindelin]

Hi,

On Mon, 14 Oct 2019, Johannes Schindelin wrote:

> On Thu, 11 Oct 2018, Junio C Hamano wrote:
>
> > Ævar Arnfjörð Bjarmason  writes:
> >
> > >> It also means that we no longer need the inet_pton() and inet_ntop()
> > >> emulation, which is nice.
> > >
> > > Earlier in this series you add a:
> > >
> > > #if defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x600
> > > ...
> > > #endif
> > >
> > > Shouldn't that now be something like:
> > >
> > > #if defined(_WIN32_WINNT)
> > > #if _WIN32_WINNT >= 0x600
> > > ...
> > > #else
> > > #error "You need at least Windows Vista to build Git!"
> > > #endif
> > > #endif
> > >
> > > Or do we catch users building on non-supported versions earlier somehow
> > > (i.e. not just with a flood of compilation errors).
> >
> > That sounds like a reasonable thing to be curious about.
>
> I specifically wanted to leave the door open for people who might want
> to put in the effort of keeping Windows XP support alive.
>
> But I guess this does not make any sense anymore, what with Windows
> _Vista_ being at its end of its life in 3 months.
>
> Thanks! Will prepare a patch,

Actually, while preparing that patch, I noticed that this is in the
`compat/poll/` part of our source code. I'd rather not inflict the
Vista-or-later requirement on that part, as it is relatively
self-contained and could still be copied into software that does support
Windows XP.

Thanks,
Dscho

Re: Git Gui: Branch->create currently fails...

[Philip Oakley]

Hi Pratyush,
On 13/10/2019 19:50, Pratyush Yadav wrote:

I've tried both parts and seen that this looks like some form of buffer
overrun or size limit

Looks like it, but I'm not sure if that is on our end.
  

with the mods I ran:
$ git gui > branch_create.txt

which produced the 'same' error missing ", but with a slightly different
fragment.

The branch_create.txt file is size 1.43 MB (1,502,103 bytes) (from the
windows explorer file properties dialog..)
opening in Notepad++ it's 4900 lines long with the final line trucated at
col 188 (shorter than other lines). There is an empty line 4901 (CRLF)

Yeah, that's a lot of refs! On my git.git clone, I get 1299 lines, and I
have git.git, my fork of git.git, and gitster in my remotes.
  

the last two lines are:
list "refs/heads/branch-patterns" [list "commit"
"b2453cea29b58f2ec57f9627b2456b41568ba5da" [concat "" "Philip Oakley"]
[reformat_date [concat "" "Tue May 28 20:22:09 2019 +0100"]] "squash! doc
branch: provide examples for listing remote tracking branches"] [list "" ""
"" [reformat_date ""] ""]
list "refs/heads/MSVC-README" [list "commit"
"056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"]
[reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]] "compat/vc

the file starts with 1018 lines of refs/tags before listing the refs/remotes
and finally the refs/heads.

The repo is my local Git repo with multiple remotes (git.git, G-f-W, ggg,
junio, gitster, dscho, t-b, tboeg, me), so plenty of refs there!

So it does look to be specific to repos with a large number of refs/tags,
refs/remotes, and refs/heads.

something for the back-burner?

I'm not sure why or where a buffer overflow would occur. We don't store
the whole output directly in a variable. Instead, we store each line
from the pipe coming in from `git for-each-ref` in $line, so that's a
few hundred characters at most. The rest of the data stays in the pipe,
which the OS should handle, and I don't think a few MBs should cause
trouble.

If I had to guess, I'd suspect either an internal Tcl limit, or
something with Tcl pipes.

Just to be sure it is a git-gui/Tcl issue and not an upstream git.git
issue, can you run:

   fmt='list %(refname) [list %(objecttype) %(objectname) [concat %(taggername) 
%(authorname)] [reformat_date [concat %(taggerdate) %(authordate)]] %(subject)] 
[list %(*objecttype) %(*objectname) %(*authorname) [reformat_date 
%(*authordate)] %(*subject)]'
   
   git for-each-ref --tcl --format="$fmt" --sort=-taggerdate refs/heads refs/remotes refs/tags


and see if the output contains that truncated line? If it does, then
that means the bug is in git-for-each-ref. Note that this is bash
syntax, and I did a test run on Linux. Do adjust it for Windows and your
shell if needed.


ran that bit of code (as distinct commands), and got (last two lines):

[list "" "" "" [reformat_date ""] ""]
list "refs/heads/branch-patterns-v2" [list "commit" 
"d5a799d8833b0ae195915eefd5365f3fc4c7c0a4" [concat "" "Philip Oakley"] 
[reformat_date [concat "" "Sat Jun 8 22:50:06 2019 +0100"]] 
"t3203-branch-output: test -a & -r pattern options"] [list "" "" "" 
[reformat_date ""] ""]
list "refs/heads/branch-patterns" [list "commit" 
"b2453cea29b58f2ec57f9627b2456b41568ba5da" [concat "" "Philip Oakley"] 
[reformat_date [concat "" "Tue May 28 20:22:09 2019 +0100"]] "squash! 
doc branch: provide examples for listing remote tracking branches"] 
[list "" "" "" [reformat_date ""] ""]
list "refs/heads/MSVC-README" [list "commit" 
"056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"] 
[reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]] 
"compat/vcSegmentation fault



Not exactly the same, but almost. Ends the same place, with as similar 
short line.
This is run inside the bash that is started directly by the 
git-for-windows sdk start icon. (Target: C:\git-sdk-64\git-bash.exe   
Stat in: C:/git-sdk-64/)


so looks to be MSYS2/bash related.

--
Philip

Re: [PATCH v2 0/5] Use complete_action's todo list to do the rebase

[Johannes Schindelin]

Hi Alban,

On Mon, 7 Oct 2019, Alban Gruin wrote:

> This can be seen as a continuation of ag/reduce-rewriting-todo.
>
> Currently, complete_action() releases its todo list before calling
> sequencer_continue(), which reloads the todo list from the disk.  This
> series removes this useless round trip.
>
> Patches 1, 2, and 3 originally come from a series meaning to improve
> rebase.missingCommitsCheck[0].  In the original series, I wanted to
> check for missing commits in read_populate_todo(), so a warning could be
> issued after a `rebase --continue' or an `exec' commands.  But, in the
> case of the initial edit, it is already checked in complete_action(),
> and would be checked a second time in sequencer_continue() (a caller of
> read_populate_todo()).  So I hacked up sequencer_continue() to accept a
> pointer to a todo list, and if not null, would skip the call to
> read_populate_todo().  (This was really ugly, to be honest.)  Some
> issues arose with git-prompt.sh[1], hence 1, 2 and 3.
>
> Patch 5 is a new approach to what I did first.  Instead of bolting a new
> parameter to sequencer_continue(), this makes complete_action() calling
> directly pick_commits().
>
> This is based on 4c86140027 ("Third batch").
>
> Changes since v1:
>
>  - Rewording of patches 1, 2, 4 and 5 according to comments made by
>Phillip Wood, Junio C Hamano and Johannes Schindelin.
>
> The tip of this series is tagged as "reduce-todo-list-cont-v2" at
> https://github.com/agrn/git.
>
> [0] http://public-inbox.org/git/20190717143918.7406-1-alban.gr...@gmail.com/
> [1] http://public-inbox.org/git/1732521.CJWHkCQAay@andromeda/
>
> Alban Gruin (5):
>   sequencer: update `total_nr' when adding an item to a todo list
>   sequencer: update `done_nr' when skipping commands in a todo list
>   sequencer: move the code writing total_nr on the disk to a new
> function
>   rebase: fill `squash_onto' in get_replay_opts()
>   sequencer: directly call pick_commits() from complete_action()
>
>  builtin/rebase.c |  5 +
>  sequencer.c  | 26 ++
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> Diff-intervalle contre v1 :
> 1:  d177b0de1a ! 1:  9215b191c7 sequencer: update `total_nr' when adding an 
> item to a todo list
> @@ Metadata
>   ## Commit message ##
>  sequencer: update `total_nr' when adding an item to a todo list
>
> -`total_nr' is the total amount of items, done and toto, that are in a
> -todo list.  But unlike `nr', it was not updated when an item was
> -appended to the list.
> +`total_nr' is the total number of items, counting both done and todo,
> +that are in a todo list.  But unlike `nr', it was not updated when an
> +item was appended to the list.
>
>  This variable is mostly used by command prompts (ie. git-prompt.sh 
> and
> -the like).
> +the like).  By forgetting to update it, the original code made it not
> +reflect the reality, but this flaw was masked by the code calling
> +unnecessarily read_todo_list() again to update the variable to its
> +correct value.  At the end of this series, the unnecessary call will 
> be
> +removed, and the inconsistency addressed by this patch would start to
> +matter.
>
>  Signed-off-by: Alban Gruin 
>
> 2:  09fcbe159b ! 2:  7cad541092 sequencer: update `done_nr' when skipping 
> commands in a todo list
> @@ Commit message
>  or skipped, but skip_unnecessary_picks() did not update it.
>
>  This variable is mostly used by command prompts (ie. git-prompt.sh 
> and
> -the like).
> +the like).  As in the previous commit, this inconsistent behaviour is
> +not a problem yet, but it would start to matter at the end of this
> +series the same reason.
>
>  Signed-off-by: Alban Gruin 
>
> 3:  26a18cd1a9 = 3:  7c9c4ddd30 sequencer: move the code writing total_nr on 
> the disk to a new function
> 4:  5d74903cfe ! 4:  cd44fb4e10 rebase: fill `squash_onto' in 
> get_replay_opts()
> @@ Metadata
>   ## Commit message ##
>  rebase: fill `squash_onto' in get_replay_opts()
>
> -get_replay_opts() did not fill `squash_onto' if possible, meaning 
> that
> -this field should be read from the disk by the sequencer through
> -read_populate_opts().  Without this, calling `pick_commits()' 
> directly
> -will result in incorrect results with `rebase --root'.
> +Currently, the get_replay_opts() function does not initialise the
> +`squash_onto' field (which is used for the `--root' mode), only
> +read_populate_opts() does.  That would lead to incorrect results when
> +calling pick_commits() without reading the options from the disk 
> first.
>
>  Let’s change that.
>
> 5:  dc803c671f ! 5:  523fdd35a1 sequencer: directly call pick_commits() from 
> complete_action()
> @@ Commit message
> 

Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`

[James Coglan]

On 12/10/2019 01:27, Junio C Hamano wrote:
> James Coglan  writes:
> 
>> - We don't want a general solution to this problem for everything
>> `strbuf` could be used for; it only needs to address the graph
>> padding problem.
> 
> Of course.  Somebody may use strbuf to hold rows of a table and you
> do not want to contaminate strbuf with fields like width of each
> column etc, that are very specific to the application.  IOW, strbuf
> is merely _one_ component of a larger solution to each specific
> problem, and the latter may be things like "struct graphbuf" like
> Dscho suggested, which might use strbuf as an underlying
>  tuple mechanism, but that is an implementation
> detail that should not be exposed to the users of the struct (and
> that is why he did not call, and you should not call, the data
> structure "graph-strbuf" or anything with "strbuf").

Thank you for the clarification. I've now modified my patch to call the wrapper 
struct `graph_line` to better reflect its use. This was informed by uncertainty 
on this thread about whether the width calculation needs to take line breaks 
into account, so I wanted to name it to indicate it contains a single display 
line.

>> - We only want to count printing characters, not color formatting sequences.
> 
> OK.  But I'd phrase "count printing characters" as "measure display
> width" for at least two reasons.  Whitespaces are typically counted
> as non-printing, but you do want to take them into account for this
> application.  Also the graph may not be limited to ASCII graphics
> forever, and byte- or character-count may not match display width on
> a fixed-width display.
> 
>> - We only need to consider the width of a small set of characters:
>> { | / \ _ - . * }. We don't need to worry about Unicode, and the
>> simple character counting in graph.c was working fine.
> 
> I have to warn you that we saw attempts to step outside these ASCII
> graphics and use Unicode characters for prettier output in the past.
> If you can do so without too much complexity, I'd suggest you try
> not to close the door to those people who follow your footsteps to
> further improve the system by pursuing the avenue further.

That makes sense. All I've done for now is to put the calculations that were 
already being done behind an interface, consisting of just the operations 
graph.c actually uses, namely:

void graph_line_addch(struct graph_line *line, int c);

void graph_line_addchars(struct graph_line *line, int c, size_t n);

void graph_line_addstr(struct graph_line *line, const char *s);

void graph_line_write_column(struct graph_line *line, const struct column *c, 
char col_char);

Having this interface in place should not preclude extending this functionality 
to Unicode later, and may make it simpler as it puts all the disply width 
calculations in one place, rather than its current state where it's distributed 
across all the output functions and makes the assumption that all chars 
contribute one display column. It especially removes the need for statements 
like this that encode an assumption about what was printed:

graph_pad_horizontally(graph, sb, graph->num_columns * 2);

>> To this end I've prepared a different implementation that
>> introduces the indirection described above, and does not modify
>> `strbuf`:
>>
>> +struct graph_strbuf {
>> +struct strbuf *buf;
>> +size_t width;
>> +};
> 
> Is there a reason why you need a pointer to a strbuf that is
> allocated separately?  E.g. would it make it harder to manage
> if the above were
> 
>   struct graphbuf {
>   struct strbuf buf;
>   int width; /* display width in columns */
>   };
> 
> which is essentially what Dscho suggested?

I used a pointer here because I create the wrapper struct in 
`graph_next_line()`, which is an external interface that takes a `struct strbuf 
*`:

int graph_next_line(struct git_graph *graph, struct strbuf *sb)
{
struct graph_line line = { .buf = sb, .width = 0 };
// ...
}

So I'm not allocating the strbuf here, just wrapping a pointer to it. I would 
prefer to define `graph_line` as having a `strbuf` inline but I've not found a 
way to do that without breaking the other functions that call 
`graph_next_line()`. Although, as far as I know both this wrapper struct and 
every `strbuf` it points to are stack-allocated so I'm more concerned about the 
extra dereference rather than allocation cost.

If there's a way to do this I'm open to suggestions.

>> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
>> +{
>> +strbuf_addch(sb->buf, c);
>> +sb->width++;
>> +}
>> +
>> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
>> +{
>> +strbuf_addchars(sb->buf, c, n);
>> +sb->width += n;
>> +}
>> +
>> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char 
>> *s)
>> +{
>> +strbuf_addstr(sb->buf, s);
>> +sb->width += strlen(s);
>> +}
> 
> I'd probably intro

Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`

[James Coglan]

On 14/10/2019 13:55, James Coglan wrote:
> Thanks! For completeness, here is the current state of this patch after I've 
> been through the feedback up to this point:
> 
> From 241180570fbaae04a2263c5ff1ab3b92f54f8004 Mon Sep 17 00:00:00 2001
> From: James Coglan 
> Date: Thu, 22 Aug 2019 09:30:08 +0100
> Subject: [PATCH] graph: automatically track display width of graph lines
> 
> All the output functions called by `graph_next_line()` currently keep
> track of how many printable chars they've written to the buffer, before
> calling `graph_pad_horizontally()` to pad the line with spaces. Some
> functions do this by incrementing a counter whenever they write to the
> buffer, and others do it by encoding an assumption about how many chars
> are written, as in:
> 
> graph_pad_horizontally(graph, sb, graph->num_columns * 2);
> 
> This adds a fair amount of noise to the functions' logic and is easily
> broken if one forgets to increment the right counter or update the
> calculations used for padding.
> 
> To make this easier to use, I'm introducing a new struct called
> `graph_line` that wraps a `strbuf` and keeps count of its display width
> implicitly. `graph_next_line()` wraps this around the `struct strbuf *`
> it's given and passes a `struct graph_line *` to the output functions,
> which use its interface.
> 
> The `graph_line` interface wraps the `strbuf_addch()`,
> `strbuf_addchars()` and `strbuf_addstr()` functions, and adds the
> `graph_line_write_column()` function for adding a single character with
> color formatting. The `graph_pad_horizontally()` function can then use
> the `width` field from the struct rather than taking a character count
> as a parameter.
> 
> Signed-off-by: James Coglan 
> ---
>  graph.c | 194 +---
>  1 file changed, 99 insertions(+), 95 deletions(-)
> 
> diff --git a/graph.c b/graph.c
> index f53135485f..2f81a5d23d 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short 
> color)
>   return column_colors[color];
>  }
>  
> -static void strbuf_write_column(struct strbuf *sb, const struct column *c,
> - char col_char)
> +struct graph_line {
> + struct strbuf *buf;
> + size_t width;
> +};
> +
> +static inline void graph_line_addch(struct graph_line *line, int c)
> +{
> + strbuf_addch(line->buf, c);
> + line->width++;
> +}
> +
> +static inline void graph_line_addchars(struct graph_line *line, int c, 
> size_t n)
> +{
> + strbuf_addchars(line->buf, c, n);
> + line->width += n;
> +}
> +
> +static inline void graph_line_addstr(struct graph_line *line, const char *s)
> +{
> + strbuf_addstr(line->buf, s);
> + line->width += strlen(s);
> +}
> +
> +static inline void graph_line_addcolor(struct graph_line *line, unsigned 
> short color)
> +{
> + strbuf_addstr(line->buf, column_get_color_code(color));
> +}
> +
> +static void graph_line_write_column(struct graph_line *line, const struct 
> column *c,
> + char col_char)
>  {
>   if (c->color < column_colors_max)
> - strbuf_addstr(sb, column_get_color_code(c->color));
> - strbuf_addch(sb, col_char);
> + graph_line_addcolor(line, c->color);
> + graph_line_addch(line, col_char);
>   if (c->color < column_colors_max)
> - strbuf_addstr(sb, column_get_color_code(column_colors_max));
> + graph_line_addcolor(line, column_colors_max);
>  }
>  
>  struct git_graph {
> @@ -686,8 +714,7 @@ static int graph_is_mapping_correct(struct git_graph 
> *graph)
>   return 1;
>  }
>  
> -static void graph_pad_horizontally(struct git_graph *graph, struct strbuf 
> *sb,
> -int chars_written)
> +static void graph_pad_horizontally(struct git_graph *graph, struct 
> graph_line *line)
>  {
>   /*
>* Add additional spaces to the end of the strbuf, so that all
> @@ -696,12 +723,12 @@ static void graph_pad_horizontally(struct git_graph 
> *graph, struct strbuf *sb,
>* This way, fields printed to the right of the graph will remain
>* aligned for the entire commit.
>*/
> - if (chars_written < graph->width)
> - strbuf_addchars(sb, ' ', graph->width - chars_written);
> + if (line->width < graph->width)
> + graph_line_addchars(line, ' ', graph->width - line->width);
>  }
>  
>  static void graph_output_padding_line(struct git_graph *graph,
> -   struct strbuf *sb)
> +   struct graph_line *line)
>  {
>   int i;
>  
> @@ -719,11 +746,11 @@ static void graph_output_padding_line(struct git_graph 
> *graph,
>* Output a padding row, that leaves all branch lines unchanged
>*/
>   for (i = 0; i < graph->num_new_columns; i++) {
> - strbuf_write_column(sb, &graph->new_columns[i], '|');
> - strbuf_a

Re: [PATCH 06/11] graph: tidy up display of left-skewed merges

[James Coglan]

On 12/10/2019 02:36, Derrick Stolee wrote:
> On 10/11/2019 12:50 PM, James Coglan wrote:
>> On 10/10/2019 18:19, Derrick Stolee wrote:
>>> On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote:
 +++ b/t/t4215-log-skewed-merges.sh
 @@ -0,0 +1,42 @@
 +#!/bin/sh
 +
 +test_description='git log --graph of skewed merges'
 +
 +. ./test-lib.sh
 +
 +test_expect_success 'setup left-skewed merge' '
>>>
>>>
>>> Could you skew this example to include a left-skewed octopus merge
>>> (and use fewer Git processes) with the following:
>>>
>>> git checkout --orphan _a && test_commit A &&
>>> git switch -c _b _a && test_commit B &&
>>> git switch -c _c _a && test_commit C &&
>>> git switch -c _d _a && test_commit D && git switch -c _e _b && git 
>>> merge --no-ff _c _d E &&
>>> git switch -c _f _a && git merge --no-ff _d -m F && git checkout _a 
>>> && git merge --no-ff _b _c _e _f -m G
>>> and I think the resulting output will be:
>>>
>>> *-.   G
>>> |\ \ \ \
>>> | | | | * F
>>> | |_|_|/|
>>> |/| | | |
>>> | | | * | E
>>> | |_|/|\|
>>> |/| | | |
>>> | | |/  * D
>>> | |_|__/
>>> |/| |
>>> | | * C
>>> | |/
>>> |/|
>>> | * B
>>> |/
>>> * A
>>
>> At this point in the history, commit E won't render like that -- this is 
>> before the change that flattens edges that fuse with the merge's last 
>> parent. I think the display of this history at this point will be:
>>
>>  *-.   G
>>  |\ \ \ \
>>  | | | | * F
>>  | |_|_|/|
>>  |/| | | |
>>  | | | * |   E
>>  | |_|/|\ \
>>  |/| |/ / /
>>  | | | | /
>>  | | | |/
>>  | | | * D
>>  | |_|/
>>  |/| |
>>  | | * C
>>  | |/
>>  |/|
>>  | * B
>>  |/
>>  * A
>>
>> Is there a particular reason for wanting to include this test case? What 
>> particular combination of states is it designed to test? (My guess is that 
>> it includes an octopus merge where the original test does not.) I'd be happy 
>> to add it at the appropriate point in the history if it's adding coverage 
>> not provided by the other tests.
> 
> Thanks for correcting my test case. It also helps that you would show the 
> change in behavior in your later commits.
> 
> My reason to include this test is because it includes a regular merge and an 
> octopus merge, both of which have a skewed render. Many times logic that 
> applies to a normal merge breaks with octopus merges, so I try to include 
> them whenever possible.

Thanks, I've now incorporated your suggested test into my patch. I had to amend 
it slightly as it turns out the above history is not valid; G is not a possible 
merge because one of its parents (C) is an ancestor of another (E). The actual 
example I've added is this:

*-.   0_H
|\ \ \ \
| | | | * 0_G
| |_|_|/|
|/| | | |
| | | * \   0_F
| |_|/|\ \
|/| | | |/
| | | | * 0_E
| |_|_|/
|/| | |
| | * | 0_D
| | |/
| | * 0_C
| |/
|/|
| * 0_B
|/
* 0_A

I've also added another commit before beginning this work that adds the example 
from the cover letter, so you can see it changing with each commit, namely this 
history:

*   H
|\
| *   G
| |\
| | * F
| | |
| |  \
| *-. \   E
| |\ \ \
|/ / / /
| | | /
| | |/
| | * D
| * | C
| |/
* | B
|/
* A

Re: [PATCH 2/3] sequencer: use run_commit_hook()

[Phillip Wood]

Hi Dscho & Junio

On 11/10/2019 05:24, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
>>>  builtin/commit.c | 22 --
>>>  commit.h |  3 ---
>>>  sequencer.c  | 45 ++---
>>>  sequencer.h  |  2 ++
>>>  4 files changed, 36 insertions(+), 36 deletions(-)
>>
>> Hmm. I would have thought that `commit.c` would be a more logical home
>> for that function (and that the declaration could remain in `commit.h`)?
> 
> Good correction.

There are some other public commit related functions in sequencer.c -
print_commit_summary(), commit_post_rewrite(), rest_is_empty(),
cleanup_message(), message_is_empty(), template_untouched(),
update_head_with_reflog() . Would you like to see them moved to commit.c
(probably as a separate series)?

Best Wishes

Phillip

> 
> Thanks.
> 

Re: [PATCH] column: use utf8_strnwidth() to strip out ANSI color escapes

[René Scharfe]

Am 14.10.19 um 13:13 schrieb Johannes Schindelin:
> Hi René,
>
> On Sun, 13 Oct 2019, René Scharfe wrote:
>
>> This changes the behavior, though: The old code skips all kinds of
>> Control Sequence Introducer sequences, while utf8_strnwidth() only skips
>> the Select Graphic Rendition kind, i.e. those ending with "m".  They are
>> used for specifying color and font attributes like boldness.  The only
>> other kind of escape sequence we print in Git is Erase in Line, ending
>> with "K".  That's not used for columnar output, so this difference
>> actually doesn't matter here.
>
> Arguably, the "Erase in Line" thing should re-set the width to 0, no?
> But as you say, this is not needed for this patch.

It doesn't move the cursor, just clears the characters to the right, to
the left or both sides, depending on its parameter.  So ignoring it for
width calculation like  the old code did would be appropriate -- if we'd
encounter such an escape sequence in text to be shown in columns.

René

Re: Is GIT GUI still in progress?

[Pratyush Yadav]

On 14/10/19 10:55AM, Jiang Xin wrote:
> 김건우  于2019年10月10日周四 上午12:02写道：
> >
> > Hello.
> >
> > I have a question whether the GIT GUI project still in progress or not
> > because I want to contribute to translating GIT GUI into Korean. I asked
> > prati0100 who is a maintainer of GIT GUI on
> > Github(https://github.com/prati0100/git-gui), but he doesn't know the
> > project is on.
> > If the project is in progress, please reply this E-mail with how to
> > contribute the GIT GUI for translation.
> >
> > Thank you.
> >
> > --
> > Signed-off-by: Kim Geonwoo (김건우)
> >
> 
> Git GUI is a stand-alone project which is periodically merged to Git
> project by a subtree merge. According to the latest
> SubmittingPatches[1] documentation, git-gui is managed by Pratyush
> Yadav in a separate project[2].
> 
> As a separate  project, the git-l10n project will merge l10n
> contributions into itself and then merge them to Git as a whole
> through a subtree merge. As what README.md[3] of git-gui says,
> contributing to git-gui should using mailing list. This means l10n
> contributors for git-guil, have to use git-format-patch and git
> send-email commands to send patches to the mailing list, which is
> inconvenience for git-gui l10n contributors. I suppose using a
> dedicate git-guil l10n coordinator repository or simply using pull
> requests of "prati0100/git-gui" as the l10n contribution workflow,
> @Yadav.

I'd prefer a separate git-gui l10n repo. The translators can use 
whatever workflow they prefer, and then I can just directly merge in 
your pull requests, similar to how it works for Git. Does this work for 
you guys?

Thanks for volunteering to do the translations :)
 
> [1] 
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L375
> [2] https://github.com/prati0100/git-gui
> [3] https://github.com/prati0100/git-gui/blob/master/README.md

-- 
Regards,
Pratyush Yadav

Re: [PATCH 07/11] graph: commit and post-merge lines for left-skewed merges

[James Coglan]

On 13/10/2019 07:56, Jeff King wrote:
> On Fri, Oct 11, 2019 at 06:04:21PM +0100, James Coglan wrote:
> 
>>> I should have noticed in your earlier commits, but why don't you keep
>>> the output inside the test suite? You can start with "cat >expect <<-EOF"
>>> to have it ignore leading whitespace. Sorry if there's something else about
>>> this that is causing issues.
>>
>> I was following a pattern used in t/t4202-log.sh. I believe it was
>> easier to debug these tests with the setup and expectations split into
>> separate blocks, but I wouldn't be opposed to merging them.
> 
> Some of the older tests used that style, but we've been slowly
> modernizing (I know, it's hard to pick up the style by example in such
> cases!). The usual style these days is making sure everything goes in a
> test_expect_* block, with "<<-" to indent here-documents.
> 
> Another minor style nit that you picked up from t4202:
> 
 +cat > expect <<\EOF
> 
> We'd omit the space after ">" here.

Thanks, I've now made the style changes you've suggested on my branch. How 
should I go about sharing the current state of my patch series after I've 
incorporated all the changes suggested here? Should I post them as replies on 
this thread, or start a new pull request via GitGitGadget?

Re: [PATCH 00/11] Improve the readability of log --graph output

[James Coglan]

On 13/10/2019 08:15, Jeff King wrote:
> On Thu, Oct 10, 2019 at 09:13:42AM -0700, James Coglan via GitGitGadget wrote:
> 
>> A final addition to that set of changes fixes the coloring of dashes that
>> are drawn next to octopus merges, in a manner compatible with all these
>> changes. The early commits in this set are refactorings that make the
>> functional changes easier to introduce.
> 
> As somebody who has pondered the octopus coloring code (for an
> embarrassingly long time considering that it still has some bugs!), let
> me just say thank you for taking this on. :)
> 
> Moreover, I'll echo Dscho's comments elsewhere on the quality of this
> series. It's a tricky topic to explain, and the way you've broken it up,
> along with the commit messages, comments, and diagrams made it much
> easier to follow.
> 
> Others have already commented on things I saw while reading it, so I'll
> just add a few more thoughts.
> 
>> This series of patches are designed to improve the output of the log --graph
>> command; their effect can be summed up in the following diagram:
>>
>> BeforeAfter
>> ---
>>
>> *
>> |\
>> | *   *
>> | |\  |\
>> | | * | *
>> | | | | |\
>> | |  \| | *
>> | *-. \   | * |
>> | |\ \ \  |/|\|
>> |/ / / /  | | *
>> | | | /   | * |
>> | | |/| |/
>> | | * * /
>> | * | |/
>> | |/  *
>> * |
>> |/
>> *
> 
> I wondered if anybody would prefer the sparseness of the "before"
> diagram, and if that would merit having two modes that could selected at
> runtime. I'm not sure I'd want to carry the code for both types, though;
> it seems like a recipe for the non-default output format to accrue a
> bunch of bugs (since the graph code has proven itself to be a magnet for
> off-by-ones and other weirdness).

You're probably right about the non-default mode hiding bugs, but if you did 
want to do this, I believe the original rendering can be preserved by the 
following small switches:

- always set `merge_layout = 1`; this will prevent skewing to the left
- do not modify `edges_added` if the last parent fuses with its neighbor

I believe all the layout changes are driven by these values, so you should be 
able to set in such a way as to preserve the original rendering by ignoring the 
special cases that lead to them having different values.

Re: [PATCH 07/11] graph: commit and post-merge lines for left-skewed merges

[Derrick Stolee]

On 10/14/2019 11:38 AM, James Coglan wrote:
> On 13/10/2019 07:56, Jeff King wrote:
>> On Fri, Oct 11, 2019 at 06:04:21PM +0100, James Coglan wrote:
>>
 I should have noticed in your earlier commits, but why don't you keep
 the output inside the test suite? You can start with "cat >expect <<-EOF"
 to have it ignore leading whitespace. Sorry if there's something else about
 this that is causing issues.
>>>
>>> I was following a pattern used in t/t4202-log.sh. I believe it was
>>> easier to debug these tests with the setup and expectations split into
>>> separate blocks, but I wouldn't be opposed to merging them.
>>
>> Some of the older tests used that style, but we've been slowly
>> modernizing (I know, it's hard to pick up the style by example in such
>> cases!). The usual style these days is making sure everything goes in a
>> test_expect_* block, with "<<-" to indent here-documents.
>>
>> Another minor style nit that you picked up from t4202:
>>
> +cat > expect <<\EOF
>>
>> We'd omit the space after ">" here.
> 
> Thanks, I've now made the style changes you've suggested on my branch. How 
> should I go about sharing the current state of my patch series after I've 
> incorporated all the changes suggested here? Should I post them as replies on 
> this thread, or start a new pull request via GitGitGadget?

Since you sent v1 via GitGitGadget, all you need to do is
add another "/submit" comment on the same PR and it will
send a v2 to this thread.

It will auto-generate the range-diff from v1 and append it
to your cover letter.

-Stolee

Re: [PATCH 1/1] Improve Japanese translation

[Pratyush Yadav]

Hi kdnakt,

I updated the commit message subject locally to "git-gui: improve 
Japanese translation" to match our commit message style.

On 14/10/19 01:31AM, kdnakt via GitGitGadget wrote:
> From: kdnakt 
> 
> Signed-off-by: kdnakt 
> ---
>  git-gui/po/ja.po | 9 +

You based this patch on the git.git repo. For now, I munged the patch 
and applied to my tree, but for future contributions, please base your 
patches on the git-gui tree. That would mean your paths in the diff 
would look like: 'po/ja.po' instead of 'git-gui/po/ja.po/'. The 
instructions to do that in GitGitGadget can be found at [0].

>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/git-gui/po/ja.po b/git-gui/po/ja.po
> index 208651c1af..2f61153ab9 100644
> --- a/git-gui/po/ja.po
> +++ b/git-gui/po/ja.po
> @@ -4,14 +4,15 @@
>  #
>  # しらいし ななこ , 2007.
>  # Satoshi Yasushima , 2016.
> +# KIDANI Akito , 2019.
>  #
>  msgid ""
>  msgstr ""
>  "Project-Id-Version: git-gui\n"
>  "Report-Msgid-Bugs-To: \n"
>  "POT-Creation-Date: 2016-05-27 17:52+0900\n"
> -"PO-Revision-Date: 2016-06-22 12:50+0900\n"
> -"Last-Translator: Satoshi Yasushima \n"
> +"PO-Revision-Date: 2019-10-13 23:20+0900\n"
> +"Last-Translator: KIDANI Akito \n"
>  "Language-Team: Japanese\n"
>  "Language: ja\n"
>  "MIME-Version: 1.0\n"
> @@ -661,7 +662,7 @@ msgstr ""
>  #: lib/merge.tcl:108
>  #, tcl-format
>  msgid "%s of %s"
> -msgstr "%s の %s ブランチ"
> +msgstr "%2$s の %1$s ブランチ"
>  
>  #: lib/merge.tcl:122
>  #, tcl-format
> @@ -956,7 +957,7 @@ msgstr "エラー: コマンドが失敗しました"
>  #: lib/checkout_op.tcl:85
>  #, tcl-format
>  msgid "Fetching %s from %s"
> -msgstr "%s から %s をフェッチしています"
> +msgstr "%2$s から %1$s をフェッチしています"
>  
>  #: lib/checkout_op.tcl:133
>  #, tcl-format

Thanks for the translation update. Will queue.

[0] https://github.com/prati0100/git-gui#using-gitgitgadget

-- 
Regards,
Pratyush Yadav

Re: Git Gui: Branch->create currently fails...

[Pratyush Yadav]

On 14/10/19 01:45PM, Philip Oakley wrote:
> Hi Pratyush,
> On 13/10/2019 19:50, Pratyush Yadav wrote:
> > Just to be sure it is a git-gui/Tcl issue and not an upstream 
> > git.git
> > issue, can you run:
> > 
> >fmt='list %(refname) [list %(objecttype) %(objectname) [concat 
> > %(taggername) %(authorname)] [reformat_date [concat %(taggerdate) 
> > %(authordate)]] %(subject)] [list %(*objecttype) %(*objectname) 
> > %(*authorname) [reformat_date %(*authordate)] %(*subject)]'
> >git for-each-ref --tcl --format="$fmt" --sort=-taggerdate refs/heads 
> > refs/remotes refs/tags
> > 
> > and see if the output contains that truncated line? If it does, then
> > that means the bug is in git-for-each-ref. Note that this is bash
> > syntax, and I did a test run on Linux. Do adjust it for Windows and your
> > shell if needed.
> 
> ran that bit of code (as distinct commands), and got (last two lines):
> 
> [list "" "" "" [reformat_date ""] ""]
> list "refs/heads/branch-patterns-v2" [list "commit"
> "d5a799d8833b0ae195915eefd5365f3fc4c7c0a4" [concat "" "Philip Oakley"]
> [reformat_date [concat "" "Sat Jun 8 22:50:06 2019 +0100"]]
> "t3203-branch-output: test -a & -r pattern options"] [list "" "" ""
> [reformat_date ""] ""]
> list "refs/heads/branch-patterns" [list "commit"
> "b2453cea29b58f2ec57f9627b2456b41568ba5da" [concat "" "Philip Oakley"]
> [reformat_date [concat "" "Tue May 28 20:22:09 2019 +0100"]] "squash! doc
> branch: provide examples for listing remote tracking branches"] [list "" ""
> "" [reformat_date ""] ""]
> list "refs/heads/MSVC-README" [list "commit"
> "056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"]
> [reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]]
> "compat/vcSegmentation fault
> 
> 
> Not exactly the same, but almost. Ends the same place, with as similar short
> line.
> This is run inside the bash that is started directly by the git-for-windows
> sdk start icon. (Target: C:\git-sdk-64\git-bash.exe   Stat in:
> C:/git-sdk-64/)
> 
> so looks to be MSYS2/bash related.

Ah, so it is an upstream issue. I guess we can just report it and wait 
for them to fix it.

-- 
Regards,
Pratyush Yadav

Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums

[Jonathan Tan]

> Jonathan Tan  writes:
> 
> >> > Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> >> > line but that is not necessary.
> >> 
> >> That is absolutely necessary; it is not like "we do not care what
> >> exact value _COPY gets; it can be any value as long as it is _MOVE
> >> plus 1", as these are used in set of bits (and again, I do not think
> >> it is such a brilliant idea to use enum for such a purpose).
> >
> > Good point.
> 
> Doesn't that also show that enums are not quite a good fit for set
> of bits (i.e. 1<

Re: [PATCH] column: use utf8_strnwidth() to strip out ANSI color escapes

[Johannes Schindelin]

Hi René,

On Mon, 14 Oct 2019, René Scharfe wrote:

> Am 14.10.19 um 13:13 schrieb Johannes Schindelin:
>
> > On Sun, 13 Oct 2019, René Scharfe wrote:
> >
> >> This changes the behavior, though: The old code skips all kinds of
> >> Control Sequence Introducer sequences, while utf8_strnwidth() only skips
> >> the Select Graphic Rendition kind, i.e. those ending with "m".  They are
> >> used for specifying color and font attributes like boldness.  The only
> >> other kind of escape sequence we print in Git is Erase in Line, ending
> >> with "K".  That's not used for columnar output, so this difference
> >> actually doesn't matter here.
> >
> > Arguably, the "Erase in Line" thing should re-set the width to 0, no?
> > But as you say, this is not needed for this patch.
>
> It doesn't move the cursor, just clears the characters to the right, to
> the left or both sides, depending on its parameter.  So ignoring it for
> width calculation likethe old code did would be appropriate -- if we'd
> encounter such an escape sequence in text to be shown in columns.

Whoops, you're right. I brainfarted, mistaking it for `\r`... My bad!

Ciao,
Dscho

Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums

[Wambui Karuga]

On Mon, Oct 14, 2019 at 11:27:54AM -0700, Jonathan Tan wrote:
> > Jonathan Tan  writes:
> > 
> > >> > Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> > >> > line but that is not necessary.
> > >> 
> > >> That is absolutely necessary; it is not like "we do not care what
> > >> exact value _COPY gets; it can be any value as long as it is _MOVE
> > >> plus 1", as these are used in set of bits (and again, I do not think
> > >> it is such a brilliant idea to use enum for such a purpose).
> > >
> > > Good point.
> > 
> > Doesn't that also show that enums are not quite a good fit for set
> > of bits (i.e. 1< 
> Well, I agree that it could be better, but with the C language as we
> have it now, I still slightly prefer an enum to a list of #define. Both
> ways, we still have to manually enter values for each flag, but with
> enum, we get better debugger display (at least in gdb) and in the
> function declaration and definition, we can put a specific type (instead
> of "unsigned" or "int"). gdb supports the notion that a few people use
> enums this way too, but if we decide as a project to not use enums in
> this way, that's fine too. For what it's worth, I tried doing a search
> online, but most of the results I got was for C# (where it is
> recommended - they have a "[Flags]" attribute for enums), so maybe I am
> indeed in the minority.

I'll try to pick another set of constants to convert - before this is
agreed on.

Thanks,
wambui karuga

Re: [PATCH v3 02/17] sparse-checkout: create 'init' subcommand

[Derrick Stolee]

On 10/11/2019 6:14 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
>  wrote:
>> ++
>> +The init subcommand also enables the 'extensions.worktreeConfig' setting
>> +and sets the `core.sparseCheckout` setting in the worktree-specific config
>> +file. This prevents the sparse-checkout feature from interfering with other
>> +worktrees.
> 
> I'm afraid that might be mis-parsed by future readers.  Perhaps something 
> like:
> 
> The init subcommand also enables the `core.sparseCheckout` setting.

I like the paragraph below, but the sentence above is repeated from
the earlier paragraph.

> To avoid interfering with other worktrees, it first enables the
> `extensions.worktreeConfig` setting and makes sure to set the
> `core.sparseCheckout` setting in the worktree-specific config file.
> 
>> +enum sparse_checkout_mode {
>> +   MODE_NONE = 0,
>> +   MODE_FULL = 1,
>> +};
> 
> So MODE_FULL is "true" and MODE_NONE is "false".  MODE_NONE seems
> confusing to me, but let's keep reading...
> 
>> +
>> +static int sc_set_config(enum sparse_checkout_mode mode)
>> +{
>> +   struct argv_array argv = ARGV_ARRAY_INIT;
>> +
>> +   if (git_config_set_gently("extensions.worktreeConfig", "true")) {
>> +   error(_("failed to set extensions.worktreeConfig setting"));
>> +   return 1;
>> +   }
>> +
>> +   argv_array_pushl(&argv, "config", "--worktree", 
>> "core.sparseCheckout", NULL);
>> +
>> +   if (mode)
>> +   argv_array_pushl(&argv, "true", NULL);
>> +   else
>> +   argv_array_pushl(&argv, "false", NULL);
> 
> Wait, what?  MODE_FULL is used to specify that you want a sparse
> checkout, and MODE_NONE is used to denote that you want a full (i.e.
> non-sparse) checkout?  These are *very* confusing names.

I understand they are confusing, hopefully it makes more sense with
the cone mode later.

* NONE == "No patterns at all"
* FULL == "all patterns allowed"
* CONE == "only cone patterns" (appears later)

Since this is just an internal detail, what if I switched it to

* MODE_NO_PATTERNS
* MODE_ALL_PATTERNS
* MODE_CONE_PATTERNS

Would that make more sense?

>> +static int sparse_checkout_init(int argc, const char **argv)
>> +{
>> +   struct pattern_list pl;
>> +   char *sparse_filename;
>> +   FILE *fp;
>> +   int res;
>> +
>> +   if (sc_set_config(MODE_FULL))
>> +   return 1;
> 
> Seems confusing here too.
> 
> 
> Everything else in the patch looks good, though.

Thanks,
-Stolee

Re: [PATCH v3 05/17] sparse-checkout: add '--stdin' option to set subcommand

[Derrick Stolee]

On 10/11/2019 6:27 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
>  wrote:
>>
>> From: Derrick Stolee 
>>
>> The 'git sparse-checkout set' subcommand takes a list of patterns
>> and places them in the sparse-checkout file. Then, it updates the
>> working directory to match those patterns. For a large list of
>> patterns, the command-line call can get very cumbersome.
>>
>> Add a '--stdin' option to instead read patterns over standard in.
>>
>> Signed-off-by: Derrick Stolee 
>> ---
>>  builtin/sparse-checkout.c  | 40 --
>>  t/t1091-sparse-checkout-builtin.sh | 27 
>>  2 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 52d4f832f3..68f3d8433e 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -145,6 +145,11 @@ static int write_patterns_and_update(struct 
>> pattern_list *pl)
>> char *sparse_filename;
>> FILE *fp;
>>
>> +   if (!core_apply_sparse_checkout) {
>> +   warning(_("core.sparseCheckout is disabled, so changes to 
>> the sparse-checkout file will have no effect"));
>> +   warning(_("run 'git sparse-checkout init' to enable the 
>> sparse-checkout feature"));
>> +   }
>> +
>> sparse_filename = get_sparse_checkout_filename();
>> fp = fopen(sparse_filename, "w");
>> write_patterns_to_file(fp, pl);
>> @@ -154,16 +159,47 @@ static int write_patterns_and_update(struct 
>> pattern_list *pl)
>> return update_working_directory();
>>  }
>>
>> +static char const * const builtin_sparse_checkout_set_usage[] = {
>> +   N_("git sparse-checkout set [--stdin|]"),
>> +   NULL
>> +};
>> +
>> +static struct sparse_checkout_set_opts {
>> +   int use_stdin;
>> +} set_opts;
>> +
>>  static int sparse_checkout_set(int argc, const char **argv, const char 
>> *prefix)
>>  {
>> static const char *empty_base = "";
>> int i;
>> struct pattern_list pl;
>> int result;
>> +
>> +   static struct option builtin_sparse_checkout_set_options[] = {
>> +   OPT_BOOL(0, "stdin", &set_opts.use_stdin,
>> +N_("read patterns from standard in")),
>> +   OPT_END(),
>> +   };
>> +
>> memset(&pl, 0, sizeof(pl));
>>
>> -   for (i = 1; i < argc; i++)
>> -   add_pattern(argv[i], empty_base, 0, &pl, 0);
>> +   argc = parse_options(argc, argv, prefix,
>> +builtin_sparse_checkout_set_options,
>> +builtin_sparse_checkout_set_usage,
>> +PARSE_OPT_KEEP_UNKNOWN);
> 
> Does this mean users can also spell it 'git sparse-checkout --stdin
> set', instead of the expected 'git sparse-checkout set --stdin'?

No, because the parse_options() inside cmd_sparse_checkout() parses until
it doesn't recognize an option. ('stdin' in your example). After we "consume"
the subcommand "set", we call this method and the parse_options() can then
read the '--stdin'.

Here is the output from my local command of 'git sparse-checkout --stdin set'
at this commit:

$ ./git sparse-checkout --stdin set
error: unknown option `stdin'
usage: git sparse-checkout [init|list|set] 

Thanks,
-Stolee

Re: [PATCH v3 13/17] read-tree: show progress by default

[Derrick Stolee]

On 10/12/2019 6:16 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
>  wrote:
>>
>> From: Derrick Stolee 
>>
>> The read-tree builtin has a --verbose option that signals to show
>> progress and other data while updating the index. Update this to
>> be on by default when stderr is a terminal window.
>>
>> This will help tools like 'git sparse-checkout' to automatically
>> benefit from progress indicators when a user runs these commands.
> 
> This change seems fine, but in patch 2 you said:
> 
>> The use of running another process for 'git read-tree' is sub-
>> optimal. This will be removed in a later change.
> 
> leaving me slightly confused about the goal/plan.

True, this is not necessary for the whole series. I created this
patch as a way to show progress in our microsoft/git fork [1], then
removed the read-tree call in a later change [2]. When preparing v3,
I took all of the changes together.

I thought this was valuable on its own, for those users who are
using the old mechanisms for sparse-checkout updates.

Thanks,
-Stolee

[1] https://github.com/microsoft/git/pull/200

[2] https://github.com/microsoft/git/pull/204

Re: [PATCH v3 15/17] sparse-checkout: update working directory in-process

[Derrick Stolee]

On 10/12/2019 6:57 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
>  wrote:
>>
>> From: Derrick Stolee 
>>
>> The sparse-checkout builtin used 'git read-tree -mu HEAD' to update the
>> skip-worktree bits in the index and to update the working directory.
>> This extra process is overly complex, and prone to failure. It also
>> requires that we write our changes to the sparse-checkout file before
>> trying to update the index.
>>
>> Remove this extra process call by creating a direct call to
>> unpack_trees() in the same way 'git read-tree -mu HEAD' does. In
>> adition, provide an in-memory list of patterns so we can avoid
> 
> s/adition/addition/
> 
>> reading from the sparse-checkout file. This allows us to test a
>> proposed change to the file before writing to it.
>>
>> Signed-off-by: Derrick Stolee 
>> ---
>>  builtin/read-tree.c|  2 +-
>>  builtin/sparse-checkout.c  | 85 +-
>>  t/t1091-sparse-checkout-builtin.sh | 17 ++
>>  unpack-trees.c |  5 +-
>>  unpack-trees.h |  3 +-
>>  5 files changed, 95 insertions(+), 17 deletions(-)
>>
>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
>> index 69963d83dc..d7eeaa26ec 100644
>> --- a/builtin/read-tree.c
>> +++ b/builtin/read-tree.c
>> @@ -186,7 +186,7 @@ int cmd_read_tree(int argc, const char **argv, const 
>> char *cmd_prefix)
>>
>> if (opts.reset || opts.merge || opts.prefix) {
>> if (read_cache_unmerged() && (opts.prefix || opts.merge))
>> -   die("You need to resolve your current index first");
>> +   die(_("You need to resolve your current index 
>> first"));
> 
> A good change, but isn't this unrelated to the current commit?

It's related because I'm repeating the error in the sparse-checkout builtin, but
it should be localized in both places.

>> stage = opts.merge = 1;
>> }
>> resolve_undo_clear();
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 25786f8bb0..542d57fac6 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -7,6 +7,11 @@
>>  #include "run-command.h"
>>  #include "strbuf.h"
>>  #include "string-list.h"
>> +#include "cache.h"
>> +#include "cache-tree.h"
>> +#include "lockfile.h"
>> +#include "resolve-undo.h"
>> +#include "unpack-trees.h"
>>
>>  static char const * const builtin_sparse_checkout_usage[] = {
>> N_("git sparse-checkout [init|list|set|disable] "),
>> @@ -60,18 +65,53 @@ static int sparse_checkout_list(int argc, const char 
>> **argv)
>> return 0;
>>  }
>>
>> -static int update_working_directory(void)
>> +static int update_working_directory(struct pattern_list *pl)
>>  {
>> -   struct argv_array argv = ARGV_ARRAY_INIT;
>> int result = 0;
>> -   argv_array_pushl(&argv, "read-tree", "-m", "-u", "HEAD", NULL);
>> +   struct unpack_trees_options o;
>> +   struct lock_file lock_file = LOCK_INIT;
>> +   struct object_id oid;
>> +   struct tree *tree;
>> +   struct tree_desc t;
>> +   struct repository *r = the_repository;
>>
>> -   if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> -   error(_("failed to update index with new sparse-checkout 
>> paths"));
>> -   result = 1;
>> +   if (repo_read_index_unmerged(r))
>> +   die(_("You need to resolve your current index first"));
> 
> Well, at least that ensures that the user gets a good error message.
> I'm not sure I like the error, because e.g. if a user hits a conflict
> while merging in a sparse checkout and wants to return to a non-sparse
> checkout because they think other files might help them resolve the
> conflicts, then they ought to be able to do it.  Basically, unless
> they are trying use sparsification to remove entries from the working
> directory that differ from the index (and conflicted entries always
> differ), then it seems like we should be able to support
> sparsification despite the presence of conflicts.
> 
> Your series is long enough, doesn't make this problem any worse (and
> appears to make it slightly better), and so you really don't need to
> tackle that problem in this series. I'm just stating a gripe with
> sparse checkouts again.  :-)

Absolutely, we should revisit the entire feature and how it handles these
conflicts in the best possible ways. As far as I can see, the only way these
conflicts arise is if the user creates conflicting files _outside_ their
sparse cone and then expand their cone. Finding all the strange cases
will require experimentation.
 
> [...]
> 
>>  static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf 
>> *path)
>>  {
>> -   struct pattern_entry *e = xmalloc(sizeof(struct pattern_entry));
>> +   struct pattern_entry *e = xmalloc(sizeof(*e));
> 
> This is a good fix, but shouldn't it be squashed into the
> "spa

Re: [PATCH v3 16/17] sparse-checkout: write using lockfile

[Derrick Stolee]

On 10/12/2019 6:59 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
>  wrote:
>>
>> From: Derrick Stolee 
>>
>> If two 'git sparse-checkout set' subcommands are launched at the
>> same time, the behavior can be unexpected as they compete to write
>> the sparse-checkout file and update the working directory.
>>
>> Take a lockfile around the writes to the sparse-checkout file. In
>> addition, acquire this lock around the working directory update
>> to avoid two commands updating the working directory in different
>> ways.
> 
> Wow, there's something I never would have thought to check.  Did you
> have folks run into this, or is this just some defensive programming?
> Either way, I'm impressed.

This is defensive programming thanks to Kevin Willford's careful
review [1].

-Stolee

[1] https://github.com/microsoft/git/pull/204#discussion_r330252848

Re: [PATCH 07/11] graph: commit and post-merge lines for left-skewed merges

[Johannes Schindelin]

Hi James,

On Mon, 14 Oct 2019, James Coglan wrote:

> [...] How should I go about sharing the current state of my patch
> series after I've incorporated all the changes suggested here? Should
> I post them as replies on this thread, or start a new pull request via
> GitGitGadget?

Just force-push the branch, and tell GitGitGadget to `/submit`. It will
take care of tagging a new iteration, generating a range-diff, and
sending it off to the list.

Thanks,
Dscho

[PATCH v5 1/3] format-patch: change erroneous and condition

[Denton Liu]

Commit 30984ed2e9 (format-patch: support deep threading, 2009-02-19),
introduced the following lines:

#define THREAD_SHALLOW 1

[...]

thread = git_config_bool(var, value) && THREAD_SHALLOW;

Since git_config_bool() returns a bool, the trailing `&& THREAD_SHALLOW`
is a no-op.

In Python, `x and y` is equivalent to `y if x else x`[1]. Since this
seems to be a Python-ism that's mistakenly leaked into our code, convert
this to the equivalent C expression.

[1]: https://docs.python.org/3/reference/expressions.html#boolean-operations

Signed-off-by: Denton Liu 
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 44b10b3415..351f4ffcfd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -835,7 +835,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
thread = THREAD_SHALLOW;
return 0;
}
-   thread = git_config_bool(var, value) && THREAD_SHALLOW;
+   thread = git_config_bool(var, value) ? THREAD_SHALLOW : 
THREAD_UNSET;
return 0;
}
if (!strcmp(var, "format.signoff")) {
-- 
2.23.0.17.g315c308950

[PATCH v5 0/3] format-patch: learn --cover-from-description option

[Denton Liu]

Currently, format-patch only puts "*** SUBJECT HERE ***" when a cover
letter is generated. However, it is already smart enough to be able to
populate the cover letter with the branch description so there's no
reason why it cannot populate the subject as well.

Teach format-patch the `--cover-from-description` option and
corresponding `format.coverFromDescription` configuration option which
will allow it to populate not only the body but the subject as well.

Changes since v4:

* Modify 1/3 to more closely reflect intent of the original author

* Incorporate Junio's suggestions into the documentation

* Extract branch desc logic into pp_from_desc()

* Fix broken tests

Changes since v3:

* Change --infer-cover-subject to --cover-from-description

* No more test cleanup patches (they were merged in
  'dl/format-patch-doc-test-cleanup')

Changes since v2:

* Break 1/4 into many different patches (one per paragraph of the
  original patch)

* Incorporate Eric's documentation/commit message suggestions

Changes since v1:

* Incorporate Eric's suggestions for cleanup in all patches

* Add patch 3/4 to make it clear what is the default value for
  format.coverLetter (since format.inferCoverSubject was borrowed from
  this config but it also did not state what the default value was)

* In 1/4, rename all instances of "expected" to "expect"

Denton Liu (3):
  format-patch: change erroneous and condition
  format-patch: use enum variables
  format-patch: teach --cover-from-description option

 Documentation/config/format.txt|   6 +
 Documentation/git-format-patch.txt |  22 
 builtin/log.c  | 125 +++--
 t/t4014-format-patch.sh| 172 +
 t/t9902-completion.sh  |   5 +-
 5 files changed, 296 insertions(+), 34 deletions(-)

Range-diff against v4:
1:  267bc00dc8 ! 1:  56fb230ad2 format-patch: remove erroneous and condition
@@ Metadata
 Author: Denton Liu 
 
  ## Commit message ##
-format-patch: remove erroneous and condition
+format-patch: change erroneous and condition
 
 Commit 30984ed2e9 (format-patch: support deep threading, 2009-02-19),
 introduced the following lines:
@@ Commit message
 thread = git_config_bool(var, value) && THREAD_SHALLOW;
 
 Since git_config_bool() returns a bool, the trailing `&& 
THREAD_SHALLOW`
-is a no-op. Remove this erroneous and condition.
+is a no-op.
+
+In Python, `x and y` is equivalent to `y if x else x`[1]. Since this
+seems to be a Python-ism that's mistakenly leaked into our code, 
convert
+this to the equivalent C expression.
+
+[1]: 
https://docs.python.org/3/reference/expressions.html#boolean-operations
 
 Signed-off-by: Denton Liu 
-Signed-off-by: Junio C Hamano 
 
  ## builtin/log.c ##
 @@ builtin/log.c: static int git_format_config(const char *var, const char 
*value, void *cb)
@@ builtin/log.c: static int git_format_config(const char *var, const char 
*value,
return 0;
}
 -  thread = git_config_bool(var, value) && THREAD_SHALLOW;
-+  thread = git_config_bool(var, value);
++  thread = git_config_bool(var, value) ? THREAD_SHALLOW : 
THREAD_UNSET;
return 0;
}
if (!strcmp(var, "format.signoff")) {
2:  638a5b40d2 ! 2:  e2769092fa format-patch: use enum variables
@@ Commit message
 variables to use these new definitions.
 
 Signed-off-by: Denton Liu 
-Signed-off-by: Junio C Hamano 
 
  ## builtin/log.c ##
 @@ builtin/log.c: static void add_header(const char *value)
3:  3289ce62bb ! 3:  315c308950 format-patch: teach --cover-from-description 
option
@@ Commit message
 populate different parts of the cover letter (including the subject
 now).
 
-Signed-off-by: Denton Liu 
-Signed-off-by: Junio C Hamano 
-
  ## Documentation/config/format.txt ##
 @@ Documentation/config/format.txt: format.subjectPrefix::
The default for format-patch is to output files with the '[PATCH]'
@@ Documentation/git-format-patch.txt: will want to ensure that threading 
is disabl
 ++
 +If `` is `message` or `default`, the cover letter subject will be
 +populated with placeholder text. The body of the cover letter will be
-+populated with the branch's description.
++populated with the branch's description. This is the default mode when
++no configuration nor command line option is specified.
 ++
-+If `` is `subject`, the beginning of the branch description (up to
-+the first blank line) will populate the cover letter subject. The
-+remainder of the description will populate the body of the cover
-+letter.
++If `` is `subject`, the first paragraph of the branch description 
will
++pop

[PATCH v5 2/3] format-patch: use enum variables

[Denton Liu]

Before, `thread` and `config_cover_letter` were defined as ints even
though they behaved as enums. Define actual enums and change these
variables to use these new definitions.

Signed-off-by: Denton Liu 
---
Hi Junio, I double-checked and made sure that there is no arithmetic
done on the new enums.

 builtin/log.c | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 351f4ffcfd..d212a8305d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -764,24 +764,28 @@ static void add_header(const char *value)
item->string[len] = '\0';
 }
 
-#define THREAD_SHALLOW 1
-#define THREAD_DEEP 2
-static int thread;
+enum cover_setting {
+   COVER_UNSET,
+   COVER_OFF,
+   COVER_ON,
+   COVER_AUTO
+};
+
+enum thread_level {
+   THREAD_UNSET,
+   THREAD_SHALLOW,
+   THREAD_DEEP
+};
+
+static enum thread_level thread;
 static int do_signoff;
 static int base_auto;
 static char *from;
 static const char *signature = git_version_string;
 static const char *signature_file;
-static int config_cover_letter;
+static enum cover_setting config_cover_letter;
 static const char *config_output_directory;
 
-enum {
-   COVER_UNSET,
-   COVER_OFF,
-   COVER_ON,
-   COVER_AUTO
-};
-
 static int git_format_config(const char *var, const char *value, void *cb)
 {
struct rev_info *rev = cb;
@@ -1248,9 +1252,9 @@ static int output_directory_callback(const struct option 
*opt, const char *arg,
 
 static int thread_callback(const struct option *opt, const char *arg, int 
unset)
 {
-   int *thread = (int *)opt->value;
+   enum thread_level *thread = (enum thread_level *)opt->value;
if (unset)
-   *thread = 0;
+   *thread = THREAD_UNSET;
else if (!arg || !strcmp(arg, "shallow"))
*thread = THREAD_SHALLOW;
else if (!strcmp(arg, "deep"))
-- 
2.23.0.17.g315c308950

[PATCH v5 3/3] format-patch: teach --cover-from-description option

[Denton Liu]

Before, when format-patch generated a cover letter, only the body would
be populated with a branch's description while the subject would be
populated with placeholder text. However, users may want to have the
subject of their cover letter automatically populated in the same way.

Teach format-patch to accept the `--cover-from-description` option and
corresponding `format.coverFromDescription` config, allowing users to
populate different parts of the cover letter (including the subject
now).

Signed-off-by: Denton Liu 
---
 Documentation/config/format.txt|   6 +
 Documentation/git-format-patch.txt |  22 
 builtin/log.c  |  95 
 t/t4014-format-patch.sh| 172 +
 t/t9902-completion.sh  |   5 +-
 5 files changed, 279 insertions(+), 21 deletions(-)

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index cb629fa769..735dfcf827 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -36,6 +36,12 @@ format.subjectPrefix::
The default for format-patch is to output files with the '[PATCH]'
subject prefix. Use this variable to change that prefix.
 
+format.coverFromDescription::
+   The default mode for format-patch to determine which parts of
+   the cover letter will be populated using the branch's
+   description. See the `--cover-from-description` option in
+   linkgit:git-format-patch[1].
+
 format.signature::
The default for format-patch is to output a signature containing
the Git version number. Use this variable to change that default.
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 0ac56f4b70..6800e1ab9a 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -19,6 +19,7 @@ SYNOPSIS
   [--start-number ] [--numbered-files]
   [--in-reply-to=] [--suffix=.]
   [--ignore-if-in-upstream]
+  [--cover-from-description=]
   [--rfc] [--subject-prefix=]
   [(--reroll-count|-v) ]
   [--to=] [--cc=]
@@ -171,6 +172,26 @@ will want to ensure that threading is disabled for `git 
send-email`.
patches being generated, and any patch that matches is
ignored.
 
+--cover-from-description=::
+   Controls which parts of the cover letter will be automatically
+   populated using the branch's description.
++
+If `` is `message` or `default`, the cover letter subject will be
+populated with placeholder text. The body of the cover letter will be
+populated with the branch's description. This is the default mode when
+no configuration nor command line option is specified.
++
+If `` is `subject`, the first paragraph of the branch description will
+populate the cover letter subject. The remainder of the description will
+populate the body of the cover letter.
++
+If `` is `auto`, if the first paragraph of the branch description
+is greater than 100 bytes, then the mode will be `message`, otherwise
+`subject` will be used.
++
+If `` is `none`, both the cover letter subject and body will be
+populated with placeholder text.
+
 --subject-prefix=::
Instead of the standard '[PATCH]' prefix in the subject
line, instead use '[]'. This
@@ -347,6 +368,7 @@ with configuration variables.
signOff = true
outputDirectory = 
coverLetter = auto
+   coverFromDescription = auto
 
 
 
diff --git a/builtin/log.c b/builtin/log.c
index d212a8305d..af33fe9ffb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -37,6 +37,7 @@
 #include "range-diff.h"
 
 #define MAIL_DEFAULT_WRAP 72
+#define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -777,6 +778,13 @@ enum thread_level {
THREAD_DEEP
 };
 
+enum cover_from_description {
+   COVER_FROM_NONE,
+   COVER_FROM_MESSAGE,
+   COVER_FROM_SUBJECT,
+   COVER_FROM_AUTO
+};
+
 static enum thread_level thread;
 static int do_signoff;
 static int base_auto;
@@ -785,6 +793,23 @@ static const char *signature = git_version_string;
 static const char *signature_file;
 static enum cover_setting config_cover_letter;
 static const char *config_output_directory;
+static enum cover_from_description cover_from_description_mode = 
COVER_FROM_MESSAGE;
+
+static enum cover_from_description parse_cover_from_description(const char 
*arg)
+{
+   if (!arg || !strcmp(arg, "default"))
+   return COVER_FROM_MESSAGE;
+   else if (!strcmp(arg, "none"))
+   return COVER_FROM_NONE;
+   else if (!strcmp(arg, "message"))
+   return COVER_FROM_MESSAGE;
+   else if (!strcmp(arg, "subject"))
+   return COVER_FROM_SUBJECT;
+   else if (!strcmp(arg, "auto"))
+   return COV

Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums

[Jonathan Tan]

> > Well, I agree that it could be better, but with the C language as we
> > have it now, I still slightly prefer an enum to a list of #define. Both
> > ways, we still have to manually enter values for each flag, but with
> > enum, we get better debugger display (at least in gdb) and in the
> > function declaration and definition, we can put a specific type (instead
> > of "unsigned" or "int"). gdb supports the notion that a few people use
> > enums this way too, but if we decide as a project to not use enums in
> > this way, that's fine too. For what it's worth, I tried doing a search
> > online, but most of the results I got was for C# (where it is
> > recommended - they have a "[Flags]" attribute for enums), so maybe I am
> > indeed in the minority.
> 
> I'll try to pick another set of constants to convert - before this is
> agreed on.

Thanks - perhaps that's the best way to proceed for now. Do you remember
where you found the idea to convert #define to enum? Maybe I could add a
note there to avoid converting bitsets/bitflags.

[RFC PATCH 0/1] Teach remote add a --prefix-tags option

[Wink Saville]

Hello,

This patch was originally created as a pull request on github [1] and
then languisheh as I forgot about it. Recently I was asked to revive it
and have done so. I've rebased on top of master and validated it still
works.

Please review.

-- Wink

[1]: https://github.com/git/git/pull/486

Wink Saville (1):
  Teach remote add the --prefix-tags option

 Documentation/git-remote.txt |  8 +--
 builtin/remote.c | 42 
 remote.c |  2 ++
 3 files changed, 46 insertions(+), 6 deletions(-)

-- 
2.16.2.7164.g7daebe18fb

[RFC PATCH 1/1] Teach remote add the --prefix-tags option

[Wink Saville]

When --prefix-tags is passed to `git remote add` the tagopt is set to
--prefix-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.

For example:
  $ git remote add -f --prefix-tags gbenchmark g...@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]  clangtidy   -> gbenchmark/clangtidy
   * [new branch]  iter_report -> gbenchmark/iter_report
   * [new branch]  master  -> gbenchmark/master
   * [new branch]  releasing   -> gbenchmark/releasing
   * [new branch]  reportercleanup -> gbenchmark/reportercleanup
   * [new branch]  rmheaders   -> gbenchmark/rmheaders
   * [new branch]  v2  -> gbenchmark/v2
   * [new tag] v0.0.9  -> tags/gbenchmark/v0.0.9
   * [new tag] v0.1.0  -> tags/gbenchmark/v0.1.0
   * [new tag] v1.0.0  -> tags/gbenchmark/v1.0.0
   * [new tag] v1.1.0  -> tags/gbenchmark/v1.1.0
   * [new tag] v1.2.0  -> tags/gbenchmark/v1.2.0
   * [new tag] v1.3.0  -> tags/gbenchmark/v1.3.0
   * [new tag] v1.4.0  -> tags/gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
url = g...@github.com:google/benchmark
fetch = +refs/heads/*:refs/remotes/gbenchmark/*
fetch = +refs/tags/*:refs/remotes/tags/gbenchmark/*
tagopt = --prefix-tags

Based on a solution proposed by Junio on the email list [1]

[1]: 
https://public-inbox.org/git/xmqqbme51rgn@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Helped-by: Junio C Hamano 
Helped-by: Jacob Keller 
Signed-off-by: Wink Saville 
---
 Documentation/git-remote.txt |  8 +--
 builtin/remote.c | 42 
 remote.c |  2 ++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 9659abbf8e..db0238e8bd 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t ] [-m ] [-f] [--[no-]tags] 
[--mirror=]  
+'git remote add' [-t ] [-m ] [-f] [--tags | --prefix-tags | 
--no-tags] [--mirror=]  
 'git remote rename'  
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
@@ -54,7 +54,11 @@ With `-f` option, `git fetch ` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch ` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --prefix-tags to import them
+to refs/remotes/tags//.
++
+With `--prefix-tags` option, `git fetch ` imports every tag from the
+remote repository to refs/remotes/tags//.
 +
 With `--no-tags` option, `git fetch ` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 5591cef775..88991f9fbe 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -14,7 +14,7 @@
 
 static const char * const builtin_remote_usage[] = {
N_("git remote [-v | --verbose]"),
-   N_("git remote add [-t ] [-m ] [-f] [--tags | 
--no-tags] [--mirror=]  "),
+   N_("git remote add [-t ] [-m ] [-f] [--prefix-tags | 
--tags | --no-tags] [--mirror=]  "),
N_("git remote rename  "),
N_("git remote remove "),
N_("git remote set-head  (-a | --auto | -d | --delete | 
)"),
@@ -104,7 +104,8 @@ static int fetch_remote(const char *name)
 enum {
TAGS_UNSET = 0,
TAGS_DEFAULT = 1,
-   TAGS_SET = 2
+   TAGS_SET = 2,
+   TAGS_SET_PREFIX = 3
 };
 
 #define MIRROR_NONE 0
@@ -126,6 +127,14 @@ static void add_branch(const char *key, const char 
*branchname,
git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *remotename,
+   struct strbuf *tmp)
+{
+   strbuf_reset(tmp);
+   strbuf_addf(tmp, "+refs/tags/*:refs/remotes/tags/%s/*", remotename);
+   git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
"\t use --mirror=fetch or --mirror=push instead");
@@ -164,6 +173,9 @@ static int add(int argc, const char **argv)
OPT_SET_INT(0, "tags", &fetch_tags,
N_("import all tags and associated objects when 
fetching"),
TAGS_SET),
+   OPT_SET_INT(0, "prefix-tags", &fetch_tags,
+   N_("import all tags and associated ob

Re: Git Gui: Branch->create currently fails...

[Philip Oakley]

On 14/10/2019 18:57, Pratyush Yadav wrote:

list "refs/heads/MSVC-README" [list "commit"
"056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"]
[reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]]
"compat/vcSegmentation fault


Not exactly the same, but almost. Ends the same place, with as similar short
line.
This is run inside the bash that is started directly by the git-for-windows
sdk start icon. (Target: C:\git-sdk-64\git-bash.exe   Stat in:
C:/git-sdk-64/)

so looks to be MSYS2/bash related.

Ah, so it is an upstream issue. I guess we can just report it and wait
for them to fix it.
I'd missed the final 'Segmentation fault' on the last line in the bash 
output that wasn't there for the captured file.


That was repeatable in re-testing.
But failed if I changed the $fmt string to a plain text 500 char string 
("1234567890123...").


I've still to trim down the complicated $fmt string to see if I can see 
where that seg fault starts (i.e. some form of MVCE), so that it can be 
investigated.
Possibly should check if the --tcl flag actually invokes any tcl! 
Otherwise it's fully in the Git/G-f-W zone.


...
Just rebuilt (I hope) the Windows Subsystem for Linux (WSL) with git 
v2.23.0 installed and got:


list "refs/heads/MSVC-README" [list "commit" 
"056fb95c8e983ec07e9f5f8baa0b119bf3d13fed" [concat "" "Philip Oakley"] 
[reformat_date [concat "" "Sun May 19 22:33:37 2019 +0100"]] 
"compat/vcbuild/README: clean/update 'vcpkg' env for Visual Studio 
updates"] [list "" "" "" [reformat_date ""] ""]

munmap_chunk(): invalid pointer
Aborted (core dumped)
root@Philip-Win10:/mnt/c/git-sdk-64/usr/src/git#


That said, haven't got the gitk and git gui to work yet on the WSL 
because it doesn't like the tcl/tk.


It's a bit of a hole digging exercise.

Philip

Re: [PATCH 2/3] sequencer: use run_commit_hook()

[Johannes Schindelin]

Hi Phillip,

On Mon, 14 Oct 2019, Phillip Wood wrote:

> Hi Dscho & Junio
>
> On 11/10/2019 05:24, Junio C Hamano wrote:
> > Johannes Schindelin  writes:
> >
> >>>  builtin/commit.c | 22 --
> >>>  commit.h |  3 ---
> >>>  sequencer.c  | 45 ++---
> >>>  sequencer.h  |  2 ++
> >>>  4 files changed, 36 insertions(+), 36 deletions(-)
> >>
> >> Hmm. I would have thought that `commit.c` would be a more logical home
> >> for that function (and that the declaration could remain in `commit.h`)?
> >
> > Good correction.
>
> There are some other public commit related functions in sequencer.c -
> print_commit_summary(), commit_post_rewrite(), rest_is_empty(),
> cleanup_message(), message_is_empty(), template_untouched(),
> update_head_with_reflog() . Would you like to see them moved to commit.c
> (probably as a separate series)?

I don't think that it is necessary to move any of those functions out of
their existing habitat just yet. While I haven't looked more closely
which of these functions are specific to the sequencer and which are
more generic, I would argue that moving any of them is outside of the
goals of your patch series.

Thanks,
Dscho

Re: [PATCH] fetch-pack: write fetched refs to .promisor

[Josh Steadmon]

I have a few questions below, but they're probably due to lack of a full
understanding on my part of how packfiles are managed.

On 2019.08.26 14:47, Jonathan Tan wrote:
> The specification of promisor packfiles (in partial-clone.txt) states
> that the .promisor files that accompany packfiles do not matter (just
> like .keep files), so whenever a packfile is fetched from the promisor
> remote, Git has been writing empty .promisor files. But these files
> could contain more useful information.
> 
> So instead of writing empty files, write the refs fetched to these
> files. This makes it easier to debug issues with partial clones, as we
> can identify what refs (and their associated hashes) were fetched at the
> time the packfile was downloaded, and if necessary, compare those hashes
> against what the promisor remote reports now.
> 
> Signed-off-by: Jonathan Tan 
> ---
> As written in the NEEDSWORK comment, repack does not preserve the
> contents of .promisor files, but I thought I'd send this out anyway as
> this change is already useful for users who don't run repack much.
> ---
>  builtin/repack.c |  5 +
>  fetch-pack.c | 41 
>  t/t5616-partial-clone.sh |  8 
>  3 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 632c0c0a79..8c1621d414 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -232,6 +232,11 @@ static void repack_promisor_objects(const struct 
> pack_objects_args *args,
>   /*
>* pack-objects creates the .pack and .idx files, but not the
>* .promisor file. Create the .promisor file, which is empty.
> +  *
> +  * NEEDSWORK: fetch-pack generates non-empty .promisor files,
> +  * but this would not preserve their contents. Maybe
> +  * concatenate the contents of all .promisor files instead of
> +  * just creating a new empty file.
>*/
>   promisor_name = mkpathdup("%s-%s.promisor", packtmp,
> line.buf);

Since this is just diagnostic information, it seems fine. Maybe
explicitly note in the comment what information is being lost?

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 65be043f2a..07029e1bbf 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -758,8 +758,33 @@ static int sideband_demux(int in, int out, void *data)
>   return ret;
>  }
>  
> +static void write_promisor_file(const char *keep_name,
> + struct ref **sought, int nr_sought)
> +{
> + struct strbuf promisor_name = STRBUF_INIT;
> + int suffix_stripped;
> + FILE *output;
> + int i;
> +
> + strbuf_addstr(&promisor_name, keep_name);
> + suffix_stripped = strbuf_strip_suffix(&promisor_name, ".keep");
> + if (!suffix_stripped)
> + BUG("name of pack lockfile should end with .keep (was '%s')",
> + keep_name);
> + strbuf_addstr(&promisor_name, ".promisor");
> +
> + output = xfopen(promisor_name.buf, "w");
> + for (i = 0; i < nr_sought; i++)
> + fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
> + sought[i]->name);
> + fclose(output);
> +
> + strbuf_release(&promisor_name);
> +}
> +

I am not sure why we want to tie creating the .promisor to creating the
lockfile. I'll keep reading and see if it becomes clear later. Other
than that, the logic here seems clear.

>  static int get_pack(struct fetch_pack_args *args,
> - int xd[2], char **pack_lockfile)
> + int xd[2], char **pack_lockfile,
> + struct ref **sought, int nr_sought)
>  {
>   struct async demux;
>   int do_keep = args->keep_pack;
> @@ -821,7 +846,13 @@ static int get_pack(struct fetch_pack_args *args,
>   }
>   if (args->check_self_contained_and_connected)
>   argv_array_push(&cmd.args, 
> "--check-self-contained-and-connected");
> - if (args->from_promisor)
> + /*
> +  * If we're obtaining the filename of a lockfile, we'll use
> +  * that filename to write a .promisor file with more
> +  * information below. If not, we need index-pack to do it for
> +  * us.
> +  */
> + if (!(do_keep && pack_lockfile) && args->from_promisor)
>   argv_array_push(&cmd.args, "--promisor");
>   }
>   else {

This makes me wonder why we don't also change index-pack to write a
similar message to the .promisor. I guess there's potentially too much
information to shove all the refs on the command-line?

> @@ -859,6 +890,8 @@ static int get_pack(struct fetch_pack_args *args,
>   die(_("fetch-pack: unable to fork off %s"), cmd_name);
>   if (do_keep && pack_lockfile) {
>   *pack_lockfile = index_pac

Re: [PATCH v2 1/1] doc: Change zsh git completion file name

[SZEDER Gábor]

On Fri, Oct 11, 2019 at 10:54:28AM -0700, Maxim Belsky via GitGitGadget wrote:
> From: Maxim Belsky 
> 
> The original comment does not describe type of ~/.zsh/_git explicitly
> and zsh does not warn or fail if a user create it as a dictionary.
> So unexperienced users could be misled by the original comment.
> 
> There is a small update to clarify it.
> 
> Signed-off-by: Maxim Belsky 
> Helped-by: Johannes Schindelin 
> Helped-by: Junio C Hamano 
> ---
>  contrib/completion/git-completion.zsh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.zsh 
> b/contrib/completion/git-completion.zsh
> index 886bf95d1f..eef4eff53d 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -11,8 +11,9 @@
>  #
>  #  zstyle ':completion:*:*:git:*' script ~/.git-completion.zsh
>  #
> -# The recommended way to install this script is to copy to '~/.zsh/_git', and
> -# then add the following to your ~/.zshrc file:
> +# The recommended way to install this script is to make a copy of it in
> +# ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following

These updated instructions don't work for me, following them gives me
zsh's git completion instead of ours:

  $ ls -l /etc/bash_completion.d/git
  -rwxr-xr-x 1 root root 72165 Oct 15 01:27 /etc/bash_completion.d/git
  $ ls -l ~/.zsh/
  total 8
  -rw-r--r-- 1 szeder szeder 6013 Oct 15 01:27 git-completion.zsh
  $ cat ~/.zshrc 
  # Use modern completion system
  autoload -Uz compinit
  compinit
  
  fpath=(~/.zsh $fpath)
  $ zsh
  % git init --
  --bare  -- create a bare repository
  --quiet -- do not print any results to stdout
  --separate-git-dir  -- create git dir elsewhere and link it using the gitdir
  --shared-- share repository amongst several users
  --template  -- directory to use as a template for the object databas

That's clearly zsh's fancy completion.

As a non-zsh user I had the impression that it's a well-established
convention that the completion script of a command for zsh is called
'_command', see e.g. all the scripts in:

  https://github.com/zsh-users/zsh-completions/tree/master/src

Instructing users to copy our completion script to
'~/.zsh/git-completion.zsh' goes against this convention.

More importantly, it appears that this is more than a mere convention,
maybe a requirement even; at least renaming our zsh completion script
to follow the convention (IOW following the current install
instructions) makes it work all of a sudden:

  $ mv .zsh/git-completion.zsh .zsh/_git
  $ zsh
  % git init --
  --  --no-separate-git-dir   --shared 
  --bare  --no-template   --template=
  --no-bare   --quiet 
  --no-quiet  --separate-git-dir= 

Now that's our Bash completion script used by zsh.

Re: [PATCH] fetch-pack: write fetched refs to .promisor

[Jonathan Tan]

Thanks for your comments. Rearranging them:

> This makes me wonder why we don't also change index-pack to write a
> similar message to the .promisor. I guess there's potentially too much
> information to shove all the refs on the command-line?

index-pack already is capable of writing messages to .promisor using the
"--promisor" argument. You're right that I'm not using that because I
don't want to run into argument length limits.

> I am not sure why we want to tie creating the .promisor to creating the
> lockfile. I'll keep reading and see if it becomes clear later. Other
> than that, the logic here seems clear.

[snip]

> Apart from using the lockfile name as the base for the .promisor
> filename, I'm still not seeing why we need to tie this to the fact that
> we're creating a lockfile. Could we instead just unconditionally create
> the .promisor when args->from_promisor is set, and then remove the logic
> in the previous chunk that adds the "--promisor" flag to the index-pack
> call?

I'm tying the promisor to the lockfile to avoid overcomplicating things:
fetch-pack currently reads filename information from index-pack only
when there is a lockfile. (It could do so even when there is no
lockfile, but it currently does not.) We need this filename to know what
to call the ".promisor" file.

And the situation that I'm interested in - when the user fetches with
"git fetch" from http or ssh - always uses a lockfile (see
fetch_refs_via_pack() in transport.c). So I'm writing additional data in
this case, and falling back on the "--promisor" flag otherwise. I'll
elaborate in the commit message.

[PATCH v2] fetch-pack: write fetched refs to .promisor

[Jonathan Tan]

The specification of promisor packfiles (in partial-clone.txt) states
that the .promisor files that accompany packfiles do not matter (just
like .keep files), so whenever a packfile is fetched from the promisor
remote, Git has been writing empty .promisor files. But these files
could contain more useful information.

So instead of writing empty files, write the refs fetched to these
files. This makes it easier to debug issues with partial clones, as we
can identify what refs (and their associated hashes) were fetched at the
time the packfile was downloaded, and if necessary, compare those hashes
against what the promisor remote reports now.

This is implemented by teaching fetch-pack to write its own non-empty
.promisor file whenever it knows the name of the pack's lockfile. This
covers the case wherein the user runs "git fetch" with an internal
protocol or HTTP protocol v2 (fetch_refs_via_pack() in transport.c sets
lock_pack) and with HTTP protocol v0/v1 (fetch_git() in remote-curl.c
passes "--lock-pack" to "fetch-pack").

Signed-off-by: Jonathan Tan 
---
Changes from v1:
 - commit message explains scope of change ("it knows the name of the
   pack's lockfile)
 - explained what .promisor contains in comment in builtin/repack.c
 - moved .promisor writing until after we know that index-pack succeeded
---
 builtin/repack.c |  7 ++
 fetch-pack.c | 47 
 t/t5616-partial-clone.sh |  8 +++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 094c2f8ea4..78b23d7a9a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -233,6 +233,13 @@ static void repack_promisor_objects(const struct 
pack_objects_args *args,
/*
 * pack-objects creates the .pack and .idx files, but not the
 * .promisor file. Create the .promisor file, which is empty.
+*
+* NEEDSWORK: fetch-pack sometimes generates non-empty
+* .promisor files containing the ref names and associated
+* hashes at the point of generation of the corresponding
+* packfile, but this would not preserve their contents. Maybe
+* concatenate the contents of all .promisor files instead of
+* just creating a new empty file.
 */
promisor_name = mkpathdup("%s-%s.promisor", packtmp,
  line.buf);
diff --git a/fetch-pack.c b/fetch-pack.c
index 947da545de..b9e63b52ff 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -754,8 +754,33 @@ static int sideband_demux(int in, int out, void *data)
return ret;
 }
 
+static void write_promisor_file(const char *keep_name,
+   struct ref **sought, int nr_sought)
+{
+   struct strbuf promisor_name = STRBUF_INIT;
+   int suffix_stripped;
+   FILE *output;
+   int i;
+
+   strbuf_addstr(&promisor_name, keep_name);
+   suffix_stripped = strbuf_strip_suffix(&promisor_name, ".keep");
+   if (!suffix_stripped)
+   BUG("name of pack lockfile should end with .keep (was '%s')",
+   keep_name);
+   strbuf_addstr(&promisor_name, ".promisor");
+
+   output = xfopen(promisor_name.buf, "w");
+   for (i = 0; i < nr_sought; i++)
+   fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
+   sought[i]->name);
+   fclose(output);
+
+   strbuf_release(&promisor_name);
+}
+
 static int get_pack(struct fetch_pack_args *args,
-   int xd[2], char **pack_lockfile)
+   int xd[2], char **pack_lockfile,
+   struct ref **sought, int nr_sought)
 {
struct async demux;
int do_keep = args->keep_pack;
@@ -817,7 +842,13 @@ static int get_pack(struct fetch_pack_args *args,
}
if (args->check_self_contained_and_connected)
argv_array_push(&cmd.args, 
"--check-self-contained-and-connected");
-   if (args->from_promisor)
+   /*
+* If we're obtaining the filename of a lockfile, we'll use
+* that filename to write a .promisor file with more
+* information below. If not, we need index-pack to do it for
+* us.
+*/
+   if (!(do_keep && pack_lockfile) && args->from_promisor)
argv_array_push(&cmd.args, "--promisor");
}
else {
@@ -871,6 +902,14 @@ static int get_pack(struct fetch_pack_args *args,
die(_("%s failed"), cmd_name);
if (use_sideband && finish_async(&demux))
die(_("error in sideband demultiplexer"));
+
+   /*
+* Now that index-pack has succeeded, write the promisor file using the
+* obtained .keep filename if necessary
+*/
+   if (do_keep && pack_lo

[PATCH 1/1] gitk: Preserve window dimensions on exit when not using ttk themes

[Eric Huber via GitGitGadget]

From: Eric Huber 

Bug was: gitk would overwrite the botwidth setting in .gitk with
a nonsense value when not using tk themes. I'm not sure if this
is the right fix or not but it seems to work. Moving the affected
line within the conditional results in the expected behavior.

Signed-off-by: Eric Huber 
---
 gitk-git/gitk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index abe4805ade..4846f2a983 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2526,9 +2526,9 @@ proc makewindow {} {
 bind %W  {}
 %W sashpos 0 $::geometry(botwidth)
 }
+   bind .pwbottom  {resizecdetpanes %W %w}
 }
-
-bind .pwbottom  {resizecdetpanes %W %w}
+
 pack .ctop -fill both -expand 1
 bindall <1> {selcanvline %W %x %y}
 #bindall  {selcanvline %W %x %y}
-- 
gitgitgadget

[PATCH 0/1] gitk: Preserve window dimensions on exit when not using ttk themes

[Eric Huber via GitGitGadget]

This fix is intended to let gitk preserve the window pane dimensions
regardless of whether ttk is enabled or not. I'm not an expert on Tcl/Tk but
as far as I can tell, this edit works and doesn't cause problems. Please
double-check what I did.

Eric Huber (1):
  gitk: Preserve window dimensions on exit when not using ttk themes

 gitk-git/gitk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 70bf0b755af4d1e66da25b7805cac0e481a082e4
Published-As: 
https://github.com/gitgitgadget/git/releases/tag/pr-389%2Fechuber2%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-389/echuber2/patch-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/389
-- 
gitgitgadget

Re: Local Config File: Submodule initialization looks broken with the --recurse-submodules option when cloning a repository

[Ron]

> I've got someone to test this with the latest 2.23.0 version on archlinux.
>
> It is still happening:
>
> git clone --depth=1 --recurse-submodules --origin upstream 
> https://github.com/git-for-windows/git;
> git -C git config --get submodule.active
> expected true, got .
>
> Anything more I need to add to help this get solved?
>
> Sent with ProtonMail Secure Email.

This appears to have been a false positive, there are 2 different kind of 
"active" parameters in the config file
and this is, according to the docs, expected behavior. I'm not sure if the 
other tester wrote the last line
himself or did git wrote that for him, because on GFW that command only outputs 
the value if there is one.

I cannot reproduce the error anymore that I thought was happening because of 
this, it never caused a problem
when cloning or when applying submodule later, it must have been a coincidence 
at the time.

The effect is "equivalent" as the docs say, whether doing "--recurse-submodule" 
(without specifying pathspec)
at clone time or doing a normal clone and running "git submodule update --init 
--recursive" afterward,
but technically the config file will not be the same.

Sorry for the inconvenience.

[PATCH] remote-curl: pass on atomic capability to remote side

[brian m. carlson]

When pushing more than one reference with the --atomic option, the
server is supposed to perform a single atomic transaction to update the
references, leaving them either all to succeed or all to fail.  This
works fine when pushing locally or over SSH, but when pushing over HTTP,
we fail to pass the atomic capability to the remote side.  In fact, we
have not reported this capability to any remote helpers during the life
of the feature.

Now normally, things happen to work nevertheless, since we actually
check for most types of failures, such as non-fast-forward updates, on
the client side, and just abort the entire attempt.  However, if the
server side reports a problem, such as the inability to lock a ref, the
transaction isn't atomic, because we haven't passed the appropriate
capability over and the remote side has no way of knowing that we wanted
atomic behavior.

Fix this by passing the option from the transport code through to remote
helpers, and from the HTTP remote helper down to send-pack.  With this
change, we can detect if the server side rejects the push and report
back appropriately.  Note the difference in the messages: the remote
side reports "atomic transaction failed", while our own checking rejects
pushes with the message "atomic push failed".

Document the atomic option in the remote helper documentation, so other
implementers can implement it if they like.

Signed-off-by: brian m. carlson 
---
I discovered this at work when implementing support for atomic pushes.
Our equivalent of the pre-receive hook is not all-or-nothing, and
passing the atomic capability through to our backend worked for SSH, but
not HTTP.  I discovered with GIT_TRACE_PACKET=1 that we didn't pass the
atomic capability through during HTTP, so I thought I'd send a patch.

As I mentioned in the commit message, to my knowledge, this
functionality has been broken since the atomic capability was introduced
circa 2.4.0.

 Documentation/gitremote-helpers.txt |  5 
 remote-curl.c   | 13 +-
 t/t5541-http-push-smart.sh  | 40 +++--
 transport-helper.c  |  4 +++
 transport.h |  3 +++
 5 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index a5c3c04371..670d72c174 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -509,6 +509,11 @@ set by Git if the remote helper has the 'option' 
capability.
Indicate that only the objects wanted need to be fetched, not
their dependents.
 
+'option atomic' {'true'|'false'}::
+  When pushing, request the remote server to update refs in a single atomic
+  transaction.  If successful, all refs will be updated, or none will.  If the
+  remote side does not support this capability, the push will fail.
+
 SEE ALSO
 
 linkgit:git-remote[1]
diff --git a/remote-curl.c b/remote-curl.c
index 051f26629d..5232ed84b6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -40,7 +40,8 @@ struct options {
push_cert : 2,
deepen_relative : 1,
from_promisor : 1,
-   no_dependents : 1;
+   no_dependents : 1,
+   atomic : 1;
 };
 static struct options options;
 static struct string_list cas_options = STRING_LIST_INIT_DUP;
@@ -148,6 +149,14 @@ static int set_option(const char *name, const char *value)
else
return -1;
return 0;
+   } else if (!strcmp(name, "atomic")) {
+   if (!strcmp(value, "true"))
+   options.atomic = 1;
+   else if (!strcmp(value, "false"))
+   options.atomic = 0;
+   else
+   return -1;
+   return 0;
} else if (!strcmp(name, "push-option")) {
if (*value != '"')
string_list_append(&options.push_options, value);
@@ -1196,6 +1205,8 @@ static int push_git(struct discovery *heads, int nr_spec, 
char **specs)
argv_array_push(&args, "--signed=yes");
else if (options.push_cert == SEND_PACK_PUSH_CERT_IF_ASKED)
argv_array_push(&args, "--signed=if-asked");
+   if (options.atomic)
+   argv_array_push(&args, "--atomic");
if (options.verbosity == 0)
argv_array_push(&args, "--quiet");
else if (options.verbosity > 1)
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 92bac43257..4c970787b0 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -184,11 +184,12 @@ test_expect_success 'push --atomic also prevents branch 
creation, reports collat
test_config -C "$d" http.receivepack true &&
up="$HTTPD_URL"/smart/atomic-branches.git &&
 
-   # Tell "$up" about two branches for now
+   # Tell "$up" about three branches for n

Re: [PATCH v4] stash: avoid recursive hard reset on submodules

[Junio C Hamano]

Jakob Jarmar  writes:

> git stash push does not recursively stash submodules, but if
> submodule.recurse is set, it may recursively reset --hard them. Having
> only the destructive action recurse is likely to be surprising
> behaviour, and unlikely to be desirable, so the easiest fix should be to
> ensure that the call to git reset --hard never recurses into submodules.
>
> This matches the behavior of check_changes_tracked_files, which ignores
> submodules.
>
> Signed-off-by: Jakob Jarmar 
> ---
>
> Notes:
> 1. Added space between function name and parentheses
> 2. Moved test_when_finished cleanup to top of setup_basic

Looks good; will queue.

Thanks.

Re: ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)

[Eric Wong]

Johannes Schindelin  wrote:
> On Fri, 11 Oct 2019, Junio C Hamano wrote:
> > * ds/sparse-cone (2019-10-08) 17 commits
> >  - sparse-checkout: cone mode should not interact with .gitignore
> >  - sparse-checkout: write using lockfile
> >  - sparse-checkout: update working directory in-process
> >  - sparse-checkout: sanitize for nested folders
> >  - read-tree: show progress by default
> >  - unpack-trees: add progress to clear_ce_flags()
> >  - unpack-trees: hash less in cone mode
> >  - sparse-checkout: init and set in cone mode
> >  - sparse-checkout: use hashmaps for cone patterns
> >  - sparse-checkout: add 'cone' mode
> >  - trace2: add region in clear_ce_flags
> >  - sparse-checkout: create 'disable' subcommand
> >  - sparse-checkout: add '--stdin' option to set subcommand
> >  - sparse-checkout: 'set' subcommand
> >  - clone: add --sparse mode
> >  - sparse-checkout: create 'init' subcommand
> >  - sparse-checkout: create builtin with 'list' subcommand
> >
> >  Management of sparsely checked-out working tree has gained a
> >  dedicated "sparse-checkout" command.
> >
> >  Seems not to play well with the hashmap updates.
> 
> Hrm. I had sent out links to the three fixups needed to make the build
> green:
> 
> https://public-inbox.org/git/nycvar.qro.7.76.6.1910081055210...@tvgsbejvaqbjf.bet/
> 
> In particular, the patches to squash were:
> 
> https://github.com/git-for-windows/git/commit/f74259754971b427a14e6290681e18950824b99d
> https://github.com/git-for-windows/git/commit/124c8bc08e974e76ca7d956dc07eb288e71d639e
> https://github.com/git-for-windows/git/commit/45948433d1b48ff513fbd37f134c0f1491c78192

> diff --git a/dir.c b/dir.c
> index 0135f9e2180..9efcdc9aacd 100644
> --- a/dir.c
> +++ b/dir.c


> @@ -706,8 +710,8 @@ static void add_pattern_to_hashsets(struct pattern_list 
> *pl, struct path_pattern
> 
>  clear_hashmaps:
>   warning(_("disabling cone pattern matching"));
> - hashmap_free(&pl->parent_hashmap, 1);
> - hashmap_free(&pl->recursive_hashmap, 1);
> + hashmap_free(&pl->parent_hashmap);
> + hashmap_free(&pl->recursive_hashmap);

I just took a brief look, but that appears to leak memory.

"hashmap_free(var, 1)" should be replaced with
"hashmap_free_entries(var, struct foo, member)"

Only "hashmap_free(var, 0)" can become "hashmap_free(var)"

Re: [PATCH] http-push: simplify deleting a list item

[Junio C Hamano]

René Scharfe  writes:

> The first step for deleting an item from a linked list is to locate the
> item preceding it.  Be more careful in release_request() and handle an
> empty list.  This only has consequences for invalid delete requests
> (removing the same item twice, or deleting an item that was never added
> to the list), but simplifies the loop condition as well as the check
> after the loop.
>
> Once we found the item's predecessor in the list, update its next
> pointer to skip over the item, which removes it from the list.  In other
> words: Make the item's successor the successor of its predecessor.
> (At this point entry->next == request and prev->next == lock,
> respectively.)  This is a bit simpler and saves a pointer dereference.
>
> Signed-off-by: René Scharfe 
> ---
>  http-push.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Nice simplification.  I wonder how much longer we should be
maintaining this program, though;-)


Will queue.

>
> diff --git a/http-push.c b/http-push.c
> index 0353f9f514..822f326599 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -501,10 +501,10 @@ static void release_request(struct transfer_request 
> *request)
>   if (request == request_queue_head) {
>   request_queue_head = request->next;
>   } else {
> - while (entry->next != NULL && entry->next != request)
> + while (entry && entry->next != request)
>   entry = entry->next;
> - if (entry->next == request)
> - entry->next = entry->next->next;
> + if (entry)
> + entry->next = request->next;
>   }
>
>   free(request->url);
> @@ -981,7 +981,7 @@ static int unlock_remote(struct remote_lock *lock)
>   while (prev && prev->next != lock)
>   prev = prev->next;
>   if (prev)
> - prev->next = prev->next->next;
> + prev->next = lock->next;
>   }
>
>   free(lock->owner);
> --
> 2.23.0

Re: [PATCH 1/1] Improve Japanese translation

[Junio C Hamano]

"kdnakt via GitGitGadget"  writes:

> @@ -661,7 +662,7 @@ msgstr ""
>  #: lib/merge.tcl:108
>  #, tcl-format
>  msgid "%s of %s"
> -msgstr "%s の %s ブランチ"
> +msgstr "%2$s の %1$s ブランチ"
>  
>  #: lib/merge.tcl:122
>  #, tcl-format
> @@ -956,7 +957,7 @@ msgstr "エラー: コマンドが失敗しました"
>  #: lib/checkout_op.tcl:85
>  #, tcl-format
>  msgid "Fetching %s from %s"
> -msgstr "%s から %s をフェッチしています"
> +msgstr "%2$s から %1$s をフェッチしています"

Both of these changes to word order make sense.

It's a bit surprising that these haven't been noticed/fixed for
quite some time ;-).

Re: [PATCH v2 1/1] doc: Change zsh git completion file name

[Junio C Hamano]

SZEDER Gábor  writes:

>> -# The recommended way to install this script is to copy to '~/.zsh/_git', 
>> and
>> -# then add the following to your ~/.zshrc file:
>> +# The recommended way to install this script is to make a copy of it in
>> +# ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following
>
> These updated instructions don't work for me, following them gives me
> zsh's git completion instead of ours:
> ...
> Instructing users to copy our completion script to
> '~/.zsh/git-completion.zsh' goes against this convention.
> More importantly, it appears that this is more than a mere convention,
> maybe a requirement even; at least renaming our zsh completion script
> to follow the convention (IOW following the current install
> instructions) makes it work all of a sudden:

Thanks for a dose of sanity.  My "Helped-by" was about phrasing and
had nothing with the name of the file, so it should be fixable
without invalidating it ;-)

> -# The recommended way to install this script is to copy to '~/.zsh/_git', and
> -# then add the following to your ~/.zshrc file:
> +# The recommended way to install this script is to make a copy of it in
> +# ~/.zsh/ directory as ~/.zsh/git-completion.zsh and then add the following

# The recommended way to install this script is to make a copy of it in
# '~/.zsh/' directory as '~/.zsh/_git' and then add the following

Re: [PATCH v5 1/3] format-patch: change erroneous and condition

[Junio C Hamano]

Denton Liu  writes:

>  Since this
> seems to be a Python-ism that's mistakenly leaked into our code, convert

The conclusion is OK, but as the inventor of format-patch and a
non-pythonista, I do not think the above claim is correct, and even
if Thomas thought it was a good idea to follow Python style in
30984ed2 ("format-patch: support deep threading", 2009-02-19), which
I doubt he did, I do not think there is much point in speculating.

Both the log message and the patch text in the previous round were
better than this round, I would have to say.

Thanks.



> diff --git a/builtin/log.c b/builtin/log.c
> index 44b10b3415..351f4ffcfd 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -835,7 +835,7 @@ static int git_format_config(const char *var, const char 
> *value, void *cb)
>   thread = THREAD_SHALLOW;
>   return 0;
>   }
> - thread = git_config_bool(var, value) && THREAD_SHALLOW;
> + thread = git_config_bool(var, value) ? THREAD_SHALLOW : 
> THREAD_UNSET;
>   return 0;
>   }
>   if (!strcmp(var, "format.signoff")) {

Re: [PATCH v5 3/3] format-patch: teach --cover-from-description option

[Junio C Hamano]

Denton Liu  writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index d212a8305d..af33fe9ffb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1057,6 +1072,44 @@ static void show_diffstat(struct rev_info *rev,
>   fprintf(rev->diffopt.file, "\n");
>  }
>  
> +static void pp_from_desc(struct pretty_print_context *pp,
> +  const char *branch_name,
> +  struct strbuf *sb,
> +  const char *encoding,
> +  int need_8bit_cte)
> +{
> + const char *subject = "*** SUBJECT HERE ***";
> + const char *body = "*** BLURB HERE ***";
> + struct strbuf description_sb = STRBUF_INIT;
> + struct strbuf subject_sb = STRBUF_INIT;
> +
> + if (cover_from_description_mode == COVER_FROM_NONE)
> + goto do_pp;
> +
> + if (branch_name && *branch_name)
> + read_branch_desc(&description_sb, branch_name);
> + if (!description_sb.len)
> + goto do_pp;
> +
> + if (cover_from_description_mode == COVER_FROM_SUBJECT ||
> + cover_from_description_mode == COVER_FROM_AUTO)
> + body = format_subject(&subject_sb, description_sb.buf, " ");
> +
> + if (cover_from_description_mode == COVER_FROM_MESSAGE ||
> + (cover_from_description_mode == COVER_FROM_AUTO &&
> +  subject_sb.len > COVER_FROM_AUTO_MAX_SUBJECT_LEN))
> + body = description_sb.buf;
> + else
> + subject = subject_sb.buf;
> +
> +do_pp:
> + pp_title_line(pp, &subject, sb, encoding, need_8bit_cte);
> + pp_remainder(pp, &body, sb, 0);
> +
> + strbuf_release(&description_sb);
> + strbuf_release(&subject_sb);
> +}
> +

This implementation is very clear and easy to follow, and ...

> @@ -1064,8 +1117,6 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
> int quiet)
>  {
>   const char *committer;
>   struct shortlog log;
>   struct strbuf sb = STRBUF_INIT;
>   int i;
> @@ -1095,15 +1146,12 @@ static void make_cover_letter(struct rev_info *rev, 
> int use_stdout,
>   if (!branch_name)
>   branch_name = find_branch_name(rev);
>  
>   pp.fmt = CMIT_FMT_EMAIL;
>   pp.date_mode.type = DATE_RFC2822;
>   pp.rev = rev;
>   pp.print_email_subject = 1;
>   pp_user_info(&pp, NULL, &sb, committer, encoding);
> + pp_from_desc(&pp, branch_name, &sb, encoding, need_8bit_cte);
>   fprintf(rev->diffopt.file, "%s\n", sb.buf);
>  
>   strbuf_release(&sb);

... made the caller much simpler.

One large nit is that pp_user_info() is about "pretty printing the
user info", pp_title_line() is about "pretty printing the title
line", but pp_from_desc() is not about "pretty printing the from
desc".  Naming matters.

How about calling it with more emphasis on what it does (i.e. the
helper is about preparing the subject and body of the cover letter
e-mail) and less emphasis on how it does or what it bases its
decision on?  prepare_cover_text() or soemthing, perhaps?

Other than that, this version is very much more preferrable than the
previous one.  Quite nicely done.

Thanks.

Re: [RFC PATCH 1/1] Teach remote add the --prefix-tags option

[Junio C Hamano]

Wink Saville  writes:

> When --prefix-tags is passed to `git remote add` the tagopt is set to
> --prefix-tags and a second fetch line is added so tags are placed in
> a separate hierarchy per remote.


In the olden days, there was no refs/remotes/$remoteName/ hiearchy,
and until we made it the default at around Git 1.5.0, such a modern
layout for the branches were called the "separate remote" layout,
and can be opted into with "clone --use-separate-remote" by early
adopters.

I doubt that use of refs/tags/$remoteName/ is a good design if we
want to achieve similar isolation between local tags and and tags
obtained from each remote.

An obvious alternative, refs/remotes/$remoteName/tags/, is not a
good design for exactly the same reason.  You cannot tell between a
local tag foo/bar and a tag bar obtained from remote foo when you
see refs/tags/foo/bar, and you cannot tell between a branch tag/bar
obtained from remote foo and a tag bar obtained from remote foo when
you see refs/remotes/foo/tags/bar.  In the past, people suggested to
use refs/remoteTags/$remoteName/ for proper isolation, and it might
be a better middle-ground than either of the two, at least in the
shorter term, but not ideal.

In short, if you truly want to see "separate hierarchy per remote",
you should consider how you can reliably implement an equivalent of
"git branch --list --remote"; a design that does not allow it is a
failure.

A better solution with longer lifetime would probably be to use

refs/remotes/$remoteName/{heads,tags,...}/

when core.useTotallySeparateRemote configuration exists (and
eventually at Git 3.0 make the layout the default).  It would
involve changes in the refname look-up rules, but it would not have
to pollute refs/ namespace like the refs/remoteTags/ half-ground
design, which would require us to add refs/remoteNotes/ and friends,
who knows how many more we would end up having to support if we go
that route.

Thanks.

Re: ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)

[Junio C Hamano]

Eric Wong  writes:

> I just took a brief look, but that appears to leak memory.
>
> "hashmap_free(var, 1)" should be replaced with
> "hashmap_free_entries(var, struct foo, member)"
>
> Only "hashmap_free(var, 0)" can become "hashmap_free(var)"

I deliberately avoided merge-time band-aid fixups on this topic and
ew/hashmap exactly because I was sure that I'd introduce a similar
bugs by doing so myself.  Using evil merges can be a great way to
help multiple topics polished independently at the same time, but
when overused, can hide this kind of gotchas quite easily.

A reroll on top of ew/hashmap would be desirable, now that topic is
ready for 'master'.

Thanks.  

[PATCH 0/1] diff-highlight: fix a whitespace nit

[Norman Rasmussen via GitGitGadget]

This changes the indent from
  ""
to
  ""
so that the statement lines up with the rest of the block.

Signed-off-by: Norman Rasmussen 

Norman Rasmussen (1):
  diff-highlight: fix a whitespace nit

 contrib/diff-highlight/DiffHighlight.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 08da6496b61341ec45eac36afcc8f94242763468
Published-As: 
https://github.com/gitgitgadget/git/releases/tag/pr-397%2Fnormanr%2Fdiff-highlight-whitespace-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-397/normanr/diff-highlight-whitespace-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/397
-- 
gitgitgadget

[PATCH 1/1] diff-highlight: fix a whitespace nit

[Norman Rasmussen via GitGitGadget]

From: Norman Rasmussen 

This changes the indent from
  ""
to
  ""
so that the statement lines up with the rest of the block.

Signed-off-by: Norman Rasmussen 
---
 contrib/diff-highlight/DiffHighlight.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm 
b/contrib/diff-highlight/DiffHighlight.pm
index 7440aa1c46..e2589922a6 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -72,7 +72,7 @@ sub handle_line {
  (?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|"
 [ ]*  # trailing whitespace for merges
/x) {
-   my $graph_prefix = $&;
+   my $graph_prefix = $&;
 
# We must flush before setting graph indent, since the
# new commit may be indented differently from what we
-- 
gitgitgadget

Re: [PATCH v5 1/3] format-patch: change erroneous and condition

[Denton Liu]

Hi Junio,

Thanks for the feedback.

On Tue, Oct 15, 2019 at 11:16:35AM +0900, Junio C Hamano wrote:
> Denton Liu  writes:
> 
> >  Since this
> > seems to be a Python-ism that's mistakenly leaked into our code, convert
> 
> The conclusion is OK, but as the inventor of format-patch and a
> non-pythonista, I do not think the above claim is correct, and even
> if Thomas thought it was a good idea to follow Python style in
> 30984ed2 ("format-patch: support deep threading", 2009-02-19), which
> I doubt he did, I do not think there is much point in speculating.

I agree, I probably shouldn't be putting speculation in the log
messages. I'll change this for the next reroll.

> 
> Both the log message and the patch text in the previous round were
> better than this round, I would have to say.

I'll probably keep the patch text, however. In the previous version, we
were implicitly relying on the value of THREAD_SHALLOW to be 1. This
seems a little bit flimsy to me since it's possible that the enum can be
changed in the future and it may invalidate that assumption.

I'll keep it explicit so that it's a little bit more robust and also, so
that it's more obvious to future readers what's going on.

Thanks,

Denton

> 
> Thanks.
> 
> 
> 
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 44b10b3415..351f4ffcfd 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -835,7 +835,7 @@ static int git_format_config(const char *var, const 
> > char *value, void *cb)
> > thread = THREAD_SHALLOW;
> > return 0;
> > }
> > -   thread = git_config_bool(var, value) && THREAD_SHALLOW;
> > +   thread = git_config_bool(var, value) ? THREAD_SHALLOW : 
> > THREAD_UNSET;
> > return 0;
> > }
> > if (!strcmp(var, "format.signoff")) {

Re: [PATCH 1/1] diff-highlight: fix a whitespace nit

[Jeff King]

On Tue, Oct 15, 2019 at 03:31:26AM +, Norman Rasmussen via GitGitGadget 
wrote:

> From: Norman Rasmussen 
> 
> This changes the indent from
>   ""
> to
>   ""
> so that the statement lines up with the rest of the block.

Yep, that makes sense. Looks like I introduced the problem (most of my
perl used to be written in a style that forbids tabs, so it may have
snuck in that way, but the rest of the file definitely follows Git's
usual style of tabs).

> diff --git a/contrib/diff-highlight/DiffHighlight.pm 
> b/contrib/diff-highlight/DiffHighlight.pm
> index 7440aa1c46..e2589922a6 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -72,7 +72,7 @@ sub handle_line {
> (?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|"
>[ ]*  # trailing whitespace for merges
>   /x) {
> - my $graph_prefix = $&;
> + my $graph_prefix = $&;

There are a few lines just above that have 8+ spaces. Arguably those
could be tabs, too, depending on your view of tabs. We usually do "8
spaces is a tab" in the Git project, but the oft-repeated "tabs to
indent, spaces to align" mantra would apply here (and I suspect you're
using a different tabwidth since you noticed this one case). So I'd just
as soon leave them be, and take your patch as-is.

-Peff

Re: [PATCH 1/1] diff-highlight: fix a whitespace nit

[Norman Rasmussen]

On Mon, Oct 14, 2019 at 9:20 PM Jeff King  wrote:
> There are a few lines just above that have 8+ spaces. Arguably those
> could be tabs, too, depending on your view of tabs. We usually do "8
> spaces is a tab" in the Git project, but the oft-repeated "tabs to
> indent, spaces to align" mantra would apply here (and I suspect you're
> using a different tabwidth since you noticed this one case). So I'd just
> as soon leave them be, and take your patch as-is.

Yep, the lines above are using the spaces to align the sections of the
multi-line if statement. This happens again for the return statements
in highlight_pair and is_pair_interesting. So this is the only line
that doesn't stick to the rule (and probably because of editor
auto-indenting). I have another change for the same change (which I'll
send once I've written tests) and I only noticed this line when I
changed my editor's default tabwidth after a while of coding.

-- 
- Norman Rasmussen
 - Email: nor...@rasmussen.co.za
 - Home page: http://norman.rasmussen.co.za/

What's cooking in git.git (Oct 2019, #04; Tue, 15)

[Junio C Hamano]

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.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/pcre-jit-fixes (2019-08-19) 18 commits
  (merged to 'next' on 2019-10-04 at 2d55f2b470)
 + grep: under --debug, show whether PCRE JIT is enabled
 + grep: do not enter PCRE2_UTF mode on fixed matching
 + grep: stess test PCRE v2 on invalid UTF-8 data
 + grep: create a "is_fixed" member in "grep_pat"
 + grep: consistently use "p->fixed" in compile_regexp()
 + grep: stop using a custom JIT stack with PCRE v1
 + grep: stop "using" a custom JIT stack with PCRE v2
 + grep: remove overly paranoid BUG(...) code
 + grep: use PCRE v2 for optimized fixed-string search
 + grep: remove the kwset optimization
 + grep: drop support for \0 in --fixed-strings 
 + grep: make the behavior for NUL-byte in patterns sane
 + grep tests: move binary pattern tests into their own file
 + grep tests: move "grep binary" alongside the rest
 + grep: inline the return value of a function call used only once
 + t4210: skip more command-line encoding tests on MinGW
 + grep: don't use PCRE2?_UTF8 with "log --encoding="
 + log tests: test regex backends in "--encode=" tests
 (this branch is used by cb/pcre1-cleanup.)

 A few simplification and bugfixes to PCRE interface.


* ah/cleanups (2019-10-03) 4 commits
  (merged to 'next' on 2019-10-04 at 1345f09afb)
 + git_mkstemps_mode(): replace magic numbers with computed value
 + wrapper: use a loop instead of repetitive statements
 + diffcore-break: use a goto instead of a redundant if statement
 + commit-graph: remove a duplicate assignment

 Miscellaneous code clean-ups.


* am/t0028-utf16-tests (2019-09-28) 2 commits
  (merged to 'next' on 2019-10-09 at 453900a4e8)
 + t0028: add more tests
 + t0028: fix test for UTF-16-LE-BOM

 Test fixes.


* am/visual-studio-config-fix (2019-09-28) 1 commit
  (merged to 'next' on 2019-10-04 at 135d93143b)
 + contrib/buildsystems: fix Visual Studio Debug configuration

 Dev support.


* as/shallow-slab-use-fix (2019-10-02) 1 commit
  (merged to 'next' on 2019-10-04 at f3a22d2b18)
 + shallow.c: don't free unallocated slabs

 Correct code that tried to reference all entries in a sparse array
 of pointers by mistake.


* bc/object-id-part17 (2019-08-19) 26 commits
  (merged to 'next' on 2019-10-04 at b0460b0db2)
 + midx: switch to using the_hash_algo
 + builtin/show-index: replace sha1_to_hex
 + rerere: replace sha1_to_hex
 + builtin/receive-pack: replace sha1_to_hex
 + builtin/index-pack: replace sha1_to_hex
 + packfile: replace sha1_to_hex
 + wt-status: convert struct wt_status to object_id
 + cache: remove null_sha1
 + builtin/worktree: switch null_sha1 to null_oid
 + builtin/repack: write object IDs of the proper length
 + pack-write: use hash_to_hex when writing checksums
 + sequencer: convert to use the_hash_algo
 + bisect: switch to using the_hash_algo
 + sha1-lookup: switch hard-coded constants to the_hash_algo
 + config: use the_hash_algo in abbrev comparison
 + combine-diff: replace GIT_SHA1_HEXSZ with the_hash_algo
 + bundle: switch to use the_hash_algo
 + connected: switch GIT_SHA1_HEXSZ to the_hash_algo
 + show-index: switch hard-coded constants to the_hash_algo
 + blame: remove needless comparison with GIT_SHA1_HEXSZ
 + builtin/rev-parse: switch to use the_hash_algo
 + builtin/blame: switch uses of GIT_SHA1_HEXSZ to the_hash_algo
 + builtin/receive-pack: switch to use the_hash_algo
 + fetch-pack: use parse_oid_hex
 + patch-id: convert to use the_hash_algo
 + builtin/replace: make hash size independent

 Preparation for SHA-256 upgrade continues.


* cb/pcre1-cleanup (2019-08-26) 2 commits
  (merged to 'next' on 2019-10-04 at a2dd896ee8)
 + grep: refactor and simplify PCRE1 support
 + grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1
 (this branch uses ab/pcre-jit-fixes.)

 PCRE fixes.


* dl/format-patch-doc-test-cleanup (2019-10-09) 1 commit
  (merged to 'next' on 2019-10-11 at 992da06f37)
 + t4014: treat rev-list output as the expected value
 (this branch is used by dl/format-patch-cover-from-desc.)

 test cleanup.


* dl/octopus-graph-bug (2019-10-04) 5 commits
  (merged to 'next' on 2019-10-07 at c6bc2fe4a0)
 + t4214: demonstrate octopus graph coloring failure
 + t4214: explicitly list tags in log
 + t4214: generate expect in their own test cases
 + t4214: use test_merge
 + test-lib: let test_merge() perform octopus merges

 "git log --graph" for an octopus merge is sometimes colored
 incorrectly, which is demonstrated and documented but not yet
 fixed.


* dl/rev-list-doc-cleanup (2019-10-06) 1 commit
  (merged to 'next' on 2019-10-07 at