Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>

[...]

>> >> Reusing existing concepts where possible doesn`t have this problem.
>> >
>> > Existing concepts are great. As long as they fit the requirements of
>> > the new scenarios. In this case, `pick` does *not* fit the requirement
>> > of "rebase a merge commit".
>> 
>> It does, provided you use suitable syntax.
>
> You know what `pick` would also do, provided you use suitable syntax? Pick
> your nose.
>
> Don't blame me for this ridiculous turn the discussion took.
>
> Of course, using the suitable syntax you can do anything. Unless there is
> *already* a syntax and you cannot break it for backwards-compatibility
> reasons, as is the case here.

Backward compatibility to what? To a broken '--preserve-merges'? I had a
feel you've invented '--recreate-merges' exactly to break that
compatibility. No?

Or is it "Backwards compatibility of a feature that existed only as a
topic branch in `next` before being worked on more?", as you say
yourself below?

[...]

>> > The implementation detail is, of course, that I will introduce this with
>> > the technically-simpler strategy: always recreating merge commits with the
>> > recursive strategy. A follow-up patch series will add support for rebasing
>> > merge commits, and then use it by default.
>> 
>> Switching to use it by default would be backward incompatible again? Yet
>> another option to obsolete? Sigh. 
>
> Oh wow.
>
> Backwards compatibility of a feature that existed only as a topic branch
> in `next` before being worked on more? Any other splendid ideas?

Either you care about compatibility or not. You can't have it both ways,
sorry.

And "technically-simpler strategy: always recreating merge commits with
the recursive strategy" vs. "rebasing merge commits" is not just a minor
strategy change, it's entire paradigm shift in handling merge commits
while rebasing. I'm afraid you will still come up with a wrong design
unless you finally accept this fact.

-- Sergey

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:

> Hi Sergey,

[...]

> But I'll stop here. Even my account how there are conceptual differences
> between the changes in merge vs non-merge commits (the non-merge commit
> *introduces* changes, the merge commit *reconciles existing* changes)
> seems to fly by without convincing you.

Good for you, but Git should keep caring about content, it should care
not about meaning. Please leave it to the user to assign meaning to
their content.

If you rather want a SCM that focuses on meaning, I'd suggest to look at
Bzr and see how it goes.

> I use rebase every day. I use the Git garden shears every week. If you do
> not trust my experience with these things, nothing will convince you. 

Unfortunately you have exactly zero experience with rebasing merges as
you've never actually rebased them till now, and it's rebasing merges
that matters in this particular discussion.

> You are just stuck with your pre-existing opinion.

I'm afraid that it's rather your huge experience with re-creating merges
that makes you stuck to your pre-existing opinion and carefully shields
you from experiencing actual paradigm shift.

-- Sergey

[PATCH] l10n: de.po: translate 132 new messages

[Ralf Thielow]

Translate 132 new message came from git.pot update in abc8de64d (l10n:
git.pot: v2.17.0 round 1 (132 new, 44 removed)).

Signed-off-by: Ralf Thielow 
---
 po/de.po | 455 +--
 1 file changed, 209 insertions(+), 246 deletions(-)

diff --git a/po/de.po b/po/de.po
index 852c02a9b..793bd2a80 100644
--- a/po/de.po
+++ b/po/de.po
@@ -1654,15 +1654,14 @@ msgstr "externes Diff-Programm unerwartet beendet, 
angehalten bei %s"
 #: diff.c:4146
 msgid "--name-only, --name-status, --check and -s are mutually exclusive"
 msgstr ""
 "--name-only, --name-status, --check und -s schließen sich gegenseitig aus"
 
 #: diff.c:4149
-#, fuzzy
 msgid "-G, -S and --find-object are mutually exclusive"
-msgstr "-b, -B und --detach schließen sich gegenseitig aus"
+msgstr "-G, -S und --find-object schließen sich gegenseitig aus"
 
 #: diff.c:4237
 msgid "--follow requires exactly one pathspec"
 msgstr "--follow erfordert genau eine Pfadspezifikation"
 
 #: diff.c:4403
@@ -1695,15 +1694,15 @@ msgid ""
 "you may want to set your %s variable to at least %d and retry the command."
 msgstr ""
 "Sie könnten die Variable %s auf mindestens %d setzen und den Befehl\n"
 "erneut versuchen."
 
 #: dir.c:1866
-#, fuzzy, c-format
+#, c-format
 msgid "could not open directory '%s'"
-msgstr "Konnte Verzeichnis '%s' nicht erstellen."
+msgstr "Konnte Verzeichnis '%s' nicht öffnen."
 
 #: dir.c:2108
 msgid "failed to get kernel name and information"
 msgstr "Fehler beim Sammeln von Namen und Informationen zum Kernel"
 
 #: dir.c:2232
@@ -1731,27 +1730,25 @@ msgstr "Hinweis: Warte auf das Schließen der Datei 
durch Ihren Editor...%c"
 msgid "Filtering content"
 msgstr "Filtere Inhalt"
 
 #: entry.c:435
 #, c-format
 msgid "could not stat file '%s'"
-msgstr "konnte Datei '%s' nicht lesen"
+msgstr "Konnte Datei '%s' nicht lesen."
 
 #: fetch-object.c:17
-#, fuzzy
 msgid "Remote with no URL"
-msgstr "git archive: Externes Archiv ohne URL"
+msgstr "Remote-Repository ohne URL"
 
 #: fetch-pack.c:253
 msgid "git fetch-pack: expected shallow list"
 msgstr "git fetch-pack: erwartete shallow-Liste"
 
 #: fetch-pack.c:265
-#, fuzzy
 msgid "git fetch-pack: expected ACK/NAK, got a flush packet"
-msgstr "git fetch-pack: ACK/NAK erwartet, '%s' bekommen"
+msgstr "git fetch-pack: ACK/NAK erwartet, Flush-Paket bekommen"
 
 #: fetch-pack.c:284 builtin/archive.c:63
 #, c-format
 msgid "remote error: %s"
 msgstr "Fehler am anderen Ende: %s"
 
@@ -1883,15 +1880,14 @@ msgstr "Server unterstützt allow-reachable-sha1-in-want"
 
 #: fetch-pack.c:979
 msgid "Server supports ofs-delta"
 msgstr "Server unterstützt ofs-delta"
 
 #: fetch-pack.c:985
-#, fuzzy
 msgid "Server supports filter"
-msgstr "Server unterstützt ofs-delta"
+msgstr "Server unterstützt Filter"
 
 #: fetch-pack.c:993
 #, c-format
 msgid "Server version is %.*s"
 msgstr "Server-Version ist %.*s"
 
@@ -2104,19 +2100,18 @@ msgstr "Name besteht nur aus nicht erlaubten Zeichen: 
%s"
 #: ident.c:416 builtin/commit.c:582
 #, c-format
 msgid "invalid date format: %s"
 msgstr "Ungültiges Datumsformat: %s"
 
 #: list-objects-filter-options.c:36
-#, fuzzy
 msgid "multiple filter-specs cannot be combined"
-msgstr "Mehrere Arten von Objekt-Filtern können nicht kombiniert werden."
+msgstr "Mehrere filter-specs können nicht kombiniert werden."
 
 #: list-objects-filter-options.c:126
 msgid "cannot change partial clone promisor remote"
-msgstr ""
+msgstr "Kann Remote-Repository für partielles Klonen nicht ändern."
 
 #: lockfile.c:151
 #, c-format
 msgid ""
 "Unable to create '%s.lock': %s.\n"
 "\n"
@@ -2964,20 +2959,20 @@ msgstr "  (benutzen Sie \"git branch --unset-upstream\" 
zum Beheben)\n"
 #: remote.c:2139
 #, c-format
 msgid "Your branch is up to date with '%s'.\n"
 msgstr "Ihr Branch ist auf demselben Stand wie '%s'.\n"
 
 #: remote.c:2143
-#, fuzzy, c-format
+#, c-format
 msgid "Your branch and '%s' refer to different commits.\n"
-msgstr "Ihr Branch ist %2$d Commit vor '%1$s'.\n"
+msgstr "Ihr Branch und '%s' zeigen auf unterschiedliche Commits.\n"
 
 #: remote.c:2146
 #, c-format
 msgid "  (use \"%s\" for details)\n"
-msgstr ""
+msgstr "  (benutzen Sie \"%s\" für Details)\n"
 
 #: remote.c:2150
 #, c-format
 msgid "Your branch is ahead of '%s' by %d commit.\n"
 msgid_plural "Your branch is ahead of '%s' by %d commits.\n"
 msgstr[0] "Ihr Branch ist %2$d Commit vor '%1$s'.\n"
@@ -3048,15 +3043,14 @@ msgid ""
 msgstr ""
 "Der '%s' Hook wurde ignoriert, weil er nicht als ausführbar markiert ist.\n"
 "Sie können diese Warnung mit `git config advice.ignoredHook false` "
 "deaktivieren."
 
 #: send-pack.c:141
-#, fuzzy
 msgid "unexpected flush packet while reading remote unpack status"
-msgstr "Konnte Status des Entpackens der Gegenseite nicht parsen: %s"
+msgstr "Unerwartetes Flush-Paket beim Lesen des Remote-Unpack-Status."
 
 #: send-pack.c:143
 #, c-format
 msgid "unable to parse remote unpack status: %s"
 msgstr "Konnte Status des Entpackens der 

Re: [PATCH v2] test_must_be_empty: simplify file existence check

[Junio C Hamano]

Junio C Hamano  writes:

>>  test_must_be_empty () {
>> -if ! test -f "$1"
>> -then
>> -echo "'$1' is missing"
>> -return 1
>> -elif test -s "$1"
>> +test_path_is_file "$1" &&
>> +if test -s "$1"
>>  then
>>  echo "'$1' is not empty, it contains:"
>>  cat "$1"
>
> "Just call it" is fine as an idea but
>
>   A &&
>   if B
>   then
>   ...
>   fi
>
> is somewhat questionable.  Shouldn't we make it
>
>   if A && B
>   then
>   ...
>   fi
>
> instead?

Nah, you want to treat A's success as a condition *not* to enter the
"then" clause in this case, so my rewrite is bogus.  SOrry for the
noise.

Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully

[Junio C Hamano]

Stefan Beller  writes:

> This fixes a regression introduced in 22e612731b5 (submodule: port

s/22e/2e/, I think.

> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
> pathspecs that do not exist gracefully. This restores the historic behavior
> of reporting the pathspec as unknown and returning instead of reporting a
> bug.
>
> Reported-by: Peter Oberndorfer 
> Signed-off-by: Stefan Beller 
> ---
>  builtin/submodule--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

It seems that all the other callersof module-list expect that a
negative return from the function is a normal "nothing to do"
condition and returns 1, and this patch makes the oddball "deinit"
do the same.

Sounds good.  Will queue.

Thanks.


>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..6ba8587b6d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, 
> const char *prefix)
>   die(_("Use '--all' if you really want to deinitialize all 
> submodules"));
>  
>   if (module_list_compute(argc, argv, prefix, , ) < 0)
> - BUG("module_list_compute should not choke on empty pathspec");
> + return 1;
>  
>   info.prefix = prefix;
>   if (quiet)

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>
> On Tue, 27 Mar 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > On Mon, 12 Mar 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin  writes:
>> >> >
>> >> > On Wed, 7 Mar 2018, Sergey Organov wrote:
>> >> >
>> >> >> Johannes Schindelin  writes:
>> >> >> 
>> >> >> > How can your approach -- which relies *very much* on having the
>> >> >> > original parent commits -- not *require* that consistency check?
>> >> >> 
>> >> >> I don't understand what you mean, sorry. Could you please point me
>> >> >> to the *require* you talk about in the original proposal?
>> >> >
>> >> > Imagine a todo list that contains this line
>> >> >
>> >> > merge -C abcdef 123456
>> >> >
>> >> > and now the user edits it (this is an interactive rebase, after
>> >> > all), adding another merge head:
>> >> >
>> >> > merge -C abcdef 987654 123456
>> >> >
>> >> > Now your strategy would have a serious problem: to find the
>> >> > original version of 987654. If there was one.
>> >> 
>> >> We are talking about different checks then. My method has a built-in
>> >> check that Pillip's one doesn't.
>> >
>> > Since you did not bother to elaborate, I have to assume that your
>> > "built-in check" is that thing where intermediate merges can give you
>> > conflicts?
>> >
>> > If so, there is a possibility in Phillip's method for such conflicts,
>> > too: we have to perform as many 3-way merges as there are parent
>> > commits.
>> >
>> > It does make me uncomfortable to have to speculate what you meant,
>> > though.
>> 
>> It doesn't matter anymore as this check could easily be added to
>> Phillip's algorithm as well, see [1].
>> 
>> [1] https://public-inbox.org/git/87efkn6s1h@javad.com
>
> Ah, and there I was, thinking that finally you would answer my questions
> directly, instead you keep directing me elsewhere ("read that! Somewhere
> in there you will find the answer you are looking for").

Except I've copy-pasted it for /you/ from that reference in another
answer to /you/, and /you/ denied it there as being unexplained. As it
actually happens to be discussed and explained in the referenced
material, should I rather copy-paste the entire reference to fulfill
your requirements?

Here I repeat, directly again, that essential quote from that reference,
in case you forgot it:


git-rebase-first-parent --onto A' M
tree_U1'=$(git write-tree)
git merge-recursive B -- $tree_U1' B'
tree=$(git write-tree)
M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB')
[ $conflicted_last_merge = "yes" ] ||
  trees-match $tree_U1' $tree || 
  stop-for-user-amendment
  
where 'git-rebase-first-parent' denotes whatever machinery is currently
being used to rebase simple non-merge commit.


> My time is a bit too valuable, and I will not continue a discussion where
> my questions are constantly deflected that way.

No deflection on my side was ever intended. The referenced discussion
actually has explanations. Maybe one whole page of reading, and it is to
be read in context, and then a few follow-ups in that discussion could
also be of interest, provided you are interested. I'm sorry should you
have no time for that.

-- Sergey

Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully

[Martin Ågren]

On 28 March 2018 at 01:28, Stefan Beller  wrote:
> This fixes a regression introduced in 22e612731b5 (submodule: port

s/22/2/

> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
> pathspecs that do not exist gracefully. This restores the historic behavior
> of reporting the pathspec as unknown and returning instead of reporting a
> bug.

Re: [PATCH v2 3/6] stash: convert apply to builtin

[Joel Teichroeb]

On Mon, Mar 26, 2018 at 12:05 AM, Christian Couder
 wrote:
> On Mon, Mar 26, 2018 at 3:14 AM, Joel Teichroeb  wrote:
>> Signed-off-by: Joel Teichroeb 
>
> The commit message in this patch and the following ones could be a bit
> more verbose. It could at least tell that the end goal is to convert
> git-stash.sh to a C builtin.
>
>> +static void destroy_stash_info(struct stash_info *info)
>> +{
>> +   strbuf_release(>revision);
>> +}
>
> Not sure if "destroy" is the right word in the function name. I would
> have used "free" instead.
>
>> +static int get_stash_info(struct stash_info *info, int argc, const char 
>> **argv)
>> +{
>> +   struct strbuf w_commit_rev = STRBUF_INIT;
>> +   struct strbuf b_commit_rev = STRBUF_INIT;
>> +   struct strbuf w_tree_rev = STRBUF_INIT;
>> +   struct strbuf b_tree_rev = STRBUF_INIT;
>> +   struct strbuf i_tree_rev = STRBUF_INIT;
>> +   struct strbuf u_tree_rev = STRBUF_INIT;
>> +   struct strbuf symbolic = STRBUF_INIT;
>> +   struct strbuf out = STRBUF_INIT;
>> +   int ret;
>> +   const char *revision;
>> +   const char *commit = NULL;
>> +   char *end_of_rev;
>> +   info->is_stash_ref = 0;
>> +
>> +   if (argc > 1) {
>> +   int i;
>> +   fprintf(stderr, _("Too many revisions specified:"));
>> +   for (i = 0; i < argc; ++i) {
>> +   fprintf(stderr, " '%s'", argv[i]);
>> +   }
>
> The brackets are not needed.
>
>> +   fprintf(stderr, "\n");
>> +
>> +   return -1;
>> +   }
>> +
>> +   if (argc == 1)
>> +   commit = argv[0];
>> +
>> +   strbuf_init(>revision, 0);
>> +   if (commit == NULL) {
>> +   if (have_stash()) {
>> +   destroy_stash_info(info);
>> +   return error(_("No stash entries found."));
>> +   }
>> +
>> +   strbuf_addf(>revision, "%s@{0}", ref_stash);
>> +   } else if (strspn(commit, "0123456789") == strlen(commit)) {
>> +   strbuf_addf(>revision, "%s@{%s}", ref_stash, commit);
>> +   } else {
>> +   strbuf_addstr(>revision, commit);
>> +   }
>> +
>> +   revision = info->revision.buf;
>> +
>> +   strbuf_addf(_commit_rev, "%s", revision);
>
> Maybe use strbuf_addstr()?
>
>> +
>> +
>
> Spurious new line.
>
> [...]
>
>> +static int diff_cached_index(struct strbuf *out, struct object_id *c_tree)
>> +{
>> +   struct child_process cp = CHILD_PROCESS_INIT;
>> +const char *c_tree_hex = oid_to_hex(c_tree);
>
> Indent looks weird.
>
>> +
>> +   cp.git_cmd = 1;
>> +   argv_array_pushl(, "diff-index", "--cached", "--name-only", 
>> "--diff-filter=A", NULL);
>> +   argv_array_push(, c_tree_hex);
>> +   return pipe_command(, NULL, 0, out, 0, NULL, 0);
>> +}
>> +
>> +static int update_index(struct strbuf *out) {
>
> The opening bracket should be on its own line.
>
>> +   struct child_process cp = CHILD_PROCESS_INIT;
>
> Maybe add a new line here to be more consistent with other such functions.
>
>> +   cp.git_cmd = 1;
>> +   argv_array_pushl(, "update-index", "--add", "--stdin", NULL);
>> +   return pipe_command(, out->buf, out->len, NULL, 0, NULL, 0);
>> +}
>
> [...]
>
>> +   if (info->has_u) {
>> +   struct child_process cp = CHILD_PROCESS_INIT;
>> +   struct child_process cp2 = CHILD_PROCESS_INIT;
>> +   int res;
>> +
>> +   cp.git_cmd = 1;
>> +   argv_array_push(, "read-tree");
>> +   argv_array_push(, oid_to_hex(>u_tree));
>> +   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>> +
>> +   cp2.git_cmd = 1;
>> +   argv_array_pushl(, "checkout-index", "--all", NULL);
>> +   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>
> Maybe use small functions for the above read-tree and checkout-index.
>
>> +   res = run_command() || run_command();
>> +   remove_path(stash_index_path);
>> +   if (res)
>> +   return error(_("Could not restore untracked files 
>> from stash"));
>> +   }
>
> Thanks.

Thanks for your detailed comments! I've fixed them all in my next patch set.

Re: [PATCH 1/4] stash: convert apply to builtin

[Joel Teichroeb]

On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer  wrote:
> On 03/24, Joel Teichroeb wrote:
>> ---
>
> Missing sign-off?  I saw it's missing in the other patches as well.
>

Thanks! I always forget to add a sign-off.

>> [...]
>> +
>> + if (info->has_u) {
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + struct child_process cp2 = CHILD_PROCESS_INIT;
>> + int res;
>> +
>> + cp.git_cmd = 1;
>> + argv_array_push(, "read-tree");
>> + argv_array_push(, sha1_to_hex(info->u_tree.hash));
>> + argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>> +
>> + cp2.git_cmd = 1;
>> + argv_array_pushl(, "checkout-index", "--all", NULL);
>> + argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>> +
>> + res = run_command() || run_command();
>> + remove_path(stash_index_path);
>> + if (res)
>> + return error(_("Could not restore untracked files from 
>> stash"));
>
> A minor change in behaviour here is that we are removing the temporary
> index file unconditionally here, while we would previously only remove
> it if both 'read-tree' and 'checkout-index' would succeed.
>
> I don't think that's a bad thing, we probably don't want users to try
> and use that index file in any way, and I doubt that's part of anyones
> workflow, so I think cleaning it up makes sense.
>

I'm not sure about that. The shell script has a trap near the start in
order to clean up the temp index, unless I'm understanding the shell
script incorrectly.

Re: [PATCH 2/2] send-email: supply a --send-delay=1 by default

[Eric Wong]

Ævar Arnfjörð Bjarmason  wrote:
> Good point. I also see that (via git log --author=Ævar --grep='^\[PATCH
> ') that this series itself arrived out of order (0 -> 2 -> 1), but I
> don't know to what extent public-inbox itself might be batching things.

public-inbox doesn't batch, aside from when the
public-inbox-watch process gets restarted and needs to catch up
using readdir.  Once it's done catching up with readdir, it
gets into an inotify loop which injects messages in the order
the MTA (or offlineimap) puts them in a Maildir.

Right now, public-inbox only sorts by Date: header in the mail.

The next Xapian schema revision of public-inbox will use
internally sorts search results(*) by the date in the newest
Received: header.  That is analogous to git committer date.  The
displayed message date will still be sorted by the Date: header
(analogous to git author date); since git-send-email already
alters the Date: in a series for sorting.

This allow messages/threads which are actually new get bumped to
the top of the homepage; regardless of how wrong the original
sender's clock was.

It should help prevent kernel developers from crafting message
dates with optimization for classic^Wextra reviews in mind :)


(*) all the look-at-a-bunch-of-messages operations, including
the landing page (e.g. https://public-inbox.org/git/)
is a Xapian search query, nowadays; but "git log"

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Junio C Hamano]

Ramsay Jones  writes:

> BTW, I forgot to mention that you had some whitespace problems
> with this patch, viz:
>
>   $ git log --oneline -1 --check master-json
>   ab643d838 (master-json) json_writer: new routines to create data in JSON 
> format
>   t/helper/test-json-writer.c:280: trailing whitespace.
>   + */ 
>   t/t0019-json-writer.sh:179: indent with spaces.
>   +"g": 0,
>   t/t0019-json-writer.sh:180: indent with spaces.
>   +"h": 1
>   $ 

Yes, and the here-doc that shows expected output looked somewhat
old-fashioned without using <<- indent.

Writing it in a way like this might be a reasonable workaround for
"indent with spaces", which is only about the leading blank used to
indent the line:

<--HT-->sed -e "s/^\|//" <<-\EOF
<--HT-->|  ...
<--HT-->|"g": 0,
<--HT-->|"h": 0,
<--HT-->|  ...
<--HT-->EOF

That is, (1) Use the same number of HT as the line that begins the
here-doc to indent the expected output; (2) but explicitly mark the
left-most column with '|' or something; and then (3) write 8 or more
spaces liberally as needed to reproduce the expected output.

I called it a "workaround", as another possibility is to loosen the
whitespace rule in t/.gitattributes; we _could_ declare that indent
with spaces is OK in these scripts as they contain many expected
output examples in here-doc form.

The trailing whitespace after closing C-comment is simply a style
violation that is not excuable (I think I've dealt with it when I
queued the patch); it should just be fixed.

Thanks.

Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule

[Stefan Beller]

On Tue, Mar 27, 2018 at 5:07 PM, Jonathan Tan  wrote:

> s/submoduled/submodules

> s/superprojects/superproject's/

> s/and //
>
> s/force/forcing/

All wording fixed.

>> + sub_path = sub_worktree + strlen(super_worktree) + 1;
>> +
>> + if (repo_submodule_init(, superproject, sub_path))
>> + return;
>> +
>> + repo_read_index();
>
> From the name of this function and its usage in
> connect_work_tree_and_git_dir(), I expected this function to just
> iterate through all the files in its workdir (which it is doing in the
> "for" loop below) and connect any nested submodules. Why does it need
> access to its superproject? (I would think that repo_init() would be
> sufficient here instead of repo_submodule_init().)

Testing validates your thinking (for now).

If we ever want to have good error reporting (see bmwills hint to
check for index corruption), we may want to have all the repos constructed
as submodules from the_repository, as then the error messages might
be better (e.g. in the future we could display the
"submodule nesting stack").

I'll remove the superproject argument for now.

>> +
>> + strbuf_reset(_wt);
>> + strbuf_addf(_wt, "%s/%s/.git", sub_worktree, sub->path);
>> +
>> + strbuf_reset(_gd);
>> + strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
>> +
>> + strbuf_setlen(_wt, sub_wt.len - strlen("/.git"));
>> +
>> + if (is_submodule_active(, ce->name)) {
>> + connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 
>> 0);
>> + connect_wt_gitdir_in_nested(sub_wt.buf, sub_gd.buf, 
>> );
>
> The modifications of sub_wt and sub_gd should probably go here, since
> they are not used unless this "if" block is executed.

Thanks! I also cut out the setlen call by giving the correct format string.
(The code presented here is not very old, I just fired it off as soon as the
test passed)

>
>> +void connect_work_tree_and_git_dir(const char *work_tree_,
>> +const char *git_dir_,
>> +int recurse_into_nested)
>
> How is this function expected to be used? From what I see:
>  - if recurse_into_nested is 0, this works regardless of whether the
>work_tree_ and git_dir_ is directly or indirectly a submodule of
>the_repository
>  - if recurse_into_nested is 1, work_tree_ and git_dir_ must be directly
>a submodule of the_repository (since it is referenced directly)
>
> This seems confusing - is this expected?

In the next revision of the series connect_wt_gitdir_in_nested
will no longer have a third argument for the repo, as we use repo_init.

That eases the handling a bit here.

Re: [PATCH] grep: remove "repo" arg from non-supporting funcs

[Stefan Beller]

On Tue, Mar 27, 2018 at 5:24 PM, Jonathan Tan  wrote:
> On Tue, 27 Mar 2018 16:20:25 -0700
> Stefan Beller  wrote:
>
>> On Tue, Mar 27, 2018 at 3:58 PM, Jonathan Tan  
>> wrote:
>> > As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
>> > repository'", 2017-08-02), many functions in builtin/grep.c were
>> > converted to also take "struct repository *" arguments. Among them were
>> > grep_object() and grep_objects().
>> >
>> > However, at least grep_objects() was converted incompletely - it calls
>> > gitmodules_config_oid(), which references the_repository.
>> >
>> > But it turns out that the conversion was extraneous anyway - there has
>> > been no user-visible effect - because grep_objects() is never invoked
>> > except with the_repository.
>> >
>> > Revert the changes to grep_objects() and grep_object() (which conversion
>> > is also extraneous) to show that both these functions do not support
>> > repositories other than the_repository.
>>
>> I'd rather convert gitmodules_config_oid instead of reverting the other
>> functions into a world without an arbitrary repository object.
>
> I don't really think of it as a reversion, since grep_objects() didn't
> fully support repos other than the_repository anyway.
>
> Besides that, that's fine, but then:
>
>  (1) You would have to explain both in the gitmodules_config_oid() and
>  in this patch why "gitmodules_config_oid(...)" ->
>  "gitmodules_config_oid(repo, ...)" and "submodule_free()" ->
>  "submodule_free(repo)" are safe, since they have different behavior
>  upon first glance. (They have the same behavior only because
>  grep_objects() is always called with the_repository.) I was trying
>  to explain this in as clear a way as possible, by showing (with
>  code) that grep_objects() only works with, and is only called with,
>  the_repository.
>  (2) The code path where repo != the_repository would still never be
>  exercised, and I prefer to not have such code paths.
>
> I don't feel too strongly about (1), since they just concern commit
> messages, of which there are many valid opinions on how to write them. I
> feel a bit more strongly about (2), but can concede my point if the
> project is OK with a code path not being exercised.

Ok. I admit not having looked at the code beforehand, and I was
just arguing based on intuition. Turns out I was wrong.

I thought that we'd have to have arbitrary repos for proper submodule
recursion, but this specific code is for grepping thru given tree objects,
which for now is assumed that the user can only give trees of
the_repository and we'd error out in case of a submodule tree.
Makes sense.

In that case, I agree with your patch. Thanks for writing it.
I'll take it into the series.

Thanks,
Stefan

Re: [PATCH] grep: remove "repo" arg from non-supporting funcs

[Jonathan Tan]

On Tue, 27 Mar 2018 16:20:25 -0700
Stefan Beller  wrote:

> On Tue, Mar 27, 2018 at 3:58 PM, Jonathan Tan  
> wrote:
> > As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
> > repository'", 2017-08-02), many functions in builtin/grep.c were
> > converted to also take "struct repository *" arguments. Among them were
> > grep_object() and grep_objects().
> >
> > However, at least grep_objects() was converted incompletely - it calls
> > gitmodules_config_oid(), which references the_repository.
> >
> > But it turns out that the conversion was extraneous anyway - there has
> > been no user-visible effect - because grep_objects() is never invoked
> > except with the_repository.
> >
> > Revert the changes to grep_objects() and grep_object() (which conversion
> > is also extraneous) to show that both these functions do not support
> > repositories other than the_repository.
> 
> I'd rather convert gitmodules_config_oid instead of reverting the other
> functions into a world without an arbitrary repository object.

I don't really think of it as a reversion, since grep_objects() didn't
fully support repos other than the_repository anyway.

Besides that, that's fine, but then:

 (1) You would have to explain both in the gitmodules_config_oid() and
 in this patch why "gitmodules_config_oid(...)" ->
 "gitmodules_config_oid(repo, ...)" and "submodule_free()" ->
 "submodule_free(repo)" are safe, since they have different behavior
 upon first glance. (They have the same behavior only because
 grep_objects() is always called with the_repository.) I was trying
 to explain this in as clear a way as possible, by showing (with
 code) that grep_objects() only works with, and is only called with,
 the_repository.
 (2) The code path where repo != the_repository would still never be
 exercised, and I prefer to not have such code paths.

I don't feel too strongly about (1), since they just concern commit
messages, of which there are many valid opinions on how to write them. I
feel a bit more strongly about (2), but can concede my point if the
project is OK with a code path not being exercised.

> Thanks for looking at the patches!
> I'd think this patch is orthogonal to the series, as this is about the effort
> of converting parts of git-grep whereas this series is fixing a bug (by
> converting parts of the submodule config machinery))?

It is orthogonal in the same way as your patch 1/5, I think - a
preparatory patch to make your "real" patches easier to understand.

[RFC PATCH 0/1] bdl-lib.sh: add bash debug logger

[Wink Saville]

Add bdl-lib.sh which provides functions to assit in debugging git
shell scripts and tests. The primary public interace are two routines,
bdl and bdl_nsl which print strings. The difference between the two
is that bdl outputs location of the statement optionally followed by
a string to print. For example:

  $ cat -n bdl-exmpl1.sh
   1  #!/usr/bin/env bash
   2  . bdl-lib.sh
   3  bdl "hi"
   4  
   5  # If no parameters just the location is printed
   6  bdl
  $ ./bdl-exmpl1.sh 
  bdl-exmpl1.sh:3: hi
  bdl-exmpl1.sh:6:

bdl_nsl means bdl with no source location being printed and at least
one parameter is required. For example:

  $ cat -n bdl-exmpl2.sh 
   1  #!/usr/bin/env bash
   2  . bdl-lib.sh
   3  bdl_nsl "hi"
   4
   5  # If no parameters nothing is printed
   6  bdl_nsl
  $ ./bdl-exmpl1.sh 
  hi

These routines can also take two parameters where the first parameter is
the destination for the string. There are two types of destinations,
either a single digit file descriptor 1..9 or a file name. For example:

  $ cat -n bdl-exmpl3.sh
   1  #!/usr/bin/env bash
   2  . bdl-lib.sh
   3  
   4  # Output to STDOUT
   5  bdl_nsl 1 "hi there"
   6  
   7  # Map 5 to STDOUT
   8  exec 5>&1
   9  bdl 5 "yo dude"
  10  
  11  # Output to a file
  12  bdl bdl_out.txt "good bye!"
  13  cat bdl_out.txt
  14  rm bdl_out.txt
  $ ./bdl-exmpl3.sh 
  hi there
  bdl-exmpl3.sh:9: yo dude
  bdl-exmpl3.sh:12: good bye!

If a destination is not provided as a parameter than there are two
variables, bdl_dst and bdl_stdout, that can be used to provide a
defaults. With bdl_dst taking presedence over bdl_stdout and a
destination parameter taking presedence over the variables. For example:

  $ cat -n bdl-exmpl4.sh 
   1  #!/usr/bin/env bash
   2  . bdl-lib.sh
   3  
   4  # Set defaults with bdl_dst taking presedence
   5  bdl_dst=bdl_dst_out.txt
   6  bdl_stdout=5
   7  
   8  bdl_nsl "printed by bdl_nsl to bdl_dst_out.txt"
   9  bdl "printed by bdl to bdl_dst_out.txt"
  10  
  11  # But the parameter the ultimate presedence
  12  bdl bdl_out.txt "good bye to bdl_out.txt"
  13  
  14  cat bdl_dst_out.txt
  15  cat bdl_out.txt
  16  
  17  rm bdl_dst_out.txt
  18  rm bdl_out.txt
  19  
  20  # Now clear bdl_dst and bdl_stdout takes presedence
  21  # but parameters take presedence
  22  bdl_dst=
  23  exec 5>&1
  24  
  25  bdl_nsl "monkeys via 5"
  26  bdl 1 "horses via 1"
  27  bdl bdl_out.txt "orcas to bdl_out.txt"
  28  
  29  cat bdl_out.txt
  30  rm bdl_out.txt
  $ ./bdl-exmpl4.sh 
  printed by bdl_nsl to bdl_dst_out.txt
  bdl-exmpl4.sh:9: printed by bdl to bdl_dst_out.txt
  bdl-exmpl4.sh:26: orcas to bdl_out.txt
  bdl-exmpl4.sh:12: good bye to bdl_out.txt
  monkeys via 5
  bdl-exmpl4.sh:26: horses via 1
  bdl-exmpl4.sh:27: orcas to bdl_out.txt


TODO: More tests and documentation needed.


Wink Saville (1):
  bdl-lib.sh: add bash debug logger

 bdl-exmpl.sh   |  46 
 bdl-lib.sh | 215 +
 t/t0014-bdl-lib.sh | 115 
 t/test-lib.sh  |   4 +
 4 files changed, 380 insertions(+)
 create mode 100755 bdl-exmpl.sh
 create mode 100644 bdl-lib.sh
 create mode 100755 t/t0014-bdl-lib.sh

-- 
2.16.3

[RFC PATCH 1/1] bdl-lib.sh: add bash debug logger

[Wink Saville]

Add bdl-lib to assist the programmer in debugging scripts and tests.
---
 bdl-exmpl.sh   |  46 
 bdl-lib.sh | 215 +
 t/t0014-bdl-lib.sh | 115 
 t/test-lib.sh  |   4 +
 4 files changed, 380 insertions(+)
 create mode 100755 bdl-exmpl.sh
 create mode 100644 bdl-lib.sh
 create mode 100755 t/t0014-bdl-lib.sh

diff --git a/bdl-exmpl.sh b/bdl-exmpl.sh
new file mode 100755
index 0..a47d82bca
--- /dev/null
+++ b/bdl-exmpl.sh
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# Examples using bdl-lib.sh
+
+# Source bdl-lib.sh
+. bdl-lib.sh
+
+# These both output to the default bdl_stdout=1
+bdl
+bdl "hi"
+bdl 1 "hi"
+
+# Output to a file as parameter
+echo -n >bdl_out.txt
+bdl bdl_out.txt "hi to bdl_out.txt"
+cat bdl_out.txt
+
+# Output to a file via bdl_dst
+echo -n >bdl_out.txt
+bdl_dst=bdl_out.txt
+bdl "hi to bdl_out.txt"
+cat bdl_out.txt
+bdl_dst=
+
+# Output to FD 5 connected to 1 via bdl_stdout
+bdl_stdout=5
+exec 5>&1
+bdl
+bdl "hi"
+bdl 5 "hi"
+exec 1>&5
+bdl_stdout=1
+
+# No printing to stdout
+echo -n >bdl_out.txt
+bdl_stdout=1
+bdl_dst=
+bdl 0 "not printed"
+bdl_stdout=0
+bdl
+bdl bdl_out.txt "printed to bdl_out.txt"
+bdl "not printed"
+bdl_stdout=1
+cat bdl_out.txt
+
+# This prints a "0" since there is only one parameter
+bdl 0
diff --git a/bdl-lib.sh b/bdl-lib.sh
new file mode 100644
index 0..cecf726bb
--- /dev/null
+++ b/bdl-lib.sh
@@ -0,0 +1,215 @@
+# ##
+# Bash Debug logger scriplet, source into your script to use.
+#
+# Write debug info with file name and line number.
+#
+# If the number of parameters == 0 then just the file
+# name and line number are printed to the destination.
+#
+# If the number of parameters == 1 then the file
+# name and line number are printed followed by a space
+# and then the parameter.
+# 
+# If number of parameters > 1 then the first parameter
+# is the destination and the other parameters are written
+# to the destination.
+#
+# The destination can be the first parameter to bdl
+# or bdl_stdout or bdl_dst. If the destination is empty
+# then the data is written to bdl_stdout unless bdl_stdout
+# is empty or 0 then no data is written. Also, if the
+# destination is 0 no data is written.
+#
+# Examples:
+#$ cat -n bdl-exmpl.sh 
+# 1#!/usr/bin/env bash
+# 2# Examples using bdl-lib.sh
+# 3
+# 4# Source bdl-lib.sh
+# 5. bdl-lib.sh
+# 6
+# 7# These both output to the default bdl_stdout=1
+# 8bdl
+# 9bdl "hi"
+#10bdl 1 "hi"
+#11
+#12# Output to a file as parameter
+#13echo -n >bdl_out.txt
+#14bdl bdl_out.txt "hi to bdl_out.txt"
+#15cat bdl_out.txt
+#16
+#17# Output to a file via bdl_dst
+#18echo -n >bdl_out.txt
+#19bdl_dst=bdl_out.txt
+#20bdl "hi to bdl_out.txt"
+#21cat bdl_out.txt
+#22bdl_dst=
+#23
+#24# Output to FD 5 connected to 1 via bdl_stdout
+#25bdl_stdout=5
+#26exec 5>&1
+#27bdl
+#28bdl "hi"
+#29bdl 5 "hi"
+#30exec 1>&5
+#31bdl_stdout=1
+#32
+#33# No printing to stdout
+#34echo -n >bdl_out.txt
+#35bdl_stdout=1
+#36bdl_dst=
+#37bdl 0 "not printed"
+#38bdl_stdout=0
+#39bdl
+#40bdl bdl_out.txt "printed to bdl_out.txt"
+#41bdl "not printed"
+#42bdl_stdout=1
+#43cat bdl_out.txt
+#44
+#45# This prints a "0" since there is only one parameter
+#46bdl 0
+#
+#
+# This is the output of bdl-exmpl.sh
+#
+#  $ /bin/bash ./bdl-exmpl.sh
+#  bdl-exmpl.sh:8:
+#  bdl-exmpl.sh:9: hi
+#  bdl-exmpl.sh:10: hi
+#  bdl-exmpl.sh:14: hi to bdl_out.txt
+#  bdl-exmpl.sh:20: hi to bdl_out.txt
+#  bdl-exmpl.sh:27:
+#  bdl-exmpl.sh:28: hi
+#  bdl-exmpl.sh:29: hi
+#  bdl-exmpl.sh:40: printed to bdl_out.txt
+#  bdl-exmpl.sh:46: 0
+# ##
+
+BDL_LOADED=t
+
+# Prompt waitng for a return, q will exit
+bdl_pause () {
+   read -p "Line ${BASH_LINENO}: $@" bdl_pause_v_
+   [[ "$bdl_pause_v_" == "q" ]] && exit 1
+}
+
+# Initialize bdl variables if user didn't
+[[ "$bdl_dst" == "" ]] && bdl_dst=
+[[ "$bdl_stdout" == "" ]] && bdl_stdout=1
+[[ "$bdl_call_depth" == "" ]] && bdl_call_depth=0
+[[ "$bdl_call_stack_view" == "" ]] && bdl_call_stack_view=f
+
+# Initialize priviate bdl variables
+_bdl_call_lineno_offset_array=()
+_bdl_call_lineno_offset_array_idx=0
+_bdl_call_save=()
+_bdl_call_save_idx=0
+
+# Push bdl state and initialize call meta data.
+#
+# $1 is value for bdl_call_depth
+# $2 Optional text of a script with where 

Re: [PATCH v2] test_must_be_empty: simplify file existence check

[Junio C Hamano]

SZEDER Gábor  writes:

> Commit 11395a3b4b (test_must_be_empty: make sure the file exists, not
> just empty, 2018-02-27) basically duplicated the 'test_path_is_file'
> helper function in 'test_must_be_empty'.
>
> Just call 'test_path_is_file' to avoid this code duplication.
>
> Signed-off-by: SZEDER Gábor 
> ---
>
> The only change is to refer to the right commit in the log message.
>
>  t/test-lib-functions.sh | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index d2eaf5ab67..36ad8accdd 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -718,11 +718,8 @@ verbose () {
>  # otherwise.
>  
>  test_must_be_empty () {
> - if ! test -f "$1"
> - then
> - echo "'$1' is missing"
> - return 1
> - elif test -s "$1"
> + test_path_is_file "$1" &&
> + if test -s "$1"
>   then
>   echo "'$1' is not empty, it contains:"
>   cat "$1"

"Just call it" is fine as an idea but

A &&
if B
then
...
fi

is somewhat questionable.  Shouldn't we make it

if A && B
then
...
fi

instead?  That way, if we ever need to add an else clause, the logic
flow would be more obvious, no?

Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule

[Jonathan Tan]

On Tue, 27 Mar 2018 14:39:18 -0700
Stefan Beller  wrote:

> connect_work_tree_and_git_dir is used to connect a submodule worktree with
> its git directory and vice versa after events that require a reconnection
> such as moving around the working tree. As submodules can have nested
> submoduled themselves, we'd also want to fix the nested submodules when

s/submoduled/submodules

> asked to. Add an option to recurse into the nested submodules and connect
> them as well.
> 
> As submodules are identified by their name (which determines their git
> directory in relation to their superprojects git directory) internally

s/superprojects/superproject's/

> and by their path in the working tree of the superproject, we need to
> make sure that the mapping of name <-> path is kept intact. We can do
> that in the git-mv command by writing out the gitmodules file and first

s/and //

> and then force a reload of the submodule config machinery.

s/force/forcing/

> 
> Signed-off-by: Stefan Beller 

[snip]

> +static void connect_wt_gitdir_in_nested(const char *sub_worktree,
> + const char *sub_gitdir,
> + struct repository *superproject)
> +{
> + int i;
> + struct repository subrepo;
> + struct strbuf sub_wt = STRBUF_INIT;
> + struct strbuf sub_gd = STRBUF_INIT;
> + const struct submodule *sub;
> + const char *super_worktree,
> +*sub_path; /* path inside the superproject */
> +
> + /* subrepo got moved, so superproject has outdated information */
> + submodule_free(superproject);
> +
> + super_worktree = real_pathdup(superproject->worktree, 1);
> +
> + sub_path = sub_worktree + strlen(super_worktree) + 1;
> +
> + if (repo_submodule_init(, superproject, sub_path))
> + return;
> +
> + repo_read_index();

>From the name of this function and its usage in
connect_work_tree_and_git_dir(), I expected this function to just
iterate through all the files in its workdir (which it is doing in the
"for" loop below) and connect any nested submodules. Why does it need
access to its superproject? (I would think that repo_init() would be
sufficient here instead of repo_submodule_init().)

> +
> + for (i = 0; i < subrepo.index->cache_nr; i++) {
> + const struct cache_entry *ce = subrepo.index->cache[i];
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + while (i + 1 < subrepo.index->cache_nr &&
> +!strcmp(ce->name, subrepo.index->cache[i + 1]->name))
> + /*
> +  * Skip entries with the same name in different stages
> +  * to make sure an entry is returned only once.
> +  */
> + i++;
> +
> + sub = submodule_from_path(, _oid, ce->name);
> + if (!sub)
> + /* submodule not checked out? */
> + continue;
> +
> + strbuf_reset(_wt);
> + strbuf_addf(_wt, "%s/%s/.git", sub_worktree, sub->path);
> +
> + strbuf_reset(_gd);
> + strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
> +
> + strbuf_setlen(_wt, sub_wt.len - strlen("/.git"));
> +
> + if (is_submodule_active(, ce->name)) {
> + connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 
> 0);
> + connect_wt_gitdir_in_nested(sub_wt.buf, sub_gd.buf, 
> );

The modifications of sub_wt and sub_gd should probably go here, since
they are not used unless this "if" block is executed.

> +void connect_work_tree_and_git_dir(const char *work_tree_,
> +const char *git_dir_,
> +int recurse_into_nested)

How is this function expected to be used? From what I see:
 - if recurse_into_nested is 0, this works regardless of whether the
   work_tree_ and git_dir_ is directly or indirectly a submodule of
   the_repository
 - if recurse_into_nested is 1, work_tree_ and git_dir_ must be directly
   a submodule of the_repository (since it is referenced directly)

This seems confusing - is this expected?

Re: [PATCH 3/5] submodule-config: add repository argument to submodule_from_{name, path}

[Stefan Beller]

On Tue, Mar 27, 2018 at 4:04 PM, Jonathan Tan  wrote:
> On Tue, 27 Mar 2018 14:39:16 -0700
> Stefan Beller  wrote:
>
>> -extern const struct submodule *submodule_from_name(
>> +extern const struct submodule *submodule_from_name(struct repository *r,
>>   const struct object_id *commit_or_tree, const char *name);
>> -extern const struct submodule *submodule_from_path(
>> +extern const struct submodule *submodule_from_path(struct repository *r,
>>   const struct object_id *commit_or_tree, const char *path);
>
> There is a recent change to CodingGuidelines that states to not include
> "extern" in 89a9f2c862 ("CodingGuidelines: mention "static" and
> "extern"", 2018-02-08), so consider removing "extern" since you're
> already changing this file.

Gah!

I have that fixed locally; it will be included in the next version

>
> Other than that,
>
> Reviewed-by: Jonathan Tan 

Thanks!

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Sun, Mar 25, 2018 at 3:58 AM René Scharfe  wrote:

> Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:
> >
> > Hmm... That's weird. I can reproduce it on 3 independant systems with
> > versions 2.16.2 up, although it does not work with version 2.11.0.
> > Anyway, I figured out how to reproduce this bug. It is caused when a
> > submodule is added and then the directory it resides in is moved or
> > deleted without commiting. For example:
> >
> > git init
> > git submodule add https://github.com/git/git git
> > mv git git.BAK
> > git submodule status #this command segfaults

> With the patch I sent in my first reply the last command reports:

>  fatal: no ref store in submodule 'git'

> That may not be the most helpful message -- not just the ref store is
> missing, the whole submodule is gone!

> Come to think about it, this removal may be intended.  How about
> showing the submodule as not being initialized at that point?

At first I thought we could still retrieve the ref store via a lookup of
path -> name in .gitmodules and then navigate to
.git/modules/ (as seen from the superproject)
and load the ref store. But loading the refstore is a mere detail.

"not initialized" is technically correct, the existing git directory
inside the superproject doesn't matter.


> -- >8 --
> Subject: [PATCH v2] submodule: check for NULL return of
get_submodule_ref_store()

Maybe more imperative, telling what we actually want
to achieve instead of what we do?

   submodule: report deleted submodules as not initialized

> If we can't find a ref store for a submodule then assume it the latter
> is not initialized (or was removed).  Print a status line accordingly
> instead of causing a segmentation fault by passing NULL as the first
> parameter of refs_head_ref().

Thanks for the message here. Looks good!

> Reported-by: Jeremy Feusi 

Please also sign off instead of just claiming to report it.
(The sign off has legal implications, see
https://developercertificate.org/ or our copy in
Documentation/SubmittingPatches)

> Signed-off-by: Rene Scharfe 
> ---
> Test missing..

Which would be added in t/t7400-submodule-basic.sh

Thanks for coming up with a sensible patch!
Stefan


>   builtin/submodule--helper.c | 8 ++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..ae3014ac5a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -654,9 +654,13 @@ static void status_submodule(const char *path, const
struct object_id *ce_oid,
>   displaypath);
>  } else if (!(flags & OPT_CACHED)) {
>  struct object_id oid;
> +   struct ref_store *refs = get_submodule_ref_store(path);

> -   if (refs_head_ref(get_submodule_ref_store(path),
> - handle_submodule_head_ref, ))
> +   if (!refs) {
> +   print_status(flags, '-', path, ce_oid,
displaypath);
> +   goto cleanup;
> +   }
> +   if (refs_head_ref(refs, handle_submodule_head_ref, ))
>  die(_("could not resolve HEAD ref inside the "
>"submodule '%s'"), path);

> --
> 2.17.0.rc1.38.g7c51fd80b8

[PATCH] submodule deinit: handle non existing pathspecs gracefully

[Stefan Beller]

This fixes a regression introduced in 22e612731b5 (submodule: port
submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
pathspecs that do not exist gracefully. This restores the historic behavior
of reporting the pathspec as unknown and returning instead of reporting a
bug.

Reported-by: Peter Oberndorfer 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..6ba8587b6d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, 
const char *prefix)
die(_("Use '--all' if you really want to deinitialize all 
submodules"));
 
if (module_list_compute(argc, argv, prefix, , ) < 0)
-   BUG("module_list_compute should not choke on empty pathspec");
+   return 1;
 
info.prefix = prefix;
if (quiet)
-- 
2.17.0.rc1.321.gba9d0f2565-goog

Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule

[Brandon Williams]

On 03/27, Stefan Beller wrote:
> connect_work_tree_and_git_dir is used to connect a submodule worktree with
> its git directory and vice versa after events that require a reconnection
> such as moving around the working tree. As submodules can have nested
> submoduled themselves, we'd also want to fix the nested submodules when
> asked to. Add an option to recurse into the nested submodules and connect
> them as well.
> 
> As submodules are identified by their name (which determines their git
> directory in relation to their superprojects git directory) internally
> and by their path in the working tree of the superproject, we need to
> make sure that the mapping of name <-> path is kept intact. We can do
> that in the git-mv command by writing out the gitmodules file and first
> and then force a reload of the submodule config machinery.
> 
> Signed-off-by: Stefan Beller 
> ---
>  builtin/mv.c|  6 ++--
>  builtin/submodule--helper.c |  3 +-
>  dir.c   | 70 +++--
>  dir.h   | 12 ++-
>  submodule.c |  6 ++--
>  t/t7001-mv.sh   |  2 +-
>  6 files changed, 87 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 6d141f7a53..7a63667d64 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char 
> *prefix)
>   die_errno(_("renaming '%s' failed"), src);
>   }
>   if (submodule_gitfile[i]) {
> - if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
> - connect_work_tree_and_git_dir(dst, 
> submodule_gitfile[i]);
>   if (!update_path_in_gitmodules(src, dst))
>   gitmodules_modified = 1;
> + if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
> + connect_work_tree_and_git_dir(dst,
> +   
> submodule_gitfile[i],
> +   1);
>   }
>  
>   if (mode == WORKING_DIRECTORY)
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a921fbbf56..05fd657f99 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   strbuf_reset();
>   }
>  
> - /* Connect module worktree and git dir */
> - connect_work_tree_and_git_dir(path, sm_gitdir);
> + connect_work_tree_and_git_dir(path, sm_gitdir, 0);
>  
>   p = git_pathdup_submodule(path, "config");
>   if (!p)
> diff --git a/dir.c b/dir.c
> index dedbf5d476..313176e291 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -19,6 +19,7 @@
>  #include "varint.h"
>  #include "ewah/ewok.h"
>  #include "fsmonitor.h"
> +#include "submodule-config.h"
>  
>  /*
>   * Tells read_directory_recursive how a file or directory should be treated.
> @@ -3010,8 +3011,67 @@ void untracked_cache_add_to_index(struct index_state 
> *istate,
>   untracked_cache_invalidate_path(istate, path, 1);
>  }
>  
> -/* Update gitfile and core.worktree setting to connect work tree and git dir 
> */
> -void connect_work_tree_and_git_dir(const char *work_tree_, const char 
> *git_dir_)
> +static void connect_wt_gitdir_in_nested(const char *sub_worktree,
> + const char *sub_gitdir,
> + struct repository *superproject)
> +{
> + int i;
> + struct repository subrepo;

You never clear this struct which means it leaks the memory it points
to.

> + struct strbuf sub_wt = STRBUF_INIT;
> + struct strbuf sub_gd = STRBUF_INIT;
> + const struct submodule *sub;
> + const char *super_worktree,
> +*sub_path; /* path inside the superproject */
> +
> + /* subrepo got moved, so superproject has outdated information */
> + submodule_free(superproject);
> +
> + super_worktree = real_pathdup(superproject->worktree, 1);
> +
> + sub_path = sub_worktree + strlen(super_worktree) + 1;
> +
> + if (repo_submodule_init(, superproject, sub_path))
> + return;
> +
> + repo_read_index();

You may want to check the return value to see if reading the index was
successful.

> +
> + for (i = 0; i < subrepo.index->cache_nr; i++) {
> + const struct cache_entry *ce = subrepo.index->cache[i];
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + while (i + 1 < subrepo.index->cache_nr &&
> +!strcmp(ce->name, subrepo.index->cache[i + 1]->name))
> + /*
> +  * Skip entries with the same name in different stages
> +  * to make sure an entry is returned only 

Re: [PATCH] grep: remove "repo" arg from non-supporting funcs

[Stefan Beller]

On Tue, Mar 27, 2018 at 3:58 PM, Jonathan Tan  wrote:
> As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
> repository'", 2017-08-02), many functions in builtin/grep.c were
> converted to also take "struct repository *" arguments. Among them were
> grep_object() and grep_objects().
>
> However, at least grep_objects() was converted incompletely - it calls
> gitmodules_config_oid(), which references the_repository.
>
> But it turns out that the conversion was extraneous anyway - there has
> been no user-visible effect - because grep_objects() is never invoked
> except with the_repository.
>
> Revert the changes to grep_objects() and grep_object() (which conversion
> is also extraneous) to show that both these functions do not support
> repositories other than the_repository.

I'd rather convert gitmodules_config_oid instead of reverting the other
functions into a world without an arbitrary repository object.

>
> Signed-off-by: Jonathan Tan 
> ---
> Patch 1/5 of your series is obviously correct.
>
> I investigated the change to grep_objects() in patch 2/5, and here is a
> patch summarizing my findings. Consider including this patch before 2/5
> (or before 1/5). You'll probably need to write
> "submodule_free(the_repository);" instead of what you have currently,
> but other than that, I don't think this affects your patch set much.

Thanks for looking at the patches!
I'd think this patch is orthogonal to the series, as this is about the effort
of converting parts of git-grep whereas this series is fixing a bug (by
converting parts of the submodule config machinery))?

Thanks,
Stefan

Re: [PATCH 4/5] submodule-config: remove submodule_from_cache

[Jonathan Tan]

On Tue, 27 Mar 2018 14:39:17 -0700
Stefan Beller  wrote:

> This continues the story of bf12fcdf5e (submodule-config: store
> the_submodule_cache in the_repository, 2017-06-22).
> 
> The previous patch taught submodule_from_path to take a repository into
> account, such that submodule_from_{path, cache} are the same now.
> Remove submodule_from_cache, migrating all its callers to
> submodule_from_path.
> 
> Signed-off-by: Stefan Beller 

Obviously correct, since submodule_from_{path,cache} are word-for-word
identical (other than parameter names).

Reviewed-by: Jonathan Tan 

[no subject]

[Cheah Teng Chye]

发自我的手机

[PATCH] grep: remove "repo" arg from non-supporting funcs

[Jonathan Tan]

As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
repository'", 2017-08-02), many functions in builtin/grep.c were
converted to also take "struct repository *" arguments. Among them were
grep_object() and grep_objects().

However, at least grep_objects() was converted incompletely - it calls
gitmodules_config_oid(), which references the_repository.

But it turns out that the conversion was extraneous anyway - there has
been no user-visible effect - because grep_objects() is never invoked
except with the_repository.

Revert the changes to grep_objects() and grep_object() (which conversion
is also extraneous) to show that both these functions do not support
repositories other than the_repository.

Signed-off-by: Jonathan Tan 
---
Patch 1/5 of your series is obviously correct.

I investigated the change to grep_objects() in patch 2/5, and here is a
patch summarizing my findings. Consider including this patch before 2/5
(or before 1/5). You'll probably need to write
"submodule_free(the_repository);" instead of what you have currently,
but other than that, I don't think this affects your patch set much.
---
 builtin/grep.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 789a89133..f286f2375 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -601,8 +601,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
-  struct object *obj, const char *name, const char *path,
-  struct repository *repo)
+  struct object *obj, const char *name, const char *path)
 {
if (obj->type == OBJ_BLOB)
return grep_oid(opt, >oid, name, 0, path);
@@ -629,7 +628,7 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
}
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
-   obj->type == OBJ_COMMIT, repo);
+   obj->type == OBJ_COMMIT, the_repository);
strbuf_release();
free(data);
return hit;
@@ -638,7 +637,6 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
 }
 
 static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
-   struct repository *repo,
const struct object_array *list)
 {
unsigned int i;
@@ -654,8 +652,8 @@ static int grep_objects(struct grep_opt *opt, const struct 
pathspec *pathspec,
submodule_free();
gitmodules_config_oid(_obj->oid);
}
-   if (grep_object(opt, pathspec, real_obj, list->objects[i].name, 
list->objects[i].path,
-   repo)) {
+   if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
+   list->objects[i].path)) {
hit = 1;
if (opt->status_only)
break;
@@ -1107,7 +1105,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
if (cached)
die(_("both --cached and trees are given."));
 
-   hit = grep_objects(, , the_repository, );
+   hit = grep_objects(, , );
}
 
if (num_threads)
-- 
2.16.0.rc0.35.ga4d78ce26

Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Stefan Beller]

On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer 
wrote:

> Hi,

> i tried to run "git submodule deinit xxx"
> on a submodule that was recently removed from the Rust project.
> But git responded with a BUG/Core dump (and also did not remove the
submodule directory from the checkout).

> ~/src/rust/rust$ git submodule deinit src/rt/hoedown/
> error: pathspec 'src/rt/hoedown/' did not match any file(s) known to git.
> BUG: builtin/submodule--helper.c:1045: module_list_compute should not
choke on empty pathspec
> Aborted (core dumped)

> I had a short look at submodule--helper.c and module_list_compute() is
called from multiple places.
> Most of them handle failure by return 1;
> Only module_deinit() seems to calls BUG() on failure.

Thanks for the analysis!

> This leaves me with 2 questions:
> 1) Should this code path just ignore the error and also return 1 like
other code paths?

This would be a sensible thing to do. I would think.
I just checked out v2.0.0 (an ancient version, way before the efforts to
rewrite
git-submodule in C were taking off) and there we can do

 $ git submodule deinit gerrit-gpg-asdf/
 ignoring UNTR extension
 error: pathspec 'gerrit-gpg-asdf/' did not match any file(s) known to
git.
 Did you forget to 'git add'?
 $ echo $?
 1

(The warning about the UNTR extension can be ignored that was introduced
later).
But the important part is that we get the same error for the missing
pathspec.
The next line ("Did you forget to git-add?") comes from git-ls-files which
at the time
was invoked by module_list() implemented in shell. I would think we can
live without
that line. So to fix the segfault, we can just s/BUG(..)/return 1/ as you
suggest.

> 2) Should "git submodule deinit" work on submodules that were removed by
upstream already?

To answer the question "Is this a submodule that upstream removed
(recently)?"
we'd have to put in some effort, essentially checking if that was ever a
submodule
(and not a directory or file).

When using "git pull --recurse-submodules" the submodule ought to be removed
automatically.

When doing a fetch && merge manually, we may want to teach merge to remove
a submodule that we have locally upon merge, too.

I view the git-submodule command as a bare bones plumbing helper, that we'd
want
to deprecate eventually as all other higher level commands will know how to
deal
with submodules.

So I think we do not want to teach "git submodule deinit" to remove dormant
repositories, that were submodules removed by upstream already.

> ~/src/rust/rust$ git submodule status
...
>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))

> -> strangely I get (null) for the current branch/commit in some
submodules?

This sounds like (3). Looking into that.

Thanks,
Stefan

Re: [PATCH] Support long format for log-based submodule diff

[Stefan Beller]

> >> $ git diff --submodule=log --submodule-log-detail=(long|short)
> >>
> >> I'm not sure what makes sense here. I welcome thoughts/discussion and
> >> will provide follow-up patches.
> >
> > The case of merges is usually configured with --[no-]merges, or
> > --min-parents=.

> But that is a knob that controls an irrelevant aspect of the detail
> in the context of this discussion, isn't it?  This code is about "to
> what degree the things that happened between two submodule commits
> in an adjacent pair of commits in the superproject are summarized?"

And I took it a step further and wanted to give a general solution, which
allows giving any option that the diff machinery accepts to only apply
to the submodule diffing part of the current diff.

> The hack Robert illustrates below is to change it to stop favouring
> such projects with "clean" histories, and show "log --oneline
> --no-merges --left-right".  When presented that way, clean histories
> of topic-branch based projects will suffer by losing conciseness,
> but clean histories of totally linear projects will still be shown
> the same way, and messy history that sometimes merges, sometimes
> merges mergy histories, and sometimes directly builds on the trunk
> will be shown as an enumeration of individual commits in a flat way
> by ignoring merges and not restricting the traversal to the first
> parent chains, which would appear more uniform than what the current
> code shows.

Oh, I realize this is in the *summary* code path, I was thinking about the
show_submodule_inline_diff, which would benefit from more diff options.

> I do not see a point in introducing --min/max-parents as a knob to
> control how the history is summarized.

For a summary a flat list of commits may be fine, ignoring
(ideally non-evil) merges.

> This is a strongly related tangent, but I wonder if we can and/or
> want to share more code with the codepath that prepares the log
> message for a merge.  It summarizes what happened on the side branch
> since it forked from the history it is joining back to (I think it
> is merge.c::shortlog() that computes this)

I do not find code there. To me it looks like builtin/fmt-merge-msg.c
is responsible for coming up with a default merge message?
In that file there is a shortlog() function, which walks revisions
and puts together the subject lines of commits.

> and it is quite similar
> to what Robert wants to use for submodules here.  On the other hand,
> in a project _without_ submodule, if you are pulling history made by
> your lieutenant whose history is full of linear merges of topic
> branches to the mainline, it may not be a bad idea to allow
> fmt-merge-msg to alternatively show something similar to the "diff
> --submodule=log" gives us, i.e. summarize the history of the side
> branch being merged by just listing the commits on the first-parent
> chain.  So I sense some opportunity for cross pollination here.

The cross pollination that I sense is the desire in both cases to freely
specify the format as it may depend on the workflow.

Stefan

Re: [PATCH v5] json_writer: new routines to create data in JSON format

[Wink Saville]

On Tue, Mar 27, 2018 at 2:02 PM  wrote:
>
> From: Jeff Hostetler 
>
> Add a series of jw_ routines and "struct json_writer" structure to compose
> JSON data.  The resulting string data can then be output by commands wanting
> to support a JSON output format.



>
>
>
>  void jw_object_intmax(struct json_writer *jw, const char *key, intmax_t 
> value)
> +{
> +   object_common(jw, key);
> +   strbuf_addf(>json, "%"PRIdMAX, (intmax_t)value);

Cast is not necessary.

> +}
> +



>
> +void jw_array_intmax(struct json_writer *jw, intmax_t value)
> +{
> +   array_common(jw);
> +   strbuf_addf(>json, "%"PRIdMAX, (intmax_t)value);

Cast is not necessary

>
> +}
> +



-- Wink

Re: Bug: duplicate sections in .git/config after remote removal

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 27 2018, Jason Frey wrote:

> While the impact of this bug is minimal, and git itself is not
> affected, it can affect external tools that want to read the
> .git/config file, expecting unique section names.
>
> To reproduce:
>
> Given the following example .git/config file (I am leaving out the
> [core] section for brevity):
>
> [remote "origin"]
> url = g...@github.com:Fryguy/example.git
> fetch = +refs/heads/*:refs/remotes/origin/*
> [branch "master"]
> remote = origin
> merge = refs/heads/master
>
> Running `git remote rm origin` will result in the following contents:
>
> [branch "master"]
>
> Running `git remote add origin g...@github.com:Fryguy/example.git` will
> result in the following contents:
>
> [branch "master"]
> [remote "origin"]
> url = g...@github.com:Fryguy/example.git
> fetch = +refs/heads/*:refs/remotes/origin/*
>
> And finally, running `git fetch origin; git branch -u origin/master`
> will result in the following contents:
>
> [branch "master"]
> [remote "origin"]
> url = g...@github.com:Fryguy/example.git
> fetch = +refs/heads/*:refs/remotes/origin/*
> [branch "master"]
> remote = origin
> merge = refs/heads/master
>
> at which point you can see the duplicate sections (even though one is
> empty).  Also note that if you do the steps again, you will be left
> with 3 sections, 2 of which are empty.  This process can be repeated
> over and over.

This can be annoying and result in some very verbose config files when
we automatically edit them, e.g.:

(rm -v /tmp/test.ini; for i in {1..3}; do git config -f /tmp/test.ini 
foo.bar 0 && git config -f /tmp/test.ini --unset foo.bar; done; cat 
/tmp/test.ini)
removed '/tmp/test.ini'
[foo]
[foo]
[foo]

But it's not so clear that it should be called a bug, yes we could be a
bit smarter and not add obvious crap like the example above (duplicate
sections at the end), but it gets less obvious in more complex cases,
see my c8b2cec09e ("branch: add test for -m renaming multiple config
sections", 2017-06-18) for one such example.

Git has a config format that's hybrid human/machine editable. Consider a
case like:

[gc]
;; Here's all the gc config we set up to avoid the great outage of 2015
autoDetach = false
;; Our aliases
[alias]
st = status

Now, if I run `git config gc.auto 0` is it better if we end up with:

[gc]
;; Here's all the gc config we set up to avoid the great outage of 2015
autoDetach = false
auto = 0
;; Our aliases
[alias]
st = status

Or something that makes it more clear that a machine added something at
the end:

[gc]
;; Here's all the gc config we set up to avoid the great outage of 2015
autoDetach = false
;; Our aliases
[alias]
st = status
[gc]
auto = 0

Most importantly though, regardless of what we decide to do when we
machine-edit the file, it's also human-editable, and being able to
repeat sections is part of our config format that you're simply going to
have to deal with.

The external tool (presumably some generic *.ini parser) you're trying
to point at git's config is broken for that purpose if it doesn't handle
duplicate sections. You're probably better off trying to parse `git
config --list --null` than trying to make it work.

I don't think we'd ever want to get rid of this feature, it's *very*
useful. Both for config via the include macro, and for people to
manually paste some config they want to try out to the end of their
config, without having to manually edit it to incorporate it into their
already existing sections.

Re: Bug: duplicate sections in .git/config after remote removal

[Stefan Beller]

On Tue, Mar 27, 2018 at 1:41 PM Jason Frey  wrote:

> at which point you can see the duplicate sections (even though one is
> empty).  Also note that if you do the steps again, you will be left
> with 3 sections, 2 of which are empty.  This process can be repeated
> over and over.

I agree that this is an issue for the user, and there were some attempts
to fix it in the past. (feel free to dig them up in the archive at
https://public-inbox.org/git)

IIRC the problem is (a) with the loose file format (What if the user put
a valuable comment just after or before the '[branch "master"]' line?)
as well as (b) the way the parser/writer works (single pass, line by line)

(b) specifically made it a "huge effort, but little return" bug,
so nobody got around for a proper fix.

Thanks,
Stefan

[PATCH 1/5] submodule.h: drop declaration of connect_work_tree_and_git_dir

[Stefan Beller]

The function connect_work_tree_and_git_dir is declared in both submodule.h
and dir.h, such that one of them is redundant. As the function is
implemented in dir.c, drop the declaration from submodule.h

Signed-off-by: Stefan Beller 
---
 submodule.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/submodule.h b/submodule.h
index 9589f13127..e5526f6aaa 100644
--- a/submodule.h
+++ b/submodule.h
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array 
*commits,
const char **refspec, int refspec_nr,
const struct string_list *push_options,
int dry_run);
-extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 /*
  * Given a submodule path (as in the index), return the repository
  * path of that submodule in 'buf'. Return -1 on error or when the
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 4/5] submodule-config: remove submodule_from_cache

[Stefan Beller]

This continues the story of bf12fcdf5e (submodule-config: store
the_submodule_cache in the_repository, 2017-06-22).

The previous patch taught submodule_from_path to take a repository into
account, such that submodule_from_{path, cache} are the same now.
Remove submodule_from_cache, migrating all its callers to
submodule_from_path.

Signed-off-by: Stefan Beller 
---
 repository.c   | 2 +-
 submodule-config.c | 9 -
 submodule-config.h | 3 ---
 submodule.c| 4 ++--
 4 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/repository.c b/repository.c
index 4ffbe9bc94..fa0a132e22 100644
--- a/repository.c
+++ b/repository.c
@@ -167,7 +167,7 @@ int repo_submodule_init(struct repository *submodule,
struct strbuf worktree = STRBUF_INIT;
int ret = 0;
 
-   sub = submodule_from_cache(superproject, _oid, path);
+   sub = submodule_from_path(superproject, _oid, path);
if (!sub) {
ret = -1;
goto out;
diff --git a/submodule-config.c b/submodule-config.c
index 4b7803e6ed..cb65354d4c 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct 
repository *r,
return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
-const struct submodule *submodule_from_cache(struct repository *repo,
-const struct object_id 
*treeish_name,
-const char *key)
-{
-   gitmodules_read_check(repo);
-   return config_from(repo->submodule_cache, treeish_name,
-  key, lookup_path);
-}
-
 void submodule_free(struct repository *r)
 {
if (r->submodule_cache)
diff --git a/submodule-config.h b/submodule-config.h
index ff3c9e0b5c..3ae8a1e51b 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_name(struct 
repository *r,
const struct object_id *commit_or_tree, const char *name);
 extern const struct submodule *submodule_from_path(struct repository *r,
const struct object_id *commit_or_tree, const char *path);
-extern const struct submodule *submodule_from_cache(struct repository *repo,
-   const struct object_id 
*treeish_name,
-   const char *key);
 extern void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index e94b7f9acd..89d0aee086 100644
--- a/submodule.c
+++ b/submodule.c
@@ -230,7 +230,7 @@ int is_submodule_active(struct repository *repo, const char 
*path)
const struct string_list *sl;
const struct submodule *module;
 
-   module = submodule_from_cache(repo, _oid, path);
+   module = submodule_from_path(repo, _oid, path);
 
/* early return if there isn't a path->module mapping */
if (!module)
@@ -1235,7 +1235,7 @@ static int get_next_submodule(struct child_process *cp,
if (!S_ISGITLINK(ce->ce_mode))
continue;
 
-   submodule = submodule_from_cache(spf->r, _oid, ce->name);
+   submodule = submodule_from_path(spf->r, _oid, ce->name);
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 5/5] submodule: fixup nested submodules after moving the submodule

[Stefan Beller]

connect_work_tree_and_git_dir is used to connect a submodule worktree with
its git directory and vice versa after events that require a reconnection
such as moving around the working tree. As submodules can have nested
submoduled themselves, we'd also want to fix the nested submodules when
asked to. Add an option to recurse into the nested submodules and connect
them as well.

As submodules are identified by their name (which determines their git
directory in relation to their superprojects git directory) internally
and by their path in the working tree of the superproject, we need to
make sure that the mapping of name <-> path is kept intact. We can do
that in the git-mv command by writing out the gitmodules file and first
and then force a reload of the submodule config machinery.

Signed-off-by: Stefan Beller 
---
 builtin/mv.c|  6 ++--
 builtin/submodule--helper.c |  3 +-
 dir.c   | 70 +++--
 dir.h   | 12 ++-
 submodule.c |  6 ++--
 t/t7001-mv.sh   |  2 +-
 6 files changed, 87 insertions(+), 12 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..7a63667d64 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
die_errno(_("renaming '%s' failed"), src);
}
if (submodule_gitfile[i]) {
-   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
-   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
if (!update_path_in_gitmodules(src, dst))
gitmodules_modified = 1;
+   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+   connect_work_tree_and_git_dir(dst,
+ 
submodule_gitfile[i],
+ 1);
}
 
if (mode == WORKING_DIRECTORY)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a921fbbf56..05fd657f99 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
strbuf_reset();
}
 
-   /* Connect module worktree and git dir */
-   connect_work_tree_and_git_dir(path, sm_gitdir);
+   connect_work_tree_and_git_dir(path, sm_gitdir, 0);
 
p = git_pathdup_submodule(path, "config");
if (!p)
diff --git a/dir.c b/dir.c
index dedbf5d476..313176e291 100644
--- a/dir.c
+++ b/dir.c
@@ -19,6 +19,7 @@
 #include "varint.h"
 #include "ewah/ewok.h"
 #include "fsmonitor.h"
+#include "submodule-config.h"
 
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
@@ -3010,8 +3011,67 @@ void untracked_cache_add_to_index(struct index_state 
*istate,
untracked_cache_invalidate_path(istate, path, 1);
 }
 
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree_, const char 
*git_dir_)
+static void connect_wt_gitdir_in_nested(const char *sub_worktree,
+   const char *sub_gitdir,
+   struct repository *superproject)
+{
+   int i;
+   struct repository subrepo;
+   struct strbuf sub_wt = STRBUF_INIT;
+   struct strbuf sub_gd = STRBUF_INIT;
+   const struct submodule *sub;
+   const char *super_worktree,
+  *sub_path; /* path inside the superproject */
+
+   /* subrepo got moved, so superproject has outdated information */
+   submodule_free(superproject);
+
+   super_worktree = real_pathdup(superproject->worktree, 1);
+
+   sub_path = sub_worktree + strlen(super_worktree) + 1;
+
+   if (repo_submodule_init(, superproject, sub_path))
+   return;
+
+   repo_read_index();
+
+   for (i = 0; i < subrepo.index->cache_nr; i++) {
+   const struct cache_entry *ce = subrepo.index->cache[i];
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   while (i + 1 < subrepo.index->cache_nr &&
+  !strcmp(ce->name, subrepo.index->cache[i + 1]->name))
+   /*
+* Skip entries with the same name in different stages
+* to make sure an entry is returned only once.
+*/
+   i++;
+
+   sub = submodule_from_path(, _oid, ce->name);
+   if (!sub)
+   /* submodule not checked out? */
+   continue;
+
+   strbuf_reset(_wt);
+   strbuf_addf(_wt, "%s/%s/.git", 

[PATCH 2/5] submodule-config: allow submodule_free to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-submodule-config.txt | 2 +-
 builtin/grep.c   | 2 +-
 submodule-config.c   | 6 +++---
 submodule-config.h   | 2 +-
 t/helper/test-submodule-config.c | 2 +-
 unpack-trees.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index ee907c4a82..fb06089393 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -38,7 +38,7 @@ Data Structures
 Functions
 -
 
-`void submodule_free()`::
+`void submodule_free(struct repository *r)`::
 
Use these to free the internally cached values.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 789a89133a..8f04cde18e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -651,7 +651,7 @@ static int grep_objects(struct grep_opt *opt, const struct 
pathspec *pathspec,
 
/* load the gitmodules file for this rev */
if (recurse_submodules) {
-   submodule_free();
+   submodule_free(repo);
gitmodules_config_oid(_obj->oid);
}
if (grep_object(opt, pathspec, real_obj, list->objects[i].name, 
list->objects[i].path,
diff --git a/submodule-config.c b/submodule-config.c
index 602ba8ca8b..a3efff1a34 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct 
repository *repo,
   key, lookup_path);
 }
 
-void submodule_free(void)
+void submodule_free(struct repository *r)
 {
-   if (the_repository->submodule_cache)
-   submodule_cache_clear(the_repository->submodule_cache);
+   if (r->submodule_cache)
+   submodule_cache_clear(r->submodule_cache);
 }
diff --git a/submodule-config.h b/submodule-config.h
index a5503a5d17..df6bd6e6db 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(
 extern const struct submodule *submodule_from_cache(struct repository *repo,
const struct object_id 
*treeish_name,
const char *key);
-extern void submodule_free(void);
+extern void submodule_free(struct repository *r);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index f23db3b19a..9971c5e9dd 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv)
arg += 2;
}
 
-   submodule_free();
+   submodule_free(the_repository);
 
return 0;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index d5685891a5..05e5fa77eb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,
if (!state && ce->ce_flags & CE_WT_REMOVE) {
repo_read_gitmodules(the_repository);
} else if (state && (ce->ce_flags & CE_UPDATE)) {
-   submodule_free();
+   submodule_free(the_repository);
checkout_entry(ce, state, NULL);
repo_read_gitmodules(the_repository);
}
-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH 3/5] submodule-config: add repository argument to submodule_from_{name, path}

[Stefan Beller]

This enables submodule_from_{name, path} to handle arbitrary repositories.
All callers just pass in the_repository, a later patch will pass in other
repos.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 14 +++---
 submodule-config.c   | 14 --
 submodule-config.h   |  4 ++--
 submodule.c  | 30 --
 t/helper/test-submodule-config.c |  6 --
 5 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..a921fbbf56 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -454,7 +454,7 @@ static void init_submodule(const char *path, const char 
*prefix,
 
displaypath = get_submodule_displaypath(path, prefix);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -621,7 +621,7 @@ static void status_submodule(const char *path, const struct 
object_id *ce_oid,
struct rev_info rev;
int diff_files_result;
 
-   if (!submodule_from_path(_oid, path))
+   if (!submodule_from_path(the_repository, _oid, path))
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
  path);
 
@@ -741,7 +741,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
if (argc != 2)
usage(_("git submodule--helper name "));
 
-   sub = submodule_from_path(_oid, argv[1]);
+   sub = submodule_from_path(the_repository, _oid, argv[1]);
 
if (!sub)
die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
@@ -772,7 +772,7 @@ static void sync_submodule(const char *path, const char 
*prefix,
if (!is_submodule_active(the_repository, path))
return;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (sub && sub->url) {
if (starts_with_dot_dot_slash(sub->url) ||
@@ -925,7 +925,7 @@ static void deinit_submodule(const char *path, const char 
*prefix,
struct strbuf sb_config = STRBUF_INIT;
char *sub_git_dir = xstrfmt("%s/.git", path);
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
 
if (!sub || !sub->name)
goto cleanup;
@@ -1367,7 +1367,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
-   sub = submodule_from_path(_oid, ce->name);
+   sub = submodule_from_path(the_repository, _oid, ce->name);
 
if (suc->recursive_prefix)
displaypath = relative_path(suc->recursive_prefix,
@@ -1650,7 +1650,7 @@ static const char *remote_submodule_branch(const char 
*path)
const char *branch = NULL;
char *key;
 
-   sub = submodule_from_path(_oid, path);
+   sub = submodule_from_path(the_repository, _oid, path);
if (!sub)
return NULL;
 
diff --git a/submodule-config.c b/submodule-config.c
index a3efff1a34..4b7803e6ed 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)
repo_read_gitmodules(repo);
 }
 
-const struct submodule *submodule_from_name(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_name(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *name)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, name, 
lookup_name);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, name, lookup_name);
 }
 
-const struct submodule *submodule_from_path(const struct object_id 
*treeish_name,
+const struct submodule *submodule_from_path(struct repository *r,
+   const struct object_id 
*treeish_name,
const char *path)
 {
-   gitmodules_read_check(the_repository);
-   return config_from(the_repository->submodule_cache, treeish_name, path, 
lookup_path);
+   gitmodules_read_check(r);
+   return config_from(r->submodule_cache, treeish_name, path, lookup_path);
 }
 
 const struct submodule *submodule_from_cache(struct repository *repo,
diff --git a/submodule-config.h b/submodule-config.h
index df6bd6e6db..ff3c9e0b5c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -39,9 +39,9 @@ extern int parse_update_recurse_submodules_arg(const char 
*opt, const char *arg)
 extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
 extern void 

[PATCH 0/5] Moving submodules with nested submodules

[Stefan Beller]

This fixes the bug reported in [1] ("Bug: moving submodules that have submodules
inside them causes a fatal error in git status")

[1] https://public-inbox.org/git/20180306192017.ga5...@riseup.net/

Thanks,
Stefan

Stefan Beller (5):
  submodule.h: drop declaration of connect_work_tree_and_git_dir
  submodule-config: allow submodule_free to handle arbitrary
repositories
  submodule-config: add repository argument to submodule_from_{name,
path}
  submodule-config: remove submodule_from_cache
  submodule: fixup nested submodules after moving the submodule

 .../technical/api-submodule-config.txt|  2 +-
 builtin/grep.c|  2 +-
 builtin/mv.c  |  6 +-
 builtin/submodule--helper.c   | 17 +++--
 dir.c | 70 ++-
 dir.h | 12 +++-
 repository.c  |  2 +-
 submodule-config.c| 29 +++-
 submodule-config.h|  9 +--
 submodule.c   | 40 ++-
 submodule.h   |  1 -
 t/helper/test-submodule-config.c  |  8 ++-
 t/t7001-mv.sh |  2 +-
 unpack-trees.c|  2 +-
 14 files changed, 135 insertions(+), 67 deletions(-)

-- 
2.17.0.rc1.321.gba9d0f2565-goog

[PATCH v5] json_writer: new routines to create data in JSON format

[git]

From: Jeff Hostetler 

Add a series of jw_ routines and "struct json_writer" structure to compose
JSON data.  The resulting string data can then be output by commands wanting
to support a JSON output format.

The json-writer routines can be used to generate structured data in a
JSON-like format.  We say "JSON-like" because we do not enforce the Unicode
(usually UTF-8) requirement on string fields.  Internally, Git does not
necessarily have Unicode/UTF-8 data for most fields, so it is currently
unclear the best way to enforce that requirement.  For example, on Linx
pathnames can contain arbitrary 8-bit character data, so a command like
"status" would not know how to encode the reported pathnames.  We may want
to revisit this (or double encode such strings) in the future.

The initial use for the json-writer routines is for generating telemetry
data for executed Git commands.  Later, we may want to use them in other
commands, such as status.

Helped-by: René Scharfe 
Helped-by: Wink Saville 
Helped-by: Ramsay Jones 
Signed-off-by: Jeff Hostetler 
---
 Makefile|   2 +
 json-writer.c   | 394 ++
 json-writer.h   |  91 +++
 t/helper/test-json-writer.c | 572 
 t/t0019-json-writer.sh  | 253 
 5 files changed, 1312 insertions(+)
 create mode 100644 json-writer.c
 create mode 100644 json-writer.h
 create mode 100644 t/helper/test-json-writer.c
 create mode 100755 t/t0019-json-writer.sh

diff --git a/Makefile b/Makefile
index 1a9b23b..57f58e6 100644
--- a/Makefile
+++ b/Makefile
@@ -662,6 +662,7 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-hashmap
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-json-writer
 TEST_PROGRAMS_NEED_X += test-lazy-init-name-hash
 TEST_PROGRAMS_NEED_X += test-line-buffer
 TEST_PROGRAMS_NEED_X += test-match-trees
@@ -815,6 +816,7 @@ LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
+LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
 LIB_OBJS += line-log.o
diff --git a/json-writer.c b/json-writer.c
new file mode 100644
index 000..1b49158
--- /dev/null
+++ b/json-writer.c
@@ -0,0 +1,394 @@
+#include "cache.h"
+#include "json-writer.h"
+
+void jw_init(struct json_writer *jw)
+{
+   strbuf_reset(>json);
+   strbuf_reset(>open_stack);
+   jw->first = 0;
+   jw->pretty = 0;
+}
+
+void jw_release(struct json_writer *jw)
+{
+   strbuf_release(>json);
+   strbuf_release(>open_stack);
+}
+
+/*
+ * Append JSON-quoted version of the given string to 'out'.
+ */
+static void append_quoted_string(struct strbuf *out, const char *in)
+{
+   unsigned char c;
+
+   strbuf_addch(out, '"');
+   while ((c = *in++) != '\0') {
+   if (c == '"')
+   strbuf_addstr(out, "\\\"");
+   else if (c == '\\')
+   strbuf_addstr(out, "");
+   else if (c == '\n')
+   strbuf_addstr(out, "\\n");
+   else if (c == '\r')
+   strbuf_addstr(out, "\\r");
+   else if (c == '\t')
+   strbuf_addstr(out, "\\t");
+   else if (c == '\f')
+   strbuf_addstr(out, "\\f");
+   else if (c == '\b')
+   strbuf_addstr(out, "\\b");
+   else if (c < 0x20)
+   strbuf_addf(out, "\\u%04x", c);
+   else
+   strbuf_addch(out, c);
+   }
+   strbuf_addch(out, '"');
+}
+
+static inline void indent_pretty(struct json_writer *jw)
+{
+   int k;
+
+   if (!jw->pretty)
+   return;
+
+   for (k = 0; k < jw->open_stack.len; k++)
+   strbuf_addstr(>json, "  ");
+}
+
+static inline void begin(struct json_writer *jw, char ch_open, int pretty)
+{
+   jw->pretty = pretty;
+   jw->first = 1;
+
+   strbuf_addch(>json, ch_open);
+
+   strbuf_addch(>open_stack, ch_open);
+}
+
+/*
+ * Assert that the top of the open-stack is an object.
+ */
+static inline void assert_in_object(const struct json_writer *jw, const char 
*key)
+{
+   if (!jw->open_stack.len)
+   die("json-writer: object: missing jw_object_begin(): '%s'", 
key);
+   if (jw->open_stack.buf[jw->open_stack.len - 1] != '{')
+   die("json-writer: object: not in object: '%s'", key);
+}
+
+/*
+ * Assert that the top of the open-stack is an array.
+ */
+static inline void assert_in_array(const struct json_writer *jw)
+{
+   if (!jw->open_stack.len)
+   die("json-writer: array: missing jw_array_begin()");
+   if (jw->open_stack.buf[jw->open_stack.len - 1] != '[')
+   

[PATCH v5] routines to generate JSON data

[git]

From: Jeff Hostetler 

This is version 5 of my JSON data format routines.

This version address the uint64_t vs intmax_t formatting issues
that were discussed on the mailing list.  I removed the jw_*_int()
and jw_*_uint64() routines and replaced them with a single
jw_*_intmax() routine.

Also added a jw_release() routine similar to strbuf_release()
and fixed the indentation of sub-array when pretty printing is
enabled.

The following PR includes my WIP telemetry changes that build
upon the json-writer routines and demonstrates how they might
be used.  The first commit in this PR is this patch.

 https://github.com/jeffhostetler/git/pull/11


Jeff Hostetler (1):
  json_writer: new routines to create data in JSON format

 Makefile|   2 +
 json-writer.c   | 394 ++
 json-writer.h   |  91 +++
 t/helper/test-json-writer.c | 572 
 t/t0019-json-writer.sh  | 253 
 5 files changed, 1312 insertions(+)
 create mode 100644 json-writer.c
 create mode 100644 json-writer.h
 create mode 100644 t/helper/test-json-writer.c
 create mode 100755 t/t0019-json-writer.sh

-- 
2.9.3

Bug: duplicate sections in .git/config after remote removal

[Jason Frey]

While the impact of this bug is minimal, and git itself is not
affected, it can affect external tools that want to read the
.git/config file, expecting unique section names.

To reproduce:

Given the following example .git/config file (I am leaving out the
[core] section for brevity):

[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

Running `git remote rm origin` will result in the following contents:

[branch "master"]

Running `git remote add origin g...@github.com:Fryguy/example.git` will
result in the following contents:

[branch "master"]
[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*

And finally, running `git fetch origin; git branch -u origin/master`
will result in the following contents:

[branch "master"]
[remote "origin"]
url = g...@github.com:Fryguy/example.git
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

at which point you can see the duplicate sections (even though one is
empty).  Also note that if you do the steps again, you will be left
with 3 sections, 2 of which are empty.  This process can be repeated
over and over.

Thanks,
Jason

git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Peter Oberndorfer]

Hi,

i tried to run "git submodule deinit xxx"
on a submodule that was recently removed from the Rust project.
But git responded with a BUG/Core dump (and also did not remove the submodule 
directory from the checkout).

~/src/rust/rust$ git submodule deinit src/rt/hoedown/
error: pathspec 'src/rt/hoedown/' did not match any file(s) known to git.
BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on 
empty pathspec
Aborted (core dumped)

I had a short look at submodule--helper.c and module_list_compute() is called 
from multiple places.
Most of them handle failure by return 1;
Only module_deinit() seems to calls BUG() on failure.

This leaves me with 2 questions:
1) Should this code path just ignore the error and also return 1 like other 
code paths?
2) Should "git submodule deinit" work on submodules that were removed by 
upstream already?

For more debugging information please see below.

Thanks,
Greetings Peter



~/src/rust/rust$ git --version
git version 2.17.0.rc1.47.g9f57127417.dirty
(this should basically be 90bbd502d54fe920356fa9278055dc9c9bfe9a56 + some 
Makefile adjustments)

Git Gui reports
src/rt/hoedown
Untracked, not staged
* Git Repository (subproject)


~/src/rust/rust$ git status
On branch fix_literal_attribute_doc
Untracked files:
  (use "git add ..." to include in what will be committed)

src/rt/


~/src/rust/rust$ cat .git/config
...
[submodule "src/rt/hoedown"]
url = https://github.com/rust-lang/hoedown.git
...
-> there is no "active = true" in this hoedown section
which is present on some (not all) other submodules


~/src/rust/rust$ cat .gitmodules
-> does not contain any references to hoedown anymore as they were remove by 
upstream


~/src/rust/rust$ cat src/rt/hoedown/.git
gitdir: ../../../.git/modules/src/rt/hoedown


~/src/rust/rust/src/rt/hoedown$ git status
HEAD detached at da282f1
nothing to commit, working tree clean

-> so there is a working git repository at src/rt/hoedown


~/src/rust/rust$ git submodule status
 9b2dcac06c3e23235f8997b3c5f2325a6d3382df src/dlmalloc (heads/master)
 b889e1e30c5e9953834aa9fa6c982bb28df46ac9 src/doc/book 
(remotes/origin/ch10-edits-137-gb889e1e3)
 6a8f0a27e9a58c55c89d07bc43a176fdae5e051c src/doc/nomicon (remotes/origin/HEAD)
 76296346e97c3702974d3398fdb94af9e10111a2 src/doc/reference 
(remotes/origin/HEAD)
 d5ec87eabe5733cc2348c7dada89fc67c086f391 src/doc/rust-by-example 
(remotes/origin/HEAD)
 1f5a28755e301ac581e2048011e4e0ff3da482ef src/jemalloc (3.6.0-775-g1f5a2875)
 263a703b10351d8930e48045b4fd09768991b867 src/libcompiler_builtins 
(remotes/origin/auto-10-g263a703)
 ed04152aacf5b4798f78ff13396f3c04c0a77144 src/liblibc (0.2.37-29-ged04152aac)
 6ceaaa4b0176a200e4bbd347d6a991ab6c776ede src/llvm 
(remotes/origin/rust-llvm-release-6-0-0)
-2717444753318e461e0c3b30dacd03ffbac96903 src/llvm-emscripten
 bcb720e55861c38db47f2ebdf26b7198338cb39d src/stdsimd ((null))
 311a5eda6f90d660bb23e97c8ee77090519b9eda src/tools/cargo 
(0.14.0-2144-g311a5eda)
 eafd09010815da43302ac947afee45b0f5219e6b src/tools/clippy 
(v0.0.189-21-geafd0901)
 b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))
 d4712ca37500f26bbcbf97edcb27820717f769f7 src/tools/miri 
(remotes/origin/hack_branch_for_miri_do_not_delete_until_merged)
 f5a0c91a39368395b1c1ad322e04be7b6074bc65 src/tools/rls (0.125-131-gf5a0c91)
 118e078c5badd520d18b92813fd88789c8d341ab src/tools/rust-installer 
(remotes/origin/HEAD)
 374dba833e22cc8df8e16e19cccbde61c69d9aed src/tools/rustfmt (0.4.1-35-g374dba83)

-> strangely I get (null) for the current branch/commit in some submodules?

Re: [PATCH] branch: implement shortcut to delete last branch

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 27 2018, Ævar Arnfjörð Bjarmason wrote:

> [...]With that, some comments on the change below:

Also, didn't mean to gang up on you. I only saw Jonathan's E-Mail after
I sent mine, and it covered some of the same stuff.

Re: [PATCH] branch: implement shortcut to delete last branch

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 27 2018, Aaron Greenberg wrote:

> This patch gives git-branch the ability to delete the previous
> checked-out branch using the "-" shortcut. This shortcut already exists
> for git-checkout, git-merge, and git-revert. A common workflow is
>
> 1. Do some work on a local topic-branch and push it to a remote.
> 2. 'remote/topic-branch' gets merged in to 'remote/master'.
> 3. Switch back to local master and fetch 'remote/master'.
> 4. Delete previously checked-out local topic-branch.
>
> $ git checkout -b topic-a
> $ # Do some work...
> $ git commit -am "Implement feature A"
> $ git push origin topic-a
>
> $ git checkout master
> $ git branch -d topic-a
> $ # With this patch, a user could simply type
> $ git branch -d -
>
> "-" is a useful shortcut for cleaning up a just-merged branch
> (or a just switched-from branch.)
>
> Signed-off-by: Aaron Greenberg 

So just a tip on this E-Mail chain/patch submission. When you submit a
v2 make the subject "[PATCH v2] ...", see
Documentation/SubmittingPatches, also instead of sending two mails with
the same subject better to put any comments not in the commit message...

> ---

...right here, below the triple dash, and CC the people commenting on
the initial thread. With that, some comments on the change below:

>  builtin/branch.c  | 3 +++
>  t/t3200-branch.sh | 8 
>  2 files changed, 11 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6d0cea9..9e37078 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>   char *target = NULL;
>   int flags = 0;
>
> + if (!strcmp(argv[i], "-"))
> + argv[i] = "@{-1}";
> +

If we just do this, then when I do the following:

1. be on the 'foo' branch
2. checkout 'bar', commit
3. checkout 'foo'
4. git branch -d -

I get this message:

error: The branch 'bar' is not fully merged.
If you are sure you want to delete it, run 'git branch -D bar'

While that works, I think it's better UI for us to suggest what's
actually the important alternation to the user's command, i.e. replace
-d with -D, otherwise they'll think "oh '-' doesn't work, let's try to
name the branch", only to get the same error. I.e. this on top:

diff --git a/builtin/branch.c b/builtin/branch.c
index cdf2de4f1d..081a4384ce 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -157,17 +157,18 @@ static int branch_merged(int kind, const char *name,

 static int check_branch_commit(const char *branchname, const char *refname,
   const struct object_id *oid, struct commit 
*head_rev,
-  int kinds, int force)
+  int kinds, int force, int resolved_dash)
 {
struct commit *rev = lookup_commit_reference(oid);
if (!rev) {
-   error(_("Couldn't look up commit object for '%s'"), refname);
+   error(_("Couldn't look up commit object for '%s'"), 
resolved_dash ? "-" : refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
error(_("The branch '%s' is not fully merged.\n"
  "If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'."), branchname,
+ resolved_dash ? "-" : branchname);
return -1;
}
return 0;
@@ -220,9 +221,12 @@ static int delete_branches(int argc, const char **argv, 
int force, int kinds,
for (i = 0; i < argc; i++, strbuf_reset()) {
char *target = NULL;
int flags = 0;
+   int resolved_dash = 0;

-   if (!strcmp(argv[i], "-"))
+   if (!strcmp(argv[i], "-")) {
argv[i] = "@{-1}";
+   resolved_dash = 1;
+   }

strbuf_branchname(, argv[i], allowed_interpret);
free(name);
@@ -255,7 +259,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,

if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
check_branch_commit(bname.buf, name, , head_rev, kinds,
-   force)) {
+   force, resolved_dash)) {
ret = 1;
goto next;
}

There are other error messages there, but as far as I can tell it's best
if those just talk about the "bar" branch, but have a look.

A test for that with i18ngrep left as an exercise...

>   strbuf_branchname(, argv[i], allowed_interpret);
>   free(name);
>   name = mkpathdup(fmt, bname.buf);
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 

Re: [PATCH] branch: implement shortcut to delete last branch

[Jonathan Nieder]

Hi,

Aaron Greenberg wrote:

> This patch gives git-branch the ability to delete the previous
> checked-out branch using the "-" shortcut. This shortcut already exists
> for git-checkout, git-merge, and git-revert. A common workflow is
>
> 1. Do some work on a local topic-branch and push it to a remote.
> 2. 'remote/topic-branch' gets merged in to 'remote/master'.
> 3. Switch back to local master and fetch 'remote/master'.
> 4. Delete previously checked-out local topic-branch.

Thanks for a clear example.

[...]
>  builtin/branch.c  | 3 +++
>  t/t3200-branch.sh | 8 
>  2 files changed, 11 insertions(+)
[...]
> With the approvals listed in [*1*] and in accordance with the
> guidelines set out in Documentation/SubmittingPatches, I am submitting
> this patch to be applied upstream.
>
> After work on this patch is done, I'll look into picking up where the
> prior work done in [*2*] left off.
>
> Is there anything else that needs to be done before this can be
> accepted?
>
> [Reference]
>
> *1* 
> https://public-inbox.org/git/1521844835-23956-2-git-send-emai...@aaronjgreenberg.com/
> *2* 
> https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddhart...@gmail.com/

For the future, please don't use a separate cover letter message in a
single-patch series like this one.  Instead, please put any discussion
that you don't want to go in the commit message after the three-dash
divider in the same message as the patch, like the diffstat.  See the
section "Sending your patches" in Documentation/SubmittingPatches for
more details:

| You often want to add additional explanation about the patch,
| other than the commit message itself.  Place such "cover letter"
| material between the three-dash line and the diffstat.  For
| patches requiring multiple iterations of review and discussion,
| an explanation of changes between each iteration can be kept in
| Git-notes and inserted automatically following the three-dash
| line via `git format-patch --notes`.

That makes it easier for reviewers to see all the information in one
place and in particular can help them in fleshing out the commit
message if it is missing details.

[...]
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6d0cea9..9e37078 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>   char *target = NULL;
>   int flags = 0;
>  
> + if (!strcmp(argv[i], "-"))
> + argv[i] = "@{-1}";
> +
>   strbuf_branchname(, argv[i], allowed_interpret);

This makes me wonder: should the "-" shortcut be handled in
strbuf_branchname itself?  That would presumably simplify callers like
this one.

[...]
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out 
> branch fails' '
>   test_must_fail git branch -d my7
>  '
>  
> +test_expect_success 'test deleting last branch' '
> + git checkout -b my7.1 &&

This naming scheme feels likely to conflict with other patches.
How about something like

git checkout -B previous &&
git checkout -B new-branch &&
git show-ref --verify refs/heads/previous &&
git branch -d - &&
test_must_fail git show-ref --verify refs/heads/previous

?

> + git checkout  - &&
> + test_path_is_file .git/refs/heads/my7.1 &&
> + git branch -d - &&
> + test_path_is_missing .git/refs/heads/my7.1

not specific to this test, but this is relying on low-level details
and means that an implementation that e.g. deleted a loose ref but
kept a packed ref would pass the test despite being broken.

Some of the other tests appear to use show-ref, so that might work
well.

No need to act on this, since what you have here is at least
consistent with some of the other tests in the file.  In other words,
it might be even better to address this throughout the file in a
separate patch.

> +'
> +

A few questions that the tests leave unanswered for me:

 1. Does "git branch -d -" refuse to delete an un-merged branch
like "git branch -d topic" would?  (That seems like a valuable
thing to test for typo protection reasons.)

 2. What happens if there is no previous branch, as in e.g. a new
clone?

 3. What does the error message look like when it cannot delete the
previous branch for whatever reason?  Does it identify the branch
that can't be deleted?

>  test_expect_success 'test --track without .fetch entries' '
>   git branch --track my8 &&
>   test "$(git config branch.my8.remote)" &&

Thanks and hope that helps,
Jonathan

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

This patch gives git-branch the ability to delete the previous
checked-out branch using the "-" shortcut. This shortcut already exists
for git-checkout, git-merge, and git-revert. A common workflow is

1. Do some work on a local topic-branch and push it to a remote.
2. 'remote/topic-branch' gets merged in to 'remote/master'.
3. Switch back to local master and fetch 'remote/master'.
4. Delete previously checked-out local topic-branch.

$ git checkout -b topic-a
$ # Do some work...
$ git commit -am "Implement feature A"
$ git push origin topic-a

$ git checkout master
$ git branch -d topic-a
$ # With this patch, a user could simply type
$ git branch -d -

"-" is a useful shortcut for cleaning up a just-merged branch
(or a just switched-from branch.)

Signed-off-by: Aaron Greenberg 
---
 builtin/branch.c  | 3 +++
 t/t3200-branch.sh | 8 
 2 files changed, 11 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9..9e37078 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
char *target = NULL;
int flags = 0;
 
+   if (!strcmp(argv[i], "-"))
+   argv[i] = "@{-1}";
+
strbuf_branchname(, argv[i], allowed_interpret);
free(name);
name = mkpathdup(fmt, bname.buf);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea..78c25aa 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch 
fails' '
test_must_fail git branch -d my7
 '
 
+test_expect_success 'test deleting last branch' '
+   git checkout -b my7.1 &&
+   git checkout  - &&
+   test_path_is_file .git/refs/heads/my7.1 &&
+   git branch -d - &&
+   test_path_is_missing .git/refs/heads/my7.1
+'
+
 test_expect_success 'test --track without .fetch entries' '
git branch --track my8 &&
test "$(git config branch.my8.remote)" &&
-- 
2.7.4

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

With the approvals listed in [*1*] and in accordance with the
guidelines set out in Documentation/SubmittingPatches, I am submitting
this patch to be applied upstream.

After work on this patch is done, I'll look into picking up where the
prior work done in [*2*] left off.

Is there anything else that needs to be done before this can be
accepted?

[Reference]

*1* 
https://public-inbox.org/git/1521844835-23956-2-git-send-emai...@aaronjgreenberg.com/
*2* 
https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddhart...@gmail.com/

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
> johannes.schinde...@gmx.de> wrote:
> 
> > I guess we should add a test where we copy the `git` executable into a
> > subdirectory with the name "git" and call `git/git --exec-path` and
> > verify that its output matches our expectation?
> 
> I'm actually a little fuzzy on the testing model here.

Alright, I'll bite.

You are correct that the test must be contingent on the RUNTIME_PREFIX
prerequisite. This could be tested thusly:

test_lazy_prereq RUNTIME_PREFIX '
# test whether we built with RUNTIME_PREFIX support
grep " -DRUNTIME_PREFIX" "$GIT_BUILD_DIR/GIT-CFLAGS"
'

The subsequent test would run like this:

test_expect_success RUNTIME_PREFIX '
mkdir git &&
cp "$GIT_BUILD_DIR/git$X" git/ &&
path="$(git/git$X --exec-path)" &&
case "$(echo "$path" | tr '\\' /)" in
"$(pwd)/libexec/git-core") ;; # okay
*)
echo "Unexpected exec path: $path" >&2
return 1
;;
esac
'

I say "like this" because it is a little bit tricky to get right, in
particular when supporting Windows ;-)

For example, when building with Visual C, the dependencies' .dll files
need to be copied into the same directory as the .exe files because there
is no good central place to put them (don't get me started on the problems
incurred by some software copying some random OpenSSL version's
ssleay32.dll into C:\Windows\system32, unless you buy me beer all night
and want to be entertained). And that obviously would fail with this
approach.

> As things are, this test will only work if Git is relocatable; however,
> the test suite doesn't seem to be equipped to build multiple versions of
> Git for different tests.  From this I conclude that the right approach
> would be to make a test that runs conditional on RUNTIME_PREFIX being
> set, but I'm not familiar enough with the testing framework to be
> confident that this is correct, or really how to go about writing such a
> test.
> 
> A simple grep suggests that the current test suite doesn't seem to have any
> RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
> been doing it with a "config.mak" file that explicitly enables
> RUNTIME_PREFIX to get the runtime prefix code tested against the standard
> Git testing suites.

Indeed, this would be the first test.

>  From a Git maintainer's perspective, would such a test be a
>  prerequisite for landing this patch series, or is this a good candidate
>  for follow-up work to improve our testing coverage?

I cannot speak for Junio, but from my understanding he would probably be
fine without such a test. Or a separate patch at a later stage that
introduces that.

Or something completely different such as a helper in t/helper/ that
always succeeds if RUNTIME_PREFIX is not defined, otherwise passes argv[1]
as parameter to git_resolve_executable_dir() and outputs that. Would be a
lot more robust than what I described above. But I would want for Duy's
test-tool patch series to land first because I would hate to introduce
*yet* another stand-alone .exe in t/helper/.

Ciao,
Dscho

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Ramsay Jones]

On 27/03/18 18:14, Jeff Hostetler wrote:
> 
> 
> On 3/27/2018 11:45 AM, Ramsay Jones wrote:
>>
>>
>> On 27/03/18 04:18, Ramsay Jones wrote:
>>> On 26/03/18 15:31, g...@jeffhostetler.com wrote:
 From: Jeff Hostetler 

>>> [snip]
>>>
>>> Thanks, this version fixes all issues I had (with the compilation
>>> and sparse warnings).
>>>
>>> [Was using UINT64_C(0x) a problem on windows?]
>>
>> BTW, I forgot to mention that you had some whitespace problems
>> with this patch, viz:
> 
> I ran "make sparse" on this and it did not complain about any of this.

No, sparse doesn't check for whitespace issues.

> What command do you run to get these messages?

'git am' told me when I applied the patch from the ML, and ...

> I know they appear as red in diffs (and I usually double check that),
> but I had not seen a command to complain about them like this.

... since I applied the patch yesterday, it was easier to
demonstrate the issue today using this command:

>>    $ git log --oneline -1 --check master-json
>>    ab643d838 (master-json) json_writer: new routines to create data in JSON 
>> format
>>    t/helper/test-json-writer.c:280: trailing whitespace.
>>    + */
>>    t/t0019-json-writer.sh:179: indent with spaces.
>>    +    "g": 0,
>>    t/t0019-json-writer.sh:180: indent with spaces.
>>    +    "h": 1
>>    $
> 
> the leading spaces are required in this case.
> the pretty json output contains 8 spaces for that sub-structure not a tab.
> is there a preferred way to denote this in the test script?

OK, I should have looked at the output more closely! :(

Only the trailing whitespace in test-json-writer.c needs
to be addressed then.

ATB,
Ramsay Jones

RE: [PATCH v4] json_writer: new routines to create data in JSON format

[Randall S. Becker]

On March 27, 2018 1:43 PM, Wink Saville wrote:
> > the leading spaces are required in this case.
> > the pretty json output contains 8 spaces for that sub-structure not a tab.
> > is there a preferred way to denote this in the test script?
> >
> > Jeff
> 
> I've used "git diff --check" which I got from
> Documentation/SubmittingPatches.

While I have not done this in the git suite, my own suites use something along 
the lines of the following when I need (and have to validate) exact spacing at 
the beginning of lines in expected output:

cat <<-EOF | sed "1,\$s/_/ /g" >expect
{ level1 ...
{ level2
Etc.
EOF

Providing you don't use _ elsewhere in the output. It's a bit hacky but works.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Wink Saville]

> the leading spaces are required in this case.
> the pretty json output contains 8 spaces for that sub-structure not a tab.
> is there a preferred way to denote this in the test script?
>
> Jeff

I've used "git diff --check" which I got from Documentation/SubmittingPatches.

-- Wink

[GSoC][PATCH v5] test: avoid pipes in git related commands for test

[Pratik Karki]

Thank you Eric, I made changes according to your review.


Cheers,
Pratik

-- >8 --

Avoid using pipes downstream of Git commands since the exit codes
of commands upstream of pipes get swallowed, thus potentially
hiding failure of those commands. Instead, capture Git command
output to a file and apply the downstream command(s) to that file.


Signed-off-by: Pratik Karki 
---
 t/t5300-pack-object.sh |  8 ++---
 t/t5510-fetch.sh   |  8 ++---
 t/t7001-mv.sh  | 22 ++---
 t/t7003-filter-branch.sh   |  9 --
 t/t9104-git-svn-follow-parent.sh   | 16 +-
 t/t9108-git-svn-glob.sh| 14 +
 t/t9109-git-svn-multi-glob.sh  | 24 --
 t/t9110-git-svn-use-svm-props.sh   | 40 
 t/t9111-git-svn-use-svnsync-props.sh   | 36 ++---
 t/t9114-git-svn-dcommit-merge.sh   | 10 +++---
 t/t9130-git-svn-authors-file.sh| 28 ++---
 t/t9138-git-svn-authors-prog.sh| 31 +-
 t/t9153-git-svn-rewrite-uuid.sh|  8 ++---
 t/t9168-git-svn-partially-globbed-names.sh | 34 +++-
 t/t9350-fast-export.sh | 50 ++
 15 files changed, 179 insertions(+), 159 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 9c68b9925..156beb2d5 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -311,8 +311,8 @@ test_expect_success 'unpacking with --strict' '
rm -f .git/index &&
tail -n 10 LIST | git update-index --index-info &&
ST=$(git write-tree) &&
-   PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
-   git pack-objects test-5 ) &&
+   git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
+   PACK5=$( git pack-objects test-5 actual &&
+   PACK5=$( git pack-objects test-5 &1 | \
-   grep -e "->" | cut -c 22- >../actual
+   git -c fetch.output=full fetch origin >actual 2>&1 &&
+   grep -e "->" actual | cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master   -> origin/master
@@ -855,8 +855,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
test_commit extraaa &&
(
cd compact &&
-   git -c fetch.output=compact fetch origin 2>&1 | \
-   grep -e "->" | cut -c 22- >../actual
+   git -c fetch.output=compact fetch origin >actual 2>&1 &&
+   grep -e "->" actual | cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master -> origin/*
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d4e6485a2..e96cbdb10 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -21,8 +21,8 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-grep "^R100..*path0/COPYING..*path1/COPYING"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+grep "^R100..*path0/COPYING..*path1/COPYING" actual'
 
 test_expect_success \
 'moving the file back into subdirectory' \
@@ -35,8 +35,8 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-grep "^R100..*path1/COPYING..*path0/COPYING"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+grep "^R100..*path1/COPYING..*path0/COPYING" actual'
 
 test_expect_success \
 'mv --dry-run does not move file' \
@@ -122,10 +122,9 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path0/COPYING..*path2/COPYING" &&
- git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path0/README..*path2/README"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
+ grep "^R100..*path0/README..*path2/README" actual'
 
 test_expect_success \
 'succeed when source is a prefix of destination' \
@@ -141,10 +140,9 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path2/COPYING..*path1/path2/COPYING" &&
- git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path2/README..*path1/path2/README"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual &&
+ grep "^R100..*path2/README..*path1/path2/README" actual'
 
 test_expect_success \
 'do not move directory over existing directory' \
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..6a28b6cce 100755
--- a/t/t7003-filter-branch.sh
+++ 

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:
> I would rather have something like ref_store_reinit() in the same
> spirit as the second call of set_git_dir() in setup_work_tree. It is
> hacky, but it works and keeps changes to minimal (so that it could be
> easily replaced later).

So in the name of hacky and dirty things, it would look something like
this. This passed your test case. The test suite is still running
(slow laptop) but I don't expect breakages there.

-- 8< --
diff --git a/refs.c b/refs.c
index 20ba82b434..c6116c4f7a 100644
--- a/refs.c
+++ b/refs.c
@@ -1660,6 +1660,16 @@ struct ref_store *get_main_ref_store(void)
return main_ref_store;
 }
 
+void make_main_ref_store_use_absolute_paths(void)
+{
+   files_force_absolute_paths(get_main_ref_store());
+}
+
+void make_main_ref_store_use_relative_paths(const char *cwd)
+{
+   files_make_relative_paths(get_main_ref_store(), cwd);
+}
+
 /*
  * Associate a ref store with a name. It is a fatal error to call this
  * function twice for the same name.
diff --git a/refs.h b/refs.h
index 01be5ae32f..532a4ad09d 100644
--- a/refs.h
+++ b/refs.h
@@ -759,6 +759,9 @@ int reflog_expire(const char *refname, const struct 
object_id *oid,
 int ref_storage_backend_exists(const char *name);
 
 struct ref_store *get_main_ref_store(void);
+void make_main_ref_store_use_absolute_paths(void);
+void make_main_ref_store_use_relative_paths(const char *cwd);
+
 /*
  * Return the ref_store instance for the specified submodule. For the
  * main repository, use submodule==NULL; such a call cannot fail. For
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..629198826f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3092,6 +3092,32 @@ static int files_reflog_expire(struct ref_store 
*ref_store,
return -1;
 }
 
+void files_force_absolute_paths(struct ref_store *ref_store)
+{
+   struct files_ref_store *refs =
+   files_downcast(ref_store, REF_STORE_WRITE, "don't ask");
+
+   char *path = refs->gitdir;
+   refs->gitdir = absolute_pathdup(path);
+   free(path);
+
+   path = refs->gitcommondir;
+   refs->gitcommondir = absolute_pathdup(path);
+   free(path);
+}
+
+void files_make_relative_paths(struct ref_store *ref_store, const char *cwd)
+{
+   struct files_ref_store *refs =
+   files_downcast(ref_store, REF_STORE_WRITE, "don't ask");
+
+   const char *path = remove_leading_path(refs->gitdir, cwd);
+   refs->gitdir = absolute_pathdup(path);
+
+   path = remove_leading_path(refs->gitcommondir, cwd);
+   refs->gitcommondir = absolute_pathdup(path);
+}
+
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
struct files_ref_store *refs =
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..827e97bcca 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -669,4 +669,7 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
+void files_force_absolute_paths(struct ref_store *refs);
+void files_make_relative_paths(struct ref_store *refs, const char *cwd);
+
 #endif /* REFS_REFS_INTERNAL_H */
diff --git a/setup.c b/setup.c
index 7287779642..a5f4396b4e 100644
--- a/setup.c
+++ b/setup.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "dir.h"
 #include "string-list.h"
+#include "refs.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -389,8 +390,10 @@ void setup_work_tree(void)
 
work_tree = get_git_work_tree();
git_dir = get_git_dir();
-   if (!is_absolute_path(git_dir))
+   if (!is_absolute_path(git_dir)) {
git_dir = real_path(get_git_dir());
+   make_main_ref_store_use_absolute_paths();
+   }
if (!work_tree || chdir(work_tree))
die(_("this operation must be run in a work tree"));
 
@@ -402,6 +405,7 @@ void setup_work_tree(void)
setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
 
set_git_dir(remove_leading_path(git_dir, work_tree));
+   make_main_ref_store_use_relative_paths(work_tree);
initialized = 1;
 }
 
-- 8< --

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff Hostetler]

On 3/27/2018 11:45 AM, Ramsay Jones wrote:



On 27/03/18 04:18, Ramsay Jones wrote:

On 26/03/18 15:31, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 


[snip]

Thanks, this version fixes all issues I had (with the compilation
and sparse warnings).

[Was using UINT64_C(0x) a problem on windows?]


BTW, I forgot to mention that you had some whitespace problems
with this patch, viz:


I ran "make sparse" on this and it did not complain about any of this.

What command do you run to get these messages?
I know they appear as red in diffs (and I usually double check that),
but I had not seen a command to complain about them like this.



   $ git log --oneline -1 --check master-json
   ab643d838 (master-json) json_writer: new routines to create data in JSON 
format
   t/helper/test-json-writer.c:280: trailing whitespace.
   + */
   t/t0019-json-writer.sh:179: indent with spaces.
   +"g": 0,
   t/t0019-json-writer.sh:180: indent with spaces.
   +"h": 1
   $


the leading spaces are required in this case.
the pretty json output contains 8 spaces for that sub-structure not a tab.
is there a preferred way to denote this in the test script?

Jeff

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

On Tue, Mar 27, 2018 at 6:47 PM, Jeff King  wrote:
>> So I would not mind papering over it right now (with an understanding
>> that absolute path pays some more overhead in path walking, which was
>> th reason we tried to avoid it in setup code). A slightly better patch
>> is trigger this path absolutization from setup_work_tree(), near
>> set_git_dir(). But then you face some ugliness there: how to find out
>> all ref stores to update, or just update the main ref store alone.
>
> I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to
> avoid the way the path is munged? Or is it to avoid some lazy-setup that
> is triggered by calling get_git_dir() at all (which doesn't make much
> sense to me, because we'd already have called get_git_dir() much
> earlier). Or is it just because we may sometimes fill in refs->git_dir
> with something besides get_git_dir() (for a submodule or worktree or
> something)?

None of those, I think. git_path() does some magic to translate paths
so that refs/... ends up with $GIT_COMMON_DIR/refs/... while "index"
ends up with $GIT_DIR/index. Michael wanted to avoid that magic and
keep the control within refs code (i.e. this code knows refs/ and
packed-refs are shared, and pseudo refs are not, what git_path()
decides does not matter).

> I.e., can we do one of (depending on which of those answers is "yes"):
>
>   1. Stop caching the value of get_git_dir(), and instead call it
>  on-demand instead of looking at refs->git_dir? (If we just want to
>  avoid git_path()).

This probably works, but I count it as papering over the problem too.

>
>   2. If we need to avoid even calling get_git_dir(), can we add a
>  "light" version of it that avoids whatever side effects we're
>  trying to skip?
>
>   3. If the problem is just that sometimes we need get_git_dir() and
>  sometimes not, could we perhaps store NULL as a sentinel to mean
>  "look up get_git_dir() when you need it"?
>
>  That would let submodules and worktrees fill in their paths as
>  necessary (assuming they never change after init), but handle the
>  case of get_git_dir() changing.
>
> Hmm. Typing that out, it seems like (3) is probably the right path.
> Something like the patch below seems to fix it and passes the tests.

Honestly I think this is just another way to work around the problem
(with even more changes than your abspath approach). The problem is
with setup_work_tree(). We create a ref store at a specific location
and it should stay working without lazily calling get_git_dir(), which
has nothing to do (anymore) with the path we have given a ref store.
If somebody changes a global setting like $CWD, it should be well
communicated to everybody involved.

I would rather have something like ref_store_reinit() in the same
spirit as the second call of set_git_dir() in setup_work_tree. It is
hacky, but it works and keeps changes to minimal (so that it could be
easily replaced later).
-- 
Duy

Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader

[Duy Nguyen]

On Tue, Mar 27, 2018 at 6:25 PM, Duy Nguyen  wrote:
> On Tue, Mar 27, 2018 at 6:11 PM, Jeff King  wrote:
>> On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
>>
>>> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
>>> > In order to allow for better control flow when protocol_v2 is introduced
>>> > +static enum protocol_version discover_version(struct packet_reader 
>>> > *reader)
>>> > +{
>>> > +   enum protocol_version version = protocol_unknown_version;
>>> > +
>>> > +   /*
>>> > +* Peek the first line of the server's response to
>>> > +* determine the protocol version the server is speaking.
>>> > +*/
>>> > +   switch (packet_reader_peek(reader)) {
>>> > +   case PACKET_READ_EOF:
>>> > +   die_initial_contact(0);
>>> > +   case PACKET_READ_FLUSH:
>>>
>>> gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
>>> at least gcc 7.x), it fails to realize that this die_initial_contact()
>>> will not fall through (even though we do tell it about die() not
>>> returning, but I guess that involves more flow analysis to realize
>>> die_initial_contact is in the same boat).
>>> [...]
>>> @@ -124,6 +124,7 @@ enum protocol_version discover_version(struct 
>>> packet_reader *reader)
>>>   switch (packet_reader_peek(reader)) {
>>>   case PACKET_READ_EOF:
>>>   die_initial_contact(0);
>>> + break;
>>
>> Would it make sense just to annotate that function to help the flow
>> analysis?
>
> Yes that works wonderfully with my gcc-7.3.0

And this changes things. Since this series is 35 patches and there's
no sign of reroll needed, I'm going to make this change separately.
Don't reroll just because of this
-- 
Duy

Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader

[Brandon Williams]

On 03/27, Duy Nguyen wrote:
> On Tue, Mar 27, 2018 at 6:25 PM, Duy Nguyen  wrote:
> > On Tue, Mar 27, 2018 at 6:11 PM, Jeff King  wrote:
> >> On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
> >>
> >>> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
> >>> > In order to allow for better control flow when protocol_v2 is introduced
> >>> > +static enum protocol_version discover_version(struct packet_reader 
> >>> > *reader)
> >>> > +{
> >>> > +   enum protocol_version version = protocol_unknown_version;
> >>> > +
> >>> > +   /*
> >>> > +* Peek the first line of the server's response to
> >>> > +* determine the protocol version the server is speaking.
> >>> > +*/
> >>> > +   switch (packet_reader_peek(reader)) {
> >>> > +   case PACKET_READ_EOF:
> >>> > +   die_initial_contact(0);
> >>> > +   case PACKET_READ_FLUSH:
> >>>
> >>> gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
> >>> at least gcc 7.x), it fails to realize that this die_initial_contact()
> >>> will not fall through (even though we do tell it about die() not
> >>> returning, but I guess that involves more flow analysis to realize
> >>> die_initial_contact is in the same boat).
> >>> [...]
> >>> @@ -124,6 +124,7 @@ enum protocol_version discover_version(struct 
> >>> packet_reader *reader)
> >>>   switch (packet_reader_peek(reader)) {
> >>>   case PACKET_READ_EOF:
> >>>   die_initial_contact(0);
> >>> + break;
> >>
> >> Would it make sense just to annotate that function to help the flow
> >> analysis?
> >
> > Yes that works wonderfully with my gcc-7.3.0
> 
> And this changes things. Since this series is 35 patches and there's
> no sign of reroll needed, I'm going to make this change separately.
> Don't reroll just because of this
> -- 
> Duy

Looks like a good change, but yes, it should work fine as a patch on
top.

-- 
Brandon Williams

Re: [PATCH v3] git{,-blame}.el: remove old bitrotting Emacs code

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote[1]:

> The git-blame.el mode has been superseded by Emacs's own
> vc-annotate (invoked by C-x v g). Users of the git.el mode are now
> much better off using either Magit or the Git backend for Emacs's own
> VC mode.
>
> These modes were added over 10 years ago when Emacs's own Git support
> was much less mature, and there weren't other mature modes in the wild
> or shipped with Emacs itself.
>
> These days these modes have few if any users, and users of git aren't
> well served by us shipping these (some OS's install them alongside git
> by default, which is confusing and leads users astray).
>
> So let's remove these per Alexandre Julliard's message to the
> ML[1]. If someone still wants these for some reason they're better
> served by hosting these elsewhere (e.g. on ELPA), instead of us
> distributing them with git.

The trouble with removing these so abruptly is that it makes for a bad
user experience.

  Warning (initialization): An error occurred while loading ‘/home/jrn/.emacs’:

  File error: Cannot open load file, No such file or directory, git

In some sense that is the distributor's fault: just because Git
upstream stops removing the git.el file doesn't mean that the
distributor needs to.  But the same thing would happen if the user
symlinked git.el into a place that emacs could find when using
upstream Git directly.  And we are putting the distributor in a bad
place.

Ami Fischman (cc-ed) writes:

| IMO a placeholder git.el that did something like:
|
|   (error "git.el is no more; replace (require 'git) with (require 'magit) or
|   simply delete the former in your initialization file(s)")
|
| ideally with a pointer to a short URL explaining the rationale would have
| been fine.
| (note that though I've seen
| https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=893734 I'm _still_ unclear
| as to why the change was made; you might want to clarify in that bug and
| point to it from this, or something else)

What do you think?  Would adding such a placeholder during a
transitional period work well for you?

Thanks,
Jonathan

[1] https://public-inbox.org/git/20180310184545.16950-1-ava...@gmail.com/

Re: [PATCH v7 00/13] nd/pack-objects-pack-struct updates

[Jeff King]

On Mon, Mar 26, 2018 at 07:04:54PM +0200, Duy Nguyen wrote:

> >> +unsigned long oe_get_size_slow(struct packing_data *pack,
> >> +const struct object_entry *e)
> [...]
> > But short of that, it's probably worth a comment explaining what's going
> > on.
> 
> I thought the elaboration on "size" in the big comment block in front
> of struct object_entry was enough. I was wrong. Will add something
> here.

It may be my fault for reading the interdiff, which didn't include that
comment. I was literally just thinking something like:

  /*
   * Return the size of the object without doing any delta
   * reconstruction (so non-deltas are true object sizes, but
   * deltas return the size of the delta data).
   */

> > I see there's a one-off test for GIT_TEST_FULL_IN_PACK_ARRAY, which I
> > think is a good idea, since it makes sure the code is exercised in a
> > normal test suite run. Should we do the same for GIT_TEST_OE_SIZE_BITS?
> 
> I think the problem with OE_SIZE_BITS is it has many different code
> paths (like reused deltas) which is hard to make sure it runs. But yes
> I think I could construct a pack that executes both code paths in
> oe_get_size_slow(). Will do in a reroll.

OK. If it's too painful to construct a good example, don't worry about
it.  It sounds like we're unlikely to get full coverage anyway.

> > I haven't done an in-depth read of each patch yet; this was just what
> > jumped out at me from reading the interdiff.
> 
> I would really appreciate it if you could find some time to do it. The
> bugs I found in this round proved that I had no idea what's really
> going on in pack-objects. Sure I know the big picture but that's far
> from enough to do changes like this.

I didn't get to it today, but I'll try to give it a careful read. There
are quite a few corners of pack-objects I don't know well, but I think
at this point I may be the most expert of remaining people. Scary. :)

-Peff

Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

[Junio C Hamano]

Duy Nguyen  writes:

 On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano  wrote:
>>
>>  - connect.c -Werror=implicit-fallthough around die_initial_contact().
>
> This I did not expect. But I just looked again and I had this option
> explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not
> complain about this. gcc-7.3 does. What's your compiler/version?

gcc (Debian 7.3.0-5) 7.3.0

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

On Tue, Mar 27, 2018 at 04:56:00PM +0200, Duy Nguyen wrote:

> The way setup_work_tree() does it is just bad to me. When it calls
> set_git_dir() again, the assumption is the new path is exactly the
> same as the old one. The only difference is relative vs absolute. But
> this is super hard to see from set_git_dir implementation. The new
> struct repository design also inherits this (i.e. allowing to call
> set_git_dir multiple times, which really does not make sense), but
> this won't fly long term. When cwd moves, all cached relative paths
> must be adjusted, we need a better mechanism to tell everybody that,
> not just do it for $GIT_DIR and related paths.

Yeah, I agree it's questionable.

> I am planning to fix this. This is part of the "setup cleanup" effort
> to keep repository.c design less messy and hopefully unify how the
> main repo and submodule repo are created/set up. But frankly that may
> take a long while before I could submit anything substantial (I also
> have the "report multiple worktree's HEADs correctly and make fsck
> count all HEADs" task, which is not small and to me have higher
> priority).
> 
> So I would not mind papering over it right now (with an understanding
> that absolute path pays some more overhead in path walking, which was
> th reason we tried to avoid it in setup code). A slightly better patch
> is trigger this path absolutization from setup_work_tree(), near
> set_git_dir(). But then you face some ugliness there: how to find out
> all ref stores to update, or just update the main ref store alone.

I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to
avoid the way the path is munged? Or is it to avoid some lazy-setup that
is triggered by calling get_git_dir() at all (which doesn't make much
sense to me, because we'd already have called get_git_dir() much
earlier). Or is it just because we may sometimes fill in refs->git_dir
with something besides get_git_dir() (for a submodule or worktree or
something)?

I.e., can we do one of (depending on which of those answers is "yes"):

  1. Stop caching the value of get_git_dir(), and instead call it
 on-demand instead of looking at refs->git_dir? (If we just want to
 avoid git_path()).

  2. If we need to avoid even calling get_git_dir(), can we add a
 "light" version of it that avoids whatever side effects we're
 trying to skip?

  3. If the problem is just that sometimes we need get_git_dir() and
 sometimes not, could we perhaps store NULL as a sentinel to mean
 "look up get_git_dir() when you need it"?

 That would let submodules and worktrees fill in their paths as
 necessary (assuming they never change after init), but handle the
 case of get_git_dir() changing.

Hmm. Typing that out, it seems like (3) is probably the right path.
Something like the patch below seems to fix it and passes the tests.

diff --git a/refs.c b/refs.c
index 20ba82b434..4094f0e7d4 100644
--- a/refs.c
+++ b/refs.c
@@ -1656,7 +1656,7 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
+   main_ref_store = ref_store_init(NULL, REF_STORE_ALL_CAPS);
return main_ref_store;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..22118b5764 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -69,6 +69,7 @@ struct files_ref_store {
struct ref_store base;
unsigned int store_flags;
 
+   /* may be NULL to force lookup of get_git_dir() */
char *gitdir;
char *gitcommondir;
 
@@ -94,17 +95,23 @@ static struct ref_store *files_ref_store_create(const char 
*gitdir,
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
-   struct strbuf sb = STRBUF_INIT;
 
base_ref_store_init(ref_store, _be_files);
refs->store_flags = flags;
 
-   refs->gitdir = xstrdup(gitdir);
-   get_common_dir_noenv(, gitdir);
-   refs->gitcommondir = strbuf_detach(, NULL);
-   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
-   refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
-   strbuf_release();
+   if (gitdir) {
+   struct strbuf sb = STRBUF_INIT;
+   refs->gitdir = xstrdup(gitdir);
+   get_common_dir_noenv(, gitdir);
+   refs->gitcommondir = strbuf_detach(, NULL);
+   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
+   refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+   strbuf_release();
+   } else {
+   refs->gitdir = NULL;
+   refs->gitcommondir = NULL;
+   refs->packed_ref_store = packed_ref_store_create(NULL, flags);
+   }
 
return ref_store;
 }
@@ -147,6 +154,16 @@ static struct files_ref_store 

Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own file

[Junio C Hamano]

Jameson Miller  writes:

> This moves the reusable parts of the memory pool logic used by
> fast-import.c into its own file for use by other components.
>
> Signed-off-by: Jameson Miller 
> ---
>  Makefile  |  1 +
>  fast-import.c | 70 
> +--
>  mem-pool.c| 55 ++
>  mem-pool.h| 34 +
>  4 files changed, 91 insertions(+), 69 deletions(-)
>  create mode 100644 mem-pool.c
>  create mode 100644 mem-pool.h

OK.  This is indeed straight-forward line movements and nothing else,
other than obviously a few static helpers are now extern.

I said I'd anticipate that the allocation that bypasses the pool
subsystem would want to become traceable by the pool subsystem,
which would allow us to free() the pieces of memory allocated
directly with xmalloc() in mem_pool_alloc() instead of leaking.  I
am OK if the structure of this series is to make that change after
these three steps we see here.  When that happens, it will start to
make sense to bill the "this is too big so do not attempt to carve
it out from block, and do not overallocate and make the remainder
available for later requests" to the pool instance mem_pool_alloc()
is working on, as that piece of memory is also known to a specific
pool instance.

After I wrote review for 2/3, I found out that you changed the
meaning of total_allocd (which should probably be described in its
log message).  Unlike the original that counted "total", it now is
used only for memory that is allocated directly by fast-import.c and
does not account for memory obtained by calling mem-pool.

The output routine is changed in 2/3 to add fi_mem_pool's pool_alloc
to it, so billing oversized allocation that does *not* belong to any
specific pool to _some_ pool and ignoring total_allocd would cancel
things out.  It still feels a bit fishy, but I think it is OK.

So all in all, I think we are in no worse shape than the original
(we call it "bug-to-bug compatible" ;-)), and successfully extracted
a reusable piece in a separate file in a clean way so that we can
refine and extend it further.  Nicely done.

Will queue; the proposed log for step 2/3 may want to be a bit
polished, though.

Thanks.

Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader

[Duy Nguyen]

On Tue, Mar 27, 2018 at 6:11 PM, Jeff King  wrote:
> On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
>
>> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
>> > In order to allow for better control flow when protocol_v2 is introduced
>> > +static enum protocol_version discover_version(struct packet_reader 
>> > *reader)
>> > +{
>> > +   enum protocol_version version = protocol_unknown_version;
>> > +
>> > +   /*
>> > +* Peek the first line of the server's response to
>> > +* determine the protocol version the server is speaking.
>> > +*/
>> > +   switch (packet_reader_peek(reader)) {
>> > +   case PACKET_READ_EOF:
>> > +   die_initial_contact(0);
>> > +   case PACKET_READ_FLUSH:
>>
>> gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
>> at least gcc 7.x), it fails to realize that this die_initial_contact()
>> will not fall through (even though we do tell it about die() not
>> returning, but I guess that involves more flow analysis to realize
>> die_initial_contact is in the same boat).
>> [...]
>> @@ -124,6 +124,7 @@ enum protocol_version discover_version(struct 
>> packet_reader *reader)
>>   switch (packet_reader_peek(reader)) {
>>   case PACKET_READ_EOF:
>>   die_initial_contact(0);
>> + break;
>
> Would it make sense just to annotate that function to help the flow
> analysis?

Yes that works wonderfully with my gcc-7.3.0

> Like:
>
> diff --git a/connect.c b/connect.c
> index c3a014c5ba..49eca46462 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
> return check_ref(ref->name, flags);
>  }
>
> -static void die_initial_contact(int unexpected)
> +static NORETURN void die_initial_contact(int unexpected)
>  {
> if (unexpected)
> die(_("The remote end hung up upon initial contact"));
>
> That should let the callers know what's going on, and inside the
> function itself, the compiler should confirm that all code paths hit
> another NORETURN function.
>
> -Peff



-- 
Duy

Re: [PATCH v3 2/5] stash: convert apply to builtin

[Joel Teichroeb]

On Tue, Mar 27, 2018 at 9:02 AM, Johannes Schindelin
 wrote:
> Hi Joel,
>
> [...]
>> +
>> +static int do_apply_stash(const char *prefix, struct stash_info *info, int 
>> index)
>> +{
>> + struct merge_options o;
>> + struct object_id c_tree;
>> + struct object_id index_tree;
>> + const struct object_id *bases[1];
>> + int bases_count = 1;
>> + struct commit *result;
>> + int ret;
>> + int has_index = index;
>> +
>> + read_cache_preload(NULL);
>> + if (refresh_cache(REFRESH_QUIET))
>> + return -1;
>> +
>> + if (write_cache_as_tree(_tree, 0, NULL) || reset_tree(_tree, 0, 0))
>
> When applied on top of current `master`, I need to replace the _tree by
> c_tree.hash.
>
> Likewise...
>

I based this revision off next because of the object_id changes. I
probably should have mentioned in my cover-letter.

>> [...]
>> +
>> + index_file = get_index_file();
>> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%d", index_file, pid);
>
> Since `pid_t` is `unsigned long long` on Windows, I changed the %d" to
> %"PRIuMAX and cast `pid` to `(uintmax_t)`.

Thanks for testing on windows! I'll have that fixed in the next revision.

Re: [PATCH v2 00/36] Combine t/helper binaries into a single one

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Mar 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This iteration, with the SQUASH??? I proposed (and that Junio will
> > hopefully pick up soon), works well on Windows.
> 
> Thanks; is that the "call it fn, as main is macro-ed away by us?"
> change?

Precisely.

Thanks,
Dscho

Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader

[Jeff King]

On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:

> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
> > In order to allow for better control flow when protocol_v2 is introduced
> > +static enum protocol_version discover_version(struct packet_reader *reader)
> > +{
> > +   enum protocol_version version = protocol_unknown_version;
> > +
> > +   /*
> > +* Peek the first line of the server's response to
> > +* determine the protocol version the server is speaking.
> > +*/
> > +   switch (packet_reader_peek(reader)) {
> > +   case PACKET_READ_EOF:
> > +   die_initial_contact(0);
> > +   case PACKET_READ_FLUSH:
> 
> gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
> at least gcc 7.x), it fails to realize that this die_initial_contact()
> will not fall through (even though we do tell it about die() not
> returning, but I guess that involves more flow analysis to realize
> die_initial_contact is in the same boat).
> [...]
> @@ -124,6 +124,7 @@ enum protocol_version discover_version(struct 
> packet_reader *reader)
>   switch (packet_reader_peek(reader)) {
>   case PACKET_READ_EOF:
>   die_initial_contact(0);
> + break;

Would it make sense just to annotate that function to help the flow
analysis? Like:

diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
if (unexpected)
die(_("The remote end hung up upon initial contact"));

That should let the callers know what's going on, and inside the
function itself, the compiler should confirm that all code paths hit
another NORETURN function.

-Peff

Re: [PATCH v3 2/3] fast-import: introduce mem_pool type

[Junio C Hamano]

Jameson Miller  writes:

> Introduce the mem_pool type which encapsulates all the information
> necessary to manage a pool of memory.This change moves the existing
> variables in fast-import used to support the global memory pool to use
> this structure.
>
> These changes allow for the multiple instances of a memory pool to
> exist and be reused outside of fast-import. In a future commit the
> mem_pool type will be moved to its own file.
>
> Signed-off-by: Jameson Miller 
> ---
>  fast-import.c | 80 
> +--
>  1 file changed, 51 insertions(+), 29 deletions(-)

Thanks, this got much easier to read and reason about.

> @@ -304,9 +317,7 @@ static int global_argc;
>  static const char **global_argv;
>  
>  /* Memory pools */
> -static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mp_block);
> -static size_t total_allocd;
> -static struct mp_block *mp_block_head;
> +static struct mem_pool fi_mem_pool =  {0, 2*1024*1024 - sizeof(struct 
> mp_block), 0 };
>  
>  /* Atom management */
>  static unsigned int atom_table_sz = 4451;
> @@ -324,6 +335,7 @@ static off_t pack_size;
>  /* Table of objects we've written. */
>  static unsigned int object_entry_alloc = 5000;
>  static struct object_entry_pool *blocks;
> +static size_t total_allocd;

So the design decision made at this step is that pool_alloc field
keeps track of the per-pool allocation, while total_allocd is a sum
across instances of pools.  That sounds appropriate for stats.

> @@ -634,7 +646,21 @@ static unsigned int hc_str(const char *s, size_t len)
>   return r;
>  }
>  
> -static void *pool_alloc(size_t len)
> +static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool, 
> size_t block_alloc)
> +{
> + struct mp_block *p;
> +
> + mem_pool->pool_alloc += sizeof(struct mp_block) + block_alloc;
> + p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
> + p->next_block = mem_pool->mp_block;
> + p->next_free = (char *)p->space;
> + p->end = p->next_free + block_alloc;
> + mem_pool->mp_block = p;

This, compared to what used to happen in mem_pool_alloc()'s original
code, ignores total_allocd.  I cannot tell if that is an intentional
change or a mistake.

> +
> + return p;
> +}
> +
> +static void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
>  {
>   struct mp_block *p;
>   void *r;
> @@ -643,21 +669,17 @@ static void *pool_alloc(size_t len)
>   if (len & (sizeof(uintmax_t) - 1))
>   len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
>  
> - for (p = mp_block_head; p; p = p->next_block)
> - if ((p->end - p->next_free >= len))
> - break;
> + for (p = mem_pool->mp_block; p; p = p->next_block)
> + if (p->end - p->next_free >= len)
> + break;
>  
>   if (!p) {
> - if (len >= (mem_pool_alloc/2)) {
> - total_allocd += len;
> + if (len >= (mem_pool->block_alloc / 2)) {
> + mem_pool->pool_alloc += len;
>   return xmalloc(len);

It is unfair to account this piece of memory to the mem_pool, as we
ended up not carving it out from here.  Did you mean to increment
total_allocd by len instead, perhaps?

And I do agree with the idea in the previous round to make these
oversized pieces of memory allocated here to be freeable via the
mem_pool instance (I only found it questionable to use the main
"here are the list of blocks that we could carve small pieces out"
list), and anticipate that a later step in the series would change
this part to do so.  With that anticipation, I very much appreciate
that this step did not mix that and stayed as close to the original
(except for the possible mis-accounting).  It makes it very clear
what is going on in each separate step in the series.

>   }
> - total_allocd += sizeof(struct mp_block) + mem_pool_alloc;

This is what I noticed got lost in the pool-alloc-block helper above.

> - p = xmalloc(st_add(sizeof(struct mp_block), mem_pool_alloc));
> - p->next_block = mp_block_head;
> - p->next_free = (char *) p->space;
> - p->end = p->next_free + mem_pool_alloc;
> - mp_block_head = p;
> +
> + p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
>   }
>  
>   r = p->next_free;
> @@ -665,10 +687,10 @@ static void *pool_alloc(size_t len)
>   return r;
>  }
>  
> -static void *pool_calloc(size_t count, size_t size)
> +static void *mem_pool_calloc(struct mem_pool *mem_pool, size_t count, size_t 
> size)
>  {
> - size_t len = count * size;
> - void *r = pool_alloc(len);
> + size_t len = st_mult(count, size);

Nice ;-)

> + void *r = mem_pool_alloc(mem_pool, len);
>   memset(r, 0, len);
>   return r;
>  }

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Daniel Jacques]

On Tue, Mar 27, 2018 at 11:54 AM Johannes Schindelin <
johannes.schinde...@gmx.de> wrote:

> Yes, I performed manual testing.

Alright! Just manually tested your "git" scenario myself on the Linux build
and all seems to be in order.

> I guess we should add a test where we copy the `git` executable into a
> subdirectory with the name "git" and call `git/git --exec-path` and verify
> that its output matches our expectation?

I'm actually a little fuzzy on the testing model here. As things are, this
test will only work if Git is relocatable; however, the test suite doesn't
seem to be equipped to build multiple versions of Git for different tests.
 From this I conclude that the right approach would be to make a test that
runs conditional on RUNTIME_PREFIX being set, but I'm not familiar enough
with the testing framework to be confident that this is correct, or really
how to go about writing such a test.

A simple grep suggests that the current test suite doesn't seem to have any
RUNTIME_PREFIX-specific tests. When I've been running the test suites, I've
been doing it with a "config.mak" file that explicitly enables
RUNTIME_PREFIX to get the runtime prefix code tested against the standard
Git testing suites.

 From a Git maintainer's perspective, would such a test be a prerequisite
for landing this patch series, or is this a good candidate for follow-up
work to improve our testing coverage?

-Dan

Re: [PATCH v3 2/5] stash: convert apply to builtin

[Johannes Schindelin]

Hi Joel,

On Mon, 26 Mar 2018, Joel Teichroeb wrote:

> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being
> finished.
> 
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
> 
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
> 
> Signed-off-by: Joel Teichroeb 

Makes sense.

I need a couple of adjustments before it compiles on Windows:

> [...]
> +
> +static int do_apply_stash(const char *prefix, struct stash_info *info, int 
> index)
> +{
> + struct merge_options o;
> + struct object_id c_tree;
> + struct object_id index_tree;
> + const struct object_id *bases[1];
> + int bases_count = 1;
> + struct commit *result;
> + int ret;
> + int has_index = index;
> +
> + read_cache_preload(NULL);
> + if (refresh_cache(REFRESH_QUIET))
> + return -1;
> +
> + if (write_cache_as_tree(_tree, 0, NULL) || reset_tree(_tree, 0, 0))

When applied on top of current `master`, I need to replace the _tree by
c_tree.hash.

Likewise...

> + return error(_("Cannot apply a stash in the middle of a 
> merge"));
> +
> + if (index) {
> + if (!oidcmp(>b_tree, >i_tree) || !oidcmp(_tree, 
> >i_tree)) {
> + has_index = 0;
> + } else {
> + struct strbuf out = STRBUF_INIT;
> +
> + if (diff_tree_binary(, >w_commit)) {
> + strbuf_release();
> + return -1;
> + }
> +
> + ret = apply_cached();
> + strbuf_release();
> + if (ret)
> + return -1;
> +
> + discard_cache();
> + read_cache();
> + if (write_cache_as_tree(_tree, 0, NULL))

... _tree -> index_tree.hash.

These are probably changed to use object_id's already in `pu`, I guess.

I also need this change:

> [...]
> +
> + index_file = get_index_file();
> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%d", index_file, pid);

Since `pid_t` is `unsigned long long` on Windows, I changed the %d" to
%"PRIuMAX and cast `pid` to `(uintmax_t)`.

With those changes, the entire patch series compiles here.

BTW t3903 runs in 13m30s here with this patch series, 14m30s otherwise.
That might not seem like much, until you realize that t3903 *still*
performs a metric ton of Unix shell scripting outside of `git stash` (and
that is the reason for the slowness).

Ciao,
Dscho

Re: Windows build on Travis CI (was: Re: [PATCH v2 01/36] t/helper: add an empty test-tool program)

[Johannes Schindelin]

Hi Gábor,

On Tue, 27 Mar 2018, SZEDER Gábor wrote:

> On Tue, Mar 27, 2018 at 3:57 PM, Johannes Schindelin
>  wrote:
> >
> > On Tue, 27 Mar 2018, SZEDER Gábor wrote:
> >
> >> On Tue, Mar 27, 2018 at 12:14 AM, Johannes Schindelin
> >>  wrote:
> >> > However, it seems that something is off, as
> >> > ba5bec9589e9eefe2446044657963e25b7c8d88e is reported as fine on Windows:
> >> > https://travis-ci.org/git/git/jobs/358260023 (while there is clearly a 
> >> > red
> >> > X next to that commit in
> >> > https://github.com/git/git/commits/ba5bec9589e9eefe2446044657963e25b7c8d88e,
> >> > that X is due to a hiccup on macOS).
> >> >
> >> > It seems that the good-trees feature for Travis does not quite work as
> >> > intended. Gábor?
> >>
> >> AFAICT it works as expected.
> >>
> >> When a build job encounters a commit with a tree that has previously
> >> been built and tested successfully, then first it says so, like this:
> >>
> >>   https://travis-ci.org/szeder/git/jobs/347295038#L635
> >
> > But what if it has not been built successfully (as was the case here)?
> > This very commit that is "succeeding" on Travis fails to compile on
> > Windows.
> 
> Then why has the GfW web app reported success?
> 
>   https://travis-ci.org/git/git/jobs/358260023#L512

Oy. There was a shift in build steps, so that shows the wrong output. The
correct build step ends thusly:

-- snip --
[...]
2018-03-26T06:50:55.371Z Checking out files:  97% (3136/3232)   
2018-03-26T06:50:55.0106984Z Checking out files:  98% (3168/3232)   
2018-03-26T06:50:55.0223806Z Checking out files:  99% (3200/3232)   
2018-03-26T06:50:55.0227819Z Checking out files: 100% (3232/3232)   
2018-03-26T06:50:55.0228191Z Checking out files: 100% (3232/3232), done.
2018-03-26T06:50:55.0343621Z HEAD is now at 90bbd502d Sync with Git 2.16.3
2018-03-26T06:50:55.0759061Z Updating upstream
2018-03-26T06:50:56.3001946Z From https://github.com/git/git
2018-03-26T06:50:56.3002737Z  * [new branch]  maint  -> 
upstream/maint
2018-03-26T06:50:56.3003056Z  * [new branch]  master -> 
upstream/master
2018-03-26T06:50:56.3003832Z  * [new branch]  next   -> 
upstream/next
2018-03-26T06:50:56.3354328Z  * [new branch]  pu -> upstream/pu
2018-03-26T06:50:56.3354880Z  * [new branch]  todo   -> 
upstream/todo
2018-03-26T06:50:56.8219992Z fatal: Not a valid commit name 
7a6a7fb7d0ab1052db113318478f9e40e66e59dc
2018-03-26T06:50:56.8236547Z Commit 7a6a7fb7d0ab1052db113318478f9e40e66e59dc is 
not on branch upstream/master; skipping
```

