Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Elijah Newren]

Hi,

On Fri, Apr 27, 2018 at 2:45 PM, Totsten Bögershausen  wrote:
> On 2018-04-26 19:23, Elijah Newren wrote:

>> Sure.  First, though, note that I can make it pass (or at least "not
>> ok...TODO known breakage") with the following patch (may be
>> whitespace-damaged by gmail):
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 483c8d6d7..770b91f8c 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1106,12 +1106,7 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
>>  auml=$(printf "\303\244")
>>  aumlcdiar=$(printf "\141\314\210")
>>  >"$auml" &&
>> -   case "$(echo *)" in
>> -   "$aumlcdiar")
>> -   true ;;
>> -   *)
>> -   false ;;
>> -   esac
>> +   stat "$aumlcdiar" >/dev/null 2>/dev/null
>
>
> Nicely analyzed and improved.
>
> The "stat" statement is technically correct.
> I think that a more git-style fix would be
> [] ---
> +   test -r "$aumlcdiar"
>
> instead of the stat.
>
> I looked into the 2 known breakages.
> In short: they test use cases which are not sooo important for a user in
> practice, but do a good test if the code is broken.
> IOW: I can't see a need for immediate action.
>
> As you already did all the analyzes:
> Do you want to send a patch ?

You know, despite seeing the "test_expect_failure" and "TODO...known
breakage" with these tests and even mentioning them, it somehow didn't
sink in and I was still thinking that there might be some kind of
unicode normalization handling in the codebase somewhere (similar to
the case insensitivy handling that I've seen in a place or two) that
now needed to be extended.  I should have realized that
test_expect_failure meant there wasn't, and thus all we needed to do
was to mark it as continuing to fail with the new filesystem,  Should
have realized, but didn't.  Oops.

Anyway, it looks like you've already submitted a patch and marked it
as having been reported by me, which is just fine.  Thanks!

Elijah

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[brian m. carlson]

On Mon, Apr 30, 2018 at 12:30:57PM +0900, Junio C Hamano wrote:
> Thanks.  It is true that the current output from the tool is corrupt
> mime multi-part, and we need to do something about it.
> 
> I however have to wonder if it even makes sense for --cover to pay
> attention to --attach and produce the cover template that has "BLURB
> HERE" etc.  in a multi-part format.  Shouldn't we be making a simple
> plain text file instead?

I agree that multipart/mixed is not a useful content-type for only one
plain text part.  I have a patch to add the trailing boundary, but I
think you make a good argument that perhaps omitting the entire
multipart portion would be better.

I'll have to work on this after work, so expect a patch later today.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Junio C Hamano]

"brian m. carlson"  writes:

> On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote:
>> When you use `git format-patch --cover-letter --attach`, the cover
>> letter does not have the trailing MIME boundary. RFC2046 states that the
>> last part must be followed by a closing boundary. This causes some email
>> clients (Thunderbird in my case) to discard the message body.
>> This is experienced with git 2.16.3.
>
> Thanks for reporting this.  I can confirm this with a reasonably recent
> next.  Let me see if I can come up with a patch.

Thanks.  It is true that the current output from the tool is corrupt
mime multi-part, and we need to do something about it.

I however have to wonder if it even makes sense for --cover to pay
attention to --attach and produce the cover template that has "BLURB
HERE" etc.  in a multi-part format.  Shouldn't we be making a simple
plain text file instead?

Re: [PATCH 6/6] Convert remaining die*(BUG) messages

[Eric Sunshine]

On Sun, Apr 29, 2018 at 6:19 PM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
> These were not caught by the previous commit, as they did not match the
> regular expression.
>
> Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
> ---
> diff --git a/submodule.c b/submodule.c
> @@ -2043,7 +2043,7 @@ const char *get_superproject_working_tree(void)
> if (super_sub_len > cwd_len ||
> strcmp([cwd_len - super_sub_len], super_sub))
> -   die (_("BUG: returned path string doesn't match 
> cwd?"));
> +   BUG("returned path string doesn't match cwd?");

This message used to be localizable but no longer is, which makes
sense since it's not intended as a user-facing error message but
rather is meant for Git developers. Good.

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[brian m. carlson]

On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote:
> When you use `git format-patch --cover-letter --attach`, the cover
> letter does not have the trailing MIME boundary. RFC2046 states that the
> last part must be followed by a closing boundary. This causes some email
> clients (Thunderbird in my case) to discard the message body.
> This is experienced with git 2.16.3.

Thanks for reporting this.  I can confirm this with a reasonably recent
next.  Let me see if I can come up with a patch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Patrick Hemmer]

When you use `git format-patch --cover-letter --attach`, the cover
letter does not have the trailing MIME boundary. RFC2046 states that the
last part must be followed by a closing boundary. This causes some email
clients (Thunderbird in my case) to discard the message body.
This is experienced with git 2.16.3.

For example:

$ git format-patch --cover-letter --attach --root -o /tmp/out
/tmp/out/-cover-letter.patch
/tmp/out/0001-hello-world.patch

$ cat /tmp/out/-cover-letter.patch
>From a25ac88e6216131e8b000335d32bb99d4e5185ac Mon Sep 17 00:00:00 2001
From: Patrick Hemmer 
Date: Sun, 29 Apr 2018 21:26:45 -0400
Subject: [PATCH 0/1] *** SUBJECT HERE ***
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.16.3"

This is a multi-part message in MIME format.
--2.16.3
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


*** BLURB HERE ***

Patrick Hemmer (1):
  hello world

-- 
2.16.3

[PATCH 6/6] Convert remaining die*(BUG) messages

[Johannes Schindelin]

These were not caught by the previous commit, as they did not match the
regular expression.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 git-compat-util.h | 2 +-
 pathspec.c| 2 +-
 submodule.c   | 2 +-
 vcs-svn/fast_export.c | 6 --
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 07e383257b4..3a7216f5313 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1052,7 +1052,7 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
 
 #define QSORT_S(base, n, compar, ctx) do { \
if (qsort_s((base), (n), sizeof(*(base)), compar, ctx)) \
-   die("BUG: qsort_s() failed");   \
+   BUG("qsort_s() failed");\
 } while (0)
 
 #ifndef REG_STARTEND
diff --git a/pathspec.c b/pathspec.c
index a637a6d15c0..27cd6067860 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -486,7 +486,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
if (item->nowildcard_len > item->len ||
item->prefix > item->len) {
-   die ("BUG: error initializing pathspec_item");
+   BUG("error initializing pathspec_item");
}
 }
 
diff --git a/submodule.c b/submodule.c
index 733db441714..c282fa8fe51 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2043,7 +2043,7 @@ const char *get_superproject_working_tree(void)
 
if (super_sub_len > cwd_len ||
strcmp([cwd_len - super_sub_len], super_sub))
-   die (_("BUG: returned path string doesn't match cwd?"));
+   BUG("returned path string doesn't match cwd?");
 
super_wt = xstrdup(cwd);
super_wt[cwd_len - super_sub_len] = '\0';
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 3fd047a8b82..b5b8913cb0f 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -320,7 +320,8 @@ const char *fast_export_read_path(const char *path, 
uint32_t *mode_out)
err = fast_export_ls(path, mode_out, );
    if (err) {
if (errno != ENOENT)
-       die_errno("BUG: unexpected fast_export_ls error");
+   BUG("unexpected fast_export_ls error: %s",
+   strerror(errno));
/* Treat missing paths as directories. */
*mode_out = S_IFDIR;
return NULL;
@@ -338,7 +339,8 @@ void fast_export_copy(uint32_t revision, const char *src, 
const char *dst)
err = fast_export_ls_rev(revision, src, , );
if (err) {
if (errno != ENOENT)
-   die_errno("BUG: unexpected fast_export_ls_rev error");
+   BUG("unexpected fast_export_ls_rev error: %s",
+   strerror(errno));
fast_export_delete(dst);
return;
}
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 5/6] Replace all die("BUG: ...") calls by BUG() ones

[Johannes Schindelin]

In d8193743e08 (usage.c: add BUG() function, 2017-05-12), a new macro
was introduced to use for reporting bugs instead of die(). It was then
subsequently used to convert one single caller in 588a538ae55
(setup_git_env: convert die("BUG") to BUG(), 2017-05-12).

The cover letter of the patch series containing this patch
(cf 20170513032414.mfrwabt4hovuj...@sigill.intra.peff.net) is not
terribly clear why only one call site was converted, or what the plan
is for other, similar calls to die() to report bugs.

Let's just convert all remaining ones in one fell swoop.

This trick was performed by this invocation:

sed -i 's/die("BUG: /BUG("/g' $(git grep -l 'die("BUG' \*.c)

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 apply.c  |  4 ++--
 archive-tar.c|  2 +-
 attr.c   | 10 +-
 blame.c  |  2 +-
 builtin/am.c | 20 +--
 builtin/branch.c |  2 +-
 builtin/cat-file.c   |  4 ++--
 builtin/clone.c  |  2 +-
 builtin/commit.c |  2 +-
 builtin/config.c |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  4 ++--
 builtin/init-db.c|  2 +-
 builtin/ls-files.c   |  2 +-
 builtin/notes.c  |  8 
 builtin/pack-objects.c   |  4 ++--
 builtin/pull.c   |  2 +-
 builtin/receive-pack.c   |  2 +-
 builtin/replace.c|  2 +-
 builtin/update-index.c   |  2 +-
 bulk-checkin.c   |  2 +-
 color.c  |  4 ++--
 column.c |  2 +-
 config.c | 12 +--
 date.c   |  2 +-
 diff.c   | 12 +--
 dir-iterator.c   |  2 +-
 grep.c   | 16 +++
 http.c   |  8 
 imap-send.c  |  2 +-
 lockfile.c   |  2 +-
 mailinfo.c   |  2 +-
 merge-recursive.c| 12 +--
 notes-merge.c|  4 ++--
 pack-bitmap-write.c  |  2 +-
 pack-bitmap.c|  6 +++---
 pack-objects.c   |  2 +-
 packfile.c   |  6 +++---
 pathspec.c   | 10 +-
 pkt-line.c   |  2 +-
 prio-queue.c |  2 +-
 ref-filter.c |  4 ++--
 refs.c   | 34 
 remote.c |  2 +-
 revision.c   |  4 ++--
 run-command.c| 10 +-
 setup.c  |  4 ++--
 sha1-lookup.c|  2 +-
 sha1-name.c  |  4 ++--
 shallow.c|  6 +++---
 sigchain.c   |  2 +-
 strbuf.c |  4 ++--
 submodule.c  |  6 +++---
 t/helper/test-example-decorate.c | 16 +++
 tmp-objdir.c |  2 +-
 trailer.c|  6 +++---
 transport.c  |  4 ++--
 unpack-trees.c   |  2 +-
 worktree.c   |  2 +-
 wrapper.c|  4 ++--
 wt-status.c  | 20 +--
 zlib.c   |  4 ++--
 63 files changed, 168 insertions(+), 168 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996f..3866adbc97f 100644
