Re: [PATCH] merge-message: change meaning of "empty merge message"

[Kevin Daudt]

On Thu, Jul 06, 2017 at 09:01:49AM +0530, Kaartic Sivaraam wrote:
> In the context of "git merge" the meaning of an "empty message"
> is one that contains no line of text. This is not in line with
> "git commit" where an "empty message" is one that contains only
> whitespaces and/or signed-off-by lines. This could cause surprises
> to users who are accustomed to the meaning of an "empty message"
> of "git commit".
> 
> Prevent such surprises by changing the meaning of an empty 'merge
> message' to be in line with that of an empty 'commit message'.
> 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  builtin/merge.c | 35 ++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 703827f00..db4bf1c40 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -748,6 +748,39 @@ static void abort_commit(struct commit_list 
> *remoteheads, const char *err_msg)
>   exit(1);
>  }
>  
> +/*
> + * Find out if the message in the strbuf contains only whitespace and
> + * Signed-off-by lines.
> + *
> + * This function is the "rest_is_space" function of "commit" with the 
> unwanted
> + * parameter removed.

The function is called "rest_is_empty".

But isn't it better that commit and merge use the same code, instead of
duplicating it again? Otherwise one may be updated, and the other
forgotten, getting differences in behaviur, which is what you want to
solve.

Kevin

Re: Should "head" also work for "HEAD" on case-insensitive FS?

[Kenneth Hsu]

On Tue, Jul 04, 2017 at 10:19:09AM +0300, Konstantin Khomoutov wrote:
> On Tue, Jul 04, 2017 at 12:00:49AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > I don't have a OSX box, but was helping a co-worker over Jabber the
> > other day, and he pasted something like:
> > 
> > $ git merge-base github/master head
> > 
> > Which didn't work for me, and I thought he had a local "head" branch
> > until realizing that of course we were just resolving HEAD on the FS.
> > 
> > Has this come up before? I think it makes sense to warn/error about
> > these magic /HEAD/ revisions if they're not upper-case.
> > 
> > This is likely unintentional and purely some emergent effect of how it's
> > implemented, and leads to unportable git invocations.
> 
> JFTR this is one common case of confusion on Windows as well.
> To the point that I saw people purposedly using "head" on StackOverflow
> questions.  That is, they appear to think (for some reason) that
> branches in Git have case-insensitive names and prefer to spell "head"
> since it (supposedly) easier to type.

The use of "head" also appears to be leading to some strange behavior
when resolving refs on Windows.  See the following issue in the
git-for-windows project:

https://github.com/git-for-windows/git/issues/1225

In summary, it seems that head and HEAD can resolve to different
revisions when in a worktree.

[PATCH] merge-message: change meaning of "empty merge message"

[Kaartic Sivaraam]

In the context of "git merge" the meaning of an "empty message"
is one that contains no line of text. This is not in line with
"git commit" where an "empty message" is one that contains only
whitespaces and/or signed-off-by lines. This could cause surprises
to users who are accustomed to the meaning of an "empty message"
of "git commit".

Prevent such surprises by changing the meaning of an empty 'merge
message' to be in line with that of an empty 'commit message'.

Signed-off-by: Kaartic Sivaraam 
---
 builtin/merge.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f00..db4bf1c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -748,6 +748,39 @@ static void abort_commit(struct commit_list *remoteheads, 
const char *err_msg)
exit(1);
 }
 
+/*
+ * Find out if the message in the strbuf contains only whitespace and
+ * Signed-off-by lines.
+ *
+ * This function is the "rest_is_space" function of "commit" with the unwanted
+ * parameter removed.
+ */
+static int message_is_empty(struct strbuf *sb)
+{
+   int i, eol;
+   const char *nl;
+
+   /* Check if the rest is just whitespace and Signed-off-by's. */
+   for (i = 0; i < sb->len; i++) {
+   nl = memchr(sb->buf + i, '\n', sb->len - i);
+   if (nl)
+   eol = nl - sb->buf;
+   else
+   eol = sb->len;
+
+   if (strlen(sign_off_header) <= eol - i &&
+   starts_with(sb->buf + i, sign_off_header)) {
+   i = eol;
+   continue;
+   }
+   while (i < eol)
+   if (!isspace(sb->buf[i++]))
+   return 0;
+   }
+
+   return 1;
+}
+
 static const char merge_editor_comment[] =
 N_("Please enter a commit message to explain why this merge is necessary,\n"
"especially if it merges an updated upstream into a topic branch.\n"
@@ -772,7 +805,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
}
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
-   if (!msg.len)
+   if (!msg.len || message_is_empty())
abort_commit(remoteheads, _("Empty commit message."));
strbuf_release(_msg);
strbuf_addbuf(_msg, );
-- 
2.11.0

Re: Requesting suggestions for a good sample "prepare-commit-msg" hook

[Kaartic Sivaraam]

On Wed, 2017-07-05 at 12:19 -0700, Junio C Hamano wrote:
> I honestly do not see your point.  Yes, I said that the change
> indicates that there is no useful example found (so far).  That does
> not necessarily mean that we must find a useful example so that we
> can keep this sample script, which now became useless, alive.  
> 
> I am questioning the assumption that it helps users to have a sample
> for prepare-commit-msg shipped with our source, and I suspect that
> it may no longer be true.  If the sole purpose of finding a useful
> example is to keep the sample script alive, when the sample script
> is no longer a useful thing to ship, then it does sound like "a
> solution looking for a problem", no?
> 
Ah, now I see your point. When you said "it's no longer useful", I
thought you were mentioning only the part of the script that commented
the sign-off. Now I get that your point of not shipping the sample
script itself. Now that clears the confusion and sorry for the
confusion I may have caused.

-- 
Kaartic

[PATCH] comment: fix a typo in the comment

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam 
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..aff6bf7aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -984,7 +984,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
int i, eol;
const char *nl;
 
-   /* Check if the rest is just whitespace and Signed-of-by's. */
+   /* Check if the rest is just whitespace and Signed-off-by's. */
for (i = start; i < sb->len; i++) {
nl = memchr(sb->buf + i, '\n', sb->len - i);
if (nl)
-- 
2.11.0

Re: [PATCH] comment: fix a typo in the comment

[Kaartic Sivaraam]

On Wed, 2017-07-05 at 11:18 -0700, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > ---
> >  Though very trivial, I wanted to correct this as I didn't
> >  want to ignore it after seeing it.
> 
> Thanks for sharp eyes.  Sign-off?  (or Sign-of? ;-))
> 
I should also thank you for your sharp eyes! BTW, this won't repeat
again as I have made 'git' worry about adding the signed-off-by to the
commits I do on my local version of git.git :)

-- 
Kaartic

Re: [PATCH v2 0/3] branch: add a --copy to go with --move

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Even though this modifies some of the same files as
> mh/packed-ref-store it looks to me like this doesn't conflict with
> that topic in any meaningful way, but I may be missing something. I
> can't get a merge between this & gitster/mh/packed-ref-store
> compiling,...

That is because each ref backend is required to implement a new
method by that topic.  The following patch needs to be squashed in
while merging the two topics, making the resulting commit an evil
merge.

FWIW, the result of applying these three patches on 'master' and
merging the result to a commit on 'pu' that is just before the merge
of previous round of the same topic, with the attached fix-up, exactly
matches the commit on 'pu' that merges the previous round, iow, I see
there is no meaningful change (perhaps other than log message?  I
didn't check) in this new round.

Note that I am not complaining that this new round does not bring
any improvements---I am not commending that you didn't screw up
while rebasing to a new commit, either ;-).

diff --git b/refs/packed-backend.c a/refs/packed-backend.c
index dc09012300..96f9141656 100644
--- b/refs/packed-backend.c
+++ a/refs/packed-backend.c
@@ -794,6 +794,13 @@ static int packed_rename_ref(struct ref_store *ref_store,
die("BUG: packed reference store does not support renaming references");
 }
 
+static int packed_copy_ref(struct ref_store *ref_store,
+  const char *oldrefname, const char *newrefname,
+  const char *logmsg)
+{
+   die("BUG: packed reference store does not support copying references");
+}
+
 static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store 
*ref_store)
 {
return empty_ref_iterator_begin();
@@ -859,6 +866,7 @@ struct ref_storage_be refs_be_packed = {
packed_create_symref,
packed_delete_refs,
packed_rename_ref,
+   packed_copy_ref,
 
packed_ref_iterator_begin,
packed_read_raw_ref,

Dear Talented

[Betty Miles]

Dear Talented,

I am Talent Scout For Sony Pictures Animation, Present Sony Pictures Animation 
Movies a Film Corporation Located in the United State, is Soliciting for the
Right to use Your Photo/Face and Personality as One of the Semi -Major
Role/ Character in our Upcoming ANIMATED Stereoscope 3D Movie-The Story
of Peter Rabbit (Peter Rabbit 2018) The Movie is Currently Filming (In
Production) Please Note That There Will Be No Auditions, Traveling or
Any Special / Professional Acting Skills, Since the Production of This
Movie Will Be Done with our State of Art Computer -Generating Imagery
Equipment. We Are Prepared to Pay the Total Sum of $560,000.00 USD. For
More Information/Understanding, Please Write us on the E-Mail Below.
CONTACT EMAIL: sonypicturesanimati...@usa.com
All Reply to:  sonypicturesanimati...@usa.com
Note: Only the Response send to this mail will be Given a Prior
Consideration.

Talent Scout
Becky Miles

[PATCH v2 2/3] branch: add test for -m renaming multiple config sections

[Ævar Arnfjörð Bjarmason]

Add a test for how 'git branch -m' handles the renaming of multiple
config sections existing for one branch.

The config format we use is hybrid machine/human editable, and we do
our best to preserve the likes of comments and formatting when editing
the file with git-config.

This adds a test for the currently expected semantics in the face of
some rather obscure edge cases which are unlikely to occur in
practice.

Helped-by: Sahil Dua 
Signed-off-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Sahil Dua 
---
 t/t3200-branch.sh | 41 +
 1 file changed, 41 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 48d152b9a9..596fbc8483 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -358,6 +358,47 @@ test_expect_success 'config information was renamed, too' '
test_must_fail git config branch.s/s.dummy
 '
 
+test_expect_success 'git branch -m correctly renames multiple config sections' 
'
+   test_when_finished "git checkout master" &&
+   git checkout -b source master &&
+
+   # Assert that a config file with multiple config sections has
+   # those sections preserved...
+   cat >expect <<-\EOF &&
+   branch.dest.key1=value1
+   some.gar.b=age
+   branch.dest.key2=value2
+   EOF
+   cat >config.branch <<\EOF &&
+;; Note the lack of -\EOF above & mixed indenting here. This is
+;; intentional, we are also testing that the formatting of copied
+;; sections is preserved.
+
+;; Comment for source. Tabs
+[branch "source"]
+   ;; Comment for the source value
+   key1 = value1
+;; Comment for some.gar. Spaces
+[some "gar"]
+;; Comment for the some.gar value
+b = age
+;; Comment for source, again. Mixed tabs/spaces.
+[branch "source"]
+;; Comment for the source value, again
+   key2 = value2
+EOF
+   cat config.branch >>.git/config &&
+   git branch -m source dest &&
+   git config -f .git/config -l | grep -F -e source -e dest -e some.gar 
>actual &&
+   test_cmp expect actual &&
+
+   # ...and that the comments for those sections are also
+   # preserved.
+   cat config.branch | sed "s/\"source\"/\"dest\"/" >expect &&
+   sed -n -e "/Note the lack/,\$p" .git/config >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'deleting a symref' '
git branch target &&
git symbolic-ref refs/heads/symref refs/heads/target &&
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 3/3] branch: add a --copy (-c) option to go with --move (-m)

[Ævar Arnfjörð Bjarmason]

From: Sahil Dua 

Add the ability to --copy a branch and its reflog and configuration,
this uses the same underlying machinery as the --move (-m) option
except the reflog and configuration is copied instead of being moved.

This is useful for e.g. copying a topic branch to a new version,
e.g. work to work-2 after submitting the work topic to the list, while
preserving all the tracking info and other configuration that goes
with the branch, and unlike --move keeping the other already-submitted
branch around for reference.

Like --move, when the source branch is the currently checked out
branch the HEAD is moved to the destination branch. In the case of
--move we don't really have a choice (other than remaining on a
detached HEAD) and in order to keep the functionality consistent, we
are doing it in similar way for --copy too.

The most common usage of this feature is expected to be moving to a
new topic branch which is a copy of the current one, in that case
moving to the target branch is what the user wants, and doesn't
unexpectedly behave differently than --move would.

One outstanding caveat of this implementation is that:

git checkout maint &&
git checkout master &&
git branch -c topic &&
git checkout -

Will check out 'maint' instead of 'master'. This is because the @{-N}
feature (or its -1 shorthand "-") relies on HEAD reflogs created by
the checkout command, so in this case we'll checkout maint instead of
master, as the user might expect. What to do about that is left to a
future change.

Helped-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Sahil Dua 
---
 Documentation/git-branch.txt |  14 ++-
 builtin/branch.c |  67 ++
 config.c | 102 
 config.h |   2 +
 refs.c   |  11 +++
 refs.h   |   9 +-
 refs/files-backend.c |  46 +++--
 refs/refs-internal.h |   4 +
 t/t3200-branch.sh| 215 +++
 9 files changed, 424 insertions(+), 46 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b77..94fd89ddc5 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -18,6 +18,7 @@ SYNOPSIS
 'git branch' (--set-upstream-to= | -u ) []
 'git branch' --unset-upstream []
 'git branch' (-m | -M) [] 
+'git branch' (-c | -C) [] 
 'git branch' (-d | -D) [-r] ...
 'git branch' --edit-description []
 
@@ -64,6 +65,10 @@ If  had a corresponding reflog, it is renamed to 
match
 renaming. If  exists, -M must be used to force the rename
 to happen.
 
+The `-c` and `-C` options have the exact same semantics as `-m` and
+`-M`, except instead of the branch being renamed it along with its
+config and reflog will be copied to a new name.
+
 With a `-d` or `-D` option, `` will be deleted.  You may
 specify more than one branch for deletion.  If the branch currently
 has a reflog then the reflog will also be deleted.