So as you see, by the time we fetched `pu`, it was no longer reachable
(otherwise we would have been able to fetch it).

That's a bummer.

Ciao,
Dscho

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Johannes Schindelin]

Hi Dan,

On Tue, 27 Mar 2018, Daniel Jacques wrote:

> On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
> johannes.schinde...@gmx.de> wrote:
> 
> > Even if the RUNTIME_PREFIX feature originates from Git for Windows, the
> > current patch series is different enough in its design that it leaves the
> > Windows-specific RUNTIME_PREFIX handling in place: On Windows, we still
> > have to override argv[0] with the absolute path of the current `git`
> > executable.
> 
> > Let's just port the Windows-specific code over to the new design and get
> > rid of that argv[0] overwriting.
> 
> > This also partially addresses a very obscure problem reported on the Git
> > for Windows bug tracker, where misspelling a builtin command using a
> > creative mIxEd-CaSe version could lead to an infinite ping-pong between
> > git.exe and Git for Windows' "Git wrapper" (that we use in place of
> > copies when on a file system without hard-links, most notably FAT).
> 
> > Dan, I would be delighted if you could adopt these patches into your patch
> > series.
> 
> Great, I'm glad this patch set could be useful to you! I'm happy to apply
> this to the patch series. They applied cleanly, so I'll push a new version
> after Travis validates the candidate.
> 
> I don't have a Windows testing facility available, so I'm hoping that you
> verified that this works locally. I suppose that's what the unstable branch
> series is for.