--- a/apply.c
+++ b/apply.c
@@ -2375,7 +2375,7 @@ static void update_pre_post_images(struct image *preimage,
if (postlen
? postlen < new_buf - postimage->buf
: postimage->len < new_buf - postimage->buf)
-   die("BUG: caller miscounted postlen: asked %d, orig = %d, used 
= %d",
+   BUG("caller miscounted postlen: asked %d, orig = %d, used = %d",
(int)postlen, (int) postimage->len, (int)(new_buf - 
postimage->buf));
 
/* Fix the length of the whole thing */
@@ -3509,7 +3509,7 @@ static int load_current(struct apply_state *state,
unsigned mode = patch->new_mode;
 
if (!patch->is_new)
-   die("BUG: patch to %s is not a creation", patch->old_name);
+   BUG("patch to %s is not a creation", patch->old_name);
 
pos = cache_name_pos(name, strlen(name));
if (pos < 0)
diff --git a/archive-tar.c b/archive-tar.c
index 3563bcb9f26..61a1a2547cc 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -441,7 +441,7 @@ static int write_tar_filter_archive(const struct archiver 
*ar,
int r;
 
if (!ar->data)
-

[PATCH 3/6] refs/*: report bugs using the BUG() macro

[Johannes Schindelin]

We just prepared t1406 to be okay with BUG reports resulting in SIGABRT
instead of a regular exit code indicating failure. This commit now makes
it so: by calling BUG() (which eventually calls `abort()`), we no longer
exit with code 128 but instead throw that signal.

This trick was performed by this invocation:

sed -i 's/die("BUG: /BUG("/' $(git grep -l 'die("BUG' refs/\*.c)

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 refs/files-backend.c  | 20 ++--
 refs/iterator.c   |  6 +++---
 refs/packed-backend.c | 16 
 refs/ref-cache.c  |  2 +-
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a92a2aa8213..332da47edd9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -125,7 +125,7 @@ static void files_assert_main_repository(struct 
files_ref_store *refs,
if (refs->store_flags & REF_STORE_MAIN)
        return;
 
-   die("BUG: operation %s only allowed for main ref store", caller);
+   BUG("operation %s only allowed for main ref store", caller);
 }
 
 /*
@@ -141,13 +141,13 @@ static struct files_ref_store *files_downcast(struct 
ref_store *ref_store,
struct files_ref_store *refs;
 
if (ref_store->be != _be_files)
-   die("BUG: ref_store is type \"%s\" not \"files\" in %s",
+   BUG("ref_store is type \"%s\" not \"files\" in %s",
ref_store->be->name, caller);
 
refs = (struct files_ref_store *)ref_store;
 
if ((refs->store_flags & required_flags) != required_flags)
-   die("BUG: operation %s requires abilities 0x%x, but only have 
0x%x",
+   BUG("operation %s requires abilities 0x%x, but only have 0x%x",
caller, required_flags, refs->store_flags);
 
return refs;
@@ -166,7 +166,7 @@ static void files_reflog_path(struct files_ref_store *refs,
strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
break;
default:
-   die("BUG: unknown ref type %d of ref %s",
+   BUG("unknown ref type %d of ref %s",
ref_type(refname), refname);
}
 }
@@ -184,7 +184,7 @@ static void files_ref_path(struct files_ref_store *refs,
    strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
break;
default:
-   die("BUG: unknown ref type %d of ref %s",
+   BUG("unknown ref type %d of ref %s",
        ref_type(refname), refname);
}
 }
@@ -2010,7 +2010,7 @@ static int files_for_each_reflog_ent_reverse(struct 
ref_store *ref_store,
 
}
if (!ret && sb.len)
-   die("BUG: reverse reflog parser had leftover data");
+   BUG("reverse reflog parser had leftover data");
 
fclose(logfp);
strbuf_release();
@@ -2088,7 +2088,7 @@ static int files_reflog_iterator_advance(struct 
ref_iterator *ref_iterator)
 static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator,
   struct object_id *peeled)
 {
-   die("BUG: ref_iterator_peel() called for reflog_iterator");
+   BUG("ref_iterator_peel() called for reflog_iterator");
 }
 
 static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
@@ -2873,7 +2873,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
assert(err);
 
if (transaction->state != REF_TRANSACTION_OPEN)
-   die("BUG: commit called for transaction that is not open");
+   BUG("commit called for transaction that is not open");
 
/* Fail if a refname appears more than once in the transaction: */
for (i = 0; i < transaction->nr; i++)
@@ -2899,7 +2899,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 */
if (refs_for_each_rawref(>base, ref_present,
 _refnames))
-   die("BUG: initial ref transaction called with existing refs");
+   BUG("initial ref transaction called with existing refs");
 
packed_transaction = 
ref_store_transaction_begin(refs->packed_ref_store, err);
if (!packed_transaction) {
@@ -2912,7 +2912,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 
if ((update->flags & REF_HAVE_OLD) &&
!is_null_oid(>old_oid))
-   die("BUG: initial ref transaction with old_sha1 set");
+   BUG("initial ref transaction with old

[PATCH 4/6] run-command: use BUG() to report bugs, not die()

[Johannes Schindelin]

The slightly misleading name die_bug() of the function intended to
report a bug is actually called always, and only reports a bug if the
passed-in parameter `err` is non-zero.

It uses die_errno() to report the bug, to helpfully include the error
message corresponding to `err`.

However, as these messages indicate bugs, we really should use BUG().
And as BUG() is a macro to be able to report the exact file and line
number, we need to convert die_bug() to a macro instead of only
replacing the die_errno() by a call to BUG().

While at it, use a name more indicative of the purpose: CHECK_BUG().

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 run-command.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 12c94c1dbe5..0ad6f135d5a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -471,15 +471,12 @@ struct atfork_state {
sigset_t old;
 };
 
-#ifndef NO_PTHREADS
-static void bug_die(int err, const char *msg)
-{
-   if (err) {
-   errno = err;
-   die_errno("BUG: %s", msg);
-   }
-}
-#endif
+#define CHECK_BUG(err, msg) \
+   do { \
+   int e = (err); \
+   if (e) \
+       BUG("%s: %s", msg, strerror(e)); \
+   } while(0)
 
 static void atfork_prepare(struct atfork_state *as)
 {
@@ -491,9 +488,9 @@ static void atfork_prepare(struct atfork_state *as)
if (sigprocmask(SIG_SETMASK, , >old))
die_errno("sigprocmask");
 #else
-   bug_die(pthread_sigmask(SIG_SETMASK, , >old),
+   CHECK_BUG(pthread_sigmask(SIG_SETMASK, , >old),
"blocking all signals");
-   bug_die(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, >cs),
+   CHECK_BUG(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, >cs),
"disabling cancellation");
 #endif
 }
@@ -504,9 +501,9 @@ static void atfork_parent(struct atfork_state *as)
if (sigprocmask(SIG_SETMASK, >old, NULL))
die_errno("sigprocmask");
 #else
-   bug_die(pthread_setcancelstate(as->cs, NULL),
+   CHECK_BUG(pthread_setcancelstate(as->cs, NULL),
"re-enabling cancellation");
-   bug_die(pthread_sigmask(SIG_SETMASK, >old, NULL),
+   CHECK_BUG(pthread_sigmask(SIG_SETMASK, >old, NULL),
"restoring signal mask");
 #endif
 }
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Schindelin]

t1406 specifically verifies that certain code paths fail with a BUG: ...
message.

In the upcoming commit, we will convert that message to be generated via
BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
regular exit code.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1406-submodule-ref-store.sh | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e093782cc37..0ea3457cae3 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pack_refs() not allowed' '
-   test_must_fail $RUN pack-refs 3
+   test_must_fail ok=sigabrt $RUN pack-refs 3
 '
 
 test_expect_success 'peel_ref(new-tag)' '
@@ -27,15 +27,18 @@ test_expect_success 'peel_ref(new-tag)' '
 '
 
 test_expect_success 'create_symref() not allowed' '
-   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+   test_must_fail ok=sigabrt \
+   $RUN create-symref FOO refs/heads/master nothing
 '
 
 test_expect_success 'delete_refs() not allowed' '
-   test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
+   test_must_fail ok=sigabrt \
+   $RUN delete-refs 0 nothing FOO refs/tags/new-tag
 '
 
 test_expect_success 'rename_refs() not allowed' '
-   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+   test_must_fail ok=sigabrt \
+   $RUN rename-ref refs/heads/master refs/heads/new-master
 '
 
 test_expect_success 'for_each_ref(refs/heads/)' '
@@ -91,11 +94,11 @@ test_expect_success 'reflog_exists(HEAD)' '
 '
 
 test_expect_success 'delete_reflog() not allowed' '
-   test_must_fail $RUN delete-reflog HEAD
+   test_must_fail ok=sigabrt $RUN delete-reflog HEAD
 '
 
 test_expect_success 'create-reflog() not allowed' '
-   test_must_fail $RUN create-reflog HEAD 1
+   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
 '
 
 test_done
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 0/6] Finish the conversion from die("BUG: ...") to BUG()

[Johannes Schindelin]

The BUG() macro was introduced in this patch series:
https://public-inbox.org/git/20170513032414.mfrwabt4hovuj...@sigill.intra.peff.net

The second patch in that series converted one caller from die("BUG: ")
to use the BUG() macro.

It seems that there was no concrete plan to address the same issue in
the rest of the code base.

This patch series tries to do that.

Note: I separated out 4/6 ("refs/*: report bugs using the BUG() macro")
from 5/6 ("Replace all die("BUG: ...") to keep it cuddled with the patch
2/6 that prepares t1406 for this change of refs/' behavior.

Note also: I would be very surprised if the monster commit 5/6 ("Replace
all die("BUG: ...") calls by BUG() ones") would apply cleanly on `pu` (I
develop this on top of `master`).

For that reason, the commit message contains the precise Unix shell
invocation (GNU sed semantics, not BSD sed ones, because I know that the
Git maintainer as well as the author of the patch introducing BUG() both
use Linux and not macOS or any other platform that would offer a BSD
sed). It should be straight-forward to handle merge
conflicts/non-applying patches by simply re-running that command.


Johannes Schindelin (6):
  test_must_fail: support ok=sigabrt
  t1406: prepare for the refs code to fail with BUG()
  refs/*: report bugs using the BUG() macro
  run-command: use BUG() to report bugs, not die()
  Replace all die("BUG: ...") calls by BUG() ones
  Convert remaining die*(BUG) messages

 apply.c  |  4 ++--
 archive-tar.c|  2 +-
 attr.c   | 10 +-
 blame.c  |  2 +-
 builtin/am.c | 20 +--
 builtin/branch.c |  2 +-
 builtin/cat-file.c   |  4 ++--
 builtin/clone.c  |  2 +-
 builtin/commit.c |  2 +-
 builtin/config.c |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  4 ++--
 builtin/init-db.c|  2 +-
 builtin/ls-files.c   |  2 +-
 builtin/notes.c  |  8 
 builtin/pack-objects.c   |  4 ++--
 builtin/pull.c   |  2 +-
 builtin/receive-pack.c   |  2 +-
 builtin/replace.c|  2 +-
 builtin/update-index.c   |  2 +-
 bulk-checkin.c   |  2 +-
 color.c  |  4 ++--
 column.c |  2 +-
 config.c | 12 +--
 date.c   |  2 +-
 diff.c   | 12 +--
 dir-iterator.c   |  2 +-
 git-compat-util.h|  2 +-
 grep.c   | 16 +++
 http.c   |  8 
 imap-send.c  |  2 +-
 lockfile.c   |  2 +-
 mailinfo.c   |  2 +-
 merge-recursive.c| 12 +--
 notes-merge.c|  4 ++--
 pack-bitmap-write.c  |  2 +-
 pack-bitmap.c|  6 +++---
 pack-objects.c   |  2 +-
 packfile.c   |  6 +++---
 pathspec.c   | 12 +--
 pkt-line.c   |  2 +-
 prio-queue.c |  2 +-
 ref-filter.c |  4 ++--
 refs.c   | 34 
 refs/files-backend.c | 20 +--
 refs/iterator.c  |  6 +++---
 refs/packed-backend.c| 16 +++
 refs/ref-cache.c |  2 +-
 remote.c |  2 +-
 revision.c   |  4 ++--
 run-command.c| 33 ++-
 setup.c  |  4 ++--
 sha1-lookup.c|  2 +-
 sha1-name.c  |  4 ++--
 shallow.c|  6 +++---
 sigchain.c   |  2 +-
 strbuf.c |  4 ++--
 submodule.c  |  8 
 t/helper/test-example-decorate.c | 16 +++
 t/t1406-submodule-ref-store.sh   | 15 --
 t/test-lib-functions.sh  |  5 -
 tmp-objdir.c |  2 +-
 trailer.c|  6 +++---
 transport.c  |  4 ++--
 unpack-trees.c   |  2 +-
 vcs-svn/fast_export.c|  6 --
 worktree.c   |  2 +-
 wrapper.c|  4 ++--
 wt-status.c  | 20 +--
 zlib.c   |  4 ++--
 71 files changed, 220 insertions(+), 215 deletions(-)


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/use-bug-macro-v1
Fetch-It-Via: git fetch http

[PATCH v7 03/12] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

Re: Branch deletion question / possible bug?

[Johannes Schindelin]

Hi,

On Sat, 28 Apr 2018, Philip Oakley wrote:

> From: "Jacob Keller" <jacob.kel...@gmail.com>
> > On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S <pik.s.t...@boeing.com>
> > wrote:
> > > Hi,
> > >
> > > I discovered that I was able to delete the feature branch I was in, due
> > > to some fat fingering on my part and case insensitivity.  I never
> > > realized this could be done before.  A quick google search did not give
> > > me a whole lot to work with...
> > >
> > > Steps to reproduce:
> > > 1. Create a feature branch, "editCss"
> > > 2. git checkout master
> > > 3. git checkout editCSS
> > > 4. git checkout editCss
> > > 5. git branch -d editCSS
> > >
> >
> > Are you running on a case-insensitive file system? What version of
> > git? I thought I recalled seeing commits to help avoid creating
> > branches of the same name with separate case when we know we're on a
> > file system which is case-insensitive..
> >
> > > Normally, it should have been impossible for a user to delete the branch
> > > they're on.  And the deletion left me in a weird state that took a while
> > > to dig out of.
> > >
> > > I know this was a user error, but I was also wondering if this was a bug.
> >
> > If we have not yet done this, I think we should. Long term this would
> > be fixed by using a separate format to store refs than the filesystem,
> > which has a few projects being worked on but none have been put into a
> > release.
> 
> Yes, this is an on-going problem on Windows and other case insentive
> systems. At the moment the branch name becomes embedded as a file name, so
> when Git requests details of a branch from the filesystem, it can get a case
> insensitive equivalent. Meanwhile, internally Git is checking for equality
> in a case sensitive [Linux] way with obvious consequences such as this - The
> most obvious being when there is no "*" current branch marker in the branch
> status list.
> 
> It's a bit tricky to fix (internally the name and the path are passed down
> different call chains), and depends on how one expects the case
> insensitivity to work - the kicker is when someone does an edit of the name
> via the file system and expects Git to cope (i.e. devs knowing, or think
> they know, too much detail ;-).
> 
> The refs can also get packed, so the "bad spelling" gets baked in.
> Ultimately it probably means that GfW and other systems will need  a case
> sensitivity check when opening paths...

FWIW I outlined what I think is the best route to fix this for good:

https://github.com/git-for-windows/git/issues/1623#issuecomment-380085257

Essentially, I think we should teach Git the trick to check the spelling
before calling lstat() in refs/files-backend.c.

To check the spelling, we would need an API to get the on-disk
representation of a given path. On Windows, I know this call. On Linux,
apparently canonicalize_file_name() might do the job, but that is a GNU
libc extension, and won't help us on macOS.

Any ideas?

Ciao,
Dscho

Re: [Bug] git log --show-signature print extra carriage return ^M

[Johannes Schindelin]

Hi Larry,


On Tue, 6 Mar 2018, Larry Hunter wrote:

> The same ^M is shown in the output of tutorial
> 
> 
> https://www.geekality.net/2017/08/23/setting-up-gpg-signing-for-gitgithub-on-windows/
> 
> at item "4. Verify commit was signed"


Please understand that it would be helpful if you could take the lead on
resolving this issue.

If you need pointers how to get started with fixing this behavior, please
just tell us where you got stuck, so we can help you get un-stuck.

Ciao,
Johannes

> I confirm the output is right (no ^M characters) with commands
> 
> git verify-commit HEAD
> git -p verify-commit HEAD
> git verify-commit --v HEAD
> git verify-commit --raw HEAD
> 
> and wrong (ending with ^M characters) with
> 
> git  log --show-signature -1 HEAD
> git  -p log --show-signature -1 HEAD
> 
> I need gpg version 2.1 or greater to generate a gpg key for my windows
> system, as stated by the github documentation:
> 
> https://help.github.com/articles/generating-a-new-gpg-key/
> 
> that saves my keys in ~/AppData/Roaming/GnuPG.
> 
> 2018-03-05 15:29 GMT+01:00 Johannes Schindelin <johannes.schinde...@gmx.de>:
> > Hi Larry,
> >
> > On Sun, 4 Mar 2018, Larry Hunter wrote:
> >
> >> There is bug using "git log --show-signature" in my installation
> >>
> >> git 2.16.2.windows.1
> >> gpg (GnuPG) 2.2.4
> >> libgcrypt 1.8.2
> >
> > The gpg.exe shipped in Git for Windows should say something like this:
> >
> > $ gpg --version
> > gpg (GnuPG) 1.4.22
> > Copyright (C) 2015 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later
> > <http://gnu.org/licenses/gpl.html>
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.
> >
> > Home: ~/.gnupg
> > Supported algorithms:
> > Pubkey: RSA, RSA-E, RSA-S, ELG-E, DSA
> > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> > CAMELLIA128, CAMELLIA192, CAMELLIA256
> > Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> > Compression: Uncompressed, ZIP, ZLIB, BZIP2
> >
> > Therefore, the GNU Privacy Guard version you use is not the one shipped
> > and supported by the Git for Windows project.
> >
> >> that prints (with colors) an extra ^M (carriage return?) at the end of
> >> the gpg lines. As an example, the output of "git log --show-signature
> >> HEAD" looks like:
> >>
> >> $ git log --show-signature HEAD
> >> commit 46c490188ebd216f20c454ee61108e51b481844e (HEAD -> master)
> >> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale^M
> >> gpg:using RSA key ...^M
> >> gpg: Good signature from "..." [ultimate]^M
> >> Author: ... <...>
> >> Date:   Sun Mar 4 16:53:06 2018 +0100
> >> ...
> >>
> >> To help find a fix, I tested the command "git verify-commit HEAD" that
> >> prints (without colors) the same lines without extra ^M characters.
> >>
> >> $ git verify-commit HEAD
> >> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale
> >> gpg:using RSA key ...
> >> gpg: Good signature from "..." [ultimate]
> >
> > My guess is that the latter command simply does not go through the pager
> > while the former does.
> >
> > Do you see the ^M in the output of `git -p verify-commit HEAD`?
> >
> > Ciao,
> > Johannes
> 

Re: Branch deletion question / possible bug?

[Philip Oakley]

From: "Jacob Keller" <jacob.kel...@gmail.com>

On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S <pik.s.t...@boeing.com>
wrote:

Hi,

I discovered that I was able to delete the feature branch I was in, due
to some fat fingering on my part and case insensitivity.  I never
realized this could be done before.  A quick google search did not give
me a whole lot to work with...

Steps to reproduce:
1. Create a feature branch, "editCss"
2. git checkout master
3. git checkout editCSS
4. git checkout editCss
5. git branch -d editCSS



Are you running on a case-insensitive file system? What version of
git? I thought I recalled seeing commits to help avoid creating
branches of the same name with separate case when we know we're on a
file system which is case-insensitive..


Normally, it should have been impossible for a user to delete the branch
they're on.  And the deletion left me in a weird state that took a while
to dig out of.

I know this was a user error, but I was also wondering if this was a bug.


If we have not yet done this, I think we should. Long term this would
be fixed by using a separate format to store refs than the filesystem,
which has a few projects being worked on but none have been put into a
release.


Yes, this is an on-going problem on Windows and other case insentive
systems. At the moment the branch name becomes embedded as a file name, so
when Git requests details of a branch from the filesystem, it can get a case
insensitive equivalent. Meanwhile, internally Git is checking for equality
in a case sensitive [Linux] way with obvious consequences such as this - The
most obvious being when there is no "*" current branch marker in the branch
status list.

It's a bit tricky to fix (internally the name and the path are passed down
different call chains), and depends on how one expects the case
insensitivity to work - the kicker is when someone does an edit of the name
via the file system and expects Git to cope (i.e. devs knowing, or think
they know, too much detail ;-).

The refs can also get packed, so the "bad spelling" gets baked in.
Ultimately it probably means that GfW and other systems will need  a case
sensitivity check when opening paths...

Philip


Thanks,
Jake




Thanks,

Pik Tang

Re: Branch deletion question / possible bug?

[Jacob Keller]

On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S <pik.s.t...@boeing.com> wrote:
> Hi,
>
> I discovered that I was able to delete the feature branch I was in, due to 
> some fat fingering on my part and case insensitivity.  I never realized this 
> could be done before.  A quick google search did not give me a whole lot to 
> work with...
>
> Steps to reproduce:
> 1. Create a feature branch, "editCss"
> 2. git checkout master
> 3. git checkout editCSS
> 4. git checkout editCss
> 5. git branch -d editCSS
>

Are you running on a case-insensitive file system? What version of
git? I thought I recalled seeing commits to help avoid creating
branches of the same name with separate case when we know we're on a
file system which is case-insensitive..

> Normally, it should have been impossible for a user to delete the branch 
> they're on.  And the deletion left me in a weird state that took a while to 
> dig out of.
>
> I know this was a user error, but I was also wondering if this was a bug.

If we have not yet done this, I think we should. Long term this would
be fixed by using a separate format to store refs than the filesystem,
which has a few projects being worked on but none have been put into a
release.

Thanks,
Jake

>
>
> Thanks,
>
> Pik Tang
>

Branch deletion question / possible bug?

[Tang (US), Pik S]

Hi,

I discovered that I was able to delete the feature branch I was in, due to some 
fat fingering on my part and case insensitivity.  I never realized this could 
be done before.  A quick google search did not give me a whole lot to work 
with...  

Steps to reproduce:
1. Create a feature branch, "editCss"
2. git checkout master
3. git checkout editCSS
4. git checkout editCss
5. git branch -d editCSS

Normally, it should have been impossible for a user to delete the branch 
they're on.  And the deletion left me in a weird state that took a while to dig 
out of.

I know this was a user error, but I was also wondering if this was a bug.


Thanks,

Pik Tang

Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Totsten Bögershausen]

On 2018-04-26 19:23, Elijah Newren wrote:

On Thu, Apr 26, 2018 at 10:13 AM, Torsten Bögershausen  wrote:

Hm,
thanks for the report.
I don't have a high sierra box, but I can probably get one.
t0050 -should- pass automagically, so I feel that I can do something.
Unless someone is faster of course.


Sweet, thanks for taking a look.


Is it possible that  you run
debug=t verbose=t ./t0050-filesystem.sh
and send the output to me ?


Sure.  First, though, note that I can make it pass (or at least "not
ok...TODO known breakage") with the following patch (may be
whitespace-damaged by gmail):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 483c8d6d7..770b91f8c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1106,12 +1106,7 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 auml=$(printf "\303\244")
 aumlcdiar=$(printf "\141\314\210")
 >"$auml" &&
-   case "$(echo *)" in
-   "$aumlcdiar")
-   true ;;
-   *)
-   false ;;
-   esac
+   stat "$aumlcdiar" >/dev/null 2>/dev/null


Nicely analyzed and improved.

The "stat" statement is technically correct.
I think that a more git-style fix would be
[] ---
+   test -r "$aumlcdiar"

instead of the stat.

I looked into the 2 known breakages.
In short: they test use cases which are not sooo important for a user in 
practice, but do a good test if the code is broken.

IOW: I can't see a need for immediate action.

As you already did all the analyzes:
Do you want to send a patch ?

[PATCH v6 03/11] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.33.gfcbb1fa0445

Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Elijah Newren]

On Thu, Apr 26, 2018 at 10:13 AM, Torsten Bögershausen  wrote:
> Hm,
> thanks for the report.
> I don't have a high sierra box, but I can probably get one.
> t0050 -should- pass automagically, so I feel that I can do something.
> Unless someone is faster of course.

Sweet, thanks for taking a look.

> Is it possible that  you run
> debug=t verbose=t ./t0050-filesystem.sh
> and send the output to me ?

Sure.  First, though, note that I can make it pass (or at least "not
ok...TODO known breakage") with the following patch (may be
whitespace-damaged by gmail):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 483c8d6d7..770b91f8c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1106,12 +1106,7 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
auml=$(printf "\303\244")
aumlcdiar=$(printf "\141\314\210")
>"$auml" &&
-   case "$(echo *)" in
-   "$aumlcdiar")
-   true ;;
-   *)
-   false ;;
-   esac
+   stat "$aumlcdiar" >/dev/null 2>/dev/null
 '

 test_lazy_prereq AUTOIDENT '


I'm just worried there are bugs elsewhere in dealing with filesystems
like this that would need to be fixed and that this papers over them.

Anyway, the output you requested, at least for the last two failing tests, is:


expecting success:
git mv "$aumlcdiar" "$auml" &&
git commit -m rename

fatal: destination exists, source=ä, destination=ä
not ok 9 - rename (silent unicode normalization)

#
# git mv "$aumlcdiar" "$auml" &&
# git commit -m rename
#

expecting success:
git reset --hard initial &&
git merge topic

HEAD is now at 1b3caf6 initial
Updating 1b3caf6..2db1bf9
error: The following untracked working tree files would be overwritten by merge:
ä
Please move or remove them before you merge.
Aborting
not ok 10 - merge (silent unicode normalization)

#
# git reset --hard initial &&
# git merge topic
#

# still have 1 known breakage(s)
# failed 2 among remaining 9 test(s)

Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Torsten Bögershausen]

On 26.04.18 18:48, Elijah Newren wrote:
> On HFS (which appears to be the default Mac filesystem prior to High
> Sierra), unicode names are "normalized" before recording.  Thus with a
> script like:
> 
> mkdir tmp
> cd tmp
> 
> auml=$(printf "\303\244")
> aumlcdiar=$(printf "\141\314\210")
> >"$auml"
> 
> echo "auml:  " $(echo -n "$auml" | xxd)
> echo "aumlcdiar: " $(echo -n "$aumlcdiar" | xxd)
> echo "Dir contents:  " $(echo -n * | xxd)
> 
> echo "Stat auml: " "$(stat -f "%i   %Sm   %Su %N" "$auml")"
> echo "Stat aumlcdiar:" "$(stat -f "%i   %Sm   %Su %N" "$aumlcdiar")"
> 
> We see output like:
> 
> auml:   : c3a4 ..
> aumlcdiar:  : 61cc 88 a..
> Dir contents:   : 61cc 88 a..
> Stat auml:  857473   Apr 26 09:40:40 2018   newren ä
> Stat aumlcdiar: 857473   Apr 26 09:40:40 2018   newren ä
> 
> On APFS, which appears to be the new default filesystem in Mac OS High
> Sierra, we instead see:
> 
> auml:   : c3a4 ..
> aumlcdiar:  : 61cc 88 a..
> Dir contents:   : c3a4 ..
> Stat auml:  8591766636   Apr 26 09:40:59 2018   newren ä
> Stat aumlcdiar: 8591766636   Apr 26 09:40:59 2018   newren ä
> 
> i.e. APFS appears to record the filename as specified by the user, but
> continues to allow the user to access it via any name that normalizes
> to the same thing.  This difference causes t0050-filesystem.sh to fail
> the final two tests.  I could change the "UTF8_NFD_TO_NFC" flag
> checking in test-lib.sh to instead test the exit code of stat to make
> it pass these two tests, but I have no idea if there are problems
> elsewhere that this would just be papering over.
> 
> I dislike Mac OS and avoid it, so I'd prefer to find someone else
> motivated to fix this.  If no one is, I may eventually try to fix this
> up...in a year or three from now.  But is someone else interested?
> Would this serve as a good microproject for our microprojects list (or
> are the internals hairy enough that this is too big of a project for
> that list)?
> 
> 
> Elijah
> 

Hm,
thanks for the report.
I don't have a high sierra box, but I can probably get one.
t0050 -should- pass automagically, so I feel that I can do something.
Unless someone is faster of course.

Is it possible that  you run
debug=t verbose=t ./t0050-filesystem.sh 
and send the output to me ?

BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Elijah Newren]

On HFS (which appears to be the default Mac filesystem prior to High
Sierra), unicode names are "normalized" before recording.  Thus with a
script like:

mkdir tmp
cd tmp

auml=$(printf "\303\244")
aumlcdiar=$(printf "\141\314\210")
>"$auml"

echo "auml:  " $(echo -n "$auml" | xxd)
echo "aumlcdiar: " $(echo -n "$aumlcdiar" | xxd)
echo "Dir contents:  " $(echo -n * | xxd)

echo "Stat auml: " "$(stat -f "%i   %Sm   %Su %N" "$auml")"
echo "Stat aumlcdiar:" "$(stat -f "%i   %Sm   %Su %N" "$aumlcdiar")"

We see output like:

auml:   : c3a4 ..
aumlcdiar:  : 61cc 88 a..
Dir contents:   : 61cc 88 a..
Stat auml:  857473   Apr 26 09:40:40 2018   newren ä
Stat aumlcdiar: 857473   Apr 26 09:40:40 2018   newren ä

On APFS, which appears to be the new default filesystem in Mac OS High
Sierra, we instead see:

auml:   : c3a4 ..
aumlcdiar:  : 61cc 88 a..
Dir contents:   : c3a4 ..
Stat auml:  8591766636   Apr 26 09:40:59 2018   newren ä
Stat aumlcdiar: 8591766636   Apr 26 09:40:59 2018   newren ä

i.e. APFS appears to record the filename as specified by the user, but
continues to allow the user to access it via any name that normalizes
to the same thing.  This difference causes t0050-filesystem.sh to fail
the final two tests.  I could change the "UTF8_NFD_TO_NFC" flag
checking in test-lib.sh to instead test the exit code of stat to make
it pass these two tests, but I have no idea if there are problems
elsewhere that this would just be papering over.

I dislike Mac OS and avoid it, so I'd prefer to find someone else
motivated to fix this.  If no one is, I may eventually try to fix this
up...in a year or three from now.  But is someone else interested?
Would this serve as a good microproject for our microprojects list (or
are the internals hairy enough that this is too big of a project for
that list)?


Elijah

[PATCH v5 03/11] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v4 03/11] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.15.gaa56ade3205

[PATCH v3 03/11] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.15.gaa56ade3205

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> > Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.
> 
> I actually wanted to review the code leading to this commit, and to find
> where to start reviewing I had 'git grep "This is a combination of"' which
> lead me to the translation files.

Heh...

> s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
> s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Yes, I actually knew that, but my usage of a s/// as a shorthand for the
idea to replace `grep` with `test_i18ngrep` was indeed misleading. Sorry
about that, and thank you for helping me getting this right.

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Eric,

On Fri, 20 Apr 2018, Eric Sunshine wrote:

> On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine  
> wrote:
> > On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
> >  wrote:
> >> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> >> +   git rebase -i HEAD~4 &&
> >> +
> >> +   : now there is a conflict, and comments in the commit message &&
> >> +   git show HEAD >out &&
> >> +   grep "This is a combination of" out &&
> >> +
> >> +   : skip and continue &&
> >> +   git rebase --skip &&
> >
> > I see that this test script doesn't utilize it, but do you want a
> >
> > test_when_finished "reset_rebase" &&
> >
> > before starting the rebase to clean up in case something before "git
> > rebase --skip" fails?

No, because then one of the next test cases fails:

not ok 15 - rebase -i --continue remembers --rerere-autoupdate

;-)

I'll use

test_when_finished "test_might_fail git rebase --abort"

instead, okay? ;-)

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

Hi Johannes,

> Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.

I actually wanted to review the code leading to this commit, and to find
where to start reviewing I had 'git grep "This is a combination of"' which
lead me to the translation files.

s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Johannes Schindelin wrote:

> A brief test shows, however, that it is not quite as easy as
> s/grep/test_i18ngrep/, something more seems to be broken.

It seems that this week is my Rabbit Hole Week.

Turns out that we have a really, really long-standing bug in our rebase -i
where we construct the commit messages for fixup/squash chains.

Background: when having multiple fixup!/squash! commits for the same
original commit, the intermediate commits have messages starting with the
message

# This is a combination of  commits.

and then every fixup/squash command increments that  and adds a header

# This is the commit message #:

before writing the respective commit message.

The problem arises from the fact that we deduce  from parsing the first
number in ASCII encoding on the first line.

That breaks e.g. when compiling with GETTEXT_POISON, and it is probably
not true in general, either.

So I introduced a patch that handles the absence of an ASCII-encoded
number gracefully, and now the test passes with and without
GETTEXT_POISON.

Thanks for the review that let me find and fix this bug!
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
>  wrote:
> > When multiple fixup/squash commands are processed and the last one
> > causes merge conflicts and is skipped, we leave the "This is a
> > combination of ..." comments in the commit message.
> >
> > Noticed by Eric Sunshine.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  t/t3418-rebase-continue.sh | 21 +
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > index 9214d0bb511..b177baee322 100755
> > --- a/t/t3418-rebase-continue.sh
> > +++ b/t/t3418-rebase-continue.sh
> > @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy 
> > options correctly' '
> > git rebase --continue
> >  '
> >
> > +test_expect_failure '--continue after failed fixup cleans commit message' '
> > +   git checkout -b with-conflicting-fixup &&
> > +   test_commit wants-fixup &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> > +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> > +   git rebase -i HEAD~4 &&
> > +
> > +   : now there is a conflict, and comments in the commit message &&
> > +   git show HEAD >out &&
> > +   grep "This is a combination of" out &&
> 
> test_i18n_grep ?

Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded. So
I dug deeper why: I never understood that this is a *build* option. I
always thought that would be a test-time option... Oh well ;-)

> > +
> > +   : skip and continue &&
> > +   git rebase --skip &&
> > +
> > +   : now the comments in the commit message should have been cleaned 
> > up &&
> > +   git show HEAD >out &&
> > +   ! grep "This is a combination of" out
> 
> same

Will work on a fix. A brief test shows, however, that it is not quite as
easy as s/grep/test_i18ngrep/, something more seems to be broken.

Will keep you posted,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine  wrote:
> On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
>  wrote:
>> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
>> +   git rebase -i HEAD~4 &&
>> +
>> +   : now there is a conflict, and comments in the commit message &&
>> +   git show HEAD >out &&
>> +   grep "This is a combination of" out &&
>> +
>> +   : skip and continue &&
>> +   git rebase --skip &&
>
> I see that this test script doesn't utilize it, but do you want a
>
> test_when_finished "reset_rebase" &&
>
> before starting the rebase to clean up in case something before "git
> rebase --skip" fails?

Stated less ambiguously:

... in case something fails between "git rebase -i ..."
and "git rebase --skip"?

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
 wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options 
> correctly' '
> +test_expect_failure '--continue after failed fixup cleans commit message' '
> +   git checkout -b with-conflicting-fixup &&
> +   test_commit wants-fixup &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +   git rebase -i HEAD~4 &&
> +
> +   : now there is a conflict, and comments in the commit message &&
> +   git show HEAD >out &&
> +   grep "This is a combination of" out &&
> +
> +   : skip and continue &&
> +   git rebase --skip &&

I see that this test script doesn't utilize it, but do you want a

test_when_finished "reset_rebase" &&

before starting the rebase to clean up in case something before "git
rebase --skip" fails?

> +   : now the comments in the commit message should have been cleaned up 
> &&
> +   git show HEAD >out &&
> +   ! grep "This is a combination of" out
> +'

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
 wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Noticed by Eric Sunshine.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  t/t3418-rebase-continue.sh | 21 +
>  1 file changed, 21 insertions(+)
>
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 9214d0bb511..b177baee322 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options 
> correctly' '
> git rebase --continue
>  '
>
> +test_expect_failure '--continue after failed fixup cleans commit message' '
> +   git checkout -b with-conflicting-fixup &&
> +   test_commit wants-fixup &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +   git rebase -i HEAD~4 &&
> +
> +   : now there is a conflict, and comments in the commit message &&
> +   git show HEAD >out &&
> +   grep "This is a combination of" out &&

test_i18n_grep ?

> +
> +   : skip and continue &&
> +   git rebase --skip &&
> +
> +   : now the comments in the commit message should have been cleaned up 
> &&
> +   git show HEAD >out &&
> +   ! grep "This is a combination of" out

same

Thanks,
Stefan

RE: [BUG] Git fast-export with import marks file omits merge commits

[Isaac Chou]

Hi Martin,

No problem.  I was thinking of the peek/pop pattern as well.  :)  If you don't 
mind, can you please go ahead and submit a patch for this?  Thanks so much.

Isaac

-Original Message-
From: Martin Ågren [mailto:martin.ag...@gmail.com] 
Sent: Friday, April 20, 2018 1:08 AM
To: Junio C Hamano <gits...@pobox.com>
Cc: Isaac Chou <isaac.c...@microfocus.com>; Johannes Schindelin 
<johannes.schinde...@gmx.de>; Jonathan Tan <jonathanta...@google.com>; 
git@vger.kernel.org
Subject: Re: [BUG] Git fast-export with import marks file omits merge commits

On 20 April 2018 at 00:48, Junio C Hamano <gits...@pobox.com> wrote:
> Isaac Chou <isaac.c...@microfocus.com> writes:
>
>> I inspected the source code (builtin/fast-export.c) for the 
>> fast-export issue I encountered, and it looks like the merge commit 
>> is discarded too early by the call to object_array_pop() after only 
>> one of the two UNSHOWN parents is processed in the method 
>> handle_tail().  The poped merge commit still has one UNSHOWN parent, 
>> therefore it is not processed and is lost in the output.  Can someone 
>> advise me on how to submit a code change or bug report in order to 
>> get the fix into the code base?
>
> There indeed are some differences between v2.14 and v2.15 around the 
> code that returns early when has_unshown_parent() says "yes" [*1*], 
> but the decision to return early when the function says "yes" hasn't 
> changed between that timeperiod---it dates back to f2dc849e ("Add 'git 
> fast-export', the sister of 'git fast-import'", 2007-12-02), i.e. the 
> very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do 
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23), which 
> are the only two commits that touch the surrounding area during that 
> timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
> reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 
> d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
> return strbuf_detach(, len);  }
>
> -static void handle_tail(struct object_array *commits, struct rev_info 
> *revs)
> +static void handle_tail(struct object_array *commits, struct rev_info *revs,
> +   struct string_list *paths_of_changed_objects)
>  {
> struct commit *commit;
> while (commits->nr) {
> -   commit = (struct commit *)commits->objects[commits->nr - 
> 1].item;
> +   commit = (struct commit *)object_array_pop(commits);
> if (has_unshown_parent(commit))
> return;
> -   handle_commit(commit, revs);
> -   commits->nr--;
> +   handle_commit(commit, revs, paths_of_changed_objects);
> }
>  }

Indeed. This looks wrong and the guilty person would be me.

If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-09-23) would instead have done something like 
s/commits->nr--/(void)object_array_pop(commits)/ it would not have screwed up 
as much. It could also use a peek+pop-pattern.

Isaac, are you up for submitting a patch? Just let me know if you encounter any 
issues. Otherwise, I can submit a patch shortly since I was the one who dropped 
the ball originally.

Thanks for diagnosing this.

Martin

[PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

When multiple fixup/squash commands are processed and the last one
causes merge conflicts and is skipped, we leave the "This is a
combination of ..." comments in the commit message.

Noticed by Eric Sunshine.

Signed-off-by: Johannes Schindelin 
---
 t/t3418-rebase-continue.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 9214d0bb511..b177baee322 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options 
correctly' '
git rebase --continue
 '
 
+test_expect_failure '--continue after failed fixup cleans commit message' '
+   git checkout -b with-conflicting-fixup &&
+   test_commit wants-fixup &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
+   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
+   git rebase -i HEAD~4 &&
+
+   : now there is a conflict, and comments in the commit message &&
+   git show HEAD >out &&
+   grep "This is a combination of" out &&
+
+   : skip and continue &&
+   git rebase --skip &&
+
+   : now the comments in the commit message should have been cleaned up &&
+   git show HEAD >out &&
+   ! grep "This is a combination of" out
+'
+
 test_expect_success 'setup rerere database' '
rm -fr .git/rebase-* &&
git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.17.0.windows.1.15.gaa56ade3205

Re: Bug Report - Pull remote branch does not retrieve new tags

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 20 2018, Andrew Ducker wrote:

> Thanks Bryan, that does clear it up a bit.
>
> The reason that this came up is that Visual Studio Code has switched from 
> "git pull" to "git pull remote branch" when the "sync" button is clicked, and 
> this has meant that tags are no longer being fetched.
>
> What _does_ seem to work is adding "--tags" on the end of the git pull.  But 
> this isn't actually in the documentation[1], and I'm a bit nervous that this 
> is mid-deprecation.
>
> Is "--tags" going away shortly?  Or are they ok to depend on this?
>
> The bug is at https://github.com/Microsoft/vscode/issues/48211 if anyone 
> wants to chime in with advice over there :-)

It's in the documentation, it's just in the git-fetch documentation, and
the git-pull docs say:

More precisely, git pull runs git fetch with the given parameters

So --tags is not going away, however using --tags is likely not the
right thing either, because it'll also get tags that don't point to any
of the refs being tracked as the docs explain, which isn't what was
happening with "git pull".

As to what VS /should/ be doing, I have no idea because I don't know why
they switched away from "git pull" to "git pull remote branch" in the
first place. Maybe they'd like to clone with --single-branch?

Unfortunately there's no way to replicate that on an existing repo
without re-cloning, as my
https://public-inbox.org/git/874lkuvtve@evledraar.gmail.com/
explains.

>
>> -Original Message-
>> From: Bryan Turner [mailto:btur...@atlassian.com]V
>> Sent: 19 April 2018 23:14
>> To: Andrew Ducker
>> Cc: git@vger.kernel.org
>> Subject: Re: Bug Report - Pull remote branch does not retrieve new tags
>>
>> Andrew,
>>
>> On Thu, Apr 19, 2018 at 6:55 AM, Andrew Ducker
>> <andrew_duc...@standardlife.com> wrote:
>> >
>> > What happens:
>> > When I create a new tag on the remote (changing nothing else)
>> > "git pull origin master" produces the following:
>> >   From git.internal.company.com:team/testrepo
>> >* branchmaster -> FETCH_HEAD
>> >   Already up-to-date.
>> >
>> > If I instead do a "git pull" I get:
>> >   From git.internal.company.com:team/testrepo
>> >* [new tag] Testing11  -> Testing11
>> >   Already up-to-date.
>> >
>> > What I think should happen:
>> > The "git pull origin master" should retrieve the tag.
>> >
>> > This is with 2.16.2.windows.1, but also occurred on my previously installed
>> version (2.12.2.windows.2)
>> >
>> > My understanding is that "git pull" and "git pull $repo $currentbranch"
>> should function identically.
>> >
>> > Is this a bug, or am I misunderstanding the manual page?
>>
>> Looks like a misunderstanding, to me. Perhaps I can help clarify.
>>
>> "git pull" without arguments fetches from the "origin" repository
>> using the configured "fetch" refspecs, which typically looks something
>> like "fetch = +refs/heads/*:refs/remotes/origin/*". It _doesn't_
>> actually fetch all tags, but any tag referencing any object/commit
>> included in the branches is brought along for the ride. This is
>> documented on "git pull":
>>
>> --no-tags
>>
>> By default, tags that point at objects that are downloaded from
>> the remote repository are fetched and stored locally. This option
>> disables this automatic tag following. The default behavior for a
>> remote may be specified with the remote..tagOpt setting. See
>> git-config(1).
>>
>> By comparison, on your "git pull $repo $currentBranch", what you're
>> calling "$currentBranch" is actually "[...]" from the
>> documentation. In other words, by passing "master", you've told "git
>> pull" to fetch _nothing but "master"_, ignoring the configured
>> refspec(s). Additionally, since you haven't told "git pull" where to
>> _put_ "master" once it's fetched, it writes it to "FETCH_HEAD". If you
>> have a tracking branch setup, "git pull origin master" will also
>> update the tracking branch. For example, the same command for me
>> produces:
>>
>> $ git pull origin master
>> From ...
>>  * branchmaster -> FETCH_HEAD
>>aca5eb0fef5..ad484477508  master -> origin/master
>>
>> As you can see, both FETC

RE: Bug Report - Pull remote branch does not retrieve new tags

[Andrew Ducker]

Thanks Bryan, that does clear it up a bit.

The reason that this came up is that Visual Studio Code has switched from "git 
pull" to "git pull remote branch" when the "sync" button is clicked, and this 
has meant that tags are no longer being fetched.

What _does_ seem to work is adding "--tags" on the end of the git pull.  But 
this isn't actually in the documentation[1], and I'm a bit nervous that this is 
mid-deprecation.

Is "--tags" going away shortly?  Or are they ok to depend on this?

The bug is at https://github.com/Microsoft/vscode/issues/48211 if anyone wants 
to chime in with advice over there :-)

Thanks,

Andy

> -Original Message-
> From: Bryan Turner [mailto:btur...@atlassian.com]
> Sent: 19 April 2018 23:14
> To: Andrew Ducker
> Cc: git@vger.kernel.org
> Subject: Re: Bug Report - Pull remote branch does not retrieve new tags
>
> Andrew,
>
> On Thu, Apr 19, 2018 at 6:55 AM, Andrew Ducker
> <andrew_duc...@standardlife.com> wrote:
> >
> > What happens:
> > When I create a new tag on the remote (changing nothing else)
> > "git pull origin master" produces the following:
> >   From git.internal.company.com:team/testrepo
> >* branchmaster -> FETCH_HEAD
> >   Already up-to-date.
> >
> > If I instead do a "git pull" I get:
> >   From git.internal.company.com:team/testrepo
> >* [new tag] Testing11  -> Testing11
> >   Already up-to-date.
> >
> > What I think should happen:
> > The "git pull origin master" should retrieve the tag.
> >
> > This is with 2.16.2.windows.1, but also occurred on my previously installed
> version (2.12.2.windows.2)
> >
> > My understanding is that "git pull" and "git pull $repo $currentbranch"
> should function identically.
> >
> > Is this a bug, or am I misunderstanding the manual page?
>
> Looks like a misunderstanding, to me. Perhaps I can help clarify.
>
> "git pull" without arguments fetches from the "origin" repository
> using the configured "fetch" refspecs, which typically looks something
> like "fetch = +refs/heads/*:refs/remotes/origin/*". It _doesn't_
> actually fetch all tags, but any tag referencing any object/commit
> included in the branches is brought along for the ride. This is
> documented on "git pull":
>
> --no-tags
>
> By default, tags that point at objects that are downloaded from
> the remote repository are fetched and stored locally. This option
> disables this automatic tag following. The default behavior for a
> remote may be specified with the remote..tagOpt setting. See
> git-config(1).
>
> By comparison, on your "git pull $repo $currentBranch", what you're
> calling "$currentBranch" is actually "[...]" from the
> documentation. In other words, by passing "master", you've told "git
> pull" to fetch _nothing but "master"_, ignoring the configured
> refspec(s). Additionally, since you haven't told "git pull" where to
> _put_ "master" once it's fetched, it writes it to "FETCH_HEAD". If you
> have a tracking branch setup, "git pull origin master" will also
> update the tracking branch. For example, the same command for me
> produces:
>
> $ git pull origin master
> From ...
>  * branchmaster -> FETCH_HEAD
>aca5eb0fef5..ad484477508  master -> origin/master
>
> As you can see, both FETCH_HEAD and origin/master were updated, since
> my local "master" tracks "origin"'s "master":
>
> [branch "master"]
> remote = origin
> merge = refs/heads/master
>
> Hope this helps!
> Bryan
Confidentiality - This email is confidential.
Not meant for you? - If you don't think this email is meant for you, please let 
us know. Do not copy or forward the information it contains, and delete this 
email from your system.
Views expressed - Any personal views or opinions expressed in this email are 
the sender's, and do not necessarily reflect the views of Standard Life 
Aberdeen group.
Monitoring - We filter and monitor emails to protect our systems and to keep 
them running smoothly.
Emailing us - Email isn't a secure form of communication. If you want to send 
us confidential information please send it by post. However, if you do 
communicate with us by email on any subject, you are giving us permission to 
email you back.
Phoning us - Calls may be monitored and/or recorded to protect both you and us 
and help with our training. Call charges will vary.
Standard Life Aberdeen group - Standa

Re: [BUG] Git fast-export with import marks file omits merge commits

[Martin Ågren]

On 20 April 2018 at 00:48, Junio C Hamano <gits...@pobox.com> wrote:
> Isaac Chou <isaac.c...@microfocus.com> writes:
>
>> I inspected the source code (builtin/fast-export.c) for the
>> fast-export issue I encountered, and it looks like the merge
>> commit is discarded too early by the call to object_array_pop()
>> after only one of the two UNSHOWN parents is processed in the
>> method handle_tail().  The poped merge commit still has one
>> UNSHOWN parent, therefore it is not processed and is lost in the
>> output.  Can someone advise me on how to submit a code change or
>> bug report in order to get the fix into the code base?
>
> There indeed are some differences between v2.14 and v2.15 around the
> code that returns early when has_unshown_parent() says "yes" [*1*],
> but the decision to return early when the function says "yes" hasn't
> changed between that timeperiod---it dates back to f2dc849e ("Add
> 'git fast-export', the sister of 'git fast-import'", 2007-12-02),
> i.e. the very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23),
> which are the only two commits that touch the surrounding area
> during that timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
> reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
> return strbuf_detach(, len);
>  }
>
> -static void handle_tail(struct object_array *commits, struct rev_info *revs)
> +static void handle_tail(struct object_array *commits, struct rev_info *revs,
> +   struct string_list *paths_of_changed_objects)
>  {
> struct commit *commit;
> while (commits->nr) {
> -   commit = (struct commit *)commits->objects[commits->nr - 
> 1].item;
> +   commit = (struct commit *)object_array_pop(commits);
> if (has_unshown_parent(commit))
> return;
> -   handle_commit(commit, revs);
> -   commits->nr--;
> +   handle_commit(commit, revs, paths_of_changed_objects);
> }
>  }

Indeed. This looks wrong and the guilty person would be me.

If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-09-23) would instead have done something like
s/commits->nr--/(void)object_array_pop(commits)/ it would not have
screwed up as much. It could also use a peek+pop-pattern.

Isaac, are you up for submitting a patch? Just let me know if you
encounter any issues. Otherwise, I can submit a patch shortly since I
was the one who dropped the ball originally.

Thanks for diagnosing this.

Martin

Re: [BUG] Git fast-export with import marks file omits merge commits

[Junio C Hamano]

Isaac Chou <isaac.c...@microfocus.com> writes:

> I inspected the source code (builtin/fast-export.c) for the
> fast-export issue I encountered, and it looks like the merge
> commit is discarded too early by the call to object_array_pop()
> after only one of the two UNSHOWN parents is processed in the
> method handle_tail().  The poped merge commit still has one
> UNSHOWN parent, therefore it is not processed and is lost in the
> output.  Can someone advise me on how to submit a code change or
> bug report in order to get the fix into the code base?
>
> Thanks,
>
> Isaac
>
> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf 
> Of Isaac Chou
> Sent: Monday, April 16, 2018 3:58 PM
> To: git@vger.kernel.org
> Subject: Git fast-export with import marks file omits merge commits
>
> Hello,
>
> I came across a change of behavior with Git version 2.15 and later
> where the fast-export command would omit the merge commits.  The
> same use case works correctly with Git version 2.14 and older.
> ...

There indeed are some differences between v2.14 and v2.15 around the
code that returns early when has_unshown_parent() says "yes" [*1*],
but the decision to return early when the function says "yes" hasn't
changed between that timeperiod---it dates back to f2dc849e ("Add
'git fast-export', the sister of 'git fast-import'", 2007-12-02),
i.e. the very beginning of the program's life.

I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
not copy from modified file", 2017-09-20) and 71992039
("object_array: add and use `object_array_pop()`", 2017-09-23),
which are the only two commits that touch the surrounding area
during that timeperiod, to ask if something jumps at them.

Thanks.


[Footnotes]

*1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
reads like so:

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d412c0a8f3..2fb60d6d48 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
...
@@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
return strbuf_detach(, len);
 }
 
-static void handle_tail(struct object_array *commits, struct rev_info *revs)
+static void handle_tail(struct object_array *commits, struct rev_info *revs,
+   struct string_list *paths_of_changed_objects)
 {
struct commit *commit;
while (commits->nr) {
-   commit = (struct commit *)commits->objects[commits->nr - 
1].item;
+   commit = (struct commit *)object_array_pop(commits);
if (has_unshown_parent(commit))
return;
-   handle_commit(commit, revs);
-   commits->nr--;
+   handle_commit(commit, revs, paths_of_changed_objects);
}
 }

Re: [BUG] Git fast-export with import marks file omits merge commits

[Elijah Newren]

Hi Isaac,

On Thu, Apr 19, 2018 at 2:46 PM, Isaac Chou <isaac.c...@microfocus.com> wrote:
> I inspected the source code (builtin/fast-export.c) for the fast-export issue 
> I encountered, and it looks like the merge commit is discarded too early by 
> the call to object_array_pop() after only one of the two UNSHOWN parents is 
> processed in the method handle_tail().  The poped merge commit still has one 
> UNSHOWN parent, therefore it is not processed and is lost in the output.  Can 
> someone advise me on how to submit a code change or bug report in order to 
> get the fix into the code base?

Careful now, fast-export was also the location of my first patch.
It's easy to get addicted to contributing changes to git.  :-)

Inside the git.git repository, there are two files that explain the
basic process -- Documentation/SubmittingPatches and
Documentation/CodingGuidelines.  If they don't cover the process well,
that's probably a bug itself, but feel free to ask on the list if you
still run into questions.  Those documents can be slighly overwhelming
at first glance, but they've got good information.

Elijah

Re: Bug Report - Pull remote branch does not retrieve new tags

[Bryan Turner]

Andrew,

On Thu, Apr 19, 2018 at 6:55 AM, Andrew Ducker
<andrew_duc...@standardlife.com> wrote:
>
> What happens:
> When I create a new tag on the remote (changing nothing else)
> "git pull origin master" produces the following:
>   From git.internal.company.com:team/testrepo
>* branchmaster -> FETCH_HEAD
>   Already up-to-date.
>
> If I instead do a "git pull" I get:
>   From git.internal.company.com:team/testrepo
>* [new tag] Testing11  -> Testing11
>   Already up-to-date.
>
> What I think should happen:
> The "git pull origin master" should retrieve the tag.
>
> This is with 2.16.2.windows.1, but also occurred on my previously installed 
> version (2.12.2.windows.2)
>
> My understanding is that "git pull" and "git pull $repo $currentbranch" 
> should function identically.
>
> Is this a bug, or am I misunderstanding the manual page?

Looks like a misunderstanding, to me. Perhaps I can help clarify.

"git pull" without arguments fetches from the "origin" repository
using the configured "fetch" refspecs, which typically looks something
like "fetch = +refs/heads/*:refs/remotes/origin/*". It _doesn't_
actually fetch all tags, but any tag referencing any object/commit
included in the branches is brought along for the ride. This is
documented on "git pull":

--no-tags

By default, tags that point at objects that are downloaded from
the remote repository are fetched and stored locally. This option
disables this automatic tag following. The default behavior for a
remote may be specified with the remote..tagOpt setting. See
git-config(1).

By comparison, on your "git pull $repo $currentBranch", what you're
calling "$currentBranch" is actually "[...]" from the
documentation. In other words, by passing "master", you've told "git
pull" to fetch _nothing but "master"_, ignoring the configured
refspec(s). Additionally, since you haven't told "git pull" where to
_put_ "master" once it's fetched, it writes it to "FETCH_HEAD". If you
have a tracking branch setup, "git pull origin master" will also
update the tracking branch. For example, the same command for me
produces:

$ git pull origin master
>From ...
 * branchmaster -> FETCH_HEAD
   aca5eb0fef5..ad484477508  master -> origin/master

As you can see, both FETCH_HEAD and origin/master were updated, since
my local "master" tracks "origin"'s "master":

[branch "master"]
remote = origin
merge = refs/heads/master

Hope this helps!
Bryan

RE: [BUG] Git fast-export with import marks file omits merge commits

[Isaac Chou]

I inspected the source code (builtin/fast-export.c) for the fast-export issue I 
encountered, and it looks like the merge commit is discarded too early by the 
call to object_array_pop() after only one of the two UNSHOWN parents is 
processed in the method handle_tail().  The poped merge commit still has one 
UNSHOWN parent, therefore it is not processed and is lost in the output.  Can 
someone advise me on how to submit a code change or bug report in order to get 
the fix into the code base?

Thanks,

Isaac

-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of 
Isaac Chou
Sent: Monday, April 16, 2018 3:58 PM
To: git@vger.kernel.org
Subject: Git fast-export with import marks file omits merge commits

Hello,

I came across a change of behavior with Git version 2.15 and later where the 
fast-export command would omit the merge commits.  The same use case works 
correctly with Git version 2.14 and older.  Here is the detail of the use case:

0> git --version
git version 2.16.2.windows.1

1> git init
Initialized empty Git repository in c:/./.git/

2> echo  >> file.txt

3> git add file.txt

4> git commit -m "first commit"
[master (root-commit) 711d4d5] first commit
1 file changed, 1 insertion(+)
create mode 100644 file.txt

5> git checkout -b test
Switched to a new branch 'test'

6> echo  >> file.txt

7> git add file.txt

8> git commit -m "commit on test branch"
[test 76d231c] commit on test branch
1 file changed, 1 insertion(+)

9> git checkout master
Switched to branch 'master'

10> echo  >> file.txt

11> git add file.txt

12> git commit -m "commit on master branch"
[master 61c55fd] commit on master branch
1 file changed, 1 insertion(+)

13> git merge test
Auto-merging file.txt
CONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix 
conflicts and then commit the result.

14> notepad file.txt

15> git add file.txt

16> git commit -m "merged with test branch"
[master 1442e0e] merged with test branch

17> git log
commit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master)
Merge: 61c55fd 76d231c
Author: isaac <...>
Date:   Mon Apr 16 15:08:39 2018 -0400

    merged with test branch

commit 61c55fdb883fc403e63c91b49bc11bdade62b3e8
Author: isaac <...>
Date:   Mon Apr 16 15:07:41 2018 -0400

    commit on master branch

commit 76d231cdb12eb84f45abdebede06a56f912613d3 (test)
Author: isaac <...>
Date:   Mon Apr 16 15:07:07 2018 -0400

    commit on test branch

commit 711d4d5781df41924421f8629af040e7c91a8d2e
Author: isaac <...>
Date:   Mon Apr 16 15:06:07 2018 -0400

    first commit

18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks

19> cat git-marks
:1 711d4d5781df41924421f8629af040e7c91a8d2e

20> git fast-export --use-done-feature --import-marks=git-marks 
refs/heads/master --
feature done
blob
mark :2
data 12



commit refs/heads/master
mark :3
author isaac <...> 1523905627 -0400
committer isaac <...> 1523905627 -0400
data 22
commit on test branch
from :1
M 100644 :2 file.txt

blob
mark :4
data 12



commit refs/heads/master
mark :5
author isaac <...> 1523905661 -0400
committer isaac <...> 1523905661 -0400
data 24
commit on master branch
from :1
M 100644 :4 file.txt

done

Thanks,

Isaac

Bug Report - Pull remote branch does not retrieve new tags

[Andrew Ducker]

What happens:
When I create a new tag on the remote (changing nothing else)
"git pull origin master" produces the following:
  From git.internal.company.com:team/testrepo
   * branchmaster -> FETCH_HEAD
  Already up-to-date.

If I instead do a "git pull" I get:
  From git.internal.company.com:team/testrepo
   * [new tag] Testing11  -> Testing11
  Already up-to-date.

What I think should happen:
The "git pull origin master" should retrieve the tag.

This is with 2.16.2.windows.1, but also occurred on my previously installed 
version (2.12.2.windows.2)

My understanding is that "git pull" and "git pull $repo $currentbranch" should 
function identically.

Is this a bug, or am I misunderstanding the manual page?

Thanks,

Andy
Confidentiality - This email is confidential.
Not meant for you? - If you don't think this email is meant for you, please let 
us know. Do not copy or forward the information it contains, and delete this 
email from your system.
Views expressed - Any personal views or opinions expressed in this email are 
the sender's, and do not necessarily reflect the views of Standard Life 
Aberdeen group.
Monitoring - We filter and monitor emails to protect our systems and to keep 
them running smoothly.
Emailing us - Email isn't a secure form of communication. If you want to send 
us confidential information please send it by post. However, if you do 
communicate with us by email on any subject, you are giving us permission to 
email you back.
Phoning us - Calls may be monitored and/or recorded to protect both you and us 
and help with our training. Call charges will vary.
Standard Life Aberdeen group - Standard Life Aberdeen group comprises Standard 
Life Aberdeen plc and its subsidiaries. For more information on Standard Life 
Aberdeen group visit our website http://www.standardlifeaberdeen.com/.
Standard Life Aberdeen plc (SC286832), Standard Life Assurance Limited 
(SC286833) and Standard Life Employee Services Limited (SC271355) are all 
registered in Scotland at Standard Life House, 30 Lothian Road, Edinburgh EH1 
2DH. Standard Life Assurance Limited is authorised by the Prudential Regulation 
Authority and regulated by the Financial Conduct Authority and the Prudential 
Regulation Authority.
For more information on Standard Life Assurance limited visit our website 
http://www.standardlife.co.uk

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 16/04/18 10:48, Phillip Wood wrote:
> On 14/04/18 14:11, Johannes Schindelin wrote:
>> Hi,
>>
>> On Sat, 14 Apr 2018, Phillip Wood wrote:
>>
>> FWIW I agree with Hannes' patch.
>>
>>> I think 'git am' probably gives all patches the same commit time as well
>>> if the commit date is cached though it wont suffer from the time-travel
>>> problem.
>>
>> I thought that `git am` was the subject of such a complaint recently, but
>> I thought that had been resolved? Apparently I misremember...
> 
> I had a quick look and couldn't see anything about that, it looks to me
> like it just calls commit_tree() and only does anything to change the
> default commit date if '--committer-date-is-author-date' was given.

Ah you were right, I just didn't look far enough back in the history
.(it's a shame it calls reset_ident_date() in a different function to
the one that creates the commit otherwise I would probably have noticed
it when I wrote the original patches)

Best Wishes

Phillip

> 
> Best Wishes
> 
> Phillip
>> Ciao,
>> Dscho
>>
> 

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?
--> Correct.

Does that happen with the 17.2 version of p4?
-->Correct.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way)
-->Sure.

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.
-->Most of other places are already doing key check in the hash. Looks
like this line was missed out.

On Wed, Apr 18, 2018 at 4:08 AM, Luke Diamand  wrote:
> On 17 April 2018 at 20:12, Thandesha VK  wrote:
>> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
>> the size value even with latest version of p4 (17.2). In that case, we
>> have to regenerate the digest for file save it - It mean something is
>> wrong with the file in perforce.
>
> Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.
>
> We are just talking about the output from "p4 print" and the
> "fileSize" key, right?
>
> Does that happen with the 17.2 version of p4?
>
>> Regarding, sys.stdout.write v/s print, I see script using both of them
>> without a common pattern. I can change it to whatever is more
>> appropriate.
>
> print() probably makes more sense; can we try to use the function form
> so that we don't deliberately make the path to python3 harder (albeit
> in a very tiny way).
>
>>
>> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
>>> Does a missing "fileSize" actually mean that there is something wrong with 
>>> the file?
>>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>>> (which I attribute to our rather ancient (2007.2) Perforce server)
>>> I'm not an expert in Perforce, so don't know for sure.
>
> My 2015 version of p4d reports a fileSize.
>
>>>
>>> However, `p4 -G sizes` works fine even with our p4 server.
>>> Should we then go one step further and use `p4 -G sizes` to obtain the 
>>> "fileSize" when it's not returned by `p4 -G print`?
>>> Or is it an overkill for a simple verbose print out?
>
> If your server isn't reporting "fileSize" then there are a few other
> places where I would expect git-p4 to also fail.
>
> If we're going to support this very ancient version of p4d, then
> gracefully handling a missing fileSize will be useful.
>
>>>
>>> Also, please, find one comment inline below.
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Thandesha VK 
 Sounds good. How about an enhanced version of fix from both of us.
 This will let us know that something is not right with the file but
 will not bark

 $ git diff
 diff --git a/git-p4.py b/git-p4.py
 index 7bb9cadc6..df901976f 100755
 --- a/git-p4.py
 +++ b/git-p4.py
 @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
  relPath = self.stripRepoPath(file['depotFile'], 
 self.branchPrefixes)
  relPath = self.encodeWithUTF8(relPath)
  if verbose:
 -size = int(self.stream_file['fileSize'])
 +if 'fileSize' not in self.stream_file:
 +   print "WARN: File size from perforce unknown. Please 
 verify by p4 sizes %s" %(file['depotFile'])
>>> For whatever reason, the code below uses sys.stdout.write() instead of 
>>> print().
>>> Should it be used here for consistency as well?
>>>
 +   size = "-1"
 +else:
 +   size = self.stream_file['fileSize']
 +size = int(size)
  sys.stdout.write('\r%s --> %s (%i MB)\n' %
 (file['depotFile'], relPath, size/1024/1024))
  sys.stdout.flush()


 On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  
 wrote:
> Sure, I totally agree.
> Sorry, I just wasn't clear enough in my previous email.
> I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
> "fileSize" is not available,
> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
> known.
> In other words,
>  * if "fileSize" is known:
>  ** both yours and mine patches don't change existing behavior;
>  * if "fileSize" is not known:
>  ** your patch makes streamOneP4File() not print anything;
>  ** my patch makes streamOneP4File() print "%s --> %s".
>
> Hope, I'm clearer this time.
>
> Thank you,
> Andrey
>
> From: Thandesha VK 
>> *I* think keeping the filesize info is better with --verbose option as
>> that gives some clue about the file we are working on. What do you
>> think?
>> Script has similar checks of key existence at other places where it is
>> looking for fileSize.
>>
>> On Tue, Apr 17, 2018 at 9:22 AM, 

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Luke Diamand]

On 17 April 2018 at 20:12, Thandesha VK  wrote:
> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
> the size value even with latest version of p4 (17.2). In that case, we
> have to regenerate the digest for file save it - It mean something is
> wrong with the file in perforce.

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?

Does that happen with the 17.2 version of p4?

> Regarding, sys.stdout.write v/s print, I see script using both of them
> without a common pattern. I can change it to whatever is more
> appropriate.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way).

>
> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
>> Does a missing "fileSize" actually mean that there is something wrong with 
>> the file?
>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>> (which I attribute to our rather ancient (2007.2) Perforce server)
>> I'm not an expert in Perforce, so don't know for sure.

My 2015 version of p4d reports a fileSize.

>>
>> However, `p4 -G sizes` works fine even with our p4 server.
>> Should we then go one step further and use `p4 -G sizes` to obtain the 
>> "fileSize" when it's not returned by `p4 -G print`?
>> Or is it an overkill for a simple verbose print out?

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.

If we're going to support this very ancient version of p4d, then
gracefully handling a missing fileSize will be useful.

>>
>> Also, please, find one comment inline below.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK 
>>> Sounds good. How about an enhanced version of fix from both of us.
>>> This will let us know that something is not right with the file but
>>> will not bark
>>>
>>> $ git diff
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 7bb9cadc6..df901976f 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>>  relPath = self.stripRepoPath(file['depotFile'], 
>>> self.branchPrefixes)
>>>  relPath = self.encodeWithUTF8(relPath)
>>>  if verbose:
>>> -size = int(self.stream_file['fileSize'])
>>> +if 'fileSize' not in self.stream_file:
>>> +   print "WARN: File size from perforce unknown. Please verify 
>>> by p4 sizes %s" %(file['depotFile'])
>> For whatever reason, the code below uses sys.stdout.write() instead of 
>> print().
>> Should it be used here for consistency as well?
>>
>>> +   size = "-1"
>>> +else:
>>> +   size = self.stream_file['fileSize']
>>> +size = int(size)
>>>  sys.stdout.write('\r%s --> %s (%i MB)\n' %
>>> (file['depotFile'], relPath, size/1024/1024))
>>>  sys.stdout.flush()
>>>
>>>
>>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
 Sure, I totally agree.
 Sorry, I just wasn't clear enough in my previous email.
 I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
 "fileSize" is not available,
 while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
 known.
 In other words,
  * if "fileSize" is known:
  ** both yours and mine patches don't change existing behavior;
  * if "fileSize" is not known:
  ** your patch makes streamOneP4File() not print anything;
  ** my patch makes streamOneP4File() print "%s --> %s".

 Hope, I'm clearer this time.

 Thank you,
 Andrey

 From: Thandesha VK 
> *I* think keeping the filesize info is better with --verbose option as
> that gives some clue about the file we are working on. What do you
> think?
> Script has similar checks of key existence at other places where it is
> looking for fileSize.
>
> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
>> Huh, I actually have a slightly different fix for the same issue.
>> It doesn't suppress the corresponding verbose output completely, but 
>> just removes the size information from it.
>>
>> Also, I'd mention that the workaround is trivial -- simply omit the 
>> "--verbose" option.
>>
>> Andrey Mazo (1):
>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>
>>  git-p4.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>

Thanks
Luke

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 16/04/18 06:56, Johannes Sixt wrote:
> 
> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:
>> Ah, do you mean we have an internal sequence like this, when "rebase
>> --continue" wants to conclude an edit/reword?
> 
> Yes, it's only 'reword' that is affected, because then subsequent picks
> are processed by the original process.
> 
>>   - we figure out the committer ident, which grabs a timestamp and
>>     cache it;
>>
>>   - we spawn "commit" to conclude the stopped step, letting it record
>>     its beginning time (which is a bit older than the above) or its
>>     ending time (which is much older due to human typing speed);
> 
> Younger in both cases, of course. According to my tests, we seem to pick
> the beginning time, because the first 'reword'ed commit typically has
> the same timestamp as the preceding picks. Later 'reword'ed commits have
> noticably younger timestamps.
> 
>>   - subsequent "picks" are made in the same process, and share the
>>     timestamp we grabbed in the first step, which is older than the
>>     second one.
>>
>> I guess we'd want a mechanism to tell ident.c layer "discard the
>> cached one, as we are no longer in the same automated sequence", and
>> use that whenever we spawn an editor (or otherwise go interactive).
> 
> Frankly, I think that this caching is overengineered (or prematurly
> optimized). If the design requires that different callers of datestamp()
> must see the same time, then the design is broken. In a fixed design,
> there would be a single call of datestamp() in advance, and then the
> timestamp, which then obviously is a very important piece of data, would
> be passed along as required.

I'm inclined to agree, though it creates complications if we're going to
keep giving commits the same author and committer dates when neither is
explicitly specified.

Best Wishes

Phillip

> 
> -- Hannes

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

I have few cases where even p4 -G sizes (or p4 sizes) is not returning
the size value even with latest version of p4 (17.2). In that case, we
have to regenerate the digest for file save it - It mean something is
wrong with the file in perforce.
Regarding, sys.stdout.write v/s print, I see script using both of them
without a common pattern. I can change it to whatever is more
appropriate.

On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
> Does a missing "fileSize" actually mean that there is something wrong with 
> the file?
> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
> (which I attribute to our rather ancient (2007.2) Perforce server)
> I'm not an expert in Perforce, so don't know for sure.
>
> However, `p4 -G sizes` works fine even with our p4 server.
> Should we then go one step further and use `p4 -G sizes` to obtain the 
> "fileSize" when it's not returned by `p4 -G print`?
> Or is it an overkill for a simple verbose print out?
>
> Also, please, find one comment inline below.
>
> Thank you,
> Andrey
>
> From: Thandesha VK 
>> Sounds good. How about an enhanced version of fix from both of us.
>> This will let us know that something is not right with the file but
>> will not bark
>>
>> $ git diff
>> diff --git a/git-p4.py b/git-p4.py
>> index 7bb9cadc6..df901976f 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>  relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
>>  relPath = self.encodeWithUTF8(relPath)
>>  if verbose:
>> -size = int(self.stream_file['fileSize'])
>> +if 'fileSize' not in self.stream_file:
>> +   print "WARN: File size from perforce unknown. Please verify 
>> by p4 sizes %s" %(file['depotFile'])
> For whatever reason, the code below uses sys.stdout.write() instead of 
> print().
> Should it be used here for consistency as well?
>
>> +   size = "-1"
>> +else:
>> +   size = self.stream_file['fileSize']
>> +size = int(size)
>>  sys.stdout.write('\r%s --> %s (%i MB)\n' %
>> (file['depotFile'], relPath, size/1024/1024))
>>  sys.stdout.flush()
>>
>>
>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
>>> Sure, I totally agree.
>>> Sorry, I just wasn't clear enough in my previous email.
>>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
>>> "fileSize" is not available,
>>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
>>> known.
>>> In other words,
>>>  * if "fileSize" is known:
>>>  ** both yours and mine patches don't change existing behavior;
>>>  * if "fileSize" is not known:
>>>  ** your patch makes streamOneP4File() not print anything;
>>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>>
>>> Hope, I'm clearer this time.
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Thandesha VK 
 *I* think keeping the filesize info is better with --verbose option as
 that gives some clue about the file we are working on. What do you
 think?
 Script has similar checks of key existence at other places where it is
 looking for fileSize.

 On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
> Huh, I actually have a slightly different fix for the same issue.
> It doesn't suppress the corresponding verbose output completely, but just 
> removes the size information from it.
>
> Also, I'd mention that the workaround is trivial -- simply omit the 
> "--verbose" option.
>
> Andrey Mazo (1):
>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>
>  git-p4.py | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
>
> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
> --
> 2.16.1
>

 --
 Thanks & Regards
 Thandesha VK | Cellphone +1 (703) 459-5386
>>
>>
>>
>> --
>> Thanks & Regards
>> Thandesha VK | Cellphone +1 (703) 459-5386



-- 
Thanks & Regards
Thandesha VK | Cellphone +1 (703) 459-5386

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Mazo, Andrey]

Does a missing "fileSize" actually mean that there is something wrong with the 
file?
Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
(which I attribute to our rather ancient (2007.2) Perforce server)
I'm not an expert in Perforce, so don't know for sure.

However, `p4 -G sizes` works fine even with our p4 server.
Should we then go one step further and use `p4 -G sizes` to obtain the 
"fileSize" when it's not returned by `p4 -G print`?
Or is it an overkill for a simple verbose print out?

Also, please, find one comment inline below.

Thank you,
Andrey

From: Thandesha VK 
> Sounds good. How about an enhanced version of fix from both of us.
> This will let us know that something is not right with the file but
> will not bark
> 
> $ git diff
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..df901976f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
>  relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
>  relPath = self.encodeWithUTF8(relPath)
>  if verbose:
> -    size = int(self.stream_file['fileSize'])
> +    if 'fileSize' not in self.stream_file:
> +   print "WARN: File size from perforce unknown. Please verify 
> by p4 sizes %s" %(file['depotFile'])
For whatever reason, the code below uses sys.stdout.write() instead of print().
Should it be used here for consistency as well?

> +   size = "-1"
> +    else:
> +   size = self.stream_file['fileSize']
> +    size = int(size)
>  sys.stdout.write('\r%s --> %s (%i MB)\n' %
> (file['depotFile'], relPath, size/1024/1024))
>  sys.stdout.flush()
> 
> 
> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
>> Sure, I totally agree.
>> Sorry, I just wasn't clear enough in my previous email.
>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
>> "fileSize" is not available,
>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
>> known.
>> In other words,
>>  * if "fileSize" is known:
>>  ** both yours and mine patches don't change existing behavior;
>>  * if "fileSize" is not known:
>>  ** your patch makes streamOneP4File() not print anything;
>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>
>> Hope, I'm clearer this time.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK 
>>> *I* think keeping the filesize info is better with --verbose option as
>>> that gives some clue about the file we are working on. What do you
>>> think?
>>> Script has similar checks of key existence at other places where it is
>>> looking for fileSize.
>>>
>>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
 Huh, I actually have a slightly different fix for the same issue.
 It doesn't suppress the corresponding verbose output completely, but just 
 removes the size information from it.

 Also, I'd mention that the workaround is trivial -- simply omit the 
 "--verbose" option.

 Andrey Mazo (1):
   git-p4: fix `sync --verbose` traceback due to 'fileSize'

  git-p4.py | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)


 base-commit: 468165c1d8a442994a825f3684528361727cd8c0
 --
 2.16.1

>>>
>>> --
>>> Thanks & Regards
>>> Thandesha VK | Cellphone +1 (703) 459-5386
>
>
>
> -- 
> Thanks & Regards
> Thandesha VK | Cellphone +1 (703) 459-5386

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

Sounds good. How about an enhanced version of fix from both of us.
This will let us know that something is not right with the file but
will not bark

$ git diff
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc6..df901976f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
 relPath = self.encodeWithUTF8(relPath)
 if verbose:
-size = int(self.stream_file['fileSize'])
+if 'fileSize' not in self.stream_file:
+   print "WARN: File size from perforce unknown. Please
verify by p4 sizes %s" %(file['depotFile'])
+   size = "-1"
+else:
+   size = self.stream_file['fileSize']
+size = int(size)
 sys.stdout.write('\r%s --> %s (%i MB)\n' %
(file['depotFile'], relPath, size/1024/1024))
 sys.stdout.flush()


On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
> Sure, I totally agree.
> Sorry, I just wasn't clear enough in my previous email.
> I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
> "fileSize" is not available,
> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
> known.
> In other words,
>  * if "fileSize" is known:
>  ** both yours and mine patches don't change existing behavior;
>  * if "fileSize" is not known:
>  ** your patch makes streamOneP4File() not print anything;
>  ** my patch makes streamOneP4File() print "%s --> %s".
>
> Hope, I'm clearer this time.
>
> Thank you,
> Andrey
>
> From: Thandesha VK 
>> *I* think keeping the filesize info is better with --verbose option as
>> that gives some clue about the file we are working on. What do you
>> think?
>> Script has similar checks of key existence at other places where it is
>> looking for fileSize.
>>
>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
>>> Huh, I actually have a slightly different fix for the same issue.
>>> It doesn't suppress the corresponding verbose output completely, but just 
>>> removes the size information from it.
>>>
>>> Also, I'd mention that the workaround is trivial -- simply omit the 
>>> "--verbose" option.
>>>
>>> Andrey Mazo (1):
>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>
>>>  git-p4.py | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>>
>>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>>> --
>>> 2.16.1
>>>
>>
>> --
>> Thanks & Regards
>> Thandesha VK | Cellphone +1 (703) 459-5386



-- 
Thanks & Regards
Thandesha VK | Cellphone +1 (703) 459-5386

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Mazo, Andrey]

Sure, I totally agree.
Sorry, I just wasn't clear enough in my previous email.
I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" 
is not available,
while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
In other words,
 * if "fileSize" is known:
 ** both yours and mine patches don't change existing behavior;
 * if "fileSize" is not known:
 ** your patch makes streamOneP4File() not print anything;
 ** my patch makes streamOneP4File() print "%s --> %s".

Hope, I'm clearer this time.
 
Thank you,
Andrey

From: Thandesha VK 
> *I* think keeping the filesize info is better with --verbose option as
> that gives some clue about the file we are working on. What do you
> think?
> Script has similar checks of key existence at other places where it is
> looking for fileSize.
> 
> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
>> Huh, I actually have a slightly different fix for the same issue.
>> It doesn't suppress the corresponding verbose output completely, but just 
>> removes the size information from it.
>>
>> Also, I'd mention that the workaround is trivial -- simply omit the 
>> "--verbose" option.
>>
>> Andrey Mazo (1):
>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>
>>  git-p4.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>>
>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>> --
>> 2.16.1
>>
> 
> -- 
> Thanks & Regards
> Thandesha VK | Cellphone +1 (703) 459-5386

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

*I* think keeping the filesize info is better with --verbose option as
that gives some clue about the file we are working on. What do you
think?
Script has similar checks of key existence at other places where it is
looking for fileSize.

On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
> Huh, I actually have a slightly different fix for the same issue.
> It doesn't suppress the corresponding verbose output completely, but just 
> removes the size information from it.
>
> Also, I'd mention that the workaround is trivial -- simply omit the 
> "--verbose" option.
>
> Andrey Mazo (1):
>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>
>  git-p4.py | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
>
> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
> --
> 2.16.1
>



-- 
Thanks & Regards
Thandesha VK | Cellphone +1 (703) 459-5386

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Mazo, Andrey]

Huh, I actually have a slightly different fix for the same issue.
It doesn't suppress the corresponding verbose output completely, but just 
removes the size information from it.
I'll (try to) post it as a reply to this email.

Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" 
option.

Thank you,
Andrey

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Andrey Mazo]

Huh, I actually have a slightly different fix for the same issue.
It doesn't suppress the corresponding verbose output completely, but just 
removes the size information from it.

Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" 
option.

Andrey Mazo (1):
  git-p4: fix `sync --verbose` traceback due to 'fileSize'

 git-p4.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 468165c1d8a442994a825f3684528361727cd8c0
-- 
2.16.1

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Junio C Hamano]

Johannes Sixt  writes:

> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:
>> Ah, do you mean we have an internal sequence like this, when "rebase
>> --continue" wants to conclude an edit/reword?
>
> Yes, it's only 'reword' that is affected, because then subsequent
> picks are processed by the original process.

Ah, OK, that is a good explanation.

[BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

git p4 clone fails when p4 sizes does not return 'fileSize' key. There
are few cases when p4 sizes returens 0 size and with marshaled output,
it doesn’t return the fileSize attribute.

Here is the demonstration and potential fix



$ cd /tmp/git/



$ git remote -v

origin  https://github.com/git/git.git (fetch)

origin  https://github.com/git/git.git (push)



$ git branch  -v

* master fe0a9eaf3 Merge branch 'svn/authors-prog-2' of
git://bogomips.org/git-svn



Problem:



$ /tmp/git/git-p4.py clone //depot//@all   . –verbose

.

.

.

Traceback (most recent call last):

  File "/tmp/git/git-p4.py", line 3840, in 

main()

  File "/tmp/git/git-p4.py", line 3834, in main

if not cmd.run(args):

  File "/tmp/git/git-p4.py", line 3706, in run

if not P4Sync.run(self, depotPaths):

  File "/tmp/git/git-p4.py", line 3568, in run

self.importChanges(changes)

  File "/tmp/git/git-p4.py", line 3240, in importChanges

self.initialParent)

  File "/tmp/git/git-p4.py", line 2858, in commit

self.streamP4Files(files)

  File "/tmp/git/git-p4.py", line 2750, in streamP4Files

cb=streamP4FilesCbSelf)

  File "/tmp/git/git-p4.py", line 552, in p4CmdList

cb(entry)

  File "/tmp/git/git-p4.py", line 2744, in streamP4FilesCbSelf

self.streamP4FilesCb(entry)

  File "/tmp/git/git-p4.py", line 2692, in streamP4FilesCb

self.streamOneP4File(self.stream_file, self.stream_contents)

  File "/tmp/git/git-p4.py", line 2569, in streamOneP4File

size = int(self.stream_file['fileSize'])

KeyError: 'fileSize'



Signature of the sizes output resulting in this problem:

$ p4 -p   sizes //foo.c

//foo.c#5  bytes



$ p4 -p   -G sizes //foo.c

{scodesstatsdepotFiles4//fooc.c50



Signature for a file without problem:



$ p4 -p   sizes //bar.c

//bar.c#5 1105 bytes



$ p4 -p  -G  sizes //bar.c

{scodesstatsdepotFiles;//bar.csrevs5fileSizes11050



Patch:

$ git diff

diff --git a/git-p4.py b/git-p4.py

index 7bb9cadc6..f908e805e 100755

--- a/git-p4.py

+++ b/git-p4.py

@@ -2565,7 +2565,7 @@ class P4Sync(Command, P4UserMap):

 def streamOneP4File(self, file, contents):

 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)

 relPath = self.encodeWithUTF8(relPath)

-if verbose:

+if verbose and 'fileSize' in self.stream_file:

 size = int(self.stream_file['fileSize'])

 sys.stdout.write('\r%s --> %s (%i MB)\n' %
(file['depotFile'], relPath, size/1024/1024))

 sys.stdout.flush()



Thanks & Regards

Thandesha

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 14/04/18 14:11, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 14 Apr 2018, Phillip Wood wrote:
> 
>> On 13/04/18 17:52, Johannes Sixt wrote:
>>>
>>> I just noticed that all commits in a 70-commit branch have the same
>>> committer timestamp. This is very unusual on Windows, where rebase -i of
>>> such a long branch takes more than one second (but not more than 3 or
>>> so thanks to the builtin nature of the command!).
>>>
>>> And, in fact, if you mark some commits with 'reword' to delay the quick
>>> processing of the patches, then the reworded commits have later time
>>> stamps, but subsequent not reworded commits receive the earlier time
>>> stamp. This is clearly not intended.
>>
>> Oh dear, I think this is probably due to my series making rebase commit
>> in-process when the commit message isn't being edited. I didn't realize
>> that git cached the commit date rather than using the current time when
>> calling commit_tree_extended(). I'll take a look at it next week.
> 
> Thanks.
> 
> However, a quick lock at `git log @{u}.. --format=%ct` in my
> `sequencer-shears` branch thicket (which I rebase frequently on top of
> upstream's `master` using the last known-good `rebase-merges` sub-branch)
> shows that the commits have different-enough commit timestamps. (It is
> satisfying to see that multiple commits were made during the same second,
> of course.)
> 
> So while I cannot find anything in the code that disagrees with Hannes'
> assessment, it looks on the surface as if I did not encounter the bug
> here.
> 
> Curious.

That's strange (I'd have expected the picks after recreated merges to
have the earlier timestamps than the merge), if I do 'git rebase -i
--force-rebase --exec="sleep 2" @~5' then all the new commits have the
same timestamp.

> FWIW I agree with Hannes' patch.
> 
>> I think 'git am' probably gives all patches the same commit time as well
>> if the commit date is cached though it wont suffer from the time-travel
>> problem.
> 
> I thought that `git am` was the subject of such a complaint recently, but
> I thought that had been resolved? Apparently I misremember...

I had a quick look and couldn't see anything about that, it looks to me
like it just calls commit_tree() and only does anything to change the
default commit date if '--committer-date-is-author-date' was given.

Best Wishes

Phillip
> Ciao,
> Dscho
> 

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Sixt]

Am 15.04.2018 um 23:35 schrieb Junio C Hamano:

Ah, do you mean we have an internal sequence like this, when "rebase
--continue" wants to conclude an edit/reword?


Yes, it's only 'reword' that is affected, because then subsequent picks 
are processed by the original process.



  - we figure out the committer ident, which grabs a timestamp and
cache it;

  - we spawn "commit" to conclude the stopped step, letting it record
its beginning time (which is a bit older than the above) or its
ending time (which is much older due to human typing speed);


Younger in both cases, of course. According to my tests, we seem to pick 
the beginning time, because the first 'reword'ed commit typically has 
the same timestamp as the preceding picks. Later 'reword'ed commits have 
noticably younger timestamps.



  - subsequent "picks" are made in the same process, and share the
timestamp we grabbed in the first step, which is older than the
second one.

I guess we'd want a mechanism to tell ident.c layer "discard the
cached one, as we are no longer in the same automated sequence", and
use that whenever we spawn an editor (or otherwise go interactive).


Frankly, I think that this caching is overengineered (or prematurly 
optimized). If the design requires that different callers of datestamp() 
must see the same time, then the design is broken. In a fixed design, 
there would be a single call of datestamp() in advance, and then the 
timestamp, which then obviously is a very important piece of data, would 
be passed along as required.


-- Hannes

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Junio C Hamano]

Johannes Sixt  writes:

> I just noticed that all commits in a 70-commit branch have the same
> committer timestamp. This is very unusual on Windows, where rebase -i of
> such a long branch takes more than one second (but not more than 3 or
> so thanks to the builtin nature of the command!).
>
> And, in fact, if you mark some commits with 'reword' to delay the quick
> processing of the patches, then the reworded commits have later time
> stamps, but subsequent not reworded commits receive the earlier time
> stamp. This is clearly not intended.

Hmm, I may be missing something without enough caffeine but I am
puzzled how that would be possible.  With a "few picks, an edit, and
a yet more picks" sequence, the first picks may share the same
timestamp due to the git_default_date caching (which I think is a
deliberate design choice we made), an edit that stops will let the
concluding "commit" (either by the end user or invoked internally
via "rebase --continue"), but because that process restarts afresh,
the commits made by "yet more picks" cannot share the timestamp that
was cached for the earliest ones from the same series, no?

Ah, do you mean we have an internal sequence like this, when "rebase
--continue" wants to conclude an edit/reword?

 - we figure out the committer ident, which grabs a timestamp and
   cache it;

 - we spawn "commit" to conclude the stopped step, letting it record
   its beginning time (which is a bit older than the above) or its
   ending time (which is much older due to human typing speed);

 - subsequent "picks" are made in the same process, and share the
   timestamp we grabbed in the first step, which is older than the
   second one.

I guess we'd want a mechanism to tell ident.c layer "discard the
cached one, as we are no longer in the same automated sequence", and
use that whenever we spawn an editor (or otherwise go interactive).

>
> Perhaps something like this below is needed.
>
> diff --git a/ident.c b/ident.c
> index 327abe557f..2c6bff7b9d 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -178,8 +178,8 @@ const char *ident_default_email(void)
>  
>  static const char *ident_default_date(void)
>  {
> - if (!git_default_date.len)
> - datestamp(_default_date);
> + strbuf_reset(_default_date);
> + datestamp(_default_date);
>   return git_default_date.buf;
>  }
>  

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Schindelin]

Hi,

On Sat, 14 Apr 2018, Phillip Wood wrote:

> On 13/04/18 17:52, Johannes Sixt wrote:
> > 
> > I just noticed that all commits in a 70-commit branch have the same
> > committer timestamp. This is very unusual on Windows, where rebase -i of
> > such a long branch takes more than one second (but not more than 3 or
> > so thanks to the builtin nature of the command!).
> > 
> > And, in fact, if you mark some commits with 'reword' to delay the quick
> > processing of the patches, then the reworded commits have later time
> > stamps, but subsequent not reworded commits receive the earlier time
> > stamp. This is clearly not intended.
> 
> Oh dear, I think this is probably due to my series making rebase commit
> in-process when the commit message isn't being edited. I didn't realize
> that git cached the commit date rather than using the current time when
> calling commit_tree_extended(). I'll take a look at it next week.

Thanks.

However, a quick lock at `git log @{u}.. --format=%ct` in my
`sequencer-shears` branch thicket (which I rebase frequently on top of
upstream's `master` using the last known-good `rebase-merges` sub-branch)
shows that the commits have different-enough commit timestamps. (It is
satisfying to see that multiple commits were made during the same second,
of course.)

So while I cannot find anything in the code that disagrees with Hannes'
assessment, it looks on the surface as if I did not encounter the bug
here.

Curious.

FWIW I agree with Hannes' patch.

> I think 'git am' probably gives all patches the same commit time as well
> if the commit date is cached though it wont suffer from the time-travel
> problem.

I thought that `git am` was the subject of such a complaint recently, but
I thought that had been resolved? Apparently I misremember...

Ciao,
Dscho

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 13/04/18 17:52, Johannes Sixt wrote:
> 
> I just noticed that all commits in a 70-commit branch have the same
> committer timestamp. This is very unusual on Windows, where rebase -i of
> such a long branch takes more than one second (but not more than 3 or
> so thanks to the builtin nature of the command!).
> 
> And, in fact, if you mark some commits with 'reword' to delay the quick
> processing of the patches, then the reworded commits have later time
> stamps, but subsequent not reworded commits receive the earlier time
> stamp. This is clearly not intended.

Oh dear, I think this is probably due to my series making rebase commit
in-process when the commit message isn't being edited. I didn't realize
that git cached the commit date rather than using the current time when
calling commit_tree_extended(). I'll take a look at it next week. I
think 'git am' probably gives all patches the same commit time as well
if the commit date is cached though it wont suffer from the time-travel
problem.

Best Wishes

Phillip

> Perhaps something like this below is needed.
> 
> diff --git a/ident.c b/ident.c
> index 327abe557f..2c6bff7b9d 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -178,8 +178,8 @@ const char *ident_default_email(void)
>  
>  static const char *ident_default_date(void)
>  {
> - if (!git_default_date.len)
> - datestamp(_default_date);
> + strbuf_reset(_default_date);
> + datestamp(_default_date);
>   return git_default_date.buf;
>  }
>  
> 

Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Sixt]

I just noticed that all commits in a 70-commit branch have the same
committer timestamp. This is very unusual on Windows, where rebase -i of
such a long branch takes more than one second (but not more than 3 or
so thanks to the builtin nature of the command!).

And, in fact, if you mark some commits with 'reword' to delay the quick
processing of the patches, then the reworded commits have later time
stamps, but subsequent not reworded commits receive the earlier time
stamp. This is clearly not intended.

Perhaps something like this below is needed.

diff --git a/ident.c b/ident.c
index 327abe557f..2c6bff7b9d 100644
--- a/ident.c
+++ b/ident.c
@@ -178,8 +178,8 @@ const char *ident_default_email(void)
 
 static const char *ident_default_date(void)
 {
-   if (!git_default_date.len)
-   datestamp(_default_date);
+   strbuf_reset(_default_date);
+   datestamp(_default_date);
return git_default_date.buf;
 }
 

[PATCH v3 08/15] t1300: `--unset-all` can leave an empty section behind (bug)

[Johannes Schindelin]

We already have a test demonstrating that removing the last entry from a
config section fails to remove the section header of the now-empty
section.

The same can happen, of course, if we remove the last entries in one fell
swoop. This is *also* a bug, and should be fixed at the same time.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index bc30cfb3468..9d23a8ca972 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1495,6 +1495,17 @@ test_expect_failure '--unset last key removes section 
(except if commented)' '
test_line_count = 3 .git/config
 '
 
+test_expect_failure '--unset-all removes section if empty & uncommented' '
+   cat >.git/config <<-\EOF &&
+   [section]
+   key = value1
+   key = value2
+   EOF
+
+   git config --unset-all section.key &&
+   test_line_count = 0 .git/config
+'
+
 test_expect_failure 'adding a key into an empty section reuses header' '
cat >.git/config <<-\EOF &&
[section]
-- 
2.17.0.windows.1.4.g7e4058d72e3

[PATCH v3 05/15] t1300: avoid relying on a bug

[Johannes Schindelin]

The test case 'unset with cont. lines' relied on a bug that is about to
be fixed: it tests *explicitly* that removing the last entry from a
config section leaves an *empty* section behind.

Let's fix this test case not to rely on that behavior, simply by
preventing the section from becoming empty.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index aed12be492f..7c0ee208dea 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -108,6 +108,7 @@ bar = foo
 [beta]
 baz = multiple \
 lines
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines' '
@@ -118,6 +119,7 @@ cat > expect <<\EOF
 [alpha]
 bar = foo
 [beta]
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines is correct' 'test_cmp expect 
.git/config'
-- 
2.17.0.windows.1.4.g7e4058d72e3

[PATCH v3 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

This patch series originally only tried to help fixing that annoying bug that
has been reported several times over the years, where `git config --unset`
would leave empty sections behind, and `git config --add` would not reuse them.

The first patch is somewhat of a "while at it" bug fix that I first thought
would be a lot more critical than it actually is: It really only affects config
files that start with a section followed immediately (i.e. without a newline)
by a one-letter boolean setting (i.e. without a `= ` part). So while it
is a real bug fix, I doubt anybody ever got bitten by it.

The next swath of patches add and fix some tests, while also fixing the bug
where --replace-all would sometimes insert extra line breaks.

Then, I introduce a couple of building blocks: a "config parser event stream",
i.e. an optional callback that can be used to report events such as "comment",
"white-space", etc together with the corresponding extents in the config file.

Finally, the interesting part, where I do two things, essentially (with
preparatory steps for each thing):

1. I add the ability for `git config --unset/--unset-all` to detect that it
   can remove a section that has just become empty (see below for some more
   discussion of what I consider "become empty"), and

2. I add the ability for `git config [--add] key value` to re-use empty
   sections.

To reiterate why does this patch series not conflict with my very early
statements that we cannot simply remove empty sections because we may end up
with stale comments?

Well, the patch in question takes pains to determine *iff* there are any
comments surrounding, or included in, the section. If any are found: previous
behavior. Under the assumption that the user edited the file, we keep it as
intact as possible (see below for some argument against this). If no comments
are found, and let's face it, this is probably *the* common case, as few people
edit their config files by hand these days (neither should they because it is
too easy to end up with an unparseable one), the now-empty section *is*
removed.

So what is the argument against this extra care to detect comments? Well, if
you have something like this:

[section]
; Here we comment about the variable called snarf
snarf = froop

and we run `git config --unset section.snarf`, we end up with this config:

[section]
; Here we comment about the variable called snarf

which obviously does not make sense. However, that is already established
behavior for quite a few years, and I do not even try to think of a way how
this could be solved.

Changes since v2:

- removed the `inline` attribute from the `do_event()` function.

- renamed `struct config_set_store` to `struct config_store_data`, to make its
  roled more obvious.

- a whole slew of concocted test cases were added to the test to verify that
  a section that becomes empty is removed, based on Peff's analysis at
  https://public-inbox.org/git/20180329213229.gg2...@sigill.intra.peff.net/


Johannes Schindelin (15):
  git_config_set: fix off-by-two
  t1300: rename it to reflect that `repo-config` was deprecated
  t1300: demonstrate that --replace-all can "invent" newlines
  config --replace-all: avoid extra line breaks
  t1300: avoid relying on a bug
  t1300: remove unreasonable expectation from TODO
  t1300: add a few more hairy examples of sections becoming empty
  t1300: `--unset-all` can leave an empty section behind (bug)
  config: introduce an optional event stream while parsing
  config: avoid using the global variable `store`
  config_set_store: rename some fields for consistency
  git_config_set: do not use a state machine
  git_config_set: make use of the config parser's event stream
  git config --unset: remove empty sections (in the common case)
  git_config_set: reuse empty sections

 config.c| 448 ++--
 config.h|  25 ++
 t/{t1300-repo-config.sh => t1300-config.sh} | 102 -
 3 files changed, 439 insertions(+), 136 deletions(-)
 rename t/{t1300-repo-config.sh => t1300-config.sh} (95%)


base-commit: 468165c1d8a442994a825f3684528361727cd8c0
Published-As: https://github.com/dscho/git/releases/tag/empty-config-section-v3
Fetch-It-Via: git fetch https://github.com/dscho/git empty-config-section-v3

Interdiff vs v2:
 diff --git a/config.c b/config.c
 index ee7ea24123d..6155d0651bd 100644
 --- a/config.c
 +++ b/config.c
 @@ -659,8 +659,7 @@ struct parse_event_data {
const struct config_options *opts;
  };
  
 -static inline int do_event(enum config_event_t type,
 - struct parse_event_data *data)
 +static int do_event(enum config_event_t type, struct parse_event_data *data)
  {
size_t offset;
  
 @@ -2297,7 +2296,7 @@ void git_die_config(const char *key, const char *err, 
...)
   

Re: [PATCH v2 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

> On Tue, Apr 03, 2018 at 06:27:55PM +0200, Johannes Schindelin wrote:
> 
> > I am very, very grateful for the time Peff spent on reviewing the
> > previous iteration, and hope that he realizes just how much the
> > elegance of the event-stream-based version is due to his excellent
> > review.
> 
> Unfortunately I ran out of time this week to give this version an
> equally careful review, and I'm about to go on vacation for a few weeks.

No worries, and thank you for your review. I know I am adding more stuff
to review these days than I review other stuff, but I promise that I will
try to get more reviews in once I am done with this patch series (and with
the --rebase-merges one).

> I did give a cursory look over it, and the new maybe_remove_section() is
> much more pleasant. So aside from a few minor nits I pointed out, this
> generally looks good.

Thanks!

> One thing I'd like to have seen is a few more tests covering exotic
> cases that I turned up in my earlier review. Some of the weird multiline
> cases I care less about, but we should probably cover at least:
> 
>   1. Comment behavior when removing a section that isn't at the
>  beginning of the file.
> 
>   2. Removing the final key from a section with a subsection.
> 
> Those should both be natural fallouts of the new method, but it would be
> good to have test coverage.

I added this, in a new commit I call "t1300: add a few more hairy examples
of sections becoming empty".

> Thanks for reworking this, and if it's still not merged when I get back,
> I promise to review it more carefully then. :)

:-)

Have a good vacation!
Dscho

Re: [PATCH v2 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Jeff King]

On Tue, Apr 03, 2018 at 06:27:55PM +0200, Johannes Schindelin wrote:

> I am very, very grateful for the time Peff spent on reviewing the previous
> iteration, and hope that he realizes just how much the elegance of the
> event-stream-based version is due to his excellent review.

Unfortunately I ran out of time this week to give this version an
equally careful review, and I'm about to go on vacation for a few weeks.

I did give a cursory look over it, and the new maybe_remove_section() is
much more pleasant. So aside from a few minor nits I pointed out, this
generally looks good.

One thing I'd like to have seen is a few more tests covering exotic
cases that I turned up in my earlier review. Some of the weird multiline
cases I care less about, but we should probably cover at least:

  1. Comment behavior when removing a section that isn't at the
 beginning of the file.

  2. Removing the final key from a section with a subsection.

Those should both be natural fallouts of the new method, but it would be
good to have test coverage.

Thanks for reworking this, and if it's still not merged when I get back,
I promise to review it more carefully then. :)

-Peff

Re: [PATCH v2 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi team,

On Tue, 3 Apr 2018, Johannes Schindelin wrote:

> Johannes Schindelin (15):
>   git_config_set: fix off-by-two
>   t1300: rename it to reflect that `repo-config` was deprecated
>   t1300: demonstrate that --replace-all can "invent" newlines
>   config --replace-all: avoid extra line breaks
>   t1300: avoid relying on a bug
>   t1300: remove unreasonable expectation from TODO
>   t1300: `--unset-all` can leave an empty section behind (bug)
>   config: introduce an optional event stream while parsing
>   config: avoid using the global variable `store`
>   config_set_store: rename some fields for consistency
>   git_config_set: do not use a state machine
>   git_config_set: make use of the config parser's event stream
>   git config --unset: remove empty sections (in the common case)
>   git_config_set: reuse empty sections
>   TODOs

Please note that the `TODOs` commit is a left-over of my internal
book-keeping, and its diff is actually empty. Hence `format-patch` does
not even generate a mail for it, so there is no [PATCH v2 15/15].

Thanks,
Dscho

[PATCH v2 07/15] t1300: `--unset-all` can leave an empty section behind (bug)

[Johannes Schindelin]

We already have a test demonstrating that removing the last entry from a
config section fails to remove the section header of the now-empty
section.

The same can happen, of course, if we remove the last entries in one fell
swoop. This is *also* a bug, and should be fixed at the same time.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 187fc5b195f..10b9bf4b088 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1452,6 +1452,17 @@ test_expect_failure '--unset last key removes section 
(except if commented)' '
test_cmp expect .git/config
 '
 
+test_expect_failure '--unset-all removes section if empty & uncommented' '
+   cat >.git/config <<-\EOF &&
+   [section]
+   key = value1
+   key = value2
+   EOF
+
+   git config --unset-all section.key &&
+   test_line_count = 0 .git/config
+'
+
 test_expect_failure 'adding a key into an empty section reuses header' '
cat >.git/config <<-\EOF &&
[section]
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH v2 05/15] t1300: avoid relying on a bug

[Johannes Schindelin]

The test case 'unset with cont. lines' relied on a bug that is about to
be fixed: it tests *explicitly* that removing the last entry from a
config section leaves an *empty* section behind.

Let's fix this test case not to rely on that behavior, simply by
preventing the section from becoming empty.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index aed12be492f..7c0ee208dea 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -108,6 +108,7 @@ bar = foo
 [beta]
 baz = multiple \
 lines
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines' '
@@ -118,6 +119,7 @@ cat > expect <<\EOF
 [alpha]
 bar = foo
 [beta]
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines is correct' 'test_cmp expect 
.git/config'
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH v2 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

This patch series originally only tried to help fixing that annoying bug that
has been reported several times over the years, where `git config --unset`
would leave empty sections behind, and `git config --add` would not reuse them.

The first patch is somewhat of a "while at it" bug fix that I first thought
would be a lot more critical than it actually is: It really only affects config
files that start with a section followed immediately (i.e. without a newline)
by a one-letter boolean setting (i.e. without a `= ` part). So while it
is a real bug fix, I doubt anybody ever got bitten by it.

The next swath of patches add and fix some tests, while also fixing the bug
where --replace-all would sometimes insert extra line breaks.

These fixes are pretty straight-forward, and I always try to keep my added
tests as concise as possible, so please tell me if you find a way to make them
smaller (without giving up readability and debuggability).

Then, I introduce a couple of building blocks: a "config parser event stream",
i.e. an optional callback that can be used to report events such as "comment", 
"white-space", etc together with the corresponding extents in the config file.

Finally, the interesting part, where I do two things, essentially (with
preparatory steps for each thing):

1. I add the ability for `git config --unset/--unset-all` to detect that it
   can remove a section that has just become empty (see below for some more
   discussion of what I consider "become empty"), and

2. I add the ability for `git config [--add] key value` to re-use empty
   sections.

I am very, very grateful for the time Peff spent on reviewing the previous
iteration, and hope that he realizes just how much the elegance of the
event-stream-based version is due to his excellent review.

To reiterate why does this patch series not conflict with my very early
statements that we cannot simply remove empty sections because we may end up
with stale comments?

Well, the patch in question takes pains to determine *iff* there are any
comments surrounding, or included in, the section. If any are found: previous
behavior. Under the assumption that the user edited the file, we keep it as
intact as possible (see below for some argument against this). If no comments
are found, and let's face it, this is probably *the* common case, as few people
edit their config files by hand these days (neither should they because it is
too easy to end up with an unparseable one), the now-empty section *is*
removed.

So what is the argument against this extra care to detect comments? Well, if
you have something like this:

[section]
; Here we comment about the variable called snarf
snarf = froop

and we run `git config --unset section.snarf`, we end up with this config:

[section]
; Here we comment about the variable called snarf

which obviously does not make sense. However, that is already established
behavior for quite a few years, and I do not even try to think of a way how
this could be solved.

Changes since v1:

- a new feature was introduced where the config parser can be asked to report
  all "events" (like, section header or comment) via a callback function.

- the patches to reuse empty sections, and to remove sections that just became
  empty (without any surrounding comments) were rewritten to make use of that
  config parser event stream (incidentally fixing a couple of problems with
  the backtracking version which were pointed out by Peff).

- to make those changes easier to review, they have been split up into several
  tiny logical steps: the file-local `store` was replaced with callback data,
  some fields were renamed for consistency, the state machine when parsing the
  config was replaced by easier-to-understand flags, etc.

- while pouring over the code, I managed to find another obscure bug: under
  certain circumstances, --replace-all could produce extra new-lines. This is
  now fixed as part of the preparatory patches.


Johannes Schindelin (15):
  git_config_set: fix off-by-two
  t1300: rename it to reflect that `repo-config` was deprecated
  t1300: demonstrate that --replace-all can "invent" newlines
  config --replace-all: avoid extra line breaks
  t1300: avoid relying on a bug
  t1300: remove unreasonable expectation from TODO
  t1300: `--unset-all` can leave an empty section behind (bug)
  config: introduce an optional event stream while parsing
  config: avoid using the global variable `store`
  config_set_store: rename some fields for consistency
  git_config_set: do not use a state machine
  git_config_set: make use of the config parser's event stream
  git config --unset: remove empty sections (in the common case)
  git_config_set: reuse empty sections
  TODOs

 config.c| 449 
 config.h|  

Re: Possible bug in git log with date ranges

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 02 2018, David Hoyle wrote:

> Hi,
>
> Hopefully I've read the readme file correctly for submitting something
> that might be a bug.
>
> I've recently migrated projects from an old version control system
> (JEDI VCS) to Git (which I really like BTW). The way this was done was
> by extracting the files from the original database and saving them to
> a folder layout and then running git add / commit on the files. When
> using the commit command I've used the --date switch to commit the
> files using their original dates. However if I run git log with say
> --since=date it seems as if this command uses the actual date the
> commit was entered not the date given for the commit. The same seems
> to apply to the other date filtering switches.

The --date=* switch to git-commit sets the author date, but the date
narrowing options to git-log use the committer date. See if when you
run:

git log --pretty=format:"%aD - %cD"

Whether what you're getting doesn't make sense in terms of the second
date.

You can use GIT_COMMITTER_DATE to get what you want, see "DATE FORMATS"
in git-commit(1).

Possible bug in git log with date ranges

[David Hoyle]

Hi,

Hopefully I've read the readme file correctly for submitting something
that might be a bug.

I've recently migrated projects from an old version control system
(JEDI VCS) to Git (which I really like BTW). The way this was done was
by extracting the files from the original database and saving them to
a folder layout and then running git add / commit on the files. When
using the commit command I've used the --date switch to commit the
files using their original dates. However if I run git log with say
--since=date it seems as if this command uses the actual date the
commit was entered not the date given for the commit. The same seems
to apply to the other date filtering switches.

Below is an example using a log alias switch shows dates in a single
line format.

Date: Mon  2 Apr 2018, Time: 12:39:21, Location: D:\Documents\RAD
Studio\Applications\Eidolon.GIT
>git lg1 --since=01/Jan/2018
* 9ce470f - (Sat Jan 20 11:54:54 2018 +) Prevent an overflow with
an Int64 for integers by not converting. - DGH2112 (HEAD ->
Development, master)
* 863988f - (Sat Jan 20 11:53:44 2018 +) Tested large hard coded
integer conversion integers - stopped conversion and left as string. -
DGH2112
* e14ecc9 - (Thu Jan 4 16:33:49 2018 +) Added new sub-option for
Hard Coded Integers to skip 'DIV 2'. - DGH2112
* 6039285 - (Wed Jan 3 21:51:08 2018 +) Bracketed CodeSiteLogging
with a CODESITE IFDEF. - DGH2112
* 651c682 - (Wed Jan 3 21:50:39 2018 +) Bracketed CodeSiteLogging
with a CODESITE IFDEF. - DGH2112
* c94ba4e - (Wed Jan 3 19:40:42 2018 +) Fixed unit name
correction. - DGH2112
* 368258b - (Wed Jan 3 14:09:48 2018 +) Separated Metric and Check
options from their sub-options (made them a simple enumerte set). -
DGH2112
* d7aa03e - (Tue Jan 2 18:10:20 2018 +) Fixed issues with disabled
metrics and checks showing up in the editor reports. - DGH2112
* f7fbe87 - (Fri Dec 29 20:59:22 2017 +) Fixed cyclometric
complexity test as the method default is 1 not 0. - DGH2112
* ab609f9 - (Thu Dec 28 22:54:45 2017 +) Added two new document
options for auto hiding checks and metrics with no issues. - DGH2112
* 6ed4786 - (Thu Dec 28 22:54:24 2017 +) Added two new document
options for auto hiding checks and metrics with no issues. - DGH2112
* e422751 - (Thu Dec 28 22:42:31 2017 +) Added two new document
options for auto hiding checks and metrics with no issues. - DGH2112
* d8a9b06 - (Thu Dec 28 18:11:11 2017 +) Updated all reference to
AddModuleCheck to AddCheck and update the method with checks. -
DGH2112
* 52bd768 - (Wed Dec 27 23:31:52 2017 +) Fixed depreciated
IsLetter().\nAdd the ability for metrics to be marked as overridden. -
DGH2112
* 0b05b16 - (Wed Dec 27 22:51:53 2017 +) Split metrics and checks. - DGH2112
* e94ab97 - (Wed Dec 27 16:13:00 2017 +) Fixed unit backward
compatibility. - DGH2112
* d6fde37 - (Wed Dec 27 15:12:53 2017 +) Fixed TParallel.For(). - DGH2112
* 9161ded - (Sat Dec 23 16:51:33 2017 +) Updated code for
VirtualTress 5.5.2. - DGH2112
* 2fa264d - (Sun Dec 17 14:02:54 2017 +) Fixed tests. - DGH2112
* 75d438f - (Sun Dec 17 12:09:04 2017 +) Fixed cyclometric
comlpexity sub options for boolean expressions. - DGH2112
* 9075a62 - (Sun Dec 17 11:56:55 2017 +) Broke a part metrics and
checks and their sub options. - DGH2112
* 76b5ec9 - (Sat Dec 16 21:01:18 2017 +) Broke a part metrics and
checks and sub options. - DGH2112
* 9807ad4 - (Tue Dec 12 20:43:04 2017 +) Tested unicode
identifiers. - DGH2112
* d831bc0 - (Sat Dec 9 20:35:05 2017 +) Fixed missing CONST in
parameters. - DGH2112
* fab9981 - (Sun Nov 26 19:22:21 2017 +) Moved some of the metric
checks so that they only work on implemented methods not declarations.
- DGH2112
* c61f460 - (Sun Nov 19 20:22:43 2017 +) Updated the special tags
to have custom fonts styles, fore and background colours. - DGH2112
* 7d1fced - (Sun Nov 12 19:47:00 2017 +) Fixed metrics for non
unit implementations. - DGH2112
* 32fe4de - (Sun Nov 12 19:45:21 2017 +) Added test for checks and
metrics. - DGH2112
* 6827a22 - (Sun Nov 12 13:40:54 2017 +) Fixed
TestGrammarForErrors. - DGH2112
* 8713e52 - (Sun Nov 12 13:07:57 2017 +) Added Doc Conflicts and
Mertrics to the list of checks. - DGH2112
* 0e5c169 - (Sun Nov 12 10:03:10 2017 +) Added an END line to
record, objects ,classes and interfaces. - DGH2112
* ac8091c - (Sun Nov 5 20:22:23 2017 +) Updated the special tags
to have custom fonts styles, fore and background colours. - DGH2112
* 10dacab - (Sun Nov 5 16:19:31 2017 +) Fixed tests for Program,
Library and Packages where Uses does not have Interface and
Implementation sections. - DGH2112
* e7996fa - (Sun Nov 5 16:17:44 2017 +) Ensured metrics are
expanded. - DGH2112
* 3f6d401 - (Sun Nov 5 16:17:25 2017 +) Added checks for Empty
FOR, WHILE, REPEAT, and BEGIN END. - DGH2112
* 579ebc8 - (Fri Nov 3 19:07:44 2017 +) Fixed capitalised USES
clause. - D

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Ævar,

On Fri, 30 Mar 2018, Ævar Arnfjörð Bjarmason wrote:

> 
> On Thu, Mar 29 2018, Johannes Schindelin wrote:
> 
> > Nonetheless, I would be confortable with this patch going into
> > v2.17.0, even at this late stage. The final verdict is Junio's, of
> > course.
> 
> Thanks a lot for working on this. I'm keen to stress test this, but
> won't have time in the next few days, and in any case think that the
> parts that change functionality should wait until after 2.17 (but e.g.
> the test renaming would be fine for a cherry-pick).

Obviously this was never meant to get into v2.17.0 (apart maybe from 1/9,
which however is so contested over that addition of the test case under
the assumption that anybody but me would dare to touch those parts of the
code).

Ciao,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 29 2018, Johannes Schindelin wrote:

> Nonetheless, I would be confortable with this patch going into v2.17.0, even 
> at
> this late stage. The final verdict is Junio's, of course.

Thanks a lot for working on this. I'm keen to stress test this, but
won't have time in the next few days, and in any case think that the
parts that change functionality should wait until after 2.17 (but
e.g. the test renaming would be fine for a cherry-pick).

Re: [PATCH 3/9] t1300: avoid relying on a bug

[Johannes Schindelin]

Hi Peff,

On Thu, 29 Mar 2018, Jeff King wrote:

> On Thu, Mar 29, 2018 at 05:18:45PM +0200, Johannes Schindelin wrote:
> 
> > The test case 'unset with cont. lines' relied on a bug that is about to
> > be fixed: it tests *explicitly* that removing the last entry from a
> > config section leaves an *empty* section behind.
> > 
> > Let's fix this test case not to rely on that behavior, simply by
> > preventing the section from becoming empty.
> 
> Seems like a good solution. I don't think we care in particular about
> testing a multi-line value at the end of the file.

... and if we did, we should have documented that.

Ciao,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Peff,

On Thu, 29 Mar 2018, Jeff King wrote:

> On Thu, Mar 29, 2018 at 05:18:30PM +0200, Johannes Schindelin wrote:
> 
> > The first patch is somewhat of a "while at it" bug fix that I first
> > thought would be a lot more critical than it actually is: It really
> > only affects config files that start with a section followed
> > immediately (i.e. without a newline) by a one-letter boolean setting
> > (i.e. without a `= ` part). So while it is a real bug fix, I
> > doubt anybody ever got bitten by it.
> 
> That makes me wonder if somebody could craft a malicious config to do
> something bad.

I thought about that, and could not think of anything other than social
engineering vectors. Even in that case, the error message is instructive
enough that the user should be able to fix the config without consulting
StackOverflow.

> > Now, to the really important part: why does this patch series not
> > conflict with my very early statements that we cannot simply remove
> > empty sections because we may end up with stale comments?
> > 
> > Well, the patch in question takes pains to determine *iff* there are
> > any comments surrounding, or included in, the section. If any are
> > found: previous behavior. Under the assumption that the user edited
> > the file, we keep it as intact as possible (see below for some
> > argument against this). If no comments are found, and let's face it,
> > this is probably *the* common case, as few people edit their config
> > files by hand these days (neither should they because it is too easy
> > to end up with an unparseable one), the now-empty section *is*
> > removed.
> 
> I'm not against people editing their config files by hand. But I think
> what you propose here makes a lot of sense, because it works as long as
> you don't intermingle hand- and auto-editing in the same section (and it
> even works if you do intermingle, as long as you don't use comments,
> which are probably even more rare).
> 
> So it seems like quite a sensible compromise, and I think should make
> most people happy.

Thanks for confirming my line of thinking,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Stefan,

On Thu, 29 Mar 2018, Stefan Beller wrote:

> On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
> <johannes.schinde...@gmx.de> wrote:
> 
> > So what is the argument against this extra care to detect comments? Well, if
> > you have something like this:
> >
> > [section]
> > ; Here we comment about the variable called snarf
> > snarf = froop
> >
> > and we run `git config --unset section.snarf`, we end up with this config:
> >
> > [section]
> > ; Here we comment about the variable called snarf
> >
> > which obviously does not make sense. However, that is already established
> > behavior for quite a few years, and I do not even try to think of a way how
> > this could be solved.
> 
> By commenting out the key/value pair instead of deleting it.
> It's called --unset, not --delete ;)

That would open the door to new bug reports when a user starts with this
concocted config:

[section]
# This is a comment about the `key` setting
key = value

and then does this:

git config --unset section.key
git config section.key value
git config --unset section.key
git config section.key value
git config --unset section.key
git config section.key value

and then ends up with a config like this:

[section]
# This is a comment about the `key` setting
;key = value
;key = value
;key = value
key = value

And note that the comment might be about `value` instead, so reusing a
commented-out `key` setting won't fly, either.

I *did* give this problem a couple of minutes of thought before writing my
assessment that is quoted above ;-)

Ciao,
Dscho

Re: [PATCH 5/9] t1300: `--unset-all` can leave an empty section behind (bug)

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:53PM +0200, Johannes Schindelin wrote:

> We already have a test demonstrating that removing the last entry from a
> config section fails to remove the section header of the now-empty
> section.
> 
> The same can happen, of course, if we remove the last entries in one fell
> swoop. This is *also* a bug, and should be fixed at the same time.

Yep, makes sense, and the diff is obviously correct.

-Peff

Re: [PATCH 3/9] t1300: avoid relying on a bug

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:45PM +0200, Johannes Schindelin wrote:

> The test case 'unset with cont. lines' relied on a bug that is about to
> be fixed: it tests *explicitly* that removing the last entry from a
> config section leaves an *empty* section behind.
> 
> Let's fix this test case not to rely on that behavior, simply by
> preventing the section from becoming empty.

Seems like a good solution. I don't think we care in particular about
testing a multi-line value at the end of the file.

-Peff

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:30PM +0200, Johannes Schindelin wrote:

> Little did I know that this would turn not only into a full patch to fix this
> issue, but into a full-blown series of nine patches.

It's amazing how often that happens. :)

> The first patch is somewhat of a "while at it" bug fix that I first thought
> would be a lot more critical than it actually is: It really only affects 
> config
> files that start with a section followed immediately (i.e. without a newline)
> by a one-letter boolean setting (i.e. without a `= ` part). So while it
> is a real bug fix, I doubt anybody ever got bitten by it.

That makes me wonder if somebody could craft a malicious config to do
something bad. But I don't think so. Config is trusted already, and it
looks like this bug is both hard to trigger and doesn't result in any
kind of memory funniness, just a bogus output.

> Now, to the really important part: why does this patch series not conflict 
> with
> my very early statements that we cannot simply remove empty sections because 
> we
> may end up with stale comments?
> 
> Well, the patch in question takes pains to determine *iff* there are any
> comments surrounding, or included in, the section. If any are found: previous
> behavior. Under the assumption that the user edited the file, we keep it as
> intact as possible (see below for some argument against this). If no comments
> are found, and let's face it, this is probably *the* common case, as few 
> people
> edit their config files by hand these days (neither should they because it is
> too easy to end up with an unparseable one), the now-empty section *is*
> removed.

I'm not against people editing their config files by hand. But I think
what you propose here makes a lot of sense, because it works as long as
you don't intermingle hand- and auto-editing in the same section (and it
even works if you do intermingle, as long as you don't use comments,
which are probably even more rare).

So it seems like quite a sensible compromise, and I think should make
most people happy.

-Peff

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Stefan Beller]

On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
 wrote:

> So what is the argument against this extra care to detect comments? Well, if
> you have something like this:
>
> [section]
> ; Here we comment about the variable called snarf
> snarf = froop
>
> and we run `git config --unset section.snarf`, we end up with this config:
>
> [section]
> ; Here we comment about the variable called snarf
>
> which obviously does not make sense. However, that is already established
> behavior for quite a few years, and I do not even try to think of a way how
> this could be solved.

By commenting out the key/value pair instead of deleting it.
It's called --unset, not --delete ;)

Now onto reviewing the patches.

Stefan

[PATCH 3/9] t1300: avoid relying on a bug

[Johannes Schindelin]

The test case 'unset with cont. lines' relied on a bug that is about to
be fixed: it tests *explicitly* that removing the last entry from a
config section leaves an *empty* section behind.

Let's fix this test case not to rely on that behavior, simply by
preventing the section from becoming empty.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 4f8e6f5fde3..1ece7bad05f 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -108,6 +108,7 @@ bar = foo
 [beta]
 baz = multiple \
 lines
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines' '
@@ -118,6 +119,7 @@ cat > expect <<\EOF
 [alpha]
 bar = foo
 [beta]
+foo = bar
 EOF
 
 test_expect_success 'unset with cont. lines is correct' 'test_cmp expect 
.git/config'
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 5/9] t1300: `--unset-all` can leave an empty section behind (bug)

[Johannes Schindelin]

We already have a test demonstrating that removing the last entry from a
config section fails to remove the section header of the now-empty
section.

The same can happen, of course, if we remove the last entries in one fell
swoop. This is *also* a bug, and should be fixed at the same time.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
 t/t1300-config.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 3ad3df0c83e..ff79a213567 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1452,6 +1452,17 @@ test_expect_failure '--unset last key removes section 
(except if commented)' '
test_cmp expect .git/config
 '
 
+test_expect_failure '--unset-all removes section if empty & uncommented' '
+   cat >.git/config <<-\EOF &&
+   [section]
+   key = value1
+   key = value2
+   EOF
+
+   git config --unset-all section.key &&
+   test_line_count = 0 .git/config
+'
+
 test_expect_failure 'adding a key into an empty section reuses header' '
cat >.git/config <<-\EOF &&
[section]
-- 
2.16.2.windows.1.26.g2cc3565eb4b

[PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

This patch series started out as a single patch trying to figure out what it
takes to fix that annoying bug that has been reported several times over the
years, where `git config --unset` would leave empty sections behind, and `git
config --add` would not reuse them.

Little did I know that this would turn not only into a full patch to fix this
issue, but into a full-blown series of nine patches.

The first patch is somewhat of a "while at it" bug fix that I first thought
would be a lot more critical than it actually is: It really only affects config
files that start with a section followed immediately (i.e. without a newline)
by a one-letter boolean setting (i.e. without a `= ` part). So while it
is a real bug fix, I doubt anybody ever got bitten by it.

Nonetheless, I would be confortable with this patch going into v2.17.0, even at
this late stage. The final verdict is Junio's, of course.

The next swath of patches add some tests, and adjust one test about which I
already complained at length yesterday, so I will spare you the same ordeal
today. These fixes are pretty straight-forward, and I always try to keep my
added tests as concise as possible, so please tell me if you find a way to make
them smaller (without giving up readability and debuggability).

Finally, the interesting part, where I do two things, essentially (with
preparatory steps for each thing):

1. I add the ability for `git config --unset/--unset-all` to detect that it
   can remove a section that has just become empty (see below for some more
   discussion of what I consider "become empty"), and

2. I add the ability for `git config [--add] key value` to re-use empty
   sections.

Note that the --unset/--unset-all part is the hairy one, and I would love it if
people could concentrate on wrapping their heads around that function, and
obviously tell me how I can change it to make it more readable (or even point
out incorrect behavior).

Now, to the really important part: why does this patch series not conflict with
my very early statements that we cannot simply remove empty sections because we
may end up with stale comments?

Well, the patch in question takes pains to determine *iff* there are any
comments surrounding, or included in, the section. If any are found: previous
behavior. Under the assumption that the user edited the file, we keep it as
intact as possible (see below for some argument against this). If no comments
are found, and let's face it, this is probably *the* common case, as few people
edit their config files by hand these days (neither should they because it is
too easy to end up with an unparseable one), the now-empty section *is*
removed.

So what is the argument against this extra care to detect comments? Well, if
you have something like this:

[section]
; Here we comment about the variable called snarf
snarf = froop

and we run `git config --unset section.snarf`, we end up with this config:

[section]
; Here we comment about the variable called snarf

which obviously does not make sense. However, that is already established
behavior for quite a few years, and I do not even try to think of a way how
this could be solved.


Johannes Schindelin (9):
  git_config_set: fix off-by-two
  t1300: rename it to reflect that `repo-config` was deprecated
  t1300: avoid relying on a bug
  t1300: remove unreasonable expectation from TODO
  t1300: `--unset-all` can leave an empty section behind (bug)
  git_config_set: simplify the way the section name is remembered
  git config --unset: remove empty sections (in normal situations)
  git_config_set: use do_config_from_file() directly
  git_config_set: reuse empty sections

 config.c| 234 +---
 t/{t1300-repo-config.sh => t1300-config.sh} |  36 -
 2 files changed, 250 insertions(+), 20 deletions(-)
 rename t/{t1300-repo-config.sh => t1300-config.sh} (98%)


base-commit: 03df4959472e7d4b5117bb72ac86e1e2bcf21723
Published-As: https://github.com/dscho/git/releases/tag/empty-config-section-v1
Fetch-It-Via: git fetch https://github.com/dscho/git empty-config-section-v1
-- 
2.16.2.windows.1.26.g2cc3565eb4b

Re: Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Quick note: I did submit the patch, look for subject line " [PATCH] credential: 
cred helper fast exit can cause SIGPIPE, crash".

Thanks again Jeff,
Erik



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

[Stefan Beller]

On Wed, Mar 28, 2018 at 12:37 PM, Peter Oberndorfer  wrote:

>>> 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).
>>
>
> Hmm, yeah looks a bit more complicated than I initially imagined
> since submodules can have a name that's different from their path.
> And after the rebase, the name <-> path mapping via .gitmodules is not 
> available anymore.
>
> Naively I think it could work the following way:
> * Either iterate over all submodules in .git/modules/ and check their config
>   has a worktree = "../../path" that resolves to the submodule path we want 
> to remove.

This would work but scales linearly with the number of submodules.


> * Or check the "gitlink:" path in submodule/.git if it points to our 
> .git/modules/
> Then if .git/config contains a [submodule "name"] entry
> we should have a pretty good idea if this folder contains a stale submodule.

If you move a submodule a directory up or down, the relative path is not exact
any more, we'd need to check for the last part to loosely match.


>> 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.
>>
>
> Yeah that would be nice :-)
> In my case I updated the repository via a rebase, so that would also have to 
> be covered.

Oh rebase itself has not yet learned about recursion into submodules.
("git pull --rebase --recurse-submodules" is a thing though)

>> 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.
>>
>
> My gut feeling makes me expect the following:
> * It would be nice if such stale submodules showed up in "git submodule 
> status" or "git status"
>   Now "git submodule" shows nothing related to this stale submodule

That has currently only two ways "+" or "-" for there/not there.
Maybe we'd need to add some characters similar to "git status --porcelain"
such as "?"

>   Now "git status" shows  Untracked files: src/rt which is a bit confusing as 
> the actual submodule is in src/rt/hoedown
>   Now "Git gui" shows src/rt/hoedown as untracked git repository

hm. The current state of affairs doesn't sound intriguing.
Though, I think we'd want to step back one more step and rather want
to ask how a dormant submodule comes into existence, instead of
just improving the reporting. Reportingthem is of course also important,
but in the long run I'd rather want to have situations like these happen
less often. When upstream deletes a file, they are also not required to be
deleted manually, but merge/checkout would take care of them.

> * There should be an easy(and safe) way for the user to deinit such a 
> submodule
>   if if the automatic submodule updating during a merge/rebase was not 
> enabled or somehow failed.
> (Minus the problem of somebody having to actually do the work...)
>
>>> ~/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.
>
> Sorry, what do you mean by (3)?

I meant the ((null)) issue is another third thought that we can
discuss separately,
slightly unrelated to the others (that you marked as (1) and (2))

Thanks,
Stefan

Re: Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Sure, I can submit a patch if the change looks good to you (with my lack of 
experience in the git source and very rusty C I would, of course, defer to an 
expert in the area on exactly where to place the SIGPIPE ignore push and pop 
and such... but what's below seems to avoid the race for us so I can submit 
that as-is).

Thanks for the quick response!
Erik

On 3/28/18, 11:46 AM, "Jeff King"  wrote:

On Wed, Mar 28, 2018 at 06:26:08PM +, Erik Brady -X (brady - ROBERT 
HALF INTERNATIONAL INC at Cisco) wrote:

> The location of the problem is in credential.c, 
run_credential_helper()... this code:
> 
>...
> fp = xfdopen(helper.in, "w");
> credential_write(c, fp);
> fclose(fp);
>..
> 
> Which I think needs to become something like this:
> 
> fp = xfdopen(helper.in, "w");
> sigchain_push(SIGPIPE, SIG_IGN);
> credential_write(c, fp);
> fclose(fp);
> sigchain_pop(SIGPIPE);
> 
> The basics are that we wrote a credential helper in Go and, for the
> store action, it simply exits 0.  It is fast.  This is similar to the
> example here:

Yeah, that makes sense. Generally a pipe buffer would be plenty to hold
a credential, but we're racing against whether the other process exits
before we even write anything, so it's bound to fail eventually in a
racy way.

I don't think we've ever made a promise[1] about whether credential
helpers have to read their input, but it makes sense to me for Git to be
friendly and handle this case. We've done similar things for hooks.

Curiously, I have a very similar helper myself, which I did as an inline
shell snippet in my ~/.gitconfig:

  [credential "https://github.com;]
  username = peff
  helper = "!f() { test $1 = get && echo password=`pass peff/github/oauth`; 
}; f"

I guess I've never lost the race because of the sheer number of
sub-processes that get spawned (shell to "pass" which is itself a shell
script, which spawns gpg -- yikes!).

Do you want to send your change as a patch? There's some guidance in
Documentation/SubmittingPatches.

-Peff

[1] I know you pulled a similar example from the Pro Git book content,
which we mirror on git-scm.com.  The quality there is usually quite
good, but I don't consider it as authoritative as the manpages. :)

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

[Peter Oberndorfer]

On 2018-03-28 00:56, Stefan Beller wrote:
> On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer <kumbay...@arcor.de>
> wrote:

Hi,

as expected your patch fixed the BUG output.
Thanks!

>> 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).
> 

Hmm, yeah looks a bit more complicated than I initially imagined
since submodules can have a name that's different from their path.
And after the rebase, the name <-> path mapping via .gitmodules is not 
available anymore.

Naively I think it could work the following way:
* Either iterate over all submodules in .git/modules/ and check their config
  has a worktree = "../../path" that resolves to the submodule path we want to 
remove.
* Or check the "gitlink:" path in submodule/.git if it points to our 
.git/modules/
Then if .git/config contains a [submodule "name"] entry
we should have a pretty good idea if this folder contains a stale submodule.

> 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.
> 

Yeah that would be nice :-)
In my case I updated the repository via a rebase, so that would also have to be 
covered.

> 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.
> 

My gut feeling makes me expect the following:
* It would be nice if such stale submodules showed up in "git submodule status" 
or "git status"
  Now "git submodule" shows nothing related to this stale submodule
  Now "git status" shows  Untracked files: src/rt which is a bit confusing as 
the actual submodule is in src/rt/hoedown
  Now "Git gui" shows src/rt/hoedown as untracked git repository
* There should be an easy(and safe) way for the user to deinit such a submodule
  if if the automatic submodule updating during a merge/rebase was not enabled 
or somehow failed.
(Minus the problem of somebody having to actually do the work...)

>> ~/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.

Sorry, what do you mean by (3)?

Thanks,
Greetings Peter

Re: Apparent bug in credential tool running...

[Jeff King]

On Wed, Mar 28, 2018 at 06:26:08PM +, Erik Brady -X (brady - ROBERT HALF 
INTERNATIONAL INC at Cisco) wrote:

> The location of the problem is in credential.c, run_credential_helper()... 
> this code:
> 
>...
> fp = xfdopen(helper.in, "w");
> credential_write(c, fp);
> fclose(fp);
>..
> 
> Which I think needs to become something like this:
> 
> fp = xfdopen(helper.in, "w");
> sigchain_push(SIGPIPE, SIG_IGN);
> credential_write(c, fp);
> fclose(fp);
> sigchain_pop(SIGPIPE);
> 
> The basics are that we wrote a credential helper in Go and, for the
> store action, it simply exits 0.  It is fast.  This is similar to the
> example here:

Yeah, that makes sense. Generally a pipe buffer would be plenty to hold
a credential, but we're racing against whether the other process exits
before we even write anything, so it's bound to fail eventually in a
racy way.

I don't think we've ever made a promise[1] about whether credential
helpers have to read their input, but it makes sense to me for Git to be
friendly and handle this case. We've done similar things for hooks.

Curiously, I have a very similar helper myself, which I did as an inline
shell snippet in my ~/.gitconfig:

  [credential "https://github.com;]
  username = peff
  helper = "!f() { test $1 = get && echo password=`pass peff/github/oauth`; }; 
f"

I guess I've never lost the race because of the sheer number of
sub-processes that get spawned (shell to "pass" which is itself a shell
script, which spawns gpg -- yikes!).

Do you want to send your change as a patch? There's some guidance in
Documentation/SubmittingPatches.

-Peff

[1] I know you pulled a similar example from the Pro Git book content,
which we mirror on git-scm.com.  The quality there is usually quite
good, but I don't consider it as authoritative as the manpages. :)

Apparent bug in credential tool running...

[Erik Brady -X (brady - ROBERT HALF INTERNATIONAL INC at Cisco)]

Hi git Experts,

I believe we've encountered a bug in git.  I built the latest, git 2.16.3, and 
it still appears to be a problem.  I'm not a git expert and my C is 
ridiculously rusty but I think a co-worker and I figured it out... apologies if 
we are incorrect in any assumptions (as I do not wish to waste anyone's time).

The location of the problem is in credential.c, run_credential_helper()... this 
code:

   ...
fp = xfdopen(helper.in, "w");
credential_write(c, fp);
fclose(fp);
   ..

Which I think needs to become something like this:

fp = xfdopen(helper.in, "w");
sigchain_push(SIGPIPE, SIG_IGN);
credential_write(c, fp);
fclose(fp);
sigchain_pop(SIGPIPE);

The basics are that we wrote a credential helper in Go and, for the store 
action, it simply exits 0.  It is fast.  This is similar to the example here:

https://git-scm.com/book/en/v2/Git-Tools-Credential-Storage#_a_custom_credential_cache

Which is, of course, in Ruby, not Go (so, not so fast).  It exits if it is not 
a get cred helper action without reading stdin.  Anyhow, for our credential 
helper the store operation completes very quickly.  What we've found is that 
occasionally the git command will be killed off just after running the 
credential store operation.  We can see that our credential helper is being 
fired up (it has a debug mode) and it quickly exits.  We can see that git dies 
on the fclose(fp) by putting trace_printf() calls before and after that fclose 
in the git source code (ie: the trace message before the fclose() prints, the 
trace message after the fclose does not).

Our belief is that the write is buffered and written at the time of fclose()... 
and that the credential helper tool has already exited "sometimes" (as it is 
very fast, but even so this doesn't fail very often).  For those times when our 
cred helper has exited "quickly enough", a SIGPIPE can be generated... and, as 
SIGPIPE is not ignored, git goes "kaboom!" and dies.

To catch this scenario we wrote a simple hack Perl tool to run a bunch of 
serial git ls-remote commands like so:

#!/usr/cisco/bin/perl -w

$ENV{'GIT_TRACE'} = 2;
$ENV{'GIT_TRACE_CURL'} = 2;
$ENV{'GIT_TRACE_PERFORMANCE'} = 1;
$ENV{'GIT_TRACE_PACKET'} = 1;

for ( my $i = 1; $i<=10; $i++) {
print("Run: $i\n");
my $output = `/ws/brady-sjc/git/git-2.16.3/bin/git ls-remote 
https://hostname.company.com/git/path/repo.git HEAD 2>&1`;
if ( $? != 0 ) {
print("FAIL: output:\n$output\n");
exit(1);
}
}
exit(0);

The problem seemed to come up most commonly on older linux VM's... in this case 
running 2.6.18-416.el5 #1 SMP.  The tool will iterate for a while and then, 
boom, it blows up (usually within 1000 iterations, sometimes a couple/few 
thousand but usually well before that).

Anyhow... we did a few things to test our theory that it is, indeed, SIGPIPE 
causing git to exit:

1) My co-worker modified our credential manager to read in the data sent by git 
before exiting... that solved the problem as we accept the data written so the 
child is still there and no SIGPIPE is generated... this is our current 
workaround (so we are OK, but would be good to fix this in the git code we 
think)

2) I modified the above code to use a signal handler in credential.c (instead 
of SIG_IGN) and put a trace_printf() and an exit(1) inside the signal 
handler... similar to the problem we're seeing it'll run a bunch successfully 
until, boom, timing is hit such that the child exits quickly enough and causes 
the SIGPIPE to occur at which point git is killed so using the cheesy Perl 
test script it ran through a couple hundred iterations fine and then a SIGPIPE 
was seen and it died in the signal handler I wrote... so clearly SIGPIPE is 
being generated but only occasionally (it is timing based, so occurs only now 
and then... and we almost never see it on some of our boxes)

3) I modified the git code (using our old cred helper which exits quickly) per 
the above recommended credential.c changes (you folks can likely do a better 
fix)... and re-run the Perl test script... no longer fails now that we are 
ignoring SIGPIPE (ran about 20,000+ iterations).

Note that the build-in credential manager was not failing... it reads the cred 
helper store data so it would not have the problem.

Let me know if you need any additional information... and thanks for your time 
and consideration.

Erik
br...@cisco.com

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

[Johannes Schindelin]

Hi Stefan & Jason,

On Tue, 27 Mar 2018, Stefan Beller wrote:

> On Tue, Mar 27, 2018 at 1:41 PM Jason Frey <jf...@redhat.com> 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)

Note: as far as I remember, the attempted fixes were exclusively trying to
remove the empty section. But this report suggests that we could instead
*keep* empty sections, but then reuse them when a new value is added.

> 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.

Yes, (a) makes removing an empty section something less of a desirable
thing. Unless there are no comments before and after the section, of
course, and yes, (b) is a real thing.

On a positive note: I just finished work on a set of patches addressing
this:
https://github.com/git/git/compare/master...dscho:empty-config-section (I
plan on submitting this tomorrow)

Ciao,
Dscho

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

[Philip Oakley]

From: "Ævar Arnfjörð Bjarmason" <ava...@gmail.com>

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.


One option may be to create  a simple 'lint' style checker that simply 
hiughlights and suggests options so the user can decide for themselves what 
they need to do. This would help span the gap between hard format and the 
soft format capabiulities of machine readable ini files, the Git config 
reader and being human readable.


Thus duplicate sections would be noted, likewise the presence of comments 
immediately preceding a section header, or terminating a section (with or 
without spacing?), etc.Such a config_lint could reside in the contrib as a 
supprt tool, and may in the long term be a guide to a common format. 
However, as noted, it would be more of a long term aspiration..





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.



--
Philip 

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

[Philip Oakley]

From: "Ævar Arnfjörð Bjarmason" <ava...@gmail.com>

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.


One option may be to create  a simple 'lint' style checker that simply 
hiughlights and suggests options so the user can decide for themselves what 
they need to do. This would help span the gap between hard format and the 
soft format capabiulities of machine readable ini files, the Git config 
reader and being human readable.


Thus duplicate sections would be noted, likewise the presence of comments 
immediately preceding a section header, or terminating a section (with or 
without spacing?), etc.Such a config_lint could reside in the contrib as a 
supprt tool, and may in the long term be a guide to a common format. 
However, as noted, it would be more of a long term aspiration..





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.



--
Philip 

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

[Philip Oakley]

From: "Ævar Arnfjörð Bjarmason" <ava...@gmail.com>

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.


One option may be to create  a simple 'lint' style checker that simply 
hiughlights and suggests options so the user can decide for themselves what 
they need to do. This would help span the gap between hard format and the 
soft format capabiulities of machine readable ini files, the Git config 
reader and being human readable.


Thus duplicate sections would be noted, likewise the presence of comments 
immediately preceding a section header, or terminating a section (with or 
without spacing?), etc.Such a config_lint could reside in the contrib as a 
supprt tool, and may in the long term be a guide to a common format. 
However, as noted, it would be more of a long term aspiration..





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.



--
Philip 

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

[Philip Oakley]

From: "Ævar Arnfjörð Bjarmason" <ava...@gmail.com>

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.


One option may be to create  a simple 'lint' style checker that simply
hiughlights and suggests options so the user can decide for themselves what
they need to do. This would help span the gap between hard format and the
soft format capabiulities of machine readable ini files, the Git config
reader and being human readable.

Thus duplicate sections would be noted, likewise the presence of comments
immediately preceding a section header, or terminating a section (with or
without spacing?), etc.Such a config_lint could reside in the contrib as a
supprt tool, and may in the long term be a guide to a common format.
However, as noted, it would be more of a long term aspiration..




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.



--
Philip

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 <kumbay...@arcor.de>
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: 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 <jf...@redhat.com> 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