@@ -104,7 +109,7 @@ OPTIONS
In combination with `-d` (or `--delete`), allow deleting the
branch irrespective of its merged status. In combination with
`-m` (or `--move`), allow renaming the branch even if the new
-   branch name already exists.
+   branch name already exists, the same applies for `-c` (or `--copy`).
 
 -m::
 --move::
@@ -113,6 +118,13 @@ OPTIONS
 -M::
Shortcut for `--move --force`.
 
+-c::
+--copy::
+   Copy a branch and the corresponding reflog.
+
+-C::
+   Shortcut for `--copy --force`.
+
 --color[=]::
Color branches to highlight current, local, and
remote-tracking branches.
diff --git a/builtin/branch.c b/builtin/branch.c
index c958e93257..684f2c4f42 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -28,6 +28,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [] [-l] [-f]  []"),
N_("git branch [] [-r] (-d | -D) ..."),
N_("git branch [] (-m | -M) [] "),
+   N_("git branch [] (-c | -C) [] "),
N_("git branch [] [-r | -a] [--points-at]"),
N_("git branch [] [-r | -a] [--format]"),
NULL
@@ -450,15 +451,19 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
-static void rename_branch(const char *oldname, const char *newname, int force)
+static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
int clobber_head_ok;
 
-   if (!oldname)
-   die(_("cannot rename the current branch while not on any."));
+   if (!oldname) {
+   if (copy)
+   

[PATCH v2 1/3] config: create a function to format section headers

[Ævar Arnfjörð Bjarmason]

From: Sahil Dua 

Factor out the logic which creates section headers in the config file,
e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.

This introduces no function changes, but is needed for a later change
which adds support for copying branch sections in the config file.

Signed-off-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Ramsay Jones 
Signed-off-by: Sahil Dua 
---
 config.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 4638b0696a..b7afb5941b 100644
--- a/config.c
+++ b/config.c
@@ -2244,10 +2244,10 @@ static int write_error(const char *filename)
return 4;
 }
 
-static int store_write_section(int fd, const char *key)
+static struct strbuf store_create_section(const char *key)
 {
const char *dot;
-   int i, success;
+   int i;
struct strbuf sb = STRBUF_INIT;
 
dot = memchr(key, '.', store.baselen);
@@ -2263,6 +2263,15 @@ static int store_write_section(int fd, const char *key)
strbuf_addf(, "[%.*s]\n", store.baselen, key);
}
 
+   return sb;
+}
+
+static int store_write_section(int fd, const char *key)
+{
+   int success;
+
+   struct strbuf sb = store_create_section(key);
+
success = write_in_full(fd, sb.buf, sb.len) == sb.len;
strbuf_release();
 
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 0/3] branch: add a --copy to go with --move

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 05 2017, Junio C. Hamano jotted:

> * sd/branch-copy (2017-06-18) 3 commits
>  - branch: add a --copy (-c) option to go with --move (-m)
>  - branch: add test for -m renaming multiple config sections
>  - config: create a function to format section headers
>
>  "git branch" learned "-c/-C" to create and switch to a new branch
>  by copying an existing one.
>
>  Has a bit of interaction with mh/packed-ref-store and bw/config-h,
>  so perhaps needs to wait for the former to stabilize a bit more
>  and possibly rebasing on them.

Now that bw/config-h has landed in master here's a version that's
rebased on that. No changes from v1 except:

 - moving the new config header addition from cache.h to config.h,
   corresponding to what was done in bw/config-h.

 - fixing a trivial comment whitespace issue which I see you applied
   locally.

Even though this modifies some of the same files as
mh/packed-ref-store it looks to me like this doesn't conflict with
that topic in any meaningful way, but I may be missing something. I
can't get a merge between this & gitster/mh/packed-ref-store
compiling, but that's due to issues in the latter which seem to be
fixed by some subsequent merge/fixup in pu, not something to do with a
genuine conflict with this topic.

Hopefully this'll allow this topic to land in 2.14.

Sahil Dua (2):
  config: create a function to format section headers
  branch: add a --copy (-c) option to go with --move (-m)

Ævar Arnfjörð Bjarmason (1):
  branch: add test for -m renaming multiple config sections

 Documentation/git-branch.txt |  14 ++-
 builtin/branch.c |  67 ---
 config.c | 115 +++
 config.h |   2 +
 refs.c   |  11 ++
 refs.h   |   9 +-
 refs/files-backend.c |  46 ++--
 refs/refs-internal.h |   4 +
 t/t3200-branch.sh| 256 +++
 9 files changed, 476 insertions(+), 48 deletions(-)

-- 
2.13.1.611.g7e3b11ae1

Re: What's cooking in git.git (Jul 2017, #01; Wed, 5)

[Stefan Beller]

> [Graduated to "master"]
>
> * bw/repo-object (2017-06-23) 21 commits
...
>
>  Introduce a "repository" object to eventually make it easier to
>  work in multiple repositories (the primary focus is to work with
>  the superproject and its submodules) in a single process.

It's pretty rad to see this advancing to master.
FYI: I started working on teaching the object store how to work
with repository objects. This would allow us to get rid of hacks in
submodule.c: namely add_submodule_odb, which adds submodule
objects to the (main) object store for processing. Ideally we want
to free the objects of a submodule once we are done with a submodule.
(or integrate it into our try_to_free_routine)

> * sb/hashmap-cleanup (2017-07-05) 10 commits
...
>  Will wait for feedback, then merge to and cook in 'next'.

Thanks.

> * sb/pull-rebase-submodule (2017-06-27) 4 commits
>  - builtin/fetch cleanup: always set default value for submodule recursing
>  - pull: optionally rebase submodules (remote submodule changes only)
>  - builtin/fetch: parse recurse-submodules-default at default options parsing
>  - builtin/fetch: factor submodule recurse parsing out to submodule config
>
>  "git pull --rebase --recurse-submodules" learns to rebase the
>  branch in the submodules to an updated base.

Speaking of submodules, It's not just features, but I also send bug fixes. ;)
https://public-inbox.org/git/20170630003851.17288-1-sbel...@google.com/
(That patch is not related to this series, except for working in the submodule
area, but I consider that patch more important than e.g. this series.)

> * sb/submodule-doc (2017-06-22) 1 commit
>  - submodules: overhaul documentation
>
>  Doc update.
>
>  What's the status of this thing?

There was some review on the list (mostly from Brandon and Jonathan T.),
but I felt like it was bikeshedding, as there is no black/white correctness
with words. (Same for code, but for code it is easier to come to a
consensus at least.)

So I had a couple of internal rounds with them on a Google doc, hence
I assume they agree on this patch being ok as-is.  But it has been a while
I can reread it myself to check. But I guess most valuable input
would come from others.

> * sb/diff-color-move (2017-06-30) 26 commits
...
>  Will merge to 'next'.

cool. Let's see how a larger audience reacts to this one. Maybe there
is more input for a good heuristic.

Thanks,
Stefan

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Junio C Hamano]

Jeff King  writes:

>> > +test_expect_failure 'walking multiple reflogs shows both' '
>> > +  {
>> > +  do_walk HEAD &&
>> > +  do_walk side
>> > +  } >expect &&
>> > +  do_walk HEAD side >actual &&
>> > +  test_cmp expect actual
>> > +'
>> 
>> I somehow find this one a bit iffy.  
>> 
>> The order that commits appear in the "walk from HEAD and side at the
>> same time" may want to be different from what this test expects.
>> "rev-list --since=3.days -g master next", for example, would want to
>> refrain from reading the reflog of 'master' for all 90 days before
>> switching to the reflog of 'next', no?
>
> I did make the ordering intentional. We process each reflog sequentially
> in turn. I don't think it would be wrong to interleave them by date, but
> I actually think it's useful for somebody who switches that behavior to
> consciously update the test. Maybe it's worth calling out in the commit
> message.
>
> I stopped short of actually documenting and guaranteeing that behavior
> to the user. I don't know how comfortable we would be changing it later.

I somehow feel that the "showing all entries from HEAD and then
showing all from side" is simply a buggy behaviour.  I do not think
our users would terribly mind if we changed it later.  But I may be
missing the reason why (sometimes?) the sequential behaviour may be
useful.

> (As an aside, I also prowled through the documentation looking for any
> guarantees we make to the user about the fake-parent thing, but I
> couldn't find any. So I considered its user-visible effects an unwanted
> side effect of the implementation).

Yes, I think the corner cases you documented here in these tests are
not something we desired to have in the first place.  Rather they
are merely fallouts from the hackyness of the implementation.

What's cooking in git.git (Jul 2017, #01; Wed, 5)

[Junio C Hamano]

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

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

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

--
[Graduated to "master"]

* bw/repo-object (2017-06-23) 21 commits
  (merged to 'next' on 2017-06-26 at ed9c0b77c3)
 + ls-files: use repository object
 + repository: enable initialization of submodules
 + submodule: convert is_submodule_initialized to work on a repository
 + submodule: add repo_read_gitmodules
 + submodule-config: store the_submodule_cache in the_repository
 + repository: add index_state to struct repo
 + config: read config from a repository object
 + path: add repo_worktree_path and strbuf_repo_worktree_path
 + path: add repo_git_path and strbuf_repo_git_path
 + path: worktree_git_path() should not use file relocation
 + path: convert do_git_path to take a 'struct repository'
 + path: convert strbuf_git_common_path to take a 'struct repository'
 + path: always pass in commondir to update_common_dir
 + path: create path.h
 + environment: store worktree in the_repository
 + environment: place key repository state in the_repository
 + repository: introduce the repository object
 + environment: remove namespace_len variable
 + setup: add comment indicating a hack
 + setup: don't perform lazy initialization of repository state
 + Merge branches 'bw/ls-files-sans-the-index' and 'bw/config-h' into 
bw/repo-object

 Introduce a "repository" object to eventually make it easier to
 work in multiple repositories (the primary focus is to work with
 the superproject and its submodules) in a single process.


* cc/shared-index-permfix (2017-06-25) 3 commits
  (merged to 'next' on 2017-06-26 at bb41584bf0)
 + t1700: make sure split-index respects core.sharedrepository
 + t1301: move modebits() to test-lib-functions.sh
 + read-cache: use shared perms when writing shared index

 The split index code did not honor core.sharedrepository setting
 correctly.


* jt/unify-object-info (2017-06-26) 8 commits
  (merged to 'next' on 2017-06-26 at 540ea81983)
 + sha1_file: refactor has_sha1_file_with_flags
 + sha1_file: do not access pack if unneeded
 + sha1_file: teach sha1_object_info_extended more flags
 + sha1_file: refactor read_object
 + sha1_file: move delta base cache code up
 + sha1_file: rename LOOKUP_REPLACE_OBJECT
 + sha1_file: rename LOOKUP_UNKNOWN_OBJECT
 + sha1_file: teach packed_object_info about typename

 Code clean-ups.


* rs/sha1-name-readdir-optim (2017-06-24) 4 commits
  (merged to 'next' on 2017-06-26 at a70587f2b9)
 + sha1_file: guard against invalid loose subdirectory numbers
 + sha1_file: let for_each_file_in_obj_subdir() handle subdir names
 + p4205: add perf test script for pretty log formats
 + sha1_name: cache readdir(3) results in find_short_object_filename()

 Optimize "what are the object names already taken in an alternate
 object database?" query that is used to derive the length of prefix
 an object name is uniquely abbreviated to.

--
[New Topics]

* bc/object-id (2017-07-04) 12 commits
 - sha1_name: convert GET_SHA1* flags to GET_OID*
 - sha1_name: convert get_sha1* to get_oid*
 - Convert remaining callers of get_sha1 to get_oid.
 - builtin/verify-tag: convert to struct object_id
 - builtin/unpack-file: convert to struct object_id
 - bisect: convert bisect_checkout to struct object_id
 - builtin/update_ref: convert to struct object_id
 - sequencer: convert to struct object_id
 - remote: convert struct push_cas to struct object_id
 - submodule: convert submodule config lookup to use object_id
 - builtin/merge-tree: convert remaining caller of get_sha1 to object_id
 - builtin/fsck: convert remaining caller of get_sha1 to object_id

 Conversion from uchar[20] to struct object_id continues.


* jk/reflog-walk (2017-07-05) 7 commits
 - reflog-walk: stop using fake parents
 - rev-list: check reflog_info before showing usage
 - get_revision_1(): replace do-while with an early return
 - log: do not free parents when walking reflog
 - [SQUASH LOG MESSAGE ONLY] t1414: document some reflog-walk oddities
 - t1414: document some reflog-walk oddities
 - Merge branch 'jk/reflog-walk-maint' into jk/reflog-walk
 (this branch uses jk/reflog-walk-maint.)

 Numerous bugs in walking of reflogs via "log -g" and friends have
 been fixed.


* jk/reflog-walk-maint (2017-07-05) 1 commit
 - reflog-walk: skip over double-null oid due to HEAD rename
 (this branch is used by jk/reflog-walk.)

 After "git branch --move" of the currently checked out branch, the
 code to walk the reflog of HEAD via "log -g" and friends
 incorrectly stopped at the reflog entry that records the renaming
 of 

Re: [PATCH 5/6] rev-list: check reflog_info before showing usage

[Jeff King]

On Wed, Jul 05, 2017 at 11:07:35AM -0700, Junio C Hamano wrote:

> > This is actually the main "gotcha" I'm worried about with this series.
> > I'm not sure if any other code would care about seeing the pending items
> > in revs->commits. I still think the series is the right direction; if
> > there is such a place, we'd want to teach it to handle reflog walking in
> > a similar way, too.
> 
> Ah, very good thinking---when I was following along your drafts to
> bypass the revs.commits queue for reflog walking, I didn't think of
> this one.  
> 
> Perhaps "!revs.commits && reflog_walk_empty(revs.reflog_info)" may
> want to become a macro "walk_finished()" or something to
> replace existing !revs.commits that is not accompanied by the check
> on .reflog_info field?

Yeah, I had the same thought. But I don't think it's really worth adding
a helper if we don't end up with another call-site. So I was stalling to
see if that happened.