Yes, I performed manual testing.

I guess we should add a test where we copy the `git` executable into a
subdirectory with the name "git" and call `git/git --exec-path` and verify
that its output matches our expectation?

Ciao,
Dscho

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Ramsay Jones]

On 27/03/18 04:18, Ramsay Jones wrote:
> On 26/03/18 15:31, g...@jeffhostetler.com wrote:
>> From: Jeff Hostetler 
>>
> [snip]
> 
> Thanks, this version fixes all issues I had (with the compilation
> and sparse warnings).
> 
> [Was using UINT64_C(0x) a problem on windows?]

BTW, I forgot to mention that you had some whitespace problems
with this patch, viz:

  $ git log --oneline -1 --check master-json
  ab643d838 (master-json) json_writer: new routines to create data in JSON 
format
  t/helper/test-json-writer.c:280: trailing whitespace.
  + */ 
  t/t0019-json-writer.sh:179: indent with spaces.
  +"g": 0,
  t/t0019-json-writer.sh:180: indent with spaces.
  +"h": 1
  $ 

ATB,
Ramsay Jones

Re: [PATCH v2 00/36] Combine t/helper binaries into a single one

[Junio C Hamano]

Johannes Schindelin  writes:

> This iteration, with the SQUASH??? I proposed (and that Junio will
> hopefully pick up soon), works well on Windows.

Thanks; is that the "call it fn, as main is macro-ed away by us?"
change?

Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader

[Duy Nguyen]

On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
> In order to allow for better control flow when protocol_v2 is introduced
> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
> + enum protocol_version version = protocol_unknown_version;
> +
> + /*
> +  * Peek the first line of the server's response to
> +  * determine the protocol version the server is speaking.
> +  */
> + switch (packet_reader_peek(reader)) {
> + case PACKET_READ_EOF:
> + die_initial_contact(0);
> + case PACKET_READ_FLUSH:

gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on
at least gcc 7.x), it fails to realize that this die_initial_contact()
will not fall through (even though we do tell it about die() not
returning, but I guess that involves more flow analysis to realize
die_initial_contact is in the same boat).

Since -Wimplicit-fallthrough may be useful to spot bugs elsewhere and
there are only two places in this series that tick it off, is it
possible to squash in something like this? On the off chance that we
fail horribly to die, "break;" would stop the wrong code from
executing even more.

This covers another patch too, but you get the idea.

diff --git a/connect.c b/connect.c
index 54971166ac..847bf2f7d6 100644
--- a/connect.c
+++ b/connect.c
@@ -124,6 +124,7 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
switch (packet_reader_peek(reader)) {
case PACKET_READ_EOF:
die_initial_contact(0);
+   break;
case PACKET_READ_FLUSH:
case PACKET_READ_DELIM:
version = protocol_v0;
@@ -303,6 +304,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,
switch (packet_reader_read(reader)) {
case PACKET_READ_EOF:
die_initial_contact(1);
+   break;
case PACKET_READ_NORMAL:
len = reader->pktlen;
if (len > 4 && skip_prefix(reader->line, "ERR ", ))

--
Duy

Re: [PATCH v3 2/5] stash: convert apply to builtin

[Johannes Schindelin]

Hi Joel,

On Mon, 26 Mar 2018, Joel Teichroeb wrote:

> Add a bulitin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> let conversion get started without the other command being
> finished.
> 
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
> 
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.
> 
> Signed-off-by: Joel Teichroeb 

Very good!

In the interest of as incremental a change as possible, I would wager a
bet that this is the best way we can go about it, later replacing the
parts that still spawn Git processes (such as get_symbolic_name and
have_stash) with direct calls into libgit.a, one by one.

Thank you!
Dscho

Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

[Duy Nguyen]

On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> The set of extra warnings we enable when DEVELOPER has to be
>> conservative because we can't assume any compiler version the
>> developer may use. Detect the compiler version so we know when it's
>> safe to enable -Wextra and maybe more.
>
> This is a good idea in general, but we are not quite ready without
> some fixups.
>
> Here is a quick summary (not exhaustive) from my trial merge to 'pu'
> (which will be reverted before today's integration is pushed out).
>
>  - json-writer.c triggers -Werror=old-style-decl
>
>  - t/helper/test-json-writer.c triggers Werror=missing-prototypes
>quite a few times.

I expected these (and it was a good reason to push this patch
forward). I think people also reported style problems with this
series.

>
>  - connect.c -Werror=implicit-fallthough around die_initial_contact().
>

This I did not expect. But I just looked again and I had this option
explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not
complain about this. gcc-7.3 does. What's your compiler/version?


-- 
Duy

Windows build on Travis CI (was: Re: [PATCH v2 01/36] t/helper: add an empty test-tool program)

[SZEDER Gábor]

On Tue, Mar 27, 2018 at 3:57 PM, Johannes Schindelin
 wrote:
> Hi Gábor,
>
> On Tue, 27 Mar 2018, SZEDER Gábor wrote:
>
>> On Tue, Mar 27, 2018 at 12:14 AM, Johannes Schindelin
>>  wrote:
>> > However, it seems that something is off, as
>> > ba5bec9589e9eefe2446044657963e25b7c8d88e is reported as fine on Windows:
>> > https://travis-ci.org/git/git/jobs/358260023 (while there is clearly a red
>> > X next to that commit in
>> > https://github.com/git/git/commits/ba5bec9589e9eefe2446044657963e25b7c8d88e,
>> > that X is due to a hiccup on macOS).
>> >
>> > It seems that the good-trees feature for Travis does not quite work as
>> > intended. Gábor?
>>
>> AFAICT it works as expected.
>>
>> When a build job encounters a commit with a tree that has previously
>> been built and tested successfully, then first it says so, like this:
>>
>>   https://travis-ci.org/szeder/git/jobs/347295038#L635
>
> But what if it has not been built successfully (as was the case here)?
> This very commit that is "succeeding" on Travis fails to compile on
> Windows.

Then why has the GfW web app reported success?

  https://travis-ci.org/git/git/jobs/358260023#L512

>> and then skips the rest of the build job (see the 'exit 0' a few lines
>> later).
>>
>> In case of this Windows build job we haven't seen this tree yet:
>>
>>   https://travis-ci.org/git/git/jobs/358260023#L467
>>
>> so the build job continues as usual (see the 'test -z Windows' two lines
>> later).
>>
>> Unfortunately, I have no idea about how the rest of the Windows build
>> job is supposed to work...
>
> Maybe Travis timed out waiting for the result, and marked it as a success?

This Windows build ran for 9 min 27 sec, i.e. not long enough for a
timeout on Travis CI.  (OTOH, clearly not long enough to build Git and
run the test suite on Windows, I know.)

BTW, a timeouted build job is marked as "errored" and the timeout is
mentioned in its trace log:

  https://travis-ci.org/git/git/jobs/331669291#L509

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

On Tue, Mar 27, 2018 at 8:31 AM, Jeff King  wrote:
>...
>
> But that really feels like we're papering over the problem.

It is papering over the problem in my opinion. setup_work_tree() is
involved here and when it moves cwd around, it re-init git-dir (and
all other paths). Before my patch we call git_path() only when we need
it and git-dir is likely already updated by setup_work_tree(). After
this patch, the path is set in stone before setup_work_tree() kicks
in. Once it moves cwd, relative paths all become invalid.

The way setup_work_tree() does it is just bad to me. When it calls
set_git_dir() again, the assumption is the new path is exactly the
same as the old one. The only difference is relative vs absolute. But
this is super hard to see from set_git_dir implementation. The new
struct repository design also inherits this (i.e. allowing to call
set_git_dir multiple times, which really does not make sense), but
this won't fly long term. When cwd moves, all cached relative paths
must be adjusted, we need a better mechanism to tell everybody that,
not just do it for $GIT_DIR and related paths.

I am planning to fix this. This is part of the "setup cleanup" effort
to keep repository.c design less messy and hopefully unify how the
main repo and submodule repo are created/set up. But frankly that may
take a long while before I could submit anything substantial (I also
have the "report multiple worktree's HEADs correctly and make fsck
count all HEADs" task, which is not small and to me have higher
priority).

So I would not mind papering over it right now (with an understanding
that absolute path pays some more overhead in path walking, which was
th reason we tried to avoid it in setup code). A slightly better patch
is trigger this path absolutization from setup_work_tree(), near
set_git_dir(). But then you face some ugliness there: how to find out
all ref stores to update, or just update the main ref store alone.

> It's not
> clear to me exactly what f57f37e2 is trying to accomplish, and whether
> it would work for it to look call get_git_dir() whenever it needed the
> path.
>
> -Peff
-- 
Duy

Re: [PATCH v3 0/5] Convert some stash functionality to a builtin

[Johannes Schindelin]

Hi Joel,

On Mon, 26 Mar 2018, Joel Teichroeb wrote:

> I've been working on converting all of git stash to be a
> builtin, however it's hard to get it all working at once with
> limited time, so I've moved around half of it to a new
> stash--helper builtin and called these functions from the shell
> script. Once this is stabalized, it should be easier to convert
> the rest of the commands one at a time without breaking
> anything.
> 
> I've sent most of this code before, but that was targetting a
> full replacement of stash. The code is overall the same, but
> with some code review changes and updates for internal api
> changes.
> 
> Since there seems to be interest from GSOC students who want to
> work on converting builtins, I figured I should finish what I
> have that works now so they could build on top of it.

Great! This will help tremendously, I am sure.

Ciao,
Dscho

Re: [PATCH 00/10] Hash-independent tests (part 1)

[Johannes Schindelin]

Hi Junio,

On Sun, 25 Mar 2018, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > What's the plan for oddball cases such as 66ae9a57b8 (t3404: rebase
> > -i: demonstrate short SHA-1 collision, 2013-08-23) which depend
> > implicitly upon SHA-1 without actually hardcoding any hashes? The test
> > added by 66ae9a57b8, for instance, won't start failing in the face of
> > NewHash, but it also won't be testing anything meaningful.
> >
> > Should such tests be dropped altogether? Should they be marked with a
> > 'SHA1' predicate or be annotated with a comment as being
> > SHA-1-specific? Something else?
> 
> Ideally, the existing one be annotated with prereq SHA1, and also
> duplicated with a tweak to cause the same kind of (half-)collision
> under the NewHash and be annotated with prereq NewHash.

That's a good idea. I wonder whether we want to be a bit more specific,
though, so that we have something grep'able for the future? Something like
SHA1_SHORT_COLLISION or some such?

> It's a different matter how feasible it is to attain such an ideal,
> though.  t1512 was fun to write, but it was quite a lot of work to
> come up with bunch of blobs, trees and commits whose object names
> share the common prefix 0{10}.

Did you write a helper to brute-force those? If so, we might want to have
such a helper in t/helper/ to generate/re-generate those (i.e. it could be
asked to generate a blob whose ID starts with five NUL bytes, and it would
have hard-coded values for already-generated ones). Even if you did not
write such a helper, we might want to have such a helper. That would take
the responsibility away from the shell script.

Ciao,
Dscho

Re: [PATCH 0/2] Add Windows support to the new RUNTIME_PREFIX design

[Daniel Jacques]

On Mon, Mar 26, 2018 at 5:31 PM Johannes Schindelin <
johannes.schinde...@gmx.de> wrote:

> Even if the RUNTIME_PREFIX feature originates from Git for Windows, the
> current patch series is different enough in its design that it leaves the
> Windows-specific RUNTIME_PREFIX handling in place: On Windows, we still
> have to override argv[0] with the absolute path of the current `git`
> executable.

> Let's just port the Windows-specific code over to the new design and get
> rid of that argv[0] overwriting.

> This also partially addresses a very obscure problem reported on the Git
> for Windows bug tracker, where misspelling a builtin command using a
> creative mIxEd-CaSe version could lead to an infinite ping-pong between
> git.exe and Git for Windows' "Git wrapper" (that we use in place of
> copies when on a file system without hard-links, most notably FAT).