(And just for reference in case we do end up adding it, it's not so much
walk_finished() as walk_empty_in_the_first_place()).

-Peff

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Jeff King]

On Wed, Jul 05, 2017 at 09:05:14PM +0100, Ramsay Jones wrote:

> On 05/07/17 09:00, Jeff King wrote:
> > Since its inception, the general strategy of the reflog-walk
> > code has been to start with the tip commit for the ref, and
> > as we traverse replace each commit's parent pointers with
> > fake parents pointing to the previous reflog entry.
> > 
> > This lets us traverse the reflog as if it were a real
> > history, but it has some user-visible oddities. Namely:
> > 
> >   1. The fake parents are used for commit selection and
> >  display. So for example, "--merges" or "--no-merges"
> >  are useful, because the history appears as a linear
> >  string. Likewise, pathspec limiting is based on the
> >  diff between adjacent entries, not the changes actually
> >  introduced by a commit.
> > 
> >  These are often the same (e.g., because the entry was
> >  just running "git commit" and the adjacent entry _is_
> >  the true parent), but it may not be in several common
> >  cases. For instance, using "git reset" to jump around
> >  history, or "git checkout" to move HEAD.
> > 
> >   2. We reverse-map each commit back to its reflog. So when
> >  it comes time to show commit X, we say "a-ha, we added
> >  X because it was at the tip of the 'foo' reflog, so
> >  let's show the foo reflog". But this leads to nonsense
> >  results when you ask to traverse multiple reflogs: if
> >  two reflogs have the same tip commit, we only map back
> >  to one of them.
> > 
> >  Instead, we should show each reflog individually, in
> >  the order the user gave us on the command line.
> > 
> >   2. If the tip of the reflog and the ref tip disagree on
> ^^
> It seems hard to get off the second point! ;-)

Heh. You know, I even remember checking that I didn't screw that up
(because I originally wrote the first and third, and later went back to
add in the second point). But somehow I botched the proofreading, too.

I'll plan to re-roll this to update the little bits you and Junio have
pointed out. Junio, I'll probably just do the whole thing on "maint" and
then the merge-up to "master" should be straight-forward, I'd think.

-Peff

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Jeff King]

On Wed, Jul 05, 2017 at 10:56:42AM -0700, Junio C Hamano wrote:

> >   1. The fake parents are used for commit selection and
> >  display. So for example, "--merges" or "--no-merges"
> >  are useful, because the history appears as a linear
> 
> s/useful/useless/ perhaps?

Oops, yes ("not useful" is probably what I was going for).

> > +test_expect_success 'set up some reflog entries' '
> > +   test_commit one &&
> > +   test_commit two &&
> > +   git checkout -b side HEAD^ &&
> > +   test_commit three &&
> > +   git merge --no-commit master &&
> > +   echo evil-merge-content >>one.t &&
> > +   test_tick &&
> > +   git commit --no-edit -a
> > +'
> 
> Mental note: logically, what we want to see in the log are:
> 
> master:  one-->two
> side:one-->three-->(evil)
> HEAD:one-->two-->one-->three-->(evil)
> 
> where the middle one in HEAD is "switching from master to side".

Yeah. I was tempted to document that, but I think the expect.all mostly
does that for HEAD (and I don't really check the others). The grossest
thing is this numeric selection:

> > +test_expect_failure 'pathspec limiting handles merges' '
> > +   sed -n "1p;3p;5p" expect.all >expect &&

I tried to think of an easy way to pick them out by name but couldn't
come up with one. I don't know if that sed invocation deserves a comment
or not.

> > +   do_walk -- one.t >actual &&
> > +   test_cmp expect actual
> > +'
> 
> OK (it was a bit tricky to see why the topmost one should, but the
> evilness of the merge makes it eligible).

Yeah, that was why I added the evilness. A more real-world test would
perhaps be a file with an actual conflict that got resolved (but not
matching either parent exactly). I actually started by adding the evil
content to "three", which is a bit closer. But it passes even without
the patch, because the diff to the first parent still matches.

So I dunno. I think it's OK as is. My main goal was just to make sure
that my TREESAME hackery catches both normal and merge commits.

> > +test_expect_failure 'walking multiple reflogs shows both' '
> > +   {
> > +   do_walk HEAD &&
> > +   do_walk side
> > +   } >expect &&
> > +   do_walk HEAD side >actual &&
> > +   test_cmp expect actual
> > +'
> 
> I somehow find this one a bit iffy.  
> 
> The order that commits appear in the "walk from HEAD and side at the
> same time" may want to be different from what this test expects.
> "rev-list --since=3.days -g master next", for example, would want to
> refrain from reading the reflog of 'master' for all 90 days before
> switching to the reflog of 'next', no?

I did make the ordering intentional. We process each reflog sequentially
in turn. I don't think it would be wrong to interleave them by date, but
I actually think it's useful for somebody who switches that behavior to
consciously update the test. Maybe it's worth calling out in the commit
message.

I stopped short of actually documenting and guaranteeing that behavior
to the user. I don't know how comfortable we would be changing it later.

(As an aside, I also prowled through the documentation looking for any
guarantees we make to the user about the fake-parent thing, but I
couldn't find any. So I considered its user-visible effects an unwanted
side effect of the implementation).

-Peff

Re: [PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename

[Jeff King]

On Wed, Jul 05, 2017 at 10:34:03AM -0700, Junio C Hamano wrote:

> > +   if (!logobj)
> > +   logobj = parse_object(>ooid);
> > }
> 
> For the current 'maint', this would need to be backported to the
> uchar[20] interface (which is trivial to do, and merging it upwards
> while adjusting it back to "struct object_id" is also trivial; there
> is no need to resend).
> 
> Thanks.  Will queue.

Ah, right. I should probably actually apply the patches on "maint"
before suggesting you do so. ;)

In theory it should be applied directly on 39ee4c6c2 and then merged up
(but I think the same wiggling would apply either way).

The rest of the patches would need to be adjusted on top, but it should
be easy to resolve; all of that code just goes away.

-Peff

Re: [PATCH v3 1/1] cygwin: Allow pushing to UNC paths

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
>  cygwin can use an UNC path like //server/share/repo
>  $ cd //server/share/dir
>  $ mkdir test
>  $ cd test
>  $ git init --bare
>
>  However, when we try to push from a local Git repository to this repo,
>  there is a problem: Git converts the leading "//" into a single "/".
>
>  As cygwin handles an UNC path so well, Git can support them better:
>  - Introduce cygwin_offset_1st_component() which keeps the leading "//",
>similar to what Git for Windows does.
>  - Move CYGWIN out of the POSIX in the tests for path normalization in t0060
>
> Signed-off-by: Torsten Bögershausen 
> ---
>
> I think I skip all the changing in setup.c and cygwin_access() for the
> moment:
> - It is not clear, what is a regression and what is an improvement
> - It may be a problem that could be solved in cygwin itself
> - I was able to push a an UNC path on a Windows server
>   when the domain controller was reachable.

OK.  

It certainly makes it simpler to improve things one at a time ;-)

> diff --git a/compat/cygwin.h b/compat/cygwin.h
> new file mode 100644
> index 000..8e52de4
> --- /dev/null
> +++ b/compat/cygwin.h
> @@ -0,0 +1,2 @@
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component

I originally found this somewhat iffy, but decided to take it as-is.
But we may want to fix it (and the original sin that brought this
file into this shape) later.

The reason of "iffy"-ness I felt were twofold:

 - This header file is only two lines long and is designed to be
   included at a single place (git-compat-util.h).  It might be
   better to inline its contents directly there, if we do not expect
   it to grow aggressively in the future.

 - If it is to be a header file on its own, it should have the
   standard double-inclusion guard

#ifndef COMPAT_CYGWIN_H
#define COMPAT_CYGWIN_H
...
#endif /* COMPAT_CYGWIN_H */

  around it.

I see this was modeled after existing compat/mingw.h, which is
sufficiently large that it deserves to be its own header, but then
it should have the double-inclusion guard around it.  

I am neutral between inlining the contents of cygwin.h to where it
is included and keeping the organization this patch proposes.  If we
anticipate futher development on Cygwin, perhaps having the header
as a separate file, even if it starts small, makes sense, so I do
not mind it.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 047172d..db9c22d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -189,6 +189,9 @@
>  #include 
>  #endif
>  
> +#if defined(__CYGWIN__)
> +#include "compat/cygwin.h"
> +#endif
>  #if defined(__MINGW32__)
>  /* pull in Windows compatibility stuff */
>  #include "compat/mingw.h"


Thanks.

Re: [PATCH 00/10] Friday night special: hash map cleanup

[Junio C Hamano]

Stefan Beller  writes:

> This goes on top of sb/hashmap-customize-comparison.
> No functional impact but a pure cleanup series:
> No casts to hashmap_cmp_fn in the whole code base any more.
>
> This revealed another interesting thing, which is not a bug per se:
> The const correctness of hashmap_cmp_fn as it requires all its void
> pointers to be const!
>
> We violate this in patch-ids.c as we modify the `fndata` at some
> uncritical part (a part that doesn't change the equal-functionality
> AFAICT).

I am undecided, but perhaps we should loosen that, if some real-world
user has a legitimate need to take a mutable fndata?

I dunno.  The patches look good to me.

Thanks.

Re: [PATCH] hooks: add signature using "interpret-trailers"

[Ramsay Jones]

On 05/07/17 18:35, Kaartic Sivaraam wrote:
> The sample hook to prepare the commit message before
> a commit allows users to opt-in to add the signature
> to the commit message. The signature is added at a place
> that isn't consistent with the "-s" option of "git commit".
> Further, it could go out of view in certain cases.
> 
> Add the signature in a way similar to "-s" option of
> "git commit" using git's interpret-trailers command.
> 
> It works well in all cases except when the user invokes
> "git commit" without any arguments. In that case manually
> add a new line after the first line to ensure it's consistent
> with the output of "-s" option.
> 
> While at it, name the input parameters to improve readability
> of script.

I assume each occurrence of 'signature' in the commit message,
including the subject, should be 'sign-off' (or Signed-off-by)
instead. Yes?

(when I hear 'signature', I think GPG signature).

ATB,
Ramsay Jones

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Ramsay Jones]

On 05/07/17 09:00, Jeff King wrote:
> Since its inception, the general strategy of the reflog-walk
> code has been to start with the tip commit for the ref, and
> as we traverse replace each commit's parent pointers with
> fake parents pointing to the previous reflog entry.
> 
> This lets us traverse the reflog as if it were a real
> history, but it has some user-visible oddities. Namely:
> 
>   1. The fake parents are used for commit selection and
>  display. So for example, "--merges" or "--no-merges"
>  are useful, because the history appears as a linear
>  string. Likewise, pathspec limiting is based on the
>  diff between adjacent entries, not the changes actually
>  introduced by a commit.
> 
>  These are often the same (e.g., because the entry was
>  just running "git commit" and the adjacent entry _is_
>  the true parent), but it may not be in several common
>  cases. For instance, using "git reset" to jump around
>  history, or "git checkout" to move HEAD.
> 
>   2. We reverse-map each commit back to its reflog. So when
>  it comes time to show commit X, we say "a-ha, we added
>  X because it was at the tip of the 'foo' reflog, so
>  let's show the foo reflog". But this leads to nonsense
>  results when you ask to traverse multiple reflogs: if
>  two reflogs have the same tip commit, we only map back
>  to one of them.
> 
>  Instead, we should show each reflog individually, in
>  the order the user gave us on the command line.
> 
>   2. If the tip of the reflog and the ref tip disagree on
^^
It seems hard to get off the second point! ;-)

ATB,
Ramsay Jones

Re: [PATCH] hooks: replace irrelevant hook sample

[Junio C Hamano]

Kaartic Sivaraam  writes:

> +sed -e '/^# Please enter the .*/ {
> +  N
> +  N
> +  d
> +}' "$1" >'.sed-output.temp' && mv '.sed-output.temp' "$1"

Three things that caught my eyes:

 - Between "git commit --cleanup=strip" and "git commit --cleanup=verbatim",
   lines that make up this initial instruction section are different.

 - "git grep 'Please enter the '" finds that this string is subject
   to translation, so the pattern may not match (in which case it
   will be a no-op without doing any harm, which is OK).

 - core.commentChar can be set to something other than '#', so the
   pattern may not match (I do not offhand know if that may cause a
   wrong line to match, causing harm, or not).

As merely an example, it probably is OK to say "this won't work if
you are not using the C locale, and/or you are using custom
core.commentChar".  So if we disregard the latter two, I would think

sed -e '/^# Please enter the commit message /,/^#$/d'

may be simpler to reason about to achieve the same goal.  

Thanks.

Re: [PATCH] hooks: add signature using "interpret-trailers"

[Junio C Hamano]

Kaartic Sivaraam  writes:

> The sample hook to prepare the commit message before
> a commit allows users to opt-in to add the signature
> to the commit message. The signature is added at a place
> that isn't consistent with the "-s" option of "git commit".
> Further, it could go out of view in certain cases.
>
> Add the signature in a way similar to "-s" option of
> "git commit" using git's interpret-trailers command.

OK.

> It works well in all cases except when the user invokes
> "git commit" without any arguments. In that case manually
> add a new line after the first line to ensure it's consistent
> with the output of "-s" option.

OK (but its execution can be simplified).

> While at it, name the input parameters to improve readability
> of script.

Good.

>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Added a close quote that got missed in the previous patch.
>
>  templates/hooks--prepare-commit-msg.sample | 19 +++
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/templates/hooks--prepare-commit-msg.sample 
> b/templates/hooks--prepare-commit-msg.sample
> index 86b8f227e..4ddab7896 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -20,17 +20,28 @@
>  # The third example adds a Signed-off-by line to the message, that can
>  # still be edited.  This is rarely a good idea.
>  
> -case "$2,$3" in
> +COMMIT_MSG_FILE=$1
> +COMMIT_SOURCE=$2
> +SHA1=$3
> +NEW_LINE='\
> +'
> +SED_TEMP_FILE='.sed-output.temp'

Move the latter two variable definitions to where they matter,
i.e. the commented-out part that computes and adds $SOB.