> Dan, I would be delighted if you could adopt these patches into your patch
> series.

Great, I'm glad this patch set could be useful to you! I'm happy to apply
this to the patch series. They applied cleanly, so I'll push a new version
after Travis validates the candidate.

I don't have a Windows testing facility available, so I'm hoping that you
verified that this works locally. I suppose that's what the unstable branch
series is for.

Re: [PATCH 00/10] Hash-independent tests (part 1)

[Johannes Schindelin]

Hi Brian,

On Sun, 25 Mar 2018, brian m. carlson wrote:

> This is a series to make our tests hash-independent.  Many tests have
> hard-coded SHA-1 values in them, and it would be valuable to express
> these items in a hash-independent way for our hash transitions.
> 
> The approach in this series relies on only three components for hash
> independence: git rev-parse, git hash-object, and EMPTY_BLOB and
> EMPTY_TREE.  Because many of our shell scripts and test components
> already rely on the first two, this seems like a safe assumption.
> 
> For the same reason, this series avoids modifying tests that test these
> components or their expected SHA-1 values.  I expect that when we add
> another hash function, we'll copy these tests to expose both SHA-1 and
> NewHash versions.
> 
> Many of our tests use heredocs for defining expected values.  My
> approach has been to interpolate values into the heredocs, as that
> produces the best readability in my view.
> 
> These tests have been tested using my "short BLAKE2b" series (branch
> blake2b-test-hash) and have also been tested based off master.
> 
> Comments on any aspect of this series are welcome, but opinions on the
> approach or style are especially so.

Thank you for this patch series!

I reviewed all 10 patches, and while I cannot say anything about whether
they miss any spot, they all look sensible and correct.

Thanks,
Dscho

Re: [PATCH v2 00/36] Combine t/helper binaries into a single one

[Johannes Schindelin]

Hi Duy,

On Sat, 24 Mar 2018, Nguyễn Thái Ngọc Duy wrote:

> v2 fixes a couple of typos in commit messages and use the cmd__ prefix
> for test commands instead of test_, which avoids a naming conflict
> with the existing function test_lazy_init_name_hash
> 
> [the previous v2 send out was aborted because I messed it up with some
> other patches]

This iteration, with the SQUASH??? I proposed (and that Junio will
hopefully pick up soon), works well on Windows.

Thank you,
Dscho

Re: [PATCH v2 01/36] t/helper: add an empty test-tool program

[Johannes Schindelin]

Hi Gábor,

On Tue, 27 Mar 2018, SZEDER Gábor wrote:

> On Tue, Mar 27, 2018 at 12:14 AM, Johannes Schindelin
>  wrote:
> > However, it seems that something is off, as
> > ba5bec9589e9eefe2446044657963e25b7c8d88e is reported as fine on Windows:
> > https://travis-ci.org/git/git/jobs/358260023 (while there is clearly a red
> > X next to that commit in
> > https://github.com/git/git/commits/ba5bec9589e9eefe2446044657963e25b7c8d88e,
> > that X is due to a hiccup on macOS).
> >
> > It seems that the good-trees feature for Travis does not quite work as
> > intended. Gábor?
> 
> AFAICT it works as expected.
> 
> When a build job encounters a commit with a tree that has previously
> been built and tested successfully, then first it says so, like this:
> 
>   https://travis-ci.org/szeder/git/jobs/347295038#L635