> +case "$COMMIT_SOURCE,$SHA1" in
>merge,)
> -@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; 
> print' "$1" ;;
> +@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; 
> print' "$COMMIT_MSG_FILE" ;;
>  
>  # ,|template,)
>  #   @PERL_PATH@ -i.bak -pe '
>  #  print "\n" . `git diff --cached --name-status -r`
> -# if /^#/ && $first++ == 0' "$1" ;;
> +# if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
>  
>*) ;;
>  esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
> \1/p')
> -# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
> +# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
> +# if [ -z "$COMMIT_SOURCE" ]
> +# then

In our codebase (see Documentation/CodingGuidelines) we tend to
prefer "test" over "[".  I.e.

if test -z "$COMMIT_SOURCE"
then

> +#  sed -e "1i$NEW_LINE" "$COMMIT_MSG_FILE" >"$SED_TEMP_FILE" && mv 
> "$SED_TEMP_FILE" "$COMMIT_MSG_FILE"

This looks like a more complex way to say

{
echo 
cat "$COMMIT_MSG_FILE"
} >"$SED_TEMP_FILE" &&
mv "$SED_TEMP_FILE" "$COMMIT_MSG_FILE"

I wondered if it is safe to use a single hardcoded "$SED_TEMP_FILE",
but I think it is OK.

> +# fi

Thanks.

Re: Requesting suggestions for a good sample "prepare-commit-msg" hook

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> On Wed, 2017-07-05 at 10:00 -0700, Junio C Hamano wrote:
>> > I am not so sure that we are searching for them, to be honest (who
>> > are we in this context anyway?)
>> 
> Imagining HTML could be used in plain-text,
>
>  I think I misinterpreted your sentence in one of the other
> mails (found below),  Sorry for that sloppy sentence. More
> correctly, the "we" in that context is the same as the "we" in the
> context of the text quoted below,
>
>> That sounds like a sample that is there not because it would be
>> useful, but because we couldn't think of any useful example.
>> 
>
> Link to the post that has the quoted text,
> http://public-inbox.org/git/%3cxmqqy3s7nbkm@gitster.mtv.corp.google.com%3E/

I honestly do not see your point.  Yes, I said that the change
indicates that there is no useful example found (so far).  That does
not necessarily mean that we must find a useful example so that we
can keep this sample script, which now became useless, alive.  

I am questioning the assumption that it helps users to have a sample
for prepare-commit-msg shipped with our source, and I suspect that
it may no longer be true.  If the sole purpose of finding a useful
example is to keep the sample script alive, when the sample script
is no longer a useful thing to ship, then it does sound like "a
solution looking for a problem", no?

Re: [PATCH] status: suppress additional warning output in plumbing modes

[Stefan Beller]

On Sat, Jul 1, 2017 at 10:35 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> When status is called with '--porcelain' (as implied by '-z'), we promise
>> to output only messages as described in the man page.
>>
>> Suppress CRLF warnings.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>> Maybe something like this?
>
> This looks to me like a stimulus having enough time to go to the
> spinal cord to induce a knee-jerk reaction, without giving a chance
> to the brain to think things through.
>

sort of.

> Surely the reported symptom may have only been about CRLF, but who
> says that would be the only kind of warning that would be seen
> during "status --porcelain" codepath?

I was slightly worried about this, too.

>
> I tend to agree with Ævar's "output for the script can be read from
> our standard output" should probably be our first response.
>
> The patch _is_ a good start to document that we may want to do
> something differently under _PORCELAIN output modes and one location
> in the code that may be a good place to make that decision, but if
> we are to squelch the warnings, we should make sure we do not give
> any warning, not limited to squelching the safe-crlf warning, to the
> standard error, but still diagnose errors and show error messages,
> or something like that, I would think.

So for now we'd rather want to go with a documentation patch first
and then the refinement of the porcelain mode of potentially
suppressing more warnings?

Note that this patch was a one-off by me, so I no longer pursue
fixing the problem here, someone else is kindly asked to step up.

Thanks,
Stefan

Re: [PATCH] push: add config option to --force-with-lease by default.

[Mike Rappazzo]

On Wed, Jul 5, 2017 at 12:43 PM, Francesco Mazzoli  wrote:
>> On 5 Jul 2017, at 17:17, Junio C Hamano  wrote:
>>
>> The take-away lesson that the earlier thread gave me was that the
>> order in which the three options are ranked by their desirebility
>> in the UI (and the order we would like to encourage users to use)
>> is, from the most to the least preferrable:
>>
>> - "--force-with-lease=:" that is safer than "--force";
>>
>> - "--force" that is known to be dangerous, and does not pretend to
>>   be anything but;
>>
>> - "--force-with-lease" that pretends to be safer but is not.
>>
>> The last form should eventually be eliminated, as there is no way to
>> correctly intuit what the expected object should be.
>
> What's not clear to me is what the intended workflow using
> `--force-with-lease=:` is. Intuitively it seems extremely
> cumbersome to manually pluck a revision each time, especially when
> dealing with commits that all have the same description.
>
> On the other hand for my workflow `--force-with-lease` works quite well
> because I tend to use it in cases where me and a colleague are working
> on the same PR, and thus I'm not doing anything else (including fetching).
>
> Moreover, it seems to me that the problem `--force-with-lease` is
> just one of marketing. `--force-with-lease` is strictly more "safe"
> than `--force` in the sense that it'll reject some pushes that `--force`
> will let through. I think that if we advertise it better including its
> drawbacks it can still be better than no checks at all.
>
> Francesco

I am in your camp on this, and I will also only ever explicitly fetch.
I would hate for --force-with-lease to disappear.

However, I believe that the problem is that there are many third party
tools which do a fetch behind the scenes (for example Atlassian
SourceTree).  This can update the local refs without a user
necessarily thinking about it.  This can lead to a force-with-lease
being used unsafely (without the stated lease).

_Mike

Re: [PATCH 11/12] sha1_name: convert get_sha1* to get_oid*

[Junio C Hamano]

Stefan Beller  writes:

>> @@ -636,7 +636,7 @@ static int get_sha1_basic(const char *str, int len, 
>> unsigned char *sha1,
>> int detached;
>>
>> if (interpret_nth_prior_checkout(str, len, ) > 0) {
>> -   detached = (buf.len == 40 && !get_sha1_hex(buf.buf, 
>> sha1));
>> +   detached = (!get_oid_hex(buf.buf, oid));
>
> omitting the length check here?

Good eyes.  It probably should check with the possible oid lengths.

Re: Requesting suggestions for a good sample "prepare-commit-msg" hook

[Kaartic Sivaraam]

> On Wed, 2017-07-05 at 10:00 -0700, Junio C Hamano wrote:
> > I am not so sure that we are searching for them, to be honest (who
> > are we in this context anyway?)
> 
Imagining HTML could be used in plain-text,

 I think I misinterpreted your sentence in one of the other
mails (found below),  Sorry for that sloppy sentence. More
correctly, the "we" in that context is the same as the "we" in the
context of the text quoted below,

> That sounds like a sample that is there not because it would be
> useful, but because we couldn't think of any useful example.
> 

Link to the post that has the quoted text,
http://public-inbox.org/git/%3cxmqqy3s7nbkm@gitster.mtv.corp.google.com%3E/

Re: [PATCH] comment: fix a typo in the comment

[Junio C Hamano]

Kaartic Sivaraam  writes:

> ---
>  Though very trivial, I wanted to correct this as I didn't
>  want to ignore it after seeing it.

Thanks for sharp eyes.  Sign-off?  (or Sign-of? ;-))

>
>  builtin/commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac0..aff6bf7 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -984,7 +984,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
>   int i, eol;
>   const char *nl;
>  
> - /* Check if the rest is just whitespace and Signed-of-by's. */
> + /* Check if the rest is just whitespace and Signed-off-by's. */
>   for (i = start; i < sb->len; i++) {
>   nl = memchr(sb->buf + i, '\n', sb->len - i);
>   if (nl)

Re: [PATCH 5/6] rev-list: check reflog_info before showing usage

[Junio C Hamano]

Jeff King  writes:

> When git-rev-list sees no pending commits, it shows a usage
> message. This works even when reflog-walking is requested,
> because the reflog-walk code currently puts the reflog tips
> into the pending queue.
>
> In preparation for refactoring the reflog-walk code, let's
> explicitly check whether we have any reflogs to walk. For
> now this is a noop, but the existing reflog tests will make
> sure that it kicks in after the refactoring. Likewise, we'll
> add a test that "rev-list -g" without specifying any reflogs
> continues to fail (so that we know our check does not kick
> in too aggressively).
>
> Note that the implementation needs to go into its own
> sub-function, as the walk code does not expose its innards
> outside of reflog-walk.c.
>
> Signed-off-by: Jeff King 
> ---
> This is actually the main "gotcha" I'm worried about with this series.
> I'm not sure if any other code would care about seeing the pending items
> in revs->commits. I still think the series is the right direction; if
> there is such a place, we'd want to teach it to handle reflog walking in
> a similar way, too.

Ah, very good thinking---when I was following along your drafts to
bypass the revs.commits queue for reflog walking, I didn't think of
this one.  

Perhaps "!revs.commits && reflog_walk_empty(revs.reflog_info)" may
want to become a macro "walk_finished()" or something to
replace existing !revs.commits that is not accompanied by the check
on .reflog_info field?

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 05 2017, Stefan Beller jotted:

> On Tue, Jul 4, 2017 at 6:56 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
>>>  wrote:
>>>

 I think some invocation of "git submodule update ???" will do the same,
 but I can't from the docs see what that is right now.
>>>
>>> '--remote' is what you are looking for.
>>>
>>> When we have the branch field configured, the workflow for *creating*
>>> the patch sent
>>> to Junio might be different than it currently is. Currently, you would
>>> send a patch that is
>>> produced as:
>>>
>>>   git -C sha1collisiondetection pull origin master
>>>   git add sha1collisiondetection
>>>   git commit -m "serious reasoning in the commit message"
>>>
>>> This could be 'simplified' to
>>>
>>>   git submodule update --remote
>>>   git add sha1collisiondetection
>>>   git commit -m "serious reasoning in the commit message"
>>>
>>> but as we had different branches in the submodule field,
>>> I wonder how much of an ease this is.
>>>
>>> For Junio the workflow stays the same, as he would just
>>> apply the patch, so I understand why he might see the
>>> branch field as pollution.
>>
>> My reaction was more about "the rest of us", not me or those who
>> choose to bind a new/different commit in the submodule to the
>> superproject.
>
> Currently the rest of us would not care IMHO, as there is no
> difference with the field recorded or not. (I just checked if it
> would provide slight benefits for shallow clones, but not so)
>
>> I was recalling a wish by submodule users in a distant past that
>> lets "submodule update" to detach to the tip of the named branch in
>> the submodule, regardless of what commit the superproject points at
>> with its gitlink.
>
> Yes, I heard that a couple times when poking around for opinions
> and I think the issue has 2 facets:
> * They actually want to be on a branch, such that the workflow
>   is 'normal'. (Whatever that means, "detached HEAD" sounds
>   scary, but workflow-wise it is not. It just requires an additional
>   "checkout -b" once the work done seems worth preserving)
> * None of the people I talked to wanted to get rid of exact-tracking,
>   solely. But they always came in trade off with the first point
>   outweighing the benefits of exact tracking.
>   Although for this purpose the "update --remote" also doesn't
>   quite fit as it does not re-attach the HEAD to a branch at
>   the same commit as the remote tracking branch.

For my submodule use, I'd like to have it checkout the branchname
recorded in the .gitmodules with tracking info set to upstream, and then
"git reset --hard" to the commit that's recorded in the commit info.

This would exactly mirror the state the repo was in when I initally ran
"git submodule add" on it, and make it easy to start making new commits
on my local branch, submit a PR of those to upstream, and have things
like "git fetch && git log @{u}.." work.

>> When those merely following along with this project did "pull &&
>> submodule update", I do not want the submodule directory to check
>> out the commit that happens to be at the tip of their 'master'
>> branch.  If "submodule update" requires an explicit "--remote"
>> option to work in that way, then my worry is allayed, greatly.
>>
>> Thanks.

Re: [PATCH 00/12] object_id part 9

[Stefan Beller]

On Tue, Jul 4, 2017 at 9:10 PM, Junio C Hamano  wrote:
> "brian m. carlson"  writes:
>
>> It is possible there will be some other conflicts with in flight topics,
>> as get_sha1 is commonly used in the codebase.  This is unavoidable to
>> some extent, but should be kept in mind.  My experience is that usually
>> the required changes for conversion are minimal.
>
> Thanks.
>
> It did have a few conflicts in submodule area and sequencer, but
> they were (hopefully) trivial to resolve.  The result is queued at
> the tip of 'pu'.  It seems to pass the tests locally and also at
> Travis.

The series as queued (modulo 2 minor nits/questions asked in
patch 11/12) as well as the conflict resolution look good to me.

Thanks,
Stefan

Re: [PATCH 11/12] sha1_name: convert get_sha1* to get_oid*

[Stefan Beller]

> @@ -496,8 +496,8 @@ int find_unique_abbrev_r(char *hex, const unsigned char 
> *sha1, int len)
> return 40;
> exists = has_sha1_file(sha1);
> while (len < 40) {

Here are two prime candidates for GIT_SHA1_HEXSZ, too.

> @@ -636,7 +636,7 @@ static int get_sha1_basic(const char *str, int len, 
> unsigned char *sha1,
> int detached;
>
> if (interpret_nth_prior_checkout(str, len, ) > 0) {
> -   detached = (buf.len == 40 && !get_sha1_hex(buf.buf, 
> sha1));
> +   detached = (!get_oid_hex(buf.buf, oid));

omitting the length check here?

Re: [PATCH 2/6] t1414: document some reflog-walk oddities

[Junio C Hamano]

Jeff King  writes:

> Since its inception, the general strategy of the reflog-walk
> code has been to start with the tip commit for the ref, and
> as we traverse replace each commit's parent pointers with
> fake parents pointing to the previous reflog entry.
>
> This lets us traverse the reflog as if it were a real
> history, but it has some user-visible oddities. Namely:
>
>   1. The fake parents are used for commit selection and
>  display. So for example, "--merges" or "--no-merges"
>  are useful, because the history appears as a linear

s/useful/useless/ perhaps?

>  string. Likewise, pathspec limiting is based on the
>  diff between adjacent entries, not the changes actually
>  introduced by a commit.
> ...
> This commit adds several expect_failure tests, to show how
> the tool ought to behave.
>
> Signed-off-by: Jeff King 
> ---

> diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
> new file mode 100755
> index 00..bb847f797d
> --- /dev/null
> +++ b/t/t1414-reflog-walk.sh
> @@ -0,0 +1,83 @@
> +#!/bin/sh
> +
> +test_description='various tests of reflog walk (log -g) behavior'
> +. ./test-lib.sh
> +
> +test_expect_success 'set up some reflog entries' '
> + test_commit one &&
> + test_commit two &&
> + git checkout -b side HEAD^ &&
> + test_commit three &&
> + git merge --no-commit master &&
> + echo evil-merge-content >>one.t &&
> + test_tick &&
> + git commit --no-edit -a
> +'

Mental note: logically, what we want to see in the log are:

master:  one-->two
side:one-->three-->(evil)
HEAD:one-->two-->one-->three-->(evil)

where the middle one in HEAD is "switching from master to side".

> +test_expect_failure 'pathspec limiting handles merges' '
> + sed -n "1p;3p;5p" expect.all >expect &&
> + do_walk -- one.t >actual &&
> + test_cmp expect actual
> +'

OK (it was a bit tricky to see why the topmost one should, but the
evilness of the merge makes it eligible).

> +test_expect_failure '--parents shows true parents' '
> + # convert newlines to spaces
> + echo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&

I saw something related to this nearby this morning.  Good thinking
to add a comment on this 'echo' ;-).

> + git rev-list -g --parents -1 HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_failure 'walking multiple reflogs shows both' '
> + {
> + do_walk HEAD &&
> + do_walk side
> + } >expect &&
> + do_walk HEAD side >actual &&
> + test_cmp expect actual
> +'

I somehow find this one a bit iffy.  

The order that commits appear in the "walk from HEAD and side at the
same time" may want to be different from what this test expects.
"rev-list --since=3.days -g master next", for example, would want to
refrain from reading the reflog of 'master' for all 90 days before
switching to the reflog of 'next', no?

All others I did not comment on and omitted from quoting make sense
to me.

Thanks.

Re: [PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Stefan Beller]

On Tue, Jul 4, 2017 at 6:56 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Tue, Jul 4, 2017 at 3:50 PM, Ævar Arnfjörð Bjarmason
>>  wrote:
>>
>>>
>>> I think some invocation of "git submodule update ???" will do the same,
>>> but I can't from the docs see what that is right now.
>>
>> '--remote' is what you are looking for.
>>
>> When we have the branch field configured, the workflow for *creating*
>> the patch sent
>> to Junio might be different than it currently is. Currently, you would
>> send a patch that is
>> produced as:
>>
>>   git -C sha1collisiondetection pull origin master
>>   git add sha1collisiondetection
>>   git commit -m "serious reasoning in the commit message"
>>
>> This could be 'simplified' to
>>
>>   git submodule update --remote
>>   git add sha1collisiondetection
>>   git commit -m "serious reasoning in the commit message"
>>
>> but as we had different branches in the submodule field,
>> I wonder how much of an ease this is.
>>
>> For Junio the workflow stays the same, as he would just
>> apply the patch, so I understand why he might see the
>> branch field as pollution.
>
> My reaction was more about "the rest of us", not me or those who
> choose to bind a new/different commit in the submodule to the
> superproject.

Currently the rest of us would not care IMHO, as there is no
difference with the field recorded or not. (I just checked if it
would provide slight benefits for shallow clones, but not so)

> I was recalling a wish by submodule users in a distant past that
> lets "submodule update" to detach to the tip of the named branch in
> the submodule, regardless of what commit the superproject points at
> with its gitlink.

Yes, I heard that a couple times when poking around for opinions
and I think the issue has 2 facets:
* They actually want to be on a branch, such that the workflow
  is 'normal'. (Whatever that means, "detached HEAD" sounds
  scary, but workflow-wise it is not. It just requires an additional
  "checkout -b" once the work done seems worth preserving)
* None of the people I talked to wanted to get rid of exact-tracking,
  solely. But they always came in trade off with the first point
  outweighing the benefits of exact tracking.
  Although for this purpose the "update --remote" also doesn't
  quite fit as it does not re-attach the HEAD to a branch at
  the same commit as the remote tracking branch.

> When those merely following along with this project did "pull &&
> submodule update", I do not want the submodule directory to check
> out the commit that happens to be at the tip of their 'master'
> branch.  If "submodule update" requires an explicit "--remote"
> option to work in that way, then my worry is allayed, greatly.
>
> Thanks.

[PATCH] hooks: add signature using "interpret-trailers"

[Kaartic Sivaraam]

The sample hook to prepare the commit message before
a commit allows users to opt-in to add the signature
to the commit message. The signature is added at a place
that isn't consistent with the "-s" option of "git commit".
Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of
"git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

While at it, name the input parameters to improve readability
of script.

Signed-off-by: Kaartic Sivaraam 
---
 Added a close quote that got missed in the previous patch.

 templates/hooks--prepare-commit-msg.sample | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..4ddab7896 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -20,17 +20,28 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
+NEW_LINE='\
+'
+SED_TEMP_FILE='.sed-output.temp'
+
+case "$COMMIT_SOURCE,$SHA1" in
   merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
+@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$COMMIT_MSG_FILE" ;;
 
 # ,|template,)
 #   @PERL_PATH@ -i.bak -pe '
 #  print "\n" . `git diff --cached --name-status -r`
-#   if /^#/ && $first++ == 0' "$1" ;;
+#   if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
 
   *) ;;
 esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if [ -z "$COMMIT_SOURCE" ]
+# then
+#  sed -e "1i$NEW_LINE" "$COMMIT_MSG_FILE" >"$SED_TEMP_FILE" && mv 
"$SED_TEMP_FILE" "$COMMIT_MSG_FILE"
+# fi
-- 
2.13.1.78.gbaba0bc7d

Re: [PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename

[Junio C Hamano]

Jeff King  writes:

> Since 39ee4c6c2f (branch: record creation of renamed branch
> in HEAD's log, 2017-02-20), a rename on the currently
> checked out branch will create two entries in the HEAD
> reflog: one where the branch goes away (switching to the
> null oid), and one where it comes back (switching away from
> the null oid).
> ...
> The resulting behavior may not be the _best_ thing to do in
> the long run (for example, we show both reflog entries each
> with the same commit id), but it's a simple way to fix the
> problem without risking further regressions.
>
> Signed-off-by: Jeff King 
> ---
> I do still think it would be worth looking into making this rename
> create a single reflog entry, but that's largely orthogonal to making
> the display code sane(r).

I agree with this assessment.

>  reflog-walk.c |  2 ++
>  t/t3200-branch.sh | 11 +++
>  2 files changed, 13 insertions(+)
>
> diff --git a/reflog-walk.c b/reflog-walk.c
> index ed99437ad2..b7e489ad32 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
> struct commit *commit)
>   /* a root commit, but there are still more entries to show */
>   reflog = _reflog->reflogs->items[commit_reflog->recno];
>   logobj = parse_object(>noid);
> + if (!logobj)
> + logobj = parse_object(>ooid);
>   }

For the current 'maint', this would need to be backported to the
uchar[20] interface (which is trivial to do, and merging it upwards
while adjusting it back to "struct object_id" is also trivial; there
is no need to resend).

Thanks.  Will queue.

>   if (!logobj || logobj->type != OBJ_COMMIT) {
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 48d152b9a9..dd37ac47c5 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add 
> entries to .git/logs/HEAD'
>   grep "^0\{40\}.*$msg$" .git/logs/HEAD
>  '
>  
> +test_expect_success 'resulting reflog can be shown by log -g' '
> + oid=$(git rev-parse HEAD) &&
> + cat >expect <<-EOF &&
> + HEAD@{0} $oid $msg
> + HEAD@{1} $oid $msg
> + HEAD@{2} $oid checkout: moving from foo to baz
> + EOF
> + git log -g --format="%gd %H %gs" -3 HEAD >actual &&
> + test_cmp expect actual
> +'
> +
>  test_expect_success 'git branch -M baz bam should succeed when baz is 
> checked out as linked working tree' '
>   git checkout master &&
>   git worktree add -b baz bazdir &&

Re: Requesting suggestions for a good sample "prepare-commit-msg" hook

[Kaartic Sivaraam]

On Wed, 2017-07-05 at 10:00 -0700, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > So, we're searching for scripts that
> > could both be  helpful to users and could serve as a good sample to
> > prove what could be done using the hooks.
> 
> I am not so sure that we are searching for them, to be honest (who
> are we in this context anyway?)
I think I misinterpreted your sentence in one of the other mails (found
below),

That sounds like a sample that is there not because it would be
useful, but because we couldn't think of any useful example.


> This sounds like a solution looking for a problem to me.  The
> simplest option may be to remove the sample hook---perhaps it
> outlived its usefulness.
Removing it is going to be done for sure. I thought it would be even
better to replace it for the reasons I specified previously. Anyways,
in case the last path I sent[1] seems not to be useful and not other
interesting alternatives turn up in this thread then the "simplest
option" has to be chosen.


[1]: 
http://public-inbox.org/git/20170705165114.20662-1-kaarticsivaraam91...@gmail.com/raw

-- 
Kaartic

Re: Should "head" also work for "HEAD" on case-insensitive FS?

[Junio C Hamano]

Jeff King  writes:

> Ultimately I think the path forward is to have a ref backend that
> behaves uniformly (either because it avoids the filesystem, or because
> it encodes around the differences). See:
>
>   http://public-inbox.org/git/xmqqvb4udyf9@gitster.mtv.corp.google.com/
>
> and its reply.

Once Michael's packed-refs backend stabilizes, we may have a nice
calm period in the refs subsystem and I expect that this will become
a good medium-sized project for a contributor who does not have to 
be so experienced (but not a complete newbie).

It needs to:

 - add icase-files-backend, preferrably sharing as much code as the
   existing files-backend, in refs/.

 - design a mechanism to configure which refs backend to use at
   runtime; as this has to be done fairly early in the control flow,
   this will likely to use early configuration mechanism and will
   probably need to be done in the set-up code, but doing it lazy
   may even be nicer, as not all subcommands need access to refs.

Thanks for a pointer to the archive.

Re: Help needed for solving a few issues with building git

[Kaartic Sivaraam]

On Wed, 2017-07-05 at 01:30 +0530, Kaartic Sivaraam wrote:
> I tried pointing it to the installed location, it doesn't seem to be
> working. To elaborate a little on what I did,
> 
> * I installed the "libcurl4-openssl-dev" package b
> * I found that the 'include' directory to be present  at
> '/usr/include/x86_64-linux-gnu/curl'. I wasn't sure if
> '/usr/lib/x86_64-linux-gnu/' is the corresponding library
> directory. 
> * I took the common parent of both '/usr' and ran the
> following 
>   commands to build 'git'
> 
> $ make CURLDIR=/usr prefix=/custom/location
> $ make CURLDIR=/usr install prefix=/custom/location
> 
> * The build did succeed but I get an error that "'https'
> helper
> is not found"
> 
> Was anything I did, wrong?
> 
Ok, at last I was able to build git with https support using 'curl'
built from it's source. Anyways, thanks for the help, folks.

> >  This is probably because you are trying to run without installing?
> 
> Nope. I'm *installing* git not using the binary wrappers.
> 
> >  Ask the "git" you built what its --exec-path is, and run "ls" on
> >  that directory to see if you have git-remote-https installed?
> >  
> 
> Obviously, I don't see any 'git-remote-https' binary in the folder to
> which I built git.
> 
> >  Trying a freshly built Git binaries without installing is done by
> >  setting GIT_EXEC_PATH to point at bin-wrappers/ directory at the
> >  top-level of your build tree (that is how our tests can run on an
> >  otherwise virgin box with no Git installed).
> > 
> 

[PATCH] comment: fix a typo in the comment

[Kaartic Sivaraam]

---
 Though very trivial, I wanted to correct this as I didn't
 want to ignore it after seeing it.

 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac0..aff6bf7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -984,7 +984,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
int i, eol;
const char *nl;
 
-   /* Check if the rest is just whitespace and Signed-of-by's. */
+   /* Check if the rest is just whitespace and Signed-off-by's. */
for (i = start; i < sb->len; i++) {
nl = memchr(sb->buf + i, '\n', sb->len - i);
if (nl)
-- 
2.7.4

[PATCH] hooks: add signature using "interpret-trailers"

[Kaartic Sivaraam]

The sample hook to prepare the commit message before
a commit allows users to opt-in to add the signature
to the commit message. The signature is added at a place
that isn't consistent with the "-s" option of "git commit".
Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of
"git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

While at it, name the input parameters to improve readability
of script.

Signed-off-by: Kaartic Sivaraam 
---
  Removed the GNUism in sed command. In case no other changes are
  required this one's the final patch.

 templates/hooks--prepare-commit-msg.sample | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f22..f404f8f 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -20,17 +20,28 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
+NEW_LINE='\
+'
+SED_TEMP_FILE='.sed-output.temp'
+
+case "$COMMIT_SOURCE,$SHA1" in
   merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
+@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$COMMIT_MSG_FILE" ;;
 
 # ,|template,)
 #   @PERL_PATH@ -i.bak -pe '
 #  print "\n" . `git diff --cached --name-status -r`
-#   if /^#/ && $first++ == 0' "$1" ;;
+#   if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
 
   *) ;;
 esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if [ -z "$COMMIT_SOURCE" ]
+# then
+#  sed -e "1i$NEW_LINE" "$COMMIT_MSG_FILE" >"$SED_TEMP_FILE" && mv 
"$SED_TEMP_FILE "$COMMIT_MSG_FILE"
+# fi
-- 
2.7.4

Re: Requesting suggestions for a good sample "prepare-commit-msg" hook

[Junio C Hamano]

Kaartic Sivaraam  writes:

> So, we're searching for scripts that
> could both be  helpful to users and could serve as a good sample to
> prove what could be done using the hooks.

I am not so sure that we are searching for them, to be honest (who
are we in this context anyway?)  

This sounds like a solution looking for a problem to me.  The
simplest option may be to remove the sample hook---perhaps it
outlived its usefulness.

[PATCH] hooks: replace irrelevant hook sample

[Kaartic Sivaraam]

The pre-commit-msg hook sample has an example that comments
the "Conflicts:" part of the merge commmit. It isn't relevant
anymore as it's done by default since 261f315b ("merge & sequencer:
turn "Conflicts:" hint into a comment", 2014-08-28).

Add an alternative example that replaces it. This ensures there's
at the least a simple example that illustrates what could be done
using the hook just by enabling it.

Signed-off-by: Kaartic Sivaraam 
---
 This one's the final proposed patch for this simple hook. I have
 requested for suggestions for alternative scripts in the mailing
 list[1]. In case something interesting turns out, we could use that
 instead of this one.

 [1]: http://public-inbox.org/git/1499273152.16389.2.ca...@gmail.com/T/#u

 templates/hooks--prepare-commit-msg.sample | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..0e2ccef11 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -9,8 +9,9 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes three examples.  The first comments out the
-# "Conflicts:" part of a merge commit.
+# This hook includes three examples.  The first one removes three
+# comment lines starting from the line that has the words
+# "# Please enter the" in it's beginning.
 #
 # The second includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
@@ -20,17 +21,20 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
-  merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
+sed -e '/^# Please enter the .*/ {
+  N
+  N
+  d
+}' "$1" >'.sed-output.temp' && mv '.sed-output.temp' "$1"
 
+# case "$2,$3" in
 # ,|template,)
 #   @PERL_PATH@ -i.bak -pe '
 #  print "\n" . `git diff --cached --name-status -r`
 #   if /^#/ && $first++ == 0' "$1" ;;
-
-  *) ;;
-esac
+#
+#  *) ;;
+# esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
-- 
2.13.2.14.gb9bd03b20.dirty

Requesting suggestions for a good sample "prepare-commit-msg" hook

[Kaartic Sivaraam]

Hello all,

There's an attempt in progress to replace the part of the sample
"prepare-commit-msg" hook that comments the "Conflicts" part of a merge
commit message. This is being done as the "Conflicts" portion is
commented by default for quite some time (since 2014) and thus the
script in the hook is useless.

It's worth noting that it was the only part of the sample hook that
*wasn't* commented by default i.e., it was the only portion of the
sample hook that could be used just by enabling it by renaming the
hook. So, just removing that portion of the solution wouldn't be a
solution as it wouldn't be nice to have a whole sample hook to be
commented for various reasons. So, we're searching for scripts that
could both be  helpful to users and could serve as a good sample to
prove what could be done using the hooks.

In case anyone has any suggestions about what could prove to be an
alternative please send them in!

-- 
Kaartic

Re: [PATCH] push: add config option to --force-with-lease by default.

[Francesco Mazzoli]

> On 5 Jul 2017, at 17:17, Junio C Hamano  wrote:
> 
> The take-away lesson that the earlier thread gave me was that the
> order in which the three options are ranked by their desirebility
> in the UI (and the order we would like to encourage users to use)
> is, from the most to the least preferrable:
> 
> - "--force-with-lease=:" that is safer than "--force";
> 
> - "--force" that is known to be dangerous, and does not pretend to
>   be anything but;
> 
> - "--force-with-lease" that pretends to be safer but is not.
> 
> The last form should eventually be eliminated, as there is no way to
> correctly intuit what the expected object should be.

What's not clear to me is what the intended workflow using
`--force-with-lease=:` is. Intuitively it seems extremely
cumbersome to manually pluck a revision each time, especially when
dealing with commits that all have the same description.

On the other hand for my workflow `--force-with-lease` works quite well
because I tend to use it in cases where me and a colleague are working
on the same PR, and thus I'm not doing anything else (including fetching).

Moreover, it seems to me that the problem `--force-with-lease` is
just one of marketing. `--force-with-lease` is strictly more "safe"
than `--force` in the sense that it'll reject some pushes that `--force`
will let through. I think that if we advertise it better including its
drawbacks it can still be better than no checks at all.

Francesco

Re: [PATCH] t5534: fix misleading grep invocation

[Junio C Hamano]

Johannes Schindelin  writes:

> It seems to be a little-known feature of `grep` (and it certainly came
> as a surprise to this here developer who believed to know the Unix tools
> pretty well) that multiple patterns can be passed in the same
> command-line argument simply by separating them by newlines. Watch, and
> learn:
>
>   $ printf '1\n2\n3\n' | grep "$(printf '1\n3\n')"
>   1
>   3
>
> That behavior also extends to patterns passed via `-e`, and it is not
> modified by passing the option `-E` (but trying this with -P issues the
> error "grep: the -P option only supports a single pattern").
>
> It seems that there are more old Unix hands who are surprised by this
> behavior, as grep invocations of the form
>
>   grep "$(git rev-parse A B) C" file
>
> were introduced in a85b377d041 (push: the beginning of "git push
> --signed", 2014-09-12), and later faithfully copy-edited in b9459019bbb
> (push: heed user.signingkey for signed pushes, 2014-10-22).
>
> Please note that the output of `git rev-parse A B` separates the object
> IDs via *newlines*, not via spaces, and those newlines are preserved
> because the interpolation is enclosed in double quotes.
>
> As a consequence, these tests try to validate that the file contains
> either A's object ID, or B's object ID followed by C, or both. Clearly,
> however, what the test wanted to see is that there is a line that
> contains all of them.
>
> This is clearly unintended, and the grep invocations in question really
> match too many lines.
>
> Fix the test by avoiding the newlines in the patterns.
>
> Signed-off-by: Johannes Schindelin 
> ---

The invocation this fixes is not just misleading but simply wrong.
Nicely spotted.

Thanks, will queue.

>  t/t5534-push-signed.sh | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 5bcb288f5c4..464ffdd147a 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push 
> certificate' '
>   sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>   ) >expect &&
>  
> - grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
> - grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
> + noop=$(git rev-parse noop) &&
> + ff=$(git rev-parse ff) &&
> + noff=$(git rev-parse noff) &&
> + grep "$noop $ff refs/heads/ff" dst/push-cert &&
> + grep "$noop $noff refs/heads/noff" dst/push-cert &&
>   test_cmp expect dst/push-cert-status
>  '
>  
> @@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed 
> user.signingkey' '
>   sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
>   ) >expect &&
>  
> - grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
> - grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
> + noop=$(git rev-parse noop) &&
> + ff=$(git rev-parse ff) &&
> + noff=$(git rev-parse noff) &&
> + grep "$noop $ff refs/heads/ff" dst/push-cert &&
> + grep "$noop $noff refs/heads/noff" dst/push-cert &&
>   test_cmp expect dst/push-cert-status
>  '
>  
>
> base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a

Re: What's cooking in git.git (Jun 2017, #09; Fri, 30)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Fri, Jun 30 2017, Junio C. Hamano jotted:
>
>> * xz/send-email-batch-size (2017-05-23) 1 commit
>>  - send-email: --batch-size to work around some SMTP server limit
>>
>>  "git send-email" learned to overcome some SMTP server limitation
>>  that does not allow many pieces of e-mails to be sent over a single
>>  session.
>>
>>  Waiting for a response.
>>  cf. 
>>  cf. 
>>
>>  """I thought your wish (which I found reasonable) was to record
>>  whatever information that would help us in the future in the log
>>  message?  I was waiting for that to happen."""
>
> I think it's fine in lieu of xiaoqiang zhao not being responsive to just
> merge this as-is. The info that can help us in the future is in the ML
> archive, which should be good enough.

OK.  I'll amend to add a few lines of note to the commit log and
then merge it down.

> So my WIP is, and I'd like feedback on the viability of the general approach:
>
> create_test_file() {
>   file=$1
>
>   # `touch .` will succeed but obviously not do what we intend
>   # here.

If you want to create, do not use "touch" that gives readers a false
and confusing impression that you care about the timestamp of the
thing being updated.  If you say ">./$file", you can get an error from
the shell just fine, I think.

>   test "$file" = "." && return 1
>   # We cannot create a file with an empty filename.
>   test "$file" = "" && return 1

Likewise, as that would become ">./".

>   # The tests that are testing that e.g. foo//bar is matched by
>   # foo/*/bar can't be tested on filesystems since there's no
>   # way we're getting a double slash.
>   echo "$file" | grep -F '//' && return 1
>   dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')

GNUism already pointed out, I think.
>
>   # We touch "./$file" instead of "$file" because even an
>   # escaped "touch -- -" means something different.
>   if test "$file" != "$dirs"
>   then
>   mkdir -p -- "$dirs" 2>/dev/null &&
>   touch -- "./$file" 2>/dev/null &&
>   return 0
>   else
>   touch -- "./$file" 2>/dev/null &&
>   return 0
>   fi
>   return 1
> }
>
> And then later on for the tests I do:
>
>   # The existing test
>   test_expect_success "wildmatch: match '$text' '$pattern'" "
>   test-wildmatch wildmatch '$text' '$pattern'
>   "
>
>   # My new test
>   if create_test_file "$text"
>   then
>   test_expect_success "wildmatch(ls): match '$pattern' '$text'" "
>   test_when_finished \"
>   rm -rf -- * &&
>   git reset
>   \" &&
>   git add -A &&
>   >expect.err &&
>   printf '%s' '$text' >expect &&
>   git --glob-pathspecs ls-files -z -- '$pattern' 
> 2>actual.err | tr -d '\0' >actual &&
>   test_cmp expect.err actual.err &&
>   test_cmp expect actual
>   "
>   else
>   test_expect_failure "wildmatch(ls): match skip '$pattern' 
> '$text'" 'false'
>   fi
>
> This still needs to be cleaned up a bit to be parameterized (i.e. the
> --glob-pathspecs argument, whether it should error etc.).
>
> It works for me on Linux, but I'm wondering if others see any issues
> with the approach, does simply trying to create bizarro filenames on
> some OSs cause issues? I don't think so, but let's make sure.

Issues meaning that the test wants to see how a pathname with ^I in
it works but create_test_file step would fail?  Your above construct
covers that with the if/then/else test_expect_failure/fi just fine,
I think.

Re: [PATCH] push: add config option to --force-with-lease by default.

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 5, 2017 at 1:26 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Wed, Jul 05 2017, Junio C. Hamano jotted:
>
>> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli  wrote:
>>>
>>> Could you clarify the danger you're referring to? E.g. give an example
>>> of surprising --force-with-lease behavior that we do not want to
>>> encourage?
>>
>> https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/
>
> In the context of this patch I don't understand why you're concerned
> that making --force mean --force-with-lease makes things worse.
>
> See my
> https://public-inbox.org/git/cacbzzx48ranjhsv1usnxkbxrtqkrggmcgmtvqqmr84h5j8a...@mail.gmail.com/
> (follow-up to the E-Mail you posted):
>
> To me the *main* feature of --force-with-lease is that it's less
> shitty than --force, without imposing too much UI overhead. We have to
> be really careful not to make --force-with-lease so complex by default
> that people just give u and go back to using --force, which would be
> worse than either whatever current problems there are with the
> current --force-with-lease behavior, or anything we replace it with.
>
> I.e. yes there are workflows with some background auto-update that will
> make it less safe, which I documented in f17d642d3b ("push: document &
> test --force-with-lease with multiple remotes", 2017-04-19).
>
> But it is still the case that --force-with-lease is categorically a more
> safer option than simply --force, which has none of the safety
> --force-with-lease has. It would still wipe away history in this
> scenario you're pointing out *and others*.
>
> Surely the point of having an option like this is to have a net
> reduction in complexity.

I mean reduction in risk...

> I think it can be argued that it's bad UI design though to have --force
> mean different things depending on the config, and we'd be better off
> with a patch that disables --force.

Re: [PATCH] push: add config option to --force-with-lease by default.

[Junio C Hamano]

Francesco Mazzoli  writes:

> So we would have something like
>
> * `push.disableForce`: config flag that disables `--force` and suggests
> `--force-with-lease` instead;
> * `--disable-force` and `--no-disable-force`, config flags to tune the above
> config parameter at will.
>
> What do you think?

The take-away lesson that the earlier thread gave me was that the
order in which the three options are ranked by their desirebility
in the UI (and the order we would like to encourage users to use)
is, from the most to the least preferrable:

 - "--force-with-lease=:" that is safer than "--force";

 - "--force" that is known to be dangerous, and does not pretend to
   be anything but;

 - "--force-with-lease" that pretends to be safer but is not.

The last form should eventually be eliminated, as there is no way to
correctly intuit what the expected object should be.

To me, a disableForce configuration that encourages use of either
the best one or the worst one alone does not look like a step
forward, unless we also have a change to disable the last form.

[PATCH] t5534: fix misleading grep invocation

[Johannes Schindelin]

It seems to be a little-known feature of `grep` (and it certainly came
as a surprise to this here developer who believed to know the Unix tools
pretty well) that multiple patterns can be passed in the same
command-line argument simply by separating them by newlines. Watch, and
learn:

$ printf '1\n2\n3\n' | grep "$(printf '1\n3\n')"
1
3

That behavior also extends to patterns passed via `-e`, and it is not
modified by passing the option `-E` (but trying this with -P issues the
error "grep: the -P option only supports a single pattern").

It seems that there are more old Unix hands who are surprised by this
behavior, as grep invocations of the form

grep "$(git rev-parse A B) C" file

were introduced in a85b377d041 (push: the beginning of "git push
--signed", 2014-09-12), and later faithfully copy-edited in b9459019bbb
(push: heed user.signingkey for signed pushes, 2014-10-22).

Please note that the output of `git rev-parse A B` separates the object
IDs via *newlines*, not via spaces, and those newlines are preserved
because the interpolation is enclosed in double quotes.

As a consequence, these tests try to validate that the file contains
either A's object ID, or B's object ID followed by C, or both. Clearly,
however, what the test wanted to see is that there is a line that
contains all of them.

This is clearly unintended, and the grep invocations in question really
match too many lines.

Fix the test by avoiding the newlines in the patterns.

Signed-off-by: Johannes Schindelin 
---
 t/t5534-push-signed.sh | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 5bcb288f5c4..464ffdd147a 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -119,8 +119,11 @@ test_expect_success GPG 'signed push sends push 
certificate' '
sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
) >expect &&
 
-   grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
-   grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+   noop=$(git rev-parse noop) &&
+   ff=$(git rev-parse ff) &&
+   noff=$(git rev-parse noff) &&
+   grep "$noop $ff refs/heads/ff" dst/push-cert &&
+   grep "$noop $noff refs/heads/noff" dst/push-cert &&
test_cmp expect dst/push-cert-status
 '
 
@@ -200,8 +203,11 @@ test_expect_success GPG 'fail without key and heed 
user.signingkey' '
sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
) >expect &&
 
-   grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
-   grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+   noop=$(git rev-parse noop) &&
+   ff=$(git rev-parse ff) &&
+   noff=$(git rev-parse noff) &&
+   grep "$noop $ff refs/heads/ff" dst/push-cert &&
+   grep "$noop $noff refs/heads/noff" dst/push-cert &&
test_cmp expect dst/push-cert-status
 '
 

base-commit: 5116f791c12dda6b6c22fa85b600a8e30dfa168a
-- 
2.13.2.windows.1

Published-As: 
https://github.com/dscho/git/releases/tag/t5534-fix-grep-pattern-v1
Fetch-It-Via: git fetch https://github.com/dscho/git t5534-fix-grep-pattern-v1

Re: [PATCH] push: add config option to --force-with-lease by default.

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 05 2017, Junio C. Hamano jotted:

> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli  wrote:
>>
>> Could you clarify the danger you're referring to? E.g. give an example
>> of surprising --force-with-lease behavior that we do not want to
>> encourage?
>
> https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/

In the context of this patch I don't understand why you're concerned
that making --force mean --force-with-lease makes things worse.

See my
https://public-inbox.org/git/cacbzzx48ranjhsv1usnxkbxrtqkrggmcgmtvqqmr84h5j8a...@mail.gmail.com/
(follow-up to the E-Mail you posted):

To me the *main* feature of --force-with-lease is that it's less
shitty than --force, without imposing too much UI overhead. We have to
be really careful not to make --force-with-lease so complex by default
that people just give u and go back to using --force, which would be
worse than either whatever current problems there are with the
current --force-with-lease behavior, or anything we replace it with.

I.e. yes there are workflows with some background auto-update that will
make it less safe, which I documented in f17d642d3b ("push: document &
test --force-with-lease with multiple remotes", 2017-04-19).

But it is still the case that --force-with-lease is categorically a more
safer option than simply --force, which has none of the safety
--force-with-lease has. It would still wipe away history in this
scenario you're pointing out *and others*.

Surely the point of having an option like this is to have a net
reduction in complexity.

I think it can be argued that it's bad UI design though to have --force
mean different things depending on the config, and we'd be better off
with a patch that disables --force.

Re: [PATCH v3 00/30] Create a reference backend for packed refs

[Jeff King]

On Sat, Jul 01, 2017 at 08:30:38PM +0200, Michael Haggerty wrote:

> This is v3 of a patch series creating a `packed_ref_store` reference
> backend. Thanks to Peff and Junio for their comments about v2 [1].
> 
> Changes since v2:
> 
> * Delete some debugging `cat` commands in t1408.
> 
> * Add some tests of reading packed-refs files with bogus contents.
> 
> * When reporting corruption in packed-refs files, distinguish between
>   unterminated lines and other corruption.
> 
> * Fixed a typo in a commit message.

Thanks. I just quickly re-reviewed based on the diff from v2, and it
looks good to me.

-Peff

Re: [PATCHv3 2/3] patch-ids.c: use hashmap correctly

[Jeff King]

On Fri, Jun 30, 2017 at 12:14:06PM -0700, Stefan Beller wrote:

> As eluded to in the previous patch, the code in patch-ids.c is using
> the hashmaps API wrong.
> 
> Luckily we do not have a bug, as all hashmap functionality that we use
> here (hashmap_get) passes through the keydata.  If hashmap_get_next were
> to be used, a bug would occur as that passes NULL for the key_data.
> 
> So instead use the hashmap API correctly and provide the caller required
> data in the compare function via the first argument that always gets
> passed and was setup via the hashmap_init function.

Reviewing this a bit late, but it looks good to me. And I think the
explanation above nicely covers what is going on (and why it isn't a
bug).

-Peff

PS I think you meant "alluded" in the first sentence, unless you really
   were trying to escape the previous patch. :)

Re: Should "head" also work for "HEAD" on case-insensitive FS?

[Jeff King]

On Tue, Jul 04, 2017 at 10:24:30AM +0200, Ævar Arnfjörð Bjarmason wrote:

> I.e. we allow any arbitrary ref sitting in .git/, but presumably we
> could just record the original string the user provided so that this
> dies on OSX/Windows too:
> 
> $ cp .git/{HEAD,Whatever}
> $ git rev-parse wHATEVER
> wHATEVER
> fatal: ambiguous argument 'wHATEVER': unknown revision or path not in the 
> working tree.
> 
> But this may be a much deeper rabbit hole than I initially thought, I
> was fishing to see if someone knew of a place in the code or WIP patch
> that dealt with these special refs, but between the low-level machinery
> & sha1_name.c (and others) there may be no easy one place to do this...

I think we talked at one point about allowing only [A-Z_] for top-level
refs. My recollection is that it generally seemed like a good idea, but
I don't think we ever had patches.

I think it would work to enforce it via check_refname_format(). That
would catch reading via dwim_ref(), which is what your example is
hitting. But it should also prevent people from writing ".git/foo" (or
worse, ".git/config") as a ref.

I do think that's the tip of the iceberg for case-sensitivity problems
with refs, though. Because packed-refs is case-sensitive, I think you
can create some pretty confusing states on case-insensitive filesystems.
For example:

  http://public-inbox.org/git/20150825052123.ga...@sigill.intra.peff.net/

Ultimately I think the path forward is to have a ref backend that
behaves uniformly (either because it avoids the filesystem, or because
it encodes around the differences). See:

  http://public-inbox.org/git/xmqqvb4udyf9@gitster.mtv.corp.google.com/

and its reply.

-Peff

Re: Flurries of 'git reflog expire'

[Jeff King]

On Tue, Jul 04, 2017 at 09:57:58AM +0200, Andreas Krey wrote:

> Questions:
> 
> What can be done about this? Cronjob 'git reflog expire' at midnight,
> so the heuristic don't trigger during the day? (The relnotes don't
> mention anything after 2.4.0, so I suppose a git upgrade won't help.)
> 
> What is the actual cause? Bad heuristics in git itself, or does
> bitbucket run them too often (improbable)?

If it's using --expire-unreachable (which a default "git gc" does), that
means the we have to traverse the entire history to see what is
reachable and what is not. Added on to a normal git-gc, that's usually
not a big deal (it has to do that traversal and much more for the
repack). But if bitbucket is triggering it for other operations, that
could be related (I don't think anything but gc should ever run it
otherwise).

I seem to recall that using --stale-fix is also extremely expensive,
too. What do the command line arguments for the slow commands look like?
And what does the process tree look like?

-Peff

[PATCH 6/6] reflog-walk: stop using fake parents

[Jeff King]

The reflog-walk system works by putting a ref's tip into the
pending queue, and then "traversing" the reflog by
pretending that the parent of each commit is the previous
reflog entry.

This causes a number of user-visible oddities, as documented
in t1414 (and the commit message which introduced it). We
can fix all of them in one go by replacing the fake-reflog
system with a much simpler one: just keeping a list of
reflogs to show, and walking through them entry by entry.

The implementation is fairly straight-forward, but there are
a few items to note:

  1. We obviously must skip calling add_parents_to_list()
 when we are traversing reflogs, since we do not want to
 walk the original parents at all.

 As a result, we must call try_to_simplify_commit()
 ourselves and skip any TREESAME commits.

 There are other parts of add_parents_to_list() we skip,
 as well, but none of them should matter for a reflog
 traversal:

   -  We do not allow UNINTERESTING commits, nor
  symmetric ranges (and we bail when these are used
  with "-g").

   - Using --source makes no sense, since we aren't
 traversing. The reflog selector shows the same
 information with more detail.

   - Using --first-parent is still sensible, since you
 may want to see the first-parent diff for each
 entry. But since we're not traversing, we don't
 need to cull the parent list here.

  2. Since we now just walk the reflog entries themselves,
 rather than starting with the ref tip, we now look at
 the "new" field of each entry rather than the "old"
 (i.e., we are showing entries, not faking parents).
 This removes all of the tricky logic around skipping
 past root commits.

 But note that we have no way to show an entry with the
 null sha1 in its "new" field (because such a commit
 obviously does not exist). Normally this would not
 happen, since we delete reflogs along with refs, but
 there is one special case. When we rename the currently
 checked out branch, we write two reflog entries into
 the HEAD log: one where the commit goes away, and
 another where it comes back.

 Prior to this commit, we show both entries with
 identical reflog messages. After this commit, we show
 only the "comes back" entry. See the update in t3200
 which demonstrates this.

 Arguably either is fine, as the whole double-entry
 thing is a bit hacky in the first place. And until a
 recent fix, we truncated the traversal in such a case
 anyway, which was _definitely_ wrong.

Signed-off-by: Jeff King 
---
 reflog-walk.c  | 116 +
 reflog-walk.h  |   4 +-
 revision.c |  30 -
 t/t1414-reflog-walk.sh |  12 ++---
 t/t3200-branch.sh  |   3 +-
 5 files changed, 57 insertions(+), 108 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 89e719c459..a7644d944e 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -78,45 +78,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs 
*array,
return -1;
 }
 