But what if it has not been built successfully (as was the case here)?
This very commit that is "succeeding" on Travis fails to compile on
Windows.

> and then skips the rest of the build job (see the 'exit 0' a few lines
> later).
> 
> In case of this Windows build job we haven't seen this tree yet:
> 
>   https://travis-ci.org/git/git/jobs/358260023#L467
> 
> so the build job continues as usual (see the 'test -z Windows' two lines
> later).
> 
> Unfortunately, I have no idea about how the rest of the Windows build
> job is supposed to work...

Maybe Travis timed out waiting for the result, and marked it as a success?

Ciao,
Dscho

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Johannes Schindelin]

Hi Sergey,

On Tue, 27 Mar 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> > On Tue, 13 Mar 2018, Igor Djordjevic wrote:
> >
> >> On 12/03/2018 11:46, Johannes Schindelin wrote:
> >> > 
> >> > > Sometimes one just needs to read the manual, and I don`t really
> >> > > think this is a ton complicated, but just something we didn`t
> >> > > really have before (real merge rebasing), so it requires a moment
> >> > > to grasp the concept.
> >> > 
> >> > If that were the case, we would not keep getting bug reports about
> >> > --preserve-merges failing to reorder patches.
> >> 
> >> Not sure where that is heading to, but what I`m arguing about is that
> >> introducing new commands and concepts (`merge`, and with `-R`) just
> >> makes the situation even worse (more stuff to grasp).
> >
> > The problem with re-using `pick` is that its concept does not apply to
> > merges. The cherry-pick of a non-merge commit is well-defined: the
> > current HEAD is implicitly chosen as the cherry-picked commit's
> > (single) parent commit. There is no ambiguity here.
> >
> > But for merge commits, we need to specify the parent commits (apart
> > from the first one) *explicitly*. There was no need for that in the
> > `pick` command, nor in the concept of a cherry-pick.
> >
> >> Reusing existing concepts where possible doesn`t have this problem.
> >
> > Existing concepts are great. As long as they fit the requirements of
> > the new scenarios. In this case, `pick` does *not* fit the requirement
> > of "rebase a merge commit".
> 
> It does, provided you use suitable syntax.

You know what `pick` would also do, provided you use suitable syntax? Pick
your nose.

Don't blame me for this ridiculous turn the discussion took.

Of course, using the suitable syntax you can do anything. Unless there is
*already* a syntax and you cannot break it for backwards-compatibility
reasons, as is the case here.

But I'll stop here. Even my account how there are conceptual differences
between the changes in merge vs non-merge commits (the non-merge commit
*introduces* changes, the merge commit *reconciles existing* changes)
seems to fly by without convincing you.

I use rebase every day. I use the Git garden shears every week. If you do
not trust my experience with these things, nothing will convince you. You
are just stuck with your pre-existing opinion.

> > If you really want to force the `pick` concept onto the use case where
> > you need to "reapply" merges, then the closest you get really is
> > Sergey's idea, which I came to reject when considering its practical
> > implications.
> 
> Which one, and what are the implications that are bad, I wonder?

The strategy described in RFC v2, which does too much work, forces the
user to potentially address the same merge conflicts multiple times, and
worst of all: risks merge conflicts with changes the user *already*
dropped.

> > Even so, you would have to make the `pick` command more complicated to
> > support merge commits. And whatever you would do to extend the `pick`
> > command would *not make any sense* to the current use case of the `pick`
> > command.
> 
> It would rather make a lot of sense. Please don't use 'merge' to pick
> commits, merge ones or not!

It would rather make a lot of sense. If you completely ignored everything
I said about preserve-merges. If you ignored what I said about problems
moving regular `pick` lines across merge commits. If you ignored all the
experience I have with Git garden shears and that I tried really patiently
for an impatient man to impart on you.

> > The real problem, of course, is that a non-merge commit, when viewed
> > from the perspective of the changes it introduced, is a very different
> > beast than a merge commit: it does not need to reconcile changes,
> > ever, because there is really only one "patch" to one revision. That
> > is very different from a merge commit, whose changes can even disagree
> > with one another (and in fact be resolved with changes disagreeing
> > *yet again*)!
> 
> You'd still 'pick' it though, not 'merge'. You don't merge "merge
> commit", it makes no sense. It only makes perfect sense when you get rid
> of original "merge commit" and re-merge from scratch, as you were doing
> till now.

No, you merge "merge head". And you use "merge commit"'s commit message.
*That* makes sense.

Picking a merge commit? Not so. What do you merge? The original merge
commit's second parent? Or a rebased version thereof? What if that commit
has been `pick`ed *twice*?

No, you can repeat it all you want, it still does not make sense. Now that
I think of the possiblity of picking the original parents multiple times,
it does not even make theoretical sense.

> > The implementation detail is, of course, that I will introduce this with
> > the technically-simpler strategy: always recreating merge commits with the
> > recursive strategy. A follow-up patch series will add support for rebasing
> > merge 

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Johannes Schindelin]

Hi Sergey,

On Tue, 27 Mar 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> >
> > On Tue, 13 Mar 2018, Igor Djordjevic wrote:
> >
> >> On 12/03/2018 13:56, Sergey Organov wrote:
> >> > 
> >> > > > I agree with both of you that `pick ` is inflexible
> >> > > > (not to say just plain wrong), but I never thought about it like
> >> > > > that.
> >> > > >
> >> > > > If we are to extract further mentioned explicit old:new merge
> >> > > > parameter mapping to a separate discussion point, what we`re
> >> > > > eventually left with is just replacing this:
> >> > > >
> >> > > >  merge -R -C  
> >> > > >
> >> > > > ... with this:
> >> > > >
> >> > > >  pick  
> >> > >
> >> > > I see where you are coming from.
> >> > >
> >> > > I also see where users will be coming from. Reading a todo list in
> >> > > the editor is as much documentation as it is a "program to execute".
> >> > > And I am afraid that reading a command without even mentioning the
> >> > > term "merge" once is pretty misleading in this setting.
> >> > >
> >> > > And even from the theoretical point of view: cherry-picking
> >> > > non-merge commits is *so much different* from "rebasing merge
> >> > > commits" as discussed here, so much so that using the same command
> >> > > would be even more misleading.
> >> > 
> >> > This last statement is plain wrong when applied to the method in the
> >> > [RFC] you are replying to.
> >
> > That is only because the RFC seems to go out of its way to break down a
> > single merge commit into as many commits as there are merge commit
> > parents.
> 
> Complex entity is being split for ease of reasoning. People tend to use
> this often.

Sure. Divide and conquer. Not duplicate and complicate, though.

> > This is a pretty convoluted way to think about it: if you have three
> > parent commits, for example, that way of thinking would introduce three
> > intermediate commits, one with the changes of parent 2 & 3 combined, one
> > with the changes of parent 1 & 3 combined, and one with the changes of
> > parent 1 & 2 combined.
> 
> No.

Sorry. This is unacceptable. If you disagree, sure, you are free to do
that. If you want to contribute to a fruitful discussion, just saying "No"
without explaining why you *think* that my statement is wrong is just...
unconstructive.

> > To rebase those commits, you essentially have to rebase *every
> > parent's changes twice*.
> 
> No.

Same here.

> > It gets worse with merge commits that have 4 parents. In that case, you
> > have to rebase every parent's changes *three times*.
> 
> Sorry, the [RFC] has nothing of the above. Once again, it's still just
> as simple is: rebase every side of the merge then merge the results
> using the original merge commit as a merge base.
> 
> And if you can't or don't want to grok the explanation in the RFC, just
> forget the explanation, no problem.

Your RFC talks about U1 and U2, for the two merge parents.

Obviously this strategy can be generalized to n parents. I thought you had
thought of that and simply did not bother to talk about it.

Sorry, my mistake. I should not assume so much.

> > And so on.
> >
> >> > Using the method in [RFC], "cherry-pick non-merge" is nothing more or
> >> > less than reduced version of generic "cherry-pick merge", exactly as
> >> > it should be.
> >
> > I really get the impression that you reject Phillip's proposal on the
> > ground of not being yours. In other words, the purpose of this here
> > argument is to praise one proposal because of its heritage, rather than
> > trying to come up with the best solution.
> 
> No. As the discussion evolved, I inclined to conclusion that modified
> Phillip's algorithm is actually better suited for the implementation
> [1].

Again a link.

If that's what you are looking for, I will throw a hundred links your way
and see how constructive a discussion you find that.

> > On that basis, I will go with the proposal that is clearly the simplest
> > and does the job and gets away with avoiding unnecessary work.
> 
> These algorithms are actually the same one, as has already been shown
> elsewhere in the discussion.

I disproved that already. My example showed that instead of reconciling
the diverging changes starting from the original merge parents, RFC v2
tries to rebase those parents first, and then use the original merge
commit as base of "diverging changes" that never started from that
original merge commit.

Essentially, where Phillip's strategy imitates a cherry-pick's 3-way
merge, your strategy tries to rebase the merge tips independently from the
user (who already rebased them, thank you very much), and then runs a
*revert*: while a cherry-pick uses the picked commit's parent as merge
base, a revert uses the to-be-reverted commit itself as merge base.

In short: Phillip's strategy is only equivalent to yours if you ignore the
fact that you perform unnecessary work only to undo it in the end.

> Asymmetric incremental nature of 

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Johannes Schindelin]

Hi Sergey,

On Tue, 27 Mar 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> > On Mon, 12 Mar 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  writes:
> >> >
> >> > On Wed, 7 Mar 2018, Sergey Organov wrote:
> >> >
> >> >> Johannes Schindelin  writes:
> >> >> 
> >> >> > How can your approach -- which relies *very much* on having the
> >> >> > original parent commits -- not *require* that consistency check?
> >> >> 
> >> >> I don't understand what you mean, sorry. Could you please point me
> >> >> to the *require* you talk about in the original proposal?
> >> >
> >> > Imagine a todo list that contains this line
> >> >
> >> >  merge -C abcdef 123456
> >> >
> >> > and now the user edits it (this is an interactive rebase, after
> >> > all), adding another merge head:
> >> >
> >> >  merge -C abcdef 987654 123456
> >> >
> >> > Now your strategy would have a serious problem: to find the
> >> > original version of 987654. If there was one.
> >> 
> >> We are talking about different checks then. My method has a built-in
> >> check that Pillip's one doesn't.
> >
> > Since you did not bother to elaborate, I have to assume that your
> > "built-in check" is that thing where intermediate merges can give you
> > conflicts?
> >
> > If so, there is a possibility in Phillip's method for such conflicts,
> > too: we have to perform as many 3-way merges as there are parent
> > commits.
> >
> > It does make me uncomfortable to have to speculate what you meant,
> > though.
> 
> It doesn't matter anymore as this check could easily be added to
> Phillip's algorithm as well, see [1].
> 
> [1] https://public-inbox.org/git/87efkn6s1h@javad.com

Ah, and there I was, thinking that finally you would answer my questions
directly, instead you keep directing me elsewhere ("read that! Somewhere
in there you will find the answer you are looking for").

My time is a bit too valuable, and I will not continue a discussion where
my questions are constantly deflected that way.

Ciao,
Johannes

Re: A bug in git merge

[Jeff King]

On Tue, Mar 27, 2018 at 12:53:52PM +0300, Orgad Shaneh wrote:

> If I cherry-pick a commit that added a line, then merge another commit
> which removes this line, the line remains in the file instead of being
> removed.
> 
> The following script demonstrates the bug.
> 
> file should be equivalent on both branches
> 
> git init
> seq 1 20 > file
> git add file
> git commit -m 'Initial'
> sed -i "s/^5/5\n55/" file
> git commit -a -m 'Added 55'
> git checkout -b other HEAD^
> git cherry-pick master
> git commit --amend --author 'Author ' --no-edit # generate a new hash
> sed -i '/55/d' file
> git commit -a -m 'Removed 55'
> git checkout master
> git merge --no-edit other
> git diff other # Should be equal

This isn't a bug; it's the expected behavior for a 3-way merge.

The merge looks only at the two final states to be merged, and the merge
base. The three states we have are:

base: without line 55
ours: with line 55
  theirs: without line 55

And since only one side made a change, the resolution is to take the
change in the final result. The fact that the other branch actually
manipulated the file (and how it manipulated it) isn't considered at
all.

The fact that it was a cherry-pick doesn't change that. A cherry-pick is
an application of the same changes, but it has no history-relationship
with the original commit in Git.

One could argue for or against the user experience of history as a DAG,
3-way endpoint merges, and how cherry-picks are stored, but this is all
working according to Git's design and not a bug.

-Peff

I NEED YOUR URGENT ASSISTANT

[Salif Musa]

-- 
Hi friend

I am a banker in ADB BANK. I want to transfer an abandoned sum of
USD15.6Million to your Bank account. 40/percent will be your share.

No risk involved but keeps it as secret. Contact me for more details.
Please reply me through my alternative email id only (salif.musa...@gmail.com)
for confidential reasons.


Yours
Dr Salif Musa

Re: Fwd: New Defects reported by Coverity Scan for git

[Jeff Hostetler]

On 3/26/2018 7:39 PM, Stefan Beller wrote:

coverity scan failed for the last couple month (since Nov 20th)
without me noticing, I plan on running it again nightly for the
Git project.

Anyway, here are issues that piled up (in origin/pu) since then.

Stefan


-- Forwarded message --

[...]


*** CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
/t/helper/test-json-writer.c: 278 in scripted()
272 struct json_writer jw = JSON_WRITER_INIT;
273 int k;
274
275 if (!strcmp(argv[0], "@object"))
276 jw_object_begin();
277 else if (!strcmp(argv[0], "@array"))

 CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
 Passing "" to "jw_array_begin", which dereferences null "jw.levels".

278 jw_array_begin();
279 else
280 die("first script term must be '@object' or
'@array': '%s'", argv[0]);
281
282 for (k = 1; k < argc; k++) {
283 const char *a_k = argv[k];

** CID 1433538:  Null pointer dereferences  (FORWARD_NULL)



The "jw.levels" field has been removed in the json-writer V4 reroll,
so this isn't an issue going forward.

Thanks,
Jeff

Business-Vorschlag für Sie zu behandeln!

[misbella . frazzeto]

Ich bin Sgt.Monica Lin Brown, ursprünglich aus Lake Jackson Texas USA. Ich habe 
persönlich eine spezielle Recherche zum Internet-Adressbuch durchgeführt und 
bin auf Ihre Informationen gestoßen. Ich schreibe Ihnen gerade diese Mail von 
der US-Militärbasis Kabul Afghanistan. Ich habe einen gesicherten 
Geschäftsvorschlag für Sie. Antworten Sie mir bei Interesse für weitere 
Informationen zu meiner privaten E-Mail (sgt.monicalinbrow...@outlook.com)

Business-Vorschlag für Sie zu behandeln!

[misbella . frazzeto]

Ich bin Sgt.Monica Lin Brown, ursprünglich aus Lake Jackson Texas USA. Ich habe 
persönlich eine spezielle Recherche zum Internet-Adressbuch durchgeführt und 
bin auf Ihre Informationen gestoßen. Ich schreibe Ihnen gerade diese Mail von 
der US-Militärbasis Kabul Afghanistan. Ich habe einen gesicherten 
Geschäftsvorschlag für Sie. Antworten Sie mir bei Interesse für weitere 
Informationen zu meiner privaten E-Mail (sgt.monicalinbrow...@outlook.com)

Re: [PATCH v4] json_writer: new routines to create data in JSON format

[Jeff Hostetler]

On 3/26/2018 11:18 PM, Ramsay Jones wrote:

On 26/03/18 15:31, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 

[snip]

Thanks, this version fixes all issues I had (with the compilation
and sparse warnings).

[Was using UINT64_C(0x) a problem on windows?]


Thanks for the confirmation.

I was building on Linux.  I haven't tried using UINT64_C()
for anything, but I'll keep that in mind next time.

Thanks,
Jeff

Re: [RFC PATCH 1/1] json-writer: incorrect format specifier

[Jeff Hostetler]

On 3/26/2018 11:26 PM, Ramsay Jones wrote:

On 26/03/18 18:04, Junio C Hamano wrote:

Ramsay Jones  writes:

[...]

I must confess to not having given any thought to the wider
implications of the code. I don't really know what this code
is going to be used for. [Although I did shudder when I read
some mention of a 'universal interchange format' - I still
have nightmares about XML :-D ]

[...]

My current goals are to add telemetry in a friendly way and
have events written in JSON to some audit destination.
Something like:

{ "argv":["./git","status"],
  "pid":84941,
  "exit-code":0,
  "elapsed-time":0.011121,
  "version":"2.16.2.5.g71445db.dirty",
  ... }

Later, we could add a JSON formatter to a command like "status"
and then do things like:

$ git status --json | python '... json.load ...'

and eliminate the need to write custom parsers for normal
or porcelain formats.  There are other commands that could
be similarly adapted and save callers a lot of screen-scraping
code.  But that is later.

Thanks,
Jeff

Business-Vorschlag für Sie zu behandeln!

[misbella . frazzeto]

Ich bin Sgt.Monica Lin Brown, ursprünglich aus Lake Jackson Texas USA. Ich habe 
persönlich eine spezielle Recherche zum Internet-Adressbuch durchgeführt und 
bin auf Ihre Informationen gestoßen. Ich schreibe Ihnen gerade diese Mail von 
der US-Militärbasis Kabul Afghanistan. Ich habe einen gesicherten 
Geschäftsvorschlag für Sie. Antworten Sie mir bei Interesse für weitere 
Informationen zu meiner privaten E-Mail (sgt.monicalinbrow...@outlook.com)

Business-Vorschlag für Sie zu behandeln!

[misbella . frazzeto]

Ich bin Sgt.Monica Lin Brown, ursprünglich aus Lake Jackson Texas USA. Ich habe 
persönlich eine spezielle Recherche zum Internet-Adressbuch durchgeführt und 
bin auf Ihre Informationen gestoßen. Ich schreibe Ihnen gerade diese Mail von 
der US-Militärbasis Kabul Afghanistan. Ich habe einen gesicherten 
Geschäftsvorschlag für Sie. Antworten Sie mir bei Interesse für weitere 
Informationen zu meiner privaten E-Mail (sgt.monicalinbrow...@outlook.com)

Re: [RFC PATCH v5 0/8] rebase-interactive

[Jeff Hostetler]

On 3/27/2018 1:07 AM, Junio C Hamano wrote:

Jeff Hostetler  writes:

[...]

So I would think it is most sensible to have double, uintmax_t and
intmax_t variants.  If you do not care about the extra value range
that unsigned integral types afford, a single intmax_t variant would
also be fine.


I'll reroll with just the double and intmax_t variants.
Thanks for the feedback and sorry for all the noise.

Jeff

A bug in git merge

[Orgad Shaneh]

Hi,

If I cherry-pick a commit that added a line, then merge another commit
which removes this line, the line remains in the file instead of being
removed.

The following script demonstrates the bug.

file should be equivalent on both branches

git init
seq 1 20 > file
git add file
git commit -m 'Initial'
sed -i "s/^5/5\n55/" file
git commit -a -m 'Added 55'
git checkout -b other HEAD^
git cherry-pick master
git commit --amend --author 'Author ' --no-edit # generate a new hash
sed -i '/55/d' file
git commit -a -m 'Removed 55'
git checkout master
git merge --no-edit other
git diff other # Should be equal

- Orgad

Re: [PATCH v5 5/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer  wrote:
> Currently 'git worktree add ' creates a new branch named after the
> basename of the path by default.  If a branch with that name already
> exists, the command refuses to do anything, unless the '--force' option
> is given.
>
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else.  This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -317,7 +318,10 @@ static int add_worktree(const char *path, const char 
> *refname,
> -   if (opts->new_branch)
> +   if (opts->checkout_existing_branch)
> +   fprintf_ln(stderr, _("checking out branch '%s'"),
> +  refname);

This fprintf_ln() can easily fit on one line; no need to wrap
'refname' to the next one.

Not worth a re-roll, though.

> +   else if (opts->new_branch)
> fprintf_ln(stderr, _("creating branch '%s'"), 
> opts->new_branch);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -198,13 +198,26 @@ test_expect_success '"add" with  omitted' '
> -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> +test_expect_success '"add" checks out existing branch of dwimd name' '
> test_commit c1 &&
> test_commit c2 &&
> git branch precious HEAD~1 &&
> -   test_must_fail git worktree add precious &&
> +   git worktree add precious &&
> test_cmp_rev HEAD~1 precious &&
> -   test_path_is_missing precious
> +   (
> +   cd precious &&
> +   test_cmp_rev precious HEAD
> +   )
> +'

Looking at this more closely, once it stops being a "don't clobber
precious branch" test, it's doing more than it needs to do, thus is
confusing for future readers. The setup -- creating two commits and
making "precious" point one commit back -- was very intentional and
allowed the test to verify not only that the worktree wasn't created
but that "precious" didn't change to reference a different commit.
However, _none_ of this matters once it becomes a dwim'ing test; the
unnecessary code is confusing since it doesn't make sense in the
context of dwim'ing. I _think_ the entire test can collapse to:

git branch existing &&
git worktree add existing &&
(
cd existing &&
test_cmp_rev existing HEAD
)

In other words, this patch should drop the "precious" test altogether
and just introduce a new dwim'ing test (and drop patch 6/6).

I do think that with the potential confusion to future readers, this
does deserve a re-roll.

> +test_expect_success '"add" auto-vivify fails with checked out branch' '
> +   git checkout -b test-branch &&
> +   test_must_fail git worktree add test-branch &&
> +   test_path_is_missing test-branch
> +'
> +
> +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> +   git worktree add --force test-branch
>  '

Re: [PATCH v5 4/6] worktree: factor out dwim_branch function

[Eric Sunshine]

On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer  wrote:
> Factor out a dwim_branch function, which takes care of the dwim'ery in
> 'git worktree add '.  It's not too much code currently, but we're
> adding a new kind of dwim in a subsequent patch, at which point it makes
> more sense to have it as a separate function.
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -417,16 +431,9 @@ static int add(int ac, const char **av, const char 
> *prefix)
> if (ac < 2 && !opts.new_branch && !opts.detach) {
> -   int n;
> -   const char *s = worktree_basename(path, );
> -   opts.new_branch = xstrndup(s, n);
> -   if (guess_remote) {
> -   struct object_id oid;
> -   const char *remote =
> -   unique_tracking_name(opts.new_branch, );
> -   if (remote)
> -   branch = remote;
> -   }
> +   const char *dwim_branchname = dwim_branch(path, );
> +   if (dwim_branchname)
> +   branch = dwim_branchname;

I don't care strongly but the name 'dwim_branchname' is awfully long
in a context where it's assigned the result of a call to
dwim_branch(). In this tiny code block, a short and sweet name 's'
would be easy to grok; there'd be no confusion.

Anyhow, it's subjective and not at all worth a re-roll.

Re: [PATCH v5 3/6] worktree: remove force_new_branch from struct add_opts

[Eric Sunshine]

On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer  wrote:
> The 'force_new_branch' flag in 'struct add_opts' is only used inside the
> add function, where we already have the same information stored in the
> 'new_branch_force' variable.  Avoid that unnecessary duplication.

When I was reviewing your original dwim-ery series (inferring 'foo'
from 'origin/foo'), I noticed that 'struct add_opts' had accumulated a
number of unnecessary members over time, including this one. My plan
was to submit a patch removing all those pointless members after your
dwim-ery series settled, however, I never got around to it.

This patch might be a good opportunity for doing that cleanup
wholesale, removing all unneeded members rather than just the one. (If
so, you'd probably want to move to this patch to the front of the
series as cleanup before the meatier changes.) Anyhow, it's just a
thought; feel free to respond with "it's outside the scope of this
series" if you're not interested in doing it.

> Signed-off-by: Thomas Gummerer