-struct commit_info_lifo {
-   struct commit_info {
-   struct commit *commit;
-   void *util;
-   } *items;
-   int nr, alloc;
-};
-
-static struct commit_info *get_commit_info(struct commit *commit,
-   struct commit_info_lifo *lifo, int pop)
-{
-   int i;
-   for (i = 0; i < lifo->nr; i++)
-   if (lifo->items[i].commit == commit) {
-   struct commit_info *result = >items[i];
-   if (pop) {
-   if (i + 1 < lifo->nr)
-   memmove(lifo->items + i,
-   lifo->items + i + 1,
-   (lifo->nr - i) *
-   sizeof(struct commit_info));
-   lifo->nr--;
-   }
-   return result;
-   }
-   return NULL;
-}
-
-static void add_commit_info(struct commit *commit, void *util,
-   struct commit_info_lifo *lifo)
-{
-   struct commit_info *info;
-   ALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);
-   info = lifo->items + lifo->nr;
-   info->commit = commit;
-   info->util = util;
-   lifo->nr++;
-}
-
 struct commit_reflog {
int recno;
enum selector_type {
@@ -128,7 +89,8 @@ struct commit_reflog {
 };
 
 struct reflog_walk_info {
-   struct commit_info_lifo reflogs;
+   struct commit_reflog **logs;
+   size_t nr, alloc, cur;
struct string_list complete_reflogs;
struct commit_reflog *last_commit_reflog;
 };
@@ -226,52 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
commit_reflog->selector = selector;

[PATCH 5/6] rev-list: check reflog_info before showing usage

[Jeff King]

When git-rev-list sees no pending commits, it shows a usage
message. This works even when reflog-walking is requested,
because the reflog-walk code currently puts the reflog tips
into the pending queue.

In preparation for refactoring the reflog-walk code, let's
explicitly check whether we have any reflogs to walk. For
now this is a noop, but the existing reflog tests will make
sure that it kicks in after the refactoring. Likewise, we'll
add a test that "rev-list -g" without specifying any reflogs
continues to fail (so that we know our check does not kick
in too aggressively).

Note that the implementation needs to go into its own
sub-function, as the walk code does not expose its innards
outside of reflog-walk.c.

Signed-off-by: Jeff King 
---
This is actually the main "gotcha" I'm worried about with this series.
I'm not sure if any other code would care about seeing the pending items
in revs->commits. I still think the series is the right direction; if
there is such a place, we'd want to teach it to handle reflog walking in
a similar way, too.

 builtin/rev-list.c | 3 ++-
 reflog-walk.c  | 5 +
 reflog-walk.h  | 2 ++
 t/t1414-reflog-walk.sh | 3 +++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cda..53a746dd89 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -11,6 +11,7 @@
 #include "graph.h"
 #include "bisect.h"
 #include "progress.h"
+#include "reflog-walk.h"
 
 static const char rev_list_usage[] =
 "git rev-list [OPTION] ... [ -- paths... ]\n"
@@ -348,7 +349,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
/* Only --header was specified */
revs.commit_format = CMIT_FMT_RAW;
 
-   if ((!revs.commits &&
+   if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
  !revs.pending.nr)) ||
revs.diff)
diff --git a/reflog-walk.c b/reflog-walk.c
index b7e489ad32..89e719c459 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -358,3 +358,8 @@ void show_reflog_message(struct reflog_walk_info 
*reflog_info, int oneline,
strbuf_release();
}
 }
+
+int reflog_walk_empty(struct reflog_walk_info *info)
+{
+   return !info || !info->reflogs.nr;
+}
diff --git a/reflog-walk.h b/reflog-walk.h
index 27886f793e..af32361072 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -20,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb,
const struct date_mode *dmode, int force_date,
int shorten);
 
+extern int reflog_walk_empty(struct reflog_walk_info *walk);
+
 #endif
diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index bb847f797d..fba6788e94 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -79,5 +79,8 @@ test_expect_failure 'walk prefers reflog to ref tip' '
test_cmp expect actual
 '
 
+test_expect_success 'rev-list -g complains when there are no reflogs' '
+   test_must_fail git rev-list -g
+'
 
 test_done
-- 
2.13.2.892.g25f9b59978

Re: [PATCH] push: add config option to --force-with-lease by default.

[Francesco Mazzoli]

On 5 July 2017 at 09:43, Junio C Hamano  wrote:
> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli  wrote:
>>
>> Could you clarify the danger you're referring to? E.g. give an example
>> of surprising --force-with-lease behavior that we do not want to
>> encourage?
>
> https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/

Thanks for clarifying, I had not encountered this because of my git workflow.
I'd also be happy with a config item  that simply disables `--force`
and suggests
`--force-with-lease`, but then we'd need a flag to override that config item
when the user definitely wants to force push.

So we would have something like

* `push.disableForce`: config flag that disables `--force` and suggests
`--force-with-lease` instead;
* `--disable-force` and `--no-disable-force`, config flags to tune the above
config parameter at will.

What do you think?

Thanks,
Francesco

[PATCH 4/6] get_revision_1(): replace do-while with an early return

[Jeff King]

The get_revision_1() function tries to avoid entering its
main loop at all when there are no commits to look at. But
it's perfectly safe to call pop_commit() on an empty list
(in which case it will return NULL). Switching to an early
return from the loop lets us skip repeating the loop
condition before we enter the do-while. That will get more
important when we start pulling reflog-walk commits from a
source besides the revs->commits queue, as that condition
will get much more complicated.

Signed-off-by: Jeff King 
---
 revision.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index e181ad1b70..4dc7c63654 100644
--- a/revision.c
+++ b/revision.c
@@ -3109,12 +3109,12 @@ static void track_linear(struct rev_info *revs, struct 
commit *commit)
 
 static struct commit *get_revision_1(struct rev_info *revs)
 {
-   if (!revs->commits)
-   return NULL;
-
-   do {
+   while (1) {
struct commit *commit = pop_commit(>commits);
 
+   if (!commit)
+   return NULL;
+
if (revs->reflog_info) {
save_parents(revs, commit);
fake_reflog_parent(revs->reflog_info, commit);
@@ -3148,8 +3148,7 @@ static struct commit *get_revision_1(struct rev_info 
*revs)
track_linear(revs, commit);
return commit;
}
-   } while (revs->commits);
-   return NULL;
+   }
 }
 
 /*
-- 
2.13.2.892.g25f9b59978

[PATCH 3/6] log: do not free parents when walking reflog

[Jeff King]

When we're doing a reflog walk (instead of walking the
actual parent pointers), we may see commits multiple times.
For this reason, we hold on to the commit buffer for each
commit rather than freeing it after we've showed the commit.

We should do the same for the parent list. Right now this is
just a minor optimization. But once we refactor how reflog
walks are performed, keeping the parents will avoid
confusing us the second time we see the commit.

Signed-off-by: Jeff King 
---
I didn't dig deep into the details of "confusing", so there may be other
ways to solve it. But this seems like the right thing to do regardless.

 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8ca1de9894..9c8bb3b5c3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
if (!rev->reflog_info) {
/* we allow cycles in reflog ancestry */
free_commit_buffer(commit);
+   free_commit_list(commit->parents);
+   commit->parents = NULL;
}
-   free_commit_list(commit->parents);
-   commit->parents = NULL;
if (saved_nrl < rev->diffopt.needed_rename_limit)
saved_nrl = rev->diffopt.needed_rename_limit;
if (rev->diffopt.degraded_cc_to_c)
-- 
2.13.2.892.g25f9b59978

[PATCH 2/6] t1414: document some reflog-walk oddities

[Jeff King]

Since its inception, the general strategy of the reflog-walk
code has been to start with the tip commit for the ref, and
as we traverse replace each commit's parent pointers with
fake parents pointing to the previous reflog entry.

This lets us traverse the reflog as if it were a real
history, but it has some user-visible oddities. Namely:

  1. The fake parents are used for commit selection and
 display. So for example, "--merges" or "--no-merges"
 are useful, because the history appears as a linear
 string. Likewise, pathspec limiting is based on the
 diff between adjacent entries, not the changes actually
 introduced by a commit.

 These are often the same (e.g., because the entry was
 just running "git commit" and the adjacent entry _is_
 the true parent), but it may not be in several common
 cases. For instance, using "git reset" to jump around
 history, or "git checkout" to move HEAD.

  2. We reverse-map each commit back to its reflog. So when
 it comes time to show commit X, we say "a-ha, we added
 X because it was at the tip of the 'foo' reflog, so
 let's show the foo reflog". But this leads to nonsense
 results when you ask to traverse multiple reflogs: if
 two reflogs have the same tip commit, we only map back
 to one of them.

 Instead, we should show each reflog individually, in
 the order the user gave us on the command line.

  2. If the tip of the reflog and the ref tip disagree on
 the current value, we show the ref tip but give no
 indication of the value in the reflog.  This situation
 isn't supposed to happen (since any ref update should
 touch the reflog). But if it does, given that the
 requested operation is to show the reflog, it makes
 sense to prefer that.

This commit adds several expect_failure tests, to show how
the tool ought to behave.

Signed-off-by: Jeff King 
---
 t/t1414-reflog-walk.sh | 83 ++
 1 file changed, 83 insertions(+)
 create mode 100755 t/t1414-reflog-walk.sh

diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
new file mode 100755
index 00..bb847f797d
--- /dev/null
+++ b/t/t1414-reflog-walk.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='various tests of reflog walk (log -g) behavior'
+. ./test-lib.sh
+
+test_expect_success 'set up some reflog entries' '
+   test_commit one &&
+   test_commit two &&
+   git checkout -b side HEAD^ &&
+   test_commit three &&
+   git merge --no-commit master &&
+   echo evil-merge-content >>one.t &&
+   test_tick &&
+   git commit --no-edit -a
+'
+
+do_walk () {
+   git log -g --format="%gd %gs" "$@"
+}
+
+sq="'"
+test_expect_success 'set up expected reflog' '
+   cat >expect.all <<-EOF
+   HEAD@{0} commit (merge): Merge branch ${sq}master${sq} into side
+   HEAD@{1} commit: three
+   HEAD@{2} checkout: moving from master to side
+   HEAD@{3} commit: two
+   HEAD@{4} commit (initial): one
+   EOF
+'
+
+test_expect_success 'reflog walk shows expected logs' '
+   do_walk >actual &&
+   test_cmp expect.all actual
+'
+
+test_expect_failure 'reflog can limit with --no-merges' '
+   grep -v merge expect.all >expect &&
+   do_walk --no-merges >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'reflog can limit with pathspecs' '
+   grep two expect.all >expect &&
+   do_walk -- two.t >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'pathspec limiting handles merges' '
+   sed -n "1p;3p;5p" expect.all >expect &&
+   do_walk -- one.t >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure '--parents shows true parents' '
+   # convert newlines to spaces
+   echo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect &&
+   git rev-list -g --parents -1 HEAD >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'walking multiple reflogs shows both' '
+   {
+   do_walk HEAD &&
+   do_walk side
+   } >expect &&
+   do_walk HEAD side >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'walk prefers reflog to ref tip' '
+   head=$(git rev-parse HEAD) &&
+   one=$(git rev-parse one) &&
+   ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" 
&&
+   echo "$head $one $ident broken reflog entry" >>.git/logs/HEAD &&
+
+   echo $one >expect &&
+   git log -g --format=%H -1 >actual &&
+   test_cmp expect actual
+'
+
+
+test_done
-- 
2.13.2.892.g25f9b59978

[PATCH 1/6] reflog-walk: skip over double-null oid due to HEAD rename

[Jeff King]

Since 39ee4c6c2f (branch: record creation of renamed branch
in HEAD's log, 2017-02-20), a rename on the currently
checked out branch will create two entries in the HEAD
reflog: one where the branch goes away (switching to the
null oid), and one where it comes back (switching away from
the null oid).

This confuses the reflog-walk code. When walking backwards,
it first sees the null oid in the "old" field of the second
entry. Thanks to the "root commit" logic added by 71abeb753f
(reflog: continue walking the reflog past root commits,
2016-06-03), we keep looking for the next entry by scanning
the "new" field from the previous entry. But that field is
also null! We need to go just a tiny bit further, and look
at its "old" field. But with the current code, we decide the
reflog has nothing else to show and just give up. To the
user this looks like the reflog was truncated by the rename
operation, when in fact those entries are still there.

This patch does the absolute minimal fix, which is to look
back that one extra level and keep traversing.

The resulting behavior may not be the _best_ thing to do in
the long run (for example, we show both reflog entries each
with the same commit id), but it's a simple way to fix the
problem without risking further regressions.

Signed-off-by: Jeff King 
---
I do still think it would be worth looking into making this rename
create a single reflog entry, but that's largely orthogonal to making
the display code sane(r).

 reflog-walk.c |  2 ++
 t/t3200-branch.sh | 11 +++
 2 files changed, 13 insertions(+)

diff --git a/reflog-walk.c b/reflog-walk.c
index ed99437ad2..b7e489ad32 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
struct commit *commit)
/* a root commit, but there are still more entries to show */
reflog = _reflog->reflogs->items[commit_reflog->recno];
logobj = parse_object(>noid);
+   if (!logobj)
+   logobj = parse_object(>ooid);
}
 
if (!logobj || logobj->type != OBJ_COMMIT) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 48d152b9a9..dd37ac47c5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add 
entries to .git/logs/HEAD'
grep "^0\{40\}.*$msg$" .git/logs/HEAD
 '
 
+test_expect_success 'resulting reflog can be shown by log -g' '
+   oid=$(git rev-parse HEAD) &&
+   cat >expect <<-EOF &&
+   HEAD@{0} $oid $msg
+   HEAD@{1} $oid $msg
+   HEAD@{2} $oid checkout: moving from foo to baz
+   EOF
+   git log -g --format="%gd %H %gs" -3 HEAD >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked 
out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
-- 
2.13.2.892.g25f9b59978

[PATCH 0/6] fixing reflog-walk oddities

[Jeff King]

On Tue, Jul 04, 2017 at 05:24:08PM -0400, Jeff King wrote:

> On Tue, Jul 04, 2017 at 07:58:06PM +, brian m. carlson wrote:
> 
> > > And here's one more patch on top of those that's necessary to get the
> > > tests to pass (I don't expect anybody to necessarily be applying this
> > > slow string of patches; it's just to show the direction I'm looking in).
> > 
> > I've looked at your original patch, which modified reflog-walk.c, and it
> > does fix the issue.  I'm happy to send in a patch with that and a test
> > (provided you're okay with me adding your sign-off), or if you wanted to
> > send in something a bit more complete, like the series of patches here,
> > that's fine, too.
> 
> I've been on vacation for the past week, but wrapping this up is on my
> todo. I'll try to get to it tonight.

OK, so here's what I came up with.

The first patch is my original small fix with an extra test. I think
that would be appropriate for 'maint'. Its behavior still has some
quirks, but it avoids the confusion that you experienced and has a low
risk of breaking anything else.

The rest of it replaces the fake-parent thing with a more
straight-forward iteration over the reflogs (i.e., a cleanup of the
further patches I've been posting). After digging into it and especially
after writing the new tests, I think I've convinced myself that this is
the right way forward.

I tried to anticipate the behavior changes and I think all of them are
improvements. I won't be surprised if there's some hidden gotcha,
though, so this is definitely not for 'maint'. The patches do textually
depend on the fix from 1/6; my intent was that they'd be applied in
sequence and only merge up the first one to maint.

  [1/6]: reflog-walk: skip over double-null oid due to HEAD rename
  [2/6]: t1414: document some reflog-walk oddities
  [3/6]: log: do not free parents when walking reflog
  [4/6]: get_revision_1(): replace do-while with an early return
  [5/6]: rev-list: check reflog_info before showing usage
  [6/6]: reflog-walk: stop using fake parents

 builtin/log.c  |   4 +-
 builtin/rev-list.c |   3 +-
 reflog-walk.c  | 117 ++---
 reflog-walk.h  |   6 ++-
 revision.c |  39 ++---
 t/t1414-reflog-walk.sh |  86 
 t/t3200-branch.sh  |  10 +
 7 files changed, 160 insertions(+), 105 deletions(-)
 create mode 100755 t/t1414-reflog-walk.sh

-Peff

Re: [PATCH] push: add config option to --force-with-lease by default.

[Junio C Hamano]

On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli  wrote:
>
> Could you clarify the danger you're referring to? E.g. give an example
> of surprising --force-with-lease behavior that we do not want to
> encourage?

https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/

Re: What's cooking in git.git (Jun 2017, #09; Fri, 30)

[Philip Oakley]

From: "Junio C Hamano" 
Sent: Monday, July 03, 2017 9:19 PM

"Philip Oakley"  writes:


Am I right that the What's cooking  is prepared by a script?


Because I have to keep track of so many topics, its maintenance is
heavily helped by a script. I do not think it is sensible to expect
me to (or it would be good use of my time) correctly update the list
of commits manually every time a topic is replaced with its new
version.

Definately. I was hoping that a 'contents list' element (at the point of 
sending the emails) could also be part of the automated scripting.



But I consider the use of the script just like my use of Emacs to
edit the final end result.  Yes, I use tools to prepare it, and the
tools know certain rules that I prefer to apply to the document,
such as "a topic that has not been touched since the previous issue
by default does not need its description updated."

Does that answer your question?

I see the script's location is given in a follow up response. I'll see what 
opportunities for a TOC there may be within the flow, though my local todo 
list is getting a bit long with other personal matters.

--
Philip 

Re: [PATCH] push: add config option to --force-with-lease by default.

[Francesco Mazzoli]

On 4 July 2017 at 19:51, Junio C Hamano  wrote:
> People have been burned by the lazy "--force-with-lease" that does
> not say what object to expect there and forces the command to DWIM
> incorrectly what the remote's ref ought to be pointing at.  This
> change encourages its use without the user being painfully aware of
> that danger.  Whenever you say "push --force", you'd be using the
> dangerous "--force-with-lease" that does not specify what the
> expected current state of the remote is.  The end result gives an
> illusion of being safer than a simple "--force", without being
> not really safer.

Could you clarify the danger you're referring to? E.g. give an example
of surprising --force-with-lease behavior that we do not want to
encourage?

Thanks,
Francesco